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

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

[fseidel]

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).

Len, can this be applied, please?

Thanks,
Frank
--

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

[fseidel]

[-- Attachment #1: acpi_driverregister.diff --]
[-- Type: text/plain, Size: 994 bytes --]

From: Frank Seidel <fseidel@suse.de>

Let acpi_bus_register_driver report -ENODEV if
there was no driver attached to a device.

Signed-off-by: Frank Seidel <fseidel@suse.de>
---
 drivers/acpi/scan.c |    8 ++++++++
 1 files changed, 8 insertions(+)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -596,15 +596,23 @@ static void acpi_driver_detach(struct ac
  */
 int acpi_bus_register_driver(struct acpi_driver *driver)
 {
+	int count;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
+	count = atomic_read(&driver->references);
 	spin_lock(&acpi_device_lock);
 	list_add_tail(&driver->node, &acpi_bus_drivers);
 	spin_unlock(&acpi_device_lock);
 	acpi_driver_attach(driver);
 
+	/* Return error if there were no devices this
+	 * driver was additionally attached to.
+	 */
+	if (atomic_read(&driver->references) == count)
+		return -ENODEV;
+
 	return 0;
 }
 

--

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

[fseidel]

[-- Attachment #1: acpi_asus_cleanup.diff --]
[-- Type: text/plain, Size: 1357 bytes --]

From: Frank Seidel <fseidel@suse.de>

Cleanups in asus_acpi that are not need anymore
as of the fix of acpi_bus_register_driver.

Signed-off-by: Frank Seidel <fseidel@suse.de>
---
 drivers/acpi/asus_acpi.c |   17 -----------------
 1 files changed, 17 deletions(-)

Index: linux-2.6/drivers/acpi/asus_acpi.c
===================================================================
--- linux-2.6.orig/drivers/acpi/asus_acpi.c
+++ linux-2.6/drivers/acpi/asus_acpi.c
@@ -1239,8 +1239,6 @@ static int asus_hotk_check(void)
 	return result;
 }
 
-static int asus_hotk_found;
-
 static int asus_hotk_add(struct acpi_device *device)
 {
 	acpi_status status = AE_OK;
@@ -1301,8 +1299,6 @@ static int asus_hotk_add(struct acpi_dev
 		}
 	}
 
-	asus_hotk_found = 1;
-
 	/* LED display is off by default */
 	hotk->ledd_status = 0xFFF;
 
@@ -1357,19 +1353,6 @@ static int __init asus_acpi_init(void)
 		return result;
 	}
 
-	/*
-	 * This is a bit of a kludge.  We only want this module loaded
-	 * for ASUS systems, but there's currently no way to probe the
-	 * ACPI namespace for ASUS HIDs.  So we just return failure if
-	 * we didn't find one, which will cause the module to be
-	 * unloaded.
-	 */
-	if (!asus_hotk_found) {
-		acpi_bus_unregister_driver(&asus_hotk_driver);
-		remove_proc_entry(PROC_ASUS, acpi_root_dir);
-		return result;
-	}
-
 	return 0;
 }
 

--

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

[Thomas Renninger]

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).

Looks fine to me.

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.

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.

   Thomas

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

[Bjorn Helgaas]

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.

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.

Bjorn

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

[Thomas Renninger]

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

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

[Bjorn Helgaas]

On Sunday 22 October 2006 09:09, Thomas Renninger wrote:
> On Fri, 2006-10-20 at 10:55 -0600, Bjorn Helgaas wrote:
> > 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 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).

My original change:
  http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;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.

> > 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.

> 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
seems like the ACPI core should handle those notifications and
call the .add/.remove functions.  For example, maybe that could
be done in the acpi_bus_check_device() path, which currently has
some "TBD" comments about handing device insertion and removal.

Bjorn

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

[Frank Seidel]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

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 #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Frank Seidel]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

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).
>
> 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.
After some more testings and digging through the code.. i
agree with you that this is way to go (for now).
So, i sadfully now have to vote against my own patch ;-)

Len, please don't commit it. As Bjorn wrote, fixing the asus_acpi module
with return -ENODEV is the better solution.

Thanks,
Frank

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[patch] ACPI: asus_acpi: return -ENODEV when no device found

[Bjorn Helgaas]

This driver is for non-hot-pluggable hardware.  Therefore, if the hardware
isn't present, we want to return -ENODEV so a module load will fail.

We previously returned "result" from the acpi_bus_register_driver() call.
Registering the driver usually succeeds even if no devices are present,
because the ACPI core allows you to load a driver before a device is
hot-added.

Someday the kernel will expose enough information so user-space can tell
what ACPI drivers should be loaded.  But the kernel doesn't do that yet, so
distros have to load this driver always and rely on the -ENODEV causing the
load to fail if the hardware isn't found.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-1/drivers/acpi/asus_acpi.c
===================================================================
--- work-1.orig/drivers/acpi/asus_acpi.c	2006-10-24 15:35:24.000000000 -0600
+++ work-1/drivers/acpi/asus_acpi.c	2006-10-24 15:37:54.000000000 -0600
@@ -1367,7 +1367,7 @@
 	if (!asus_hotk_found) {
 		acpi_bus_unregister_driver(&asus_hotk_driver);
 		remove_proc_entry(PROC_ASUS, acpi_root_dir);
-		return result;
+		return -ENODEV;
 	}
 
 	return 0;

Re: [patch] ACPI: asus_acpi: return -ENODEV when no device found

[Frank Seidel]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Wednesday 25 October 2006 21:58, Bjorn Helgaas wrote:
> This driver is for non-hot-pluggable hardware.  Therefore, if the
> hardware isn't present, we want to return -ENODEV so a module load
> will fail.
>
> We previously returned "result" from the acpi_bus_register_driver()
> call. Registering the driver usually succeeds even if no devices are
> present, because the ACPI core allows you to load a driver before a
> device is hot-added.
>
> Someday the kernel will expose enough information so user-space can
> tell what ACPI drivers should be loaded.  But the kernel doesn't do
> that yet, so distros have to load this driver always and rely on the
> -ENODEV causing the load to fail if the hardware isn't found.
>
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Signed-off-by: Frank Seidel <fseidel@suse.de>

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Zhang Rui]

We won't have such issues if we make ACPI use driver model.
I've sent out a patch series which throw the legacy ACPI driver model
away. The patches will be added to -mm tree.

On Sat, 2006-10-21 at 00:55 +0800, Bjorn Helgaas wrote:
> > 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.
> 
that's true. With this patch series, .uevent method of ACPI bus will be
called when an ACPI device is found. Then udev script loads the right
driver by checking hid/cid offered by .uevent method. 

thanks,
Rui

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

[Thomas Renninger]

On Wed, 2006-10-25 at 18:40 +0800, Zhang Rui wrote:
> We won't have such issues if we make ACPI use driver model.
> I've sent out a patch series which throw the legacy ACPI driver model
> away. The patches will be added to -mm tree.
> 
> On Sat, 2006-10-21 at 00:55 +0800, Bjorn Helgaas wrote:
> > > 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.
> > 
> that's true. With this patch series, .uevent method of ACPI bus will be
> called when an ACPI device is found. Then udev script loads the right
> driver by checking hid/cid offered by .uevent method. 

That are great news.
Maybe you can CC linux-acpi list on further patches, I didn't know about
this.
I am looking forward to give these a try as soon as I get some time...

Thanks,

  Thomas