[PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
entire namespace starting from the given handle even if the device
represented by that handle is present (other devices below it may
just have been added).

For this reason, modify acpi_scan_bus_device_check() to always run
acpi_bus_scan() if the notification being handled is of type
ACPI_NOTIFY_BUS_CHECK.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.10+ <stable@vger.kernel.org>
---
 drivers/acpi/scan.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -353,10 +353,12 @@ static void acpi_scan_bus_device_check(a
 	mutex_lock(&acpi_scan_lock);
 	lock_device_hotplug();
 
-	acpi_bus_get_device(handle, &device);
-	if (device) {
-		dev_warn(&device->dev, "Attempt to re-insert\n");
-		goto out;
+	if (ost_source != ACPI_NOTIFY_BUS_CHECK) {
+		acpi_bus_get_device(handle, &device);
+		if (device) {
+			dev_warn(&device->dev, "Attempt to re-insert\n");
+			goto out;
+		}
 	}
 	acpi_evaluate_hotplug_ost(handle, ost_source,
 				  ACPI_OST_SC_INSERT_IN_PROGRESS, NULL);

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Toshi Kani]

On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> entire namespace starting from the given handle even if the device
> represented by that handle is present (other devices below it may
> just have been added).
> 
> For this reason, modify acpi_scan_bus_device_check() to always run
> acpi_bus_scan() if the notification being handled is of type
> ACPI_NOTIFY_BUS_CHECK.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: 3.10+ <stable@vger.kernel.org>

Acked-by: Toshi Kani <toshi.kani@hp.com>

But, I think we need the additional patch below.

Thanks,
-Toshi

=====
From: Toshi Kani <toshi.kani@hp.com>
Subject: [PATCH] ACPI: Do not call attach() if device is attached

attach() of ACPI scan handlers does not expect to be called multiple
times on a same device.  Also, the attached handler may not be changed
without calling its detach().  Change acpi_scan_attach_handler() not
to call attach() when the given device is already attached.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/scan.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 20757e0..2b9e867 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1885,6 +1885,9 @@ static int acpi_scan_attach_handler(struct
acpi_device *device)
 	struct acpi_hardware_id *hwid;
 	int ret = 0;
 
+	if (device->handler)
+		return 1;
+
 	list_for_each_entry(hwid, &device->pnp.ids, list) {
 		const struct acpi_device_id *devid;
 		struct acpi_scan_handler *handler;

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Rafael J. Wysocki]

On Tuesday, July 09, 2013 01:32:42 PM Toshi Kani wrote:
> On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> > entire namespace starting from the given handle even if the device
> > represented by that handle is present (other devices below it may
> > just have been added).
> > 
> > For this reason, modify acpi_scan_bus_device_check() to always run
> > acpi_bus_scan() if the notification being handled is of type
> > ACPI_NOTIFY_BUS_CHECK.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: 3.10+ <stable@vger.kernel.org>
> 
> Acked-by: Toshi Kani <toshi.kani@hp.com>
> 
> But, I think we need the additional patch below.

Yes, I think you're right.

Thanks,
Rafael



> =====
> From: Toshi Kani <toshi.kani@hp.com>
> Subject: [PATCH] ACPI: Do not call attach() if device is attached
> 
> attach() of ACPI scan handlers does not expect to be called multiple
> times on a same device.  Also, the attached handler may not be changed
> without calling its detach().  Change acpi_scan_attach_handler() not
> to call attach() when the given device is already attached.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/scan.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 20757e0..2b9e867 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1885,6 +1885,9 @@ static int acpi_scan_attach_handler(struct
> acpi_device *device)
>  	struct acpi_hardware_id *hwid;
>  	int ret = 0;
>  
> +	if (device->handler)
> +		return 1;
> +
>  	list_for_each_entry(hwid, &device->pnp.ids, list) {
>  		const struct acpi_device_id *devid;
>  		struct acpi_scan_handler *handler;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Rafael J. Wysocki]

On Wednesday, July 10, 2013 02:11:05 AM Rafael J. Wysocki wrote:
> On Tuesday, July 09, 2013 01:32:42 PM Toshi Kani wrote:
> > On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> > > entire namespace starting from the given handle even if the device
> > > represented by that handle is present (other devices below it may
> > > just have been added).
> > > 
> > > For this reason, modify acpi_scan_bus_device_check() to always run
> > > acpi_bus_scan() if the notification being handled is of type
> > > ACPI_NOTIFY_BUS_CHECK.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: 3.10+ <stable@vger.kernel.org>
> > 
> > Acked-by: Toshi Kani <toshi.kani@hp.com>
> > 
> > But, I think we need the additional patch below.
> 
> Yes, I think you're right.

That said I'd prefer to put the check into acpi_bus_device_attach() like in
the appended patch.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / scan: Do not try to attach scan handlers to devices having them

In acpi_bus_device_attach(), if there is an ACPI device object
for the given handle and that device object has a scan handler
attached to it already, there's nothing more to do for that handle
and the function should just return success immediately.  Make
that happen.

Reported-by: Toshi Kani <toshi.kani@hp.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1984,6 +1984,9 @@ static acpi_status acpi_bus_device_attac
 	if (acpi_bus_get_device(handle, &device))
 		return AE_CTRL_DEPTH;
 
+	if (device->handler)
+		return AE_OK;
+
 	ret = acpi_scan_attach_handler(device);
 	if (ret)
 		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Toshi Kani]

On Thu, 2013-07-11 at 00:45 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 10, 2013 02:11:05 AM Rafael J. Wysocki wrote:
> > On Tuesday, July 09, 2013 01:32:42 PM Toshi Kani wrote:
> > > On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> > > > entire namespace starting from the given handle even if the device
> > > > represented by that handle is present (other devices below it may
> > > > just have been added).
> > > > 
> > > > For this reason, modify acpi_scan_bus_device_check() to always run
> > > > acpi_bus_scan() if the notification being handled is of type
> > > > ACPI_NOTIFY_BUS_CHECK.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Cc: 3.10+ <stable@vger.kernel.org>
> > > 
> > > Acked-by: Toshi Kani <toshi.kani@hp.com>
> > > 
> > > But, I think we need the additional patch below.
> > 
> > Yes, I think you're right.
> 
> That said I'd prefer to put the check into acpi_bus_device_attach() like in
> the appended patch.

That's fine by me.  

Acked-by: Toshi Kani <toshi.kani@hp.com>

Just a minor point, though.  Isn't it a bit inconsistent with
device_attach(), which checks dev->driver inside the function?  That
said, I am OK with either way.

Thanks,
-Toshi


> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / scan: Do not try to attach scan handlers to devices having them
> 
> In acpi_bus_device_attach(), if there is an ACPI device object
> for the given handle and that device object has a scan handler
> attached to it already, there's nothing more to do for that handle
> and the function should just return success immediately.  Make
> that happen.
> 
> Reported-by: Toshi Kani <toshi.kani@hp.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1984,6 +1984,9 @@ static acpi_status acpi_bus_device_attac
>  	if (acpi_bus_get_device(handle, &device))
>  		return AE_CTRL_DEPTH;
>  
> +	if (device->handler)
> +		return AE_OK;
> +
>  	ret = acpi_scan_attach_handler(device);
>  	if (ret)
>  		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Rafael J. Wysocki]

