| Message ID | 2147450.LFoExqrKEF@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 02 Dec 2012 14:48:50 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Sunday, December 02, 2012 07:46:25 PM NeilBrown wrote: > > On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > > > Thanks for the confirmation. > > > > > > Below it goes again with a changelog and tags. > > > > > > I don't really think that SDIO does the right thing here overall, but that's > > > all I can do to address the problem timely. > > > > > > Thanks, > > > Rafael > > > > Hi Rafael, > > I just discovered that this patch has since been reverted - with an 'ack' > > from you: > > ---------- > > commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06 > > Author: Thierry Reding <thierry.reding@avionic-design.de> > > Date: Thu Aug 9 09:32:21 2012 +0000 > > > > mmc: sdio: Fix PM_SLEEP related build warnings > > > > Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if > > the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will > > complain about them being unused. However, since the callback for this > > driver doesn't do anything it can just as well be dropped. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > > Signed-off-by: Chris Ball <cjb@laptop.org> > > ----------- > > > > Unsurprisingly the problem which your patch fixed has come back. > > > > Do you think we could get the patch back in again. This time maybe we should > > put some comments in there pointing out that having a function which does > > nothing is very different from not having any function at all? > > Well, I agree. I didn't remember that the callback had been added for > a purpose and hence my "ack" for that patch. > > What about applying the appended patch (hopefully, the build warnings should > be fixed properly this time)? Looks good to me - thanks! NeilBrown > > Rafael > > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: SDIO / PM: Add empty bus-level suspend/resume callbacks > > Suspend methods provided by SDIO drivers are not supposed to be > called by the PM core. Instead, when the SDIO core gets to suspend > a device's ancestor, it calls the device driver's suspend routine. > However, the PM core executes suspend callback routines directly for > device drivers whose bus types don't provide suspend callbacks. > In consequece, because the SDIO bus type doesn't provide a suspend > callback, the SDIO drivers' suspend routines will be executed by the > PM core (which shouldn't happen). > > To prevent this from happening, add empty system suspend/resume > callbacks for the SDIO bus type. > > An analogous change had been made already by commit (e841a7c mmc: > sdio: Use empty system suspend/resume callbacks at the bus level), > but then it was reverted inadvertently by commit (d8e2ac3 mmc: sdio: > Fix PM_SLEEP related build warnings) that attempted to fix build > warnings introduced by commit e841a7c. > > Reported-by: NeilBrown <neilb@suse.de> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/mmc/core/sdio_bus.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > Index: linux/drivers/mmc/core/sdio_bus.c > =================================================================== > --- linux.orig/drivers/mmc/core/sdio_bus.c > +++ linux/drivers/mmc/core/sdio_bus.c > @@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device > } > > #ifdef CONFIG_PM > + > +#ifdef CONFIG_PM_SLEEP > +static int pm_no_operation(struct device *dev) > +{ > + /* > + * Prevent the PM core from calling SDIO device drivers' suspend > + * callback routines, which it is not supposed to do, by using this > + * empty function as the bus type suspend callaback for SDIO. > + */ > + return 0; > +} > +#endif > + > static const struct dev_pm_ops sdio_bus_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation) > SET_RUNTIME_PM_OPS( > pm_generic_runtime_suspend, > pm_generic_runtime_resume, > >
On Sun, Dec 02, 2012 at 02:48:50PM +0100, Rafael J. Wysocki wrote: > On Sunday, December 02, 2012 07:46:25 PM NeilBrown wrote: > > On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > > > Thanks for the confirmation. > > > > > > Below it goes again with a changelog and tags. > > > > > > I don't really think that SDIO does the right thing here overall, but that's > > > all I can do to address the problem timely. > > > > > > Thanks, > > > Rafael > > > > Hi Rafael, > > I just discovered that this patch has since been reverted - with an 'ack' > > from you: > > ---------- > > commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06 > > Author: Thierry Reding <thierry.reding@avionic-design.de> > > Date: Thu Aug 9 09:32:21 2012 +0000 > > > > mmc: sdio: Fix PM_SLEEP related build warnings > > > > Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if > > the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will > > complain about them being unused. However, since the callback for this > > driver doesn't do anything it can just as well be dropped. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > > Signed-off-by: Chris Ball <cjb@laptop.org> > > ----------- > > > > Unsurprisingly the problem which your patch fixed has come back. > > > > Do you think we could get the patch back in again. This time maybe we should > > put some comments in there pointing out that having a function which does > > nothing is very different from not having any function at all? > > Well, I agree. I didn't remember that the callback had been added for > a purpose and hence my "ack" for that patch. > > What about applying the appended patch (hopefully, the build warnings should > be fixed properly this time)? > > Rafael > > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: SDIO / PM: Add empty bus-level suspend/resume callbacks > > Suspend methods provided by SDIO drivers are not supposed to be > called by the PM core. Instead, when the SDIO core gets to suspend > a device's ancestor, it calls the device driver's suspend routine. > However, the PM core executes suspend callback routines directly for > device drivers whose bus types don't provide suspend callbacks. > In consequece, because the SDIO bus type doesn't provide a suspend > callback, the SDIO drivers' suspend routines will be executed by the > PM core (which shouldn't happen). > > To prevent this from happening, add empty system suspend/resume > callbacks for the SDIO bus type. > > An analogous change had been made already by commit (e841a7c mmc: > sdio: Use empty system suspend/resume callbacks at the bus level), > but then it was reverted inadvertently by commit (d8e2ac3 mmc: sdio: > Fix PM_SLEEP related build warnings) that attempted to fix build > warnings introduced by commit e841a7c. > > Reported-by: NeilBrown <neilb@suse.de> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/mmc/core/sdio_bus.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > Index: linux/drivers/mmc/core/sdio_bus.c > =================================================================== > --- linux.orig/drivers/mmc/core/sdio_bus.c > +++ linux/drivers/mmc/core/sdio_bus.c > @@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device > } > > #ifdef CONFIG_PM > + > +#ifdef CONFIG_PM_SLEEP > +static int pm_no_operation(struct device *dev) > +{ > + /* > + * Prevent the PM core from calling SDIO device drivers' suspend > + * callback routines, which it is not supposed to do, by using this > + * empty function as the bus type suspend callaback for SDIO. > + */ > + return 0; > +} > +#endif > + > static const struct dev_pm_ops sdio_bus_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation) > SET_RUNTIME_PM_OPS( > pm_generic_runtime_suspend, > pm_generic_runtime_resume, > Hehe... if my memory serves me well, that's exactly (well, modulo the comment) what my initial patch did before somebody suggested that the empty callbacks should just be removed altogether. =) Thierry
Hi, On Sun, Dec 02 2012, NeilBrown wrote: >> What about applying the appended patch (hopefully, the build warnings should >> be fixed properly this time)? > > Looks good to me - thanks! Thanks, both of you, pushed to mmc-next for 3.8. - Chris.
Index: linux/drivers/mmc/core/sdio_bus.c =================================================================== --- linux.orig/drivers/mmc/core/sdio_bus.c +++ linux/drivers/mmc/core/sdio_bus.c @@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device } #ifdef CONFIG_PM + +#ifdef CONFIG_PM_SLEEP +static int pm_no_operation(struct device *dev) +{ + /* + * Prevent the PM core from calling SDIO device drivers' suspend + * callback routines, which it is not supposed to do, by using this + * empty function as the bus type suspend callaback for SDIO. + */ + return 0; +} +#endif + static const struct dev_pm_ops sdio_bus_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation) SET_RUNTIME_PM_OPS( pm_generic_runtime_suspend, pm_generic_runtime_resume,