ACPI hot-plug notification help needed

ACPI hot-plug notification help needed

[Bjorn Helgaas]

Can somebody explain how the notification stuff in the ACPI
memory and container drivers works?

The usual ACPI driver registers its IDs and add() and remove()
functions with acpi_bus_register_driver(), and the ACPI core
calls the add() function when it finds a device that is present
(based on _STA) and whose _HID or _CID matches.

The memory and container drivers (acpi_memhotplug.c[1] and container.c)
also do this.  But in addition, they traverse the entire ACPI namespace
and attach notify handlers to any devices that match the IDs[2], even
if the _STA indicates "not present".

It seems strange to attach notify handlers to devices that are
not present.  Maybe ACPI allows events to be sent to devices that
don't exist, but it just seems weird.  I'd expect a hotplug add to
cause events to some previously-present *parent* of the new device.
The parent's driver could then re-scan its children and add any
that are newly-added or newly-marked "present".

But these drivers seem to expect hot-add events on the new device
itself, and then they go through the motions of acpi_bus_add()
directly.  This seems like it belongs somewhere in the ACPI core.

I just noticed that the processor_core and acpiphp_glue drivers
do the same thing.  Does this mean that other ACPI drivers that
want to handle hot-plug will also have to go through this dance
of attaching notify handlers to non-present devices?  Does ACPI
have to supply nodes for every possible hot-pluggable device in
the namespace at boot-time, just to provide places to hang these
handlers?

Please enlighten me.

[1] "acpi_memhotplug.c" seems redundant.  Wouldn't "memory.c"
     be just as descriptive?  It's in the ACPI directory, so
     we don't need "acpi_", and ACPI drivers in general can
     support hotplug, so it doesn't seem like we need that either.

[2] This match is not as completely implemented as that in the
    ACPI core, so it doesn't check _CID.  So this seems like a
    case where duplication of functionality leads to maintenance
    problems.  Compare is_root_bridge(), is_memory_device(), and
    container_walk_namespace_cb().


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: ACPI hot-plug notification help needed

[Keshavamurthy Anil S]

On Mon, Jul 25, 2005 at 05:35:23PM -0600, Bjorn Helgaas wrote:
> Can somebody explain how the notification stuff in the ACPI
> memory and container drivers works?
Sure.
> 
> The usual ACPI driver registers its IDs and add() and remove()
> functions with acpi_bus_register_driver(), and the ACPI core
> calls the add() function when it finds a device that is present
> (based on _STA) and whose _HID or _CID matches.

Humm.. I would say that ACPI core calls the add() functions
for all the devices which it has seen during its boot( of course
based on _STA) and whose _HID or _CID matches. 

Here is how the current core works.
During system boot the ACPI core scans for all the devices 
in its root namespace scope i.e by calling acpi_bus_scan() 
and for each device whose _STA is present calls acpi_bus_add() 
which stores the device in linked list. Later when ACPI driver 
registers its IDs the ACPI core just looks in this device list.
The core does not keep track of devices whose _STA became present
after boot.

> 
> The memory and container drivers (acpi_memhotplug.c[1] and container.c)
> also do this.  But in addition, they traverse the entire ACPI namespace
> and attach notify handlers to any devices that match the IDs[2], even
> if the _STA indicates "not present".

This is how the ACPI core expects us to register for all the devices
for which we care notifications. We want to register for all the devices
i.e for the devices which are present and for devices which are not present.
To do this we walk the entire namespace scope and check for _HID or _CID
match and register a notify handler for this device.

> 
> It seems strange to attach notify handlers to devices that are
> not present.  Maybe ACPI allows events to be sent to devices that
> don't exist, but it just seems weird.  I'd expect a hotplug add to

When we were implementing this even for us it looked strange that
we have to attach a notify handlers for each and every device. Believe
me this is how we had to do.

> cause events to some previously-present *parent* of the new device.
> The parent's driver could then re-scan its children and add any
> that are newly-added or newly-marked "present".

This might not alway be true. Some f/w might not want to send
notification on the parent as the re-scan might be a costly operation
as the parent might contain a large number of children. Imaging a case
where we want to attach a device under _SB scope, in case if the 
notification went to _SB and we have rescan the whole namespace here.

