* [PATCH 1/4] net: smsc911x: Remove multiple exit points from smsc911x_open
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
@ 2016-09-01 20:15 ` Jeremy Linton
2016-09-01 20:43 ` Andrew Lunn
2016-09-01 20:15 ` [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering Jeremy Linton
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Jeremy Linton @ 2016-09-01 20:15 UTC (permalink / raw)
To: netdev; +Cc: steve.glendinning, sergei.shtylyov, andrew, will.deacon
Rework the error handling in smsc911x open in preparation
for the mdio startup being moved here.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..c9b0e05 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1520,17 +1520,20 @@ 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*/
if (!dev->phydev) {
SMSC_WARN(pdata, hw, "phy_dev is NULL");
- return -EAGAIN;
+ retval = -EAGAIN;
+ goto out;
}
/* Reset the LAN911x */
- if (smsc911x_soft_reset(pdata)) {
+ retval = smsc911x_soft_reset(pdata);
+ if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
- return -EIO;
+ goto out;
}
smsc911x_reg_write(pdata, HW_CFG, 0x00050000);
@@ -1600,7 +1603,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 out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1646,6 +1650,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
+out:
+ return retval;
}
/* Entry point for stopping the interface */
--
2.5.5
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
2016-09-01 20:15 ` [PATCH 1/4] net: smsc911x: Remove multiple exit points from smsc911x_open Jeremy Linton
@ 2016-09-01 20:15 ` Jeremy Linton
2016-09-01 20:49 ` Andrew Lunn
2016-09-03 21:19 ` Sergei Shtylyov
2016-09-01 20:15 ` [PATCH 3/4] net: smsc911x: Move interrupt handler before open Jeremy Linton
` (3 subsequent siblings)
5 siblings, 2 replies; 16+ messages in thread
From: Jeremy Linton @ 2016-09-01 20:15 UTC (permalink / raw)
To: netdev; +Cc: steve.glendinning, sergei.shtylyov, andrew, will.deacon
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 <jeremy.linton@arm.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 48 ++++++++++++++++++------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index c9b0e05..823ad3f 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:
@@ -1522,18 +1515,20 @@ static int smsc911x_open(struct net_device *dev)
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");
- retval = -EAGAIN;
- goto out;
+ retval = smsc911x_mii_probe(dev);
+ if (retval < 0) {
+ SMSC_WARN(pdata, probe, "Error starting phy");
+ goto out;
+ }
}
/* Reset the LAN911x */
retval = smsc911x_soft_reset(pdata);
if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
- goto out;
+ goto mii_free_out;
}
smsc911x_reg_write(pdata, HW_CFG, 0x00050000);
@@ -1604,7 +1599,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
- goto out;
+ goto mii_free_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1650,6 +1645,10 @@ 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;
}
@@ -1674,8 +1673,12 @@ 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;
+ }
+ netif_carrier_off(dev);
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2297,11 +2300,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);
@@ -2500,6 +2502,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);
@@ -2509,12 +2517,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 */
@@ -2550,8 +2552,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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
2016-09-01 20:15 ` [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering Jeremy Linton
@ 2016-09-01 20:49 ` Andrew Lunn
2016-09-03 21:19 ` Sergei Shtylyov
1 sibling, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2016-09-01 20:49 UTC (permalink / raw)
To: Jeremy Linton; +Cc: netdev, steve.glendinning, sergei.shtylyov, will.deacon
> @@ -1604,7 +1599,7 @@ static int smsc911x_open(struct net_device *dev)
> netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
> dev->irq);
> retval = -ENODEV;
> - goto out;
> + goto mii_free_out;
A nit pick, but it is not really freeing anything.
Maybe you can think of a better name?
Otherwise
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
2016-09-01 20:15 ` [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering Jeremy Linton
2016-09-01 20:49 ` Andrew Lunn
@ 2016-09-03 21:19 ` Sergei Shtylyov
1 sibling, 0 replies; 16+ messages in thread
From: Sergei Shtylyov @ 2016-09-03 21:19 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning, andrew, will.deacon
On 09/01/2016 11:15 PM, Jeremy Linton wrote:
> 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 <jeremy.linton@arm.com>
Wow, this patch has fixed DHCP/NFS boot failure on my Renesas V2H boards!
Thanks again for your work! :-)
MBR, Sergei
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/4] net: smsc911x: Move interrupt handler before open
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
2016-09-01 20:15 ` [PATCH 1/4] net: smsc911x: Remove multiple exit points from smsc911x_open Jeremy Linton
2016-09-01 20:15 ` [PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering Jeremy Linton
@ 2016-09-01 20:15 ` Jeremy Linton
2016-09-01 20:50 ` Andrew Lunn
2016-09-01 20:15 ` [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop Jeremy Linton
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Jeremy Linton @ 2016-09-01 20:15 UTC (permalink / raw)
To: netdev; +Cc: steve.glendinning, sergei.shtylyov, andrew, will.deacon
In preparation for the allocating/enabling interrupts
in the ndo_open routine move the irq handler before it.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 122 +++++++++++++++++------------------
1 file changed, 61 insertions(+), 61 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 823ad3f..c2e56f0 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1507,6 +1507,67 @@ static void smsc911x_disable_irq_chip(struct net_device *dev)
smsc911x_reg_write(pdata, INT_STS, 0xFFFFFFFF);
}
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
+{
+ struct net_device *dev = dev_id;
+ struct smsc911x_data *pdata = netdev_priv(dev);
+ u32 intsts = smsc911x_reg_read(pdata, INT_STS);
+ u32 inten = smsc911x_reg_read(pdata, INT_EN);
+ int serviced = IRQ_NONE;
+ u32 temp;
+
+ if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
+ temp = smsc911x_reg_read(pdata, INT_EN);
+ temp &= (~INT_EN_SW_INT_EN_);
+ smsc911x_reg_write(pdata, INT_EN, temp);
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
+ pdata->software_irq_signal = 1;
+ smp_wmb();
+ serviced = IRQ_HANDLED;
+ }
+
+ if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
+ /* Called when there is a multicast update scheduled and
+ * it is now safe to complete the update */
+ SMSC_TRACE(pdata, intr, "RX Stop interrupt");
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
+ if (pdata->multicast_update_pending)
+ smsc911x_rx_multicast_update_workaround(pdata);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (intsts & inten & INT_STS_TDFA_) {
+ temp = smsc911x_reg_read(pdata, FIFO_INT);
+ temp |= FIFO_INT_TX_AVAIL_LEVEL_;
+ smsc911x_reg_write(pdata, FIFO_INT, temp);
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
+ netif_wake_queue(dev);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (unlikely(intsts & inten & INT_STS_RXE_)) {
+ SMSC_TRACE(pdata, intr, "RX Error interrupt");
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (likely(intsts & inten & INT_STS_RSFL_)) {
+ if (likely(napi_schedule_prep(&pdata->napi))) {
+ /* Disable Rx interrupts */
+ temp = smsc911x_reg_read(pdata, INT_EN);
+ temp &= (~INT_EN_RSFL_EN_);
+ smsc911x_reg_write(pdata, INT_EN, temp);
+ /* Schedule a NAPI poll */
+ __napi_schedule(&pdata->napi);
+ } else {
+ SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
+ }
+ serviced = IRQ_HANDLED;
+ }
+
+ return serviced;
+}
+
static int smsc911x_open(struct net_device *dev)
{
struct smsc911x_data *pdata = netdev_priv(dev);
@@ -1820,67 +1881,6 @@ static void smsc911x_set_multicast_list(struct net_device *dev)
spin_unlock_irqrestore(&pdata->mac_lock, flags);
}
-static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
-{
- struct net_device *dev = dev_id;
- struct smsc911x_data *pdata = netdev_priv(dev);
- u32 intsts = smsc911x_reg_read(pdata, INT_STS);
- u32 inten = smsc911x_reg_read(pdata, INT_EN);
- int serviced = IRQ_NONE;
- u32 temp;
-
- if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
- temp = smsc911x_reg_read(pdata, INT_EN);
- temp &= (~INT_EN_SW_INT_EN_);
- smsc911x_reg_write(pdata, INT_EN, temp);
- smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
- pdata->software_irq_signal = 1;
- smp_wmb();
- serviced = IRQ_HANDLED;
- }
-
- if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
- /* Called when there is a multicast update scheduled and
- * it is now safe to complete the update */
- SMSC_TRACE(pdata, intr, "RX Stop interrupt");
- smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
- if (pdata->multicast_update_pending)
- smsc911x_rx_multicast_update_workaround(pdata);
- serviced = IRQ_HANDLED;
- }
-
- if (intsts & inten & INT_STS_TDFA_) {
- temp = smsc911x_reg_read(pdata, FIFO_INT);
- temp |= FIFO_INT_TX_AVAIL_LEVEL_;
- smsc911x_reg_write(pdata, FIFO_INT, temp);
- smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
- netif_wake_queue(dev);
- serviced = IRQ_HANDLED;
- }
-
- if (unlikely(intsts & inten & INT_STS_RXE_)) {
- SMSC_TRACE(pdata, intr, "RX Error interrupt");
- smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
- serviced = IRQ_HANDLED;
- }
-
- if (likely(intsts & inten & INT_STS_RSFL_)) {
- if (likely(napi_schedule_prep(&pdata->napi))) {
- /* Disable Rx interrupts */
- temp = smsc911x_reg_read(pdata, INT_EN);
- temp &= (~INT_EN_RSFL_EN_);
- smsc911x_reg_write(pdata, INT_EN, temp);
- /* Schedule a NAPI poll */
- __napi_schedule(&pdata->napi);
- } else {
- SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
- }
- serviced = IRQ_HANDLED;
- }
-
- return serviced;
-}
-
#ifdef CONFIG_NET_POLL_CONTROLLER
static void smsc911x_poll_controller(struct net_device *dev)
{
--
2.5.5
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] net: smsc911x: Move interrupt handler before open
2016-09-01 20:15 ` [PATCH 3/4] net: smsc911x: Move interrupt handler before open Jeremy Linton
@ 2016-09-01 20:50 ` Andrew Lunn
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2016-09-01 20:50 UTC (permalink / raw)
To: Jeremy Linton; +Cc: netdev, steve.glendinning, sergei.shtylyov, will.deacon
On Thu, Sep 01, 2016 at 03:15:08PM -0500, Jeremy Linton wrote:
> In preparation for the allocating/enabling interrupts
> in the ndo_open routine move the irq handler before it.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
It would of been nice if you had emphasised there was not functional
change...
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
> ---
> drivers/net/ethernet/smsc/smsc911x.c | 122 +++++++++++++++++------------------
> 1 file changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index 823ad3f..c2e56f0 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -1507,6 +1507,67 @@ static void smsc911x_disable_irq_chip(struct net_device *dev)
> smsc911x_reg_write(pdata, INT_STS, 0xFFFFFFFF);
> }
>
> +static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct smsc911x_data *pdata = netdev_priv(dev);
> + u32 intsts = smsc911x_reg_read(pdata, INT_STS);
> + u32 inten = smsc911x_reg_read(pdata, INT_EN);
> + int serviced = IRQ_NONE;
> + u32 temp;
> +
> + if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
> + temp = smsc911x_reg_read(pdata, INT_EN);
> + temp &= (~INT_EN_SW_INT_EN_);
> + smsc911x_reg_write(pdata, INT_EN, temp);
> + smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
> + pdata->software_irq_signal = 1;
> + smp_wmb();
> + serviced = IRQ_HANDLED;
> + }
> +
> + if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
> + /* Called when there is a multicast update scheduled and
> + * it is now safe to complete the update */
> + SMSC_TRACE(pdata, intr, "RX Stop interrupt");
> + smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
> + if (pdata->multicast_update_pending)
> + smsc911x_rx_multicast_update_workaround(pdata);
> + serviced = IRQ_HANDLED;
> + }
> +
> + if (intsts & inten & INT_STS_TDFA_) {
> + temp = smsc911x_reg_read(pdata, FIFO_INT);
> + temp |= FIFO_INT_TX_AVAIL_LEVEL_;
> + smsc911x_reg_write(pdata, FIFO_INT, temp);
> + smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
> + netif_wake_queue(dev);
> + serviced = IRQ_HANDLED;
> + }
> +
> + if (unlikely(intsts & inten & INT_STS_RXE_)) {
> + SMSC_TRACE(pdata, intr, "RX Error interrupt");
> + smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
> + serviced = IRQ_HANDLED;
> + }
> +
> + if (likely(intsts & inten & INT_STS_RSFL_)) {
> + if (likely(napi_schedule_prep(&pdata->napi))) {
> + /* Disable Rx interrupts */
> + temp = smsc911x_reg_read(pdata, INT_EN);
> + temp &= (~INT_EN_RSFL_EN_);
> + smsc911x_reg_write(pdata, INT_EN, temp);
> + /* Schedule a NAPI poll */
> + __napi_schedule(&pdata->napi);
> + } else {
> + SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
> + }
> + serviced = IRQ_HANDLED;
> + }
> +
> + return serviced;
> +}
> +
> static int smsc911x_open(struct net_device *dev)
> {
> struct smsc911x_data *pdata = netdev_priv(dev);
> @@ -1820,67 +1881,6 @@ static void smsc911x_set_multicast_list(struct net_device *dev)
> spin_unlock_irqrestore(&pdata->mac_lock, flags);
> }
>
> -static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
> -{
> - struct net_device *dev = dev_id;
> - struct smsc911x_data *pdata = netdev_priv(dev);
> - u32 intsts = smsc911x_reg_read(pdata, INT_STS);
> - u32 inten = smsc911x_reg_read(pdata, INT_EN);
> - int serviced = IRQ_NONE;
> - u32 temp;
> -
> - if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
> - temp = smsc911x_reg_read(pdata, INT_EN);
> - temp &= (~INT_EN_SW_INT_EN_);
> - smsc911x_reg_write(pdata, INT_EN, temp);
> - smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
> - pdata->software_irq_signal = 1;
> - smp_wmb();
> - serviced = IRQ_HANDLED;
> - }
> -
> - if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
> - /* Called when there is a multicast update scheduled and
> - * it is now safe to complete the update */
> - SMSC_TRACE(pdata, intr, "RX Stop interrupt");
> - smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
> - if (pdata->multicast_update_pending)
> - smsc911x_rx_multicast_update_workaround(pdata);
> - serviced = IRQ_HANDLED;
> - }
> -
> - if (intsts & inten & INT_STS_TDFA_) {
> - temp = smsc911x_reg_read(pdata, FIFO_INT);
> - temp |= FIFO_INT_TX_AVAIL_LEVEL_;
> - smsc911x_reg_write(pdata, FIFO_INT, temp);
> - smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
> - netif_wake_queue(dev);
> - serviced = IRQ_HANDLED;
> - }
> -
> - if (unlikely(intsts & inten & INT_STS_RXE_)) {
> - SMSC_TRACE(pdata, intr, "RX Error interrupt");
> - smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
> - serviced = IRQ_HANDLED;
> - }
> -
> - if (likely(intsts & inten & INT_STS_RSFL_)) {
> - if (likely(napi_schedule_prep(&pdata->napi))) {
> - /* Disable Rx interrupts */
> - temp = smsc911x_reg_read(pdata, INT_EN);
> - temp &= (~INT_EN_RSFL_EN_);
> - smsc911x_reg_write(pdata, INT_EN, temp);
> - /* Schedule a NAPI poll */
> - __napi_schedule(&pdata->napi);
> - } else {
> - SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
> - }
> - serviced = IRQ_HANDLED;
> - }
> -
> - return serviced;
> -}
> -
> #ifdef CONFIG_NET_POLL_CONTROLLER
> static void smsc911x_poll_controller(struct net_device *dev)
> {
> --
> 2.5.5
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
` (2 preceding siblings ...)
2016-09-01 20:15 ` [PATCH 3/4] net: smsc911x: Move interrupt handler before open Jeremy Linton
@ 2016-09-01 20:15 ` Jeremy Linton
2016-09-01 20:56 ` Andrew Lunn
` (2 more replies)
2016-09-01 21:07 ` [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Sergei Shtylyov
2016-09-03 0:28 ` David Miller
5 siblings, 3 replies; 16+ messages in thread
From: Jeremy Linton @ 2016-09-01 20:15 UTC (permalink / raw)
To: netdev; +Cc: steve.glendinning, sergei.shtylyov, andrew, will.deacon
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 <will.deacon@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
drivers/net/ethernet/smsc/smsc911x.c | 47 ++++++++++++++----------------------
1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index c2e56f0..4f8910b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1575,6 +1575,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) {
@@ -1645,6 +1646,15 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
+ irq_flags = irq_get_trigger_type(dev->irq);
+ 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 mii_free_out;
+ }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1660,7 +1670,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);
@@ -1707,6 +1717,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;
@@ -1733,6 +1745,8 @@ 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);
@@ -2308,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)
@@ -2393,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,
@@ -2433,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;
@@ -2480,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);
@@ -2552,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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
2016-09-01 20:15 ` [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop Jeremy Linton
@ 2016-09-01 20:56 ` Andrew Lunn
2016-09-02 16:53 ` Will Deacon
2016-09-03 19:23 ` Sergei Shtylyov
2 siblings, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2016-09-01 20:56 UTC (permalink / raw)
To: Jeremy Linton; +Cc: netdev, steve.glendinning, sergei.shtylyov, will.deacon
On Thu, Sep 01, 2016 at 03:15:09PM -0500, Jeremy Linton wrote:
> 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 <will.deacon@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
2016-09-01 20:15 ` [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop Jeremy Linton
2016-09-01 20:56 ` Andrew Lunn
@ 2016-09-02 16:53 ` Will Deacon
2016-09-03 19:23 ` Sergei Shtylyov
2 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2016-09-02 16:53 UTC (permalink / raw)
To: Jeremy Linton; +Cc: netdev, steve.glendinning, sergei.shtylyov, andrew
On Thu, Sep 01, 2016 at 03:15:09PM -0500, Jeremy Linton wrote:
> 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 <will.deacon@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> drivers/net/ethernet/smsc/smsc911x.c | 47 ++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 29 deletions(-)
FWIW, this fixes the interface naming under /proc/<irq num>/ethX for me:
Tested-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
2016-09-01 20:15 ` [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop Jeremy Linton
2016-09-01 20:56 ` Andrew Lunn
2016-09-02 16:53 ` Will Deacon
@ 2016-09-03 19:23 ` Sergei Shtylyov
2016-09-03 20:30 ` Sergei Shtylyov
2 siblings, 1 reply; 16+ messages in thread
From: Sergei Shtylyov @ 2016-09-03 19:23 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning, andrew, will.deacon
On 09/01/2016 11:15 PM, Jeremy Linton wrote:
> 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 <will.deacon@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> drivers/net/ethernet/smsc/smsc911x.c | 47 ++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index c2e56f0..4f8910b 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
[...]
> @@ -2480,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);
> -
Hmm, I didn't see where this code got moved to...
> - 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);
>
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
2016-09-03 19:23 ` Sergei Shtylyov
@ 2016-09-03 20:30 ` Sergei Shtylyov
2016-09-04 2:45 ` Andrew Lunn
0 siblings, 1 reply; 16+ messages in thread
From: Sergei Shtylyov @ 2016-09-03 20:30 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning, andrew, will.deacon
On 09/03/2016 10:23 PM, Sergei Shtylyov wrote:
>> 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 <will.deacon@arm.com>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>> drivers/net/ethernet/smsc/smsc911x.c | 47 ++++++++++++++----------------------
>> 1 file changed, 18 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/smsc/smsc911x.c
>> b/drivers/net/ethernet/smsc/smsc911x.c
>> index c2e56f0..4f8910b 100644
>> --- a/drivers/net/ethernet/smsc/smsc911x.c
>> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> [...]
>> @@ -2480,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);
>> -
>
> Hmm, I didn't see where this code got moved to...
Ah, it's the duplicated code... you should've either explicitly stated
that in the change log or done this in a separate patch, I think.
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
` (3 preceding siblings ...)
2016-09-01 20:15 ` [PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop Jeremy Linton
@ 2016-09-01 21:07 ` Sergei Shtylyov
2016-09-03 0:28 ` David Miller
5 siblings, 0 replies; 16+ messages in thread
From: Sergei Shtylyov @ 2016-09-01 21:07 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning, andrew, will.deacon
On 09/01/2016 11:15 PM, Jeremy Linton wrote:
> v2-v3: Move error handing into separate patch, replace a couple cases
> of fixed errors with the errors being returned from the failing functions.
> Hoist irq handler.
This is normally at end of the blurb, also you're supposed to keep all
version history here.
> 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.
/proc/interrupts maybe?
> Clean up a number of these problems by moving the mdio and interrupt
> configuration into the smsc911x_open routine.
>
> Jeremy Linton (4):
> net: smsc911x: Remove multiple exit points from smsc911x_open
> net: smsc911x: Fix register_netdev, phy startup, driver unload
> ordering
> net: smsc911x: Move interrupt handler before open
> net: smsc911x: Move interrupt allocation to open/stop
Thank you for your insistence on getting this driver straight! I'll try to
review/test your patches.
MBR, Sergei
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config
2016-09-01 20:15 [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Jeremy Linton
` (4 preceding siblings ...)
2016-09-01 21:07 ` [PATCH v3 0/4] net: smsc911x: Move phy and interrupt config Sergei Shtylyov
@ 2016-09-03 0:28 ` David Miller
5 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2016-09-03 0:28 UTC (permalink / raw)
To: jeremy.linton
Cc: netdev, steve.glendinning, sergei.shtylyov, andrew, will.deacon
From: Jeremy Linton <jeremy.linton@arm.com>
Date: Thu, 1 Sep 2016 15:15:05 -0500
> v2-v3: Move error handing into separate patch, replace a couple cases
> of fixed errors with the errors being returned from the failing functions.
> Hoist irq handler.
>
> 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.
Series applied, thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread