On Wed, 2004-11-24 at 01:46 -0800, Andrew Morton wrote:
> Ian Campbell <icampbell@xxxxxxxxx> 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@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-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
|