lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 3 Aug 2015 14:28:58 -0400 (EDT) From: Alan Stern <stern@...land.harvard.edu> To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de> cc: Andrew Lunn <andrew@...n.ch>, Lucas Stach <l.stach@...gutronix.de>, <netdev@...r.kernel.org>, <patchwork-lst@...gutronix.de>, "David S. Miller" <davem@...emloft.net>, <kernel@...gutronix.de>, "Rafael J. Wysocki" <rjw@...ysocki.net>, Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>, <linux-pm@...r.kernel.org> Subject: Re: [PATCH] net: fec: fix initial runtime PM refcount On Mon, 3 Aug 2015, Uwe [iso-8859-1] Kleine-König wrote: > Hello, > > I have no clue about runtime-pm, but I added a few people to Cc: who > should know better ... > > Best regards > Uwe > > On Mon, Aug 03, 2015 at 06:15:54PM +0200, Andrew Lunn wrote: > > On Mon, Aug 03, 2015 at 05:50:11PM +0200, Lucas Stach wrote: > > > The clocks are initially active and thus the device is marked active. > > > This still keeps the PM refcount at 0, the pm_runtime_put_autosuspend() > > > call at the end of probe then leaves us with an invalid refcount of -1, > > > which in turn leads to the device staying in suspended state even though > > > netdev open had been called. > > > > > > Fix this by initializing the refcount to be coherent with the initial > > > device status. > > > > > > Fixes: > > > 8fff755e9f8 (net: fec: Ensure clocks are enabled while using mdio bus) > > > > > > Signed-off-by: Lucas Stach <l.stach@...gutronix.de> > > > --- > > > Please apply this as a fix for 4.2 > > > --- > > > drivers/net/ethernet/freescale/fec_main.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > > > index 32e3807c650e..271bb5862346 100644 > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > @@ -3433,6 +3433,7 @@ fec_probe(struct platform_device *pdev) > > > > > > pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT); > > > pm_runtime_use_autosuspend(&pdev->dev); > > > + pm_runtime_get_noresume(&pdev->dev); > > > pm_runtime_set_active(&pdev->dev); > > > pm_runtime_enable(&pdev->dev); > > > > This might work, but is it the correct fix? It looks reasonable to me. It might also make sense to move all of that pm_runtime_* stuff to the end of the probe routine. Notice that they don't get undone if register_netdev() fails. > > Documentation/power/runtime_pm.txt says: > > > > 534 In addition to that, the initial runtime PM status of all devices is > > 535 'suspended', but it need not reflect the actual physical state of the device. > > 536 Thus, if the device is initially active (i.e. it is able to process I/O), its > > 537 runtime PM status must be changed to 'active', with the help of > > 538 pm_runtime_set_active(), before pm_runtime_enable() is called for the device. > > > > At the point we call the pm_runtime_ functions above, all the clocks > > are ticking. So according to the documentation pm_runtime_set_active() > > is the right thing to do. But it makes no mention of have to call > > pm_runtime_get_noresume(). I would of expected pm_runtime_set_active() > > to set the count to the correct value. pm_runtime_set_active() doesn't change the usage count. All it does is set the runtime PM status to "active". A call to pm_runtime_get_noresume() (or something similar) is necessary to balance the call to pm_runtime_put_autosuspend() at the end of the probe routine. Both the _get_ and the _put_ should be present or neither should be. For instance, an alternative way to accomplish the same result is to replace pm_runtime_put_autosuspend() with pm_runtime_autosuspend(). The only difference is that the usage counter would not be elevated during the register_netdev() call, so in theory the device could be suspended while that routine is running. But if all the pm_runtime_* calls were moved to the end of the probe function, even that couldn't happen. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists