| Message ID | 1395400154-25710-1-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Ben, On Fri, Mar 21, 2014 at 12:09 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > The pm_rumtime work queue is causing the device to be suspended during > initialisation, thus the initialisation may not be able to access registers > properly. As the code is called from a work queue, it is possible that this > is not seen from certain configurations/builds due to the asynchronos > nature of the code. > > Another issue has also been found where the network device registration > calls back into the driver thus causing further pm_runtime calls that > also caused issues with the MDIO bus code. This has now been checked > and is the only place the MDIO can be called without the device open. > > Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the > pm system does not suspend it during the probe() call and remove the > now unnecessary pm_runtime_resume() call. Also add a call in the error > path to call pm_runtime_disable(). > > This fixes the external abort that can cause /sbin/init or other such > init processed to die. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Thanks, still works! > --- > Cc: sergei.shtylyov@cogentembedded.com > Cc: laurent.pinchart+renesas@ideasonboard.com > cc: netdev@vger.kernel.org > > Note, Laurent this should probably be applied before your > current series as it may require changes to the exit sequence. I guess this should read: " Fixed from v4: - rebased on top of netdev-next" ? > Fixes from v1: > - use pm_runtime_put() over pm_runtime_put_sync() as > we do not need to guaranteed the device has powered > off after probe. > > Fixed from v2: > - call pm_runtime_put() in error path > > Fixed from v3: > - call pm_runtime_disable() in error path Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, Thank you for the patch. On Friday 21 March 2014 12:09:14 Ben Dooks wrote: > The pm_rumtime work queue is causing the device to be suspended during > initialisation, thus the initialisation may not be able to access registers > properly. As the code is called from a work queue, it is possible that this > is not seen from certain configurations/builds due to the asynchronos > nature of the code. > > Another issue has also been found where the network device registration > calls back into the driver thus causing further pm_runtime calls that > also caused issues with the MDIO bus code. This has now been checked > and is the only place the MDIO can be called without the device open. > > Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the > pm system does not suspend it during the probe() call and remove the > now unnecessary pm_runtime_resume() call. Also add a call in the error > path to call pm_runtime_disable(). > > This fixes the external abort that can cause /sbin/init or other such > init processed to die. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > Cc: sergei.shtylyov@cogentembedded.com > Cc: laurent.pinchart+renesas@ideasonboard.com > cc: netdev@vger.kernel.org > > Note, Laurent this should probably be applied before your > current series as it may require changes to the exit sequence. > > Fixes from v1: > - use pm_runtime_put() over pm_runtime_put_sync() as > we do not need to guaranteed the device has powered > off after probe. > > Fixed from v2: > - call pm_runtime_put() in error path > > Fixed from v3: > - call pm_runtime_disable() in error path > > Conflicts: > drivers/net/ethernet/renesas/sh_eth.c > --- > drivers/net/ethernet/renesas/sh_eth.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c > b/drivers/net/ethernet/renesas/sh_eth.c index e4bff18..6a9509c 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -2772,6 +2772,9 @@ static int sh_eth_drv_probe(struct platform_device > *pdev) if (!ndev) > return -ENOMEM; > > + pm_runtime_enable(&pdev->dev); > + pm_runtime_get_sync(&pdev->dev); > + > /* The sh Ether-specific entries in the device structure. */ > ndev->base_addr = res->start; > devno = pdev->id; > @@ -2799,8 +2802,6 @@ static int sh_eth_drv_probe(struct platform_device > *pdev) > > spin_lock_init(&mdp->lock); > mdp->pdev = pdev; > - pm_runtime_enable(&pdev->dev); > - pm_runtime_resume(&pdev->dev); > > if (pdev->dev.of_node) > pd = sh_eth_parse_dt(&pdev->dev); > @@ -2898,6 +2899,7 @@ static int sh_eth_drv_probe(struct platform_device > *pdev) netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n", > (u32)ndev->base_addr, ndev->dev_addr, ndev->irq); > > + pm_runtime_put(&pdev->dev); > platform_set_drvdata(pdev, ndev); > > return ret; > @@ -2911,6 +2913,8 @@ out_release: > if (ndev) > free_netdev(ndev); > > + pm_runtime_put(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > return ret; > }
From: Ben Dooks <ben.dooks@codethink.co.uk> Date: Fri, 21 Mar 2014 12:09:14 +0100 > The pm_rumtime work queue is causing the device to be suspended during > initialisation, thus the initialisation may not be able to access registers > properly. As the code is called from a work queue, it is possible that this > is not seen from certain configurations/builds due to the asynchronos > nature of the code. > > Another issue has also been found where the network device registration > calls back into the driver thus causing further pm_runtime calls that > also caused issues with the MDIO bus code. This has now been checked > and is the only place the MDIO can be called without the device open. > > Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the > pm system does not suspend it during the probe() call and remove the > now unnecessary pm_runtime_resume() call. Also add a call in the error > path to call pm_runtime_disable(). > > This fixes the external abort that can cause /sbin/init or other such > init processed to die. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> What tree is this against? It doesn't apply cleanly to 'net'. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/03/14 18:26, David Miller wrote: > From: Ben Dooks <ben.dooks@codethink.co.uk> > Date: Fri, 21 Mar 2014 12:09:14 +0100 > >> The pm_rumtime work queue is causing the device to be suspended during >> initialisation, thus the initialisation may not be able to access registers >> properly. As the code is called from a work queue, it is possible that this >> is not seen from certain configurations/builds due to the asynchronos >> nature of the code. >> >> Another issue has also been found where the network device registration >> calls back into the driver thus causing further pm_runtime calls that >> also caused issues with the MDIO bus code. This has now been checked >> and is the only place the MDIO can be called without the device open. >> >> Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the >> pm system does not suspend it during the probe() call and remove the >> now unnecessary pm_runtime_resume() call. Also add a call in the error >> path to call pm_runtime_disable(). >> >> This fixes the external abort that can cause /sbin/init or other such >> init processed to die. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> > > What tree is this against? It doesn't apply cleanly to 'net'. I thought it was against the net next tree, given the number of patches that are currently being applied to the sh_eth driver.
From: Ben Dooks <ben.dooks@codethink.co.uk> Date: Fri, 28 Mar 2014 19:25:51 +0000 > I thought it was against the net next tree, given the number of > patches that are currently being applied to the sh_eth driver. Fair enough, applied to net-next, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 03/28/2014 11:00 PM, David Miller wrote: >> I thought it was against the net next tree, given the number of >> patches that are currently being applied to the sh_eth driver. > Fair enough, applied to net-next, thanks! It probably makes sense to queue this for the stable kernels as well. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sat, 29 Mar 2014 00:10:20 +0300 > Hello. > > On 03/28/2014 11:00 PM, David Miller wrote: > >>> I thought it was against the net next tree, given the number of >>> patches that are currently being applied to the sh_eth driver. > >> Fair enough, applied to net-next, thanks! > > It probably makes sense to queue this for the stable kernels as well. Sorry, that's not how this works. If it's good enough for -stable, meaning that users are activly hitting the problem and it's a serious bug, then it's good enough for 'net' and should have been submitted against 'net'. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 29-03-2014 1:18, David Miller wrote: >>>> I thought it was against the net next tree, given the number of >>>> patches that are currently being applied to the sh_eth driver. >>> Fair enough, applied to net-next, thanks! >> It probably makes sense to queue this for the stable kernels as well. > Sorry, that's not how this works. > If it's good enough for -stable, meaning that users are activly hitting > the problem and it's a serious bug, then it's good enough for 'net' > and should have been submitted against 'net'. I thought that at this point only regression fixes are good for 'net'. Although, this can be considered a regression too -- since addition of runtime PM support back in 2009. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sat, 29 Mar 2014 17:52:07 +0400 > Hello. > > On 29-03-2014 1:18, David Miller wrote: > >>>>> I thought it was against the net next tree, given the number of >>>>> patches that are currently being applied to the sh_eth driver. > >>>> Fair enough, applied to net-next, thanks! > >>> It probably makes sense to queue this for the stable kernels as well. > >> Sorry, that's not how this works. > >> If it's good enough for -stable, meaning that users are activly >> hitting >> the problem and it's a serious bug, then it's good enough for 'net' >> and should have been submitted against 'net'. > > I thought that at this point only regression fixes are good for 'net'. > Although, this can be considered a regression too -- since addition of > runtime PM support back in 2009. -stable has more stringent requirements for inclusion than 'net', therefore it is never valid for something to go -stable which is not in 'net'. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index e4bff18..6a9509c 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -2772,6 +2772,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev) if (!ndev) return -ENOMEM; + pm_runtime_enable(&pdev->dev); + pm_runtime_get_sync(&pdev->dev); + /* The sh Ether-specific entries in the device structure. */ ndev->base_addr = res->start; devno = pdev->id; @@ -2799,8 +2802,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev) spin_lock_init(&mdp->lock); mdp->pdev = pdev; - pm_runtime_enable(&pdev->dev); - pm_runtime_resume(&pdev->dev); if (pdev->dev.of_node) pd = sh_eth_parse_dt(&pdev->dev); @@ -2898,6 +2899,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev) netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n", (u32)ndev->base_addr, ndev->dev_addr, ndev->irq); + pm_runtime_put(&pdev->dev); platform_set_drvdata(pdev, ndev); return ret; @@ -2911,6 +2913,8 @@ out_release: if (ndev) free_netdev(ndev); + pm_runtime_put(&pdev->dev); + pm_runtime_disable(&pdev->dev); return ret; }