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

[Toshi Kani]

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@xxxxxxxxx>
> > > > > 
> > > > > 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@xxxxxx>
> > > 
> > > 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.

Thanks,
-Toshi

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