| Message ID | 5dd177ee-f4e8-47b0-aeb8-d2fa64d3725b@mary.at.omicron.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Uh, wrong subject, this should of course read net: davinci_emac: Move call of devm_request_irq to davinci_emac_probe() --On March 04, 2014 15:07 +0100 Christian Riesch <christian.riesch@omicron.at> wrote: > In commit 6892b41d9701283085b655c6086fb57a5d63fa47 > > Author: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Date: Tue Jun 25 21:24:51 2013 +0530 > net: davinci: emac: Convert to devm_* api > > the call of request_irq is replaced by devm_request_irq and the call > of free_irq is removed. But since interrupts are requested in > emac_dev_open, doing ifconfig up/down on the board requests the > interrupts again each time, causing devm_request_irq to fail. > > This patch moves the interrupt requests to emac_dev_open and thus > fixes this regression. And of course: "This patch moves the interrupt requests to davinci_emac_probe() and thus fixes this regression. I will fix that in the next version of the patch. Christian > > Reported-by: Jon Ringle <jon@ringle.org> > Signed-off-by: Christian Riesch <christian.riesch@omicron.at> > Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Cc: <stable@vger.kernel.org> > --- > > Hi, > This is an attempt to fix the bug discussed in > https://lkml.org/lkml/2014/3/4/218 > > Since I am not really familiar with interrupts I am not sure if this is > the right way to do it. I am looking forward to your comments. > > Christian > > drivers/net/ethernet/ti/davinci_emac.c | 32 > ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 > deletions(-) > > diff --git a/drivers/net/ethernet/ti/davinci_emac.c > b/drivers/net/ethernet/ti/davinci_emac.c index cd9b164..97c6036 100644 > --- a/drivers/net/ethernet/ti/davinci_emac.c > +++ b/drivers/net/ethernet/ti/davinci_emac.c > @@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev) > { > struct device *emac_dev = &ndev->dev; > u32 cnt; > - struct resource *res; > int ret; > int i = 0; > - int k = 0; > struct emac_priv *priv = netdev_priv(ndev); > > pm_runtime_get(&priv->pdev->dev); > @@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev) > break; > } > > - /* Request IRQ */ > - > - while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { > - for (i = res->start; i <= res->end; i++) { > - if (devm_request_irq(&priv->pdev->dev, i, emac_irq, > - 0, ndev->name, ndev)) > - goto rollback; > - } > - k++; > - } > > /* Start/Enable EMAC hardware */ > emac_hw_enable(priv); > @@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev) > > return 0; > > -rollback: > - > - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); > - ret = -EBUSY; > err: > pm_runtime_put(&priv->pdev->dev); > return ret; > @@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct > platform_device *pdev) struct clk *emac_clk; > unsigned long emac_bus_frequency; > > + int k = 0; > + int i = 0; > > /* obtain emac clock from kernel */ > emac_clk = devm_clk_get(&pdev->dev, NULL); > @@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct > platform_device *pdev) (void *)priv->emac_base_phys, ndev->irq); > } > > + /* Request IRQ */ > + while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { > + for (i = res->start; i <= res->end; i++) { > + if (devm_request_irq(&priv->pdev->dev, i, emac_irq, > + 0, ndev->name, ndev)) > + goto no_irq; > + } > + k++; > + } > + > pm_runtime_enable(&pdev->dev); > pm_runtime_resume(&pdev->dev); > > return 0; > > +no_irq: > + dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); > + rc = -EBUSY; > + > no_cpdma_chan: > if (priv->txchan) > cpdma_chan_destroy(priv->txchan); > -- > 1.7.9.5 > > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Hi Christian, Thanks for the patch. On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch <christian.riesch@omicron.at> wrote: > In commit 6892b41d9701283085b655c6086fb57a5d63fa47 > > Author: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Date: Tue Jun 25 21:24:51 2013 +0530 > net: davinci: emac: Convert to devm_* api > > the call of request_irq is replaced by devm_request_irq and the call > of free_irq is removed. But since interrupts are requested in > emac_dev_open, doing ifconfig up/down on the board requests the > interrupts again each time, causing devm_request_irq to fail. > > This patch moves the interrupt requests to emac_dev_open and thus > fixes this regression. > > Reported-by: Jon Ringle <jon@ringle.org> > Signed-off-by: Christian Riesch <christian.riesch@omicron.at> > Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Cc: <stable@vger.kernel.org> This patch fixes issue pointed at http://comments.gmane.org/gmane.linux.davinci/28135 tested on Logic PD. Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Assuming you will respin the patch fixing the header. > --- > > Hi, > This is an attempt to fix the bug discussed in > https://lkml.org/lkml/2014/3/4/218 > > Since I am not really familiar with interrupts I am not sure if this is the right way to > do it. I am looking forward to your comments. > Looks OK. Thanks, --Prabhakar Lad > Christian > > drivers/net/ethernet/ti/davinci_emac.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c > index cd9b164..97c6036 100644 > --- a/drivers/net/ethernet/ti/davinci_emac.c > +++ b/drivers/net/ethernet/ti/davinci_emac.c > @@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev) > { > struct device *emac_dev = &ndev->dev; > u32 cnt; > - struct resource *res; > int ret; > int i = 0; > - int k = 0; > struct emac_priv *priv = netdev_priv(ndev); > > pm_runtime_get(&priv->pdev->dev); > @@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev) > break; > } > > - /* Request IRQ */ > - > - while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { > - for (i = res->start; i <= res->end; i++) { > - if (devm_request_irq(&priv->pdev->dev, i, emac_irq, > - 0, ndev->name, ndev)) > - goto rollback; > - } > - k++; > - } > > /* Start/Enable EMAC hardware */ > emac_hw_enable(priv); > @@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev) > > return 0; > > -rollback: > - > - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); > - ret = -EBUSY; > err: > pm_runtime_put(&priv->pdev->dev); > return ret; > @@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct platform_device *pdev) > struct clk *emac_clk; > unsigned long emac_bus_frequency; > > + int k = 0; > + int i = 0; > > /* obtain emac clock from kernel */ > emac_clk = devm_clk_get(&pdev->dev, NULL); > @@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct platform_device *pdev) > (void *)priv->emac_base_phys, ndev->irq); > } > > + /* Request IRQ */ > + while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { > + for (i = res->start; i <= res->end; i++) { > + if (devm_request_irq(&priv->pdev->dev, i, emac_irq, > + 0, ndev->name, ndev)) > + goto no_irq; > + } > + k++; > + } > + > pm_runtime_enable(&pdev->dev); > pm_runtime_resume(&pdev->dev); > > return 0; > > +no_irq: > + dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); > + rc = -EBUSY; > + > no_cpdma_chan: > if (priv->txchan) > cpdma_chan_destroy(priv->txchan); > -- > 1.7.9.5 >
2014-03-04 9:30 GMT-08:00 Prabhakar Lad <prabhakar.csengg@gmail.com>: > Hi Christian, > > Thanks for the patch. > > > On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch > <christian.riesch@omicron.at> wrote: >> In commit 6892b41d9701283085b655c6086fb57a5d63fa47 >> >> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> Date: Tue Jun 25 21:24:51 2013 +0530 >> net: davinci: emac: Convert to devm_* api >> >> the call of request_irq is replaced by devm_request_irq and the call >> of free_irq is removed. But since interrupts are requested in >> emac_dev_open, doing ifconfig up/down on the board requests the >> interrupts again each time, causing devm_request_irq to fail. >> >> This patch moves the interrupt requests to emac_dev_open and thus >> fixes this regression. >> >> Reported-by: Jon Ringle <jon@ringle.org> >> Signed-off-by: Christian Riesch <christian.riesch@omicron.at> >> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> Cc: <stable@vger.kernel.org> > > This patch fixes issue pointed at > http://comments.gmane.org/gmane.linux.davinci/28135 > tested on Logic PD. > > Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Assuming you will respin the patch fixing the header. >> --- >> >> Hi, >> This is an attempt to fix the bug discussed in >> https://lkml.org/lkml/2014/3/4/218 >> >> Since I am not really familiar with interrupts I am not sure if this is the right way to >> do it. I am looking forward to your comments. >> > Looks OK. I think a plain revert of Prabhakar patch would bring us in an "usual" situation where the drivers's ndo_open(): - masks all interrupts bits at the Ethernet MAC level - calls request_irq() - unmask relevant interrupts bits at the Ethernet Mac level just in case some uncontrolled interrupt bit triggers an interrupt while the interface is down for instance... On a wider note, it would be good to get some tool to notify people that attempting to replace devm_request_irq() in a network driver's ndo_open() function is not going to produce the expected results as the two are not strictly identical.
--On March 04, 2014 09:53 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > 2014-03-04 9:30 GMT-08:00 Prabhakar Lad <prabhakar.csengg@gmail.com>: >> Hi Christian, >> >> Thanks for the patch. >> >> >> On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch >> <christian.riesch@omicron.at> wrote: >>> In commit 6892b41d9701283085b655c6086fb57a5d63fa47 >>> >>> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com> >>> Date: Tue Jun 25 21:24:51 2013 +0530 >>> net: davinci: emac: Convert to devm_* api >>> >>> the call of request_irq is replaced by devm_request_irq and the call >>> of free_irq is removed. But since interrupts are requested in >>> emac_dev_open, doing ifconfig up/down on the board requests the >>> interrupts again each time, causing devm_request_irq to fail. >>> >>> This patch moves the interrupt requests to emac_dev_open and thus >>> fixes this regression. >>> >>> Reported-by: Jon Ringle <jon@ringle.org> >>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at> >>> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com> >>> Cc: <stable@vger.kernel.org> >> >> This patch fixes issue pointed at >> http://comments.gmane.org/gmane.linux.davinci/28135 >> tested on Logic PD. >> >> Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> >> Assuming you will respin the patch fixing the header. >>> --- >>> >>> Hi, >>> This is an attempt to fix the bug discussed in >>> https://lkml.org/lkml/2014/3/4/218 >>> >>> Since I am not really familiar with interrupts I am not sure if this is >>> the right way to do it. I am looking forward to your comments. >>> >> Looks OK. > > I think a plain revert of Prabhakar patch would bring us in an "usual" > situation where the drivers's ndo_open(): > > - masks all interrupts bits at the Ethernet MAC level > - calls request_irq() > - unmask relevant interrupts bits at the Ethernet Mac level > > just in case some uncontrolled interrupt bit triggers an interrupt > while the interface is down for instance... I am fine with either solution and I will be happy to prepare a patch if we agree that we should revert the patch. What was the reason for commit 6892b41d9701283085b655c6086fb57a5d63fa47 in the first place? Does it fix a problem? Should the full commit 6892b41d9701283085b655c6086fb57a5d63fa47 be reverted? Actually it consists of two parts, the first part replaces request_irq with devm_request_irq and throws out free_irq, the second part replaces devm_request_mem_region/devm_ioremap with devm_ioremap_resource. The first part causes the problem. Shall we keep the second part or shall this be reverted as well? Christian > > On a wider note, it would be good to get some tool to notify people > that attempting to replace devm_request_irq() in a network driver's > ndo_open() function is not going to produce the expected results as > the two are not strictly identical. > -- > Florian > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c index cd9b164..97c6036 100644 --- a/drivers/net/ethernet/ti/davinci_emac.c +++ b/drivers/net/ethernet/ti/davinci_emac.c @@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev) { struct device *emac_dev = &ndev->dev; u32 cnt; - struct resource *res; int ret; int i = 0; - int k = 0; struct emac_priv *priv = netdev_priv(ndev); pm_runtime_get(&priv->pdev->dev); @@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev) break; } - /* Request IRQ */ - - while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { - for (i = res->start; i <= res->end; i++) { - if (devm_request_irq(&priv->pdev->dev, i, emac_irq, - 0, ndev->name, ndev)) - goto rollback; - } - k++; - } /* Start/Enable EMAC hardware */ emac_hw_enable(priv); @@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev) return 0; -rollback: - - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); - ret = -EBUSY; err: pm_runtime_put(&priv->pdev->dev); return ret; @@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct platform_device *pdev) struct clk *emac_clk; unsigned long emac_bus_frequency; + int k = 0; + int i = 0; /* obtain emac clock from kernel */ emac_clk = devm_clk_get(&pdev->dev, NULL); @@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct platform_device *pdev) (void *)priv->emac_base_phys, ndev->irq); } + /* Request IRQ */ + while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) { + for (i = res->start; i <= res->end; i++) { + if (devm_request_irq(&priv->pdev->dev, i, emac_irq, + 0, ndev->name, ndev)) + goto no_irq; + } + k++; + } + pm_runtime_enable(&pdev->dev); pm_runtime_resume(&pdev->dev); return 0; +no_irq: + dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed"); + rc = -EBUSY; + no_cpdma_chan: if (priv->txchan) cpdma_chan_destroy(priv->txchan);
In commit 6892b41d9701283085b655c6086fb57a5d63fa47 Author: Lad, Prabhakar <prabhakar.csengg@gmail.com> Date: Tue Jun 25 21:24:51 2013 +0530 net: davinci: emac: Convert to devm_* api the call of request_irq is replaced by devm_request_irq and the call of free_irq is removed. But since interrupts are requested in emac_dev_open, doing ifconfig up/down on the board requests the interrupts again each time, causing devm_request_irq to fail. This patch moves the interrupt requests to emac_dev_open and thus fixes this regression. Reported-by: Jon Ringle <jon@ringle.org> Signed-off-by: Christian Riesch <christian.riesch@omicron.at> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com> Cc: <stable@vger.kernel.org> --- Hi, This is an attempt to fix the bug discussed in https://lkml.org/lkml/2014/3/4/218 Since I am not really familiar with interrupts I am not sure if this is the right way to do it. I am looking forward to your comments. Christian drivers/net/ethernet/ti/davinci_emac.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)