[7/10] ACPI / hotplug: Move container-specific code out of the core
diff mbox series

Message ID 2547688.l8Q1XKb3Ol@vostro.rjw.lan
State New, archived
Headers show
Series
  • ACPI: Device objects for all namespace nodes and PCI root hotplug integration
Related show

Commit Message

Rafael J. Wysocki Nov. 17, 2013, 4:36 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move container-specific uevents from the core hotplug code to the
container scan handler's .attach() and .detach() callbacks.

This way the core will not have to special-case containers and
the uevents will be guaranteed to happen every time a container
is either scanned or trimmed as appropriate.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/container.c |   11 ++++++++---
 drivers/acpi/scan.c      |    8 +-------
 include/acpi/acpi_bus.h  |    7 -------
 3 files changed, 9 insertions(+), 17 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Yasuaki Ishimatsu Nov. 29, 2013, 2:36 a.m. UTC | #1
Hi Rafael,

Replying to this mail may be wrong.

Do you remember following your patch?
http://lkml.org/lkml/2013/2/23/97

I want to add autoeject variable in acpi_hotplug_profile structure and
set autoecjet of container device "false".

Currently, I have a problem on ejecting container device. Since linux-3.12,
container device is removed by acpi_scan_hot_remove.

I think this has two problems.

   1. easily fail
      My container device has CPU device and Memory device, and maximum size of
      memory is 3Tbyte. In my environment, hot removing container device fails
      on offlining memory if memory is used by application.
      I think if offlininig memory, we must retly to offline memory several
      times.
   2. cannot work with userland's application
      Hot removing CPU and memory on container device, we need take care of
      userland application. Before linux-3.12, container device just notifies
      KOBJ_OFFLINE to udev. So by using udev, if application binds to removed
      CPU or node, applications can change them before hot removing container
      device.
      Currently, KOBJ_OFFLINE is notified to udev. But acpi_scan_hot_remove
      also runs simultaneously for hot removing container device. So when
      applications runs for corresponding to the deletion of the devices,
      the devices may have been deleted.

I don't know what devices are on hotpluggable conatainer device of other
vendors. At least, my container device cannot be hot removed correctly.
Then I want to add autoeject variable in acpi_hotplug_profile so that user
can change the parameter to "true" or "false".

Thanks,
Yasuaki Ishimatsu

