Re: [PATCH 2/2] ACPI / scan: Simplify container driver

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, February 07, 2013 07:32:07 AM Toshi Kani wrote:
> On Thu, 2013-02-07 at 02:32 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 06, 2013 05:51:42 PM Toshi Kani wrote:
> > > On Thu, 2013-02-07 at 01:55 +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, February 06, 2013 03:32:18 PM Toshi Kani wrote:
> > > > > On Mon, 2013-02-04 at 00:47 +0100, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > The only useful thing that the ACPI container driver does is to
> > > > > > install system notify handlers for all container and module device
> > > > > > objects it finds in the namespace.  The driver structure,
> > > > > > acpi_container_driver, and the data structures created by its
> > > > > > .add() callback are in fact not used by the driver, so remove
> > > > > > them entirely.
> > > > > > 
> > > > > > It also makes a little sense to build that driver as a module,
> > > > > > so make it non-modular and add its initialization to the
> > > > > > namespace scanning code.
> > > > > > 
> > > > > > In addition to that, make the namespace walk callback used for
> > > > > > installing the notify handlers more straightforward.
> > > > > 
> > > > > I think the container driver needs to be registered as an ACPI scan
> > > > > driver so that sysfs eject will continue to work for container devices,
> > > > > such as ACPI0004:XX/eject.  Since the container driver does not support
> > > > > ACPI eject notification (and we have been discussing how system device
> > > > > hot-plug should work), this sysfs eject is the only way to eject a
> > > > > container device at this point.  I will send an update patchset that
> > > > > applies on top of this patch.
> > > > > 
> > > > > With the update in my patchset:
> > > > > Reviewed-by: Toshi Kani <toshi.kani@hp.com>
> > > > 
> > > > Thanks, but I'd like to (1) apply your patch from
> > > > https://patchwork.kernel.org/patch/2108851/ first and then (2) fold your [2/2]
> > > > into my [2/2], if you don't mind, and apply that next.
> > > 
> > > That's fine by me.
> > > 
> > > > Moreover, I'm wondering if the #ifndef FORCE_EJECT thing in acpi_eject_store()
> > > > actually makes sense after the recent changes to acpi_bus_trim(), because that
> > > > can't fail now, so the eject will always be carried out.  So perhaps we can
> > > > simply remove the acpi_device->driver check from there entirely in the first
> > > > place?
> > > >
> > > > If we really want to be able to prevent ejects from happening in some cases,
> > > > we need to implement something along the lines discussed with Greg.
> > > 
> > > acpi_bus_trim() cannot fail, but sysfs eject can fail.  So, I think it
> > > makes sense to do some validation before calling acpi_bus_trim().  If we
> > > are to implement the no_eject flag thing, that check needs to be made
> > > before calling acpi_bus_trim().
> > 
> > Sure, but now the logic seems to be "if FORCE_EJECT is not set, don't eject
> > devices that have no ACPI drivers", so I'm wondering what the purpose of this
> > is.  It definitely isn't too obvious. :-)
> 
> The check sounds odd for container, but is necessary for CPU and memory
> for now.  CPU and memory go online without their ACPI drivers at boot.
> So, without this check (i.e. FORCE_EJECT is set), it simply ejects them
> without attempting to offline when the ACPI drivers are not bound.  Of
> course, we have the issue of a failure in offline be ignored, so this
> offlining part needs to be moved out from acpi_bus_trim() in one way or
> the other.

That was my point.

I'm going to add that change for now, but I think we need to take a step back
and talk about how we want the whole eject machinery to work, regardless of
the offline/online problem.

I think that it should work in the same way for all things that may be ejected
or inserted.  Namely, they all should use the same notify handler, for example,
and if we generate a uevent for one, we should do that for all of them.
Question is how that notify handler should work and here there are two chices
in my view: Either we'll always emit a uevent and wait for user space to start
the eject procedure via sysfs, or we won't emit uevents at all and rely on the
"no_eject" flag to trigger if something is not ready.  I'm basically fine with
any of them (the "no_eject" flag may be useful even if we rely on user space
to offline stuff before triggering the eject in my opinion), but if we're going
to rely on user space, then there needs to be a timeout for letting the BIOS
know that the eject has failed.

[There may be a flag for the common code telling it whether to emit a uevent
and wait for user space to trigger eject or to trigger eject by itself.]

Next, I think there needs to be a global list of IDs for which we'll install
hot-plug notify handlers and which we'll allow to be ejected via /sys/.../eject.
So, if a device ID is on that list, we'll install the (common) hot-plug notify
handler for its ACPI handle and we'll set an "eject_possible" flag in its
struct acpi_device (when created).  That will need to be done for every scan
of the ACPI namespace and not just once, BTW.  And we'll check the
"eject_possible" flag in acpi_eject_store() instead of the "does it have a
driver or scan handler" check.

Then, the scan handlers for hot-plug devices will be able to add their IDs to
that global list instead of walking the namespace and installing notify handlers
by themselves (which as I said has a problem that it's done once, while it
should be done every time acpi_bus_scan() runs).

I wonder what you think.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.