* [RFC PATCH] acpi: indicate to platform when hot remove returns busy
@ 2017-06-03 17:20 Lee, Chun-Yi
2017-06-03 17:37 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Lee, Chun-Yi @ 2017-06-03 17:20 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: linux-acpi, linux-kernel, Lee, Chun-Yi, Len Brown
In hotplug logic, it always indicates non-specific failure to
platform through _OST when handing acpi hot-remove event failed. Then
platform terminates the hot-remove process but it can not identify
the reason.
Base on current hot-remove code, there have two situations that it
returns busy:
- OSPM try to offline an individual device, but the device offline
function returns busy.
- When the ejection event is applied to an "not offlined yet" container.
OSPM send kobject change event to userspace and returns busy.
Both of them will returns -EBUSY to acpi device hotplug function then
hotplug function indicates non-specific failure to platform just like
any other error, e.g. -ENODEV or -EIO.
The benefit to platform for identifying the OS busy state is that
platform can be applied different approach to handle the busy but
not just terminate the hot-remove process by unknow reason. For
example, platform can wait for a while then triggers hot-remove
again.
This RFC patch adds one more parameter to the handler function of
acpi generic hotplug event to give the function a chance to propose
the return code of _OST. In this case, it sets ost return code to
ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
-EBUSY.
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
drivers/acpi/scan.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 2433569..d4aae9f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -374,8 +374,11 @@ static int acpi_scan_bus_check(struct acpi_device *adev)
return 0;
}
-static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
+static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
+ u32 *ost_code)
{
+ int error = -EINVAL;
+
switch (type) {
case ACPI_NOTIFY_BUS_CHECK:
return acpi_scan_bus_check(adev);
@@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
}
acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
- return acpi_scan_hot_remove(adev);
+ error = acpi_scan_hot_remove(adev);
+ if (error == -EBUSY && ost_code)
+ *ost_code = ACPI_OST_SC_DEVICE_BUSY;
}
- return -EINVAL;
+ return error;
}
void acpi_device_hotplug(struct acpi_device *adev, u32 src)
@@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
if (adev->flags.is_dock_station) {
error = dock_notify(adev, src);
} else if (adev->flags.hotplug_notify) {
- error = acpi_generic_hotplug_event(adev, src);
+ error = acpi_generic_hotplug_event(adev, src, &ost_code);
if (error == -EPERM) {
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
goto err_out;
--
2.10.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-03 17:20 [RFC PATCH] acpi: indicate to platform when hot remove returns busy Lee, Chun-Yi
@ 2017-06-03 17:37 ` Andy Shevchenko
2017-06-04 10:04 ` joeyli
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2017-06-03 17:37 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: Rafael J . Wysocki, linux-acpi, linux-kernel, Lee, Chun-Yi, Len Brown
On Sat, Jun 3, 2017 at 8:20 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> In hotplug logic, it always indicates non-specific failure to
> platform through _OST when handing acpi hot-remove event failed. Then
> platform terminates the hot-remove process but it can not identify
> the reason.
>
> Base on current hot-remove code, there have two situations that it
> returns busy:
> - OSPM try to offline an individual device, but the device offline
> function returns busy.
> - When the ejection event is applied to an "not offlined yet" container.
> OSPM send kobject change event to userspace and returns busy.
>
> Both of them will returns -EBUSY to acpi device hotplug function then
> hotplug function indicates non-specific failure to platform just like
> any other error, e.g. -ENODEV or -EIO.
>
> The benefit to platform for identifying the OS busy state is that
> platform can be applied different approach to handle the busy but
> not just terminate the hot-remove process by unknow reason. For
> example, platform can wait for a while then triggers hot-remove
> again.
>
> This RFC patch adds one more parameter to the handler function of
> acpi generic hotplug event to give the function a chance to propose
> the return code of _OST. In this case, it sets ost return code to
> ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
> -EBUSY.
> -static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> +static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
> + u32 *ost_code)
> {
> + int error = -EINVAL;
> +
> switch (type) {
> case ACPI_NOTIFY_BUS_CHECK:
> return acpi_scan_bus_check(adev);
> @@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> }
> acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
> ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> - return acpi_scan_hot_remove(adev);
> + error = acpi_scan_hot_remove(adev);
> + if (error == -EBUSY && ost_code)
> + *ost_code = ACPI_OST_SC_DEVICE_BUSY;
> }
> - return -EINVAL;
> + return error;
> }
Wit this change you spear a logic on two functions...
>
> void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> @@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> if (adev->flags.is_dock_station) {
> error = dock_notify(adev, src);
> } else if (adev->flags.hotplug_notify) {
> - error = acpi_generic_hotplug_event(adev, src);
> + error = acpi_generic_hotplug_event(adev, src, &ost_code);
> if (error == -EPERM) {
> ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> goto err_out;
...instead (since the first one is defined as static) I would propose
to change only here like
switch (error) {
case -EPERM:
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
break;
case -EBUSY:
ost_code = ACPI_OST_SC_DEVICE_BUSY;
break;
}
if (error)
goto err_out;
This is less intrusive and more flexible to modifications in the
future (might be split to a helper, might be easily extended, etc).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-03 17:37 ` Andy Shevchenko
@ 2017-06-04 10:04 ` joeyli
2017-06-04 19:02 ` Andy Shevchenko
2017-06-05 5:44 ` joeyli
0 siblings, 2 replies; 8+ messages in thread
From: joeyli @ 2017-06-04 10:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee, Chun-Yi, Rafael J . Wysocki, linux-acpi, linux-kernel, Len Brown
Hi Andy,
Thanks for your help to review my patch.
On Sat, Jun 03, 2017 at 08:37:51PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 3, 2017 at 8:20 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > In hotplug logic, it always indicates non-specific failure to
> > platform through _OST when handing acpi hot-remove event failed. Then
> > platform terminates the hot-remove process but it can not identify
> > the reason.
> >
> > Base on current hot-remove code, there have two situations that it
> > returns busy:
> > - OSPM try to offline an individual device, but the device offline
> > function returns busy.
> > - When the ejection event is applied to an "not offlined yet" container.
> > OSPM send kobject change event to userspace and returns busy.
> >
> > Both of them will returns -EBUSY to acpi device hotplug function then
> > hotplug function indicates non-specific failure to platform just like
> > any other error, e.g. -ENODEV or -EIO.
> >
> > The benefit to platform for identifying the OS busy state is that
> > platform can be applied different approach to handle the busy but
> > not just terminate the hot-remove process by unknow reason. For
> > example, platform can wait for a while then triggers hot-remove
> > again.
> >
> > This RFC patch adds one more parameter to the handler function of
> > acpi generic hotplug event to give the function a chance to propose
> > the return code of _OST. In this case, it sets ost return code to
> > ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
> > -EBUSY.
>
> > -static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> > +static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
> > + u32 *ost_code)
> > {
> > + int error = -EINVAL;
> > +
> > switch (type) {
> > case ACPI_NOTIFY_BUS_CHECK:
> > return acpi_scan_bus_check(adev);
> > @@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> > }
> > acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
> > ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > - return acpi_scan_hot_remove(adev);
> > + error = acpi_scan_hot_remove(adev);
> > + if (error == -EBUSY && ost_code)
> > + *ost_code = ACPI_OST_SC_DEVICE_BUSY;
> > }
> > - return -EINVAL;
> > + return error;
> > }
>
> Wit this change you spear a logic on two functions...
>
You are right.
I want to give a chance to acpi_generic_hotplug_event()
to propose a _OST code. But acpi_device_hotplug() can
overwrite it. Not good...
> >
> > void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > @@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > if (adev->flags.is_dock_station) {
> > error = dock_notify(adev, src);
> > } else if (adev->flags.hotplug_notify) {
> > - error = acpi_generic_hotplug_event(adev, src);
> > + error = acpi_generic_hotplug_event(adev, src, &ost_code);
> > if (error == -EPERM) {
> > ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > goto err_out;
>
> ...instead (since the first one is defined as static) I would propose
> to change only here like
>
> switch (error) {
> case -EPERM:
> ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> break;
> case -EBUSY:
> ost_code = ACPI_OST_SC_DEVICE_BUSY;
> break;
> }
> if (error)
> goto err_out;
>
> This is less intrusive and more flexible to modifications in the
> future (might be split to a helper, might be easily extended, etc).
>
this RFC patch changed the _OST code for BIOS that it may affects
the behavior of shipped machines. And, I am not sure that the
ACPI_OST_SC_DEVICE_BUSY approach is also useful for other hotplug
event, like ACPI_NOTIFY_BUS_CHECK or ACPI_NOTIFY_DEVICE_CHECK.
So, I prefer to apply this change only on the code path of
ACPI_NOTIFY_EJECT_REQUEST/ACPI_OST_EC_OSPM_EJECT.
Here is my first version, that it just simply put if-else logic:
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 2433569..b105087 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -414,10 +414,14 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
error = dock_notify(adev, src);
} else if (adev->flags.hotplug_notify) {
error = acpi_generic_hotplug_event(adev, src);
- if (error == -EPERM) {
+ if (error == -EPERM)
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
+ else if ((error == -EBUSY) &&
+ (src == ACPI_NOTIFY_EJECT_REQUEST ||
+ src == ACPI_OST_EC_OSPM_EJECT))
+ ost_code = ACPI_OST_SC_DEVICE_BUSY;
+ if (error)
goto err_out;
- }
} else {
int (*notify)(struct acpi_device *, u32);
Because it checks the event source that the logic is duplicate
with the switch code in acpi_generic_hotplug_event(). So I
reuse the switch code in acpi_generic_hotplug_event().
Thanks a lot!
Joey Lee
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-04 10:04 ` joeyli
@ 2017-06-04 19:02 ` Andy Shevchenko
2017-06-05 5:44 ` joeyli
1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2017-06-04 19:02 UTC (permalink / raw)
To: joeyli
Cc: Lee, Chun-Yi, Rafael J . Wysocki, linux-acpi, linux-kernel, Len Brown
On Sun, Jun 4, 2017 at 1:04 PM, joeyli <jlee@suse.com> wrote:
> On Sat, Jun 03, 2017 at 08:37:51PM +0300, Andy Shevchenko wrote:
>> On Sat, Jun 3, 2017 at 8:20 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>> > -static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
>> > +static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
>> > + u32 *ost_code)
>> > {
>> > + int error = -EINVAL;
>> > +
>> > switch (type) {
>> > case ACPI_NOTIFY_BUS_CHECK:
>> > return acpi_scan_bus_check(adev);
>> > @@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
>> > }
>> > acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
>> > ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
>> > - return acpi_scan_hot_remove(adev);
>> > + error = acpi_scan_hot_remove(adev);
>> > + if (error == -EBUSY && ost_code)
>> > + *ost_code = ACPI_OST_SC_DEVICE_BUSY;
>> > }
>> > - return -EINVAL;
>> > + return error;
>> > }
>>
>> Wit this change you spear a logic on two functions...
>>
>
> You are right.
>
> I want to give a chance to acpi_generic_hotplug_event()
> to propose a _OST code. But acpi_device_hotplug() can
> overwrite it. Not good...
...
>> This is less intrusive and more flexible to modifications in the
>> future (might be split to a helper, might be easily extended, etc).
>>
>
> this RFC patch changed the _OST code for BIOS that it may affects
> the behavior of shipped machines. And, I am not sure that the
> ACPI_OST_SC_DEVICE_BUSY approach is also useful for other hotplug
> event, like ACPI_NOTIFY_BUS_CHECK or ACPI_NOTIFY_DEVICE_CHECK.
>
> So, I prefer to apply this change only on the code path of
> ACPI_NOTIFY_EJECT_REQUEST/ACPI_OST_EC_OSPM_EJECT.
>
> Here is my first version, that it just simply put if-else logic:
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 2433569..b105087 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -414,10 +414,14 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> error = dock_notify(adev, src);
> } else if (adev->flags.hotplug_notify) {
> error = acpi_generic_hotplug_event(adev, src);
> - if (error == -EPERM) {
> + if (error == -EPERM)
> ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> + else if ((error == -EBUSY) &&
> + (src == ACPI_NOTIFY_EJECT_REQUEST ||
> + src == ACPI_OST_EC_OSPM_EJECT))
> + ost_code = ACPI_OST_SC_DEVICE_BUSY;
> + if (error)
> goto err_out;
> - }
> } else {
> int (*notify)(struct acpi_device *, u32);
>
> Because it checks the event source that the logic is duplicate
> with the switch code in acpi_generic_hotplug_event(). So I
> reuse the switch code in acpi_generic_hotplug_event().
I see. Then I leave this to Rafael to decide.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-04 10:04 ` joeyli
2017-06-04 19:02 ` Andy Shevchenko
@ 2017-06-05 5:44 ` joeyli
1 sibling, 0 replies; 8+ messages in thread
From: joeyli @ 2017-06-05 5:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee, Chun-Yi, Rafael J . Wysocki, linux-acpi, linux-kernel, Len Brown
On Sun, Jun 04, 2017 at 06:04:53PM +0800, joeyli wrote:
> Hi Andy,
>
> Thanks for your help to review my patch.
>
> On Sat, Jun 03, 2017 at 08:37:51PM +0300, Andy Shevchenko wrote:
> > On Sat, Jun 3, 2017 at 8:20 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > > In hotplug logic, it always indicates non-specific failure to
> > > platform through _OST when handing acpi hot-remove event failed. Then
> > > platform terminates the hot-remove process but it can not identify
> > > the reason.
[...snip]
> > > }
> >
> > Wit this change you spear a logic on two functions...
> >
>
> You are right.
>
> I want to give a chance to acpi_generic_hotplug_event()
> to propose a _OST code. But acpi_device_hotplug() can
> overwrite it. Not good...
>
> > >
> > > void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > > @@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > > if (adev->flags.is_dock_station) {
> > > error = dock_notify(adev, src);
> > > } else if (adev->flags.hotplug_notify) {
> > > - error = acpi_generic_hotplug_event(adev, src);
> > > + error = acpi_generic_hotplug_event(adev, src, &ost_code);
> > > if (error == -EPERM) {
> > > ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > > goto err_out;
> >
> > ...instead (since the first one is defined as static) I would propose
> > to change only here like
> >
> > switch (error) {
> > case -EPERM:
> > ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > break;
> > case -EBUSY:
> > ost_code = ACPI_OST_SC_DEVICE_BUSY;
> > break;
> > }
> > if (error)
> > goto err_out;
> >
> > This is less intrusive and more flexible to modifications in the
> > future (might be split to a helper, might be easily extended, etc).
> >
>
> this RFC patch changed the _OST code for BIOS that it may affects
> the behavior of shipped machines. And, I am not sure that the
> ACPI_OST_SC_DEVICE_BUSY approach is also useful for other hotplug
> event, like ACPI_NOTIFY_BUS_CHECK or ACPI_NOTIFY_DEVICE_CHECK.
>
> So, I prefer to apply this change only on the code path of
> ACPI_NOTIFY_EJECT_REQUEST/ACPI_OST_EC_OSPM_EJECT.
>
Actually I forgot to mention one thing. The ACPI_OST_SC_DEVICE_BUSY ost
code is specific for ejection events, ACPI_NOTIFY_EJECT_REQUEST (0x03) and
ACPI_OST_EC_OSPM_EJECT (0x103). Reference "Table 6-187" in ACPI spec v6.1.
Thanks a lot!
Joey Lee
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-07 8:50 ` Andy Shevchenko
@ 2017-06-07 15:24 ` joeyli
0 siblings, 0 replies; 8+ messages in thread
From: joeyli @ 2017-06-07 15:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee, Chun-Yi, Rafael J . Wysocki, linux-acpi, linux-kernel, Len Brown
On Wed, Jun 07, 2017 at 11:50:13AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 7, 2017 at 9:07 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > In hotplug logic, it always indicates non-specific failure to
> > platform through _OST when handing acpi hot-remove event failed. Then
> > platform terminates the hot-remove process but it can not identify
> > the reason.
> >
> > Base on current hot-remove code, there have two situations that it
> > returns busy:
> > - OSPM try to offline an individual device, but the device offline
> > function returns busy.
> > - When the ejection event is applied to an "not offlined yet" container.
> > OSPM send kobject change event to userspace and returns busy.
> >
> > Both of them will returns -EBUSY to acpi device hotplug function then
> > hotplug function indicates non-specific failure to platform just like
> > any other error, e.g. -ENODEV or -EIO.
> >
> > The benefit to platform for identifying the OS busy state is that
> > platform can be applied different approach to handle the busy but
> > not just terminate the hot-remove process by unknow reason. For
> > example, platform can wait for a while then triggers hot-remove
> > again.
> >
> > This RFC patch adds one more parameter to the handler function of
> > acpi generic hotplug event to give the function a chance to propose
> > the return code of _OST. In this case, it sets ost return code to
> > ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
> > -EBUSY.
>
> > -static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> > +static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
> > + u32 *ost_code)
> > {
> > + int error = -EINVAL;
> > +
> > switch (type) {
> > case ACPI_NOTIFY_BUS_CHECK:
> > return acpi_scan_bus_check(adev);
> > @@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> > }
> > acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
> > ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > - return acpi_scan_hot_remove(adev);
> > + error = acpi_scan_hot_remove(adev);
> > + if (error == -EBUSY && ost_code)
> > + *ost_code = ACPI_OST_SC_DEVICE_BUSY;
> > }
> > - return -EINVAL;
> > + return error;
> > }
> >
> > void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > @@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> > if (adev->flags.is_dock_station) {
> > error = dock_notify(adev, src);
> > } else if (adev->flags.hotplug_notify) {
> > - error = acpi_generic_hotplug_event(adev, src);
> > + error = acpi_generic_hotplug_event(adev, src, &ost_code);
> > if (error == -EPERM) {
> > ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
>
> Looking again to the code I still think you may easily do all stuff
> here in shorter and cleaner manner.
> Do we anticipate that there will be more callers that would like to
> get ost_code for one specific type of event?
> Above intrusion to the acpi_generic_hotplug_event() looks to me like
> non-generic hack.
>
Thanks for your suggestion, I will use switch-case to handle it in
next version.
I checked the ACPI spec and code path of other events:
- For the standard nodification, the possible return value:
- ACPI_NOTIFY_BUS_CHECK: acpi_scan_bus_check() returns 0 or -ENODEV
- ACPI_NOTIFY_DEVICE_CHECK: acpi_scan_device_check() returns 0, -ENODEV or -EALREADY
So, standard notification needs only Success(0) or Non-specific failure(1)
- For docker, currently the dock_notify() only returns 0 or -ENODEV.
But, actually the handle_eject_request() may returns 0 or -EBUSY, but
dock_notify() ignored it.
If there have any machines that it has _OST for dock device, we should
consider to return ACPI_OST_SC_DEVICE_BUSY to dock. Currently I didn't
see benefit on this.
- For additional notify handlers
I only found acpi_pci_root_scan_dependent() that it always returns 0.
- There have ACPI_OST_EC_OSPM_INSERTION(0x200) that OSPM didn't support
It's definded in "Insertion Processing (Source Event: 0x200) Status Codes"
in spec. It will use specific _OST event.
The event types are used by different acpi devices type. And, there have
the insertion event may shows in the future. I will use a switch-case to
handle the change in acpi_generic_hotplug_event().
Thanks a lot!
Joey Lee
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
2017-06-07 6:07 Lee, Chun-Yi
@ 2017-06-07 8:50 ` Andy Shevchenko
2017-06-07 15:24 ` joeyli
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2017-06-07 8:50 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: Rafael J . Wysocki, linux-acpi, linux-kernel, Lee, Chun-Yi, Len Brown
On Wed, Jun 7, 2017 at 9:07 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> In hotplug logic, it always indicates non-specific failure to
> platform through _OST when handing acpi hot-remove event failed. Then
> platform terminates the hot-remove process but it can not identify
> the reason.
>
> Base on current hot-remove code, there have two situations that it
> returns busy:
> - OSPM try to offline an individual device, but the device offline
> function returns busy.
> - When the ejection event is applied to an "not offlined yet" container.
> OSPM send kobject change event to userspace and returns busy.
>
> Both of them will returns -EBUSY to acpi device hotplug function then
> hotplug function indicates non-specific failure to platform just like
> any other error, e.g. -ENODEV or -EIO.
>
> The benefit to platform for identifying the OS busy state is that
> platform can be applied different approach to handle the busy but
> not just terminate the hot-remove process by unknow reason. For
> example, platform can wait for a while then triggers hot-remove
> again.
>
> This RFC patch adds one more parameter to the handler function of
> acpi generic hotplug event to give the function a chance to propose
> the return code of _OST. In this case, it sets ost return code to
> ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
> -EBUSY.
> -static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> +static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
> + u32 *ost_code)
> {
> + int error = -EINVAL;
> +
> switch (type) {
> case ACPI_NOTIFY_BUS_CHECK:
> return acpi_scan_bus_check(adev);
> @@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
> }
> acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
> ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> - return acpi_scan_hot_remove(adev);
> + error = acpi_scan_hot_remove(adev);
> + if (error == -EBUSY && ost_code)
> + *ost_code = ACPI_OST_SC_DEVICE_BUSY;
> }
> - return -EINVAL;
> + return error;
> }
>
> void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> @@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
> if (adev->flags.is_dock_station) {
> error = dock_notify(adev, src);
> } else if (adev->flags.hotplug_notify) {
> - error = acpi_generic_hotplug_event(adev, src);
> + error = acpi_generic_hotplug_event(adev, src, &ost_code);
> if (error == -EPERM) {
> ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
Looking again to the code I still think you may easily do all stuff
here in shorter and cleaner manner.
Do we anticipate that there will be more callers that would like to
get ost_code for one specific type of event?
Above intrusion to the acpi_generic_hotplug_event() looks to me like
non-generic hack.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH] acpi: indicate to platform when hot remove returns busy
@ 2017-06-07 6:07 Lee, Chun-Yi
2017-06-07 8:50 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Lee, Chun-Yi @ 2017-06-07 6:07 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: linux-acpi, linux-kernel, Lee, Chun-Yi, Len Brown
In hotplug logic, it always indicates non-specific failure to
platform through _OST when handing acpi hot-remove event failed. Then
platform terminates the hot-remove process but it can not identify
the reason.
Base on current hot-remove code, there have two situations that it
returns busy:
- OSPM try to offline an individual device, but the device offline
function returns busy.
- When the ejection event is applied to an "not offlined yet" container.
OSPM send kobject change event to userspace and returns busy.
Both of them will returns -EBUSY to acpi device hotplug function then
hotplug function indicates non-specific failure to platform just like
any other error, e.g. -ENODEV or -EIO.
The benefit to platform for identifying the OS busy state is that
platform can be applied different approach to handle the busy but
not just terminate the hot-remove process by unknow reason. For
example, platform can wait for a while then triggers hot-remove
again.
This RFC patch adds one more parameter to the handler function of
acpi generic hotplug event to give the function a chance to propose
the return code of _OST. In this case, it sets ost return code to
ACPI_OST_SC_DEVICE_BUSY when the acpi hot remove function returns
-EBUSY.
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
drivers/acpi/scan.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 2433569..d4aae9f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -374,8 +374,11 @@ static int acpi_scan_bus_check(struct acpi_device *adev)
return 0;
}
-static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
+static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type,
+ u32 *ost_code)
{
+ int error = -EINVAL;
+
switch (type) {
case ACPI_NOTIFY_BUS_CHECK:
return acpi_scan_bus_check(adev);
@@ -389,9 +392,11 @@ static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
}
acpi_evaluate_ost(adev->handle, ACPI_NOTIFY_EJECT_REQUEST,
ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
- return acpi_scan_hot_remove(adev);
+ error = acpi_scan_hot_remove(adev);
+ if (error == -EBUSY && ost_code)
+ *ost_code = ACPI_OST_SC_DEVICE_BUSY;
}
- return -EINVAL;
+ return error;
}
void acpi_device_hotplug(struct acpi_device *adev, u32 src)
@@ -413,7 +418,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
if (adev->flags.is_dock_station) {
error = dock_notify(adev, src);
} else if (adev->flags.hotplug_notify) {
- error = acpi_generic_hotplug_event(adev, src);
+ error = acpi_generic_hotplug_event(adev, src, &ost_code);
if (error == -EPERM) {
ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
goto err_out;
--
2.10.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-07 15:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-03 17:20 [RFC PATCH] acpi: indicate to platform when hot remove returns busy Lee, Chun-Yi
2017-06-03 17:37 ` Andy Shevchenko
2017-06-04 10:04 ` joeyli
2017-06-04 19:02 ` Andy Shevchenko
2017-06-05 5:44 ` joeyli
2017-06-07 6:07 Lee, Chun-Yi
2017-06-07 8:50 ` Andy Shevchenko
2017-06-07 15:24 ` joeyli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).