> 
> But these drivers seem to expect hot-add events on the new device
> itself, and then they go through the motions of acpi_bus_add()
> directly.  This seems like it belongs somewhere in the ACPI core.

The core functionality still recides inside ACPI core, it just that
these drivers which support hotplug are responsible for registering
for a notification and based on the notification they have to call
acpi_bus_add() to add the device and from then on the acpi_bus_add()
function takes care of doing rest of the stuff including calling
add() function of the driver.

> 
> I just noticed that the processor_core and acpiphp_glue drivers
> do the same thing.  Does this mean that other ACPI drivers that
> want to handle hot-plug will also have to go through this dance
> of attaching notify handlers to non-present devices?  Does ACPI
Don't you think even acpiphp drivers which support slot hotplug
also does the same thing?
> have to supply nodes for every possible hot-pluggable device in
> the namespace at boot-time, just to provide places to hang these
> handlers?
Yes, the ACPI namespace has to list its max possible configuration
in its namespace.

> 
> Please enlighten me.
> 
> [1] "acpi_memhotplug.c" seems redundant.  Wouldn't "memory.c"
>      be just as descriptive?  It's in the ACPI directory, so
>      we don't need "acpi_", and ACPI drivers in general can
>      support hotplug, so it doesn't seem like we need that either.
Naveen, any thoughts on this one?
> 
> [2] This match is not as completely implemented as that in the
>     ACPI core, so it doesn't check _CID.  So this seems like a
>     case where duplication of functionality leads to maintenance
>     problems.  Compare is_root_bridge(), is_memory_device(), and
>     container_walk_namespace_cb().
Patch welcome:-)

-Anil


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: ACPI hot-plug notification help needed

[Bjorn Helgaas]

On Monday 25 July 2005 6:39 pm, Keshavamurthy Anil S wrote:
> On Mon, Jul 25, 2005 at 05:35:23PM -0600, Bjorn Helgaas wrote:
> > Can somebody explain how the notification stuff in the ACPI
> > memory and container drivers works?
> Sure.

Thanks for your useful response!

> During system boot the ACPI core scans for all the devices 
> in its root namespace scope i.e by calling acpi_bus_scan() 
> and for each device whose _STA is present calls acpi_bus_add() 
> which stores the device in linked list. Later when ACPI driver 
> registers its IDs the ACPI core just looks in this device list.
> The core does not keep track of devices whose _STA became present
> after boot.

Is this the way you think it *should* work, or just the way it
happens to work today?  Certainly PCI doesn't work this way --
the driver may register at boot-time, but when a new device is
added later via hot-plug, the hot-plug infrastructure arranges
to call the driver's probe() method.

> This is how the ACPI core expects us to register for all the devices
> for which we care notifications. We want to register for all the devices
> i.e for the devices which are present and for devices which are not present.
> To do this we walk the entire namespace scope and check for _HID or _CID
> match and register a notify handler for this device.

If we really have to do this, should we change things around so
an ACPI driver can request that its add() method be called
for non-present devices as well as present ones?  That would
at least centralize all the _HID/_CID checking and remove the
namespace walks from the drivers.

> > It seems strange to attach notify handlers to devices that are
> > not present.  Maybe ACPI allows events to be sent to devices that
> > don't exist, but it just seems weird.  I'd expect a hotplug add to
> 
> When we were implementing this even for us it looked strange that
> we have to attach a notify handlers for each and every device. Believe
> me this is how we had to do.
> 
> > cause events to some previously-present *parent* of the new device.
> > The parent's driver could then re-scan its children and add any
> > that are newly-added or newly-marked "present".
> 
> This might not alway be true. Some f/w might not want to send
> notification on the parent as the re-scan might be a costly operation
> as the parent might contain a large number of children. Imaging a case
> where we want to attach a device under _SB scope, in case if the 
> notification went to _SB and we have rescan the whole namespace here.

Yes, I now see that the spec[3] allows the notify to go to either
the new device itself, or to some parent of the new device, such
as the bus the new device is on.  The first case seems like muddled
thinking to me, but I guess we're now stuck with having to support
it.  But I still don't like it :-)

