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.
Thanks,
Chester Lin