On Wednesday, July 10, 2013 05:48:26 PM Toshi Kani wrote:
> On Thu, 2013-07-11 at 00:45 +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 10, 2013 02:11:05 AM Rafael J. Wysocki wrote:
> > > On Tuesday, July 09, 2013 01:32:42 PM Toshi Kani wrote:
> > > > On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> > > > > entire namespace starting from the given handle even if the device
> > > > > represented by that handle is present (other devices below it may
> > > > > just have been added).
> > > > > 
> > > > > For this reason, modify acpi_scan_bus_device_check() to always run
> > > > > acpi_bus_scan() if the notification being handled is of type
> > > > > ACPI_NOTIFY_BUS_CHECK.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Cc: 3.10+ <stable@vger.kernel.org>
> > > > 
> > > > Acked-by: Toshi Kani <toshi.kani@hp.com>
> > > > 
> > > > But, I think we need the additional patch below.
> > > 
> > > Yes, I think you're right.
> > 
> > That said I'd prefer to put the check into acpi_bus_device_attach() like in
> > the appended patch.
> 
> That's fine by me.  
> 
> Acked-by: Toshi Kani <toshi.kani@hp.com>
> 
> Just a minor point, though.  Isn't it a bit inconsistent with
> device_attach(), which checks dev->driver inside the function?

Well, device_attach() may be called from different places while this is
the only place where acpi_scan_attach_handler() is called.

