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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
