On Sun, Feb 6, 2011 at 11:19 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote: > Hi, > > Below is the next version of the patch changing the PM core to only create > wakeup sysfs files for devices that are wakeup-capable. ÂIt contains a change > in drivers/usb/core/hub.c that should avoid the problem described in the thread > at https://lkml.org/lkml/2011/2/5/108 , but I'm not 100% it's the right > approach. ÂPlease have a look. > > Thanks, > Rafael > > > --- > From: Rafael J. Wysocki <rjw@xxxxxxx> > Subject: PM: Don't create wakeup sysfs files for devices that can't wake up (v2) > > Currently, wakeup sysfs attributes are created for all devices, > regardless of whether or not they are wakeup-capable. ÂThis is > excessive and complicates wakeup device identification from user > space (i.e. to identify wakeup-capable devices user space has to read > /sys/devices/.../power/wakeup for all devices and see if they are not > empty). > > Fix this issue by avoiding to create wakeup sysfs files for devices > that cannot wake up the system from sleep states (i.e. whose > power.can_wakeup flags are unset during registration) and modify > device_set_wakeup_capable() so that it adds (or removes) the relevant > sysfs attributes if a device's wakeup capability status is changed. > > Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx> > --- > ÂDocumentation/ABI/testing/sysfs-devices-power |  20 +++--- > ÂDocumentation/power/devices.txt        |  20 +++--- > Âdrivers/base/power/power.h          Â|  21 +++---- > Âdrivers/base/power/sysfs.c          Â|  78 +++++++++++++++++--------- > Âdrivers/base/power/wakeup.c          |  26 ++++++++ > Âdrivers/usb/core/hub.c            Â|  10 ++- > Âinclude/linux/pm_runtime.h          Â|  Â6 ++ > Âinclude/linux/pm_wakeup.h           |  10 --- > Â8 files changed, 121 insertions(+), 70 deletions(-) > > Index: linux-2.6/drivers/base/power/sysfs.c > =================================================================== > --- linux-2.6.orig/drivers/base/power/sysfs.c > +++ linux-2.6/drivers/base/power/sysfs.c > @@ -431,26 +431,18 @@ static ssize_t async_store(struct device > Âstatic DEVICE_ATTR(async, 0644, async_show, async_store); > Â#endif /* CONFIG_PM_ADVANCED_DEBUG */ > > -static struct attribute * power_attrs[] = { > -    &dev_attr_wakeup.attr, > -#ifdef CONFIG_PM_SLEEP > -    &dev_attr_wakeup_count.attr, > -    &dev_attr_wakeup_active_count.attr, > -    &dev_attr_wakeup_hit_count.attr, > -    &dev_attr_wakeup_active.attr, > -    &dev_attr_wakeup_total_time_ms.attr, > -    &dev_attr_wakeup_max_time_ms.attr, > -    &dev_attr_wakeup_last_time_ms.attr, > -#endif > +static struct attribute *power_attrs[] = { > Â#ifdef CONFIG_PM_ADVANCED_DEBUG > +#ifdef CONFIG_PM_SLEEP >    Â&dev_attr_async.attr, > +#endif > Â#ifdef CONFIG_PM_RUNTIME >    Â&dev_attr_runtime_status.attr, >    Â&dev_attr_runtime_usage.attr, >    Â&dev_attr_runtime_active_kids.attr, >    Â&dev_attr_runtime_enabled.attr, > Â#endif > -#endif > +#endif /* CONFIG_PM_ADVANCED_DEBUG */ >    ÂNULL, > Â}; > Âstatic struct attribute_group pm_attr_group = { > @@ -458,9 +450,26 @@ static struct attribute_group pm_attr_gr >    Â.attrs Â= power_attrs, > Â}; > > -#ifdef CONFIG_PM_RUNTIME > +static struct attribute *wakeup_attrs[] = { > +#ifdef CONFIG_PM_SLEEP > +    &dev_attr_wakeup.attr, > +    &dev_attr_wakeup_count.attr, > +    &dev_attr_wakeup_active_count.attr, > +    &dev_attr_wakeup_hit_count.attr, > +    &dev_attr_wakeup_active.attr, > +    &dev_attr_wakeup_total_time_ms.attr, > +    &dev_attr_wakeup_max_time_ms.attr, > +    &dev_attr_wakeup_last_time_ms.attr, > +#endif > +    NULL, > +}; > +static struct attribute_group pm_wakeup_attr_group = { > +    .name  = power_group_name, > +    .attrs Â= wakeup_attrs, > +}; > > Âstatic struct attribute *runtime_attrs[] = { > +#ifdef CONFIG_PM_RUNTIME > Â#ifndef CONFIG_PM_ADVANCED_DEBUG >    Â&dev_attr_runtime_status.attr, > Â#endif > @@ -468,6 +477,7 @@ static struct attribute *runtime_attrs[] >    Â&dev_attr_runtime_suspended_time.attr, >    Â&dev_attr_runtime_active_time.attr, >    Â&dev_attr_autosuspend_delay_ms.attr, > +#endif /* CONFIG_PM_RUNTIME */ >    ÂNULL, > Â}; > Âstatic struct attribute_group pm_runtime_attr_group = { > @@ -480,35 +490,49 @@ int dpm_sysfs_add(struct device *dev) >    Âint rc; > >    Ârc = sysfs_create_group(&dev->kobj, &pm_attr_group); > -    if (rc == 0 && !dev->power.no_callbacks) { > +    if (rc) > +        return rc; > + > +    if (pm_runtime_callbacks_present(dev)) { >        Ârc = sysfs_merge_group(&dev->kobj, &pm_runtime_attr_group); >        Âif (rc) > -            sysfs_remove_group(&dev->kobj, &pm_attr_group); > +            goto err_out; > +    } > + > +    if (device_can_wakeup(dev)) { > +        rc = sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group); > +        if (rc) { > +            if (pm_runtime_callbacks_present(dev)) > +                sysfs_unmerge_group(&dev->kobj, > +                          &pm_runtime_attr_group); > +            goto err_out; > +        } >    Â} > +    return 0; > + > + err_out: > +    sysfs_remove_group(&dev->kobj, &pm_attr_group); >    Âreturn rc; > Â} > > -void rpm_sysfs_remove(struct device *dev) > +int wakeup_sysfs_add(struct device *dev) > Â{ > -    sysfs_unmerge_group(&dev->kobj, &pm_runtime_attr_group); > +    return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group); > Â} > > -void dpm_sysfs_remove(struct device *dev) > +void wakeup_sysfs_remove(struct device *dev) > Â{ > -    rpm_sysfs_remove(dev); > -    sysfs_remove_group(&dev->kobj, &pm_attr_group); > +    sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); > Â} > > -#else /* CONFIG_PM_RUNTIME */ > - > -int dpm_sysfs_add(struct device * dev) > +void rpm_sysfs_remove(struct device *dev) > Â{ > -    return sysfs_create_group(&dev->kobj, &pm_attr_group); > +    sysfs_unmerge_group(&dev->kobj, &pm_runtime_attr_group); > Â} > > -void dpm_sysfs_remove(struct device * dev) > +void dpm_sysfs_remove(struct device *dev) > Â{ > +    rpm_sysfs_remove(dev); > +    sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); >    Âsysfs_remove_group(&dev->kobj, &pm_attr_group); > Â} > - > -#endif > Index: linux-2.6/include/linux/pm_runtime.h > =================================================================== > --- linux-2.6.orig/include/linux/pm_runtime.h > +++ linux-2.6/include/linux/pm_runtime.h > @@ -87,6 +87,11 @@ static inline bool pm_runtime_enabled(st >    Âreturn !dev->power.disable_depth; > Â} > > +static inline bool pm_runtime_callbacks_present(struct device *dev) > +{ > +    return !dev->power.no_callbacks; > +} > + > Âstatic inline void pm_runtime_mark_last_busy(struct device *dev) > Â{ >    ÂACCESS_ONCE(dev->power.last_busy) = jiffies; > @@ -133,6 +138,7 @@ static inline int pm_generic_runtime_res > Âstatic inline void pm_runtime_no_callbacks(struct device *dev) {} > Âstatic inline void pm_runtime_irq_safe(struct device *dev) {} > > +static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } > Âstatic inline void pm_runtime_mark_last_busy(struct device *dev) {} > Âstatic inline void __pm_runtime_use_autosuspend(struct device *dev, >                        Âbool use) {} > Index: linux-2.6/drivers/base/power/power.h > =================================================================== > --- linux-2.6.orig/drivers/base/power/power.h > +++ linux-2.6/drivers/base/power/power.h > @@ -58,19 +58,18 @@ static inline void device_pm_move_last(s > Â* sysfs.c > Â*/ > > -extern int dpm_sysfs_add(struct device *); > -extern void dpm_sysfs_remove(struct device *); > -extern void rpm_sysfs_remove(struct device *); > +extern int dpm_sysfs_add(struct device *dev); > +extern void dpm_sysfs_remove(struct device *dev); > +extern void rpm_sysfs_remove(struct device *dev); > +extern int wakeup_sysfs_add(struct device *dev); > +extern void wakeup_sysfs_remove(struct device *dev); > > Â#else /* CONFIG_PM */ > > -static inline int dpm_sysfs_add(struct device *dev) > -{ > -    return 0; > -} > - > -static inline void dpm_sysfs_remove(struct device *dev) > -{ > -} > +static inline int dpm_sysfs_add(struct device *dev) { return 0; } > +static inline void dpm_sysfs_remove(struct device *dev) {} > +static inline void rpm_sysfs_remove(struct device *dev) {} > +static inline int wakeup_sysfs_add(struct device *dev) { return 0; } > +static inline void wakeup_sysfs_remove(struct device *dev) {} > > Â#endif > Index: linux-2.6/include/linux/pm_wakeup.h > =================================================================== > --- linux-2.6.orig/include/linux/pm_wakeup.h > +++ linux-2.6/include/linux/pm_wakeup.h > @@ -62,18 +62,11 @@ struct wakeup_source { > Â* Changes to device_may_wakeup take effect on the next pm state change. > Â*/ > > -static inline void device_set_wakeup_capable(struct device *dev, bool capable) > -{ > -    dev->power.can_wakeup = capable; > -} > - > Âstatic inline bool device_can_wakeup(struct device *dev) > Â{ >    Âreturn dev->power.can_wakeup; > Â} > > - > - > Âstatic inline bool device_may_wakeup(struct device *dev) > Â{ >    Âreturn dev->power.can_wakeup && !!dev->power.wakeup; > @@ -88,6 +81,7 @@ extern struct wakeup_source *wakeup_sour > Âextern void wakeup_source_unregister(struct wakeup_source *ws); > Âextern int device_wakeup_enable(struct device *dev); > Âextern int device_wakeup_disable(struct device *dev); > +extern void device_set_wakeup_capable(struct device *dev, bool capable); > Âextern int device_init_wakeup(struct device *dev, bool val); > Âextern int device_set_wakeup_enable(struct device *dev, bool enable); > Âextern void __pm_stay_awake(struct wakeup_source *ws); > @@ -144,7 +138,7 @@ static inline int device_wakeup_disable( > > Âstatic inline int device_init_wakeup(struct device *dev, bool val) > Â{ > -    dev->power.can_wakeup = val; > +    device_set_wakeup_capable(dev, val); >    Âreturn val ? -EINVAL : 0; > Â} > > Index: linux-2.6/drivers/base/power/wakeup.c > =================================================================== > --- linux-2.6.orig/drivers/base/power/wakeup.c > +++ linux-2.6/drivers/base/power/wakeup.c > @@ -242,6 +242,32 @@ int device_wakeup_disable(struct device > ÂEXPORT_SYMBOL_GPL(device_wakeup_disable); > > Â/** > + * device_set_wakeup_capable - Set/reset device wakeup capability flag. > + * @dev: Device to handle. > + * @capable: Whether or not @dev is capable of waking up the system from sleep. > + * > + * If @capable is set, set the @dev's power.can_wakeup flag and add its > + * wakeup-related attributes to sysfs. ÂOtherwise, unset the @dev's > + * power.can_wakeup flag and remove its wakeup-related attributes from sysfs. > + */ > +void device_set_wakeup_capable(struct device *dev, bool capable) > +{ > +    if (!!dev->power.can_wakeup == !!capable) > +        return; > + > +    if (device_is_registered(dev)) { > +        if (capable) { > +            if (wakeup_sysfs_add(dev)) > +                return; > +        } else { > +            wakeup_sysfs_remove(dev); > +        } > +    } > +    dev->power.can_wakeup = capable; > +} > +EXPORT_SYMBOL_GPL(device_set_wakeup_capable); > + > +/** > Â* device_init_wakeup - Device wakeup initialization. > Â* @dev: Device to handle. > Â* @enable: Whether or not to enable @dev as a wakeup device. > Index: linux-2.6/Documentation/power/devices.txt > =================================================================== > --- linux-2.6.orig/Documentation/power/devices.txt > +++ linux-2.6/Documentation/power/devices.txt > @@ -159,18 +159,18 @@ matter, and the kernel is responsible fo > Âwhether or not a wakeup-capable device should issue wakeup events is a policy > Âdecision, and it is managed by user space through a sysfs attribute: the > Âpower/wakeup file. ÂUser space can write the strings "enabled" or "disabled" to > -set or clear the should_wakeup flag, respectively. ÂReads from the file will > -return the corresponding string if can_wakeup is true, but if can_wakeup is > -false then reads will return an empty string, to indicate that the device > -doesn't support wakeup events. Â(But even though the file appears empty, writes > -will still affect the should_wakeup flag.) > +set or clear the "should_wakeup" flag, respectively. ÂThis file is only present > +for wakeup-capable devices (i.e. devices whose "can_wakeup" flags are set) > +and is created (or removed) by device_set_wakeup_capable(). ÂReads from the > +file will return the corresponding string. > > ÂThe device_may_wakeup() routine returns true only if both flags are set. > -Drivers should check this routine when putting devices in a low-power state > -during a system sleep transition, to see whether or not to enable the devices' > -wakeup mechanisms. ÂHowever for runtime power management, wakeup events should > -be enabled whenever the device and driver both support them, regardless of the > -should_wakeup flag. > +This information is used by subsystems, like the PCI bus type code, to see > +whether or not to enable the devices' wakeup mechanisms. ÂIf device wakeup > +mechanisms are enabled or disabled directly by drivers, they also should use > +device_may_wakeup() to decide what to do during a system sleep transition. > +However for runtime power management, wakeup events should be enabled whenever > +the device and driver both support them, regardless of the should_wakeup flag. > > > Â/sys/devices/.../power/control files > Index: linux-2.6/Documentation/ABI/testing/sysfs-devices-power > =================================================================== > --- linux-2.6.orig/Documentation/ABI/testing/sysfs-devices-power > +++ linux-2.6/Documentation/ABI/testing/sysfs-devices-power > @@ -29,9 +29,8 @@ Description: >        Â"disabled" to it. > >        ÂFor the devices that are not capable of generating system wakeup > -        events this file contains "\n". ÂIn that cases the user space > -        cannot modify the contents of this file and the device cannot be > -        enabled to wake up the system. > +        events this file is not present. ÂIn that case the device cannot > +        be enabled to wake up the system from sleep states. > > ÂWhat:     Â/sys/devices/.../power/control > ÂDate:     ÂJanuary 2009 > @@ -85,7 +84,7 @@ Description: >        ÂThe /sys/devices/.../wakeup_count attribute contains the number >        Âof signaled wakeup events associated with the device. ÂThis >        Âattribute is read-only. ÂIf the device is not enabled to wake up > -        the system from sleep states, this attribute is empty. > +        the system from sleep states, this attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_active_count > ÂDate:     ÂSeptember 2010 > @@ -95,7 +94,7 @@ Description: >        Ânumber of times the processing of wakeup events associated with >        Âthe device was completed (at the kernel level). ÂThis attribute >        Âis read-only. ÂIf the device is not enabled to wake up the > -        system from sleep states, this attribute is empty. > +        system from sleep states, this attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_hit_count > ÂDate:     ÂSeptember 2010 > @@ -105,7 +104,8 @@ Description: >        Ânumber of times the processing of a wakeup event associated with >        Âthe device might prevent the system from entering a sleep state. >        ÂThis attribute is read-only. ÂIf the device is not enabled to > -        wake up the system from sleep states, this attribute is empty. > +        wake up the system from sleep states, this attribute is not > +        present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_active > ÂDate:     ÂSeptember 2010 > @@ -115,7 +115,7 @@ Description: >        Âor 0, depending on whether or not a wakeup event associated with >        Âthe device is being processed (1). ÂThis attribute is read-only. >        ÂIf the device is not enabled to wake up the system from sleep > -        states, this attribute is empty. > +        states, this attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_total_time_ms > ÂDate:     ÂSeptember 2010 > @@ -125,7 +125,7 @@ Description: >        Âthe total time of processing wakeup events associated with the >        Âdevice, in milliseconds. ÂThis attribute is read-only. ÂIf the >        Âdevice is not enabled to wake up the system from sleep states, > -        this attribute is empty. > +        this attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_max_time_ms > ÂDate:     ÂSeptember 2010 > @@ -135,7 +135,7 @@ Description: >        Âthe maximum time of processing a single wakeup event associated >        Âwith the device, in milliseconds. ÂThis attribute is read-only. >        ÂIf the device is not enabled to wake up the system from sleep > -        states, this attribute is empty. > +        states, this attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/wakeup_last_time_ms > ÂDate:     ÂSeptember 2010 > @@ -146,7 +146,7 @@ Description: >        Âsignaling the last wakeup event associated with the device, in >        Âmilliseconds. ÂThis attribute is read-only. ÂIf the device is >        Ânot enabled to wake up the system from sleep states, this > -        attribute is empty. > +        attribute is not present. > > ÂWhat:     Â/sys/devices/.../power/autosuspend_delay_ms > ÂDate:     ÂSeptember 2010 > Index: linux-2.6/drivers/usb/core/hub.c > =================================================================== > --- linux-2.6.orig/drivers/usb/core/hub.c > +++ linux-2.6/drivers/usb/core/hub.c > @@ -1465,6 +1465,7 @@ void usb_set_device_state(struct usb_dev >        Âenum usb_device_state new_state) > Â{ >    Âunsigned long flags; > +    int wakeup = -1; > >    Âspin_lock_irqsave(&device_state_lock, flags); >    Âif (udev->state == USB_STATE_NOTATTACHED) > @@ -1479,11 +1480,10 @@ void usb_set_device_state(struct usb_dev >                    Â|| new_state == USB_STATE_SUSPENDED) >                Â;    /* No change to wakeup settings */ >            Âelse if (new_state == USB_STATE_CONFIGURED) > -                device_set_wakeup_capable(&udev->dev, > -                    (udev->actconfig->desc.bmAttributes > -                    Â& USB_CONFIG_ATT_WAKEUP)); > +                wakeup = udev->actconfig->desc.bmAttributes > +                    Â& USB_CONFIG_ATT_WAKEUP; >            Âelse > -                device_set_wakeup_capable(&udev->dev, 0); > +                wakeup = 0; >        Â} >        Âif (udev->state == USB_STATE_SUSPENDED && >            Ânew_state != USB_STATE_SUSPENDED) > @@ -1495,6 +1495,8 @@ void usb_set_device_state(struct usb_dev >    Â} else >        Ârecursively_mark_NOTATTACHED(udev); >    Âspin_unlock_irqrestore(&device_state_lock, flags); > +    if (wakeup >= 0) > +        device_set_wakeup_capable(&udev->dev, wakeup); > Â} > ÂEXPORT_SYMBOL_GPL(usb_set_device_state); > > I can't say the work of usb since I am not a expert about that. Anyway, the patch works well in my machine. Just a nitpick. Please, add a kind comment for new exported device_set_wakeup_capable to show the function should be called by process context. Feel free to use below signs. Reported-by: Minchan Kim <minchan.kim@xxxxxxxxx> Tested-by: Minchan Kim <minchan.kim@xxxxxxxxx> -- Kind regards, Minchan Kim _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