The current scheme of attaching notify handlers only works if
every possible device is present in the namespace at the time
the driver is loaded.  Is there a requirement in the spec for
this?  I could imagine a hot-add event using LoadTable() to
add new devices to the namespace before notifying the parent
bus.  Is there code somewhere in ACPI to re-scan the bus and
attach drivers?

> [Those] drivers which support hotplug are responsible for registering
> for a notification and based on the notification they have to call
> acpi_bus_add() to add the device and from then on the acpi_bus_add()
> function takes care of doing rest of the stuff including calling
> add() function of the driver.

This is much different from the PCI philosophy, where new-style
drivers handle hot-plug seamlessly.  I think ACPI should move in
that direction.  The current style is clunky and error-prone.

> > Does ACPI
> > have to supply nodes for every possible hot-pluggable device in
> > the namespace at boot-time, just to provide places to hang these
> > handlers?
> Yes, the ACPI namespace has to list its max possible configuration
> in its namespace.

That seems broken.  Does the spec actually require this somewhere?
I can understand that some firmware might want to have the max
possible configuration at boot, but it seems like there ought to
be the option to do it dynamically.

> > [2] This match is not as completely implemented as that in the
> >     ACPI core, so it doesn't check _CID.  So this seems like a
> >     case where duplication of functionality leads to maintenance
> >     problems.  Compare is_root_bridge(), is_memory_device(), and
> >     container_walk_namespace_cb().
> Patch welcome:-)

OK, as soon as I understand better what's going on, I'll work on that :-)

[3] ACPI 3.0 spec, section 6.3.


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: ACPI hot-plug notification help needed

[Keshavamurthy Anil S]

On Tue, Jul 26, 2005 at 01:56:08PM -0600, Bjorn Helgaas wrote:
> > During system boot the ACPI core scans for all the devices 
> > in its root namespace scope i.e by calling acpi_bus_scan() 
> > and for each device whose _STA is present calls acpi_bus_add() 
> > which stores the device in linked list. Later when ACPI driver 
> > registers its IDs the ACPI core just looks in this device list.
> > The core does not keep track of devices whose _STA became present
> > after boot.
> 
> Is this the way you think it *should* work, or just the way it
> happens to work today?  Certainly PCI doesn't work this way --
> the driver may register at boot-time, but when a new device is
> added later via hot-plug, the hot-plug infrastructure arranges
> to call the driver's probe() method.
I am _not_ saying this is how it should work, all I am saying is
this is how it is today. Yes I would love to see ACPI core
behave in the same manner as PCI for Hotplug case i.e I want
the ACPI core to handle all the hotplug notifications and just call
add() method of the corresponding driver when the device gets
added/removed, instead of current way of every driver having to
parse the namespace and having to register notifications for every
device.
> 
> > This is how the ACPI core expects us to register for all the devices
> > for which we care notifications. We want to register for all the devices
> > i.e for the devices which are present and for devices which are not present.
> > To do this we walk the entire namespace scope and check for _HID or _CID
> > match and register a notify handler for this device.
> 
> If we really have to do this, should we change things around so
> an ACPI driver can request that its add() method be called
> for non-present devices as well as present ones?  That would
> at least centralize all the _HID/_CID checking and remove the
> namespace walks from the drivers.
Having to call add() method for non-present device would be a bad idea,
instead of this I prefer to see add() getting called when new device gets
added. Again for this to happen the ACPI core should be changed to take 
care of handling the SCI notifications and checking to see is there a driver 
for this device and if it is present calling drivers add() method. This
will eliminate all the driver having to walk the namespace and register
a notification.

> 
> The current scheme of attaching notify handlers only works if
> every possible device is present in the namespace at the time
Yes you are correct.

> the driver is loaded.  Is there a requirement in the spec for
> this?  I could imagine a hot-add event using LoadTable() to
> add new devices to the namespace before notifying the parent
> bus.  Is there code somewhere in ACPI to re-scan the bus and
> attach drivers?
Very much true, hot-add event using LoadTable() can add new devices
to the namespace and I am not sure whether today's ACPI doing a
re-scan in this case.

