| Message ID | 20170607060527.23407-1-jlee@suse.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Jun 7, 2017 at 9:05 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > Kernel should decrements the reference count of acpi device > when scheduling acpi hotplug work is failed, and also evaluates > _OST to notify BIOS the failure. > - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > - return; > + if (hotplug_event) { > + if (ACPI_FAILURE(acpi_hotplug_schedule(adev, type))) { > + acpi_bus_put_acpi_device(adev); > + goto err; > + } else { > + return; > + } > + } Wouldn't be simpler to - return; + goto err_put_device; + err_put_device: + acpi_bus_put_acpi_device(adev); err:
Hi Andy, Thanks for your help to review my patch. On Wed, Jun 07, 2017 at 11:36:55AM +0300, Andy Shevchenko wrote: > On Wed, Jun 7, 2017 at 9:05 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > Kernel should decrements the reference count of acpi device > > when scheduling acpi hotplug work is failed, and also evaluates > > _OST to notify BIOS the failure. > > > - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > > - return; A note here... When the acpi hotplug job is scheduled success, the adev device can not be put because acpi_device_hotplug() will put it until hotplug routine finished. drivers/acpi/bus.c acpi_bus_notify() acpi_bus_get_acpi_device(handle) //get here drivers/acpi/osl.c acpi_hotplug_schedule() drivers/acpi/osl.c acpi_hotplug_work_fn() drivers/acpi/scan.c acpi_device_hotplug() acpi_bus_put_acpi_device(adev) //put here > > + if (hotplug_event) { > > + if (ACPI_FAILURE(acpi_hotplug_schedule(adev, type))) { > > + acpi_bus_put_acpi_device(adev); > > + goto err; > > + } else { > > + return; > > + } > > + } > > Wouldn't be simpler to > > - return; > + goto err_put_device; > > + err_put_device: > + acpi_bus_put_acpi_device(adev); > err: > So, do you mean like this? - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) - return; + if (hotplug_event) { + if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) + return; + else + goto err_put_device; + } acpi_bus_put_acpi_device(adev); return; +err_put_device: + acpi_bus_put_acpi_device(adev); err: acpi_evaluate_ost(handle, type, ost_code, NULL); } Thanks for your suggestion, it looks simpler. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 7, 2017 at 1:18 PM, joeyli <jlee@suse.com> wrote: > On Wed, Jun 07, 2017 at 11:36:55AM +0300, Andy Shevchenko wrote: >> On Wed, Jun 7, 2017 at 9:05 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: >> > Kernel should decrements the reference count of acpi device >> > when scheduling acpi hotplug work is failed, and also evaluates >> > _OST to notify BIOS the failure. > So, do you mean like this? Yes, see below. > > - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > - return; > + if (hotplug_event) { > + if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > + return; > + else It's redundant... > + goto err_put_device; ...perhaps if (ACPI_FAILURE(acpi_hotplug_schedule(adev, type))) goto err_put_device; return; > + } > > acpi_bus_put_acpi_device(adev); > return; > > +err_put_device: > + acpi_bus_put_acpi_device(adev); > err: > acpi_evaluate_ost(handle, type, ost_code, NULL); > }
On Wed, Jun 07, 2017 at 01:46:37PM +0300, Andy Shevchenko wrote: > On Wed, Jun 7, 2017 at 1:18 PM, joeyli <jlee@suse.com> wrote: > > On Wed, Jun 07, 2017 at 11:36:55AM +0300, Andy Shevchenko wrote: > >> On Wed, Jun 7, 2017 at 9:05 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > >> > Kernel should decrements the reference count of acpi device > >> > when scheduling acpi hotplug work is failed, and also evaluates > >> > _OST to notify BIOS the failure. > > > So, do you mean like this? > > Yes, see below. > > > > > - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > > - return; > > + if (hotplug_event) { > > > + if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > > + return; > > > + else > > It's redundant... > Oh~ Yes, you are right. The 'else' can be removed > > + goto err_put_device; > > ...perhaps > > if (ACPI_FAILURE(acpi_hotplug_schedule(adev, type))) > goto err_put_device; > return; > I think normally it should be success. So how about: if (hotplug_event) { if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) return; goto err_put_device; } > > > > acpi_bus_put_acpi_device(adev); > > return; > > > > +err_put_device: > > + acpi_bus_put_acpi_device(adev); > > err: > > acpi_evaluate_ost(handle, type, ost_code, NULL); > > } Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 7, 2017 at 6:39 PM, joeyli <jlee@suse.com> wrote: > On Wed, Jun 07, 2017 at 01:46:37PM +0300, Andy Shevchenko wrote: >> On Wed, Jun 7, 2017 at 1:18 PM, joeyli <jlee@suse.com> wrote: >> > On Wed, Jun 07, 2017 at 11:36:55AM +0300, Andy Shevchenko wrote: >> >> On Wed, Jun 7, 2017 at 9:05 AM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > I think normally it should be success. So how about: > > if (hotplug_event) { > if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) > return; > goto err_put_device; > } Fine by me.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 34fbe02..2f2cec9 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -427,8 +427,14 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data) (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS)) driver->ops.notify(adev, type); - if (hotplug_event && ACPI_SUCCESS(acpi_hotplug_schedule(adev, type))) - return; + if (hotplug_event) { + if (ACPI_FAILURE(acpi_hotplug_schedule(adev, type))) { + acpi_bus_put_acpi_device(adev); + goto err; + } else { + return; + } + } acpi_bus_put_acpi_device(adev); return;
Kernel should decrements the reference count of acpi device when scheduling acpi hotplug work is failed, and also evaluates _OST to notify BIOS the failure. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> --- drivers/acpi/bus.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)