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 see any reason for that. With memory's example, calling add_memory() at boot is not necessary (which just fails and this failure cannot cause an error), but is necessary at hot-add (which should succeed in this case). > > The model is also asymmetric since the destructor of .attach() at hot-add > > is the combination of .detach() and .untie(). > > . > > attach @ boot > > --------> > > attach @ hot-add > > -----------------> > > detach untie > > <-------<--------- > > ---------> > > reclaim > > > > I believe device on-lining and off-lining steps should not be performed > > in .add() and .remove(). With this clarification, the current .add() > > & .remove() model works fine as follows. That is, .add() only performs > > acpi dev init, and .remove() only perform acpi dev delete (which is same > > as your .detach()). My system device hot-plug framework is designed to > > work with this model. > > Well, if I understand the above correctly, you're basically saying that if we > add a layer on top of the ACPI subsystem, we can separate "online" from "add" > and "offline" from "remove" in such a way that the "add" and "remove" will be > handled by the ACPI subsystem and "online" and "offline" will be done by the > extra layer. Right. In memory's example, the "online" part should be done by the mm module itself. > That quite precisely is what we should be doing, but the "add" operation should > include the creation of a "physical device" object, like for example struct > platform_device, and the additional layer should be a proper driver (a platform > driver for example) that will bind to that "physical device" object and > initialize the device (i.e. hardware). > > Analogously, the "remove" operation should include the removal of the "physical > device" object from which the driver will have to be unbound first. Agreed. With memory's example, the "remove" is also required to do "off-lining" (i.e. call remove_memory), which should not be the role of ACPI driver. > That I believe is what Greg meant when he was discussing your earlier proposal > with you. > > Now, however, the problem is what kind of a device object we should create > during the "add" phase (struct platform_device may not be suitable in some > cases) and whether that needs to be a single object or a whole bunch of them > (e.g. when the given struct acpi_device represents a bus or bridge, like in the > PCI host bridge case). That's what the ACPI scan handlers I'm proposing are > for. OK, so, we are thinking of different issues... :-) > So, an ACPI scan handler's .attach() is supposed to recognize what kind of > hardware is there to handle and to create whatever device objects (based on > struct device) are there to create etc. Then, there should be drivers that > will bind to those objects and so on. .detach(), in turn, is supposed to > reverse whatever .attach() has done. There is an additional complication, > though, that there may be an eject request between .attach() and .detach() > and it needs to be responded to. > > This really is about responding to three types of events related to the ACPI > namespace. Those events are, essentially: > > (1) Device node (i.e. struct acpi_device) has been registered. > (2) Eject has been requested for a device node. > (3) Device node goes away (i.e. it is going to be unregistered). > > Whatever the "model", we have to respond to the above events, this way or > another. > > Of course, (2) need not be the same as (3) in general, because one may envision > a refusal to carry out the eject. Currently, though, there is no distinction > between (2) and (3). > > The purpose of ACPI scan handlers I'm proposing is precisely to handle these > three types of events without abusing the driver core. How exactly they are > going to be handled will depend on the implementation of those handlers. > > The idea is that .attach(), .untie(), and .detach() will be called to handle > (1), (2), and (3), respectively, with the additional twist that after an eject > refusal .reclaim() needs to be called to do the cleanup. > > Well, perhaps the names .untie() and .reclaim() are not the best ones and it's > better to use names like .eject_requested() and .eject_refused() explicitly > for those callbacks? And analogously for the flag indicating that > .eject_requested() has succeeded for the given device? > > So, this is not about creating any new "model", it's just about doing what > needs to be done in a possibly straightforward way. > > Now, perhaps I should just post some code so that it's more clear what I mean. :-) Sounds like I did confuse completely! Anyway, even we have .untie() or .eject_requested(), I think all the hot-delete procedure may not be done within this function since an ACPI driver is not responsible for managing/controlling actual device. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
