Re: "deadlock" between smc91x driver and link_watch

[Nicolas Pitre]

On Thu, 25 Nov 2004, Ian Campbell wrote:

> Hi,
> 
> I've taken Nico's comments on board and used lp->work_pending to detect
> whether smc_phy_configure is pending or not instead of dev_hold/put(). I
> was able to do away with smc_phy_configure_wq() from the previous
> version since setting lp->work_pending=0 can safely be done in
> smc_phy_configure() itself, unlike dev_put().
> 
> Also fixed a typo 'ence' -> 'Hence' and renamed smc_detect_phy to
> smc_phy_detect in order to follow the same pattern as the other
> smc_phy_* functions, I was typing smc_phy_detect() every time anyway and
> it was getting on my wick. I hope that's OK...
> 
> Signed-off-by: Ian Campbell <icampbell@xxxxxxxxx>

Good!  Please add "Signed-off-by: Nicolas Pitre <nico@xxxxxxx>" and 
send it to Jeff Garzik <jgarzik@xxxxxxxxx>.


> Index: 2.6/drivers/net/smc91x.c
> ===================================================================
> --- 2.6.orig/drivers/net/smc91x.c	2004-11-16 09:26:52.000000000 +0000
> +++ 2.6/drivers/net/smc91x.c	2004-11-25 09:49:38.830953019 +0000
> @@ -203,7 +203,10 @@
>  	u32	msg_enable;
>  	u32	phy_type;
>  	struct mii_if_info mii;
> +
> +	/* work queue */
>  	struct work_struct phy_configure;
> +	int	work_pending;
>  
>  	spinlock_t lock;
>  
> @@ -903,7 +906,7 @@
>  /*
>   * Finds and reports the PHY address
>   */
> -static void smc_detect_phy(struct net_device *dev)
> +static void smc_phy_detect(struct net_device *dev)
>  {
>  	struct smc_local *lp = netdev_priv(dev);
>  	int phyaddr;
> @@ -1155,6 +1158,7 @@
>  
>  smc_phy_configure_exit:
>  	spin_unlock_irq(&lp->lock);
> +	lp->work_pending = 0;
>  }
>  
>  /*
> @@ -1350,10 +1354,13 @@
>  	/*
>  	 * Reconfiguring the PHY doesn't seem like a bad idea here, but
>  	 * smc_phy_configure() calls msleep() which calls schedule_timeout()
> -	 * which calls schedule().  Ence we use a work queue.
> +	 * which calls schedule().  Hence we use a work queue.
>  	 */
> -	if (lp->phy_type != 0)
> -		schedule_work(&lp->phy_configure);
> +	if (lp->phy_type != 0) {
> +		if (schedule_work(&lp->phy_configure)) {
> +			lp->work_pending = 1;
> +		}
> +	}
>  
>  	/* We can accept TX packets again */
>  	dev->trans_start = jiffies;
> @@ -1537,7 +1544,18 @@
>  	smc_shutdown(dev);
>  
>  	if (lp->phy_type != 0) {
> -		flush_scheduled_work();
> +		/* We need to ensure that no calls to
> +		   smc_phy_configure are pending. 
> +
> +		   flush_scheduled_work() cannot be called because we
> +		   are running with the netlink semaphore held (from
> +		   devinet_ioctl()) and the pending work queue
> +		   contains linkwatch_event() (scheduled by
> +		   netif_carrier_off() above). linkwatch_event() also
> +		   wants the netlink semaphore.
> +		*/
> +		while(lp->work_pending)
> +			schedule();
>  		smc_phy_powerdown(dev, lp->mii.phy_id);
>  	}
>  
> @@ -1904,7 +1922,7 @@
>  	 * Locate the phy, if any.
>  	 */
>  	if (lp->version >= (CHIP_91100 << 4))
> -		smc_detect_phy(dev);
> +		smc_phy_detect(dev);
>  
>  	/* Set default parameters */
>  	lp->msg_enable = NETIF_MSG_LINK;
> 
> 
> -- 
> Ian Campbell, Senior Design Engineer
>                                         Web: http://www.arcom.com
> Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
> Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200
> 
> 
> _____________________________________________________________________
> The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.
> 
> This message has been virus scanned by MessageLabs.
> 


Nicolas
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/