[PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[Bjorn Helgaas]

pci_register_driver() used to return the number of devices claimed,
and we made acpi_bus_register_driver() consistent with that.  But
pci_register_driver() recently changed to return only success/failure
(because the interface is to register the *driver*, not claim devices,
and devices may be hot-plugged later).

So let's do the same with acpi_bus_register_driver().

Note that asus_acpi.c used to unregister itself if it didn't claim
any devices.  Now it will remain registered, like everything else
in drivers/acpi.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas-VXdhtT5mjnY@public.gmane.org>

Index: work/drivers/acpi/scan.c
===================================================================
--- work.orig/drivers/acpi/scan.c	2005-07-27 10:17:43.000000000 -0600
+++ work/drivers/acpi/scan.c	2005-07-27 10:17:55.000000000 -0600
@@ -578,10 +578,9 @@
 	return_VALUE(result);
 }
 
-static int acpi_driver_attach(struct acpi_driver * drv)
+static void acpi_driver_attach(struct acpi_driver * drv)
 {
 	struct list_head * node, * next;
-	int count = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_driver_attach");
 
@@ -597,7 +596,6 @@
 			if (!acpi_bus_driver_init(dev, drv)) {
 				acpi_start_single_object(dev);
 				atomic_inc(&drv->references);
-				count++;
 				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
 						  drv->name, dev->pnp.bus_id));
 			}
@@ -605,7 +603,6 @@
 		spin_lock(&acpi_device_lock);
 	}
 	spin_unlock(&acpi_device_lock);
-	return_VALUE(count);
 }
 
 static int acpi_driver_detach(struct acpi_driver * drv)
@@ -636,16 +633,13 @@
  * acpi_bus_register_driver 
  * ------------------------ 
  * Registers a driver with the ACPI bus.  Searches the namespace for all
- * devices that match the driver's criteria and binds.  Returns the
- * number of devices that were claimed by the driver, or a negative
- * error status for failure.
+ * devices that match the driver's criteria and binds.  Returns zero for
+ * success or a negative error status for failure.
  */
 int
 acpi_bus_register_driver (
 	struct acpi_driver	*driver)
 {
-	int count;
-
 	ACPI_FUNCTION_TRACE("acpi_bus_register_driver");
 
 	if (acpi_disabled)
@@ -657,9 +651,9 @@
 	spin_lock(&acpi_device_lock);
 	list_add_tail(&driver->node, &acpi_bus_drivers);
 	spin_unlock(&acpi_device_lock);
-	count = acpi_driver_attach(driver);
+	acpi_driver_attach(driver);
 
-	return_VALUE(count);
+	return_VALUE(0);
 }
 EXPORT_SYMBOL(acpi_bus_register_driver);
 
Index: work/drivers/acpi/asus_acpi.c
===================================================================
--- work.orig/drivers/acpi/asus_acpi.c	2005-07-27 10:17:43.000000000 -0600
+++ work/drivers/acpi/asus_acpi.c	2005-07-27 10:17:55.000000000 -0600
@@ -1216,8 +1216,7 @@
 	asus_proc_dir->owner = THIS_MODULE;
 
 	result = acpi_bus_register_driver(&asus_hotk_driver);
-	if (result < 1) {
-		acpi_bus_unregister_driver(&asus_hotk_driver);
+	if (result < 0) {
 		remove_proc_entry(PROC_ASUS, acpi_root_dir);
 		return -ENODEV;
 	}


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[Karol Kozimor]

Thus wrote Bjorn Helgaas:
> pci_register_driver() used to return the number of devices claimed,
> and we made acpi_bus_register_driver() consistent with that.  But
> pci_register_driver() recently changed to return only success/failure
> (because the interface is to register the *driver*, not claim devices,
> and devices may be hot-plugged later).
> 
> So let's do the same with acpi_bus_register_driver().
> 
> Note that asus_acpi.c used to unregister itself if it didn't claim
> any devices.  Now it will remain registered, like everything else
> in drivers/acpi.

Ugh, not again... we've been there (early 2.6.x), done that and it was
absolutely confusing (I mean users reporting bogus bugs just because the
module was loaded type of confusing). Note that there's currently no easy
way to probe for a specific HID in ACPI namespace other than
acpi_bus_register_driver(), and unlike pci_register_driver() the former
has no hotplug mechanism to help. 

That's why at least some distros just plug in every ACPI module that is
kind enough to register on boot. The patch would cause asus_acpi to be
loaded on possibly every ACPI system and that's exactly the type of noise
I'd prefer to avoid (especially that there's no point in registering the
driver to wait for hotplug events in this specific case).

Therefore, while I generally agree with the idea, I'd rather ACPI used
sysfs and hot-/coldplug first so that acpi_bus_register_driver() might be
used in the same way pci_register_driver() is used now.