> 
> > [Those] drivers which support hotplug are responsible for registering
> > for a notification and based on the notification they have to call
> > acpi_bus_add() to add the device and from then on the acpi_bus_add()
> > function takes care of doing rest of the stuff including calling
> > add() function of the driver.
> 
> This is much different from the PCI philosophy, where new-style
> drivers handle hot-plug seamlessly.  I think ACPI should move in
> that direction.  The current style is clunky and error-prone.
> 
> > > Does ACPI
> > > have to supply nodes for every possible hot-pluggable device in
> > > the namespace at boot-time, just to provide places to hang these
> > > handlers?
> > Yes, the ACPI namespace has to list its max possible configuration
> > in its namespace.
> 
> That seems broken.  Does the spec actually require this somewhere?
> I can understand that some firmware might want to have the max
> possible configuration at boot, but it seems like there ought to
> be the option to do it dynamically.
Sure, you can grow namespace dynamically but the current hotplug won;t work
and needs modifications in that case.
> 
> > > [2] This match is not as completely implemented as that in the
> > >     ACPI core, so it doesn't check _CID.  So this seems like a
> > >     case where duplication of functionality leads to maintenance
> > >     problems.  Compare is_root_bridge(), is_memory_device(), and
> > >     container_walk_namespace_cb().
> > Patch welcome:-)
> 
> OK, as soon as I understand better what's going on, I'll work on that :-)
Since I am busy with some other work at the moment, I can definetly help in
reviewing your code.

Thanks,
-Anil


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: ACPI hot-plug notification help needed

[Bjorn Helgaas]

On Tuesday 26 July 2005 6:05 pm, Keshavamurthy Anil S wrote:
> Having to call add() method for non-present device would be a bad idea,
> instead of this I prefer to see add() getting called when new device gets
> added. Again for this to happen the ACPI core should be changed to take 
> care of handling the SCI notifications and checking to see is there a driver 
> for this device and if it is present calling drivers add() method. This
> will eliminate all the driver having to walk the namespace and register
> a notification.

OK, this sounds like a fruitful area for inquiry.  At least
for memory and container, it looks like the code we run for 
ACPI_NOTIFY_DEVICE_CHECK is pretty generic and very similar.

I don't know very much about how notifiers work.  But I'll
poke around and see if there's a way to have the ACPI core
catch the event when a driver hasn't registered for it.
Then it could do the generic stuff leading up to acpi_bus_add(),
and the driver add() methods could do the rest.


-------------------------------------------------------
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: ACPI hot-plug notification help needed

[Bjorn Helgaas]

On Monday 01 August 2005 6:03 pm, Keshavamurthy Anil S wrote:
> I have patch to trigger
> fake SCI add and remove notifications to test some of this.
> Let me know if you need such patch for your testing.

Yes, I'd be interested in your patch.

Here's a patch (not ready for merging) that makes
acpi_bus_notify() pass the handle, rather than the device,
down to acpi_bus_check_device(), which then adds the device
if it didn't previously exist.

Is this heading down the right track?

Index: work/drivers/acpi/bus.c
===================================================================
--- work.orig/drivers/acpi/bus.c	2005-08-01 13:40:31.000000000 -0600
+++ work/drivers/acpi/bus.c	2005-08-02 15:31:24.000000000 -0600
@@ -383,15 +383,62 @@
    -------------------------------------------------------------------------- */
 
 static int
