LinuxLists.cc - [PATCH 0/4] ath10k: cleanups

2013-08-08 08:14:16

Subject: [PATCH 0/4] ath10k: cleanups

Hi,

This patchset contains a few non-functional clean
ups.

Michal Kazior (4):
ath10k: clean up monitor start code
ath10k: use sizeof(*var) in kmalloc
ath10k: clean up PCI completion states
ath10k: print errcode when CE ring setup fails

drivers/net/wireless/ath/ath10k/ce.c | 15 +++++++++------
drivers/net/wireless/ath/ath10k/mac.c | 3 ---
drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++++--------
drivers/net/wireless/ath/ath10k/pci.h | 13 +++++++------
4 files changed, 32 insertions(+), 23 deletions(-)

--
1.7.9.5

2013-08-08 08:14:20

by Michal Kazior

[permalink] [raw]

Subject: [PATCH 3/4] ath10k: clean up PCI completion states

Improve code readability by using enum and a
switch-case.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 21 +++++++++++++++------
drivers/net/wireless/ath/ath10k/pci.h | 13 +++++++------
2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 1814af1..321cc88 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -538,7 +538,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
if (!compl)
break;

- compl->send_or_recv = HIF_CE_COMPLETE_SEND;
+ compl->state = ATH10K_PCI_COMPL_SEND;
compl->ce_state = ce_state;
compl->pipe_info = pipe_info;
compl->transfer_context = transfer_context;
@@ -588,7 +588,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state,
if (!compl)
break;

- compl->send_or_recv = HIF_CE_COMPLETE_RECV;
+ compl->state = ATH10K_PCI_COMPL_RECV;
compl->ce_state = ce_state;
compl->pipe_info = pipe_info;
compl->transfer_context = transfer_context;
@@ -810,7 +810,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
return -ENOMEM;
}

- compl->send_or_recv = HIF_CE_COMPLETE_FREE;
+ compl->state = ATH10K_PCI_COMPL_FREE;
list_add_tail(&compl->list, &pipe_info->compl_free);
}
}
@@ -909,12 +909,14 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
list_del(&compl->list);
spin_unlock_bh(&ar_pci->compl_lock);

