[PATCH 0/2] net: smsc911x: Move phy and interrupt config

Hi,

The smsc911x driver is doing a number of things in its probe routine that
should be delayed until the interface is started. Because of this, the module
cannot be unloaded, the phy states are incorrect/stale if the interface isn't
running, open's unnecessarily fail causing network configuration problems, and
the /proc/irq nodes are incorrectly named.

Clean up a number of these problems by moving the mdio and interrupt
configuration into the smsc911x_open routine.

Jeremy Linton (2):
  net: smsc911x: Fix register_netdev, phy startup, driver unload
    ordering
  net/smsc911x: Move interrupt allocation to open/stop

 drivers/net/ethernet/smsc/smsc911x.c | 89 +++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

-- 
2.5.5

Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.

The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.

Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/ethernet/smsc/smsc911x.c | 51 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..6d6dcfe 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c

@@ -1099,15 +1099,8 @@ static int smsc911x_mii_init(struct platform_device *pdev,
 		goto err_out_free_bus_2;
 	}
 
-	if (smsc911x_mii_probe(dev) < 0) {
-		SMSC_WARN(pdata, probe, "Error registering mii bus");
-		goto err_out_unregister_bus_3;
-	}
-
 	return 0;
 
-err_out_unregister_bus_3:
-	mdiobus_unregister(pdata->mii_bus);
 err_out_free_bus_2:
 	mdiobus_free(pdata->mii_bus);
 err_out_1:

@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
 	unsigned int timeout;
 	unsigned int temp;
 	unsigned int intcfg;
+	int retval;
 
-	/* if the phy is not yet registered, retry later*/
+	/* find and start the given phy */
 	if (!dev->phydev) {
-		SMSC_WARN(pdata, hw, "phy_dev is NULL");
-		return -EAGAIN;
+		if (smsc911x_mii_probe(dev) < 0) {
+			SMSC_WARN(pdata, probe, "Error starting phy");
+			retval = -EAGAIN;
+			goto out;
+		}
 	}
 
 	/* Reset the LAN911x */
 	if (smsc911x_soft_reset(pdata)) {
 		SMSC_WARN(pdata, hw, "soft reset failed");
-		return -EIO;
+		retval = -EIO;
+		goto mii_free_out;
 	}
 
 	smsc911x_reg_write(pdata, HW_CFG, 0x00050000);

@@ -1600,7 +1598,8 @@ static int smsc911x_open(struct net_device *dev)
 	if (!pdata->software_irq_signal) {
 		netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
 			    dev->irq);
-		return -ENODEV;
+		retval = -ENODEV;
+		goto mii_free_out;
 	}
 	SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
 		   dev->irq);

@@ -1646,6 +1645,12 @@ static int smsc911x_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 	return 0;
+
+mii_free_out:
+	phy_disconnect(dev->phydev);
+	dev->phydev = NULL;
+out:
+	return retval;
 }
 
 /* Entry point for stopping the interface */

@@ -1668,8 +1673,11 @@ static int smsc911x_stop(struct net_device *dev)
 	smsc911x_tx_update_txcounters(dev);
 
 	/* Bring the PHY down */
-	if (dev->phydev)
+	if (dev->phydev) {
 		phy_stop(dev->phydev);
+		phy_disconnect(dev->phydev);
+		dev->phydev = NULL;
+	}
 
 	SMSC_TRACE(pdata, ifdown, "Interface stopped");
 	return 0;

@@ -2291,11 +2299,10 @@ static int smsc911x_drv_remove(struct platform_device *pdev)
 	pdata = netdev_priv(dev);
 	BUG_ON(!pdata);
 	BUG_ON(!pdata->ioaddr);
-	BUG_ON(!dev->phydev);
+	WARN_ON(dev->phydev);
 
 	SMSC_TRACE(pdata, ifdown, "Stopping driver");
 
-	phy_disconnect(dev->phydev);
 	mdiobus_unregister(pdata->mii_bus);
 	mdiobus_free(pdata->mii_bus);
 

@@ -2494,6 +2501,12 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 
 	netif_carrier_off(dev);
 
+	retval = smsc911x_mii_init(pdev, dev);
+	if (retval) {
+		SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+		goto out_free_irq;
+	}
+
 	retval = register_netdev(dev);
 	if (retval) {
 		SMSC_WARN(pdata, probe, "Error %i registering device", retval);

@@ -2503,12 +2516,6 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 			   "Network interface: \"%s\"", dev->name);
 	}
 
-	retval = smsc911x_mii_init(pdev, dev);
-	if (retval) {
-		SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
-		goto out_unregister_netdev_5;
-	}
-
 	spin_lock_irq(&pdata->mac_lock);
 
 	/* Check if mac address has been specified when bringing interface up */