+acpi_bus_add_device (
+	acpi_handle		handle)
+{
+	struct acpi_device	*child = NULL;
+	struct acpi_device	*parent = NULL;
+	acpi_handle		phandle = NULL;
+	acpi_object_type	type = 0;
+
+	/* TBD: fold this into acpi_bus_add? */
+
+	if (acpi_get_parent(handle, &phandle))
+		return -EINVAL;
+
+	if (acpi_bus_get_device(phandle, &parent))
+		return -EINVAL;
+
+	if (acpi_get_type(handle, &type))
+		return -EINVAL;
+
+	switch (type) {
+	case ACPI_TYPE_DEVICE:
+		type = ACPI_BUS_TYPE_DEVICE;
+		break;
+	case ACPI_TYPE_PROCESSOR:
+		type = ACPI_BUS_TYPE_PROCESSOR;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return acpi_bus_add(&child, parent, handle, type);
+}
+
+static int
 acpi_bus_check_device (
-	struct acpi_device	*device,
+	acpi_handle		handle,
 	int			*status_changed)
 {
 	acpi_status             status = 0;
+	struct acpi_device	*device = NULL;
 	struct acpi_device_status old_status;
+	int			result;
 
 	ACPI_FUNCTION_TRACE("acpi_bus_check_device");
 
+	/*
+	 * If no device exists for this namespace node, a DEVICE_CHECK
+	 * notification means the device has appeared.
+	 */
+	if (acpi_bus_get_device(handle, &device)) {
+		result = acpi_bus_add_device(handle);
+		if (status_changed && result == 0)
+			*status_changed = 1;
+		return_VALUE(result);
+	}
+
 	if (!device)
 		return_VALUE(-EINVAL);
 
@@ -422,15 +469,11 @@
 
 	if (status_changed)
 		*status_changed = 1;
-	
+
 	/*
-	 * Device Insertion/Removal
+	 * Device Removal
 	 */
-	if ((device->status.present) && !(old_status.present)) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device insertion detected\n"));
-		/* TBD: Handle device insertion */
-	}
-	else if (!(device->status.present) && (old_status.present)) {
+	if (!(device->status.present) && (old_status.present)) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device removal detected\n"));
 		/* TBD: Handle device removal */
 	}
@@ -441,18 +484,15 @@
 
 static int
 acpi_bus_check_scope (
-	struct acpi_device	*device)
+	acpi_handle		handle)
 {
 	int			result = 0;
 	int			status_changed = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_bus_check_scope");
 
-	if (!device)
-		return_VALUE(-EINVAL);
-
 	/* Status Change? */
-	result = acpi_bus_check_device(device, &status_changed);
+	result = acpi_bus_check_device(handle, &status_changed);
 	if (result)
 		return_VALUE(result);
 
@@ -480,19 +520,15 @@
 	void                    *data)
 {
 	int			result = 0;
-	struct acpi_device	*device = NULL;
 
 	ACPI_FUNCTION_TRACE("acpi_bus_notify");
 
-	if (acpi_bus_get_device(handle, &device))
-		return_VOID;
-
 	switch (type) {
 
 	case ACPI_NOTIFY_BUS_CHECK:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received BUS CHECK notification for device [%s]\n", 
-			device->pnp.bus_id));
-		result = acpi_bus_check_scope(device);
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received BUS CHECK notification for handle 0x%p\n",
+			handle));
+		result = acpi_bus_check_scope(handle);
 		/* 
 		 * TBD: We'll need to outsource certain events to non-ACPI
 		 *	drivers via the device manager (device.c).
@@ -500,9 +536,9 @@
 		break;
 
 	case ACPI_NOTIFY_DEVICE_CHECK:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE CHECK notification for device [%s]\n", 
-			device->pnp.bus_id));
-		result = acpi_bus_check_device(device, NULL);
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE CHECK notification for handle 0x%p\n",
+			handle));
+		result = acpi_bus_check_device(handle, NULL);
 		/* 
 		 * TBD: We'll need to outsource certain events to non-ACPI
 		 *	drivers via the device manager (device.c).
@@ -510,38 +546,38 @@
 		break;
 
 	case ACPI_NOTIFY_DEVICE_WAKE:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE WAKE notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE WAKE notification for handle 0x%p\n",
+			handle));
 		/* TBD */
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received EJECT REQUEST notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received EJECT REQUEST notification for handle 0x%p\n",
+			handle));
 		/* TBD */
 		break;
 
 	case ACPI_NOTIFY_DEVICE_CHECK_LIGHT:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE CHECK LIGHT notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received DEVICE CHECK LIGHT notification for handle 0x%p\n",
+			handle));
 		/* TBD: Exactly what does 'light' mean? */
 		break;
 
 	case ACPI_NOTIFY_FREQUENCY_MISMATCH:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received FREQUENCY MISMATCH notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received FREQUENCY MISMATCH notification for handle 0x%p\n",
+			handle));
 		/* TBD */
 		break;
 
 	case ACPI_NOTIFY_BUS_MODE_MISMATCH:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received BUS MODE MISMATCH notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received BUS MODE MISMATCH notification for handle 0x%p\n",
