[PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Ulf Hansson]

The smsc911c driver puts its device into low power state when entering
system suspend. Although it doesn't update the device's runtime PM status
to RPM_SUSPENDED, which causes problems for a parent device.

In particular, when the runtime PM status of the parent is requested to be
updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
forbidden to runtime suspend a device, which has an active child.

Fix this by updating the runtime PM status of the smsc911x device to
RPM_SUSPENDED during system suspend. In system resume, let's reverse that
action by runtime resuming the device and thus also the parent.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Steve Glendinning <steve.glendinning@shawell.net>
Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
---

Note that the commit this change fixes is currently queued for 4.10 via
Rafael's linux-pm tree. So this fix should go via that tree as well.

---
 drivers/net/ethernet/smsc/smsc911x.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index e9b8579..65fca9c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
 		PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
 		PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
 
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
 	return 0;
 }
 
@@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
 	struct smsc911x_data *pdata = netdev_priv(ndev);
 	unsigned int to = 100;
 
+	pm_runtime_enable(dev);
+	pm_runtime_resume(dev);
+
 	/* Note 3.11 from the datasheet:
 	 * 	"When the LAN9220 is in a power saving state, a write of any
 	 * 	 data to the BYTE_TEST register will wake-up the device."
-- 
1.9.1

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The smsc911c driver puts its device into low power state when entering
> system suspend. Although it doesn't update the device's runtime PM status
> to RPM_SUSPENDED, which causes problems for a parent device.
>
> In particular, when the runtime PM status of the parent is requested to be
> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
> forbidden to runtime suspend a device, which has an active child.
>
> Fix this by updating the runtime PM status of the smsc911x device to
> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
> action by runtime resuming the device and thus also the parent.

Thanks for your patch!

The changelog sounds quite innocent, but this does fix a system crash
during resume from s2ram.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")

While the abovementioned commit made the problem visible, the root cause
was present before, right?

> ---
>
> Note that the commit this change fixes is currently queued for 4.10 via
> Rafael's linux-pm tree. So this fix should go via that tree as well.

Alternatively, this could go in in v4.9 to avoid the problem from ever
appearing in upstream?

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Ulf Hansson]

On 27 October 2016 at 13:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The smsc911c driver puts its device into low power state when entering
>> system suspend. Although it doesn't update the device's runtime PM status
>> to RPM_SUSPENDED, which causes problems for a parent device.
>>
>> In particular, when the runtime PM status of the parent is requested to be
>> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>> forbidden to runtime suspend a device, which has an active child.
>>
>> Fix this by updating the runtime PM status of the smsc911x device to
>> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>> action by runtime resuming the device and thus also the parent.
>
> Thanks for your patch!
>
> The changelog sounds quite innocent, but this does fix a system crash
> during resume from s2ram.
>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Steve Glendinning <steve.glendinning@shawell.net>
>> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
>
> While the abovementioned commit made the problem visible, the root cause
> was present before, right?

Yes.

>
>> ---
>>
>> Note that the commit this change fixes is currently queued for 4.10 via
>> Rafael's linux-pm tree. So this fix should go via that tree as well.
>
> Alternatively, this could go in in v4.9 to avoid the problem from ever
> appearing in upstream?

Makes perfect sense! In that case we should remove the fixes tag.

Rafael, can you pick this up for 4.9 rc[n]?

Kind regards
Uffe

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Oct 27, 2016 at 1:53 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 27 October 2016 at 13:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> The smsc911c driver puts its device into low power state when entering
>>> system suspend. Although it doesn't update the device's runtime PM status
>>> to RPM_SUSPENDED, which causes problems for a parent device.
>>>
>>> In particular, when the runtime PM status of the parent is requested to be
>>> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>>> forbidden to runtime suspend a device, which has an active child.
>>>
>>> Fix this by updating the runtime PM status of the smsc911x device to
>>> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>>> action by runtime resuming the device and thus also the parent.
>>
>> Thanks for your patch!
>>
>> The changelog sounds quite innocent, but this does fix a system crash
>> during resume from s2ram.
>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Steve Glendinning <steve.glendinning@shawell.net>
>>> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
>>
>> While the abovementioned commit made the problem visible, the root cause
>> was present before, right?
>
> Yes.
>
>>> ---
>>>
>>> Note that the commit this change fixes is currently queued for 4.10 via
>>> Rafael's linux-pm tree. So this fix should go via that tree as well.
>>
>> Alternatively, this could go in in v4.9 to avoid the problem from ever
>> appearing in upstream?
>
> Makes perfect sense! In that case we should remove the fixes tag.
>
> Rafael, can you pick this up for 4.9 rc[n]?

Actually I was thinking about DaveM and the network tree instead.

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

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Ulf Hansson]

On 27 October 2016 at 13:54, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Oct 27, 2016 at 1:53 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 27 October 2016 at 13:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> The smsc911c driver puts its device into low power state when entering
>>>> system suspend. Although it doesn't update the device's runtime PM status
>>>> to RPM_SUSPENDED, which causes problems for a parent device.
>>>>
>>>> In particular, when the runtime PM status of the parent is requested to be
>>>> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>>>> forbidden to runtime suspend a device, which has an active child.
>>>>
>>>> Fix this by updating the runtime PM status of the smsc911x device to
>>>> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>>>> action by runtime resuming the device and thus also the parent.
>>>
>>> Thanks for your patch!
>>>
>>> The changelog sounds quite innocent, but this does fix a system crash
>>> during resume from s2ram.
>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Steve Glendinning <steve.glendinning@shawell.net>
>>>> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
>>>
>>> While the abovementioned commit made the problem visible, the root cause
>>> was present before, right?
>>
>> Yes.
>>
>>>> ---
>>>>
>>>> Note that the commit this change fixes is currently queued for 4.10 via
>>>> Rafael's linux-pm tree. So this fix should go via that tree as well.
>>>
>>> Alternatively, this could go in in v4.9 to avoid the problem from ever
>>> appearing in upstream?
>>
>> Makes perfect sense! In that case we should remove the fixes tag.
>>
>> Rafael, can you pick this up for 4.9 rc[n]?
>
> Actually I was thinking about DaveM and the network tree instead.

Well, that would work as well.

Although, perhaps it becomes easier if Rafael deals with this, as it
gives him better control of when below change also can go in.
https://patchwork.kernel.org/patch/9375061

Rafael, please tell what you prefer?

Kind regards
Uffe

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Rafael J. Wysocki]

On Thursday, October 27, 2016 01:53:03 PM Ulf Hansson wrote:
> On 27 October 2016 at 13:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Hi Ulf,
> >
> > On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> The smsc911c driver puts its device into low power state when entering
> >> system suspend. Although it doesn't update the device's runtime PM status
> >> to RPM_SUSPENDED, which causes problems for a parent device.
> >>
> >> In particular, when the runtime PM status of the parent is requested to be
> >> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
> >> forbidden to runtime suspend a device, which has an active child.
> >>
> >> Fix this by updating the runtime PM status of the smsc911x device to
> >> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
> >> action by runtime resuming the device and thus also the parent.
> >
> > Thanks for your patch!
> >
> > The changelog sounds quite innocent, but this does fix a system crash
> > during resume from s2ram.
> >
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> >> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
> >
> > While the abovementioned commit made the problem visible, the root cause
> > was present before, right?
> 
> Yes.
> 
> >
> >> ---
> >>
> >> Note that the commit this change fixes is currently queued for 4.10 via
> >> Rafael's linux-pm tree. So this fix should go via that tree as well.
> >
> > Alternatively, this could go in in v4.9 to avoid the problem from ever
> > appearing in upstream?
> 
> Makes perfect sense! In that case we should remove the fixes tag.
> 
> Rafael, can you pick this up for 4.9 rc[n]?

If that is to go into 4.9-rc, it really should go in via the networking tree,
because there is no PM dependency for it as of today.

I can rearrange my 4.10 queue to put this one before the runtime PM commit
exposing the problem in smsc911x, though.

Let me know what you prefer.

Thanks,
Rafael

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Ulf Hansson]

On 1 November 2016 at 05:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, October 27, 2016 01:53:03 PM Ulf Hansson wrote:
>> On 27 October 2016 at 13:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> > Hi Ulf,
>> >
>> > On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >> The smsc911c driver puts its device into low power state when entering
>> >> system suspend. Although it doesn't update the device's runtime PM status
>> >> to RPM_SUSPENDED, which causes problems for a parent device.
>> >>
>> >> In particular, when the runtime PM status of the parent is requested to be
>> >> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>> >> forbidden to runtime suspend a device, which has an active child.
>> >>
>> >> Fix this by updating the runtime PM status of the smsc911x device to
>> >> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>> >> action by runtime resuming the device and thus also the parent.
>> >
>> > Thanks for your patch!
>> >
>> > The changelog sounds quite innocent, but this does fix a system crash
>> > during resume from s2ram.
>> >
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> >> Cc: Steve Glendinning <steve.glendinning@shawell.net>
>> >> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
>> >
>> > While the abovementioned commit made the problem visible, the root cause
>> > was present before, right?
>>
>> Yes.
>>
>> >
>> >> ---
>> >>
>> >> Note that the commit this change fixes is currently queued for 4.10 via
>> >> Rafael's linux-pm tree. So this fix should go via that tree as well.
>> >
>> > Alternatively, this could go in in v4.9 to avoid the problem from ever
>> > appearing in upstream?
>>
>> Makes perfect sense! In that case we should remove the fixes tag.
>>
>> Rafael, can you pick this up for 4.9 rc[n]?
>
> If that is to go into 4.9-rc, it really should go in via the networking tree,
> because there is no PM dependency for it as of today.
>
> I can rearrange my 4.10 queue to put this one before the runtime PM commit
> exposing the problem in smsc911x, though.

As we spoked at LPC today, I don't mind if you take care of this
through your tree.

Kind regards
Uffe

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Jeremy Linton]

Hi,

On 10/27/2016 06:23 AM, Ulf Hansson wrote:
> The smsc911c driver puts its device into low power state when entering
> system suspend. Although it doesn't update the device's runtime PM status
> to RPM_SUSPENDED, which causes problems for a parent device.
>
> In particular, when the runtime PM status of the parent is requested to be
> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
> forbidden to runtime suspend a device, which has an active child.
>
> Fix this by updating the runtime PM status of the smsc911x device to
> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
> action by runtime resuming the device and thus also the parent.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active child")
> ---
>
> Note that the commit this change fixes is currently queued for 4.10 via
> Rafael's linux-pm tree. So this fix should go via that tree as well.
>
> ---
>  drivers/net/ethernet/smsc/smsc911x.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index e9b8579..65fca9c 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
>  		PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
>  		PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
>
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);

> +
>  	return 0;
>  }
>
> @@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
>  	struct smsc911x_data *pdata = netdev_priv(ndev);
>  	unsigned int to = 100;
>
> +	pm_runtime_enable(dev);
> +	pm_runtime_resume(dev);
> +
>  	/* Note 3.11 from the datasheet:
>  	 * 	"When the LAN9220 is in a power saving state, a write of any
>  	 * 	 data to the BYTE_TEST register will wake-up the device."
>

This seems an unusual change/sequence. I thought a successful return 
from the suspend callback would set the device state to suspended.

I just checked a few other ethernet drivers suspend/resume sequences and 
directly calling the pm_runtime seems a little unusual. Most of the 
other drivers are checking to see if the interface is running then doing 
a netif_device_detach()/attach() sequence which is missing from this 
drivers suspend/resume path. Could that be part of the problem?

Of course my knowledge of the power management system is a little thin 
so I could be really off base.