[Jaunty SRU] LP#345710 [v2: description modified] iwl3945: cancel rfkill_poll with sync when?the module is exiting

[Stefan Bader]

Huaxu Wan wrote:
> On 11:31 Wed 01 Jul, Stefan Bader wrote:
>> Huaxu Wan wrote:
>>> Hi Stefan,
>>>
>>> The patch is fine for me. It works for 2.6.28 kernel. But, this bug is
>>> not found with 2.6.30 or above kernel. This patch is not valuable to
>>> 2.6.30+, but 2.6.28 stable. I have no idea about whether it should be
>>> send to upstream. Please do it if you want.
>>>
>> Hi Huaxu,
>>
>> how stupid of me. I guess we can forget about moving he cancel statement. 
>> I still have to get it verified but looking at the upstream code we got 
>> the following patch which moves ieee80211_unregister_hw from below 
>> stopping the rfkill_poll to above. And (though I did not follow the code 
>> completely) it seems likely that ieee80211_unregister_hw could call 
>> iwl3945_mac_stop which restarts the delayed work.
>> That patch went in after 2.6.29-rc2 and the one doing the cancel as sync 
>> after 2.6.30-rc1. So the final fix is/was with 2.6.30.
>> Does this sound reasonable?
> 
> Hi Stefan,
> 
> I'm confused :S. 
> 
> Let me clarify something. The original bug(system freezed when module exit)
> was introduced by backport the patch [iwl3945: report killswitch changes
> even if the interface is down(2663516d8fb896430bf42dce41b3e2f141d63bd5)]
> to fix a bug about rfkill. Then the patch we are discussing fix the bug.
> 
> I once tried to reproduce the system freeze bug with 2.6.30 without the
> patch, but seems it was fixed in another way.
> 
> Upstream wanted to pick the patch into 2.6.30 because it's reasonable to
> cancel rfkill_poll with cancel_delayed_work_sync, I think. However, I 
> happened to be on vacation when the upstream guy ask for patch. Since no
> response from me, they made a patch directly. But, the patch does not
> fix the bug in 2.6.28.  ^_^
> 

Hi Huaxu,

it makes sense to me. :) I try to better explain my thinking. The patch you 
mention "report rfkill changes event if interface is down" added the 
rfkill_poll workqueue. One part of the problem is, that it starts the worker 
when the interface is going down:

iwl3945_mac_stop -> queue_delayed_work(priv->workqueue, &priv->rfkill_poll

And this is also being done from iwl3945_pci_remove through calling 
ieee80211_unregister_hw which is called in 2.6.28 _after_ the rfkill_poll has 
been cancelled. So it is running again. The other patch "Release resource 
before shutting down and notify upper stack." not only calls only either 
iwl3945_down or 80211_unregister_hw, but it also does it _before_ cancelling 
the rfkill_poll. So this patch is probably the one that fixes more occurances 
of this, while making the cancel call sync is another sensible step to prevent 
any remaining loopholes.
Your v2 patch basically did the same because it moved the cancel call _below_ 
the ieee80211_unregister_hw.
I hope this un-confuses things.

Cheers,
Stefan


>> Regards,
>> Stefan
>>
>> -- 
>>
>> When all other means of communication fail, try words!
>>
>>
> 
>> >From d552bfb65241a35d48e44ddb0d27e0454f579ab4 Mon Sep 17 00:00:00 2001
>> From: Kolekar, Abhijeet <abhijeet.kolekar at intel.com>
>> Date: Fri, 19 Dec 2008 10:37:41 +0800
>> Subject: [PATCH] iwl3945: release resources before shutting down
>>
>> Release resource before shutting down and notify upper stack.
>>
>> Signed-off-by: Abhijeet Kolekar <abhijeet.kolekar at intel.com>
>> Signed-off-by: Zhu Yi <yi.zhu at intel.com>
>> Signed-off-by: John W. Linville <linville at tuxdriver.com>
>> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
>> ---
>>  drivers/net/wireless/iwlwifi/iwl3945-base.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
>> index b845097..17f01a6 100644
>> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
>> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
>> @@ -7722,7 +7722,12 @@ static void __devexit iwl3945_pci_remove(struct pci_dev *pdev)
>>  
>>  	set_bit(STATUS_EXIT_PENDING, &priv->status);
>>  
>> -	iwl3945_down(priv);
>> +	if (priv->mac80211_registered) {
>> +		ieee80211_unregister_hw(priv->hw);
>> +		priv->mac80211_registered = 0;
>> +	} else {
>> +		iwl3945_down(priv);
>> +	}
>>  
>>  	/* make sure we flush any pending irq or
>>  	 * tasklet for the driver
>> @@ -7745,9 +7750,6 @@ static void __devexit iwl3945_pci_remove(struct pci_dev *pdev)
>>  	iwl3945_unset_hw_params(priv);
>>  	iwl3945_clear_stations_table(priv);
>>  
>> -	if (priv->mac80211_registered)
>> -		ieee80211_unregister_hw(priv->hw);
>> -
>>  	/*netif_stop_queue(dev); */
>>  	flush_workqueue(priv->workqueue);
>>  
>> -- 
>> 1.5.4.3
>>
> 
>