Re: [patch 0/2] acpi: driverregistration again can report ENODEV

[Thomas Renninger <trenn@suse.de>]

On Fri, 2006-10-20 at 10:55 -0600, Bjorn Helgaas wrote:
> On Friday 20 October 2006 03:59, Thomas Renninger wrote:
> > On Fri, 2006-10-20 at 11:25 +0200, fseidel@suse.de wrote:
> > > Currently acpi_bus_register_driver only reports an error
> > > (-ENODEV) if acpi_disabled is true,
> > > but many acpi drivers also depend on a negative error
> > > value if no driver could be attached to a device
> > > (as e.g. driver/acpi/asus_acpi.c).
> > > This patch adds this again (without changing return type
> > > of acpi_driver_attach for this).
> 
> I object to this.
> 
> > acpi_bus_register_driver must return -ENODEV when no device with
> > matching HID was found. E.g. battery also should currently get always
> > loaded even a system never has one.
> 
> If the driver wants to unload if it doesn't find any devices,
> it can easily do so by counting calls to its add() method, as
> asus_acpi.c currently does.  I don't think that's a good long-
> term solution, though.  I think it would be better to have a
> hotplug scheme that loads the driver when the hardware is
> found, like we do for PCI.
> 
> > driver_attach returned the amount of found devices in past, this was
> > changed recently (can't find the patch/mail) right now. This change
> > broke things and should get corrected by these patches.
> 
> Here's my patch that results in the current behavior:
>   http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=9d9f749b316ac21cb59ad3e595cbce469b409e1a
> 
> What did it break?  I haven't seen any reports of problems.
asus_acpi init func returns the return value of bus_register_driver()
> 
> 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. Not sure about devices that get
added
by dynamically loaded SSDTs later, but this also cannot be handled by
ugly
workarounds in hotpluggable device drivers as it is currently done (e.g.
walk namespace in memory hotplug).

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

I think this is the right way to cleanup things to better integrate for
hotplug
design.
Not sure about the .start and .stop funcs, I think they should be thrown
out.
Currently they are nearly the same than .add .remove and are useless.
Whether the status of the device changed to (un-)present can be
determined in
the notify handler of the driver anyway.

Again, if you think I haven't overseen anything there, I can come up
with a
patch for latest kernel and adjusting all modules to:
  - always call .add for each device listed in ACPI namespace, present
or not.
  - throws out currently unused .start/.stop callbacks in acpi_driver
struct
  - moves installing notify handler from driver to bus_register_driver
if a acpi_driver
    struct has a .notify callback associated to it. Use the driver_data
that possibly
    got filled up in .add func of the driver of each found device
    and pass it as notifier callback data.

If this is done:
  - batteries that are not present at boot time, but are added at later
time will
    be recognised and just work fine
  - memory hotplug (probably also container.c) can be cleaned up and
also make
    use of the bus_register_driver without bad hacks.

Thanks,

   Thomas