| Message ID | 1477567434-5128-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8812872960824681147fad051e6e1406fdfa07f9 |
| Headers | show |
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 -- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/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."