+			handle));
 		/* TBD */
 		break;
 
 	case ACPI_NOTIFY_POWER_FAULT:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received POWER FAULT notification for device [%s]\n", 
-			device->pnp.bus_id));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Received POWER FAULT notification for handle 0x%p\n",
+			handle));
 		/* TBD */
 		break;
 


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: Re: ACPI hot-plug notification help needed

[Kenji Kaneshige]

Hi,

>  	ACPI_FUNCTION_TRACE("acpi_bus_check_device");
>  
> +	/*
> +	 * If no device exists for this namespace node, a DEVICE_CHECK
> +	 * notification means the device has appeared.
> +	 */
> +	if (acpi_bus_get_device(handle, &device)) {
> +		result = acpi_bus_add_device(handle);
> +		if (status_changed && result == 0)
> +			*status_changed = 1;
> +		return_VALUE(result);
> +	}
> +
>  	if (!device)

I think acpi_bus_check_device() might be called even though
device's status was not changed. So I think we should check
device's current status (_STA value) before doing this.

Thanks,
Kenji Kaneshige


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: ACPI hot-plug notification help needed

[Keshavamurthy Anil S]

On Tue, Aug 02, 2005 at 04:41:25PM -0600, Bjorn Helgaas wrote:
> Here's a patch (not ready for merging) that makes
> acpi_bus_notify() pass the handle, rather than the device,
> down to acpi_bus_check_device(), which then adds the device
> if it didn't previously exist.
> 
> Is this heading down the right track?
Let me restate the goal of this patch here to make sure
we are know what we are doing.

We are trying to get away with all drivers having to explicitly
install a hotplug notify handler for all of their devices described
in the namespace. With this changes, the ACPI core would then
be responsible for handling this hotplug event, responsible for
creating acpi_device's and calling respective drivers add() method.

Yes, your patch is certainly on the right track but need lot
more carefull though and desing so as not to break existing
features and functionality.

See more comments below.

> 
> +static int
>  acpi_bus_check_device (
> -	struct acpi_device	*device,
> +	acpi_handle		handle,
>  	int			*status_changed)
>  {
>  	acpi_status             status = 0;
> +	struct acpi_device	*device = NULL;
>  	struct acpi_device_status old_status;
> +	int			result;
>  
>  	ACPI_FUNCTION_TRACE("acpi_bus_check_device");
>  
> +	/*
> +	 * If no device exists for this namespace node, a DEVICE_CHECK
> +	 * notification means the device has appeared.
> +	 */
> +	if (acpi_bus_get_device(handle, &device)) {
As Kenji stated in a separate e-mail, you need to check _STA
before calling acpi_bus_add_device().
> +		result = acpi_bus_add_device(handle);
> +		if (status_changed && result == 0)
> +			*status_changed = 1;
> +		return_VALUE(result);
> +	}
> +
Once the devices are added by calling acpi_bus_add_device(),
I think you also need to call acpi_bus_start() to actuall start
the devices.

In case of a root bridge hotplug, after the root bridge
has been successfully added by calling acpi_bus_add_device(),
the root bridge driver (acpiphp.ko see acpiphp_glue.c) needs
to configure the bridge before we call acpi_bus_start(), so
need some intraction with the ACPI core and the drivers
between acpi_bus_add() and acpi_bus_start() calls.

Cheers,
Anil


-------------------------------------------------------
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: ACPI hot-plug notification help needed

[Bjorn Helgaas]

Did you have a patch to fake add/remove notifications?  I'd
be interested in it.

On Wednesday 03 August 2005 1:36 pm, Keshavamurthy Anil S wrote:
> We are trying to get away with all drivers having to explicitly
> install a hotplug notify handler for all of their devices described
> in the namespace. With this changes, the ACPI core would then
> be responsible for handling this hotplug event, responsible for
> creating acpi_device's and calling respective drivers add() method.

Exactly.

> Once the devices are added by calling acpi_bus_add_device(),
> I think you also need to call acpi_bus_start() to actuall start
> the devices.

Yup.  It should work the same way as the initial enumeration
in acpi_scan_init() -- add and start the root, then add and
start any children.

> In case of a root bridge hotplug, after the root bridge
> has been successfully added by calling acpi_bus_add_device(),
> the root bridge driver (acpiphp.ko see acpiphp_glue.c) needs
> to configure the bridge before we call acpi_bus_start(), so
> need some intraction with the ACPI core and the drivers
> between acpi_bus_add() and acpi_bus_start() calls.

I see the acpiphp_configure_bridge() call between the add
and start calls, which I assume is what you mean here.

But rather than adding more interaction between the drivers
and the ACPI core, I think it would be better to put this
configuration in the driver's add() or start() method.
Then you have more consistency between the "present at boot"
case and the "hot-added later" case, at least in terms of
the driver/core interaction.

(I know the driver probably has to handle hot-added bridges
specially because the BIOS treats them differently, but that
seems like something only acpiphp_glue.c needs to know about.)

Sorry for so much talk and so little code :-)  I'll try to
rectify 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: ACPI hot-plug notification help needed