The check in acpi_bus_device_attach() is easier to follow to me, because
it clearly means "we don't need to do anything more if there's a handler",
while the check in acpi_scan_attach_handler() makes you wonder "why do we
need to return 1 in that case?" and then you need to go to the caller and
look at the check of the return value to see "ah, because we don't want
that device_attach() to be called then!".

> That said, I am OK with either way.

Cool. :-)

Thanks,
Rafael


> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: ACPI / scan: Do not try to attach scan handlers to devices having them
> > 
> > In acpi_bus_device_attach(), if there is an ACPI device object
> > for the given handle and that device object has a scan handler
> > attached to it already, there's nothing more to do for that handle
> > and the function should just return success immediately.  Make
> > that happen.
> > 
> > Reported-by: Toshi Kani <toshi.kani@hp.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/scan.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -1984,6 +1984,9 @@ static acpi_status acpi_bus_device_attac
> >  	if (acpi_bus_get_device(handle, &device))
> >  		return AE_CTRL_DEPTH;
> >  
> > +	if (device->handler)
> > +		return AE_OK;
> > +
> >  	ret = acpi_scan_attach_handler(device);
> >  	if (ret)
> >  		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / scan: Always call acpi_bus_scan() for bus check notifications

[Toshi Kani]

On Thu, 2013-07-11 at 02:39 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 10, 2013 05:48:26 PM Toshi Kani wrote:
> > On Thu, 2013-07-11 at 00:45 +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 10, 2013 02:11:05 AM Rafael J. Wysocki wrote:
> > > > On Tuesday, July 09, 2013 01:32:42 PM Toshi Kani wrote:
> > > > > On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > An ACPI_NOTIFY_BUS_CHECK notification means that we should scan the
> > > > > > entire namespace starting from the given handle even if the device
> > > > > > represented by that handle is present (other devices below it may
> > > > > > just have been added).
> > > > > > 
> > > > > > For this reason, modify acpi_scan_bus_device_check() to always run
> > > > > > acpi_bus_scan() if the notification being handled is of type
> > > > > > ACPI_NOTIFY_BUS_CHECK.
> > > > > > 
> > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > Cc: 3.10+ <stable@vger.kernel.org>
> > > > > 
> > > > > Acked-by: Toshi Kani <toshi.kani@hp.com>
> > > > > 
> > > > > But, I think we need the additional patch below.
> > > > 
> > > > Yes, I think you're right.
> > > 
> > > That said I'd prefer to put the check into acpi_bus_device_attach() like in
> > > the appended patch.
> > 
> > That's fine by me.  
> > 
> > Acked-by: Toshi Kani <toshi.kani@hp.com>
> > 
> > Just a minor point, though.  Isn't it a bit inconsistent with
> > device_attach(), which checks dev->driver inside the function?
> 
> Well, device_attach() may be called from different places while this is
> the only place where acpi_scan_attach_handler() is called.
> 
> The check in acpi_bus_device_attach() is easier to follow to me, because
> it clearly means "we don't need to do anything more if there's a handler",
> while the check in acpi_scan_attach_handler() makes you wonder "why do we
> need to return 1 in that case?" and then you need to go to the caller and
> look at the check of the return value to see "ah, because we don't want
> that device_attach() to be called then!".

Sounds good to me.

Thanks,
-Toshi


> 
> > That said, I am OK with either way.
> 
> Cool. :-)
> 
> Thanks,
> Rafael
> 
> 
> > > ---
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Subject: ACPI / scan: Do not try to attach scan handlers to devices having them
> > > 
> > > In acpi_bus_device_attach(), if there is an ACPI device object
> > > for the given handle and that device object has a scan handler
> > > attached to it already, there's nothing more to do for that handle
> > > and the function should just return success immediately.  Make
> > > that happen.
> > > 
> > > Reported-by: Toshi Kani <toshi.kani@hp.com>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/scan.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > Index: linux-pm/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/scan.c
> > > +++ linux-pm/drivers/acpi/scan.c
> > > @@ -1984,6 +1984,9 @@ static acpi_status acpi_bus_device_attac
> > >  	if (acpi_bus_get_device(handle, &device))
> > >  		return AE_CTRL_DEPTH;
> > >  
> > > +	if (device->handler)
> > > +		return AE_OK;
> > > +
> > >  	ret = acpi_scan_attach_handler(device);
> > >  	if (ret)
> > >  		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html