From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Cc: 3.10+ <[email protected]>
---
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);
On Mon, 2013-07-08 at 02:10 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Cc: 3.10+ <[email protected]>
Acked-by: Toshi Kani <[email protected]>
But, I think we need the additional patch below.
Thanks,
-Toshi
=====
From: Toshi Kani <[email protected]>
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 <[email protected]>
---
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;
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: 3.10+ <[email protected]>
>
> Acked-by: Toshi Kani <[email protected]>
>
> But, I think we need the additional patch below.
Yes, I think you're right.
Thanks,
Rafael
> =====
> From: Toshi Kani <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
> 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.
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Cc: 3.10+ <[email protected]>
> >
> > Acked-by: Toshi Kani <[email protected]>
> >
> > 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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;
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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Cc: 3.10+ <[email protected]>
> > >
> > > Acked-by: Toshi Kani <[email protected]>
> > >
> > > 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 <[email protected]>
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Cc: 3.10+ <[email protected]>
> > > >
> > > > Acked-by: Toshi Kani <[email protected]>
> > > >
> > > > 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 <[email protected]>
>
> 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 <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > 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 [email protected]
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
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 <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > Cc: 3.10+ <[email protected]>
> > > > >
> > > > > Acked-by: Toshi Kani <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> >
> > 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 <[email protected]>
> > > 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 <[email protected]>
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > 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 [email protected]
> > > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html