Hi,
first i want to thank you for your feedback, which i really appreciate
very much.
On Monday 23 October 2006 17:34, Bjorn Helgaas wrote:
> I think the current tree contains a merge error in asus_acpi_init().
> It says:
>
> if (!asus_hotk_found) {
> ...
> return result;
>
> But "result" is always zero at this point (it came from
> acpi_bus_register_driver(), which returns either zero or a negative
> error value).
I got convinced that this is source of the problem.
acpi_bus_register_driver() shouldn't always return zero, but -ENODEV
if no device was attached to the driver.
Currently it only does so if acpi is disabled.
>
> My original change:
>
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=co
>mmitdiff;h=578b333bfe8eb1360207a08a53c321822a8f40f3 contained:
>
> if (!asus_hotk_found) {
> ...
> return -ENODEV;
>
> which is what we want. I think the -ENODEV got lost when resolving
> conflicts with the "propagate correct return value" patches that
> happened at about the same time.
That is the very same solution i also first thought of for this problem.
But as the result var here holds the return value of
acpi_bus_register_driver(), asus_acpi is also ok again with this fix.
> > > I think it's a mistake to have acpi_bus_register_driver() return
> > > any indication of how many devices were claimed. If the driver
> > > needs to know how many devices it claimed, it can easily count
> > > them in its add() method. Returning the count from
> > > register_driver() is racey if devices can be hot-plugged.
> >
> > I think this is wrong.
> > It needs not to return the number of claimed devices, it needs to
> > return -ENODEV if no device will ever exist.
>
> There is no way for acpi_bus_register_driver() to know that no device
> will ever exist.
But it can determine if the driver could be attached to devices or not
as acpi_driver_attach increases the references counter each time
it could bind the driver.
> > The whole culprit to hotplug design (and you are right, this patch
> > also does not
> > work because of this) is this line in acpi_driver_attach:
> > if (dev->driver || !dev->status.present)
> > continue;
> > If a device is not present, but exists in ACPI namespace, .add
> > function must still be
> > called.
> > Can you/someone have a look at my mail a month ago:
> > http://marc.theaimsgroup.com/?l=linux-acpi&m=115833743117570&w=2
>
> Sorry, I missed this and haven't had time to pay any attention
> to this for a long time.
>
> I'm not sure I agree with your approach. I don't think a driver
> should have to have a notify handler just to handle hotplug. It
Perhaps i got Thomas wrong, but i think he doesn't intend the
.notify callback functions to be mandatory for every driver (as many
don't need them at all). But this is a nice way for drivers that e.g.
need to/should stay even if the devices got removed or never were
available until now (like for batteries).
In general .add and .remove sometimes are (or seemt to be) not enough
and .notify callbacks seems a elegant way to solve this in a common
way.
But i have to admit that the changes this trails in the other drivers
(using notifications) is a drawback on the one side, but on the other
this also could be the chance to do some great cleanups (e.g. in
memory hotplug).
Frank
Attachment:
pgpGPMJ535foV.pgp
Description: PGP signature
