[PATCH wireless-2.6] rt2x00: fix rmmod crash

[PATCH wireless-2.6] rt2x00: fix rmmod crash

[Stanislaw Gruszka]

Do not destroy workqueue, which still can be used by autowakeup_work,
before ieee80211_unregister_hw() is called.

Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
---
 drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index c018d67..2f2627b 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -1165,7 +1165,6 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
 		cancel_work_sync(&rt2x00dev->rxdone_work);
 		cancel_work_sync(&rt2x00dev->txdone_work);
 	}
-	destroy_workqueue(rt2x00dev->workqueue);
 
 	/*
 	 * Free the tx status fifo.
@@ -1198,6 +1197,11 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
 	rt2x00lib_remove_hw(rt2x00dev);
 
 	/*
+	 * Now nobody use workqueue anymore.
+	 */
+	destroy_workqueue(rt2x00dev->workqueue);
+
+	/*
 	 * Free firmware image.
 	 */
 	rt2x00lib_free_firmware(rt2x00dev);
-- 
1.7.4

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Gertjan van Wingerde]

On 05/29/11 12:45, Stanislaw Gruszka wrote:
> Do not destroy workqueue, which still can be used by autowakeup_work,
> before ieee80211_unregister_hw() is called.
> 
> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>

Could you add some details of the crash here, e.g. the stack trace of when this occurs, so that
we can understand what is exactly happening that is causing the crash, for our education.

> ---
>  drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index c018d67..2f2627b 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -1165,7 +1165,6 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
>  		cancel_work_sync(&rt2x00dev->rxdone_work);
>  		cancel_work_sync(&rt2x00dev->txdone_work);
>  	}
> -	destroy_workqueue(rt2x00dev->workqueue);
>  
>  	/*
>  	 * Free the tx status fifo.
> @@ -1198,6 +1197,11 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
>  	rt2x00lib_remove_hw(rt2x00dev);
>  
>  	/*
> +	 * Now nobody use workqueue anymore.
> +	 */
> +	destroy_workqueue(rt2x00dev->workqueue);
> +
> +	/*
>  	 * Free firmware image.
>  	 */
>  	rt2x00lib_free_firmware(rt2x00dev);


-- 
---
Gertjan

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Stanislaw Gruszka]

On Sun, May 29, 2011 at 06:48:13PM +0200, Gertjan van Wingerde wrote:
> On 05/29/11 12:45, Stanislaw Gruszka wrote:
> > Do not destroy workqueue, which still can be used by autowakeup_work,
> > before ieee80211_unregister_hw() is called.
> > 
> > Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
> 
> Could you add some details of the crash here, e.g. the stack trace of when this occurs, so that
> we can understand what is exactly happening that is causing the crash, for our education.

Crash is in workqueue code in interrupt context, there is no indication about rt2x00 blame in
the calltrace, but crash happen directly after rmmod rt73usb, here is a photo:
https://bugzilla.kernel.org/attachment.cgi?id=59992 

Stanislaw

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Gertjan van Wingerde]

On 05/29/11 22:42, Stanislaw Gruszka wrote:
> On Sun, May 29, 2011 at 06:48:13PM +0200, Gertjan van Wingerde wrote:
>> On 05/29/11 12:45, Stanislaw Gruszka wrote:
>>> Do not destroy workqueue, which still can be used by autowakeup_work,
>>> before ieee80211_unregister_hw() is called.
>>>
>>> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
>>
>> Could you add some details of the crash here, e.g. the stack trace of when this occurs, so that
>> we can understand what is exactly happening that is causing the crash, for our education.
> 
> Crash is in workqueue code in interrupt context, there is no indication about rt2x00 blame in
> the calltrace, but crash happen directly after rmmod rt73usb, here is a photo:
> https://bugzilla.kernel.org/attachment.cgi?id=59992 

OK. Thanks for the info.

Could you create a v2 of the patch that:
1. Includes a reference to the bugzilla entry that the patch is resolving.
2. Cc's the stable team.

In that v2 you can add my acked-by as follows:
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

Thanks for the patch.

---
Gertjan

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Stanislaw Gruszka]

