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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