(2013/11/18 1:36), Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move container-specific uevents from the core hotplug code to the
> container scan handler's .attach() and .detach() callbacks.
>
> This way the core will not have to special-case containers and
> the uevents will be guaranteed to happen every time a container
> is either scanned or trimmed as appropriate.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/container.c |   11 ++++++++---
>   drivers/acpi/scan.c      |    8 +-------
>   include/acpi/acpi_bus.h  |    7 -------
>   3 files changed, 9 insertions(+), 17 deletions(-)
>
> Index: linux-pm/drivers/acpi/container.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/container.c
> +++ linux-pm/drivers/acpi/container.c
> @@ -44,19 +44,24 @@ static const struct acpi_device_id conta
>   	{"", 0},
>   };
>
> -static int container_device_attach(struct acpi_device *device,
> +static int container_device_attach(struct acpi_device *adev,
>   				   const struct acpi_device_id *not_used)
>   {
> -	/* This is necessary for container hotplug to work. */
> +	kobject_uevent(&adev->dev.kobj, KOBJ_ONLINE);
>   	return 1;
>   }
>
> +static void container_device_detach(struct acpi_device *adev)
> +{
> +	kobject_uevent(&adev->dev.kobj, KOBJ_OFFLINE);
> +}
> +
>   static struct acpi_scan_handler container_handler = {
>   	.ids = container_device_ids,
>   	.attach = container_device_attach,
> +	.detach = container_device_detach,
>   	.hotplug = {
>   		.enabled = true,
> -		.mode = AHM_CONTAINER,
>   	},
>   };
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -206,9 +206,6 @@ static int acpi_scan_hot_remove(struct a
>   	acpi_status status;
>   	unsigned long long sta;
>
> -	if (device->handler && device->handler->hotplug.mode == AHM_CONTAINER)
> -		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> -
>   	/*
>   	 * Carry out two passes here and ignore errors in the first pass,
>   	 * because if the devices in question are memory blocks and
> @@ -288,10 +285,7 @@ static int acpi_scan_device_check(struct
>   		dev_warn(&adev->dev, "Namespace scan failure\n");
>   		return error;
>   	}
> -	if (adev->handler) {
> -		if (adev->handler->hotplug.mode == AHM_CONTAINER)
> -			kobject_uevent(&adev->dev.kobj, KOBJ_ONLINE);
> -	} else {
> +	if (!adev->handler) {
>   		dev_warn(&adev->dev, "Enumeration failure\n");
>   		return -ENODEV;
>   	}
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -91,16 +91,9 @@ struct acpi_device;
>    * -----------------
>    */
>
> -enum acpi_hotplug_mode {
> -	AHM_GENERIC = 0,
> -	AHM_CONTAINER,
> -	AHM_COUNT
> -};
> -
>   struct acpi_hotplug_profile {
>   	struct kobject kobj;
>   	bool enabled:1;
> -	enum acpi_hotplug_mode mode;
>   	int (*scan_dependent)(struct acpi_device *adev);
>   };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Nov. 29, 2013, 1:08 p.m. UTC | #2
On Friday, November 29, 2013 11:36:55 AM Yasuaki Ishimatsu wrote:
> Hi Rafael,

Hi,

> Replying to this mail may be wrong.

OK, so this particular patch doesn't break things any more?

> Do you remember following your patch?
> http://lkml.org/lkml/2013/2/23/97
> 
> I want to add autoeject variable in acpi_hotplug_profile structure and
> set autoecjet of container device "false".

Then after the series the $subject patch belongs to it will work almost the
same way as /sys/firmware/acpi/container/enabled (hot add will still work after
patch [4/10] if "enabled" is 0), but only for containers.

> Currently, I have a problem on ejecting container device. Since linux-3.12,
> container device is removed by acpi_scan_hot_remove.
> 
> I think this has two problems.
> 
>    1. easily fail
>       My container device has CPU device and Memory device, and maximum size of
>       memory is 3Tbyte. In my environment, hot removing container device fails
>       on offlining memory if memory is used by application.
>       I think if offlininig memory, we must retly to offline memory several
>       times.

Yes, that's correct.  But then you can try to offline the memory upfront
and only remove the container after that has been successful.

>    2. cannot work with userland's application
>       Hot removing CPU and memory on container device, we need take care of
>       userland application. Before linux-3.12, container device just notifies
>       KOBJ_OFFLINE to udev. So by using udev, if application binds to removed
>       CPU or node, applications can change them before hot removing container
>       device.
>       Currently, KOBJ_OFFLINE is notified to udev. But acpi_scan_hot_remove
>       also runs simultaneously for hot removing container device. So when
>       applications runs for corresponding to the deletion of the devices,
>       the devices may have been deleted.

So the expectation is that the container will refuse to offline, but instead
it will emit KOBJ_OFFLINE so that user space can do some cleanup and offline
it through the "eject" attribute, right?

> I don't know what devices are on hotpluggable conatainer device of other
> vendors. At least, my container device cannot be hot removed correctly.
> Then I want to add autoeject variable in acpi_hotplug_profile so that user
> can change the parameter to "true" or "false".

I have a different idea.

Why don't we create a bus type for containers in analogy with CPUs and memory
and make it support offline.  Then, the container scan handler will create a
"physical" container device under that bus type and the new bus type code will
implement the logic you need (that is, it will have a sysfs flag that will
cause the offline to fail emitting a uevent of some sort if set and will allow
the offline to happen when unset).  That "physical" container device will go
away (again, via the container scan handler) during container removal.

The eject work flow can be:
  (1) an eject event occurs,
  (2) the container "physical" device fails offline in acpi_scan_hot_remove()
      emmitting, say, KOBJ_CHANGE for the "physical" device,
  (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
  (4) user space changes the "physical" container device flag controlling
      offline to 0,
  (5) user space uses the sysfs "eject" attribute of the ACPI container object
      to finally eject the container,
  (6) the offline in acpi_scan_hot_remove() is now successful, because the
      flag controlling it has been set to 0 in step (4),
  (7) the "physical" container device goes away before executing _EJ0,
  (8) the container is ejected.

Of course, if the flag controlling container offline is 0 to start with, step
(6) will now occur directly after (1), so whoever wants containers to be
hot-removed automatically may just clear that flag for all of them on boot.

How does that sound?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yasuaki Ishimatsu Dec. 3, 2013, 2:46 a.m. UTC | #3
(2013/11/29 22:08), Rafael J. Wysocki wrote:
> On Friday, November 29, 2013 11:36:55 AM Yasuaki Ishimatsu wrote:
>> Hi Rafael,
>
> Hi,
>
>> Replying to this mail may be wrong.
>
> OK, so this particular patch doesn't break things any more?

Yes.

>
>> Do you remember following your patch?
>> http://lkml.org/lkml/2013/2/23/97
>>
>> I want to add autoeject variable in acpi_hotplug_profile structure and
>> set autoecjet of container device "false".
>
> Then after the series the $subject patch belongs to it will work almost the
> same way as /sys/firmware/acpi/container/enabled (hot add will still work after
> patch [4/10] if "enabled" is 0), but only for containers.
>
>> Currently, I have a problem on ejecting container device. Since linux-3.12,
>> container device is removed by acpi_scan_hot_remove.
>>
>> I think this has two problems.
>>
>>     1. easily fail
>>        My container device has CPU device and Memory device, and maximum size of
>>        memory is 3Tbyte. In my environment, hot removing container device fails
>>        on offlining memory if memory is used by application.
>>        I think if offlininig memory, we must retly to offline memory several
>>        times.
>
> Yes, that's correct.  But then you can try to offline the memory upfront
> and only remove the container after that has been successful.
>
>>     2. cannot work with userland's application
>>        Hot removing CPU and memory on container device, we need take care of
>>        userland application. Before linux-3.12, container device just notifies
>>        KOBJ_OFFLINE to udev. So by using udev, if application binds to removed
>>        CPU or node, applications can change them before hot removing container
>>        device.
>>        Currently, KOBJ_OFFLINE is notified to udev. But acpi_scan_hot_remove
>>        also runs simultaneously for hot removing container device. So when
>>        applications runs for corresponding to the deletion of the devices,
>>        the devices may have been deleted.
>

> So the expectation is that the container will refuse to offline, but instead
> it will emit KOBJ_OFFLINE so that user space can do some cleanup and offline
> it through the "eject" attribute, right?

Yes, that's right.

>
>> I don't know what devices are on hotpluggable conatainer device of other
>> vendors. At least, my container device cannot be hot removed correctly.
>> Then I want to add autoeject variable in acpi_hotplug_profile so that user
>> can change the parameter to "true" or "false".
>
> I have a different idea.
>
> Why don't we create a bus type for containers in analogy with CPUs and memory
> and make it support offline.  Then, the container scan handler will create a
> "physical" container device under that bus type and the new bus type code will
> implement the logic you need (that is, it will have a sysfs flag that will
> cause the offline to fail emitting a uevent of some sort if set and will allow
> the offline to happen when unset).  That "physical" container device will go
> away (again, via the container scan handler) during container removal.
>

> The eject work flow can be:
>    (1) an eject event occurs,
>    (2) the container "physical" device fails offline in acpi_scan_hot_remove()
>        emmitting, say, KOBJ_CHANGE for the "physical" device,
>    (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
>    (4) user space changes the "physical" container device flag controlling
>        offline to 0,
>    (5) user space uses the sysfs "eject" attribute of the ACPI container object
>        to finally eject the container,
>    (6) the offline in acpi_scan_hot_remove() is now successful, because the
>        flag controlling it has been set to 0 in step (4),
>    (7) the "physical" container device goes away before executing _EJ0,
>    (8) the container is ejected.
>
> Of course, if the flag controlling container offline is 0 to start with, step
> (6) will now occur directly after (1), so whoever wants containers to be
> hot-removed automatically may just clear that flag for all of them on boot.
>
> How does that sound?


The above ideas are almost O.K. I want kernel to notify user space of KOBJ_OFFLINE.
Even if user space catches "KOBJ_CHANGE", user doesn't know whether the notification
is offline or not.

Thanks,
Yasuaki Ishimatsu

> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 3, 2013, 1:15 p.m. UTC | #4
On Tuesday, December 03, 2013 11:46:24 AM Yasuaki Ishimatsu wrote:
> (2013/11/29 22:08), Rafael J. Wysocki wrote:
> > On Friday, November 29, 2013 11:36:55 AM Yasuaki Ishimatsu wrote:
> >> Hi Rafael,
> >
> > Hi,
> >
> >> Replying to this mail may be wrong.
> >
> > OK, so this particular patch doesn't break things any more?
> 
> Yes.
> 
> >
> >> Do you remember following your patch?
> >> http://lkml.org/lkml/2013/2/23/97
> >>
> >> I want to add autoeject variable in acpi_hotplug_profile structure and
> >> set autoecjet of container device "false".
> >
> > Then after the series the $subject patch belongs to it will work almost the
> > same way as /sys/firmware/acpi/container/enabled (hot add will still work after
> > patch [4/10] if "enabled" is 0), but only for containers.
> >
> >> Currently, I have a problem on ejecting container device. Since linux-3.12,
> >> container device is removed by acpi_scan_hot_remove.
> >>
> >> I think this has two problems.
> >>
> >>     1. easily fail
> >>        My container device has CPU device and Memory device, and maximum size of
> >>        memory is 3Tbyte. In my environment, hot removing container device fails
> >>        on offlining memory if memory is used by application.
> >>        I think if offlininig memory, we must retly to offline memory several
> >>        times.
> >
> > Yes, that's correct.  But then you can try to offline the memory upfront
> > and only remove the container after that has been successful.
> >
> >>     2. cannot work with userland's application
> >>        Hot removing CPU and memory on container device, we need take care of
> >>        userland application. Before linux-3.12, container device just notifies
> >>        KOBJ_OFFLINE to udev. So by using udev, if application binds to removed
> >>        CPU or node, applications can change them before hot removing container
> >>        device.
> >>        Currently, KOBJ_OFFLINE is notified to udev. But acpi_scan_hot_remove
> >>        also runs simultaneously for hot removing container device. So when
> >>        applications runs for corresponding to the deletion of the devices,
> >>        the devices may have been deleted.
> >
> 
> > So the expectation is that the container will refuse to offline, but instead
> > it will emit KOBJ_OFFLINE so that user space can do some cleanup and offline
> > it through the "eject" attribute, right?
> 
> Yes, that's right.
> 
> >
> >> I don't know what devices are on hotpluggable conatainer device of other
> >> vendors. At least, my container device cannot be hot removed correctly.
> >> Then I want to add autoeject variable in acpi_hotplug_profile so that user
> >> can change the parameter to "true" or "false".
> >
> > I have a different idea.
> >
> > Why don't we create a bus type for containers in analogy with CPUs and memory
> > and make it support offline.  Then, the container scan handler will create a
> > "physical" container device under that bus type and the new bus type code will
> > implement the logic you need (that is, it will have a sysfs flag that will
> > cause the offline to fail emitting a uevent of some sort if set and will allow
> > the offline to happen when unset).  That "physical" container device will go
> > away (again, via the container scan handler) during container removal.
> >
> 
> > The eject work flow can be:
> >    (1) an eject event occurs,
> >    (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> >        emmitting, say, KOBJ_CHANGE for the "physical" device,
> >    (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> >    (4) user space changes the "physical" container device flag controlling
> >        offline to 0,
> >    (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >        to finally eject the container,
> >    (6) the offline in acpi_scan_hot_remove() is now successful, because the
> >        flag controlling it has been set to 0 in step (4),
> >    (7) the "physical" container device goes away before executing _EJ0,
> >    (8) the container is ejected.
> >
> > Of course, if the flag controlling container offline is 0 to start with, step
> > (6) will now occur directly after (1), so whoever wants containers to be
> > hot-removed automatically may just clear that flag for all of them on boot.
> >
> > How does that sound?
> 
> 
> The above ideas are almost O.K. I want kernel to notify user space of KOBJ_OFFLINE.
> Even if user space catches "KOBJ_CHANGE", user doesn't know whether the notification
> is offline or not.

It is easy to figure out, though.  Since the KOBJ_CHANGE will be emitted for
container devices only in that situation, user space can see that (1) it is
from a container and (2) it is KOBJ_CHANGE, so it must mean "container offline
has been attempted".

My concern with using KOBJ_OFFLINE for that is that device_offline() emits it
too on success and it may be easily confused with the one emitted on failure
for containers.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yasuaki Ishimatsu Dec. 4, 2013, 5:43 a.m. UTC | #5
(2013/12/03 22:15), Rafael J. Wysocki wrote:
> On Tuesday, December 03, 2013 11:46:24 AM Yasuaki Ishimatsu wrote:
>> (2013/11/29 22:08), Rafael J. Wysocki wrote:
>>> On Friday, November 29, 2013 11:36:55 AM Yasuaki Ishimatsu wrote:
>>>> Hi Rafael,
>>>
>>> Hi,
>>>
>>>> Replying to this mail may be wrong.
>>>
>>> OK, so this particular patch doesn't break things any more?
>>
>> Yes.
>>
>>>
>>>> Do you remember following your patch?
>>>> http://lkml.org/lkml/2013/2/23/97
>>>>
>>>> I want to add autoeject variable in acpi_hotplug_profile structure and
>>>> set autoecjet of container device "false".
>>>
>>> Then after the series the $subject patch belongs to it will work almost the
>>> same way as /sys/firmware/acpi/container/enabled (hot add will still work after
>>> patch [4/10] if "enabled" is 0), but only for containers.
>>>
>>>> Currently, I have a problem on ejecting container device. Since linux-3.12,
>>>> container device is removed by acpi_scan_hot_remove.
>>>>
>>>> I think this has two problems.
>>>>
>>>>      1. easily fail
>>>>         My container device has CPU device and Memory device, and maximum size of
>>>>         memory is 3Tbyte. In my environment, hot removing container device fails
>>>>         on offlining memory if memory is used by application.
>>>>         I think if offlininig memory, we must retly to offline memory several
>>>>         times.
>>>
>>> Yes, that's correct.  But then you can try to offline the memory upfront
>>> and only remove the container after that has been successful.
>>>
>>>>      2. cannot work with userland's application
>>>>         Hot removing CPU and memory on container device, we need take care of
>>>>         userland application. Before linux-3.12, container device just notifies
>>>>         KOBJ_OFFLINE to udev. So by using udev, if application binds to removed
>>>>         CPU or node, applications can change them before hot removing container
>>>>         device.
>>>>         Currently, KOBJ_OFFLINE is notified to udev. But acpi_scan_hot_remove
>>>>         also runs simultaneously for hot removing container device. So when
>>>>         applications runs for corresponding to the deletion of the devices,
>>>>         the devices may have been deleted.
>>>
>>
>>> So the expectation is that the container will refuse to offline, but instead
>>> it will emit KOBJ_OFFLINE so that user space can do some cleanup and offline
>>> it through the "eject" attribute, right?
>>
>> Yes, that's right.
>>
>>>
>>>> I don't know what devices are on hotpluggable conatainer device of other
>>>> vendors. At least, my container device cannot be hot removed correctly.
>>>> Then I want to add autoeject variable in acpi_hotplug_profile so that user
>>>> can change the parameter to "true" or "false".
>>>
>>> I have a different idea.
>>>
>>> Why don't we create a bus type for containers in analogy with CPUs and memory
>>> and make it support offline.  Then, the container scan handler will create a
>>> "physical" container device under that bus type and the new bus type code will
>>> implement the logic you need (that is, it will have a sysfs flag that will
>>> cause the offline to fail emitting a uevent of some sort if set and will allow
>>> the offline to happen when unset).  That "physical" container device will go
>>> away (again, via the container scan handler) during container removal.
>>>
>>
>>> The eject work flow can be:
>>>     (1) an eject event occurs,
>>>     (2) the container "physical" device fails offline in acpi_scan_hot_remove()
>>>         emmitting, say, KOBJ_CHANGE for the "physical" device,
>>>     (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
>>>     (4) user space changes the "physical" container device flag controlling
>>>         offline to 0,
>>>     (5) user space uses the sysfs "eject" attribute of the ACPI container object
>>>         to finally eject the container,
>>>     (6) the offline in acpi_scan_hot_remove() is now successful, because the
>>>         flag controlling it has been set to 0 in step (4),
>>>     (7) the "physical" container device goes away before executing _EJ0,
>>>     (8) the container is ejected.
>>>
>>> Of course, if the flag controlling container offline is 0 to start with, step
>>> (6) will now occur directly after (1), so whoever wants containers to be
>>> hot-removed automatically may just clear that flag for all of them on boot.
>>>
>>> How does that sound?
>>
>>
>> The above ideas are almost O.K. I want kernel to notify user space of KOBJ_OFFLINE.
>> Even if user space catches "KOBJ_CHANGE", user doesn't know whether the notification
>> is offline or not.
>

> It is easy to figure out, though.  Since the KOBJ_CHANGE will be emitted for
> container devices only in that situation, user space can see that (1) it is
> from a container and (2) it is KOBJ_CHANGE, so it must mean "container offline
> has been attempted".
>
> My concern with using KOBJ_OFFLINE for that is that device_offline() emits it
> too on success and it may be easily confused with the one emitted on failure
> for containers.

I have no objection.

Thanks,
Yasuaki Ishimatsu

>
> Thanks,
> Rafael
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yasuaki Ishimatsu Dec. 13, 2013, 2:56 a.m. UTC | #6
Hi Rafael,

Please share your more detailed idea. I started to implement the following
idea. But the idea has one problem.

>>> The eject work flow can be:
>>>     (1) an eject event occurs,
>>>     (2) the container "physical" device fails offline in acpi_scan_hot_remove()
>>>         emmitting, say, KOBJ_CHANGE for the "physical" device,
>>>     (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
>>>     (4) user space changes the "physical" container device flag controlling
>>>         offline to 0,
>>>     (5) user space uses the sysfs "eject" attribute of the ACPI container object
>>>         to finally eject the container,
>>>     (6) the offline in acpi_scan_hot_remove() is now successful, because the
>>>         flag controlling it has been set to 0 in step (4),
>>>     (7) the "physical" container device goes away before executing _EJ0,
>>>     (8) the container is ejected.

I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
But acpi_scan_hot_remove() offlines devices on container device at first.
So when offline container device, devices on container has been offlined.

Thus the idea cannot fulfill my necessary feature.

Thanks,
Yasauaki Ishimatsu

Thanks,
Yasauaki Ishimatsu

>>>
>>> Of course, if the flag controlling container offline is 0 to start with, step
>>> (6) will now occur directly after (1), so whoever wants containers to be
>>> hot-removed automatically may just clear that flag for all of them on boot.
>>>
>>> How does that sound?
>>
>>
>> The above ideas are almost O.K. I want kernel to notify user space of KOBJ_OFFLINE.
>> Even if user space catches "KOBJ_CHANGE", user doesn't know whether the notification
>> is offline or not.
>
> It is easy to figure out, though.  Since the KOBJ_CHANGE will be emitted for
> container devices only in that situation, user space can see that (1) it is
> from a container and (2) it is KOBJ_CHANGE, so it must mean "container offline
> has been attempted".
>
> My concern with using KOBJ_OFFLINE for that is that device_offline() emits it
> too on success and it may be easily confused with the one emitted on failure
> for containers.
>
> Thanks,
> Rafael
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 13, 2013, 4:56 a.m. UTC | #7
On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
> Hi Rafael,

Hi,

> Please share your more detailed idea. I started to implement the following
> idea. But the idea has one problem.
> 
> >>> The eject work flow can be:
> >>>     (1) an eject event occurs,
> >>>     (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> >>>         emmitting, say, KOBJ_CHANGE for the "physical" device,
> >>>     (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> >>>     (4) user space changes the "physical" container device flag controlling
> >>>         offline to 0,
> >>>     (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >>>         to finally eject the container,
> >>>     (6) the offline in acpi_scan_hot_remove() is now successful, because the
> >>>         flag controlling it has been set to 0 in step (4),
> >>>     (7) the "physical" container device goes away before executing _EJ0,
> >>>     (8) the container is ejected.
> 
> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
> But acpi_scan_hot_remove() offlines devices on container device at first.
> So when offline container device, devices on container has been offlined.
> 
> Thus the idea cannot fulfill my necessary feature.

Well, in that case we need to treat containers in a special way at the ACPI
level.  Which is a bit unfortunate so to speak.

To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
.verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
check if all of the "physical" companions of the top-level device are offline
to start with, and if not, it would just emit KOBJ_CHANGE for the companions
that are not offline and bail out.

So the above algorithm would become:

(1) an eject event occurs,
(2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
    scan_handler structure,
(3) if set (it would always be set for containers), acpi_scan_hot_remove()
    checks the status of the target device's "physical" companions; if at least
    one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
    and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
    for the first companion that is not offline at this point.]
(4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
    process it carries out the offline operation for the container's "physical"
    companion (there's only one such companion for each container), [That
    operation for the container itself is trivial, but to succeed it requires
    all devices below the container to be taken offline in advance.]
(5) user space uses the sysfs "eject" attribute of the ACPI container object
    to finally eject the container,
(6) acpi_scan_hot_remove() is now successful, because the container's "physical"
    companion is now offline,
(7) the "physical" container device goes away before executing _EJ0,
(8) the container is ejected.

I think that should work for you.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yasuaki Ishimatsu Dec. 13, 2013, 5:17 a.m. UTC | #8
(2013/12/13 13:56), Rafael J. Wysocki wrote:
> On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
>> Hi Rafael,
>
> Hi,
>
>> Please share your more detailed idea. I started to implement the following
>> idea. But the idea has one problem.
>>
>>>>> The eject work flow can be:
>>>>>      (1) an eject event occurs,
>>>>>      (2) the container "physical" device fails offline in acpi_scan_hot_remove()
>>>>>          emmitting, say, KOBJ_CHANGE for the "physical" device,
>>>>>      (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
>>>>>      (4) user space changes the "physical" container device flag controlling
>>>>>          offline to 0,
>>>>>      (5) user space uses the sysfs "eject" attribute of the ACPI container object
>>>>>          to finally eject the container,
>>>>>      (6) the offline in acpi_scan_hot_remove() is now successful, because the
>>>>>          flag controlling it has been set to 0 in step (4),
>>>>>      (7) the "physical" container device goes away before executing _EJ0,
>>>>>      (8) the container is ejected.
>>
>> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
>> But acpi_scan_hot_remove() offlines devices on container device at first.
>> So when offline container device, devices on container has been offlined.
>>
>> Thus the idea cannot fulfill my necessary feature.
>
> Well, in that case we need to treat containers in a special way at the ACPI
> level.  Which is a bit unfortunate so to speak.
>
> To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
> .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
> check if all of the "physical" companions of the top-level device are offline
> to start with, and if not, it would just emit KOBJ_CHANGE for the companions
> that are not offline and bail out.
>
> So the above algorithm would become:
>
> (1) an eject event occurs,
> (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
>      scan_handler structure,
> (3) if set (it would always be set for containers), acpi_scan_hot_remove()
>      checks the status of the target device's "physical" companions; if at least
>      one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
>      and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
>      for the first companion that is not offline at this point.]
> (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
>      process it carries out the offline operation for the container's "physical"
>      companion (there's only one such companion for each container), [That
>      operation for the container itself is trivial, but to succeed it requires
>      all devices below the container to be taken offline in advance.]
> (5) user space uses the sysfs "eject" attribute of the ACPI container object
>      to finally eject the container,
> (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
>      companion is now offline,
> (7) the "physical" container device goes away before executing _EJ0,
> (8) the container is ejected.
>
> I think that should work for you.

This idea seems to same as your previous work.
http://lkml.org/lkml/2013/2/23/97

How about add autoremove flag into acpi_hotplug_profile and check it as follow:

---
  drivers/acpi/scan.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5383c81..c43d110 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
  			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
  			goto err_out;
  		}
+		if (!handler->hotplug.autoremove) {
+			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
+			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
+			goto err_out;
+		}
  		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
  					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
  		break;

Adding the check into "acpi_hotplug_notify_cb()", user need not change the
flag for removing container device by "sysfs eject".

Thanks,
Yasuaki Ishimatsu

>
> Thanks,
> Rafael
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 14, 2013, 5:07 a.m. UTC | #9
On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote:
> (2013/12/13 13:56), Rafael J. Wysocki wrote:
> > On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
> >> Hi Rafael,
> >
> > Hi,
> >
> >> Please share your more detailed idea. I started to implement the following
> >> idea. But the idea has one problem.
> >>
> >>>>> The eject work flow can be:
> >>>>>      (1) an eject event occurs,
> >>>>>      (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> >>>>>          emmitting, say, KOBJ_CHANGE for the "physical" device,
> >>>>>      (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> >>>>>      (4) user space changes the "physical" container device flag controlling
> >>>>>          offline to 0,
> >>>>>      (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >>>>>          to finally eject the container,
> >>>>>      (6) the offline in acpi_scan_hot_remove() is now successful, because the
> >>>>>          flag controlling it has been set to 0 in step (4),
> >>>>>      (7) the "physical" container device goes away before executing _EJ0,
> >>>>>      (8) the container is ejected.
> >>
> >> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
> >> But acpi_scan_hot_remove() offlines devices on container device at first.
> >> So when offline container device, devices on container has been offlined.
> >>
> >> Thus the idea cannot fulfill my necessary feature.
> >
> > Well, in that case we need to treat containers in a special way at the ACPI
> > level.  Which is a bit unfortunate so to speak.
> >
> > To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
> > .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
> > check if all of the "physical" companions of the top-level device are offline
> > to start with, and if not, it would just emit KOBJ_CHANGE for the companions
> > that are not offline and bail out.
> >
> > So the above algorithm would become:
> >
> > (1) an eject event occurs,
> > (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
> >      scan_handler structure,
> > (3) if set (it would always be set for containers), acpi_scan_hot_remove()
> >      checks the status of the target device's "physical" companions; if at least
> >      one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
> >      and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
> >      for the first companion that is not offline at this point.]
> > (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
> >      process it carries out the offline operation for the container's "physical"
> >      companion (there's only one such companion for each container), [That
> >      operation for the container itself is trivial, but to succeed it requires
> >      all devices below the container to be taken offline in advance.]
> > (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >      to finally eject the container,
> > (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
> >      companion is now offline,
> > (7) the "physical" container device goes away before executing _EJ0,
> > (8) the container is ejected.
> >
> > I think that should work for you.
> 
> This idea seems to same as your previous work.
> http://lkml.org/lkml/2013/2/23/97

No, it is not.  That one didn't involve physical device representations.

> How about add autoremove flag into acpi_hotplug_profile and check it as follow:

This is very similar to "enable" except that it generates the uevent and
"enable" doesn't.  You might as well modify "enable" to trigger a uevent if
eject is not enabled (note that with the latest patches in linux-next "enable"
only applies to eject).

That said I don't think we should generate any uevents for struct acpi_device
objects, because they are not devices.

> ---
>   drivers/acpi/scan.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5383c81..c43d110 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
>   			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
>   			goto err_out;
>   		}
> +		if (!handler->hotplug.autoremove) {
> +			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
> +			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
> +			goto err_out;
> +		}
>   		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
>   					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
>   		break;
> 
> Adding the check into "acpi_hotplug_notify_cb()", user need not change the
> flag for removing container device by "sysfs eject".

Which is utterly confusing.  There is no reason whatsoever why the sysfs eject
attribute should work differently from the event-triggered eject.  Quite the
opposite is the case: it should work in the same way in my opinion so that it
is possible to test the eject code path using that attribute.

I'm traveling now, but when I get back home (next week), I'll try to implement
the thing I was talking about above.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 23, 2013, 1:58 p.m. UTC | #10
On Saturday, December 14, 2013 06:07:06 AM Rafael J. Wysocki wrote:
> On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote:
> > (2013/12/13 13:56), Rafael J. Wysocki wrote:
> > > On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
> > >> Hi Rafael,
> > >
> > > Hi,
> > >
> > >> Please share your more detailed idea. I started to implement the following
> > >> idea. But the idea has one problem.
> > >>
> > >>>>> The eject work flow can be:
> > >>>>>      (1) an eject event occurs,
> > >>>>>      (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> > >>>>>          emmitting, say, KOBJ_CHANGE for the "physical" device,
> > >>>>>      (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> > >>>>>      (4) user space changes the "physical" container device flag controlling
> > >>>>>          offline to 0,
> > >>>>>      (5) user space uses the sysfs "eject" attribute of the ACPI container object
> > >>>>>          to finally eject the container,
> > >>>>>      (6) the offline in acpi_scan_hot_remove() is now successful, because the
> > >>>>>          flag controlling it has been set to 0 in step (4),
> > >>>>>      (7) the "physical" container device goes away before executing _EJ0,
> > >>>>>      (8) the container is ejected.
> > >>
> > >> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
> > >> But acpi_scan_hot_remove() offlines devices on container device at first.
> > >> So when offline container device, devices on container has been offlined.
> > >>
> > >> Thus the idea cannot fulfill my necessary feature.
> > >
> > > Well, in that case we need to treat containers in a special way at the ACPI
> > > level.  Which is a bit unfortunate so to speak.
> > >
> > > To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
> > > .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
> > > check if all of the "physical" companions of the top-level device are offline
> > > to start with, and if not, it would just emit KOBJ_CHANGE for the companions
> > > that are not offline and bail out.
> > >
> > > So the above algorithm would become:
> > >
> > > (1) an eject event occurs,
> > > (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
> > >      scan_handler structure,
> > > (3) if set (it would always be set for containers), acpi_scan_hot_remove()
> > >      checks the status of the target device's "physical" companions; if at least
> > >      one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
> > >      and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
> > >      for the first companion that is not offline at this point.]
> > > (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
> > >      process it carries out the offline operation for the container's "physical"
> > >      companion (there's only one such companion for each container), [That
> > >      operation for the container itself is trivial, but to succeed it requires
> > >      all devices below the container to be taken offline in advance.]
> > > (5) user space uses the sysfs "eject" attribute of the ACPI container object
> > >      to finally eject the container,
> > > (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
> > >      companion is now offline,
> > > (7) the "physical" container device goes away before executing _EJ0,
> > > (8) the container is ejected.
> > >
> > > I think that should work for you.
> > 
> > This idea seems to same as your previous work.
> > http://lkml.org/lkml/2013/2/23/97
> 
> No, it is not.  That one didn't involve physical device representations.
> 
> > How about add autoremove flag into acpi_hotplug_profile and check it as follow:
> 
> This is very similar to "enable" except that it generates the uevent and
> "enable" doesn't.  You might as well modify "enable" to trigger a uevent if
> eject is not enabled (note that with the latest patches in linux-next "enable"
> only applies to eject).
> 
> That said I don't think we should generate any uevents for struct acpi_device
> objects, because they are not devices.
> 
> > ---
> >   drivers/acpi/scan.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 5383c81..c43d110 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
> >   			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> >   			goto err_out;
> >   		}
> > +		if (!handler->hotplug.autoremove) {
> > +			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
> > +			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
> > +			goto err_out;
> > +		}
> >   		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> >   					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> >   		break;
> > 
> > Adding the check into "acpi_hotplug_notify_cb()", user need not change the
> > flag for removing container device by "sysfs eject".
> 
> Which is utterly confusing.  There is no reason whatsoever why the sysfs eject
> attribute should work differently from the event-triggered eject.  Quite the
> opposite is the case: it should work in the same way in my opinion so that it
> is possible to test the eject code path using that attribute.
> 
> I'm traveling now, but when I get back home (next week), I'll try to implement
> the thing I was talking about above.

It took some more time than I had expected, but I finally was able to get to that.

The following two patches implement the idea.  This is the minimum (in my opinion)
implementation and it may be extended in some ways.

Patch [1/2] introduces a new demand_offline flag for struct acpi_hotplug_profile
that makes acpi_scan_hot_remove() check the offline status of the device object's
companion physical devices to start with and return -EBUSY if at least one of them
is not offline.

Patch [2/2] uses that flag to implement the container handling.  The details are
in the changelog, but that's how it is supposed to work.

During the initial namespace scan the container ACPI scan handler should create
"physical" system container device under /sys/devices/system/container/ for
each ACPI container object (the sysfs name of that device should be the same as
the sysfs name of the corresponding container object and they should be linked
to each other via the firmware_node and physical_node symbolic links, respectively).
Those system container devices are initially online.

When a container eject event happens, acpi_scan_hot_remove() will notice that
hotplug.demand_offline is set in the device object's scan handler and will
check the online status of its "physical" companion device, which is online
(that is the system container device the above paragraph is about).  That will
cause KOBJ_CHANGE to be emitted for the system container device and -EBUSY to
be returned by acpi_scan_hot_remove().

Now, user space needs to offline the system container device through its online
sysfs attribute (that should be present, because the bus type for containers
provides the online and offline callbacks).  However, the offline for system
container devices will only succeed if the physical devices right below the
container are all offline, so user space will have to offline those devices
before attempting to offline the system container device itself.  When
finished, user space can trigger the container removal with the help of the
eject sysfs attribute of the ACPI container object pointed to by the system
container device's firmware_node link (this time the check in
acpi_scan_hot_remove() will succeed, because the system container device in
question is now offline).

The way it is implemented is a bit hackish (the driver_data pointer is slightly
abused), but that's a special case and I wanted to avoid adding new fields to
struct device just for handling it.

The patches haven't been tested yet.  I'm going to do that later today, but
first I need to take care of some other things, so that has to wait.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 26, 2013, 1:01 a.m. UTC | #11
On Monday, December 23, 2013 02:58:38 PM Rafael J. Wysocki wrote:
> On Saturday, December 14, 2013 06:07:06 AM Rafael J. Wysocki wrote:
> > On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote:
> > > (2013/12/13 13:56), Rafael J. Wysocki wrote:
> > > > On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
> > > >> Hi Rafael,
> > > >
> > > > Hi,
> > > >
> > > >> Please share your more detailed idea. I started to implement the following
> > > >> idea. But the idea has one problem.
> > > >>
> > > >>>>> The eject work flow can be:
> > > >>>>>      (1) an eject event occurs,
> > > >>>>>      (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> > > >>>>>          emmitting, say, KOBJ_CHANGE for the "physical" device,
> > > >>>>>      (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> > > >>>>>      (4) user space changes the "physical" container device flag controlling
> > > >>>>>          offline to 0,
> > > >>>>>      (5) user space uses the sysfs "eject" attribute of the ACPI container object
> > > >>>>>          to finally eject the container,
> > > >>>>>      (6) the offline in acpi_scan_hot_remove() is now successful, because the
> > > >>>>>          flag controlling it has been set to 0 in step (4),
> > > >>>>>      (7) the "physical" container device goes away before executing _EJ0,
> > > >>>>>      (8) the container is ejected.
> > > >>
> > > >> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
> > > >> But acpi_scan_hot_remove() offlines devices on container device at first.
> > > >> So when offline container device, devices on container has been offlined.
> > > >>
> > > >> Thus the idea cannot fulfill my necessary feature.
> > > >
> > > > Well, in that case we need to treat containers in a special way at the ACPI
> > > > level.  Which is a bit unfortunate so to speak.
> > > >
> > > > To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
> > > > .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
> > > > check if all of the "physical" companions of the top-level device are offline
> > > > to start with, and if not, it would just emit KOBJ_CHANGE for the companions
> > > > that are not offline and bail out.
> > > >
> > > > So the above algorithm would become:
> > > >
> > > > (1) an eject event occurs,
> > > > (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
> > > >      scan_handler structure,
> > > > (3) if set (it would always be set for containers), acpi_scan_hot_remove()
> > > >      checks the status of the target device's "physical" companions; if at least
> > > >      one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
> > > >      and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
> > > >      for the first companion that is not offline at this point.]
> > > > (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
> > > >      process it carries out the offline operation for the container's "physical"
> > > >      companion (there's only one such companion for each container), [That
> > > >      operation for the container itself is trivial, but to succeed it requires
> > > >      all devices below the container to be taken offline in advance.]
> > > > (5) user space uses the sysfs "eject" attribute of the ACPI container object
> > > >      to finally eject the container,
> > > > (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
> > > >      companion is now offline,
> > > > (7) the "physical" container device goes away before executing _EJ0,
> > > > (8) the container is ejected.
> > > >
> > > > I think that should work for you.
> > > 
> > > This idea seems to same as your previous work.
> > > http://lkml.org/lkml/2013/2/23/97
> > 
> > No, it is not.  That one didn't involve physical device representations.
> > 
> > > How about add autoremove flag into acpi_hotplug_profile and check it as follow:
> > 
> > This is very similar to "enable" except that it generates the uevent and
> > "enable" doesn't.  You might as well modify "enable" to trigger a uevent if
> > eject is not enabled (note that with the latest patches in linux-next "enable"
> > only applies to eject).
> > 
> > That said I don't think we should generate any uevents for struct acpi_device
> > objects, because they are not devices.
> > 
> > > ---
> > >   drivers/acpi/scan.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 5383c81..c43d110 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
> > >   			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > >   			goto err_out;
> > >   		}
> > > +		if (!handler->hotplug.autoremove) {
> > > +			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
> > > +			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
> > > +			goto err_out;
> > > +		}
> > >   		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> > >   					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > >   		break;
> > > 
> > > Adding the check into "acpi_hotplug_notify_cb()", user need not change the
> > > flag for removing container device by "sysfs eject".
> > 
> > Which is utterly confusing.  There is no reason whatsoever why the sysfs eject
> > attribute should work differently from the event-triggered eject.  Quite the
> > opposite is the case: it should work in the same way in my opinion so that it
> > is possible to test the eject code path using that attribute.
> > 
> > I'm traveling now, but when I get back home (next week), I'll try to implement
> > the thing I was talking about above.
> 
> It took some more time than I had expected, but I finally was able to get to that.
> 
> The following two patches implement the idea.  This is the minimum (in my opinion)
> implementation and it may be extended in some ways.
> 
> Patch [1/2] introduces a new demand_offline flag for struct acpi_hotplug_profile
> that makes acpi_scan_hot_remove() check the offline status of the device object's
> companion physical devices to start with and return -EBUSY if at least one of them
> is not offline.
> 
> Patch [2/2] uses that flag to implement the container handling.  The details are
> in the changelog, but that's how it is supposed to work.
> 
> During the initial namespace scan the container ACPI scan handler should create
> "physical" system container device under /sys/devices/system/container/ for
> each ACPI container object (the sysfs name of that device should be the same as
> the sysfs name of the corresponding container object and they should be linked
> to each other via the firmware_node and physical_node symbolic links, respectively).
> Those system container devices are initially online.
> 
> When a container eject event happens, acpi_scan_hot_remove() will notice that
> hotplug.demand_offline is set in the device object's scan handler and will
> check the online status of its "physical" companion device, which is online
> (that is the system container device the above paragraph is about).  That will
> cause KOBJ_CHANGE to be emitted for the system container device and -EBUSY to
> be returned by acpi_scan_hot_remove().
> 
> Now, user space needs to offline the system container device through its online
> sysfs attribute (that should be present, because the bus type for containers
> provides the online and offline callbacks).  However, the offline for system
> container devices will only succeed if the physical devices right below the
> container are all offline, so user space will have to offline those devices
> before attempting to offline the system container device itself.  When
> finished, user space can trigger the container removal with the help of the
> eject sysfs attribute of the ACPI container object pointed to by the system
> container device's firmware_node link (this time the check in
> acpi_scan_hot_remove() will succeed, because the system container device in
> question is now offline).
> 
> The way it is implemented is a bit hackish (the driver_data pointer is slightly
> abused), but that's a special case and I wanted to avoid adding new fields to
> struct device just for handling it.
> 
> The patches haven't been tested yet.  I'm going to do that later today, but
> first I need to take care of some other things, so that has to wait.

The series of the two patches:

[1/2] ACPI / hotplug: Add demand_offline hotplug profile flag
	https://patchwork.kernel.org/patch/3396711/

[2/2] ACPI / hotplug / driver core: Handle containers in a special way
	https://patchwork.kernel.org/patch/3399081/

has been tested now and seems to work as expected, at least for a container
that has no children (that's one I could simulate easily in a meaningful way).

For this reason, if there are no objections, I'll resend them as an official
submission during the next couple of days.

Thanks!
Yasuaki Ishimatsu Dec. 26, 2013, 2:53 a.m. UTC | #12
HI Rafael,

(2013/12/26 10:01), Rafael J. Wysocki wrote:
> On Monday, December 23, 2013 02:58:38 PM Rafael J. Wysocki wrote:
>> On Saturday, December 14, 2013 06:07:06 AM Rafael J. Wysocki wrote:
>>> On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote:
>>>> (2013/12/13 13:56), Rafael J. Wysocki wrote:
>>>>> On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
>>>>>> Hi Rafael,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> Please share your more detailed idea. I started to implement the following
>>>>>> idea. But the idea has one problem.
>>>>>>
>>>>>>>>> The eject work flow can be:
>>>>>>>>>       (1) an eject event occurs,
>>>>>>>>>       (2) the container "physical" device fails offline in acpi_scan_hot_remove()
>>>>>>>>>           emmitting, say, KOBJ_CHANGE for the "physical" device,
>>>>>>>>>       (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
>>>>>>>>>       (4) user space changes the "physical" container device flag controlling
>>>>>>>>>           offline to 0,
>>>>>>>>>       (5) user space uses the sysfs "eject" attribute of the ACPI container object
>>>>>>>>>           to finally eject the container,
>>>>>>>>>       (6) the offline in acpi_scan_hot_remove() is now successful, because the
>>>>>>>>>           flag controlling it has been set to 0 in step (4),
>>>>>>>>>       (7) the "physical" container device goes away before executing _EJ0,
>>>>>>>>>       (8) the container is ejected.
>>>>>>
>>>>>> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
>>>>>> But acpi_scan_hot_remove() offlines devices on container device at first.
>>>>>> So when offline container device, devices on container has been offlined.
>>>>>>
>>>>>> Thus the idea cannot fulfill my necessary feature.
>>>>>
>>>>> Well, in that case we need to treat containers in a special way at the ACPI
>>>>> level.  Which is a bit unfortunate so to speak.
>>>>>
>>>>> To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
>>>>> .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
>>>>> check if all of the "physical" companions of the top-level device are offline
>>>>> to start with, and if not, it would just emit KOBJ_CHANGE for the companions
>>>>> that are not offline and bail out.
>>>>>
>>>>> So the above algorithm would become:
>>>>>
>>>>> (1) an eject event occurs,
>>>>> (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
>>>>>       scan_handler structure,
>>>>> (3) if set (it would always be set for containers), acpi_scan_hot_remove()
>>>>>       checks the status of the target device's "physical" companions; if at least
>>>>>       one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
>>>>>       and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
>>>>>       for the first companion that is not offline at this point.]
>>>>> (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
>>>>>       process it carries out the offline operation for the container's "physical"
>>>>>       companion (there's only one such companion for each container), [That
>>>>>       operation for the container itself is trivial, but to succeed it requires
>>>>>       all devices below the container to be taken offline in advance.]
>>>>> (5) user space uses the sysfs "eject" attribute of the ACPI container object
>>>>>       to finally eject the container,
>>>>> (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
>>>>>       companion is now offline,
>>>>> (7) the "physical" container device goes away before executing _EJ0,
>>>>> (8) the container is ejected.
>>>>>
>>>>> I think that should work for you.
>>>>
>>>> This idea seems to same as your previous work.
>>>> http://lkml.org/lkml/2013/2/23/97
>>>
>>> No, it is not.  That one didn't involve physical device representations.
>>>
>>>> How about add autoremove flag into acpi_hotplug_profile and check it as follow:
>>>
>>> This is very similar to "enable" except that it generates the uevent and
>>> "enable" doesn't.  You might as well modify "enable" to trigger a uevent if
>>> eject is not enabled (note that with the latest patches in linux-next "enable"
>>> only applies to eject).
>>>
>>> That said I don't think we should generate any uevents for struct acpi_device
>>> objects, because they are not devices.
>>>
>>>> ---
>>>>    drivers/acpi/scan.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>>> index 5383c81..c43d110 100644
>>>> --- a/drivers/acpi/scan.c
>>>> +++ b/drivers/acpi/scan.c
>>>> @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
>>>>    			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
>>>>    			goto err_out;
>>>>    		}
>>>> +		if (!handler->hotplug.autoremove) {
>>>> +			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
>>>> +			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
>>>> +			goto err_out;
>>>> +		}
>>>>    		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
>>>>    					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
>>>>    		break;
>>>>
>>>> Adding the check into "acpi_hotplug_notify_cb()", user need not change the
>>>> flag for removing container device by "sysfs eject".
>>>
>>> Which is utterly confusing.  There is no reason whatsoever why the sysfs eject
>>> attribute should work differently from the event-triggered eject.  Quite the
>>> opposite is the case: it should work in the same way in my opinion so that it
>>> is possible to test the eject code path using that attribute.
>>>
>>> I'm traveling now, but when I get back home (next week), I'll try to implement
>>> the thing I was talking about above.
>>
>> It took some more time than I had expected, but I finally was able to get to that.
>>
>> The following two patches implement the idea.  This is the minimum (in my opinion)
>> implementation and it may be extended in some ways.
>>
>> Patch [1/2] introduces a new demand_offline flag for struct acpi_hotplug_profile
>> that makes acpi_scan_hot_remove() check the offline status of the device object's
>> companion physical devices to start with and return -EBUSY if at least one of them
>> is not offline.
>>
>> Patch [2/2] uses that flag to implement the container handling.  The details are
>> in the changelog, but that's how it is supposed to work.
>>
>> During the initial namespace scan the container ACPI scan handler should create
>> "physical" system container device under /sys/devices/system/container/ for
>> each ACPI container object (the sysfs name of that device should be the same as
>> the sysfs name of the corresponding container object and they should be linked
>> to each other via the firmware_node and physical_node symbolic links, respectively).
>> Those system container devices are initially online.
>>
>> When a container eject event happens, acpi_scan_hot_remove() will notice that
>> hotplug.demand_offline is set in the device object's scan handler and will
>> check the online status of its "physical" companion device, which is online
>> (that is the system container device the above paragraph is about).  That will
>> cause KOBJ_CHANGE to be emitted for the system container device and -EBUSY to
>> be returned by acpi_scan_hot_remove().
>>
>> Now, user space needs to offline the system container device through its online
>> sysfs attribute (that should be present, because the bus type for containers
>> provides the online and offline callbacks).  However, the offline for system
>> container devices will only succeed if the physical devices right below the
>> container are all offline, so user space will have to offline those devices
>> before attempting to offline the system container device itself.  When
>> finished, user space can trigger the container removal with the help of the
>> eject sysfs attribute of the ACPI container object pointed to by the system
>> container device's firmware_node link (this time the check in
>> acpi_scan_hot_remove() will succeed, because the system container device in
>> question is now offline).
>>
>> The way it is implemented is a bit hackish (the driver_data pointer is slightly
>> abused), but that's a special case and I wanted to avoid adding new fields to
>> struct device just for handling it.
>>
>> The patches haven't been tested yet.  I'm going to do that later today, but
>> first I need to take care of some other things, so that has to wait.
>

Thank you for implementing your idea.

> The series of the two patches:
>
> [1/2] ACPI / hotplug: Add demand_offline hotplug profile flag
> 	https://patchwork.kernel.org/patch/3396711/
>
> [2/2] ACPI / hotplug / driver core: Handle containers in a special way
> 	https://patchwork.kernel.org/patch/3399081/
>
> has been tested now and seems to work as expected, at least for a container
> that has no children (that's one I could simulate easily in a meaningful way).
>
> For this reason, if there are no objections, I'll resend them as an official
> submission during the next couple of days.

I'm testing these patches now. If I have a comment, I send it to these
threads.

Thanks,
Yasuaki Ishimatsu

>
> Thanks!
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Dec. 27, 2013, 12:31 a.m. UTC | #13
On Thursday, December 26, 2013 11:53:10 AM Yasuaki Ishimatsu wrote:
> HI Rafael,
> 
> (2013/12/26 10:01), Rafael J. Wysocki wrote:
> > On Monday, December 23, 2013 02:58:38 PM Rafael J. Wysocki wrote:
> >> On Saturday, December 14, 2013 06:07:06 AM Rafael J. Wysocki wrote:
> >>> On Friday, December 13, 2013 02:17:32 PM Yasuaki Ishimatsu wrote:
> >>>> (2013/12/13 13:56), Rafael J. Wysocki wrote:
> >>>>> On Friday, December 13, 2013 11:56:32 AM Yasuaki Ishimatsu wrote:
> >>>>>> Hi Rafael,
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>>> Please share your more detailed idea. I started to implement the following
> >>>>>> idea. But the idea has one problem.
> >>>>>>
> >>>>>>>>> The eject work flow can be:
> >>>>>>>>>       (1) an eject event occurs,
> >>>>>>>>>       (2) the container "physical" device fails offline in acpi_scan_hot_remove()
> >>>>>>>>>           emmitting, say, KOBJ_CHANGE for the "physical" device,
> >>>>>>>>>       (3) user space notices the KOBJ_CHANGE and does the cleanup as needed,
> >>>>>>>>>       (4) user space changes the "physical" container device flag controlling
> >>>>>>>>>           offline to 0,
> >>>>>>>>>       (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >>>>>>>>>           to finally eject the container,
> >>>>>>>>>       (6) the offline in acpi_scan_hot_remove() is now successful, because the
> >>>>>>>>>           flag controlling it has been set to 0 in step (4),
> >>>>>>>>>       (7) the "physical" container device goes away before executing _EJ0,
> >>>>>>>>>       (8) the container is ejected.
> >>>>>>
> >>>>>> I want to emit KOBJ_CHANGE before offlining devices on container device at (2).
> >>>>>> But acpi_scan_hot_remove() offlines devices on container device at first.
> >>>>>> So when offline container device, devices on container has been offlined.
> >>>>>>
> >>>>>> Thus the idea cannot fulfill my necessary feature.
> >>>>>
> >>>>> Well, in that case we need to treat containers in a special way at the ACPI
> >>>>> level.  Which is a bit unfortunate so to speak.
> >>>>>
> >>>>> To that end I'd try to add a new flag to struct acpi_hotplug_profile, say
> >>>>> .verify_offline, such that if set, it would cause acpi_scan_hot_remove() to
> >>>>> check if all of the "physical" companions of the top-level device are offline
> >>>>> to start with, and if not, it would just emit KOBJ_CHANGE for the companions
> >>>>> that are not offline and bail out.
> >>>>>
> >>>>> So the above algorithm would become:
> >>>>>
> >>>>> (1) an eject event occurs,
> >>>>> (2) acpi_scan_hot_remove() checks the verify_offline flag in the target device's
> >>>>>       scan_handler structure,
> >>>>> (3) if set (it would always be set for containers), acpi_scan_hot_remove()
> >>>>>       checks the status of the target device's "physical" companions; if at least
> >>>>>       one of them is offline, KOBJ_CHANGE is emitted for that "physical" device,
> >>>>>       and acpi_scan_hot_remove() returns, [I guess we can just emit KOBJ_CHANGE
> >>>>>       for the first companion that is not offline at this point.]
> >>>>> (4) user space notices the KOBJ_CHANGE and does the cleanup as needed; in the
> >>>>>       process it carries out the offline operation for the container's "physical"
> >>>>>       companion (there's only one such companion for each container), [That
> >>>>>       operation for the container itself is trivial, but to succeed it requires
> >>>>>       all devices below the container to be taken offline in advance.]
> >>>>> (5) user space uses the sysfs "eject" attribute of the ACPI container object
> >>>>>       to finally eject the container,
> >>>>> (6) acpi_scan_hot_remove() is now successful, because the container's "physical"
> >>>>>       companion is now offline,
> >>>>> (7) the "physical" container device goes away before executing _EJ0,
> >>>>> (8) the container is ejected.
> >>>>>
> >>>>> I think that should work for you.
> >>>>
> >>>> This idea seems to same as your previous work.
> >>>> http://lkml.org/lkml/2013/2/23/97
> >>>
> >>> No, it is not.  That one didn't involve physical device representations.
> >>>
> >>>> How about add autoremove flag into acpi_hotplug_profile and check it as follow:
> >>>
> >>> This is very similar to "enable" except that it generates the uevent and
> >>> "enable" doesn't.  You might as well modify "enable" to trigger a uevent if
> >>> eject is not enabled (note that with the latest patches in linux-next "enable"
> >>> only applies to eject).
> >>>
> >>> That said I don't think we should generate any uevents for struct acpi_device
> >>> objects, because they are not devices.
> >>>
> >>>> ---
> >>>>    drivers/acpi/scan.c | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >>>> index 5383c81..c43d110 100644
> >>>> --- a/drivers/acpi/scan.c
> >>>> +++ b/drivers/acpi/scan.c
> >>>> @@ -409,6 +409,11 @@ static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *data)
> >>>>    			ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> >>>>    			goto err_out;
> >>>>    		}
> >>>> +		if (!handler->hotplug.autoremove) {
> >>>> +			kobject_uevent(&device->dev.kobj, KOBJ_CHANGE);
> >>>> +			ost_code = ACPI_OST_SC_EJECT_NON_SPECIFIC_FAILURE;
> >>>> +			goto err_out;
> >>>> +		}
> >>>>    		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> >>>>    					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> >>>>    		break;
> >>>>
> >>>> Adding the check into "acpi_hotplug_notify_cb()", user need not change the
> >>>> flag for removing container device by "sysfs eject".
> >>>
> >>> Which is utterly confusing.  There is no reason whatsoever why the sysfs eject
> >>> attribute should work differently from the event-triggered eject.  Quite the
> >>> opposite is the case: it should work in the same way in my opinion so that it
> >>> is possible to test the eject code path using that attribute.
> >>>
> >>> I'm traveling now, but when I get back home (next week), I'll try to implement
> >>> the thing I was talking about above.
> >>
> >> It took some more time than I had expected, but I finally was able to get to that.
> >>
> >> The following two patches implement the idea.  This is the minimum (in my opinion)
> >> implementation and it may be extended in some ways.
> >>
> >> Patch [1/2] introduces a new demand_offline flag for struct acpi_hotplug_profile
> >> that makes acpi_scan_hot_remove() check the offline status of the device object's
> >> companion physical devices to start with and return -EBUSY if at least one of them
> >> is not offline.
> >>
> >> Patch [2/2] uses that flag to implement the container handling.  The details are
> >> in the changelog, but that's how it is supposed to work.
> >>
> >> During the initial namespace scan the container ACPI scan handler should create
> >> "physical" system container device under /sys/devices/system/container/ for
> >> each ACPI container object (the sysfs name of that device should be the same as
> >> the sysfs name of the corresponding container object and they should be linked
> >> to each other via the firmware_node and physical_node symbolic links, respectively).
> >> Those system container devices are initially online.
> >>
> >> When a container eject event happens, acpi_scan_hot_remove() will notice that
> >> hotplug.demand_offline is set in the device object's scan handler and will
> >> check the online status of its "physical" companion device, which is online
> >> (that is the system container device the above paragraph is about).  That will
> >> cause KOBJ_CHANGE to be emitted for the system container device and -EBUSY to
> >> be returned by acpi_scan_hot_remove().
> >>
> >> Now, user space needs to offline the system container device through its online
> >> sysfs attribute (that should be present, because the bus type for containers
> >> provides the online and offline callbacks).  However, the offline for system
> >> container devices will only succeed if the physical devices right below the
> >> container are all offline, so user space will have to offline those devices
> >> before attempting to offline the system container device itself.  When
> >> finished, user space can trigger the container removal with the help of the
> >> eject sysfs attribute of the ACPI container object pointed to by the system
> >> container device's firmware_node link (this time the check in
> >> acpi_scan_hot_remove() will succeed, because the system container device in
> >> question is now offline).
> >>
> >> The way it is implemented is a bit hackish (the driver_data pointer is slightly
> >> abused), but that's a special case and I wanted to avoid adding new fields to
> >> struct device just for handling it.
> >>
> >> The patches haven't been tested yet.  I'm going to do that later today, but
> >> first I need to take care of some other things, so that has to wait.
> >
> 
> Thank you for implementing your idea.

You're very welcome! :-)

> > The series of the two patches:
> >
> > [1/2] ACPI / hotplug: Add demand_offline hotplug profile flag
> > 	https://patchwork.kernel.org/patch/3396711/
> >
> > [2/2] ACPI / hotplug / driver core: Handle containers in a special way
> > 	https://patchwork.kernel.org/patch/3399081/
> >
> > has been tested now and seems to work as expected, at least for a container
> > that has no children (that's one I could simulate easily in a meaningful way).
> >
> > For this reason, if there are no objections, I'll resend them as an official
> > submission during the next couple of days.
> 
> I'm testing these patches now. If I have a comment, I send it to these
> threads.

Thanks a lot!

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -44,19 +44,24 @@  static const struct acpi_device_id conta
 	{"", 0},
 };
 
-static int container_device_attach(struct acpi_device *device,
+static int container_device_attach(struct acpi_device *adev,
 				   const struct acpi_device_id *not_used)
 {
-	/* This is necessary for container hotplug to work. */
+	kobject_uevent(&adev->dev.kobj, KOBJ_ONLINE);
 	return 1;
 }
 
+static void container_device_detach(struct acpi_device *adev)
+{
+	kobject_uevent(&adev->dev.kobj, KOBJ_OFFLINE);
+}
+
 static struct acpi_scan_handler container_handler = {
 	.ids = container_device_ids,
 	.attach = container_device_attach,
+	.detach = container_device_detach,
 	.hotplug = {
 		.enabled = true,
-		.mode = AHM_CONTAINER,
 	},
 };
 
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -206,9 +206,6 @@  static int acpi_scan_hot_remove(struct a
 	acpi_status status;
 	unsigned long long sta;
 
-	if (device->handler && device->handler->hotplug.mode == AHM_CONTAINER)
-		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
-
 	/*
 	 * Carry out two passes here and ignore errors in the first pass,
 	 * because if the devices in question are memory blocks and
@@ -288,10 +285,7 @@  static int acpi_scan_device_check(struct
 		dev_warn(&adev->dev, "Namespace scan failure\n");
 		return error;
 	}
-	if (adev->handler) {
-		if (adev->handler->hotplug.mode == AHM_CONTAINER)
-			kobject_uevent(&adev->dev.kobj, KOBJ_ONLINE);
-	} else {
+	if (!adev->handler) {
 		dev_warn(&adev->dev, "Enumeration failure\n");
 		return -ENODEV;
 	}
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -91,16 +91,9 @@  struct acpi_device;
  * -----------------
  */
 
-enum acpi_hotplug_mode {
-	AHM_GENERIC = 0,
-	AHM_CONTAINER,
-	AHM_COUNT
-};
-
 struct acpi_hotplug_profile {
 	struct kobject kobj;
 	bool enabled:1;
-	enum acpi_hotplug_mode mode;
 	int (*scan_dependent)(struct acpi_device *adev);
 };