- if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) {
+ switch (compl->state) {
+ case ATH10K_PCI_COMPL_SEND:
cb->tx_completion(ar,
compl->transfer_context,
compl->transfer_id);
send_done = 1;
- } else {
+ break;
+ case ATH10K_PCI_COMPL_RECV:
ret = ath10k_pci_post_rx_pipe(compl->pipe_info, 1);
if (ret) {
ath10k_warn("Unable to post recv buffer for pipe: %d\n",
@@ -941,9 +943,16 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
nbytes,
skb->len + skb_tailroom(skb));
}
+ break;
+ case ATH10K_PCI_COMPL_FREE:
+ ath10k_warn("free completion cannot be processed\n");
+ break;
+ default:
+ ath10k_warn("invalid completion state (%d)\n", compl->state);
+ break;
}

- compl->send_or_recv = HIF_CE_COMPLETE_FREE;
+ compl->state = ATH10K_PCI_COMPL_FREE;

/*
* Add completion back to the pipe's free list.
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index d3a2e6c..1908d61 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -43,9 +43,15 @@ struct bmi_xfer {
u32 resp_len;
};

+enum ath10k_pci_compl_state {
+ ATH10K_PCI_COMPL_FREE = 0,
+ ATH10K_PCI_COMPL_SEND,
+ ATH10K_PCI_COMPL_RECV,
+};
+
struct ath10k_pci_compl {
struct list_head list;
- int send_or_recv;
+ enum ath10k_pci_compl_state state;
struct ce_state *ce_state;
struct hif_ce_pipe_info *pipe_info;
void *transfer_context;
@@ -54,11 +60,6 @@ struct ath10k_pci_compl {
unsigned int flags;
};

-/* compl_state.send_or_recv */
-#define HIF_CE_COMPLETE_FREE 0
-#define HIF_CE_COMPLETE_SEND 1
-#define HIF_CE_COMPLETE_RECV 2
-
/*
* PCI-specific Target state
*
--
1.7.9.5

2013-08-12 14:26:43

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 2/4] ath10k: use sizeof(*var) in kmalloc

Michal Kazior <[email protected]> writes:

> This fixes checkpatch warning from the latest
> 3.11-rc kernel tree.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/pci.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index d95439b..1814af1 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -803,8 +803,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
> continue;
>
> for (i = 0; i < completions; i++) {
> - compl = kmalloc(sizeof(struct ath10k_pci_compl),
> - GFP_KERNEL);
> + compl = kmalloc(sizeof(*compl), GFP_KERNEL);
> if (!compl) {
> ath10k_warn("No memory for completion state\n");
> ath10k_pci_stop_ce(ar);

Just out of curiosity, what's the warning? kmalloc() should be called
with sizeof(*foo) style?

--
Kalle Valo

2013-08-13 05:12:59

by Michal Kazior

[permalink] [raw]

Subject: Re: [PATCH 2/4] ath10k: use sizeof(*var) in kmalloc

On 12 August 2013 16:26, Kalle Valo <[email protected]> wrote:
> Michal Kazior <[email protected]> writes:
>
>> This fixes checkpatch warning from the latest
>> 3.11-rc kernel tree.
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/pci.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index d95439b..1814af1 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -803,8 +803,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
>> continue;
>>
>> for (i = 0; i < completions; i++) {
>> - compl = kmalloc(sizeof(struct ath10k_pci_compl),
>> - GFP_KERNEL);
>> + compl = kmalloc(sizeof(*compl), GFP_KERNEL);
>> if (!compl) {
>> ath10k_warn("No memory for completion state\n");
>> ath10k_pci_stop_ce(ar);
>
> Just out of curiosity, what's the warning? kmalloc() should be called
> with sizeof(*foo) style?

Checkpatch prints:

CHECK: Prefer kmalloc(sizeof(*compl)...) over kmalloc(sizeof(struct
ath10k_pci_compl)...)

Pozdrawiam / Best regards,
Micha? Kazior.

2013-08-08 08:14:17

by Michal Kazior

[permalink] [raw]

Subject: [PATCH 1/4] ath10k: clean up monitor start code

Remove useless code that was causing WARN_ON when
a 80MHz+ vif entered promiscuous mode or monitor
interface was started.

The channel mode is already computed by
chan_to_phymode().

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index cf2ba4d..05f5f76 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -503,13 +503,10 @@ static int ath10k_monitor_start(struct ath10k *ar, int vdev_id)
{
struct ieee80211_channel *channel = ar->hw->conf.chandef.chan;
struct wmi_vdev_start_request_arg arg = {};
- enum nl80211_channel_type type;
int ret = 0;

lockdep_assert_held(&ar->conf_mutex);

- type = cfg80211_get_chandef_type(&ar->hw->conf.chandef);
-
arg.vdev_id = vdev_id;
arg.channel.freq = channel->center_freq;
arg.channel.band_center_freq1 = ar->hw->conf.chandef.center_freq1;
--
1.7.9.5

2013-08-13 05:55:06

by Michal Kazior

[permalink] [raw]

Subject: [PATCH v2 2/4] ath10k: use sizeof(*var) in kmalloc

This fixes checkpatch warning from the latest
3.11-rc kernel tree.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e2f9ef5..98edb31 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -805,8 +805,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
continue;

for (i = 0; i < completions; i++) {
- compl = kmalloc(sizeof(struct ath10k_pci_compl),
- GFP_KERNEL);
+ compl = kmalloc(sizeof(*compl), GFP_KERNEL);
if (!compl) {
ath10k_warn("No memory for completion state\n");
ath10k_pci_stop_ce(ar);
--
1.7.9.5

2013-08-13 05:55:06

by Michal Kazior

[permalink] [raw]

Subject: [PATCH v2 4/4] ath10k: print errcode when CE ring setup fails

This makes it possible to see the reason why the
setup fails. It also adheres to code style of
error checking in ath drivers.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/ce.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index f8b969f..50d2ea2 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -1128,6 +1128,7 @@ struct ce_state *ath10k_ce_init(struct ath10k *ar,
{
struct ce_state *ce_state;
u32 ctrl_addr = ath10k_ce_base_address(ce_id);
+ int ret;

ce_state = ath10k_ce_init_state(ar, ce_id, attr);
if (!ce_state) {
@@ -1136,18 +1137,20 @@ struct ce_state *ath10k_ce_init(struct ath10k *ar,
}

if (attr->src_nentries) {
- if (ath10k_ce_init_src_ring(ar, ce_id, ce_state, attr)) {
- ath10k_err("Failed to initialize CE src ring for ID: %d\n",
- ce_id);
+ ret = ath10k_ce_init_src_ring(ar, ce_id, ce_state, attr);
+ if (ret) {
+ ath10k_err("Failed to initialize CE src ring for ID: %d (%d)\n",
+ ce_id, ret);
ath10k_ce_deinit(ce_state);
return NULL;
}
}

if (attr->dest_nentries) {
- if (ath10k_ce_init_dest_ring(ar, ce_id, ce_state, attr)) {
- ath10k_err("Failed to initialize CE dest ring for ID: %d\n",
- ce_id);
+ ret = ath10k_ce_init_dest_ring(ar, ce_id, ce_state, attr);
+ if (ret) {
+ ath10k_err("Failed to initialize CE dest ring for ID: %d (%d)\n",
+ ce_id, ret);
ath10k_ce_deinit(ce_state);
return NULL;
}
--
1.7.9.5

2013-08-14 15:01:08

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH v2 0/4] ath10k: cleanups

Michal Kazior <[email protected]> writes:

> Hi,
>
> This patchset contains a few non-functional clean
> ups.
>
> v2:
> * fix line over 80 chars
>
>
> Michal Kazior (4):
> ath10k: clean up monitor start code
> ath10k: use sizeof(*var) in kmalloc
> ath10k: clean up PCI completion states
> ath10k: print errcode when CE ring setup fails

Thanks, all four applied.

--
Kalle Valo

2013-08-13 05:55:05

by Michal Kazior

[permalink] [raw]

Subject: [PATCH v2 1/4] ath10k: clean up monitor start code

2013-08-13 05:55:05

by Michal Kazior

[permalink] [raw]

Subject: [PATCH v2 0/4] ath10k: cleanups

Hi,

This patchset contains a few non-functional clean
ups.

v2:
* fix line over 80 chars

Michal Kazior (4):
ath10k: clean up monitor start code
ath10k: use sizeof(*var) in kmalloc
ath10k: clean up PCI completion states
ath10k: print errcode when CE ring setup fails

drivers/net/wireless/ath/ath10k/ce.c | 15 +++++++++------
drivers/net/wireless/ath/ath10k/mac.c | 3 ---
drivers/net/wireless/ath/ath10k/pci.c | 25 +++++++++++++++++--------
drivers/net/wireless/ath/ath10k/pci.h | 13 +++++++------
4 files changed, 33 insertions(+), 23 deletions(-)

--
1.7.9.5

2013-08-12 14:47:39

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 0/4] ath10k: cleanups

Kalle Valo <[email protected]> writes:

> Michal Kazior <[email protected]> writes:
>
>> This patchset contains a few non-functional clean
>> ups.
>>
>> Michal Kazior (4):
>> ath10k: clean up monitor start code
>> ath10k: use sizeof(*var) in kmalloc
>> ath10k: clean up PCI completion states
>> ath10k: print errcode when CE ring setup fails
>
> Thanks, all four applied.

Sorry, I was too fast and need to take that back. I now see this
warning:

drivers/net/wireless/ath/ath10k/pci.c:951: WARNING: line over 80 characters

I dropped the patches, please send v2.

--
Kalle Valo

2013-08-08 08:14:18

by Michal Kazior

[permalink] [raw]

Subject: [PATCH 2/4] ath10k: use sizeof(*var) in kmalloc

This fixes checkpatch warning from the latest
3.11-rc kernel tree.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index d95439b..1814af1 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -803,8 +803,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
continue;

for (i = 0; i < completions; i++) {
- compl = kmalloc(sizeof(struct ath10k_pci_compl),
- GFP_KERNEL);
+ compl = kmalloc(sizeof(*compl), GFP_KERNEL);
if (!compl) {
ath10k_warn("No memory for completion state\n");
ath10k_pci_stop_ce(ar);
--
1.7.9.5

2013-08-08 08:14:23

by Michal Kazior

[permalink] [raw]

Subject: [PATCH 4/4] ath10k: print errcode when CE ring setup fails

2013-08-12 14:30:28

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 0/4] ath10k: cleanups

Michal Kazior <[email protected]> writes:

> This patchset contains a few non-functional clean
> ups.
>
> Michal Kazior (4):
> ath10k: clean up monitor start code
> ath10k: use sizeof(*var) in kmalloc
> ath10k: clean up PCI completion states
> ath10k: print errcode when CE ring setup fails

Thanks, all four applied.

--
Kalle Valo

2013-08-13 05:55:07

by Michal Kazior

[permalink] [raw]

Subject: [PATCH v2 3/4] ath10k: clean up PCI completion states

Improve code readability by using enum and a
switch-case.

Signed-off-by: Michal Kazior <[email protected]>
---
v2;
* fix line over 80 chars

drivers/net/wireless/ath/ath10k/pci.c | 22 ++++++++++++++++------
drivers/net/wireless/ath/ath10k/pci.h | 13 +++++++------
2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 98edb31..630ba59 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -540,7 +540,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
if (!compl)
break;

- compl->send_or_recv = HIF_CE_COMPLETE_SEND;
+ compl->state = ATH10K_PCI_COMPL_SEND;
compl->ce_state = ce_state;
compl->pipe_info = pipe_info;
compl->transfer_context = transfer_context;
@@ -590,7 +590,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state,
if (!compl)
break;

- compl->send_or_recv = HIF_CE_COMPLETE_RECV;
+ compl->state = ATH10K_PCI_COMPL_RECV;
compl->ce_state = ce_state;
compl->pipe_info = pipe_info;
compl->transfer_context = transfer_context;
@@ -812,7 +812,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
return -ENOMEM;
}

- compl->send_or_recv = HIF_CE_COMPLETE_FREE;
+ compl->state = ATH10K_PCI_COMPL_FREE;
list_add_tail(&compl->list, &pipe_info->compl_free);
}
}
@@ -911,12 +911,14 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
list_del(&compl->list);
spin_unlock_bh(&ar_pci->compl_lock);

- if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) {
+ switch (compl->state) {
+ case ATH10K_PCI_COMPL_SEND:
cb->tx_completion(ar,
compl->transfer_context,
compl->transfer_id);
send_done = 1;
- } else {
+ break;
+ case ATH10K_PCI_COMPL_RECV:
ret = ath10k_pci_post_rx_pipe(compl->pipe_info, 1);
if (ret) {
ath10k_warn("Unable to post recv buffer for pipe: %d\n",
@@ -943,9 +945,17 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
nbytes,
skb->len + skb_tailroom(skb));
}
+ break;
+ case ATH10K_PCI_COMPL_FREE:
+ ath10k_warn("free completion cannot be processed\n");
+ break;
+ default:
+ ath10k_warn("invalid completion state (%d)\n",
+ compl->state);
+ break;
}

- compl->send_or_recv = HIF_CE_COMPLETE_FREE;
+ compl->state = ATH10K_PCI_COMPL_FREE;

/*
* Add completion back to the pipe's free list.
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 871bb33..3093733 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -43,9 +43,15 @@ struct bmi_xfer {
u32 resp_len;
};

+enum ath10k_pci_compl_state {
+ ATH10K_PCI_COMPL_FREE = 0,
+ ATH10K_PCI_COMPL_SEND,
+ ATH10K_PCI_COMPL_RECV,
+};
+
struct ath10k_pci_compl {
struct list_head list;
- int send_or_recv;
+ enum ath10k_pci_compl_state state;
struct ce_state *ce_state;
struct hif_ce_pipe_info *pipe_info;
void *transfer_context;
@@ -54,11 +60,6 @@ struct ath10k_pci_compl {
unsigned int flags;
};

-/* compl_state.send_or_recv */
-#define HIF_CE_COMPLETE_FREE 0
-#define HIF_CE_COMPLETE_SEND 1
-#define HIF_CE_COMPLETE_RECV 2
-
/*
* PCI-specific Target state
*
--
1.7.9.5