@@ -2544,8 +2551,6 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 
 	return 0;
 
-out_unregister_netdev_5:
-	unregister_netdev(dev);
 out_free_irq:
 	free_irq(dev->irq, dev);
 out_disable_resources:

-- 
2.5.5

The /proc/irq/xx information is incorrect for smsc911x because
the request_irq is happening before the register_netdev has the
proper device name. Moving it to the open also fixes the case
of when the device is renamed.

Reported-by: Will Deacon <redacted>
Signed-off-by: Jeremy Linton <redacted>
---
 drivers/net/ethernet/smsc/smsc911x.c | 50 +++++++++++++++---------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 6d6dcfe..8ef9776 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c

@@ -154,6 +154,8 @@ struct smsc911x_data {
 /* Easy access to information */
 #define __smsc_shift(pdata, reg) ((reg) << ((pdata)->config.shift))
 
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id);
+
 static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
 {
 	if (pdata->config.flags & SMSC911X_USE_32BIT)

@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
 	unsigned int temp;
 	unsigned int intcfg;
 	int retval;
+	int irq_flags;
 
 	/* find and start the given phy */
 	if (!dev->phydev) {

@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
 	pdata->software_irq_signal = 0;
 	smp_wmb();
 
+	irq_flags = irq_get_trigger_type(dev->irq);
+	if (request_irq(dev->irq, smsc911x_irqhandler,
+			irq_flags | IRQF_SHARED, dev->name, dev)) {
+		SMSC_WARN(pdata, probe,
+			  "Unable to claim requested irq: %d", dev->irq);
+		goto mii_free_out;
+	}
+
 	temp = smsc911x_reg_read(pdata, INT_EN);
 	temp |= INT_EN_SW_INT_EN_;
 	smsc911x_reg_write(pdata, INT_EN, temp);

@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
 		netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
 			    dev->irq);
 		retval = -ENODEV;
-		goto mii_free_out;
+		goto irq_stop_out;
 	}
 	SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
 		   dev->irq);

@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 	return 0;
-
+irq_stop_out:
+	free_irq(dev->irq, dev);
 mii_free_out:
 	phy_disconnect(dev->phydev);
 	dev->phydev = NULL;

@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
 	dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
 	smsc911x_tx_update_txcounters(dev);
 
+	free_irq(dev->irq, dev);
+
 	/* Bring the PHY down */
 	if (dev->phydev) {
 		phy_stop(dev->phydev);
 		phy_disconnect(dev->phydev);
 		dev->phydev = NULL;
 	}
+	netif_carrier_off(dev);
 
 	SMSC_TRACE(pdata, ifdown, "Interface stopped");
 	return 0;

@@ -2307,7 +2322,6 @@ static int smsc911x_drv_remove(struct platform_device *pdev)
 	mdiobus_free(pdata->mii_bus);
 
 	unregister_netdev(dev);
-	free_irq(dev->irq, dev);
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					   "smsc911x-memory");
 	if (!res)

@@ -2392,8 +2406,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 	struct smsc911x_data *pdata;
 	struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev);
 	struct resource *res;
-	unsigned int intcfg = 0;
-	int res_size, irq, irq_flags;
+	int res_size, irq;
 	int retval;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

@@ -2432,7 +2445,6 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 
 	pdata = netdev_priv(dev);
 	dev->irq = irq;
-	irq_flags = irq_get_trigger_type(irq);
 	pdata->ioaddr = ioremap_nocache(res->start, res_size);
 
 	pdata->dev = dev;

@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 	if (retval < 0)
 		goto out_disable_resources;
 
-	/* configure irq polarity and type before connecting isr */
-	if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-		intcfg |= INT_CFG_IRQ_POL_;
-
-	if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-		intcfg |= INT_CFG_IRQ_TYPE_;
-
-	smsc911x_reg_write(pdata, INT_CFG, intcfg);
-
-	/* Ensure interrupts are globally disabled before connecting ISR */
-	smsc911x_disable_irq_chip(dev);
-
-	retval = request_irq(dev->irq, smsc911x_irqhandler,
-			     irq_flags | IRQF_SHARED, dev->name, dev);
-	if (retval) {
-		SMSC_WARN(pdata, probe,
-			  "Unable to claim requested irq: %d", dev->irq);
-		goto out_disable_resources;
-	}
-
 	netif_carrier_off(dev);
 
 	retval = smsc911x_mii_init(pdev, dev);
 	if (retval) {
 		SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
-		goto out_free_irq;
+		goto out_disable_resources;
 	}
 
 	retval = register_netdev(dev);
 	if (retval) {
 		SMSC_WARN(pdata, probe, "Error %i registering device", retval);
-		goto out_free_irq;
+		goto out_disable_resources;
 	} else {
 		SMSC_TRACE(pdata, probe,
 			   "Network interface: \"%s\"", dev->name);

@@ -2551,8 +2543,6 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 
 	return 0;
 
-out_free_irq:
-	free_irq(dev->irq, dev);
 out_disable_resources:
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);

