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

[Andy Shevchenko]

On Sun, Jun 4, 2017 at 1:04 PM, joeyli <jlee@xxxxxxxx> 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@xxxxxxxxx> 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