From patchwork Sun Jun 4 10:04:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: joeyli X-Patchwork-Id: 9764749 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E8BF4601D7 for ; Sun, 4 Jun 2017 10:05:14 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CB8BE28174 for ; Sun, 4 Jun 2017 10:05:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id ADA67283CE; Sun, 4 Jun 2017 10:05:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 304B428174 for ; Sun, 4 Jun 2017 10:05:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751027AbdFDKFM (ORCPT ); Sun, 4 Jun 2017 06:05:12 -0400 Received: from smtp.nue.novell.com ([195.135.221.5]:51336 "EHLO smtp.nue.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750847AbdFDKFM (ORCPT ); Sun, 4 Jun 2017 06:05:12 -0400 Received: from linux-l9pv.suse (124-11-22-254.static.tfn.net.tw [124.11.22.254]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Sun, 04 Jun 2017 12:05:07 +0200 Date: Sun, 4 Jun 2017 18:04:53 +0800 From: joeyli To: Andy Shevchenko Cc: "Lee, Chun-Yi" , "Rafael J . Wysocki" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Len Brown Subject: Re: [RFC PATCH] acpi: indicate to platform when hot remove returns busy Message-ID: <20170604100453.GK30622@linux-l9pv.suse> References: <20170603172036.7645-1-jlee@suse.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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: 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 --- 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 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);