-- 
2.5.5

Hi,

On 09/01/2016 02:45 PM, Andrew Lunn wrote:

On Thu, Sep 01, 2016 at 01:47:44PM -0500, Jeremy Linton wrote:

quoted

Hi Andrew,

	Thanks for taking a look at this!

On 09/01/2016 12:06 PM, Andrew Lunn wrote:

quoted

Hi Jeremy

Please don't add forward references. Move the function earlier in the
file.

Ok, but I thought it was a fairly large move due to further
dependent functions..

There are a few other options, like moving smsc911x_open() rather than
the interrupt handler.

And i would suggest what ever you do, make it a separate patch. A
patch which says: No functional changes, just move functions around as
needed by later patches, is going to be quick and easy to review.

Yes I put it in another patch, I was busy blasting it out rather than checking my email...

quoted

quoted

quoted

+	netif_carrier_off(dev);

What has this change got to do with interrupt handling?

	This is a whoops, it should be in the previous patch..

Or a patch of its own? You also needs to be careful with ordering
against the phy_connect.

quoted

quoted

quoted

@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device *pdev)

	if (retval < 0)
		goto out_disable_resources;

-	/* configure irq polarity and type before connecting isr */
-	if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-		intcfg |= INT_CFG_IRQ_POL_;
-
-	if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-		intcfg |= INT_CFG_IRQ_TYPE_;
-
-	smsc911x_reg_write(pdata, INT_CFG, intcfg);

I see these removes, but where are the adds?

	The functionality is duplicated in open, when the IRQ handler is tested.

Ah, it is obfusticated by SMC_SET_IRQ_CFG().

If you say it is duplicated, how about a separate patch removing it,
with a clear pointer to where the duplicate is.

? Well, I didn't do that part, but I'm confused by your SMC_SET_IRQ_CFG(). AFAIK, that is smc911x not smsc911x. The code in question is in smsc911x_open() following "Set interrupt deassertion to 100uS". It looks a little different but its reprogramming the INT_CFG preceding the interrupt being enabled.

Really, if I were feeling brave (because this driver is used in so many strange pieces of hardware) I would rewrite a large portion of the interrupt management in this driver. I started looking at it last month, while looking for the mdio polling issue, because I wanted to get the link state changes directly. While at it I noticed the WOL functionality could use some attention, etc, one problem after another.

On 09/01/2016 11:58 AM, Andrew Lunn wrote:

quoted

@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
 	unsigned int timeout;
 	unsigned int temp;
 	unsigned int intcfg;
+	int retval;

-	/* if the phy is not yet registered, retry later*/
+	/* find and start the given phy */
 	if (!dev->phydev) {
-		SMSC_WARN(pdata, hw, "phy_dev is NULL");
-		return -EAGAIN;
+		if (smsc911x_mii_probe(dev) < 0) {
+			SMSC_WARN(pdata, probe, "Error starting phy");
+			retval = -EAGAIN;

smsc911x_mii_probe() returns an error code. It is better to use that,
than -EAGAIN, which is rather odd to start with.

	Thats a good point, I was just maintaining the existing behavior, but its definitely better to just return the actual error.

quoted

+			goto out;
+		}
 	}

 	/* Reset the LAN911x */
 	if (smsc911x_soft_reset(pdata)) {
 		SMSC_WARN(pdata, hw, "soft reset failed");
-		return -EIO;
+		retval = -EIO;
+		goto mii_free_out;

smsc911x_soft_reset() also returns an error code you should use.

This patch also seems to do quite a few different things. Please can
you break it up into multiple patches.

	

	That was the comment from the previous patch, but It confused me because it was only really moving the MDIO startup, the rest was a side effect of that and atomic to it (AKA it wasn't clear how to make it smaller). I read it as Steve misinterpreting the laundry list of fixes as things being individually fixed, rather than one thing fixing a bunch of stuff.

This patch does add additional code I overlooked to cleanup the phy if it fails, I guess in theory that portion could be a prereq patch, I will break that portion out. I'm still not sure how to partially move the MDIO startup...

Hi Andrew,

	Thanks for taking a look at this!

On 09/01/2016 12:06 PM, Andrew Lunn wrote:

Hi Jeremy

Please don't add forward references. Move the function earlier in the
file.

Ok, but I thought it was a fairly large move due to further dependent functions..

quoted

 static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
 {
 	if (pdata->config.flags & SMSC911X_USE_32BIT)

@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
 	unsigned int temp;
 	unsigned int intcfg;
 	int retval;
+	int irq_flags;

 	/* find and start the given phy */
 	if (!dev->phydev) {

@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
 	pdata->software_irq_signal = 0;
 	smp_wmb();

+	irq_flags = irq_get_trigger_type(dev->irq);
+	if (request_irq(dev->irq, smsc911x_irqhandler,
+			irq_flags | IRQF_SHARED, dev->name, dev)) {
+		SMSC_WARN(pdata, probe,
+			  "Unable to claim requested irq: %d", dev->irq);
+		goto mii_free_out;
+	}
+
 	temp = smsc911x_reg_read(pdata, INT_EN);
 	temp |= INT_EN_SW_INT_EN_;
 	smsc911x_reg_write(pdata, INT_EN, temp);

@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
 		netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
 			    dev->irq);
 		retval = -ENODEV;
-		goto mii_free_out;
+		goto irq_stop_out;
 	}
 	SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
 		   dev->irq);

@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)

 	netif_start_queue(dev);
 	return 0;
-
+irq_stop_out:
+	free_irq(dev->irq, dev);
 mii_free_out:
 	phy_disconnect(dev->phydev);
 	dev->phydev = NULL;

@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
 	dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
 	smsc911x_tx_update_txcounters(dev);

+	free_irq(dev->irq, dev);
+
 	/* Bring the PHY down */
 	if (dev->phydev) {
 		phy_stop(dev->phydev);
 		phy_disconnect(dev->phydev);
 		dev->phydev = NULL;
 	}
+	netif_carrier_off(dev);

What has this change got to do with interrupt handling?

	This is a whoops, it should be in the previous patch..

quoted

@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 	if (retval < 0)
 		goto out_disable_resources;

-	/* configure irq polarity and type before connecting isr */
-	if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-		intcfg |= INT_CFG_IRQ_POL_;
-
-	if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-		intcfg |= INT_CFG_IRQ_TYPE_;
-
-	smsc911x_reg_write(pdata, INT_CFG, intcfg);

I see these removes, but where are the adds?

	The functionality is duplicated in open, when the IRQ handler is tested.

On Thu, Sep 01, 2016 at 01:47:44PM -0500, Jeremy Linton wrote:

Hi Andrew,

	Thanks for taking a look at this!

On 09/01/2016 12:06 PM, Andrew Lunn wrote:

quoted

Hi Jeremy

Please don't add forward references. Move the function earlier in the
file.

Ok, but I thought it was a fairly large move due to further
dependent functions..

There are a few other options, like moving smsc911x_open() rather than
the interrupt handler.

And i would suggest what ever you do, make it a separate patch. A
patch which says: No functional changes, just move functions around as
needed by later patches, is going to be quick and easy to review.

quoted

quoted

+	netif_carrier_off(dev);

What has this change got to do with interrupt handling?

	This is a whoops, it should be in the previous patch..

Or a patch of its own? You also needs to be careful with ordering
against the phy_connect.

quoted

quoted

@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device *pdev)

	if (retval < 0)
		goto out_disable_resources;

-	/* configure irq polarity and type before connecting isr */
-	if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-		intcfg |= INT_CFG_IRQ_POL_;
-
-	if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-		intcfg |= INT_CFG_IRQ_TYPE_;
-
-	smsc911x_reg_write(pdata, INT_CFG, intcfg);

I see these removes, but where are the adds?

	The functionality is duplicated in open, when the IRQ handler is tested.

Ah, it is obfusticated by SMC_SET_IRQ_CFG().

If you say it is duplicated, how about a separate patch removing it,
with a clear pointer to where the duplicate is.

     Andrew

This patch does add additional code I overlooked to cleanup the phy
if it fails, I guess in theory that portion could be a prereq patch,
I will break that portion out. I'm still not sure how to partially
move the MDIO startup...

Hi Jeremy

You can add a cleanup patch which replaces these hard coded return
values with the value returned by the function calls. Then do the
move.

In general, it is easier to review lots of small obvious patches, than
one big patch with lots of things going on.

    Andrew

Hi Jeremy 

Please don't add forward references. Move the function earlier in the
file.

What has this change got to do with interrupt handling?

I see these removes, but where are the adds?

  Andrew

smsc911x_mii_probe() returns an error code. It is better to use that,
than -EAGAIN, which is rather odd to start with.

smsc911x_soft_reset() also returns an error code you should use.

This patch also seems to do quite a few different things. Please can
you break it up into multiple patches.

Thanks
	Andrew