On Mon, Mar 30, 2020 at 04:50:20PM +0800, Chester Lin wrote: > Hi Greg, > > On Fri, Mar 27, 2020 at 12:38:42PM +0100, Greg KH wrote: > > 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@xxxxxxxx> > > > --- > > > 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@xxxxxxxx> > > > +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. > > > > I'm sorry for this mistake. Based on my code they should be ASCII char '1' and > '0' but not bitwise ops. I will fix this description. > > > > 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. > > > > That sounds good to me. I will refine this part by declaring an attribute_group > with a is_visible() callback. > > > > + } > > > > > > 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? > > > > Since udev can see kobject events when devices are added, I haven't seen any > risk if we make offline events visible too. Besides, normally online/eject > attributes can only be written by root in userspace. > Correct my explanation here: So far udev can see several device events already, such as add, online, offline and remove. So I think it should not be risky if we send additional change events to userspace as notification. > Thanks, > Chester Lin