Michael Buesch wrote:
> On Friday 31 August 2007, Larry Finger wrote:
>> This patch fixes a problem with work queues during shutdown. The bug
>> seems to be responsible for the boot-time crashes reported in
>> http://bugzilla.kernel.org/show_bug.cgi?id=8937.
>>
>> Signed-off-by: Larry Finger <[EMAIL PROTECTED]>
>> ---
>>
>> Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> +++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> @@ -3174,6 +3174,9 @@ static void bcm43xx_periodic_every1sec(s
>>
>> static void do_periodic_work(struct bcm43xx_private *bcm)
>> {
>> + /* keep from rearming periodic work if shutting down */
>> + if (bcm43xx_status(bcm) == BCM43xx_STAT_UNINIT)
>> + return;
>
> I think this should be put at the beginning of bcm43xx_periodic_work_handler()
> after the mutex locking.
>
>> if (bcm->periodic_state % 120 == 0)
>> bcm43xx_periodic_every120sec(bcm);
>> if (bcm->periodic_state % 60 == 0)
>> @@ -3245,11 +3248,6 @@ static void bcm43xx_periodic_work_handle
>> mutex_unlock(&bcm->mutex);
>> }
>>
>> -void bcm43xx_periodic_tasks_delete(struct bcm43xx_private *bcm)
>> -{
>> - cancel_rearming_delayed_work(&bcm->periodic_work);
>> -}
>> -
>> void bcm43xx_periodic_tasks_setup(struct bcm43xx_private *bcm)
>> {
>> struct delayed_work *work = &bcm->periodic_work;
>> @@ -3335,9 +3333,14 @@ static void bcm43xx_free_board(struct bc
>> {
>> bcm43xx_rng_exit(bcm);
>> bcm43xx_sysfs_unregister(bcm);
>> - bcm43xx_periodic_tasks_delete(bcm);
>> +
>> + /* next 2 steps must be unlocked, else they may deadlock */
>> + cancel_work_sync(&bcm->restart_work);
>> + cancel_delayed_work_sync(&bcm->periodic_work);
>
>
>
>> mutex_lock(&(bcm)->mutex);
>> + bcm43xx_set_status(bcm, BCM43xx_STAT_UNINIT);
>> +
>
> This status change must be moved above the cancel calls.
>
>> bcm43xx_shutdown_all_wireless_cores(bcm);
>> bcm43xx_pctl_set_crystal(bcm, 0);
>> mutex_unlock(&(bcm)->mutex);
>> @@ -4030,7 +4033,6 @@ static int bcm43xx_net_stop(struct net_d
>> err = bcm43xx_disable_interrupts_sync(bcm);
>> assert(!err);
>> bcm43xx_free_board(bcm);
>> - flush_scheduled_work();
>>
>> return 0;
>> }
>> @@ -4164,7 +4166,6 @@ static void bcm43xx_chip_reset(struct wo
>>
>> mutex_lock(&(bcm)->mutex);
>> if (bcm43xx_status(bcm) == BCM43xx_STAT_INITIALIZED) {
>> - bcm43xx_periodic_tasks_delete(bcm);
>
> Look at the comment below. That also applies here.
>
>> phy = bcm43xx_current_phy(bcm);
>> err = bcm43xx_select_wireless_core(bcm, phy->type);
>> if (!err)
>> Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.h
>> ===================================================================
>> --- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.h
>> +++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.h
>> @@ -122,7 +122,6 @@ void bcm43xx_wireless_core_reset(struct
>> void bcm43xx_mac_suspend(struct bcm43xx_private *bcm);
>> void bcm43xx_mac_enable(struct bcm43xx_private *bcm);
>>
>> -void bcm43xx_periodic_tasks_delete(struct bcm43xx_private *bcm);
>> void bcm43xx_periodic_tasks_setup(struct bcm43xx_private *bcm);
>>
>> void bcm43xx_controller_restart(struct bcm43xx_private *bcm, const char
>> *reason);
>> Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
>> +++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
>> @@ -327,7 +327,6 @@ static ssize_t bcm43xx_attr_phymode_stor
>> goto out;
>> }
>>
>> - bcm43xx_periodic_tasks_delete(bcm);
>
> Well, this is tricky crap code.
> This breaks it.
> Look at the !err case below. We re-enable pwork there.
> We only do it for !err. So when core select failed, we
> end up without pwork, which is what we want.
> I think it might be OK to simply remove the delete _and_
> the setup below, as select core sets status to UNINIT when
> it failed. So pwork will bail out early (if you change your
> patch above as I suggested).
>
>> mutex_lock(&(bcm)->mutex);
>> err = bcm43xx_select_wireless_core(bcm, phytype);
>> if (!err)
>>
>>
>
>
> PS: Yes, this is crap code. :) All these cornercases.
>
I think I understood your comments. The revised patch is as follows:
Larry
------
Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c
+++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
@@ -3197,6 +3197,9 @@ static void bcm43xx_periodic_work_handle
unsigned long orig_trans_start = 0;
mutex_lock(&bcm->mutex);
+ /* keep from doing and rearming periodic work if shutting down */
+ if (bcm43xx_status(bcm) == BCM43xx_STAT_UNINIT)
+ goto unlock_mutex;
if (unlikely(bcm->periodic_state % 60 == 0)) {
/* Periodic work will take a long time, so we want it to
* be preemtible.
@@ -3242,14 +3245,10 @@ static void bcm43xx_periodic_work_handle
mmiowb();
bcm->periodic_state++;
spin_unlock_irqrestore(&bcm->irq_lock, flags);
+unlock_mutex:
mutex_unlock(&bcm->mutex);
}
-void bcm43xx_periodic_tasks_delete(struct bcm43xx_private *bcm)
-{
- cancel_rearming_delayed_work(&bcm->periodic_work);
-}
-
void bcm43xx_periodic_tasks_setup(struct bcm43xx_private *bcm)
{
struct delayed_work *work = &bcm->periodic_work;
@@ -3335,7 +3334,15 @@ static void bcm43xx_free_board(struct bc
{
bcm43xx_rng_exit(bcm);
bcm43xx_sysfs_unregister(bcm);
- bcm43xx_periodic_tasks_delete(bcm);
+
+ mutex_lock(&(bcm)->mutex);
+ bcm43xx_set_status(bcm, BCM43xx_STAT_UNINIT);
+ mutex_unlock(&(bcm)->mutex);
+
+ /* next 2 steps must be unlocked, else they may deadlock */
+ cancel_work_sync(&bcm->restart_work);
+ cancel_delayed_work_sync(&bcm->periodic_work);
+
mutex_lock(&(bcm)->mutex);
bcm43xx_shutdown_all_wireless_cores(bcm);
@@ -4030,7 +4037,6 @@ static int bcm43xx_net_stop(struct net_d
err = bcm43xx_disable_interrupts_sync(bcm);
assert(!err);
bcm43xx_free_board(bcm);
- flush_scheduled_work();
return 0;
}
@@ -4164,11 +4170,8 @@ static void bcm43xx_chip_reset(struct wo
mutex_lock(&(bcm)->mutex);
if (bcm43xx_status(bcm) == BCM43xx_STAT_INITIALIZED) {
- bcm43xx_periodic_tasks_delete(bcm);
phy = bcm43xx_current_phy(bcm);
err = bcm43xx_select_wireless_core(bcm, phy->type);
- if (!err)
- bcm43xx_periodic_tasks_setup(bcm);
}
mutex_unlock(&(bcm)->mutex);
Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.h
===================================================================
--- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.h
+++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.h
@@ -122,7 +122,6 @@ void bcm43xx_wireless_core_reset(struct
void bcm43xx_mac_suspend(struct bcm43xx_private *bcm);
void bcm43xx_mac_enable(struct bcm43xx_private *bcm);
-void bcm43xx_periodic_tasks_delete(struct bcm43xx_private *bcm);
void bcm43xx_periodic_tasks_setup(struct bcm43xx_private *bcm);
void bcm43xx_controller_restart(struct bcm43xx_private *bcm, const char
*reason);
Index: linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
+++ linux-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
@@ -327,11 +327,8 @@ static ssize_t bcm43xx_attr_phymode_stor
goto out;
}
- bcm43xx_periodic_tasks_delete(bcm);
mutex_lock(&(bcm)->mutex);
err = bcm43xx_select_wireless_core(bcm, phytype);
- if (!err)
- bcm43xx_periodic_tasks_setup(bcm);
mutex_unlock(&(bcm)->mutex);
if (err == -ESRCH)
err = -ENODEV;
_______________________________________________
Bcm43xx-dev mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
