| Message ID | 1438617011-19073-1-git-send-email-l.stach@pengutronix.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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@pengutronix.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? 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. Andrew -- 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
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@pengutronix.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? > > 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.
From: Lucas Stach <l.stach@pengutronix.de> Date: Mon, 3 Aug 2015 17:50:11 +0200 > 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@pengutronix.de> > --- > Please apply this as a fix for 4.2 I'm waiting for feedback to be given wrt. the runtime-pm issues. -- 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
Am Montag, den 03.08.2015, 18:15 +0200 schrieb Andrew Lunn: > 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@pengutronix.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? > > 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. > It is the correct fix. pm_runtime_enable() is the transition point between whatever state the device was in and the runtime PM managed state. pm_runtime_set_active() informs the RPM framework about the current device state. pm_runtime_get_noresume() tells the RPM framework what state we want the device to be in after the transition to RPM managed state. We expect the device to stay on while we go through the probe() routine, so we need to get the runtime PM refcount, otherwise it would be fine for RPM to turn the device off immediately after calling pm_runtime_enable(). We drop the refcount when leaving the probe routine. Regards, Lucas
Hello Lucas, 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@pengutronix.de> the two \n after "Fixes:" and before the S-o-b are unusual I think. Other than that I tested the change on i.MX27 and it works now. The power refcount oscillates between 1 and 2 now as expected and booting with NFS-root works fine. So: Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe
From: Lucas Stach <l.stach@pengutronix.de> Date: Mon, 3 Aug 2015 17:50:11 +0200 > 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@pengutronix.de> Applied, thanks. -- 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/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);
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@pengutronix.de> --- Please apply this as a fix for 4.2 --- drivers/net/ethernet/freescale/fec_main.c | 1 + 1 file changed, 1 insertion(+)