On Mon, May 30, 2011 at 08:24:12AM +0200, Gertjan van Wingerde wrote:
> On 05/29/11 22:42, Stanislaw Gruszka wrote:
> > On Sun, May 29, 2011 at 06:48:13PM +0200, Gertjan van Wingerde wrote:
> >> On 05/29/11 12:45, Stanislaw Gruszka wrote:
> >>> Do not destroy workqueue, which still can be used by autowakeup_work,
> >>> before ieee80211_unregister_hw() is called.
> >>>
> >>> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
> >>
> >> Could you add some details of the crash here, e.g. the stack trace of when this occurs, so that
> >> we can understand what is exactly happening that is causing the crash, for our education.
> > 
> > Crash is in workqueue code in interrupt context, there is no indication about rt2x00 blame in
> > the calltrace, but crash happen directly after rmmod rt73usb, here is a photo:
> > https://bugzilla.kernel.org/attachment.cgi?id=59992 
> 
> OK. Thanks for the info.
> 
> Could you create a v2 of the patch that:
> 1. Includes a reference to the bugzilla entry that the patch is resolving.

It does not solve the bugzilla,
https://bugzilla.kernel.org/show_bug.cgi?id=32132
I put image there, because of temporally lack of opportunity to put
it in some better place. Sorry.

> 2. Cc's the stable team.

It's not stable fix, this bug was introduced currently by

commit 1c0bcf89d85cc97a0d9ce4cd909351a81fa4fdde
Author: Ivo van Doorn <ivdoorn@gmail.com>
Date:   Sat Apr 30 17:18:18 2011 +0200

    rt2x00: Add autowake support for USB hardware

Thanks
Stanislaw

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Gertjan van Wingerde]

On Mon, May 30, 2011 at 11:52 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Mon, May 30, 2011 at 08:24:12AM +0200, Gertjan van Wingerde wrote:
>> On 05/29/11 22:42, Stanislaw Gruszka wrote:
>> > On Sun, May 29, 2011 at 06:48:13PM +0200, Gertjan van Wingerde wrote:
>> >> On 05/29/11 12:45, Stanislaw Gruszka wrote:
>> >>> Do not destroy workqueue, which still can be used by autowakeup_work,
>> >>> before ieee80211_unregister_hw() is called.
>> >>>
>> >>> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
>> >>
>> >> Could you add some details of the crash here, e.g. the stack trace of when this occurs, so that
>> >> we can understand what is exactly happening that is causing the crash, for our education.
>> >
>> > Crash is in workqueue code in interrupt context, there is no indication about rt2x00 blame in
>> > the calltrace, but crash happen directly after rmmod rt73usb, here is a photo:
>> > https://bugzilla.kernel.org/attachment.cgi?id=59992
>>
>> OK. Thanks for the info.
>>
>> Could you create a v2 of the patch that:
>> 1. Includes a reference to the bugzilla entry that the patch is resolving.
>
> It does not solve the bugzilla,
> https://bugzilla.kernel.org/show_bug.cgi?id=32132
> I put image there, because of temporally lack of opportunity to put
> it in some better place. Sorry.

Ah, OK. That got me confused.

>
>> 2. Cc's the stable team.
>
> It's not stable fix, this bug was introduced currently by
>
> commit 1c0bcf89d85cc97a0d9ce4cd909351a81fa4fdde
> Author: Ivo van Doorn <ivdoorn@gmail.com>
> Date:   Sat Apr 30 17:18:18 2011 +0200
>
>    rt2x00: Add autowake support for USB hardware
>

I cannot check it right now, but that patch is not yet included in
wireless-2.6, which
you indicate (in the email subject) as being the target for this patch.
Which tree do you want this patch to be applied to?

---
Gertjan

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Stanislaw Gruszka]

On Mon, May 30, 2011 at 01:26:37PM +0200, Gertjan van Wingerde wrote:
> > It's not stable fix, this bug was introduced currently by
> >
> > commit 1c0bcf89d85cc97a0d9ce4cd909351a81fa4fdde
> > Author: Ivo van Doorn <ivdoorn@gmail.com>
> > Date:   Sat Apr 30 17:18:18 2011 +0200
> >
> >    rt2x00: Add autowake support for USB hardware
> >
> 
> I cannot check it right now, but that patch is not yet included in
> wireless-2.6, which
> you indicate (in the email subject) as being the target for this patch.
> Which tree do you want this patch to be applied to?

Oh, indeed. But it is in current Linus' tree, patch is targeted there.

Stanislaw

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Gertjan van Wingerde]

