Re: [PATCH] net: fec: fix initial runtime PM refcount
From: Lucas Stach l.st...@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.st...@pengutronix.de Applied, thanks. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
On Tue, 4 Aug 2015, Lucas Stach wrote: Am Montag, den 03.08.2015, 14:28 -0400 schrieb Alan Stern: On Mon, 3 Aug 2015, Uwe [iso-8859-1] Kleine-Knig 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.st...@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? 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. Unfortunately we can not move RPM enabling to the end of probe, as the MDIO read/write functions that rely on RPM are already called while we are still in the middle of the probe function. In that case, adding a call pm_runtime_get_noresume() is the right thing to do. I agree that we need better error handling here, but that comment applies to the whole FEC probe function. I think that this might be invasive enough to justify a delay to the next merge window, not really material for the late RCs. That's entirely up to you, of course. Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
Am Montag, den 03.08.2015, 14:28 -0400 schrieb Alan Stern: On Mon, 3 Aug 2015, Uwe [iso-8859-1] Kleine-Knig 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.st...@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? 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. Unfortunately we can not move RPM enabling to the end of probe, as the MDIO read/write functions that rely on RPM are already called while we are still in the middle of the probe function. I agree that we need better error handling here, but that comment applies to the whole FEC probe function. I think that this might be invasive enough to justify a delay to the next merge window, not really material for the late RCs. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
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.st...@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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
Hello, On Tue, Aug 04, 2015 at 10:20:55AM -0400, Alan Stern wrote: In that case, adding a call pm_runtime_get_noresume() is the right thing to do. Is this an ack for Lucas' patch? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
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.st...@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-koe...@pengutronix.de Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
On Tue, 4 Aug 2015, Uwe [iso-8859-1] Kleine-K�nig wrote: Hello, On Tue, Aug 04, 2015 at 10:20:55AM -0400, Alan Stern wrote: In that case, adding a call pm_runtime_get_noresume() is the right thing to do. Is this an ack for Lucas' patch? Yes: Acked-by: Alan Stern st...@rowland.harvard.edu Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: fec: fix initial runtime PM refcount
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.st...@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); -- 2.1.4 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
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.st...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
From: Lucas Stach l.st...@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.st...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.st...@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? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: fec: fix initial runtime PM refcount
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.st...@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. -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
