Re: [PATCH] bcm43xx: Fix cancellation of work queues crashes

[Michael Buesch <mb@bu3sch.de>]

On Tuesday 04 September 2007, Larry Finger wrote:
> A crash upon booting that is caused by bcm43xx has been reported [1] and
> found to be due to a work queue being reinitialized while work on that
> queue is still pending. This fix modifies the shutdown of work queues and
> prevents periodic work from being requeued during shutdown. With this patch,
> no more crashes on reboot were observed by the original reporter. I do not
> get that particular failure on my system; however, when running a large
> number of ifdown/ifup sequences, my system would kernel panic with the
> 'caps lock' light blinking at roughly a 1 Hz rate. In addition, there were
> infrequent failures in the firmware that resulted in 'IRQ READY TIMEOUT'
> errors. With this patch, no more of the first type of failure occur, and
> incidence of the second type is greatly reduced.
> 
> [1] http://bugzilla.kernel.org/show_bug.cgi?id=8937
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> John,
> 
> This fix should be sent to 2.6.23. Once it it there, I'll send it on to
> -stable.
> 
> Larry
> 
> 
>  drivers/net/wireless/bcm43xx/bcm43xx_main.c  |   28 +++++++++++++++++++--------
>  drivers/net/wireless/bcm43xx/bcm43xx_main.h  |    2 -
>  drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c |    3 +-
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> 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;
> @@ -3299,6 +3298,14 @@ static int bcm43xx_rng_init(struct bcm43
>  	return err;
>  }
>  
> +void bcm43xx_cancel_work(struct bcm43xx_private *bcm)
> +{
> +	/* The system must be unlocked when this routine is entered.
> +	 * If not, the next 2 steps may deadlock */
> +	cancel_work_sync(&bcm->restart_work);
> +	cancel_delayed_work_sync(&bcm->periodic_work);
> +}
> +
>  static int bcm43xx_shutdown_all_wireless_cores(struct bcm43xx_private *bcm)
>  {
>  	int ret = 0;
> @@ -3335,7 +3342,12 @@ 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);
> +
> +	bcm43xx_cancel_work(bcm);
>  
>  	mutex_lock(&(bcm)->mutex);
>  	bcm43xx_shutdown_all_wireless_cores(bcm);
> @@ -4030,7 +4042,7 @@ static int bcm43xx_net_stop(struct net_d
>  	err = bcm43xx_disable_interrupts_sync(bcm);
>  	assert(!err);
>  	bcm43xx_free_board(bcm);
> -	flush_scheduled_work();
> +	bcm43xx_cancel_work(bcm);
>  
>  	return 0;
>  }
> @@ -4162,9 +4174,9 @@ static void bcm43xx_chip_reset(struct wo
>  	struct bcm43xx_phyinfo *phy;
>  	int err = -ENODEV;
>  
> +	bcm43xx_cancel_work(bcm);
>  	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)
> 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,7 @@ 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_cancel_work(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,8 +327,9 @@ static ssize_t bcm43xx_attr_phymode_stor
>  		goto out;
>  	}
>  
> -	bcm43xx_periodic_tasks_delete(bcm);
> +	bcm43xx_cancel_work(bcm);
>  	mutex_lock(&(bcm)->mutex);
> +	bcm43xx_set_status(bcm, BCM43xx_STAT_UNINIT);

Don't change the status here. That is _wrong_.
The rest of the patch is OK.

>  	err = bcm43xx_select_wireless_core(bcm, phytype);
>  	if (!err)
>  		bcm43xx_periodic_tasks_setup(bcm);