On Mon, May 30, 2011 at 2:52 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Mon, May 30, 2011 at 01:26:37PM +0200, Gertjan van Wingerde wrote:
>> > It's not stable fix, this bug was introduced currently by
>> >
>> > commit 1c0bcf89d85cc97a0d9ce4cd909351a81fa4fdde
>> > Author: Ivo van Doorn <ivdoorn@gmail.com>
>> > Date:   Sat Apr 30 17:18:18 2011 +0200
>> >
>> >    rt2x00: Add autowake support for USB hardware
>> >
>>
>> I cannot check it right now, but that patch is not yet included in
>> wireless-2.6, which
>> you indicate (in the email subject) as being the target for this patch.
>> Which tree do you want this patch to be applied to?
>
> Oh, indeed. But it is in current Linus' tree, patch is targeted there.
>

OK. Everything is clear now.

Personally I am okay with your patch; however, I would like for Ivo to
comment, as it was his patch
that caused the issues in the first place.

---
Gertjan

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Ivo Van Doorn]

Hi,

On Sun, May 29, 2011 at 12:45 PM, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> Do not destroy workqueue, which still can be used by autowakeup_work,
> before ieee80211_unregister_hw() is called.

Well isn't the bug then that autowakeup_work isn't cancelled using
cancel_work_sync
like the other workqueue tasks?

I rather add the call to cancel_work_sync then moving the killing of
the workqueue.
The position where the workqueue is destroyed should be the place
where we cancel
all scheduled work, that way we can assume no rt2x00-related threads ater this
position.

Ivo

> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
> ---
>  drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index c018d67..2f2627b 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -1165,7 +1165,6 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
>                cancel_work_sync(&rt2x00dev->rxdone_work);
>                cancel_work_sync(&rt2x00dev->txdone_work);
>        }
> -       destroy_workqueue(rt2x00dev->workqueue);
>
>        /*
>         * Free the tx status fifo.
> @@ -1198,6 +1197,11 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
>        rt2x00lib_remove_hw(rt2x00dev);
>
>        /*
> +        * Now nobody use workqueue anymore.
> +        */
> +       destroy_workqueue(rt2x00dev->workqueue);
> +
> +       /*
>         * Free firmware image.
>         */
>        rt2x00lib_free_firmware(rt2x00dev);
> --
> 1.7.4
>
>

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Stanislaw Gruszka]

Hello

On Mon, May 30, 2011 at 08:25:37PM +0200, Ivo Van Doorn wrote:
> On Sun, May 29, 2011 at 12:45 PM, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> > Do not destroy workqueue, which still can be used by autowakeup_work,
> > before ieee80211_unregister_hw() is called.
> 
> Well isn't the bug then that autowakeup_work isn't cancelled using
> cancel_work_sync
> like the other workqueue tasks?
> 
> I rather add the call to cancel_work_sync then moving the killing of
> the workqueue.
> The position where the workqueue is destroyed should be the place
> where we cancel
> all scheduled work, that way we can assume no rt2x00-related threads ater this
> position.

rt2x00lib_config() could be running simultaneously to rt2x00lib_remove_dev()
and queue just canceled autowake_work again into (just destroyed) workqueue.
Other solution to fix, would be use ieee80211_queue_work() instead of custom
workqueue, is that better?

Stanislaw

Re: [PATCH wireless-2.6] rt2x00: fix rmmod crash

[Ivo Van Doorn]

Hi,

>> Well isn't the bug then that autowakeup_work isn't cancelled using
>> cancel_work_sync
>> like the other workqueue tasks?
>>
>> I rather add the call to cancel_work_sync then moving the killing of
>> the workqueue.
>> The position where the workqueue is destroyed should be the place
>> where we cancel
>> all scheduled work, that way we can assume no rt2x00-related threads ater this
>> position.
>
> rt2x00lib_config() could be running simultaneously to rt2x00lib_remove_dev()
> and queue just canceled autowake_work again into (just destroyed) workqueue.
> Other solution to fix, would be use ieee80211_queue_work() instead of custom
> workqueue, is that better?

Well 3 things are needed then,
1) Don't schedule autowake_work when DEVICE_PRESENT flag is not set
2) The handler for autowake_work should exit immediately when
DEVICE_PRESENT flag is not set
3) add a cancel_work_sync for the autowake_work inside remove_dev

That should effictively prevent the entire work structure from running
while the driver
is being rmmodded, or the device is being unplugged.

Ivo