Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756434AbcJWO51 (ORCPT ); Sun, 23 Oct 2016 10:57:27 -0400 Received: from mailout1.hostsharing.net ([83.223.95.204]:42307 "EHLO mailout1.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755028AbcJWO5Y (ORCPT ); Sun, 23 Oct 2016 10:57:24 -0400 Date: Sun, 23 Oct 2016 16:57:57 +0200 From: Lukas Wunner To: "Bryan O'Donoghue" Cc: Ingo Molnar , x86@kernel.org, Andy Shevchenko , Bjorn Helgaas , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_state hook Message-ID: <20161023145757.GA4909@wunner.de> References: <7c1567d4c49303a4aada94ba16275cbf56b8976b.1477221514.git.lukas@wunner.de> <1477226275.2932.54.camel@nexus-software.ie> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1477226275.2932.54.camel@nexus-software.ie> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5716 Lines: 162 On Sun, Oct 23, 2016 at 01:37:55PM +0100, Bryan O'Donoghue wrote: > On Sun, 2016-10-23 at 13:55 +0200, Lukas Wunner wrote: > > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > > state") augmented struct pci_platform_pm_ops with a ->get_state hook > > and > > implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops > > existing till v4.7. > > > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: > > Add > > Power Management Unit driver").??It is missing the ->get_state hook, > > which is fatal since pci_set_platform_pm() enforces its > > presence.??Andy > > Shevchenko reports that without the present commit, such a device > > "crashes without even a character printed out on serial console and > > reboots (since watchdog)". > > > > Retrofit mid_pci_platform_pm with the missing callback to fix the > > breakage. > > > > Fixes: cc7cc02bada8 ("PCI: Query platform firmware for device power > > state") > > Cc: x86@kernel.org > > Signed-off-by: Lukas Wunner > > Acked-and-tested-by: Andy Shevchenko > om> > > Acked-by: Bjorn Helgaas > > --- > > Changes v1 -> v2: > > - Cast return value of intel_mid_pci_get_power_state() to > > ? (__force pci_power_t) to avoid "sparse -D__CHECK_ENDIAN__" warning. > > - Add ack by Andy Shevchenko. > > > > Changes v2 -> v3: > > - Amend commit message to explain the user-visible failure mode as > > ? reported by Andy. > > - Add ack by Bjorn Helgaas and Fixes tag. > > > > ?arch/x86/include/asm/intel-mid.h??|??1 + > > ?arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++ > > ?drivers/pci/pci-mid.c?????????????|??6 ++++++ > > ?3 files changed, 26 insertions(+) > > > > diff --git a/arch/x86/include/asm/intel-mid.h > > b/arch/x86/include/asm/intel-mid.h > > index 5b6753d..49da9f4 100644 > > --- a/arch/x86/include/asm/intel-mid.h > > +++ b/arch/x86/include/asm/intel-mid.h > > @@ -17,6 +17,7 @@ > > ? > > ?extern int intel_mid_pci_init(void); > > ?extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, > > pci_power_t state); > > +extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev > > *pdev); > > ? > > ?extern void intel_mid_pwr_power_off(void); > > ? > > diff --git a/arch/x86/platform/intel-mid/pwr.c > > b/arch/x86/platform/intel-mid/pwr.c > > index 5d3b45a..67375dd 100644 > > --- a/arch/x86/platform/intel-mid/pwr.c > > +++ b/arch/x86/platform/intel-mid/pwr.c > > @@ -272,6 +272,25 @@ int intel_mid_pci_set_power_state(struct pci_dev > > *pdev, pci_power_t state) > > ?} > > ?EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state); > > ? > > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev) > > +{ > > + struct mid_pwr *pwr = midpwr; > > + int id, reg, bit; > > + u32 power; > > + > > + if (!pwr || !pwr->available) > > + return PCI_UNKNOWN; > > + > > + id = intel_mid_pwr_get_lss_id(pdev); > > + if (id < 0) > > + return PCI_UNKNOWN; > > + > > + reg = (id * LSS_PWS_BITS) / 32; > > + bit = (id * LSS_PWS_BITS) % 32; > > + power = mid_pwr_get_state(pwr, reg); > > + return (__force pci_power_t)((power >> bit) & 3); > > +} > > + > > ?void intel_mid_pwr_power_off(void) > > ?{ > > ? struct mid_pwr *pwr = midpwr; > > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c > > index 55f453d..c7f3408 100644 > > --- a/drivers/pci/pci-mid.c > > +++ b/drivers/pci/pci-mid.c > > @@ -29,6 +29,11 @@ static int mid_pci_set_power_state(struct pci_dev > > *pdev, pci_power_t state) > > ? return intel_mid_pci_set_power_state(pdev, state); > > ?} > > ? > > +static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) > > +{ > > + return intel_mid_pci_get_power_state(pdev); > > +} > > + > > ?static pci_power_t mid_pci_choose_state(struct pci_dev *pdev) > > ?{ > > ? return PCI_D3hot; > > @@ -52,6 +57,7 @@ static bool mid_pci_need_resume(struct pci_dev > > *dev) > > ?static struct pci_platform_pm_ops mid_pci_platform_pm = { > > ? .is_manageable = mid_pci_power_manageable, > > ? .set_state = mid_pci_set_power_state, > > + .get_state = mid_pci_get_power_state, > > ? .choose_state = mid_pci_choose_state, > > ? .sleep_wake = mid_pci_sleep_wake, > > ? .run_wake = mid_pci_run_wake, > > Shouldn't this serialize like this > > ? ? ? might_sleep(); > > reg = (id * LSS_PWS_BITS) / 32; > bit = (id * LSS_PWS_BITS) % 32; > > ? ? ? mutex_lock(&pwr->lock); > power = mid_pwr_get_state(pwr, reg); > ? ? ? mutex_lock(&pwr->lock); > > return (__force pci_power_t)((power >> bit) & 3); > > there's a corresponding flow in mid_pwr_set_power_state() that operates > in exactly that way. mid_pwr_set_power_state() uses a series of steps (set the power state, wait for completion) so presumably Andy thought this needs to be done under a lock to prevent concurrent execution. mid_pwr_get_state() on the other hand is just a register read, which I assume is atomic. The other stuff (calling intel_mid_pwr_get_lss_id(), calculation of reg and bit) seems to be static, it never changes across invocations. Hence there doesn't seem to be a necessity to acquire the mutex and call might_sleep(). That said I'm not really familiar with these devices and rely on Andy's ack for correctness. Andy if I'm mistaken please shout, otherwise I assume the patch is correct. The usage of a mutex in mid_pwr_set_power_state() actually seems questionable since this is called with interrupts disabled: pci_pm_resume_noirq pci_pm_default_resume_early pci_power_up platform_pci_set_power_state mid_pci_set_power_state intel_mid_pci_set_power_state mid_pwr_set_power_state Thanks, Lukas