| Message ID | 1478394.ryVNRuTre2@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Multiple drivers handling hotplug-capable ACPI device nodes install > notify handlers covering the same types of events in a very similar > way. Moreover, those handlers are installed in separate namespace > walks, although that really should be done during namespace scans > carried out by acpi_bus_scan(). This leads to substantial code > duplication, unnecessary overhead and behavior that is hard to > follow. > > For this reason, introduce common code in drivers/acpi/scan.c for > handling hotplug-related notification and carrying out device > insertion and eject operations in a generic fashion, such that it > may be used by all of the relevant drivers in the future. To cover > the existing differences between those drivers introduce struct > acpi_hotplug_profile for representing collections of hotplug > settings associated with different ACPI scan handlers that can be > used by the drivers to make the common code reflect their current > behavior. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > autoexec is unset for the given scan handler. > > This will require the doc in patch [5/7] to be updated which I'm going to do if > everyone is OK with the $subject patch. > > Thanks, > Rafael : > + > +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > +{ > + struct acpi_device *device = NULL; > + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > + int error; > + > + mutex_lock(&acpi_scan_lock); > + > + acpi_bus_get_device(handle, &device); > + if (device) { > + dev_warn(&device->dev, "Attempt to re-insert\n"); > + goto out; > + } > + acpi_evaluate_hotplug_ost(handle, ost_source, > + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > + error = acpi_bus_scan(handle); > + if (error) { > + acpi_handle_warn(handle, "Namespace scan failure\n"); > + goto out; > + } > + error = acpi_bus_get_device(handle, &device); > + if (error) { > + acpi_handle_warn(handle, "Missing device node object\n"); > + goto out; > + } > + ost_code = ACPI_OST_SC_SUCCESS; > + if (device->handler && device->handler->hotplug.uevents) > + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); I confirmed that the uevent crash issue was solved. Thinking further, I wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric since we do not emit KOBJ_OFFLINE when autoeject is set. The definition of ONLINE/OFFLINE event to an ACPI device object seems also bogus since there is no online/offline operation to the ACPI device object itself. Online/offline operation is only possible to actual device, such as system/cpu/cpu% and system/memory/memory%. So, I'd suggest the following changes. - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for ACPI device objects. - Make the !autoeject case as an exception for now, and emit KOBJ_OFFLINE as a way to request off-lining to user. This uevent is tied with the !autoeject case. We can then revisit if this use-case needs to be supported going forward. If so, we may want to consider a different event type. Thanks, -Toshi -- 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
On Tue, 2013-02-26 at 00:39 +0100, Rafael J. Wysocki wrote: > On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > > On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Multiple drivers handling hotplug-capable ACPI device nodes install > > > notify handlers covering the same types of events in a very similar > > > way. Moreover, those handlers are installed in separate namespace > > > walks, although that really should be done during namespace scans > > > carried out by acpi_bus_scan(). This leads to substantial code > > > duplication, unnecessary overhead and behavior that is hard to > > > follow. > > > > > > For this reason, introduce common code in drivers/acpi/scan.c for > > > handling hotplug-related notification and carrying out device > > > insertion and eject operations in a generic fashion, such that it > > > may be used by all of the relevant drivers in the future. To cover > > > the existing differences between those drivers introduce struct > > > acpi_hotplug_profile for representing collections of hotplug > > > settings associated with different ACPI scan handlers that can be > > > used by the drivers to make the common code reflect their current > > > behavior. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > > > > This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > > > autoexec is unset for the given scan handler. > > > > > > This will require the doc in patch [5/7] to be updated which I'm going to do if > > > everyone is OK with the $subject patch. > > > > > > Thanks, > > > Rafael > > : > > > + > > > +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > > > +{ > > > + struct acpi_device *device = NULL; > > > + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > > > + int error; > > > + > > > + mutex_lock(&acpi_scan_lock); > > > + > > > + acpi_bus_get_device(handle, &device); > > > + if (device) { > > > + dev_warn(&device->dev, "Attempt to re-insert\n"); > > > + goto out; > > > + } > > > + acpi_evaluate_hotplug_ost(handle, ost_source, > > > + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > > > + error = acpi_bus_scan(handle); > > > + if (error) { > > > + acpi_handle_warn(handle, "Namespace scan failure\n"); > > > + goto out; > > > + } > > > + error = acpi_bus_get_device(handle, &device); > > > + if (error) { > > > + acpi_handle_warn(handle, "Missing device node object\n"); > > > + goto out; > > > + } > > > + ost_code = ACPI_OST_SC_SUCCESS; > > > + if (device->handler && device->handler->hotplug.uevents) > > > + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); > > > > I confirmed that the uevent crash issue was solved. Thinking further, I > > wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric > > since we do not emit KOBJ_OFFLINE when autoeject is set. > > Well, I put that in there only to be able to make the container driver behave > in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). > > If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with > your suggestion. > > > The definition of ONLINE/OFFLINE event to an ACPI device object seems also > > bogus since there is no online/offline operation to the ACPI device object > > itself. > > Online/offline operation is only possible to actual device, such as > > system/cpu/cpu% and system/memory/memory%. > > That's correct, but I don't know what the user space expectations are > currently. I see. I agree that we should keep backward compatibility with this patchset. > > So, I'd suggest the following changes. > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > ACPI device objects. > > - Make the !autoeject case as an exception for now, and emit > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > tied with the !autoeject case. We can then revisit if this use-case > > needs to be supported going forward. If so, we may want to consider a > > different event type. > > Well, what about avoiding to expose uevents and autoeject for now and > exposing enabled only? Drivers would still be able to set the other flags on > init on init to enforce the backwards-compatible behavior. > > I agree that it would be sufficient to use one additional flag then, to start > with, but its meaning would be something like "keep backwards compatibility > with the old container driver", so perhaps "autoeject" is not a good name. > > What about "user_eject" (that won't be exposed to user space) instead? Where, > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > like the old container driver did"? Good idea! Thanks, -Toshi -- 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
On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Multiple drivers handling hotplug-capable ACPI device nodes install > > notify handlers covering the same types of events in a very similar > > way. Moreover, those handlers are installed in separate namespace > > walks, although that really should be done during namespace scans > > carried out by acpi_bus_scan(). This leads to substantial code > > duplication, unnecessary overhead and behavior that is hard to > > follow. > > > > For this reason, introduce common code in drivers/acpi/scan.c for > > handling hotplug-related notification and carrying out device > > insertion and eject operations in a generic fashion, such that it > > may be used by all of the relevant drivers in the future. To cover > > the existing differences between those drivers introduce struct > > acpi_hotplug_profile for representing collections of hotplug > > settings associated with different ACPI scan handlers that can be > > used by the drivers to make the common code reflect their current > > behavior. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > > autoexec is unset for the given scan handler. > > > > This will require the doc in patch [5/7] to be updated which I'm going to do if > > everyone is OK with the $subject patch. > > > > Thanks, > > Rafael > : > > + > > +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > > +{ > > + struct acpi_device *device = NULL; > > + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > > + int error; > > + > > + mutex_lock(&acpi_scan_lock); > > + > > + acpi_bus_get_device(handle, &device); > > + if (device) { > > + dev_warn(&device->dev, "Attempt to re-insert\n"); > > + goto out; > > + } > > + acpi_evaluate_hotplug_ost(handle, ost_source, > > + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > > + error = acpi_bus_scan(handle); > > + if (error) { > > + acpi_handle_warn(handle, "Namespace scan failure\n"); > > + goto out; > > + } > > + error = acpi_bus_get_device(handle, &device); > > + if (error) { > > + acpi_handle_warn(handle, "Missing device node object\n"); > > + goto out; > > + } > > + ost_code = ACPI_OST_SC_SUCCESS; > > + if (device->handler && device->handler->hotplug.uevents) > > + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); > > I confirmed that the uevent crash issue was solved. Thinking further, I > wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric > since we do not emit KOBJ_OFFLINE when autoeject is set. Well, I put that in there only to be able to make the container driver behave in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with your suggestion. > The definition of ONLINE/OFFLINE event to an ACPI device object seems also > bogus since there is no online/offline operation to the ACPI device object > itself. > Online/offline operation is only possible to actual device, such as > system/cpu/cpu% and system/memory/memory%. That's correct, but I don't know what the user space expectations are currently. > So, I'd suggest the following changes. > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > ACPI device objects. > - Make the !autoeject case as an exception for now, and emit > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > tied with the !autoeject case. We can then revisit if this use-case > needs to be supported going forward. If so, we may want to consider a > different event type. Well, what about avoiding to expose uevents and autoeject for now and exposing enabled only? Drivers would still be able to set the other flags on init on init to enforce the backwards-compatible behavior. I agree that it would be sufficient to use one additional flag then, to start with, but its meaning would be something like "keep backwards compatibility with the old container driver", so perhaps "autoeject" is not a good name. What about "user_eject" (that won't be exposed to user space) instead? Where, if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents like the old container driver did"? Rafael
2013/02/26 8:39, Rafael J. Wysocki wrote: > On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: >> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Multiple drivers handling hotplug-capable ACPI device nodes install >>> notify handlers covering the same types of events in a very similar >>> way. Moreover, those handlers are installed in separate namespace >>> walks, although that really should be done during namespace scans >>> carried out by acpi_bus_scan(). This leads to substantial code >>> duplication, unnecessary overhead and behavior that is hard to >>> follow. >>> >>> For this reason, introduce common code in drivers/acpi/scan.c for >>> handling hotplug-related notification and carrying out device >>> insertion and eject operations in a generic fashion, such that it >>> may be used by all of the relevant drivers in the future. To cover >>> the existing differences between those drivers introduce struct >>> acpi_hotplug_profile for representing collections of hotplug >>> settings associated with different ACPI scan handlers that can be >>> used by the drivers to make the common code reflect their current >>> behavior. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> >>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if >>> autoexec is unset for the given scan handler. >>> >>> This will require the doc in patch [5/7] to be updated which I'm going to do if >>> everyone is OK with the $subject patch. >>> >>> Thanks, >>> Rafael >> : >>> + >>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) >>> +{ >>> + struct acpi_device *device = NULL; >>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; >>> + int error; >>> + >>> + mutex_lock(&acpi_scan_lock); >>> + >>> + acpi_bus_get_device(handle, &device); >>> + if (device) { >>> + dev_warn(&device->dev, "Attempt to re-insert\n"); >>> + goto out; >>> + } >>> + acpi_evaluate_hotplug_ost(handle, ost_source, >>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); >>> + error = acpi_bus_scan(handle); >>> + if (error) { >>> + acpi_handle_warn(handle, "Namespace scan failure\n"); >>> + goto out; >>> + } >>> + error = acpi_bus_get_device(handle, &device); >>> + if (error) { >>> + acpi_handle_warn(handle, "Missing device node object\n"); >>> + goto out; >>> + } >>> + ost_code = ACPI_OST_SC_SUCCESS; >>> + if (device->handler && device->handler->hotplug.uevents) >>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); >> >> I confirmed that the uevent crash issue was solved. Thinking further, I >> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric >> since we do not emit KOBJ_OFFLINE when autoeject is set. > > Well, I put that in there only to be able to make the container driver behave > in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). > > If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with > your suggestion. > >> The definition of ONLINE/OFFLINE event to an ACPI device object seems also >> bogus since there is no online/offline operation to the ACPI device object >> itself. >> Online/offline operation is only possible to actual device, such as >> system/cpu/cpu% and system/memory/memory%. > > That's correct, but I don't know what the user space expectations are > currently. My system expects this event to be notified when hot adding container device. My container device has cpu and memory. As Toshi said, these devices are offline when hot adding container device. So in my system, when notifying container device's KOBJ_ONLINE event, my application runs for onlining these devices. If this event is not notified to user land, we cannot online these devices automatically. > >> So, I'd suggest the following changes. >> - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for >> ACPI device objects. >> - Make the !autoeject case as an exception for now, and emit >> KOBJ_OFFLINE as a way to request off-lining to user. This uevent is >> tied with the !autoeject case. We can then revisit if this use-case >> needs to be supported going forward. If so, we may want to consider a >> different event type. > > Well, what about avoiding to expose uevents and autoeject for now and > exposing enabled only? Drivers would still be able to set the other flags on > init on init to enforce the backwards-compatible behavior. > > I agree that it would be sufficient to use one additional flag then, to start > with, but its meaning would be something like "keep backwards compatibility > with the old container driver", so perhaps "autoeject" is not a good name. > > What about "user_eject" (that won't be exposed to user space) instead? Where, > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > like the old container driver did"? It's good idea. 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
On Tue, 2013-02-26 at 09:40 +0900, Yasuaki Ishimatsu wrote: > 2013/02/26 8:39, Rafael J. Wysocki wrote: > > On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > >> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> > >>> Multiple drivers handling hotplug-capable ACPI device nodes install > >>> notify handlers covering the same types of events in a very similar > >>> way. Moreover, those handlers are installed in separate namespace > >>> walks, although that really should be done during namespace scans > >>> carried out by acpi_bus_scan(). This leads to substantial code > >>> duplication, unnecessary overhead and behavior that is hard to > >>> follow. > >>> > >>> For this reason, introduce common code in drivers/acpi/scan.c for > >>> handling hotplug-related notification and carrying out device > >>> insertion and eject operations in a generic fashion, such that it > >>> may be used by all of the relevant drivers in the future. To cover > >>> the existing differences between those drivers introduce struct > >>> acpi_hotplug_profile for representing collections of hotplug > >>> settings associated with different ACPI scan handlers that can be > >>> used by the drivers to make the common code reflect their current > >>> behavior. > >>> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> --- > >>> > >>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > >>> autoexec is unset for the given scan handler. > >>> > >>> This will require the doc in patch [5/7] to be updated which I'm going to do if > >>> everyone is OK with the $subject patch. > >>> > >>> Thanks, > >>> Rafael > >> : > >>> + > >>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > >>> +{ > >>> + struct acpi_device *device = NULL; > >>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > >>> + int error; > >>> + > >>> + mutex_lock(&acpi_scan_lock); > >>> + > >>> + acpi_bus_get_device(handle, &device); > >>> + if (device) { > >>> + dev_warn(&device->dev, "Attempt to re-insert\n"); > >>> + goto out; > >>> + } > >>> + acpi_evaluate_hotplug_ost(handle, ost_source, > >>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > >>> + error = acpi_bus_scan(handle); > >>> + if (error) { > >>> + acpi_handle_warn(handle, "Namespace scan failure\n"); > >>> + goto out; > >>> + } > >>> + error = acpi_bus_get_device(handle, &device); > >>> + if (error) { > >>> + acpi_handle_warn(handle, "Missing device node object\n"); > >>> + goto out; > >>> + } > >>> + ost_code = ACPI_OST_SC_SUCCESS; > >>> + if (device->handler && device->handler->hotplug.uevents) > >>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); > >> > > >> I confirmed that the uevent crash issue was solved. Thinking further, I > >> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric > >> since we do not emit KOBJ_OFFLINE when autoeject is set. > > > > Well, I put that in there only to be able to make the container driver behave > > in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). > > > > If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with > > your suggestion. > > > >> The definition of ONLINE/OFFLINE event to an ACPI device object seems also > >> bogus since there is no online/offline operation to the ACPI device object > >> itself. > >> Online/offline operation is only possible to actual device, such as > >> system/cpu/cpu% and system/memory/memory%. > > > > That's correct, but I don't know what the user space expectations are > > currently. > > My system expects this event to be notified when hot adding container device. > My container device has cpu and memory. As Toshi said, these devices are > offline when hot adding container device. So in my system, when notifying > container device's KOBJ_ONLINE event, my application runs for onlining these > devices. If this event is not notified to user land, we cannot online these > devices automatically. Thanks for the info. Can your application listen KOBJ_ADD to a container device, instead of KOBJ_ONLINE? IOW, does it distinguish between ADD and ONLINE events to a container device? -Toshi -- 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
2013/02/26 10:09, Toshi Kani wrote: > On Tue, 2013-02-26 at 09:40 +0900, Yasuaki Ishimatsu wrote: >> 2013/02/26 8:39, Rafael J. Wysocki wrote: >>> On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: >>>> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>> >>>>> Multiple drivers handling hotplug-capable ACPI device nodes install >>>>> notify handlers covering the same types of events in a very similar >>>>> way. Moreover, those handlers are installed in separate namespace >>>>> walks, although that really should be done during namespace scans >>>>> carried out by acpi_bus_scan(). This leads to substantial code >>>>> duplication, unnecessary overhead and behavior that is hard to >>>>> follow. >>>>> >>>>> For this reason, introduce common code in drivers/acpi/scan.c for >>>>> handling hotplug-related notification and carrying out device >>>>> insertion and eject operations in a generic fashion, such that it >>>>> may be used by all of the relevant drivers in the future. To cover >>>>> the existing differences between those drivers introduce struct >>>>> acpi_hotplug_profile for representing collections of hotplug >>>>> settings associated with different ACPI scan handlers that can be >>>>> used by the drivers to make the common code reflect their current >>>>> behavior. >>>>> >>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>> --- >>>>> >>>>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if >>>>> autoexec is unset for the given scan handler. >>>>> >>>>> This will require the doc in patch [5/7] to be updated which I'm going to do if >>>>> everyone is OK with the $subject patch. >>>>> >>>>> Thanks, >>>>> Rafael >>>> : >>>>> + >>>>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) >>>>> +{ >>>>> + struct acpi_device *device = NULL; >>>>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; >>>>> + int error; >>>>> + >>>>> + mutex_lock(&acpi_scan_lock); >>>>> + >>>>> + acpi_bus_get_device(handle, &device); >>>>> + if (device) { >>>>> + dev_warn(&device->dev, "Attempt to re-insert\n"); >>>>> + goto out; >>>>> + } >>>>> + acpi_evaluate_hotplug_ost(handle, ost_source, >>>>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); >>>>> + error = acpi_bus_scan(handle); >>>>> + if (error) { >>>>> + acpi_handle_warn(handle, "Namespace scan failure\n"); >>>>> + goto out; >>>>> + } >>>>> + error = acpi_bus_get_device(handle, &device); >>>>> + if (error) { >>>>> + acpi_handle_warn(handle, "Missing device node object\n"); >>>>> + goto out; >>>>> + } >>>>> + ost_code = ACPI_OST_SC_SUCCESS; >>>>> + if (device->handler && device->handler->hotplug.uevents) >>>>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); >>>> >> >>>> I confirmed that the uevent crash issue was solved. Thinking further, I >>>> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric >>>> since we do not emit KOBJ_OFFLINE when autoeject is set. >>> >>> Well, I put that in there only to be able to make the container driver behave >>> in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). >>> >>> If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with >>> your suggestion. >>> >>>> The definition of ONLINE/OFFLINE event to an ACPI device object seems also >>>> bogus since there is no online/offline operation to the ACPI device object >>>> itself. >>>> Online/offline operation is only possible to actual device, such as >>>> system/cpu/cpu% and system/memory/memory%. >>> >>> That's correct, but I don't know what the user space expectations are >>> currently. >> >> My system expects this event to be notified when hot adding container device. >> My container device has cpu and memory. As Toshi said, these devices are >> offline when hot adding container device. So in my system, when notifying >> container device's KOBJ_ONLINE event, my application runs for onlining these >> devices. If this event is not notified to user land, we cannot online these >> devices automatically. > > Thanks for the info. Can your application listen KOBJ_ADD to a > container device, instead of KOBJ_ONLINE? IOW, does it distinguish > between ADD and ONLINE events to a container device? My application does not distinguish between ADD and ONLINE events currently. But if the event is changed from ONLINE to ADD, I will change my application. Thanks, Yasuaki Ishimatsu > > -Toshi > > -- 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
On Tuesday, February 26, 2013 11:02:56 AM Yasuaki Ishimatsu wrote: > 2013/02/26 10:09, Toshi Kani wrote: > > On Tue, 2013-02-26 at 09:40 +0900, Yasuaki Ishimatsu wrote: > >> 2013/02/26 8:39, Rafael J. Wysocki wrote: > >>> On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > >>>> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>>> > >>>>> Multiple drivers handling hotplug-capable ACPI device nodes install > >>>>> notify handlers covering the same types of events in a very similar > >>>>> way. Moreover, those handlers are installed in separate namespace > >>>>> walks, although that really should be done during namespace scans > >>>>> carried out by acpi_bus_scan(). This leads to substantial code > >>>>> duplication, unnecessary overhead and behavior that is hard to > >>>>> follow. > >>>>> > >>>>> For this reason, introduce common code in drivers/acpi/scan.c for > >>>>> handling hotplug-related notification and carrying out device > >>>>> insertion and eject operations in a generic fashion, such that it > >>>>> may be used by all of the relevant drivers in the future. To cover > >>>>> the existing differences between those drivers introduce struct > >>>>> acpi_hotplug_profile for representing collections of hotplug > >>>>> settings associated with different ACPI scan handlers that can be > >>>>> used by the drivers to make the common code reflect their current > >>>>> behavior. > >>>>> > >>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>>> --- > >>>>> > >>>>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > >>>>> autoexec is unset for the given scan handler. > >>>>> > >>>>> This will require the doc in patch [5/7] to be updated which I'm going to do if > >>>>> everyone is OK with the $subject patch. > >>>>> > >>>>> Thanks, > >>>>> Rafael > >>>> : > >>>>> + > >>>>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > >>>>> +{ > >>>>> + struct acpi_device *device = NULL; > >>>>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > >>>>> + int error; > >>>>> + > >>>>> + mutex_lock(&acpi_scan_lock); > >>>>> + > >>>>> + acpi_bus_get_device(handle, &device); > >>>>> + if (device) { > >>>>> + dev_warn(&device->dev, "Attempt to re-insert\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + acpi_evaluate_hotplug_ost(handle, ost_source, > >>>>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > >>>>> + error = acpi_bus_scan(handle); > >>>>> + if (error) { > >>>>> + acpi_handle_warn(handle, "Namespace scan failure\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + error = acpi_bus_get_device(handle, &device); > >>>>> + if (error) { > >>>>> + acpi_handle_warn(handle, "Missing device node object\n"); > >>>>> + goto out; > >>>>> + } > >>>>> + ost_code = ACPI_OST_SC_SUCCESS; > >>>>> + if (device->handler && device->handler->hotplug.uevents) > >>>>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); > >>>> > >> > >>>> I confirmed that the uevent crash issue was solved. Thinking further, I > >>>> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric > >>>> since we do not emit KOBJ_OFFLINE when autoeject is set. > >>> > >>> Well, I put that in there only to be able to make the container driver behave > >>> in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). > >>> > >>> If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with > >>> your suggestion. > >>> > >>>> The definition of ONLINE/OFFLINE event to an ACPI device object seems also > >>>> bogus since there is no online/offline operation to the ACPI device object > >>>> itself. > >>>> Online/offline operation is only possible to actual device, such as > >>>> system/cpu/cpu% and system/memory/memory%. > >>> > >>> That's correct, but I don't know what the user space expectations are > >>> currently. > >> > >> My system expects this event to be notified when hot adding container device. > >> My container device has cpu and memory. As Toshi said, these devices are > >> offline when hot adding container device. So in my system, when notifying > >> container device's KOBJ_ONLINE event, my application runs for onlining these > >> devices. If this event is not notified to user land, we cannot online these > >> devices automatically. > > > > > Thanks for the info. Can your application listen KOBJ_ADD to a > > container device, instead of KOBJ_ONLINE? IOW, does it distinguish > > between ADD and ONLINE events to a container device? > > My application does not distinguish between ADD and ONLINE events > currently. But if the event is changed from ONLINE to ADD, I will > change my application. KOBJ_ADD is emitted for every struct acpi_device being registered, including container devices, by acpi_device_add_finalize(). Can your application listen to those events? Rafael
2013/02/26 12:11, Rafael J. Wysocki wrote: > On Tuesday, February 26, 2013 11:02:56 AM Yasuaki Ishimatsu wrote: >> 2013/02/26 10:09, Toshi Kani wrote: >>> On Tue, 2013-02-26 at 09:40 +0900, Yasuaki Ishimatsu wrote: >>>> 2013/02/26 8:39, Rafael J. Wysocki wrote: >>>>> On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: >>>>>> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: >>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>>> >>>>>>> Multiple drivers handling hotplug-capable ACPI device nodes install >>>>>>> notify handlers covering the same types of events in a very similar >>>>>>> way. Moreover, those handlers are installed in separate namespace >>>>>>> walks, although that really should be done during namespace scans >>>>>>> carried out by acpi_bus_scan(). This leads to substantial code >>>>>>> duplication, unnecessary overhead and behavior that is hard to >>>>>>> follow. >>>>>>> >>>>>>> For this reason, introduce common code in drivers/acpi/scan.c for >>>>>>> handling hotplug-related notification and carrying out device >>>>>>> insertion and eject operations in a generic fashion, such that it >>>>>>> may be used by all of the relevant drivers in the future. To cover >>>>>>> the existing differences between those drivers introduce struct >>>>>>> acpi_hotplug_profile for representing collections of hotplug >>>>>>> settings associated with different ACPI scan handlers that can be >>>>>>> used by the drivers to make the common code reflect their current >>>>>>> behavior. >>>>>>> >>>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>>> --- >>>>>>> >>>>>>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if >>>>>>> autoexec is unset for the given scan handler. >>>>>>> >>>>>>> This will require the doc in patch [5/7] to be updated which I'm going to do if >>>>>>> everyone is OK with the $subject patch. >>>>>>> >>>>>>> Thanks, >>>>>>> Rafael >>>>>> : >>>>>>> + >>>>>>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) >>>>>>> +{ >>>>>>> + struct acpi_device *device = NULL; >>>>>>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; >>>>>>> + int error; >>>>>>> + >>>>>>> + mutex_lock(&acpi_scan_lock); >>>>>>> + >>>>>>> + acpi_bus_get_device(handle, &device); >>>>>>> + if (device) { >>>>>>> + dev_warn(&device->dev, "Attempt to re-insert\n"); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + acpi_evaluate_hotplug_ost(handle, ost_source, >>>>>>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); >>>>>>> + error = acpi_bus_scan(handle); >>>>>>> + if (error) { >>>>>>> + acpi_handle_warn(handle, "Namespace scan failure\n"); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + error = acpi_bus_get_device(handle, &device); >>>>>>> + if (error) { >>>>>>> + acpi_handle_warn(handle, "Missing device node object\n"); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + ost_code = ACPI_OST_SC_SUCCESS; >>>>>>> + if (device->handler && device->handler->hotplug.uevents) >>>>>>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); >>>>>> >>>> >>>>>> I confirmed that the uevent crash issue was solved. Thinking further, I >>>>>> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric >>>>>> since we do not emit KOBJ_OFFLINE when autoeject is set. >>>>> >>>>> Well, I put that in there only to be able to make the container driver behave >>>>> in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). >>>>> >>>>> If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with >>>>> your suggestion. >>>>> >>>>>> The definition of ONLINE/OFFLINE event to an ACPI device object seems also >>>>>> bogus since there is no online/offline operation to the ACPI device object >>>>>> itself. >>>>>> Online/offline operation is only possible to actual device, such as >>>>>> system/cpu/cpu% and system/memory/memory%. >>>>> >>>>> That's correct, but I don't know what the user space expectations are >>>>> currently. >>>> >>>> My system expects this event to be notified when hot adding container device. >>>> My container device has cpu and memory. As Toshi said, these devices are >>>> offline when hot adding container device. So in my system, when notifying >>>> container device's KOBJ_ONLINE event, my application runs for onlining these >>>> devices. If this event is not notified to user land, we cannot online these >>>> devices automatically. >>> >> >>> Thanks for the info. Can your application listen KOBJ_ADD to a >>> container device, instead of KOBJ_ONLINE? IOW, does it distinguish >>> between ADD and ONLINE events to a container device? >> >> My application does not distinguish between ADD and ONLINE events >> currently. But if the event is changed from ONLINE to ADD, I will >> change my application. > > KOBJ_ADD is emitted for every struct acpi_device being registered, including > container devices, by acpi_device_add_finalize(). Can your application listen > to those events? Ah O.K. I understood. Even if the ONLINE event disappers, my application can get ADD event from acpi_device_add_finalize(). Of course, I need to change my application. If the ADD event of container device is notified after hot adding devices which are contained into container device, there is no reason to leave ONLINE event. 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
On Tuesday, February 26, 2013 12:40:22 PM Yasuaki Ishimatsu wrote: > 2013/02/26 12:11, Rafael J. Wysocki wrote: > > On Tuesday, February 26, 2013 11:02:56 AM Yasuaki Ishimatsu wrote: > >> 2013/02/26 10:09, Toshi Kani wrote: > >>> On Tue, 2013-02-26 at 09:40 +0900, Yasuaki Ishimatsu wrote: > >>>> 2013/02/26 8:39, Rafael J. Wysocki wrote: > >>>>> On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > >>>>>> On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > >>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>>>>> > >>>>>>> Multiple drivers handling hotplug-capable ACPI device nodes install > >>>>>>> notify handlers covering the same types of events in a very similar > >>>>>>> way. Moreover, those handlers are installed in separate namespace > >>>>>>> walks, although that really should be done during namespace scans > >>>>>>> carried out by acpi_bus_scan(). This leads to substantial code > >>>>>>> duplication, unnecessary overhead and behavior that is hard to > >>>>>>> follow. > >>>>>>> > >>>>>>> For this reason, introduce common code in drivers/acpi/scan.c for > >>>>>>> handling hotplug-related notification and carrying out device > >>>>>>> insertion and eject operations in a generic fashion, such that it > >>>>>>> may be used by all of the relevant drivers in the future. To cover > >>>>>>> the existing differences between those drivers introduce struct > >>>>>>> acpi_hotplug_profile for representing collections of hotplug > >>>>>>> settings associated with different ACPI scan handlers that can be > >>>>>>> used by the drivers to make the common code reflect their current > >>>>>>> behavior. > >>>>>>> > >>>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>>>>> --- > >>>>>>> > >>>>>>> This update causes acpi_bus_device_eject() to only emit KOBJ_OFFLINE uevent if > >>>>>>> autoexec is unset for the given scan handler. > >>>>>>> > >>>>>>> This will require the doc in patch [5/7] to be updated which I'm going to do if > >>>>>>> everyone is OK with the $subject patch. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Rafael > >>>>>> : > >>>>>>> + > >>>>>>> +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) > >>>>>>> +{ > >>>>>>> + struct acpi_device *device = NULL; > >>>>>>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; > >>>>>>> + int error; > >>>>>>> + > >>>>>>> + mutex_lock(&acpi_scan_lock); > >>>>>>> + > >>>>>>> + acpi_bus_get_device(handle, &device); > >>>>>>> + if (device) { > >>>>>>> + dev_warn(&device->dev, "Attempt to re-insert\n"); > >>>>>>> + goto out; > >>>>>>> + } > >>>>>>> + acpi_evaluate_hotplug_ost(handle, ost_source, > >>>>>>> + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); > >>>>>>> + error = acpi_bus_scan(handle); > >>>>>>> + if (error) { > >>>>>>> + acpi_handle_warn(handle, "Namespace scan failure\n"); > >>>>>>> + goto out; > >>>>>>> + } > >>>>>>> + error = acpi_bus_get_device(handle, &device); > >>>>>>> + if (error) { > >>>>>>> + acpi_handle_warn(handle, "Missing device node object\n"); > >>>>>>> + goto out; > >>>>>>> + } > >>>>>>> + ost_code = ACPI_OST_SC_SUCCESS; > >>>>>>> + if (device->handler && device->handler->hotplug.uevents) > >>>>>>> + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); > >>>>>> > >>>> > >>>>>> I confirmed that the uevent crash issue was solved. Thinking further, I > >>>>>> wonder if we need to emit KOBJ_ONLINE here. This behavior is asymmetric > >>>>>> since we do not emit KOBJ_OFFLINE when autoeject is set. > >>>>> > >>>>> Well, I put that in there only to be able to make the container driver behave > >>>>> in a backwards compatible way (which is to emit KOBJ_ONLINE at this point). > >>>>> > >>>>> If the container driver doesn't need to emit KOBJ_ONLINE at all, I agree with > >>>>> your suggestion. > >>>>> > >>>>>> The definition of ONLINE/OFFLINE event to an ACPI device object seems also > >>>>>> bogus since there is no online/offline operation to the ACPI device object > >>>>>> itself. > >>>>>> Online/offline operation is only possible to actual device, such as > >>>>>> system/cpu/cpu% and system/memory/memory%. > >>>>> > >>>>> That's correct, but I don't know what the user space expectations are > >>>>> currently. > >>>> > >>>> My system expects this event to be notified when hot adding container device. > >>>> My container device has cpu and memory. As Toshi said, these devices are > >>>> offline when hot adding container device. So in my system, when notifying > >>>> container device's KOBJ_ONLINE event, my application runs for onlining these > >>>> devices. If this event is not notified to user land, we cannot online these > >>>> devices automatically. > >>> > >> > >>> Thanks for the info. Can your application listen KOBJ_ADD to a > >>> container device, instead of KOBJ_ONLINE? IOW, does it distinguish > >>> between ADD and ONLINE events to a container device? > >> > >> My application does not distinguish between ADD and ONLINE events > >> currently. But if the event is changed from ONLINE to ADD, I will > >> change my application. > > > > KOBJ_ADD is emitted for every struct acpi_device being registered, including > > container devices, by acpi_device_add_finalize(). Can your application listen > > to those events? > > Ah O.K. I understood. Even if the ONLINE event disappers, my application can get > ADD event from acpi_device_add_finalize(). Of course, I need to change my > application. > If the ADD event of container device is notified after hot adding devices which > are contained into container device, there is no reason to leave ONLINE event. If they are located below the container device in the namespace, then the container's ADD will happen before they are added, so if the ONLINE event is supposed to mean that all devices under the container have been added, we need to keep it as is. Thanks, Rafael
Hi, On Tue, Feb 26, 2013 at 12:39:50AM +0100, Rafael J. Wysocki wrote: > On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > > On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Multiple drivers handling hotplug-capable ACPI device nodes install > > > notify handlers covering the same types of events in a very similar > > > way. Moreover, those handlers are installed in separate namespace > > > walks, although that really should be done during namespace scans > > > carried out by acpi_bus_scan(). This leads to substantial code > > > duplication, unnecessary overhead and behavior that is hard to > > > follow. > > > > > > For this reason, introduce common code in drivers/acpi/scan.c for > > > handling hotplug-related notification and carrying out device > > > insertion and eject operations in a generic fashion, such that it > > > may be used by all of the relevant drivers in the future. To cover > > > the existing differences between those drivers introduce struct > > > acpi_hotplug_profile for representing collections of hotplug > > > settings associated with different ACPI scan handlers that can be > > > used by the drivers to make the common code reflect their current > > > behavior. [...] > That's correct, but I don't know what the user space expectations are > currently. > > > So, I'd suggest the following changes. > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > ACPI device objects. > > - Make the !autoeject case as an exception for now, and emit > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > tied with the !autoeject case. We can then revisit if this use-case > > needs to be supported going forward. If so, we may want to consider a > > different event type. > > Well, what about avoiding to expose uevents and autoeject for now and > exposing enabled only? Drivers would still be able to set the other flags on > init on init to enforce the backwards-compatible behavior. Now that we don't define uevents and autoeject in v2 of this series, could you explain how we get safe ejection from userspace e.g. for memory hot-remove? What are the other flags drivers can use (on init?) to avoid autoeject and only issue KOBJ_OFFLINE? > > I agree that it would be sufficient to use one additional flag then, to start > with, but its meaning would be something like "keep backwards compatibility > with the old container driver", so perhaps "autoeject" is not a good name. > > What about "user_eject" (that won't be exposed to user space) instead? Where, > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > like the old container driver did"? I don't see user_eject in v2. Is it unnecessary for userspace ejection control or planned for later? Also why shouldn't it be exposed to userpace? thanks, - Vasilis -- 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
Sorry for the sluggish response, I've been travelling recently. -> On Monday, March 04, 2013 02:10:23 PM Vasilis Liaskovitis wrote: > Hi, > > On Tue, Feb 26, 2013 at 12:39:50AM +0100, Rafael J. Wysocki wrote: > > On Monday, February 25, 2013 11:07:52 AM Toshi Kani wrote: > > > On Sat, 2013-02-23 at 22:38 +0000, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Multiple drivers handling hotplug-capable ACPI device nodes install > > > > notify handlers covering the same types of events in a very similar > > > > way. Moreover, those handlers are installed in separate namespace > > > > walks, although that really should be done during namespace scans > > > > carried out by acpi_bus_scan(). This leads to substantial code > > > > duplication, unnecessary overhead and behavior that is hard to > > > > follow. > > > > > > > > For this reason, introduce common code in drivers/acpi/scan.c for > > > > handling hotplug-related notification and carrying out device > > > > insertion and eject operations in a generic fashion, such that it > > > > may be used by all of the relevant drivers in the future. To cover > > > > the existing differences between those drivers introduce struct > > > > acpi_hotplug_profile for representing collections of hotplug > > > > settings associated with different ACPI scan handlers that can be > > > > used by the drivers to make the common code reflect their current > > > > behavior. > [...] > > That's correct, but I don't know what the user space expectations are > > currently. > > > > > So, I'd suggest the following changes. > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > ACPI device objects. > > > - Make the !autoeject case as an exception for now, and emit > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > tied with the !autoeject case. We can then revisit if this use-case > > > needs to be supported going forward. If so, we may want to consider a > > > different event type. > > > > Well, what about avoiding to expose uevents and autoeject for now and > > exposing enabled only? Drivers would still be able to set the other flags on > > init on init to enforce the backwards-compatible behavior. > > Now that we don't define uevents and autoeject in v2 of this series, could you > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > are the other flags drivers can use (on init?) to avoid autoeject and only issue > KOBJ_OFFLINE? > > > > > I agree that it would be sufficient to use one additional flag then, to start > > with, but its meaning would be something like "keep backwards compatibility > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > like the old container driver did"? > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > or planned for later? Also why shouldn't it be exposed to userpace? -> At this point we are not sure if it is necessary to have an attribute for direct ejection control. Since the plan is to have a separate offline/online attribute anyway (and a check preventing us from ejecting things that haven't been put offline), it is not clear how useful it is going to be to control ejection directly from user space. Thanks, Rafael
Hi, On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > Sorry for the sluggish response, I've been travelling recently. -> [...] > > > > So, I'd suggest the following changes. > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > ACPI device objects. > > > > - Make the !autoeject case as an exception for now, and emit > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > needs to be supported going forward. If so, we may want to consider a > > > > different event type. > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > exposing enabled only? Drivers would still be able to set the other flags on > > > init on init to enforce the backwards-compatible behavior. > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > KOBJ_OFFLINE? > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > with, but its meaning would be something like "keep backwards compatibility > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > like the old container driver did"? > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > or planned for later? Also why shouldn't it be exposed to userpace? > > -> At this point we are not sure if it is necessary to have an attribute for > direct ejection control. Since the plan is to have a separate offline/online > attribute anyway (and a check preventing us from ejecting things that haven't > been put offline), it is not clear how useful it is going to be to control > ejection directly from user space. ok. Regarding the offline/online attribute and ejection prevention checking, do you mean the offline/online framework from Toshi: http://thread.gmane.org/gmane.linux.kernel/1420262 or something else? I assume this is the long-term plan. Is there any other short-term solution planned? If i understand correctly, until this framework is accepted, memory hot-remove is broken (=unsafe). thanks, - Vasilis -- 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
On Fri, 2013-03-15 at 11:47 +0100, Vasilis Liaskovitis wrote: > Hi, > > On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > > Sorry for the sluggish response, I've been travelling recently. -> > [...] > > > > > So, I'd suggest the following changes. > > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > > ACPI device objects. > > > > > - Make the !autoeject case as an exception for now, and emit > > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > > needs to be supported going forward. If so, we may want to consider a > > > > > different event type. > > > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > > exposing enabled only? Drivers would still be able to set the other flags on > > > > init on init to enforce the backwards-compatible behavior. > > > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > > KOBJ_OFFLINE? > > > > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > > with, but its meaning would be something like "keep backwards compatibility > > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > > like the old container driver did"? > > > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > > or planned for later? Also why shouldn't it be exposed to userpace? > > > > -> At this point we are not sure if it is necessary to have an attribute for > > direct ejection control. Since the plan is to have a separate offline/online > > attribute anyway (and a check preventing us from ejecting things that haven't > > been put offline), it is not clear how useful it is going to be to control > > ejection directly from user space. > > ok. > Regarding the offline/online attribute and ejection prevention checking, do you > mean the offline/online framework from Toshi: > http://thread.gmane.org/gmane.linux.kernel/1420262 > or something else? I assume this is the long-term plan. Unfortunately, the idea of adding a new set of common hotplug framework was not well-received. Since the driver-core does not allow any eject failure case, integrating into the driver-core framework seems also impractical. > Is there any other short-term solution planned? If i understand correctly, until > this framework is accepted, memory hot-remove is broken (=unsafe). That is correct. The alternative plan is to go with an ACPI-specific approach that user has to off-line a target device and its children beforehand from sysfs before initiating a hot-delete request. This hot-delete request will fail if any of the devices are still on-line. The sysfs online/offline interfaces may fail, and user (or user tool) has to take care of the rollback as necessary. It would move all the error handling & rollback stuff into the user space, and make the kernel part very simple & straightforward -- just delete target device objects. After looking further, however, I think this isn't the case... In case of memory hot-delete, for example, off-lining is only a part of the job done in remove_memory(). So, ACPI-core still needs to call device-specific handlers to perform device-specific hot-delete operations, such as calling remove_memory() or its sub-set function, which can fail when a device is online. In order to make sure all devices stay off-line, we need to delete their sysfs interfaces. Since we do not have a way to serialize all online/offline & hot-plug operations (the above patchset had such serialization, but did not get thru), we cannot change all devices at once but delete sysfs interface for each device one by one. If it failed on one of the devices, we need to rollback to put them back into the original state. Other implication is that this approach is not backward compatible. Given this, I am inclined to other alternative -- rework on my patchset and make it as ACPI device hotplug framework. All changes are within ACPI. This actually requires less work and provides backward compatibility as well. I have finished prototyping and is working well. I will send it out for review after all testing is done. Thanks, -Toshi -- 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
On Monday, March 25, 2013 02:45:36 PM Toshi Kani wrote: > On Fri, 2013-03-15 at 11:47 +0100, Vasilis Liaskovitis wrote: > > Hi, > > > > On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > > > Sorry for the sluggish response, I've been travelling recently. -> > > [...] > > > > > > So, I'd suggest the following changes. > > > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > > > ACPI device objects. > > > > > > - Make the !autoeject case as an exception for now, and emit > > > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > > > needs to be supported going forward. If so, we may want to consider a > > > > > > different event type. > > > > > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > > > exposing enabled only? Drivers would still be able to set the other flags on > > > > > init on init to enforce the backwards-compatible behavior. > > > > > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > > > KOBJ_OFFLINE? > > > > > > > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > > > with, but its meaning would be something like "keep backwards compatibility > > > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > > > like the old container driver did"? > > > > > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > > > or planned for later? Also why shouldn't it be exposed to userpace? > > > > > > -> At this point we are not sure if it is necessary to have an attribute for > > > direct ejection control. Since the plan is to have a separate offline/online > > > attribute anyway (and a check preventing us from ejecting things that haven't > > > been put offline), it is not clear how useful it is going to be to control > > > ejection directly from user space. > > > > ok. > > Regarding the offline/online attribute and ejection prevention checking, do you > > mean the offline/online framework from Toshi: > > http://thread.gmane.org/gmane.linux.kernel/1420262 > > or something else? I assume this is the long-term plan. > > Unfortunately, the idea of adding a new set of common hotplug framework > was not well-received. Since the driver-core does not allow any eject > failure case, integrating into the driver-core framework seems also > impractical. > > > Is there any other short-term solution planned? If i understand correctly, until > > this framework is accepted, memory hot-remove is broken (=unsafe). > > That is correct. The alternative plan is to go with an ACPI-specific > approach that user has to off-line a target device and its children > beforehand from sysfs before initiating a hot-delete request. This > hot-delete request will fail if any of the devices are still on-line. > The sysfs online/offline interfaces may fail, and user (or user tool) > has to take care of the rollback as necessary. It would move all the > error handling & rollback stuff into the user space, and make the kernel > part very simple & straightforward -- just delete target device > objects. > > After looking further, however, I think this isn't the case... In case > of memory hot-delete, for example, off-lining is only a part of the job > done in remove_memory(). So, ACPI-core still needs to call > device-specific handlers to perform device-specific hot-delete > operations, such as calling remove_memory() or its sub-set function, > which can fail when a device is online. In order to make sure all > devices stay off-line, we need to delete their sysfs interfaces. No, we don't need to. > Since we do not have a way to serialize all online/offline & hot-plug > operations (the above patchset had such serialization, but did not get > thru), we cannot change all devices at once but delete sysfs interface > for each device one by one. If it failed on one of the devices, we need > to rollback to put them back into the original state. Other implication > is that this approach is not backward compatible. No. No rollbacks, please. There are three things that are needed: (1) online/offline, (2) a flag in struct acpi_device indicating whether or not the "physical" device represented by that struct acpi_device has been offlined, and (3) a synchronization mechanism that will make the manipulation of the flag and device eject mutually exclusive (it actually would need to tie the manipulation of the flag to the online/offline). Then, acpi_scan_hot_remove() will only need to check, before it calls acpi_bus_trim(), if all of the devices that correspond to the struct device objects to be removed have been offlined. Of course, it will have to ensure that the "online/offline" status of any of those devices won't change while it is running (hence, the synchronization mechanism). And once everything has been offlined, there's no reason why the removal should fail, right? > Given this, I am inclined to other alternative -- rework on my patchset > and make it as ACPI device hotplug framework. Please don't. Thanks, Rafael
On Mon, 2013-03-25 at 23:29 +0100, Rafael J. Wysocki wrote: > On Monday, March 25, 2013 02:45:36 PM Toshi Kani wrote: > > On Fri, 2013-03-15 at 11:47 +0100, Vasilis Liaskovitis wrote: > > > Hi, > > > > > > On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > > > > Sorry for the sluggish response, I've been travelling recently. -> > > > [...] > > > > > > > So, I'd suggest the following changes. > > > > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > > > > ACPI device objects. > > > > > > > - Make the !autoeject case as an exception for now, and emit > > > > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > > > > needs to be supported going forward. If so, we may want to consider a > > > > > > > different event type. > > > > > > > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > > > > exposing enabled only? Drivers would still be able to set the other flags on > > > > > > init on init to enforce the backwards-compatible behavior. > > > > > > > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > > > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > > > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > > > > KOBJ_OFFLINE? > > > > > > > > > > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > > > > with, but its meaning would be something like "keep backwards compatibility > > > > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > > > > like the old container driver did"? > > > > > > > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > > > > or planned for later? Also why shouldn't it be exposed to userpace? > > > > > > > > -> At this point we are not sure if it is necessary to have an attribute for > > > > direct ejection control. Since the plan is to have a separate offline/online > > > > attribute anyway (and a check preventing us from ejecting things that haven't > > > > been put offline), it is not clear how useful it is going to be to control > > > > ejection directly from user space. > > > > > > ok. > > > Regarding the offline/online attribute and ejection prevention checking, do you > > > mean the offline/online framework from Toshi: > > > http://thread.gmane.org/gmane.linux.kernel/1420262 > > > or something else? I assume this is the long-term plan. > > > > Unfortunately, the idea of adding a new set of common hotplug framework > > was not well-received. Since the driver-core does not allow any eject > > failure case, integrating into the driver-core framework seems also > > impractical. > > > > > Is there any other short-term solution planned? If i understand correctly, until > > > this framework is accepted, memory hot-remove is broken (=unsafe). > > > > That is correct. The alternative plan is to go with an ACPI-specific > > approach that user has to off-line a target device and its children > > beforehand from sysfs before initiating a hot-delete request. This > > hot-delete request will fail if any of the devices are still on-line. > > The sysfs online/offline interfaces may fail, and user (or user tool) > > has to take care of the rollback as necessary. It would move all the > > error handling & rollback stuff into the user space, and make the kernel > > part very simple & straightforward -- just delete target device > > objects. > > > > After looking further, however, I think this isn't the case... In case > > of memory hot-delete, for example, off-lining is only a part of the job > > done in remove_memory(). So, ACPI-core still needs to call > > device-specific handlers to perform device-specific hot-delete > > operations, such as calling remove_memory() or its sub-set function, > > which can fail when a device is online. In order to make sure all > > devices stay off-line, we need to delete their sysfs interfaces. > > No, we don't need to. > > > Since we do not have a way to serialize all online/offline & hot-plug > > operations (the above patchset had such serialization, but did not get > > thru), we cannot change all devices at once but delete sysfs interface > > for each device one by one. If it failed on one of the devices, we need > > to rollback to put them back into the original state. Other implication > > is that this approach is not backward compatible. > > No. No rollbacks, please. > > There are three things that are needed: (1) online/offline, (2) a flag in > struct acpi_device indicating whether or not the "physical" device represented > by that struct acpi_device has been offlined, acpi_device and its associated device(s) do not match 1 to 1. For instance, a memory acpi_device usually associates with multiple memblks sysfs files, which can be individually on-lined / off-lined. This association can be M:N matching. I am not sure if the flag can be implemented easily. > and (3) a synchronization > mechanism that will make the manipulation of the flag and device eject mutually > exclusive (it actually would need to tie the manipulation of the flag to > the online/offline). This needs to be a global lock that can serialize online/offline operations of all system devices. > Then, acpi_scan_hot_remove() will only need to check, before it calls > acpi_bus_trim(), if all of the devices that correspond to the struct device > objects to be removed have been offlined. Of course, it will have to ensure > that the "online/offline" status of any of those devices won't change while > it is running (hence, the synchronization mechanism). > > And once everything has been offlined, there's no reason why the removal should > fail, right? Yes, if we can introduce such global lock, we can prevent rollbacks. I was under an assumption that we cannot make such changes to the common code. > > Given this, I am inclined to other alternative -- rework on my patchset > > and make it as ACPI device hotplug framework. > > Please don't. OK, I will keep it myself for now. Are you going to make the code changes which you summarized? I am hoping that we can make some improvement for 3.10. Thanks, -Toshi -- 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
On Monday, March 25, 2013 04:57:11 PM Toshi Kani wrote: > On Mon, 2013-03-25 at 23:29 +0100, Rafael J. Wysocki wrote: > > On Monday, March 25, 2013 02:45:36 PM Toshi Kani wrote: > > > On Fri, 2013-03-15 at 11:47 +0100, Vasilis Liaskovitis wrote: > > > > Hi, > > > > > > > > On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > > > > > Sorry for the sluggish response, I've been travelling recently. -> > > > > [...] > > > > > > > > So, I'd suggest the following changes. > > > > > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > > > > > ACPI device objects. > > > > > > > > - Make the !autoeject case as an exception for now, and emit > > > > > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > > > > > needs to be supported going forward. If so, we may want to consider a > > > > > > > > different event type. > > > > > > > > > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > > > > > exposing enabled only? Drivers would still be able to set the other flags on > > > > > > > init on init to enforce the backwards-compatible behavior. > > > > > > > > > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > > > > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > > > > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > > > > > KOBJ_OFFLINE? > > > > > > > > > > > > > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > > > > > with, but its meaning would be something like "keep backwards compatibility > > > > > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > > > > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > > > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > > > > > like the old container driver did"? > > > > > > > > > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > > > > > or planned for later? Also why shouldn't it be exposed to userpace? > > > > > > > > > > -> At this point we are not sure if it is necessary to have an attribute for > > > > > direct ejection control. Since the plan is to have a separate offline/online > > > > > attribute anyway (and a check preventing us from ejecting things that haven't > > > > > been put offline), it is not clear how useful it is going to be to control > > > > > ejection directly from user space. > > > > > > > > ok. > > > > Regarding the offline/online attribute and ejection prevention checking, do you > > > > mean the offline/online framework from Toshi: > > > > http://thread.gmane.org/gmane.linux.kernel/1420262 > > > > or something else? I assume this is the long-term plan. > > > > > > Unfortunately, the idea of adding a new set of common hotplug framework > > > was not well-received. Since the driver-core does not allow any eject > > > failure case, integrating into the driver-core framework seems also > > > impractical. > > > > > > > Is there any other short-term solution planned? If i understand correctly, until > > > > this framework is accepted, memory hot-remove is broken (=unsafe). > > > > > > That is correct. The alternative plan is to go with an ACPI-specific > > > approach that user has to off-line a target device and its children > > > beforehand from sysfs before initiating a hot-delete request. This > > > hot-delete request will fail if any of the devices are still on-line. > > > The sysfs online/offline interfaces may fail, and user (or user tool) > > > has to take care of the rollback as necessary. It would move all the > > > error handling & rollback stuff into the user space, and make the kernel > > > part very simple & straightforward -- just delete target device > > > objects. > > > > > > After looking further, however, I think this isn't the case... In case > > > of memory hot-delete, for example, off-lining is only a part of the job > > > done in remove_memory(). So, ACPI-core still needs to call > > > device-specific handlers to perform device-specific hot-delete > > > operations, such as calling remove_memory() or its sub-set function, > > > which can fail when a device is online. In order to make sure all > > > devices stay off-line, we need to delete their sysfs interfaces. > > > > No, we don't need to. > > > > > Since we do not have a way to serialize all online/offline & hot-plug > > > operations (the above patchset had such serialization, but did not get > > > thru), we cannot change all devices at once but delete sysfs interface > > > for each device one by one. If it failed on one of the devices, we need > > > to rollback to put them back into the original state. Other implication > > > is that this approach is not backward compatible. > > > > No. No rollbacks, please. > > > > There are three things that are needed: (1) online/offline, (2) a flag in > > struct acpi_device indicating whether or not the "physical" device represented > > by that struct acpi_device has been offlined, > > acpi_device and its associated device(s) do not match 1 to 1. For > instance, a memory acpi_device usually associates with multiple memblks > sysfs files, which can be individually on-lined / off-lined. This > association can be M:N matching. I am not sure if the flag can be > implemented easily. If there are more "physical devices" associated with a single struct acpi_device (which is entirely possible), then that needs to be a counter rather than a flag. > > and (3) a synchronization > > mechanism that will make the manipulation of the flag and device eject mutually > > exclusive (it actually would need to tie the manipulation of the flag to > > the online/offline). > > This needs to be a global lock that can serialize online/offline > operations of all system devices. Yes, it does, but we already have acpi_scan_lock that serializes all hotplug operations on the ACPI level, so it won't add much overhead. And as far as memory is concerned, I really think it would be better not to offline two things at a time anyway. > > Then, acpi_scan_hot_remove() will only need to check, before it calls > > acpi_bus_trim(), if all of the devices that correspond to the struct device > > objects to be removed have been offlined. Of course, it will have to ensure > > that the "online/offline" status of any of those devices won't change while > > it is running (hence, the synchronization mechanism). > > > > And once everything has been offlined, there's no reason why the removal should > > fail, right? > > Yes, if we can introduce such global lock, we can prevent rollbacks. I > was under an assumption that we cannot make such changes to the common > code. I believe we can add such a lock of online/offline operations. > > > Given this, I am inclined to other alternative -- rework on my patchset > > > and make it as ACPI device hotplug framework. > > > > Please don't. > > OK, I will keep it myself for now. Are you going to make the code > changes which you summarized? I am hoping that we can make some > improvement for 3.10. Well, for now memory offline/online is missing and that's needed in the first place regardless. I'm not sure if I have the time to add it on time for the v3.10 merge window, however, because I have two conferences to attend in the meantime (where I'm going to speak) and some power management work to do. Thanks, Rafael
On Tue, 2013-03-26 at 13:22 +0100, Rafael J. Wysocki wrote: > On Monday, March 25, 2013 04:57:11 PM Toshi Kani wrote: > > On Mon, 2013-03-25 at 23:29 +0100, Rafael J. Wysocki wrote: > > > On Monday, March 25, 2013 02:45:36 PM Toshi Kani wrote: > > > > On Fri, 2013-03-15 at 11:47 +0100, Vasilis Liaskovitis wrote: > > > > > Hi, > > > > > > > > > > On Thu, Mar 14, 2013 at 06:16:30PM +0100, Rafael J. Wysocki wrote: > > > > > > Sorry for the sluggish response, I've been travelling recently. -> > > > > > [...] > > > > > > > > > So, I'd suggest the following changes. > > > > > > > > > - Remove the "uevents" attribute. KOBJ_ONLINE/OFFLINE are not used for > > > > > > > > > ACPI device objects. > > > > > > > > > - Make the !autoeject case as an exception for now, and emit > > > > > > > > > KOBJ_OFFLINE as a way to request off-lining to user. This uevent is > > > > > > > > > tied with the !autoeject case. We can then revisit if this use-case > > > > > > > > > needs to be supported going forward. If so, we may want to consider a > > > > > > > > > different event type. > > > > > > > > > > > > > > > > Well, what about avoiding to expose uevents and autoeject for now and > > > > > > > > exposing enabled only? Drivers would still be able to set the other flags on > > > > > > > > init on init to enforce the backwards-compatible behavior. > > > > > > > > > > > > > > Now that we don't define uevents and autoeject in v2 of this series, could you > > > > > > > explain how we get safe ejection from userspace e.g. for memory hot-remove? What > > > > > > > are the other flags drivers can use (on init?) to avoid autoeject and only issue > > > > > > > KOBJ_OFFLINE? > > > > > > > > > > > > > > > > > > > > > > > I agree that it would be sufficient to use one additional flag then, to start > > > > > > > > with, but its meaning would be something like "keep backwards compatibility > > > > > > > > with the old container driver", so perhaps "autoeject" is not a good name. > > > > > > > > > > > > > > > > What about "user_eject" (that won't be exposed to user space) instead? Where, > > > > > > > > if set, it would meand "do not autoeject and emit KOBJ_OFFLINE/ONLINE uevents > > > > > > > > like the old container driver did"? > > > > > > > > > > > > > > I don't see user_eject in v2. Is it unnecessary for userspace ejection control > > > > > > > or planned for later? Also why shouldn't it be exposed to userpace? > > > > > > > > > > > > -> At this point we are not sure if it is necessary to have an attribute for > > > > > > direct ejection control. Since the plan is to have a separate offline/online > > > > > > attribute anyway (and a check preventing us from ejecting things that haven't > > > > > > been put offline), it is not clear how useful it is going to be to control > > > > > > ejection directly from user space. > > > > > > > > > > ok. > > > > > Regarding the offline/online attribute and ejection prevention checking, do you > > > > > mean the offline/online framework from Toshi: > > > > > http://thread.gmane.org/gmane.linux.kernel/1420262 > > > > > or something else? I assume this is the long-term plan. > > > > > > > > Unfortunately, the idea of adding a new set of common hotplug framework > > > > was not well-received. Since the driver-core does not allow any eject > > > > failure case, integrating into the driver-core framework seems also > > > > impractical. > > > > > > > > > Is there any other short-term solution planned? If i understand correctly, until > > > > > this framework is accepted, memory hot-remove is broken (=unsafe). > > > > > > > > That is correct. The alternative plan is to go with an ACPI-specific > > > > approach that user has to off-line a target device and its children > > > > beforehand from sysfs before initiating a hot-delete request. This > > > > hot-delete request will fail if any of the devices are still on-line. > > > > The sysfs online/offline interfaces may fail, and user (or user tool) > > > > has to take care of the rollback as necessary. It would move all the > > > > error handling & rollback stuff into the user space, and make the kernel > > > > part very simple & straightforward -- just delete target device > > > > objects. > > > > > > > > After looking further, however, I think this isn't the case... In case > > > > of memory hot-delete, for example, off-lining is only a part of the job > > > > done in remove_memory(). So, ACPI-core still needs to call > > > > device-specific handlers to perform device-specific hot-delete > > > > operations, such as calling remove_memory() or its sub-set function, > > > > which can fail when a device is online. In order to make sure all > > > > devices stay off-line, we need to delete their sysfs interfaces. > > > > > > No, we don't need to. > > > > > > > Since we do not have a way to serialize all online/offline & hot-plug > > > > operations (the above patchset had such serialization, but did not get > > > > thru), we cannot change all devices at once but delete sysfs interface > > > > for each device one by one. If it failed on one of the devices, we need > > > > to rollback to put them back into the original state. Other implication > > > > is that this approach is not backward compatible. > > > > > > No. No rollbacks, please. > > > > > > There are three things that are needed: (1) online/offline, (2) a flag in > > > struct acpi_device indicating whether or not the "physical" device represented > > > by that struct acpi_device has been offlined, > > > > acpi_device and its associated device(s) do not match 1 to 1. For > > instance, a memory acpi_device usually associates with multiple memblks > > sysfs files, which can be individually on-lined / off-lined. This > > association can be M:N matching. I am not sure if the flag can be > > implemented easily. > > If there are more "physical devices" associated with a single struct > acpi_device (which is entirely possible), then that needs to be a counter > rather than a flag. Right. > > > and (3) a synchronization > > > mechanism that will make the manipulation of the flag and device eject mutually > > > exclusive (it actually would need to tie the manipulation of the flag to > > > the online/offline). > > > > This needs to be a global lock that can serialize online/offline > > operations of all system devices. > > Yes, it does, but we already have acpi_scan_lock that serializes all hotplug > operations on the ACPI level, so it won't add much overhead. And as far as > memory is concerned, I really think it would be better not to offline two > things at a time anyway. I agree. I actually tried to introduce such serialization in my former patchset. > > > Then, acpi_scan_hot_remove() will only need to check, before it calls > > > acpi_bus_trim(), if all of the devices that correspond to the struct device > > > objects to be removed have been offlined. Of course, it will have to ensure > > > that the "online/offline" status of any of those devices won't change while > > > it is running (hence, the synchronization mechanism). > > > > > > And once everything has been offlined, there's no reason why the removal should > > > fail, right? > > > > Yes, if we can introduce such global lock, we can prevent rollbacks. I > > was under an assumption that we cannot make such changes to the common > > code. > > I believe we can add such a lock of online/offline operations. That's great. > > > > Given this, I am inclined to other alternative -- rework on my patchset > > > > and make it as ACPI device hotplug framework. > > > > > > Please don't. > > > > OK, I will keep it myself for now. Are you going to make the code > > changes which you summarized? I am hoping that we can make some > > improvement for 3.10. > > Well, for now memory offline/online is missing and that's needed in the first > place regardless. Memory offline/online is already in-place through the memblk interface, i.e. /sys/devices/system/memory/memoryN. This is the one my other patch created symbolic links to. > I'm not sure if I have the time to add it on time for the > v3.10 merge window, however, because I have two conferences to attend in the > meantime (where I'm going to speak) and some power management work to do. Understood. I was just checking if you had already planed to do so. I will look further to see if I can be any help. Thanks, -Toshi -- 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
Index: test/include/acpi/acpi_bus.h =================================================================== --- test.orig/include/acpi/acpi_bus.h +++ test/include/acpi/acpi_bus.h @@ -88,11 +88,18 @@ struct acpi_device; * ----------------- */ +struct acpi_hotplug_profile { + bool enabled:1; + bool uevents:1; + bool autoeject:1; +}; + struct acpi_scan_handler { const struct acpi_device_id *ids; struct list_head list_node; int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id); void (*detach)(struct acpi_device *dev); + struct acpi_hotplug_profile hotplug; }; /* Index: test/drivers/acpi/scan.c =================================================================== --- test.orig/drivers/acpi/scan.c +++ test/drivers/acpi/scan.c @@ -107,32 +107,19 @@ acpi_device_modalias_show(struct device } static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL); -/** - * acpi_bus_hot_remove_device: hot-remove a device and its children - * @context: struct acpi_eject_event pointer (freed in this func) - * - * Hot-remove a device and its children. This function frees up the - * memory space passed by arg context, so that the caller may call - * this function asynchronously through acpi_os_hotplug_execute(). - */ -void acpi_bus_hot_remove_device(void *context) +static int acpi_scan_hot_remove(struct acpi_device *device) { - struct acpi_eject_event *ej_event = context; - struct acpi_device *device = ej_event->device; acpi_handle handle = device->handle; - acpi_handle temp; + acpi_handle not_used; struct acpi_object_list arg_list; union acpi_object arg; - acpi_status status = AE_OK; - u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */ - - mutex_lock(&acpi_scan_lock); + acpi_status status; /* If there is no handle, the device node has been unregistered. */ - if (!device->handle) { + if (!handle) { dev_dbg(&device->dev, "ACPI handle missing\n"); put_device(&device->dev); - goto out; + return -EINVAL; } ACPI_DEBUG_PRINT((ACPI_DB_INFO, @@ -143,7 +130,7 @@ void acpi_bus_hot_remove_device(void *co put_device(&device->dev); device = NULL; - if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", &temp))) { + if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", ¬_used))) { arg_list.count = 1; arg_list.pointer = &arg; arg.type = ACPI_TYPE_INTEGER; @@ -161,18 +148,159 @@ void acpi_bus_hot_remove_device(void *co */ status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL); if (ACPI_FAILURE(status)) { - if (status != AE_NOT_FOUND) + if (status == AE_NOT_FOUND) { + return -ENODEV; + } else { acpi_handle_warn(handle, "Eject failed\n"); + return -EIO; + } + } + return 0; +} - /* Tell the firmware the hot-remove operation has failed. */ - acpi_evaluate_hotplug_ost(handle, ej_event->event, - ost_code, NULL); +static void acpi_bus_device_eject(void *context) +{ + acpi_handle handle = context; + struct acpi_device *device = NULL; + struct acpi_scan_handler *handler; + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; + + mutex_lock(&acpi_scan_lock); + + acpi_bus_get_device(handle, &device); + if (!device) + goto err_out; + + handler = device->handler; + if (!handler || !handler->hotplug.enabled) { + ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED; + goto err_out; + } + acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, + ACPI_OST_SC_EJECT_IN_PROGRESS, NULL); + if (handler->hotplug.autoeject) { + int error; + + get_device(&device->dev); + error = acpi_scan_hot_remove(device); + if (error) + goto err_out; + } else { + device->flags.eject_pending = true; + if (handler->hotplug.uevents) + kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); } out: mutex_unlock(&acpi_scan_lock); - kfree(context); return; + + err_out: + acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code, + NULL); + goto out; +} + +static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source) +{ + struct acpi_device *device = NULL; + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; + int error; + + mutex_lock(&acpi_scan_lock); + + acpi_bus_get_device(handle, &device); + if (device) { + dev_warn(&device->dev, "Attempt to re-insert\n"); + goto out; + } + acpi_evaluate_hotplug_ost(handle, ost_source, + ACPI_OST_SC_INSERT_IN_PROGRESS, NULL); + error = acpi_bus_scan(handle); + if (error) { + acpi_handle_warn(handle, "Namespace scan failure\n"); + goto out; + } + error = acpi_bus_get_device(handle, &device); + if (error) { + acpi_handle_warn(handle, "Missing device node object\n"); + goto out; + } + ost_code = ACPI_OST_SC_SUCCESS; + if (device->handler && device->handler->hotplug.uevents) + kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); + + out: + acpi_evaluate_hotplug_ost(handle, ost_source, ost_code, NULL); + mutex_unlock(&acpi_scan_lock); +} + +static void acpi_scan_bus_check(void *context) +{ + acpi_scan_bus_device_check((acpi_handle)context, + ACPI_NOTIFY_BUS_CHECK); +} + +static void acpi_scan_device_check(void *context) +{ + acpi_scan_bus_device_check((acpi_handle)context, + ACPI_NOTIFY_DEVICE_CHECK); +} + +static void acpi_hotplug_notify_cb(acpi_handle handle, u32 type, void *not_used) +{ + acpi_osd_exec_callback callback; + acpi_status status; + + switch (type) { + case ACPI_NOTIFY_BUS_CHECK: + acpi_handle_debug(handle, "ACPI_NOTIFY_BUS_CHECK event\n"); + callback = acpi_scan_bus_check; + break; + case ACPI_NOTIFY_DEVICE_CHECK: + acpi_handle_debug(handle, "ACPI_NOTIFY_DEVICE_CHECK event\n"); + callback = acpi_scan_device_check; + break; + case ACPI_NOTIFY_EJECT_REQUEST: + acpi_handle_debug(handle, "ACPI_NOTIFY_EJECT_REQUEST event\n"); + callback = acpi_bus_device_eject; + break; + default: + /* non-hotplug event; possibly handled by other handler */ + return; + } + status = acpi_os_hotplug_execute(callback, handle); + if (ACPI_FAILURE(status)) + acpi_evaluate_hotplug_ost(handle, type, + ACPI_OST_SC_NON_SPECIFIC_FAILURE, + NULL); +} + +/** + * acpi_bus_hot_remove_device: hot-remove a device and its children + * @context: struct acpi_eject_event pointer (freed in this func) + * + * Hot-remove a device and its children. This function frees up the + * memory space passed by arg context, so that the caller may call + * this function asynchronously through acpi_os_hotplug_execute(). + */ +void acpi_bus_hot_remove_device(void *context) +{ + struct acpi_eject_event *ej_event = context; + struct acpi_device *device = ej_event->device; + acpi_handle handle = device->handle; + int error; + + mutex_lock(&acpi_scan_lock); + + error = acpi_scan_hot_remove(device); + if (error && handle) + acpi_evaluate_hotplug_ost(handle, ej_event->event, + ACPI_OST_SC_NON_SPECIFIC_FAILURE, + NULL); + + mutex_unlock(&acpi_scan_lock); + kfree(context); } EXPORT_SYMBOL(acpi_bus_hot_remove_device); @@ -206,51 +334,61 @@ static ssize_t acpi_eject_store(struct device *d, struct device_attribute *attr, const char *buf, size_t count) { - int ret = count; - acpi_status status; - acpi_object_type type = 0; struct acpi_device *acpi_device = to_acpi_device(d); struct acpi_eject_event *ej_event; + acpi_object_type not_used; + acpi_status status; + u32 ost_source; + int ret; - if ((!count) || (buf[0] != '1')) { + if (!count || buf[0] != '1') return -EINVAL; - } - if (!acpi_device->driver && !acpi_device->handler) { - ret = -ENODEV; - goto err; - } - status = acpi_get_type(acpi_device->handle, &type); - if (ACPI_FAILURE(status) || (!acpi_device->flags.ejectable)) { - ret = -ENODEV; - goto err; - } - ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL); - if (!ej_event) { - ret = -ENOMEM; - goto err; - } + if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled) + && !acpi_device->driver) + return -ENODEV; + + status = acpi_get_type(acpi_device->handle, ¬_used); + if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable) + return -ENODEV; + + mutex_lock(&acpi_scan_lock); - get_device(&acpi_device->dev); - ej_event->device = acpi_device; if (acpi_device->flags.eject_pending) { - /* event originated from ACPI eject notification */ - ej_event->event = ACPI_NOTIFY_EJECT_REQUEST; + /* ACPI eject notification event. */ + ost_source = ACPI_NOTIFY_EJECT_REQUEST; acpi_device->flags.eject_pending = 0; } else { - /* event originated from user */ - ej_event->event = ACPI_OST_EC_OSPM_EJECT; - (void) acpi_evaluate_hotplug_ost(acpi_device->handle, - ej_event->event, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL); + /* Eject initiated by user space. */ + ost_source = ACPI_OST_EC_OSPM_EJECT; } - + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL); + if (!ej_event) { + ret = -ENOMEM; + goto err_out; + } + acpi_evaluate_hotplug_ost(acpi_device->handle, ost_source, + ACPI_OST_SC_EJECT_IN_PROGRESS, NULL); + ej_event->device = acpi_device; + ej_event->event = ost_source; + get_device(&acpi_device->dev); status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event); if (ACPI_FAILURE(status)) { put_device(&acpi_device->dev); kfree(ej_event); + ret = status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN; + goto err_out; } -err: + ret = count; + + out: + mutex_unlock(&acpi_scan_lock); return ret; + + err_out: + acpi_evaluate_hotplug_ost(acpi_device->handle, ost_source, + ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL); + goto out; } static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store); @@ -1548,6 +1686,30 @@ static struct acpi_scan_handler *acpi_sc return NULL; } +static void acpi_scan_init_hotplug(acpi_handle handle) +{ + struct acpi_device_info *info; + struct acpi_scan_handler *handler; + + if (ACPI_FAILURE(acpi_get_object_info(handle, &info))) + return; + + if (!(info->valid & ACPI_VALID_HID)) { + kfree(info); + return; + } + + /* + * This relies on the fact that acpi_install_notify_handler() will not + * install the same notify handler routine twice for the same handle. + */ + handler = acpi_scan_match_handler(info->hardware_id.string, NULL); + kfree(info); + if (handler && handler->hotplug.enabled) + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, + acpi_hotplug_notify_cb, NULL); +} + static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used, void *not_used, void **return_value) { @@ -1570,6 +1732,8 @@ static acpi_status acpi_bus_check_add(ac return AE_OK; } + acpi_scan_init_hotplug(handle); + if (!(sta & ACPI_STA_DEVICE_PRESENT) && !(sta & ACPI_STA_DEVICE_FUNCTIONING)) { struct acpi_device_wakeup wakeup;