Re: [RFC][PATCH] PM: Do not create wakeup sysfs files for devices that cannot wakeup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 2011-01-15 at 01:08 +0100, Rafael J. Wysocki wrote:
> On Friday, January 14, 2011, Alan Stern wrote:
> > On Fri, 14 Jan 2011, Rafael J. Wysocki wrote:

I welcome that change very well.
It was always driving me nuts seeing these empty wakeup files.

Best regards,
	Maxim Levitsky

> ...
> > ...
> > 
> > >  static struct attribute *runtime_attrs[] = {
> > > -#ifndef CONFIG_PM_ADVANCED_DEBUG
> > > +#ifdef CONFIG_PM_RUNTIME
> > >  	&dev_attr_runtime_status.attr,
> > > -#endif
> > >  	&dev_attr_control.attr,
> > >  	&dev_attr_runtime_suspended_time.attr,
> > >  	&dev_attr_runtime_active_time.attr,
> > >  	&dev_attr_autosuspend_delay_ms.attr,
> > > +#ifdef CONFIG_PM_ADVANCED_DEBUG
> > > +	&dev_attr_runtime_usage.attr,
> > > +	&dev_attr_runtime_active_kids.attr,
> > > +	&dev_attr_runtime_enabled.attr,
> > > +#endif
> > > +#endif /* CONFIG_PM_RUNTIME */
> > >  	NULL,
> > >  };
> > >  static struct attribute_group pm_runtime_attr_group = {
> > > @@ -480,35 +485,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;
> > > +	}
> > 
> > One thing I don't like about this change.  The original code defines
> > runtime_status, runtime_usage, runtime_active_kids, and runtime_enabled
> > whenever PM_RUNTIME and PM_ADVANCED_DEBUG are set.  Now you don't
> > define these attributes if runtime callbacks aren't present, even 
> > though they would still make sense.  Maybe you should have two 
> > runtime-related attribute groups, where one depends on 
> > callbacks_present and the other doesn't.
> 
> OK, I think that problem is not present any more in the new version of the
> patch, which is appended.
> 
> > Apart from that, it all seems reasonable.  But you should change the
> > description of the wakeup attribute in Documentation/power/devices.txt;  
> > it says that the attribute file is present but empty if the device is
> > not wakeup-capable.
> 
> Right and not only that.  The descriptions in ABI/sysfs-device-power have to be
> updated too.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@xxxxxxx>
> Subject: PM: Do not create wakeup sysfs files for devices that cannot wake up
> 
> 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                    |   15 ++---
>  drivers/base/power/sysfs.c                    |   78 +++++++++++++++++---------
>  drivers/base/power/wakeup.c                   |   26 ++++++++
>  include/linux/pm_runtime.h                    |    6 ++
>  include/linux/pm_wakeup.h                     |   10 ---
>  7 files changed, 112 insertions(+), 63 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
> @@ -61,16 +61,15 @@ static inline void device_pm_move_last(s
>  extern int dpm_sysfs_add(struct device *);
>  extern void dpm_sysfs_remove(struct device *);
>  extern void rpm_sysfs_remove(struct device *);
> +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 *) {}
> +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
> @@ -228,6 +228,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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm


[Index of Archives]     [Linux ACPI]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [CPU Freq]     [Kernel Newbies]     [Fedora Kernel]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux