All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ath10k:  Fix crash due to tasklet race.
@ 2013-10-29 21:38 greearb
  2013-10-30  9:03 ` Michal Kazior
  2013-11-06 13:06 ` Kalle Valo
  0 siblings, 2 replies; 6+ messages in thread
From: greearb @ 2013-10-29 21:38 UTC (permalink / raw)
  To: ath10k; +Cc: kvalo, Ben Greear

From: Ben Greear <greearb@candelatech.com>

The tasklet can run after the rings have been cleaned up,
so check for NULL before de-referencing the ring.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/ce.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index d243f28..dfcfda9 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -547,12 +547,18 @@ static int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
 	struct ath10k_ce_ring *src_ring = ce_state->src_ring;
 	u32 ctrl_addr = ce_state->ctrl_addr;
 	struct ath10k *ar = ce_state->ar;
-	unsigned int nentries_mask = src_ring->nentries_mask;
-	unsigned int sw_index = src_ring->sw_index;
+	unsigned int nentries_mask;
+	unsigned int sw_index;
 	struct ce_desc *sdesc, *sbase;
 	unsigned int read_index;
 	int ret;
 
+	if (!src_ring)
+		return -EIO;
+
+	nentries_mask = src_ring->nentries_mask;
+	sw_index = src_ring->sw_index;
+
 	if (src_ring->hw_index == sw_index) {
 		/*
 		 * The SW completion index has caught up with the cached
-- 
1.7.11.7


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: Fix crash due to tasklet race.
  2013-10-29 21:38 [PATCH] ath10k: Fix crash due to tasklet race greearb
@ 2013-10-30  9:03 ` Michal Kazior
  2013-11-06 11:50   ` Kalle Valo
  2013-11-06 13:06 ` Kalle Valo
  1 sibling, 1 reply; 6+ messages in thread
From: Michal Kazior @ 2013-10-30  9:03 UTC (permalink / raw)
  To: Ben Greear; +Cc: Kalle Valo, ath10k

On 29 October 2013 22:38,  <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> The tasklet can run after the rings have been cleaned up,
> so check for NULL before de-referencing the ring.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/ce.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
> index d243f28..dfcfda9 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -547,12 +547,18 @@ static int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
>         struct ath10k_ce_ring *src_ring = ce_state->src_ring;
>         u32 ctrl_addr = ce_state->ctrl_addr;
>         struct ath10k *ar = ce_state->ar;
> -       unsigned int nentries_mask = src_ring->nentries_mask;
> -       unsigned int sw_index = src_ring->sw_index;
> +       unsigned int nentries_mask;
> +       unsigned int sw_index;
>         struct ce_desc *sdesc, *sbase;
>         unsigned int read_index;
>         int ret;
>
> +       if (!src_ring)
> +               return -EIO;
> +
> +       nentries_mask = src_ring->nentries_mask;
> +       sw_index = src_ring->sw_index;
> +
>         if (src_ring->hw_index == sw_index) {
>                 /*
>                  * The SW completion index has caught up with the cached

I don't think this is a proper way to fix the problem. What should be
done is initialization clean up and some reordering to prevent this
from happening in the first place.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: Fix crash due to tasklet race.
  2013-10-30  9:03 ` Michal Kazior
@ 2013-11-06 11:50   ` Kalle Valo
  2013-11-06 11:54     ` Michal Kazior
  0 siblings, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2013-11-06 11:50 UTC (permalink / raw)
  To: Michal Kazior; +Cc: Ben Greear, ath10k

Michal Kazior <michal.kazior@tieto.com> writes:

> On 29 October 2013 22:38,  <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> The tasklet can run after the rings have been cleaned up,
>> so check for NULL before de-referencing the ring.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/ce.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
>> index d243f28..dfcfda9 100644
>> --- a/drivers/net/wireless/ath/ath10k/ce.c
>> +++ b/drivers/net/wireless/ath/ath10k/ce.c
>> @@ -547,12 +547,18 @@ static int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
>>         struct ath10k_ce_ring *src_ring = ce_state->src_ring;
>>         u32 ctrl_addr = ce_state->ctrl_addr;
>>         struct ath10k *ar = ce_state->ar;
>> -       unsigned int nentries_mask = src_ring->nentries_mask;
>> -       unsigned int sw_index = src_ring->sw_index;
>> +       unsigned int nentries_mask;
>> +       unsigned int sw_index;
>>         struct ce_desc *sdesc, *sbase;
>>         unsigned int read_index;
>>         int ret;
>>
>> +       if (!src_ring)
>> +               return -EIO;
>
> I don't think this is a proper way to fix the problem. What should be
> done is initialization clean up and some reordering to prevent this
> from happening in the first place.

I agree with Michal, but as we don't have any better fix for this issue
I'm inclined to take the patch anyway. Maybe there just should be a
comment stating that it's an ugly workaround and a WARN_ON() to make
sure that we properly fix the interrupt initialisation.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: Fix crash due to tasklet race.
  2013-11-06 11:50   ` Kalle Valo
@ 2013-11-06 11:54     ` Michal Kazior
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Kazior @ 2013-11-06 11:54 UTC (permalink / raw)
  To: Kalle Valo; +Cc: Ben Greear, ath10k

On 6 November 2013 12:50, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> On 29 October 2013 22:38,  <greearb@candelatech.com> wrote:
>>> From: Ben Greear <greearb@candelatech.com>
>>>
>>> The tasklet can run after the rings have been cleaned up,
>>> so check for NULL before de-referencing the ring.
>>>
>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>> ---
>>>  drivers/net/wireless/ath/ath10k/ce.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
>>> index d243f28..dfcfda9 100644
>>> --- a/drivers/net/wireless/ath/ath10k/ce.c
>>> +++ b/drivers/net/wireless/ath/ath10k/ce.c
>>> @@ -547,12 +547,18 @@ static int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
>>>         struct ath10k_ce_ring *src_ring = ce_state->src_ring;
>>>         u32 ctrl_addr = ce_state->ctrl_addr;
>>>         struct ath10k *ar = ce_state->ar;
>>> -       unsigned int nentries_mask = src_ring->nentries_mask;
>>> -       unsigned int sw_index = src_ring->sw_index;
>>> +       unsigned int nentries_mask;
>>> +       unsigned int sw_index;
>>>         struct ce_desc *sdesc, *sbase;
>>>         unsigned int read_index;
>>>         int ret;
>>>
>>> +       if (!src_ring)
>>> +               return -EIO;
>>
>> I don't think this is a proper way to fix the problem. What should be
>> done is initialization clean up and some reordering to prevent this
>> from happening in the first place.
>
> I agree with Michal, but as we don't have any better fix for this issue
> I'm inclined to take the patch anyway. Maybe there just should be a
> comment stating that it's an ugly workaround and a WARN_ON() to make
> sure that we properly fix the interrupt initialisation.

This issue alone should actually be fixed in my pci fixes RFT.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k:  Fix crash due to tasklet race.
  2013-10-29 21:38 [PATCH] ath10k: Fix crash due to tasklet race greearb
  2013-10-30  9:03 ` Michal Kazior
@ 2013-11-06 13:06 ` Kalle Valo
  2013-11-06 17:00   ` Ben Greear
  1 sibling, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2013-11-06 13:06 UTC (permalink / raw)
  To: greearb; +Cc: ath10k

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> The tasklet can run after the rings have been cleaned up,
> so check for NULL before de-referencing the ring.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

I'll drop this as Michal's patch has a proper fix for this issue.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k:  Fix crash due to tasklet race.
  2013-11-06 13:06 ` Kalle Valo
@ 2013-11-06 17:00   ` Ben Greear
  0 siblings, 0 replies; 6+ messages in thread
From: Ben Greear @ 2013-11-06 17:00 UTC (permalink / raw)
  To: Kalle Valo; +Cc: ath10k

On 11/06/2013 05:06 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
> 
>> From: Ben Greear <greearb@candelatech.com>
>>
>> The tasklet can run after the rings have been cleaned up,
>> so check for NULL before de-referencing the ring.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
> 
> I'll drop this as Michal's patch has a proper fix for this issue.

Ok, that sounds fine.  I'll rebase when you push his patches and
drop my patch for continued testing.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-06 17:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-29 21:38 [PATCH] ath10k: Fix crash due to tasklet race greearb
2013-10-30  9:03 ` Michal Kazior
2013-11-06 11:50   ` Kalle Valo
2013-11-06 11:54     ` Michal Kazior
2013-11-06 13:06 ` Kalle Valo
2013-11-06 17:00   ` Ben Greear

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.