Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656869 > unrolled thread

[RFC PATCH] acpi: indicate to platform when hot remove returns busy

Started by"Lee, Chun-Yi" <joeyli.kernel@gmail.com>
First post2017-06-03 19:30 +0200
Last post2017-06-05 07:50 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] acpi: indicate to platform when hot remove returns busy "Lee, Chun-Yi" <joeyli.kernel@gmail.com> - 2017-06-03 19:30 +0200
    Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-03 19:40 +0200
      Re: [RFC PATCH] acpi: indicate to platform when hot remove returns  busy joeyli <jlee@suse.com> - 2017-06-04 12:10 +0200
        Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-04 21:10 +0200
        Re: [RFC PATCH] acpi: indicate to platform when hot remove returns  busy joeyli <jlee@suse.com> - 2017-06-05 07:50 +0200

#1656869 — [RFC PATCH] acpi: indicate to platform when hot remove returns busy

From"Lee, Chun-Yi" <joeyli.kernel@gmail.com>
Date2017-06-03 19:30 +0200
Subject[RFC PATCH] acpi: indicate to platform when hot remove returns busy
Message-ID<tOl29-1qH-11@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1656873

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-03 19:40 +0200
Message-ID<tOlbQ-1u4-17@gated-at.bofh.it>
In reply to#1656869
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

[toc] | [prev] | [next] | [standalone]

#1657042 — Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy

Fromjoeyli <jlee@suse.com>
Date2017-06-04 12:10 +0200
SubjectRe: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
Message-ID<tOADU-3sO-15@gated-at.bofh.it>
In reply to#1656873
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

[toc] | [prev] | [next] | [standalone]

#1657139

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-04 21:10 +0200
Message-ID<tOJ4u-RW-5@gated-at.bofh.it>
In reply to#1657042
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

[toc] | [prev] | [next] | [standalone]

#1657284 — Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy

Fromjoeyli <jlee@suse.com>
Date2017-06-05 07:50 +0200
SubjectRe: [RFC PATCH] acpi: indicate to platform when hot remove returns busy
Message-ID<tOT3P-7eE-1@gated-at.bofh.it>
In reply to#1657042
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web