Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751289AbdFEFol (ORCPT ); Mon, 5 Jun 2017 01:44:41 -0400 Received: from smtp.nue.novell.com ([195.135.221.5]:56915 "EHLO smtp.nue.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751241AbdFEFok (ORCPT ); Mon, 5 Jun 2017 01:44:40 -0400 Date: Mon, 5 Jun 2017 13:44:27 +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: <20170605054427.GM30622@linux-l9pv.suse> References: <20170603172036.7645-1-jlee@suse.com> <20170604100453.GK30622@linux-l9pv.suse> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170604100453.GK30622@linux-l9pv.suse> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2488 Lines: 68 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 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