Message-ID: <2707117.gaHTJBNB2O@vostro.rjw.lan>
Date:	Sat, 26 Jan 2013 22:46:35 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Jiang Liu <liuj97@...il.com>
Cc:	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Matthew Garrett <matthew.garrett@...ula.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Toshi Kani <toshi.kani@...com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] ACPI scan handlers

On Sunday, January 27, 2013 02:42:38 AM Jiang Liu wrote:
> On 01/24/2013 08:26 AM, Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > There is a considerable amount of confusion in the ACPI subsystem about what
> > ACPI drivers are used for.  Namely, some of them are used as "normal" device
> > drivers that bind to devices and handle them using ACPI control methods (like
> > the fan or battery drivers), but some of them are just used for handling
> > namespace events, such as the creation or removal of device nodes (I guess it
> > would be fair to call that an abuse of the driver core).  These two roles are
> > quite distinct, which is particularly visible from the confusion about the role
> > of the .remove() callback.
> > 
> > For the "normal" drivers this callback is simply used to handle situations in
> > which the driver needs to be unbound from the device, because one of them
> > (either the device or the driver) is going away.  That operation can't really
> > fail, it just needs to do the necessary cleanup.
> > 
> > However, for the namespace events handling "drivers" .remove() means that not
> > only the device node in question, but generally also the whole subtree below it
> > needs to be prepared for removal, which may involve deleting multiple device
> > objects belonging to different bus types and so on and which very well may fail
> > (for example, those devices may be used for such things like swap or they may be
> > memory banks used by the kernel and it may not be safe to remove them at the
> > moment etc.).  Moreover, for these things the removal of the "driver" doesn't
> > really make sense, because it has to be there to handle the namespace events it
> > is designed to handle or else things will go remarkably awry in some places.
> > 
> > To resolve all that mess I'd like to do the following, which in part is inspired
> > by the recent Toshi Kani's hotplug framework proposal and in part is based on
> > some discussions I had with Bjorn and others (the code references made below are
> > based on the current contens of linux-pm.git/linux-next).
> Hi Rafael,
> 	Great to know that you plan to clean up these messes. We are working on
> the same topic too.
> 
> > 
> > 1) Introduce a special data type for "ACPI namespace event handlers" like:
> > 
> > struct acpi_scan_handler {
> > 	const struct acpi_device_id *ids;
> > 	struct list_head list_node;
> > 	int (*attach)(struct acpi_device *adev);
> > 	int (*untie)(struct acpi_device *adev);
> > 	int (*reclaim)(struct acpi_device *adev);
> > 	void (*detach)(struct acpi_device *adev);
> > };
> > 
> >   an additional ACPI device flag:
> > 
> > struct acpi_device_flags {
> > ...
> > 	u32 untied:1;
> > ...
> > };
> > 
> >   and an additioanl field in struc acpi_device:
> > 
> > struct acpi_device {
> > ...
> > 	struct acpi_scan_handler *scan_handler;
> > ...
> > };
> > 
> >   (the roles of these things are described below).
> > 
> > 2) Introduce a list of struct acpi_scan_handler objects within the ACPI
> >    subsystem such that acpi_bus_device_attach() will search that list first and
> >    if there's a matching object (one whose ids match the device node), it will
> >    run that object's .attach() callback.
> > 
> >   If that returns 1, it will mean that the handler has claimed the device node
> >   and is now responsible for it until its .detach() callback is run.  Thus no
> >   driver can be bound to that device node and no other handler can claim it.
> >   Then, the device node's scan_handler field will be set to point to the handler
> >   that's claimed it and its untied flag will be cleared.
> > 
> >   If .attach() returns 0, it will mean that the handler has not recognized the
> >   device node and some other scan handlers and/or drivers may be tried for it.
> This doesn't seem perfect. The device enumeration logic still interferes with
> device drivers. Should we totally separate device enumeration logic from device
> drivers?

Yes, that's the ultimate plan, but we can't do that at the moment for practical
reasons (there are many ACPI drivers already in existence and we don't want to
introduce regressions gratuitously).

Introducing scan handlers more-or-less as described would be a step in that
direction, though.

> > 
> >   If an error code is returned, it will mean a hard error in which case the
> >   scanning of the namespace will have to be aborted.
> > 
> >   This way ACPI drivers will only be bound to device nodes that haven't been
> >   claimed by any scan handlers.
> > 
> > 3) Introduce an additional function following the format of acpi_bus_trim(),
> >    say acpi_bus_untie(), that will walk the namespace starting at the given
> >    device node and execute the .untie() callbacks from the scan handlers of
> >    all devices as post-order callbacks.
> > 
> >    If the .untie() callback for the given device node returns 0, it will mean
> >    that it is now safe to delete that node as long as its scan handler's
> >    .detach() callback is executed before the deletion.  In that case, the device
> >    node's untied flag will be set.
> > 
> >    Otherwise (i.e. if an error code is returned), it will mean that the scan
> >    handler has vetoed the untying and the whole operation should be reversed.
> >    Then, acpi_bus_untie() will walk the namespace again and execute the
> >    .reclaim() callbacks from the scan handlers of the device nodes whose untied
> >    flags are set as pre-order callbacks.
> > 
> >    If .reclaim() returns 0, the device node's untied flag will be cleared, and
> >    if an error code is returned, it will remain set.
> > 
> >    This will allow us to prepare the subtree below the given device node for
> >    removal in a reversible way, if needed.  Still, though, it will be possible
> >    to carry out a forcible removal calling acpi_bus_trim() after
> >    acpi_bus_untie() even if that has returned an error code (or even
> >    acpi_bus_trim() without acpi_bus_untie()).
> > 
> > 4) Make acpi_bus_device_detach() execute the .detach() callback from the
> >    scan handler of the device node (if the scan handler is present) and clear
> >    its scan_handler field along with its untied flag.  However, the untied flags
> >    will only be cleared after executing the .detach() callbacks, so that those
> >    callbacks can see whether or not the scan handlers have been successfully
> >    "untied" from the device nodes.
> > 
> > 5) Make acpi_bus_hot_remove_device() (and other pieces of code where that is
> >    convenient) call acpi_bus_untie() before acpi_bus_trim() and bail out
> >    cleanly if the untying fails (i.e. is vetoed by one of the scan handlers).
> > 
> > That should take care of the removal problem nicely and as far as I can say
> > the majority of the ACPI drivers used only for handling namespace events can
> > be readily converted to struct acpi_scan_handler objects.
> > 
> > I wonder if anyone is seeing any major problems with this at the high level.
> This design has some conflicts with our ongoing work to provide a system device
> hotplug framework. I will send out the draft framework for comments soon.

Well, I'd prefer it if you were more specific.  That is, please tell me what
conflicts and why exactly.

It also would be good if you looked at the Toshi Kani's latest proposal and
said what's wrong with it from your perspective.  You'll need to do that
anyway if you want to convince anyone that your proposal is better, so why
don't you do that now?  Moreover, sending yet another "system devices hotplug
framework" proposal at this point won't increase the chance that any of them
will be accepted.  Odds are to the contrary, I'm afraid.

Besides, I'm starting to think that such frameworks covering "system devices"
only are insufficient, because nowadays it is quite difficult to say what
devices are "system devices" in the first place and there is no reason why
those devices should require a special framework while the other devices
don't.  Quite frankly, it seems to me that either we need some additional
hotplug-related handling for all devices, or we don't need anything like that
at all.

I'm seeing reasons why it may be necessary, but that will need to be discussed
at the concept level, not within the context of any specific proposal.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists