Re: [RFC PATCH 1/3] ACPI: scan: add userland notification while handling eject events

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Mar 27, 2020 at 07:22:45PM +0800, Chester Lin wrote:
> Add a request_offline attribute in order to tell the kernel if it's
> required to send notifications to userland first while handling an eject
> event. Userland will have to put the target device offline when this
> attribute is set.
> 
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-acpi | 16 ++++++++++
>  drivers/acpi/device_sysfs.c              | 40 +++++++++++++++++++++++-
>  drivers/acpi/scan.c                      | 39 +++++++++++++++++++----
>  include/acpi/acpi_bus.h                  |  1 +
>  4 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index e7898cfe5fb1..b9c467704889 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -93,3 +93,19 @@ Description:
>  		hardware, if the _HRV control method is present.  It is mostly
>  		useful for non-PCI devices because lspci can list the hardware
>  		version for PCI devices.
> +
> +What:		/sys/bus/acpi/devices/.../request_offline
> +Date:		Mar, 2020
> +Contact:	Chester Lin <clin@suse.com>
> +Description:
> +		(RW) Allows the userland to receive offline requests when
> +		devices are planning to be ejected.
> +
> +		If bit [0] is clear, the kernel will automatically try putting
> +		the target offline before the target can be ejected.
> +
> +		If bit [0] is set, a uevent will be sent to userland as an
> +		offline request and userland is responsible for handling offline
> +		operations before the target can be ejected. This approach
> +		provides flexibility while some applications could need more
> +		time to release resources.

Don't use "bit", use 1/0/y/n/Y/N as the kernel will parse all of that
for you with the kstrtobool() which was created just for this type of
sysfs file.

> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index 96869f1538b9..453bd1b9edf5 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -506,6 +506,37 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(status);
>  
> +static ssize_t request_offline_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct acpi_device *acpi_dev = to_acpi_device(dev);
> +
> +	return sprintf(buf, "%u\n", acpi_dev->request_offline?1:0);
> +}
> +
> +static ssize_t request_offline_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct acpi_device *acpi_dev = to_acpi_device(dev);
> +
> +	if (!count)
> +		return -EINVAL;
> +
> +	switch (buf[0]) {
> +	case '0':
> +		acpi_dev->request_offline = false;
> +		break;
> +	case '1':
> +		acpi_dev->request_offline = true;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(request_offline);
> +
>  /**
>   * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
>   * @dev: ACPI device object.
> @@ -580,6 +611,11 @@ int acpi_device_setup_files(struct acpi_device *dev)
>  		result = device_create_file(&dev->dev, &dev_attr_eject);
>  		if (result)
>  			return result;
> +
> +		result = device_create_file(&dev->dev,
> +					    &dev_attr_request_offline);
> +		if (result)
> +			return result;
>  	}
>  
>  	if (dev->flags.power_manageable) {
> @@ -623,8 +659,10 @@ void acpi_device_remove_files(struct acpi_device *dev)
>  	/*
>  	 * If device has _EJ0, remove 'eject' file.
>  	 */
> -	if (acpi_has_method(dev->handle, "_EJ0"))
> +	if (acpi_has_method(dev->handle, "_EJ0")) {
>  		device_remove_file(&dev->dev, &dev_attr_eject);
> +		device_remove_file(&dev->dev, &dev_attr_request_offline);

You all really should be using an attribute group and the is_visible()
callback to handle all of this for you automatically.

But that's a separate issue than this specific patch.

> +	}
>  
>  	if (acpi_has_method(dev->handle, "_SUN"))
>  		device_remove_file(&dev->dev, &dev_attr_sun);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 6d3448895382..1cb39c5360cf 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -145,6 +145,7 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
>  	struct acpi_device_physical_node *pn;
>  	bool second_pass = (bool)data;
>  	acpi_status status = AE_OK;
> +	char *envp[] = { "EVENT=offline", NULL };
>  
>  	if (acpi_bus_get_device(handle, &device))
>  		return AE_OK;
> @@ -166,7 +167,18 @@ static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data,
>  		} else {
>  			pn->put_online = false;
>  		}
> -		ret = device_offline(pn->dev);
> +
> +		/* Don't offline directly but need to notify userland first */
> +		if (device->request_offline) {
> +			if (pn->dev->offline)
> +				ret = 0;
> +			else
> +				ret = kobject_uevent_env(&pn->dev->kobj,
> +							KOBJ_CHANGE, envp);

So this is a userspace visable change with regards to kobject events?

Are you sure that is ok?