[Keshavamurthy Anil S]

On Wed, Aug 03, 2005 at 02:27:11PM -0600, Bjorn Helgaas wrote:
> Did you have a patch to fake add/remove notifications?  I'd
> be interested in it.
Sent to you in another mail:-)
> 
> I see the acpiphp_configure_bridge() call between the add
> and start calls, which I assume is what you mean here.

Yup,that's correct.

> 
> But rather than adding more interaction between the drivers
> and the ACPI core, I think it would be better to put this
> configuration in the driver's add() or start() method.
> Then you have more consistency between the "present at boot"
> case and the "hot-added later" case, at least in terms of
> the driver/core interaction.

Sure, I agree with your suggested approach. Once we have the
core ACPI changes done, we need to clean up the affected drivers
and all of this has to get comitted at once. This is going to be
a huge efforts in terms of testing:-)

> 
> (I know the driver probably has to handle hot-added bridges
> specially because the BIOS treats them differently, but that
> seems like something only acpiphp_glue.c needs to know about.)

> 
> Sorry for so much talk and so little code :-)  I'll try to
No problem:-)
> rectify that...
Thanks for your interest in cleaning this 
hotplug registration logic.

-Cheers,
Anil



-------------------------------------------------------
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: ACPI hot-plug notification help needed

[Bjorn Helgaas]

On Wednesday 03 August 2005 2:55 pm, Keshavamurthy Anil S wrote:
> Once we have the
> core ACPI changes done, we need to clean up the affected drivers
> and all of this has to get comitted at once. This is going to be
> a huge efforts in terms of testing:-)

Better now than later then, I guess.  As far as I can see, this
affects acpi_memhotplug, container, processor_core, and acpiphp.
Are these widely used yet?  Do distros support them?  These drivers
seem fairly immature, so I'm curious about why the testing should
be so onerous.


-------------------------------------------------------
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: ACPI hot-plug notification help needed

[Keshavamurthy Anil S]

On Wed, Aug 03, 2005 at 03:13:30PM -0600, Bjorn Helgaas wrote:
> On Wednesday 03 August 2005 2:55 pm, Keshavamurthy Anil S wrote:
> > Once we have the
> > core ACPI changes done, we need to clean up the affected drivers
> > and all of this has to get comitted at once. This is going to be
> > a huge efforts in terms of testing:-)
> 
> Better now than later then, I guess.  As far as I can see, this
> affects acpi_memhotplug, container, processor_core, and acpiphp.
> Are these widely used yet?  Do distros support them?  These drivers
> seem fairly immature, so I'm curious about why the testing should
> be so onerous.
Not sure about any distros specifically enabling this stuff but 
some of the code are common for boot path too, hence our changes 
affect lots of system. If we don't have any complains booting then
that is more than 80% coverage and rest 20% is specifically targetting
the hotplug path for CPU, Memory and Rootbridge.

Cheers,
-Anil


-------------------------------------------------------
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: ACPI hot-plug notification help needed

[S, Naveen B]

I agree with Anil's comments. 
See my comments below. 

>>> Can somebody explain how the notification stuff in the ACPI
>>> memory and container drivers works?
>>Sure.
>>>
>>> The usual ACPI driver registers its IDs and add() and remove()
>>> functions with acpi_bus_register_driver(), and the ACPI core
>>> calls the add() function when it finds a device that is present
>>> (based on _STA) and whose _HID or _CID matches.
>>
>>Humm.. I would say that ACPI core calls the add() functions
>>for all the devices which it has seen during its boot( of course
>>based on _STA) and whose _HID or _CID matches.
>>
>>Here is how the current core works.
>>During system boot the ACPI core scans for all the devices
>>in its root namespace scope i.e by calling acpi_bus_scan()
>>and for each device whose _STA is present calls acpi_bus_add()
>>which stores the device in linked list. Later when ACPI driver
>>registers its IDs the ACPI core just looks in this device list.
>>The core does not keep track of devices whose _STA became present
>>after boot.
>>
>>>
>>> The memory and container drivers (acpi_memhotplug.c[1] and
container.c)
>>> also do this.  But in addition, they traverse the entire ACPI
namespace
>>> and attach notify handlers to any devices that match the IDs[2],
even
>>> if the _STA indicates "not present".
>>
>>This is how the ACPI core expects us to register for all the devices
>>for which we care notifications. We want to register for all the
devices
>>i.e for the devices which are present and for devices which are not
>>present.
>>To do this we walk the entire namespace scope and check for _HID or
_CID
>>match and register a notify handler for this device.
>>
>>>
>>> It seems strange to attach notify handlers to devices that are
>>> not present.  Maybe ACPI allows events to be sent to devices that
>>> don't exist, but it just seems weird.  I'd expect a hotplug add to
>>
>>When we were implementing this even for us it looked strange that
>>we have to attach a notify handlers for each and every device. Believe
>>me this is how we had to do.
>>
>>> cause events to some previously-present *parent* of the new device.
>>> The parent's driver could then re-scan its children and add any
>>> that are newly-added or newly-marked "present".
>>
>>This might not alway be true. Some f/w might not want to send
>>notification on the parent as the re-scan might be a costly operation
>>as the parent might contain a large number of children. Imaging a case
>>where we want to attach a device under _SB scope, in case if the
>>notification went to _SB and we have rescan the whole namespace here.
>>
>>>
>>> But these drivers seem to expect hot-add events on the new device
>>> itself, and then they go through the motions of acpi_bus_add()
>>> directly.  This seems like it belongs somewhere in the ACPI core.
>>
>>The core functionality still recides inside ACPI core, it just that
>>these drivers which support hotplug are responsible for registering
>>for a notification and based on the notification they have to call
>>acpi_bus_add() to add the device and from then on the acpi_bus_add()
>>function takes care of doing rest of the stuff including calling
>>add() function of the driver.
>>
>>>
>>> I just noticed that the processor_core and acpiphp_glue drivers
>>> do the same thing.  Does this mean that other ACPI drivers that
>>> want to handle hot-plug will also have to go through this dance
>>> of attaching notify handlers to non-present devices?  Does ACPI
>>Don't you think even acpiphp drivers which support slot hotplug
>>also does the same thing?
>>> have to supply nodes for every possible hot-pluggable device in
>>> the namespace at boot-time, just to provide places to hang these
>>> handlers?
>>Yes, the ACPI namespace has to list its max possible configuration
>>in its namespace.
>>
>>>
>>> Please enlighten me.
>>>
>>> [1] "acpi_memhotplug.c" seems redundant.  Wouldn't "memory.c"
>>>      be just as descriptive?  It's in the ACPI directory, so
>>>      we don't need "acpi_", and ACPI drivers in general can
>>>      support hotplug, so it doesn't seem like we need that either.
>>Naveen, any thoughts on this one?
This was renamed to acpi_memhotplug.c, which is more descriptive, based
on the feedback. We might not need "acpi_" though. This driver is very
much required mainly to field hot plug notifications on memory devices
and to interface with Kernel VM subsystem, which ACPI drivers in general
cannot support.
>>>
>>> [2] This match is not as completely implemented as that in the
>>>     ACPI core, so it doesn't check _CID.  So this seems like a
>>>     case where duplication of functionality leads to maintenance
>>>     problems.  Compare is_root_bridge(), is_memory_device(), and
>>>     container_walk_namespace_cb().
>>Patch welcome:-)
>>
>>-Anil


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_idt77&alloc_id\x16492&op=click