Of course, I might have become so out of touch with the generic code that
I'm missing something obvious, so please correct me if I'm wrong.

Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[Bjorn Helgaas]

On Thursday 28 July 2005 5:58 am, Karol Kozimor wrote:
> Thus wrote Bjorn Helgaas:
> > pci_register_driver() used to return the number of devices claimed,
> > and we made acpi_bus_register_driver() consistent with that.  But
> > pci_register_driver() recently changed to return only success/failure
> > (because the interface is to register the *driver*, not claim devices,
> > and devices may be hot-plugged later).
> > 
> > So let's do the same with acpi_bus_register_driver().
> > 
> > Note that asus_acpi.c used to unregister itself if it didn't claim
> > any devices.  Now it will remain registered, like everything else
> > in drivers/acpi.
> 
> Ugh, not again... we've been there (early 2.6.x), done that and it was
> absolutely confusing (I mean users reporting bogus bugs just because the
> module was loaded type of confusing). Note that there's currently no easy
> way to probe for a specific HID in ACPI namespace other than
> acpi_bus_register_driver(), and unlike pci_register_driver() the former
> has no hotplug mechanism to help. 

True.  But your driver can use its own mechanism to determine whether
it claimed any devices, and if not, call acpi_bus_unregister_driver()
and then return -ENODEV from the module init function to cause the
module to be unloaded.

Or is there some problem that would prevent that?


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[Bjorn Helgaas]

On Thursday 28 July 2005 9:26 am, Bjorn Helgaas wrote:
> On Thursday 28 July 2005 5:58 am, Karol Kozimor wrote:
> > Ugh, not again... we've been there (early 2.6.x), done that and it was
> > absolutely confusing (I mean users reporting bogus bugs just because the
> > module was loaded type of confusing). Note that there's currently no easy
> > way to probe for a specific HID in ACPI namespace other than
> > acpi_bus_register_driver(), and unlike pci_register_driver() the former
> > has no hotplug mechanism to help. 
> 
> True.  But your driver can use its own mechanism to determine whether
> it claimed any devices, and if not, call acpi_bus_unregister_driver()
> and then return -ENODEV from the module init function to cause the
> module to be unloaded.

Here's what I had in mind (untested).  Would something like this
address your concern?

Index: work/drivers/acpi/asus_acpi.c
===================================================================
--- work.orig/drivers/acpi/asus_acpi.c	2005-07-27 13:52:21.000000000 -0600
+++ work/drivers/acpi/asus_acpi.c	2005-07-28 09:56:17.000000000 -0600
@@ -1111,6 +1111,8 @@
 }
 
 
+static int __init asus_found;
+
 static int __init asus_hotk_add(struct acpi_device *device)
 {
 	acpi_status status = AE_OK;
@@ -1168,6 +1170,8 @@
 		}
 	}
 
+	asus_found = 1;
+
       end:
 	if (result) {
 		kfree(hotk);
@@ -1221,6 +1225,12 @@
 		return -ENODEV;
 	}
 
+	if (!asus_found) {
+		acpi_bus_unregister_driver(&asus_hotk_driver);
+		remove_proc_entry(PROC_ASUS, acpi_root_dir);
+		return -ENODEV;
+	}
+
 	return 0;
 }
 


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] ACPI: make acpi_bus_register_driver() return zero for success, not device count

[Karol Kozimor]

Thus wrote Bjorn Helgaas:
> > True.  But your driver can use its own mechanism to determine whether
> > it claimed any devices, and if not, call acpi_bus_unregister_driver()
> > and then return -ENODEV from the module init function to cause the
> > module to be unloaded.
> Here's what I had in mind (untested).  Would something like this
> address your concern?

It's clunky, but yes, at least until something better (say, sysfs support
for ACPI) goes in. Note that I'm on vacation till monday, so I won't be able
to test it either.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org

Acked-by: Karol Kozimor <sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org>


Index: work/drivers/acpi/asus_acpi.c
===================================================================
--- work.orig/drivers/acpi/asus_acpi.c	2005-07-27 13:52:21.000000000 -0600
+++ work/drivers/acpi/asus_acpi.c	2005-07-28 09:56:17.000000000 -0600
@@ -1111,6 +1111,8 @@
 }
 
 
+static int __init asus_found;
+
 static int __init asus_hotk_add(struct acpi_device *device)
 {
 	acpi_status status = AE_OK;
@@ -1168,6 +1170,8 @@
 		}
 	}
 
+	asus_found = 1;
+
       end:
 	if (result) {
 		kfree(hotk);
@@ -1221,6 +1225,12 @@
 		return -ENODEV;
 	}
 
+	if (!asus_found) {
+		acpi_bus_unregister_driver(&asus_hotk_driver);
+		remove_proc_entry(PROC_ASUS, acpi_root_dir);
+		return -ENODEV;
+	}
+
 	return 0;
 }
 



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf