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
Re: [RFC] ACPI scan handlers
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?
>
> 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.
>
>
Re: [RFC] ACPI scan handlers
On Saturday, January 26, 2013 02:49:30 AM Rafael J. Wysocki wrote: > On Friday, January 25, 2013 04:07:38 PM Toshi Kani wrote: > > On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: > > > On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: > > > > On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: > > : > > > > > > > > > > I wonder if anyone is seeing any major problems with this at the high > > > > > level. > > > > > > First of all, thanks for the response. :-) > > > > > > > I agree that the current model is mess. As shown below, it requires > > > > that .add() at boot-time only performs acpi dev init, and .add() at > > > > hot-add needs both acpi dev init and device on-lining. > > > > > > I'm not sure what you're talking about, though. > > > > > > You seem to be confusing ACPI device nodes (i.e. things represented by > > > struct > > > acpi_device objects) with devices, but they are different things. They > > > are > > > just used to store static information extracted from device objects in the > > > ACPI namespace and to expose those objects (and possibly some of their > > > properties) via sysfs. Device objects in the ACPI namespace are not > > > devices, > > > however, and they don't even need to represent devices (for example, the > > > _SB thing, which is represented by struct acpi_device, is hardly a > > > device). > > > > > > So the role of struct acpi_device things is analogous to the role of > > > struct device_node things in the Device Trees world. In fact, no drivers > > > should ever bind to them and in my opinion it was a grievous mistake to > > > let them do that. But I'm digressing. > > > > > > So, when you're saying "acpi dev", I'm not sure if you think about a > > > device node > > > or a device (possibly) represented by that node. If you mean device > > > node, then > > > I'm not sure what "acpi dev init" means, because device nodes by > > > definition > > > don't require any initialization beyond what acpi_add_single_object() does > > > (and they don't require any off-lining beyod what acpi_device_unregister() > > > does, for that matter). In turn, if you mean "device represented by the > > > given > > > device node", then you can't even say "ACPI device" about it, because it > > > very > > > well may be a PCI device, or a USB device, or a SATA device etc. > > > > Let me clarify my point with the ACPI memory driver as an example since > > it is the one that has caused a problem in .remove(). > > > > acpi_memory_device_add() implements .add() and does two things below. > > > > 1. Call _CRS and initialize a list of struct acpi_memory_info that is > > attached to acpi_device->driver_data. This step is what I described as > > "acpi dev init". ACPI drivers perform driver-specific initialization to > > ACPI device objects. > > > > 2. Call add_memory() to add a target memory range to the mm module. > > This step is what I described as "on-lining". This step is not > > necessary at boot-time since the mm module has already on-lined the > > memory ranges at early boot-time. At hot-add, however, it needs to call > > add_memory() with the current framework. > > I see. > > OK, so that does handle the "struct acpi_device has been registered" event, > both on boot and hot-add. The interactions with mm are tricky, I agree, but > that's not what I want to address at this point. > > > Similarly, acpi_memory_device_remove() implements .remove() and does two > > things below. > > > > 1. Call remove_memory() to offline a target memory range. This step, > > "off-lining", can fail since the mm module may or may not be able to > > delete non-movable ranges. This failure cannot be handled properly and > > causes the system to crash at this point. > > Well, if the system administrator wants to crash the system this way, it's > basically up to him. So that should be done by .detach() anyway in that case. > > > 2. Free up the list of struct acpi_memory_info. This step deletes > > driver-specific data from an ACPI device object. > > OK > > > > That's part of the whole confusion, by the way. > > > > > > If the device represented by an ACPI device node is on a natively > > > enumerated > > > bus, like PCI, then its native bus' init code initializes the device and > > > creates a "physical" device object for it, like struct pci_dev, which is > > > then > > > "glued" to the corresponding struct acpi_device by acpi_bind_one(). > > > Then, it > > > is clear which is which and there's no confusion. The confusion starts > > > when > > > there's no native enumeration and we only have the struct acpi_device > > > thing, > > > because then everybody seems to think "oh, there's no physical device > > > object > > > now, so this must be something different", but the *only* difference is > > > that > > > there is no native bus' init code now and we should still be creating a > > > "physical device" object for the device and we should "glue" it
Re: [RFC] ACPI scan handlers
On Saturday, January 26, 2013 02:49:30 AM Rafael J. Wysocki wrote: On Friday, January 25, 2013 04:07:38 PM Toshi Kani wrote: On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: : I wonder if anyone is seeing any major problems with this at the high level. First of all, thanks for the response. :-) I agree that the current model is mess. As shown below, it requires that .add() at boot-time only performs acpi dev init, and .add() at hot-add needs both acpi dev init and device on-lining. I'm not sure what you're talking about, though. You seem to be confusing ACPI device nodes (i.e. things represented by struct acpi_device objects) with devices, but they are different things. They are just used to store static information extracted from device objects in the ACPI namespace and to expose those objects (and possibly some of their properties) via sysfs. Device objects in the ACPI namespace are not devices, however, and they don't even need to represent devices (for example, the _SB thing, which is represented by struct acpi_device, is hardly a device). So the role of struct acpi_device things is analogous to the role of struct device_node things in the Device Trees world. In fact, no drivers should ever bind to them and in my opinion it was a grievous mistake to let them do that. But I'm digressing. So, when you're saying acpi dev, I'm not sure if you think about a device node or a device (possibly) represented by that node. If you mean device node, then I'm not sure what acpi dev init means, because device nodes by definition don't require any initialization beyond what acpi_add_single_object() does (and they don't require any off-lining beyod what acpi_device_unregister() does, for that matter). In turn, if you mean device represented by the given device node, then you can't even say ACPI device about it, because it very well may be a PCI device, or a USB device, or a SATA device etc. Let me clarify my point with the ACPI memory driver as an example since it is the one that has caused a problem in .remove(). acpi_memory_device_add() implements .add() and does two things below. 1. Call _CRS and initialize a list of struct acpi_memory_info that is attached to acpi_device-driver_data. This step is what I described as acpi dev init. ACPI drivers perform driver-specific initialization to ACPI device objects. 2. Call add_memory() to add a target memory range to the mm module. This step is what I described as on-lining. This step is not necessary at boot-time since the mm module has already on-lined the memory ranges at early boot-time. At hot-add, however, it needs to call add_memory() with the current framework. I see. OK, so that does handle the struct acpi_device has been registered event, both on boot and hot-add. The interactions with mm are tricky, I agree, but that's not what I want to address at this point. Similarly, acpi_memory_device_remove() implements .remove() and does two things below. 1. Call remove_memory() to offline a target memory range. This step, off-lining, can fail since the mm module may or may not be able to delete non-movable ranges. This failure cannot be handled properly and causes the system to crash at this point. Well, if the system administrator wants to crash the system this way, it's basically up to him. So that should be done by .detach() anyway in that case. 2. Free up the list of struct acpi_memory_info. This step deletes driver-specific data from an ACPI device object. OK That's part of the whole confusion, by the way. If the device represented by an ACPI device node is on a natively enumerated bus, like PCI, then its native bus' init code initializes the device and creates a physical device object for it, like struct pci_dev, which is then glued to the corresponding struct acpi_device by acpi_bind_one(). Then, it is clear which is which and there's no confusion. The confusion starts when there's no native enumeration and we only have the struct acpi_device thing, because then everybody seems to think oh, there's no physical device object now, so this must be something different, but the *only* difference is that there is no native bus' init code now and we should still be creating a physical device object for the device and we should glue it to the existing struct acpi_device like in the natively enumerated case. It then requires .remove() to perform both off-lining and acpi dev delete. .remove() must succeed, but off-lining can fail. acpi dev online ||=| add @ boot
Re: [RFC] ACPI scan handlers
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?
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
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
Re: [RFC] ACPI scan handlers
On Friday, January 25, 2013 04:07:38 PM Toshi Kani wrote: > On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: > > On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: > > > On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: > : > > > > > > > > I wonder if anyone is seeing any major problems with this at the high > > > > level. > > > > First of all, thanks for the response. :-) > > > > > I agree that the current model is mess. As shown below, it requires > > > that .add() at boot-time only performs acpi dev init, and .add() at > > > hot-add needs both acpi dev init and device on-lining. > > > > I'm not sure what you're talking about, though. > > > > You seem to be confusing ACPI device nodes (i.e. things represented by > > struct > > acpi_device objects) with devices, but they are different things. They are > > just used to store static information extracted from device objects in the > > ACPI namespace and to expose those objects (and possibly some of their > > properties) via sysfs. Device objects in the ACPI namespace are not > > devices, > > however, and they don't even need to represent devices (for example, the > > _SB thing, which is represented by struct acpi_device, is hardly a device). > > > > So the role of struct acpi_device things is analogous to the role of > > struct device_node things in the Device Trees world. In fact, no drivers > > should ever bind to them and in my opinion it was a grievous mistake to > > let them do that. But I'm digressing. > > > > So, when you're saying "acpi dev", I'm not sure if you think about a device > > node > > or a device (possibly) represented by that node. If you mean device node, > > then > > I'm not sure what "acpi dev init" means, because device nodes by definition > > don't require any initialization beyond what acpi_add_single_object() does > > (and they don't require any off-lining beyod what acpi_device_unregister() > > does, for that matter). In turn, if you mean "device represented by the > > given > > device node", then you can't even say "ACPI device" about it, because it > > very > > well may be a PCI device, or a USB device, or a SATA device etc. > > Let me clarify my point with the ACPI memory driver as an example since > it is the one that has caused a problem in .remove(). > > acpi_memory_device_add() implements .add() and does two things below. > > 1. Call _CRS and initialize a list of struct acpi_memory_info that is > attached to acpi_device->driver_data. This step is what I described as > "acpi dev init". ACPI drivers perform driver-specific initialization to > ACPI device objects. > > 2. Call add_memory() to add a target memory range to the mm module. > This step is what I described as "on-lining". This step is not > necessary at boot-time since the mm module has already on-lined the > memory ranges at early boot-time. At hot-add, however, it needs to call > add_memory() with the current framework. I see. OK, so that does handle the "struct acpi_device has been registered" event, both on boot and hot-add. The interactions with mm are tricky, I agree, but that's not what I want to address at this point. > Similarly, acpi_memory_device_remove() implements .remove() and does two > things below. > > 1. Call remove_memory() to offline a target memory range. This step, > "off-lining", can fail since the mm module may or may not be able to > delete non-movable ranges. This failure cannot be handled properly and > causes the system to crash at this point. Well, if the system administrator wants to crash the system this way, it's basically up to him. So that should be done by .detach() anyway in that case. > 2. Free up the list of struct acpi_memory_info. This step deletes > driver-specific data from an ACPI device object. OK > > That's part of the whole confusion, by the way. > > > > If the device represented by an ACPI device node is on a natively enumerated > > bus, like PCI, then its native bus' init code initializes the device and > > creates a "physical" device object for it, like struct pci_dev, which is > > then > > "glued" to the corresponding struct acpi_device by acpi_bind_one(). Then, > > it > > is clear which is which and there's no confusion. The confusion starts when > > there's no native enumeration and we only have the struct acpi_device thing, > > because then everybody seems to think "oh, there's no physical device object > > now, so this must be something different", but the *only* difference is that > > there is no native bus' init code now and we should still be creating a > > "physical device" object for the device and we should "glue" it to the > > existing struct acpi_device like in the natively enumerated case. > > > > > It then requires .remove() to perform both off-lining and acpi dev > > > delete. .remove() must succeed, but off-lining can fail. > > > > > > acpi dev online > > > ||=| > > > > > >add @ boot > > >
Re: [RFC] ACPI scan handlers
On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: > On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: > > On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: : > > > > > > I wonder if anyone is seeing any major problems with this at the high > > > level. > > First of all, thanks for the response. :-) > > > I agree that the current model is mess. As shown below, it requires > > that .add() at boot-time only performs acpi dev init, and .add() at > > hot-add needs both acpi dev init and device on-lining. > > I'm not sure what you're talking about, though. > > You seem to be confusing ACPI device nodes (i.e. things represented by struct > acpi_device objects) with devices, but they are different things. They are > just used to store static information extracted from device objects in the > ACPI namespace and to expose those objects (and possibly some of their > properties) via sysfs. Device objects in the ACPI namespace are not devices, > however, and they don't even need to represent devices (for example, the > _SB thing, which is represented by struct acpi_device, is hardly a device). > > So the role of struct acpi_device things is analogous to the role of > struct device_node things in the Device Trees world. In fact, no drivers > should ever bind to them and in my opinion it was a grievous mistake to > let them do that. But I'm digressing. > > So, when you're saying "acpi dev", I'm not sure if you think about a device > node > or a device (possibly) represented by that node. If you mean device node, > then > I'm not sure what "acpi dev init" means, because device nodes by definition > don't require any initialization beyond what acpi_add_single_object() does > (and they don't require any off-lining beyod what acpi_device_unregister() > does, for that matter). In turn, if you mean "device represented by the given > device node", then you can't even say "ACPI device" about it, because it very > well may be a PCI device, or a USB device, or a SATA device etc. Let me clarify my point with the ACPI memory driver as an example since it is the one that has caused a problem in .remove(). acpi_memory_device_add() implements .add() and does two things below. 1. Call _CRS and initialize a list of struct acpi_memory_info that is attached to acpi_device->driver_data. This step is what I described as "acpi dev init". ACPI drivers perform driver-specific initialization to ACPI device objects. 2. Call add_memory() to add a target memory range to the mm module. This step is what I described as "on-lining". This step is not necessary at boot-time since the mm module has already on-lined the memory ranges at early boot-time. At hot-add, however, it needs to call add_memory() with the current framework. Similarly, acpi_memory_device_remove() implements .remove() and does two things below. 1. Call remove_memory() to offline a target memory range. This step, "off-lining", can fail since the mm module may or may not be able to delete non-movable ranges. This failure cannot be handled properly and causes the system to crash at this point. 2. Free up the list of struct acpi_memory_info. This step deletes driver-specific data from an ACPI device object. > That's part of the whole confusion, by the way. > > If the device represented by an ACPI device node is on a natively enumerated > bus, like PCI, then its native bus' init code initializes the device and > creates a "physical" device object for it, like struct pci_dev, which is then > "glued" to the corresponding struct acpi_device by acpi_bind_one(). Then, it > is clear which is which and there's no confusion. The confusion starts when > there's no native enumeration and we only have the struct acpi_device thing, > because then everybody seems to think "oh, there's no physical device object > now, so this must be something different", but the *only* difference is that > there is no native bus' init code now and we should still be creating a > "physical device" object for the device and we should "glue" it to the > existing struct acpi_device like in the natively enumerated case. > > > It then requires .remove() to perform both off-lining and acpi dev > > delete. .remove() must succeed, but off-lining can fail. > > > > acpi dev online > > ||=| > > > >add @ boot > > > > >add @ hot-add > > --> > > <-- > > remove > > That assumes that the "driver" is present during boot (i.e. when > acpi_bus_scan() > is run for the first time), but what if it is not? With memory's example, the mm module must be present at boot. The system does not boot without it. > > Your proposal seems to introduce the following new model. If so, I do > > not think it addresses all the issues. > > It is not supposed to address *all* issues (whatever "all" means). It is > meant > to address precisely *one* problem, which is the abuse of the driver core by > the
Re: [RFC] ACPI scan handlers
On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote:
> On Thu, 2013-01-24 at 01:26 +0100, 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).
> >
> > 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.
> >
> > 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
Re: [RFC] ACPI scan handlers
On Thu, 2013-01-24 at 01:26 +0100, 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).
>
> 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.
>
> 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,
Re: [RFC] ACPI scan handlers
On Thu, 2013-01-24 at 01:26 +0100, 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).
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.
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
Re: [RFC] ACPI scan handlers
On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote:
On Thu, 2013-01-24 at 01:26 +0100, 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).
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.
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
Re: [RFC] ACPI scan handlers
On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: : I wonder if anyone is seeing any major problems with this at the high level. First of all, thanks for the response. :-) I agree that the current model is mess. As shown below, it requires that .add() at boot-time only performs acpi dev init, and .add() at hot-add needs both acpi dev init and device on-lining. I'm not sure what you're talking about, though. You seem to be confusing ACPI device nodes (i.e. things represented by struct acpi_device objects) with devices, but they are different things. They are just used to store static information extracted from device objects in the ACPI namespace and to expose those objects (and possibly some of their properties) via sysfs. Device objects in the ACPI namespace are not devices, however, and they don't even need to represent devices (for example, the _SB thing, which is represented by struct acpi_device, is hardly a device). So the role of struct acpi_device things is analogous to the role of struct device_node things in the Device Trees world. In fact, no drivers should ever bind to them and in my opinion it was a grievous mistake to let them do that. But I'm digressing. So, when you're saying acpi dev, I'm not sure if you think about a device node or a device (possibly) represented by that node. If you mean device node, then I'm not sure what acpi dev init means, because device nodes by definition don't require any initialization beyond what acpi_add_single_object() does (and they don't require any off-lining beyod what acpi_device_unregister() does, for that matter). In turn, if you mean device represented by the given device node, then you can't even say ACPI device about it, because it very well may be a PCI device, or a USB device, or a SATA device etc. Let me clarify my point with the ACPI memory driver as an example since it is the one that has caused a problem in .remove(). acpi_memory_device_add() implements .add() and does two things below. 1. Call _CRS and initialize a list of struct acpi_memory_info that is attached to acpi_device-driver_data. This step is what I described as acpi dev init. ACPI drivers perform driver-specific initialization to ACPI device objects. 2. Call add_memory() to add a target memory range to the mm module. This step is what I described as on-lining. This step is not necessary at boot-time since the mm module has already on-lined the memory ranges at early boot-time. At hot-add, however, it needs to call add_memory() with the current framework. Similarly, acpi_memory_device_remove() implements .remove() and does two things below. 1. Call remove_memory() to offline a target memory range. This step, off-lining, can fail since the mm module may or may not be able to delete non-movable ranges. This failure cannot be handled properly and causes the system to crash at this point. 2. Free up the list of struct acpi_memory_info. This step deletes driver-specific data from an ACPI device object. That's part of the whole confusion, by the way. If the device represented by an ACPI device node is on a natively enumerated bus, like PCI, then its native bus' init code initializes the device and creates a physical device object for it, like struct pci_dev, which is then glued to the corresponding struct acpi_device by acpi_bind_one(). Then, it is clear which is which and there's no confusion. The confusion starts when there's no native enumeration and we only have the struct acpi_device thing, because then everybody seems to think oh, there's no physical device object now, so this must be something different, but the *only* difference is that there is no native bus' init code now and we should still be creating a physical device object for the device and we should glue it to the existing struct acpi_device like in the natively enumerated case. It then requires .remove() to perform both off-lining and acpi dev delete. .remove() must succeed, but off-lining can fail. acpi dev online ||=| add @ boot add @ hot-add -- -- remove That assumes that the driver is present during boot (i.e. when acpi_bus_scan() is run for the first time), but what if it is not? With memory's example, the mm module must be present at boot. The system does not boot without it. Your proposal seems to introduce the following new model. If so, I do not think it addresses all the issues. It is not supposed to address *all* issues (whatever all means). It is meant to address precisely *one* problem, which is the abuse of the driver core by the ACPI subsystem (please see below). .attach() still needs to behave differently between boot and hot-add. Why does it? I don't
Re: [RFC] ACPI scan handlers
On Friday, January 25, 2013 04:07:38 PM Toshi Kani wrote: On Fri, 2013-01-25 at 23:11 +0100, Rafael J. Wysocki wrote: On Friday, January 25, 2013 09:52:21 AM Toshi Kani wrote: On Thu, 2013-01-24 at 01:26 +0100, Rafael J. Wysocki wrote: : I wonder if anyone is seeing any major problems with this at the high level. First of all, thanks for the response. :-) I agree that the current model is mess. As shown below, it requires that .add() at boot-time only performs acpi dev init, and .add() at hot-add needs both acpi dev init and device on-lining. I'm not sure what you're talking about, though. You seem to be confusing ACPI device nodes (i.e. things represented by struct acpi_device objects) with devices, but they are different things. They are just used to store static information extracted from device objects in the ACPI namespace and to expose those objects (and possibly some of their properties) via sysfs. Device objects in the ACPI namespace are not devices, however, and they don't even need to represent devices (for example, the _SB thing, which is represented by struct acpi_device, is hardly a device). So the role of struct acpi_device things is analogous to the role of struct device_node things in the Device Trees world. In fact, no drivers should ever bind to them and in my opinion it was a grievous mistake to let them do that. But I'm digressing. So, when you're saying acpi dev, I'm not sure if you think about a device node or a device (possibly) represented by that node. If you mean device node, then I'm not sure what acpi dev init means, because device nodes by definition don't require any initialization beyond what acpi_add_single_object() does (and they don't require any off-lining beyod what acpi_device_unregister() does, for that matter). In turn, if you mean device represented by the given device node, then you can't even say ACPI device about it, because it very well may be a PCI device, or a USB device, or a SATA device etc. Let me clarify my point with the ACPI memory driver as an example since it is the one that has caused a problem in .remove(). acpi_memory_device_add() implements .add() and does two things below. 1. Call _CRS and initialize a list of struct acpi_memory_info that is attached to acpi_device-driver_data. This step is what I described as acpi dev init. ACPI drivers perform driver-specific initialization to ACPI device objects. 2. Call add_memory() to add a target memory range to the mm module. This step is what I described as on-lining. This step is not necessary at boot-time since the mm module has already on-lined the memory ranges at early boot-time. At hot-add, however, it needs to call add_memory() with the current framework. I see. OK, so that does handle the struct acpi_device has been registered event, both on boot and hot-add. The interactions with mm are tricky, I agree, but that's not what I want to address at this point. Similarly, acpi_memory_device_remove() implements .remove() and does two things below. 1. Call remove_memory() to offline a target memory range. This step, off-lining, can fail since the mm module may or may not be able to delete non-movable ranges. This failure cannot be handled properly and causes the system to crash at this point. Well, if the system administrator wants to crash the system this way, it's basically up to him. So that should be done by .detach() anyway in that case. 2. Free up the list of struct acpi_memory_info. This step deletes driver-specific data from an ACPI device object. OK That's part of the whole confusion, by the way. If the device represented by an ACPI device node is on a natively enumerated bus, like PCI, then its native bus' init code initializes the device and creates a physical device object for it, like struct pci_dev, which is then glued to the corresponding struct acpi_device by acpi_bind_one(). Then, it is clear which is which and there's no confusion. The confusion starts when there's no native enumeration and we only have the struct acpi_device thing, because then everybody seems to think oh, there's no physical device object now, so this must be something different, but the *only* difference is that there is no native bus' init code now and we should still be creating a physical device object for the device and we should glue it to the existing struct acpi_device like in the natively enumerated case. It then requires .remove() to perform both off-lining and acpi dev delete. .remove() must succeed, but off-lining can fail. acpi dev online ||=| add @ boot add @ hot-add -- -- remove That assumes that the driver is present during boot (i.e. when acpi_bus_scan() is run for the first time),
[RFC] ACPI scan handlers
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).
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.
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
[RFC] ACPI scan handlers
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).
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.
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
