Messages in this thread Patch in this message | | | Subject | Re: "deadlock" between smc91x driver and link_watch | From | Ian Campbell <> | Date | Wed, 24 Nov 2004 09:58:16 +0000 |
| |
On Wed, 2004-11-24 at 01:46 -0800, Andrew Morton wrote: > Ian Campbell <icampbell@arcom.com> wrote: > > > > On Tue, 2004-11-23 at 15:31 -0800, Andrew Morton wrote: > > > One possible fix would be to remove that flush_scheduled_work() and to do > > > refcounting around smc_phy_configure(): dev_hold() when scheduling the work > > > (if schedule_work() returned true), dev_put() in the handler. > > > > Something like the following? > > I think so. > > > +static void smc_phy_configure_wq(void *data) > > +{ > > + struct net_device *dev = data; > > + dev_put(dev); > > + smc_phy_configure(data); > > +} > > You'd want to do the dev_put() after the smc_phy_configure() though. It > may still be a tiny bit racy against module unload.
Quite right. Fixed patch included.
Signed-off-by: Ian Campbell <icampbell@arcom.com>
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-24 09:53:54.397248397 +0000 @@ -1158,6 +1158,20 @@ } /* + * smc_phy_configure_wq + * + * The net_device is referenced when the work was scheduled to avoid + * the need for a flush_scheduled_work() in smc_close(). Drop the + * reference and then do the configuration. + */ +static void smc_phy_configure_wq(void *data) +{ + struct net_device *dev = data; + smc_phy_configure(data); + dev_put(dev); +} + +/* * smc_phy_interrupt * * Purpose: Handle interrupts relating to PHY register 18. This is @@ -1350,10 +1364,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)) { + dev_hold(dev); + } + } /* We can accept TX packets again */ dev->trans_start = jiffies; @@ -1536,10 +1553,8 @@ /* clear everything */ smc_shutdown(dev); - if (lp->phy_type != 0) { - flush_scheduled_work(); + if (lp->phy_type != 0) smc_phy_powerdown(dev, lp->mii.phy_id); - } if (lp->pending_tx_skb) { dev_kfree_skb(lp->pending_tx_skb); @@ -1891,7 +1906,7 @@ dev->ethtool_ops = &smc_ethtool_ops; tasklet_init(&lp->tx_task, smc_hardware_send_pkt, (unsigned long)dev); - INIT_WORK(&lp->phy_configure, smc_phy_configure, dev); + INIT_WORK(&lp->phy_configure, smc_phy_configure_wq, dev); lp->mii.phy_id_mask = 0x1f; lp->mii.reg_num_mask = 0x1f; lp->mii.force_media = 0;
-- 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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |