* [PATCH] wcn36xx: dequeue all pending indicator messages
@ 2018-03-15 22:37 Daniel Mack
2018-03-16 9:09 ` Ramon Fried
2018-03-18 22:49 ` Bjorn Andersson
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Mack @ 2018-03-15 22:37 UTC (permalink / raw)
To: linux-wireless; +Cc: wcn36xx, kvalo, rfried, Daniel Mack
In case wcn36xx_smd_rsp_process() is called more than once before
hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
but wcn36xx_ind_smd_work() will only look at the first message in that
list.
Fix this by dequeing the messages from the list in a loop, and only stop
when it's empty.
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 95 +++++++++++++++++++---------------
1 file changed, 52 insertions(+), 43 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 7cc29285e052..a6b5352f59e9 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2409,54 +2409,63 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
{
struct wcn36xx *wcn =
container_of(work, struct wcn36xx, hal_ind_work);
- struct wcn36xx_hal_msg_header *msg_header;
- struct wcn36xx_hal_ind_msg *hal_ind_msg;
- unsigned long flags;
- spin_lock_irqsave(&wcn->hal_ind_lock, flags);
+ for (;;) {
+ struct wcn36xx_hal_msg_header *msg_header;
+ struct wcn36xx_hal_ind_msg *hal_ind_msg;
+ unsigned long flags;
- hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
- struct wcn36xx_hal_ind_msg,
- list);
- list_del(wcn->hal_ind_queue.next);
- spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+ spin_lock_irqsave(&wcn->hal_ind_lock, flags);
- msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
+ if (list_empty(&wcn->hal_ind_queue)) {
+ spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+ return;
+ }
- switch (msg_header->msg_type) {
- case WCN36XX_HAL_COEX_IND:
- case WCN36XX_HAL_DEL_BA_IND:
- case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
- break;
- case WCN36XX_HAL_OTA_TX_COMPL_IND:
- wcn36xx_smd_tx_compl_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_MISSED_BEACON_IND:
- wcn36xx_smd_missed_beacon_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- wcn36xx_smd_delete_sta_context_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_PRINT_REG_INFO_IND:
- wcn36xx_smd_print_reg_info_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_SCAN_OFFLOAD_IND:
- wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- default:
- wcn36xx_err("SMD_EVENT (%d) not supported\n",
- msg_header->msg_type);
+ hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
+ struct wcn36xx_hal_ind_msg,
+ list);
+ list_del(&hal_ind_msg->list);
+ spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+
+ msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
+
+ switch (msg_header->msg_type) {
+ case WCN36XX_HAL_COEX_IND:
+ case WCN36XX_HAL_DEL_BA_IND:
+ case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
+ break;
+ case WCN36XX_HAL_OTA_TX_COMPL_IND:
+ wcn36xx_smd_tx_compl_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_MISSED_BEACON_IND:
+ wcn36xx_smd_missed_beacon_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
+ wcn36xx_smd_delete_sta_context_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_PRINT_REG_INFO_IND:
+ wcn36xx_smd_print_reg_info_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_SCAN_OFFLOAD_IND:
+ wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ default:
+ wcn36xx_err("SMD_EVENT (%d) not supported\n",
+ msg_header->msg_type);
+ }
+
+ kfree(hal_ind_msg);
}
- kfree(hal_ind_msg);
}
int wcn36xx_smd_open(struct wcn36xx *wcn)
{
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] wcn36xx: dequeue all pending indicator messages
2018-03-15 22:37 [PATCH] wcn36xx: dequeue all pending indicator messages Daniel Mack
@ 2018-03-16 9:09 ` Ramon Fried
2018-03-16 9:28 ` Daniel Mack
2018-03-18 22:49 ` Bjorn Andersson
1 sibling, 1 reply; 6+ messages in thread
From: Ramon Fried @ 2018-03-16 9:09 UTC (permalink / raw)
To: Daniel Mack, linux-wireless; +Cc: wcn36xx, kvalo
On 3/16/2018 12:37 AM, Daniel Mack wrote:
> In case wcn36xx_smd_rsp_process() is called more than once before
> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
> but wcn36xx_ind_smd_work() will only look at the first message in that
> list.
>
> Fix this by dequeing the messages from the list in a loop, and only stop
> when it's empty.
Interesting. does it solve a specific bug ? can you elaborate ?
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 95 +++++++++++++++++++---------------
> 1 file changed, 52 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 7cc29285e052..a6b5352f59e9 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2409,54 +2409,63 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
> {
> struct wcn36xx *wcn =
> container_of(work, struct wcn36xx, hal_ind_work);
> - struct wcn36xx_hal_msg_header *msg_header;
> - struct wcn36xx_hal_ind_msg *hal_ind_msg;
> - unsigned long flags;
>
> - spin_lock_irqsave(&wcn->hal_ind_lock, flags);
> + for (;;) {
> + struct wcn36xx_hal_msg_header *msg_header;
> + struct wcn36xx_hal_ind_msg *hal_ind_msg;
> + unsigned long flags;
>
> - hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
> - struct wcn36xx_hal_ind_msg,
> - list);
> - list_del(wcn->hal_ind_queue.next);
> - spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> + spin_lock_irqsave(&wcn->hal_ind_lock, flags);
>
> - msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
> + if (list_empty(&wcn->hal_ind_queue)) {
> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> + return;
> + }
>
> - switch (msg_header->msg_type) {
> - case WCN36XX_HAL_COEX_IND:
> - case WCN36XX_HAL_DEL_BA_IND:
> - case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
> - break;
> - case WCN36XX_HAL_OTA_TX_COMPL_IND:
> - wcn36xx_smd_tx_compl_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_MISSED_BEACON_IND:
> - wcn36xx_smd_missed_beacon_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - wcn36xx_smd_delete_sta_context_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_PRINT_REG_INFO_IND:
> - wcn36xx_smd_print_reg_info_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_SCAN_OFFLOAD_IND:
> - wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - default:
> - wcn36xx_err("SMD_EVENT (%d) not supported\n",
> - msg_header->msg_type);
> + hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
> + struct wcn36xx_hal_ind_msg,
> + list);
> + list_del(&hal_ind_msg->list);
> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> +
> + msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
> +
> + switch (msg_header->msg_type) {
> + case WCN36XX_HAL_COEX_IND:
> + case WCN36XX_HAL_DEL_BA_IND:
> + case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
> + break;
> + case WCN36XX_HAL_OTA_TX_COMPL_IND:
> + wcn36xx_smd_tx_compl_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_MISSED_BEACON_IND:
> + wcn36xx_smd_missed_beacon_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> + wcn36xx_smd_delete_sta_context_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_PRINT_REG_INFO_IND:
> + wcn36xx_smd_print_reg_info_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_SCAN_OFFLOAD_IND:
> + wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + default:
> + wcn36xx_err("SMD_EVENT (%d) not supported\n",
> + msg_header->msg_type);
> + }
> +
> + kfree(hal_ind_msg);
> }
> - kfree(hal_ind_msg);
> }
> int wcn36xx_smd_open(struct wcn36xx *wcn)
> {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wcn36xx: dequeue all pending indicator messages
2018-03-16 9:09 ` Ramon Fried
@ 2018-03-16 9:28 ` Daniel Mack
2018-03-16 10:22 ` Kalle Valo
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Mack @ 2018-03-16 9:28 UTC (permalink / raw)
To: Ramon Fried, linux-wireless; +Cc: wcn36xx, kvalo
Hi,
On Friday, March 16, 2018 10:09 AM, Ramon Fried wrote:
> On 3/16/2018 12:37 AM, Daniel Mack wrote:
>> In case wcn36xx_smd_rsp_process() is called more than once before
>> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
>> but wcn36xx_ind_smd_work() will only look at the first message in that
>> list.
>>
>> Fix this by dequeing the messages from the list in a loop, and only stop
>> when it's empty.
> Interesting. does it solve a specific bug ? can you elaborate ?
I'm poking around in the driver to hopefully find issues that cause
instability and failures in joining networks, which I am seeing a lot.
There are a number of bug reports regarding this, for instance
https://bugs.96boards.org/show_bug.cgi?id=538
https://bugs.96boards.org/show_bug.cgi?id=319
I'm following your patches and also started to look into the driver
myself, and during review, I noticed that list handling issue. I have a
big fat warning locally that would tell me if the list ever contains
more than one entry, but that never happens, as the indicator messages
are way too infrequent to trigger the race. So this isn't a real-world
issue as far as I can tell, but it is still quite obviously a bug. Hence
I considered posting a patch.
I have another one that handles missed TX ACKs from the firmware (TODO
comment in wcn36xx_start_tx()), but that case doesn't happen either.
So I'll keep looking further, and will send things if I spot something.
In the meantime, as you seem to be familiar with the firmware internals,
I'd appreciate any pointer in how to narrow this down. It's extremely
important to us, as we want to release a product featuring the wcn36xx.
Thanks a lot for all your efforts!
Daniel
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>> drivers/net/wireless/ath/wcn36xx/smd.c | 95 +++++++++++++++++++---------------
>> 1 file changed, 52 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
>> index 7cc29285e052..a6b5352f59e9 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
>> @@ -2409,54 +2409,63 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
>> {
>> struct wcn36xx *wcn =
>> container_of(work, struct wcn36xx, hal_ind_work);
>> - struct wcn36xx_hal_msg_header *msg_header;
>> - struct wcn36xx_hal_ind_msg *hal_ind_msg;
>> - unsigned long flags;
>>
>> - spin_lock_irqsave(&wcn->hal_ind_lock, flags);
>> + for (;;) {
>> + struct wcn36xx_hal_msg_header *msg_header;
>> + struct wcn36xx_hal_ind_msg *hal_ind_msg;
>> + unsigned long flags;
>>
>> - hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
>> - struct wcn36xx_hal_ind_msg,
>> - list);
>> - list_del(wcn->hal_ind_queue.next);
>> - spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
>> + spin_lock_irqsave(&wcn->hal_ind_lock, flags);
>>
>> - msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
>> + if (list_empty(&wcn->hal_ind_queue)) {
>> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
>> + return;
>> + }
>>
>> - switch (msg_header->msg_type) {
>> - case WCN36XX_HAL_COEX_IND:
>> - case WCN36XX_HAL_DEL_BA_IND:
>> - case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
>> - break;
>> - case WCN36XX_HAL_OTA_TX_COMPL_IND:
>> - wcn36xx_smd_tx_compl_ind(wcn,
>> - hal_ind_msg->msg,
>> - hal_ind_msg->msg_len);
>> - break;
>> - case WCN36XX_HAL_MISSED_BEACON_IND:
>> - wcn36xx_smd_missed_beacon_ind(wcn,
>> - hal_ind_msg->msg,
>> - hal_ind_msg->msg_len);
>> - break;
>> - case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
>> - wcn36xx_smd_delete_sta_context_ind(wcn,
>> - hal_ind_msg->msg,
>> - hal_ind_msg->msg_len);
>> - break;
>> - case WCN36XX_HAL_PRINT_REG_INFO_IND:
>> - wcn36xx_smd_print_reg_info_ind(wcn,
>> - hal_ind_msg->msg,
>> - hal_ind_msg->msg_len);
>> - break;
>> - case WCN36XX_HAL_SCAN_OFFLOAD_IND:
>> - wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
>> - hal_ind_msg->msg_len);
>> - break;
>> - default:
>> - wcn36xx_err("SMD_EVENT (%d) not supported\n",
>> - msg_header->msg_type);
>> + hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
>> + struct wcn36xx_hal_ind_msg,
>> + list);
>> + list_del(&hal_ind_msg->list);
>> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
>> +
>> + msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
>> +
>> + switch (msg_header->msg_type) {
>> + case WCN36XX_HAL_COEX_IND:
>> + case WCN36XX_HAL_DEL_BA_IND:
>> + case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
>> + break;
>> + case WCN36XX_HAL_OTA_TX_COMPL_IND:
>> + wcn36xx_smd_tx_compl_ind(wcn,
>> + hal_ind_msg->msg,
>> + hal_ind_msg->msg_len);
>> + break;
>> + case WCN36XX_HAL_MISSED_BEACON_IND:
>> + wcn36xx_smd_missed_beacon_ind(wcn,
>> + hal_ind_msg->msg,
>> + hal_ind_msg->msg_len);
>> + break;
>> + case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
>> + wcn36xx_smd_delete_sta_context_ind(wcn,
>> + hal_ind_msg->msg,
>> + hal_ind_msg->msg_len);
>> + break;
>> + case WCN36XX_HAL_PRINT_REG_INFO_IND:
>> + wcn36xx_smd_print_reg_info_ind(wcn,
>> + hal_ind_msg->msg,
>> + hal_ind_msg->msg_len);
>> + break;
>> + case WCN36XX_HAL_SCAN_OFFLOAD_IND:
>> + wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
>> + hal_ind_msg->msg_len);
>> + break;
>> + default:
>> + wcn36xx_err("SMD_EVENT (%d) not supported\n",
>> + msg_header->msg_type);
>> + }
>> +
>> + kfree(hal_ind_msg);
>> }
>> - kfree(hal_ind_msg);
>> }
>> int wcn36xx_smd_open(struct wcn36xx *wcn)
>> {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wcn36xx: dequeue all pending indicator messages
2018-03-16 9:28 ` Daniel Mack
@ 2018-03-16 10:22 ` Kalle Valo
2018-03-16 11:38 ` Daniel Mack
0 siblings, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2018-03-16 10:22 UTC (permalink / raw)
To: Daniel Mack; +Cc: Ramon Fried, linux-wireless, wcn36xx
Daniel Mack <daniel@zonque.org> writes:
> Hi,
>
> On Friday, March 16, 2018 10:09 AM, Ramon Fried wrote:
>> On 3/16/2018 12:37 AM, Daniel Mack wrote:
>>> In case wcn36xx_smd_rsp_process() is called more than once before
>>> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
>>> but wcn36xx_ind_smd_work() will only look at the first message in that
>>> list.
>>>
>>> Fix this by dequeing the messages from the list in a loop, and only stop
>>> when it's empty.
>> Interesting. does it solve a specific bug ? can you elaborate ?
>
> I'm poking around in the driver to hopefully find issues that cause
> instability and failures in joining networks, which I am seeing a lot.
> There are a number of bug reports regarding this, for instance
>
> https://bugs.96boards.org/show_bug.cgi?id=538
> https://bugs.96boards.org/show_bug.cgi?id=319
>
> I'm following your patches and also started to look into the driver
> myself, and during review, I noticed that list handling issue. I have a
> big fat warning locally that would tell me if the list ever contains
> more than one entry, but that never happens, as the indicator messages
> are way too infrequent to trigger the race. So this isn't a real-world
> issue as far as I can tell, but it is still quite obviously a bug. Hence
> I considered posting a patch.
It's a good idea to mention in the commit log if the fix is for a
theoretical issue and does not necessarily fix anything visible. Helps
to understand the background, prioritise which release the fix should go
etc.
--
Kalle Valo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wcn36xx: dequeue all pending indicator messages
2018-03-16 10:22 ` Kalle Valo
@ 2018-03-16 11:38 ` Daniel Mack
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Mack @ 2018-03-16 11:38 UTC (permalink / raw)
To: Kalle Valo; +Cc: wcn36xx, Ramon Fried, linux-wireless
On Friday, March 16, 2018 11:22 AM, Kalle Valo wrote:
> Daniel Mack <daniel@zonque.org> writes:
>
>> Hi,
>>
>> On Friday, March 16, 2018 10:09 AM, Ramon Fried wrote:
>>> On 3/16/2018 12:37 AM, Daniel Mack wrote:
>>>> In case wcn36xx_smd_rsp_process() is called more than once before
>>>> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
>>>> but wcn36xx_ind_smd_work() will only look at the first message in that
>>>> list.
>>>>
>>>> Fix this by dequeing the messages from the list in a loop, and only stop
>>>> when it's empty.
>>> Interesting. does it solve a specific bug ? can you elaborate ?
>>
>> I'm poking around in the driver to hopefully find issues that cause
>> instability and failures in joining networks, which I am seeing a lot.
>> There are a number of bug reports regarding this, for instance
>>
>> https://bugs.96boards.org/show_bug.cgi?id=538
>> https://bugs.96boards.org/show_bug.cgi?id=319
>>
>> I'm following your patches and also started to look into the driver
>> myself, and during review, I noticed that list handling issue. I have a
>> big fat warning locally that would tell me if the list ever contains
>> more than one entry, but that never happens, as the indicator messages
>> are way too infrequent to trigger the race. So this isn't a real-world
>> issue as far as I can tell, but it is still quite obviously a bug. Hence
>> I considered posting a patch.
>
> It's a good idea to mention in the commit log if the fix is for a
> theoretical issue and does not necessarily fix anything visible. Helps
> to understand the background, prioritise which release the fix should go
> etc.
>
Well, it's a race which can (theoretically) happen anytime.
But sure, I will add another sentence to the commit log and resend.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wcn36xx: dequeue all pending indicator messages
2018-03-15 22:37 [PATCH] wcn36xx: dequeue all pending indicator messages Daniel Mack
2018-03-16 9:09 ` Ramon Fried
@ 2018-03-18 22:49 ` Bjorn Andersson
1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2018-03-18 22:49 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-wireless, wcn36xx, rfried, kvalo
On Thu 15 Mar 15:37 PDT 2018, Daniel Mack wrote:
> In case wcn36xx_smd_rsp_process() is called more than once before
> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
> but wcn36xx_ind_smd_work() will only look at the first message in that
> list.
>
> Fix this by dequeing the messages from the list in a loop, and only stop
> when it's empty.
>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Thanks for fixing this, I thought I already had done that.
Regards,
Bjorn
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 95 +++++++++++++++++++---------------
> 1 file changed, 52 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 7cc29285e052..a6b5352f59e9 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2409,54 +2409,63 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
> {
> struct wcn36xx *wcn =
> container_of(work, struct wcn36xx, hal_ind_work);
> - struct wcn36xx_hal_msg_header *msg_header;
> - struct wcn36xx_hal_ind_msg *hal_ind_msg;
> - unsigned long flags;
>
> - spin_lock_irqsave(&wcn->hal_ind_lock, flags);
> + for (;;) {
> + struct wcn36xx_hal_msg_header *msg_header;
> + struct wcn36xx_hal_ind_msg *hal_ind_msg;
> + unsigned long flags;
>
> - hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
> - struct wcn36xx_hal_ind_msg,
> - list);
> - list_del(wcn->hal_ind_queue.next);
> - spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> + spin_lock_irqsave(&wcn->hal_ind_lock, flags);
>
> - msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
> + if (list_empty(&wcn->hal_ind_queue)) {
> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> + return;
> + }
>
> - switch (msg_header->msg_type) {
> - case WCN36XX_HAL_COEX_IND:
> - case WCN36XX_HAL_DEL_BA_IND:
> - case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
> - break;
> - case WCN36XX_HAL_OTA_TX_COMPL_IND:
> - wcn36xx_smd_tx_compl_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_MISSED_BEACON_IND:
> - wcn36xx_smd_missed_beacon_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - wcn36xx_smd_delete_sta_context_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_PRINT_REG_INFO_IND:
> - wcn36xx_smd_print_reg_info_ind(wcn,
> - hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - case WCN36XX_HAL_SCAN_OFFLOAD_IND:
> - wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
> - hal_ind_msg->msg_len);
> - break;
> - default:
> - wcn36xx_err("SMD_EVENT (%d) not supported\n",
> - msg_header->msg_type);
> + hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
> + struct wcn36xx_hal_ind_msg,
> + list);
> + list_del(&hal_ind_msg->list);
> + spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
> +
> + msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
> +
> + switch (msg_header->msg_type) {
> + case WCN36XX_HAL_COEX_IND:
> + case WCN36XX_HAL_DEL_BA_IND:
> + case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
> + break;
> + case WCN36XX_HAL_OTA_TX_COMPL_IND:
> + wcn36xx_smd_tx_compl_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_MISSED_BEACON_IND:
> + wcn36xx_smd_missed_beacon_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> + wcn36xx_smd_delete_sta_context_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_PRINT_REG_INFO_IND:
> + wcn36xx_smd_print_reg_info_ind(wcn,
> + hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + case WCN36XX_HAL_SCAN_OFFLOAD_IND:
> + wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
> + hal_ind_msg->msg_len);
> + break;
> + default:
> + wcn36xx_err("SMD_EVENT (%d) not supported\n",
> + msg_header->msg_type);
> + }
> +
> + kfree(hal_ind_msg);
> }
> - kfree(hal_ind_msg);
> }
> int wcn36xx_smd_open(struct wcn36xx *wcn)
> {
> --
> 2.14.3
>
>
> _______________________________________________
> wcn36xx mailing list
> wcn36xx@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/wcn36xx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-18 22:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-15 22:37 [PATCH] wcn36xx: dequeue all pending indicator messages Daniel Mack
2018-03-16 9:09 ` Ramon Fried
2018-03-16 9:28 ` Daniel Mack
2018-03-16 10:22 ` Kalle Valo
2018-03-16 11:38 ` Daniel Mack
2018-03-18 22:49 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox