* [PATCH] net: fec: fix initial runtime PM refcount
@ 2015-08-03 15:50 Lucas Stach
2015-08-03 16:15 ` Andrew Lunn
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Lucas Stach @ 2015-08-03 15:50 UTC (permalink / raw)
To: David S. Miller
Cc: Andrew Lunn, Uwe Kleine-König, netdev, kernel, patchwork-lst
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);
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
@ 2015-08-03 16:15 ` Andrew Lunn
2015-08-03 18:05 ` Uwe Kleine-König
2015-08-04 7:34 ` Lucas Stach
2015-08-04 5:23 ` David Miller
` (3 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Andrew Lunn @ 2015-08-03 16:15 UTC (permalink / raw)
To: Lucas Stach
Cc: David S. Miller, Uwe Kleine-König, netdev, kernel,
patchwork-lst
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 16:15 ` Andrew Lunn
@ 2015-08-03 18:05 ` Uwe Kleine-König
2015-08-03 18:28 ` Alan Stern
2015-08-04 7:34 ` Lucas Stach
1 sibling, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2015-08-03 18:05 UTC (permalink / raw)
To: Andrew Lunn
Cc: Lucas Stach, netdev, patchwork-lst, David S. Miller, kernel,
Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm
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.
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 18:05 ` Uwe Kleine-König
@ 2015-08-03 18:28 ` Alan Stern
2015-08-04 7:43 ` Lucas Stach
0 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2015-08-03 18:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Lunn, Lucas Stach, netdev, patchwork-lst, David S. Miller,
kernel, Rafael J. Wysocki, Len Brown, Pavel Machek, linux-pm
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 3579 bytes --]
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@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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
2015-08-03 16:15 ` Andrew Lunn
@ 2015-08-04 5:23 ` David Miller
2015-08-04 8:11 ` Uwe Kleine-König
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2015-08-04 5:23 UTC (permalink / raw)
To: l.stach; +Cc: andrew, u.kleine-koenig, netdev, kernel, patchwork-lst
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 16:15 ` Andrew Lunn
2015-08-03 18:05 ` Uwe Kleine-König
@ 2015-08-04 7:34 ` Lucas Stach
1 sibling, 0 replies; 13+ messages in thread
From: Lucas Stach @ 2015-08-04 7:34 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, patchwork-lst, David S. Miller, kernel,
Uwe Kleine-König
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
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 18:28 ` Alan Stern
@ 2015-08-04 7:43 ` Lucas Stach
2015-08-04 14:20 ` Alan Stern
0 siblings, 1 reply; 13+ messages in thread
From: Lucas Stach @ 2015-08-04 7:43 UTC (permalink / raw)
To: Alan Stern
Cc: Uwe Kleine-König, Andrew Lunn, netdev, patchwork-lst,
David S. Miller, kernel, Rafael J. Wysocki, Len Brown,
Pavel Machek, linux-pm
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.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?
>
> 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/ |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
2015-08-03 16:15 ` Andrew Lunn
2015-08-04 5:23 ` David Miller
@ 2015-08-04 8:11 ` Uwe Kleine-König
2015-08-05 8:49 ` [PATCH net-master v2] " Uwe Kleine-König
2015-08-07 1:54 ` [PATCH] " David Miller
4 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2015-08-04 8:11 UTC (permalink / raw)
To: Lucas Stach; +Cc: David S. Miller, Andrew Lunn, netdev, patchwork-lst, kernel
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-04 7:43 ` Lucas Stach
@ 2015-08-04 14:20 ` Alan Stern
2015-08-04 14:35 ` Uwe Kleine-König
0 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2015-08-04 14:20 UTC (permalink / raw)
To: Lucas Stach
Cc: Uwe Kleine-König, Andrew Lunn, netdev, patchwork-lst,
David S. Miller, kernel, Rafael J. Wysocki, Len Brown,
Pavel Machek, linux-pm
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.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?
> >
> > 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-04 14:20 ` Alan Stern
@ 2015-08-04 14:35 ` Uwe Kleine-König
2015-08-04 15:59 ` Alan Stern
0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2015-08-04 14:35 UTC (permalink / raw)
To: Alan Stern
Cc: Lucas Stach, Andrew Lunn, Pavel Machek, Len Brown, netdev,
linux-pm, Rafael J. Wysocki, patchwork-lst, kernel,
David S. Miller
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/ |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-04 14:35 ` Uwe Kleine-König
@ 2015-08-04 15:59 ` Alan Stern
0 siblings, 0 replies; 13+ messages in thread
From: Alan Stern @ 2015-08-04 15:59 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lucas Stach, Andrew Lunn, Pavel Machek, Len Brown, netdev,
linux-pm, Rafael J. Wysocki, patchwork-lst, kernel,
David S. Miller
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 340 bytes --]
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 <stern@rowland.harvard.edu>
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-master v2] net: fec: fix initial runtime PM refcount
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
` (2 preceding siblings ...)
2015-08-04 8:11 ` Uwe Kleine-König
@ 2015-08-05 8:49 ` Uwe Kleine-König
2015-08-07 1:54 ` [PATCH] " David Miller
4 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2015-08-05 8:49 UTC (permalink / raw)
To: David Miller; +Cc: Andrew Lunn, netdev, kernel, Lucas Stach, Alan Stern
From: Lucas Stach <l.stach@pengutronix.de>
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>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello David,
I think this is fine to apply now for 4.2-rc. I just picked up the
patch, added Alan's ack and a tested-by for me to make it easier for
you.
Best regards
Uwe
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.4.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: fec: fix initial runtime PM refcount
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
` (3 preceding siblings ...)
2015-08-05 8:49 ` [PATCH net-master v2] " Uwe Kleine-König
@ 2015-08-07 1:54 ` David Miller
4 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2015-08-07 1:54 UTC (permalink / raw)
To: l.stach; +Cc: andrew, u.kleine-koenig, netdev, kernel, patchwork-lst
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-08-07 1:54 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-03 15:50 [PATCH] net: fec: fix initial runtime PM refcount Lucas Stach
2015-08-03 16:15 ` Andrew Lunn
2015-08-03 18:05 ` Uwe Kleine-König
2015-08-03 18:28 ` Alan Stern
2015-08-04 7:43 ` Lucas Stach
2015-08-04 14:20 ` Alan Stern
2015-08-04 14:35 ` Uwe Kleine-König
2015-08-04 15:59 ` Alan Stern
2015-08-04 7:34 ` Lucas Stach
2015-08-04 5:23 ` David Miller
2015-08-04 8:11 ` Uwe Kleine-König
2015-08-05 8:49 ` [PATCH net-master v2] " Uwe Kleine-König
2015-08-07 1:54 ` [PATCH] " David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).