From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3C40CC5B57A for ; Sat, 29 Jun 2019 11:34:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1E34820828 for ; Sat, 29 Jun 2019 11:34:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727059AbfF2Leu (ORCPT ); Sat, 29 Jun 2019 07:34:50 -0400 Received: from mail-ed1-f67.google.com ([209.85.208.67]:44776 "EHLO mail-ed1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727022AbfF2Let (ORCPT ); Sat, 29 Jun 2019 07:34:49 -0400 Received: by mail-ed1-f67.google.com with SMTP id k8so15359676edr.11 for ; Sat, 29 Jun 2019 04:34:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bOVwKJVcD24JyJ3gwPHu19lm5eeEw58EUJsKySuLKtc=; b=cfelGnDuD1dDgfcrri0RlD8ajzLzBavwfyfICRcKWO9Ubwu0FR1YDZYFIifJDf1piL ypE5zyOMWmvGISm3vHVzBACojGW/YLvGstF1et5Jutpm8PR4DECTYEe8zBkibX15M14U iatljM9N2mTJ9/RFKlhxIINyA/sXAs+OXXjQ41o86Y6X6c4g2Cc2DnLuX7NojMUq6Khg cb8pEiRuNWija2FAWFg8B6WnTMaPJeDb4Ij32N+KIvRmRMMV61VMUwh759L9T66n8RfE PtM2NlVteYasjGiv1Q8AqxgPT8YcIfsvGn2eQViwZG76J1QJvG9Q7MFK7ZaFYwr/9f1F EouA== X-Gm-Message-State: APjAAAXRFXagEcT7P49jAILMiV4tnfrwNZEJFvdIINQDVc8CYWXDwg70 vjCemYLDYy+exYaenBMPum5R7g== X-Google-Smtp-Source: APXvYqxAPfMcjNqUpnxVQkI+3WN7lHsXqQ9kgqPzyKlYbM+MO2sBMXGK/SCcixo6G7Rn8btmkGKbBg== X-Received: by 2002:a50:86dc:: with SMTP id 28mr17140260edu.132.1561808087580; Sat, 29 Jun 2019 04:34:47 -0700 (PDT) Received: from shalem.localdomain (84-106-84-65.cable.dynamic.v4.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id e3sm284872ejl.29.2019.06.29.04.34.46 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Sat, 29 Jun 2019 04:34:46 -0700 (PDT) Subject: Re: [PATCH 4/6] ACPI: LPSS: Fix ->suspend_late callbacks handling To: "Rafael J. Wysocki" , Linux PM Cc: Linux PCI , Linux ACPI , LKML , Bjorn Helgaas , Andy Shevchenko , Mika Westerberg , "Robert R. Howell" References: <2318839.0szTqvJMZa@kreacher> <2981101.tKVHzisTAg@kreacher> From: Hans de Goede Message-ID: Date: Sat, 29 Jun 2019 13:34:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <2981101.tKVHzisTAg@kreacher> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rafael, On 29-06-19 11:50, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > If the resume_from_noirq flag is set in dev_desc, the ->suspend_late > callback provided by the device driver will be invoked at the "noirq" > stage of system suspend, via acpi_lpss_do_suspend_late(), which is > incorrect. > > To fix that, drop acpi_lpss_do_suspend_late() and rearrange > acpi_lpss_suspend_late() to call pm_generic_suspend_late() > directly, before calling acpi_lpss_suspend(), in analogy with > acpi_subsys_suspend_late(). Ah now I see the logic in your previous test-patch. I'm afraid that this is going to break things though, the calling of the device-driver's suspend-late method at noirq time is *intentional* ! The resume_from_noirq flag is only set for i2c controllers which use: drivers/i2c/busses/i2c-designware-platdrv.c as driver. This driver's suspend late method looks like this: static int dw_i2c_plat_suspend(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); i_dev->suspended = true; if (i_dev->shared_with_punit) return 0; i_dev->disable(i_dev); i2c_dw_prepare_clk(i_dev, false); return 0; } The i_dev->disable(i_dev) and i2c_dw_prepare_clk(i_dev, false) calls here will make the i2c controller non functional. But (some of) these i2c controllers are used by code in the _PS0 / _PS3 methods of some PCI devices and the PCI core calls _PS0 / _PS3 at *noirq* time, so as explained in the commit message which introduced acpi_lpss_do_suspend_late(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=48402cee6889fb3fce58e95fea1471626286dc63 We must not only make sure that the suspending of the i2c controller is ordered so that it happens after these PCI devices are suspended, we must also make sure that the i2c controller stays functional until the i2c-controller is put in the suspend-noirq state. If you really want to go this route, we must duplicate the resume_from_noirq flag inside drivers/i2c/busses/i2c-designware-platdrv.c, setting it only for acpi_lpss enumerated devices (the driver handles a whole lot more devices) ans then make the driver's suspend_late method a no-op and instead to the suspend from its suspend_noirq callback. Since pm_generic_suspend_late() is just a wrapper to call dev->driver->pm->suspend_late duplicating the resume_from_noirq flag inside i2c-designware-platdrv.c seems unproductive. Note that we have the same thing going on in acpi_lpss.c with resume_early vs resume_noirq, we call the resume_early callback from acpi_lpss_resume_noirq if the resume_from_noirq flag is set. TL;DR: the behavior you are trying to fix here is intentional and IMHO this patch should be dropped. I guess we could / should do a patch adding a comment that the calling the drivers' suspend_late / resume_early callback at noirq time is intentional to avoid this confusing people in the future. ### Patches 1-3 and 6 look good to me and you may add my: Reviewed-by: Hans de Goede to those. The comments from this patch also impact patch 5 "ACPI: PM: Introduce "poweroff" callbacks for ACPI PM domain and LPSS" that should be rewritten to call the device's poweroff_late method at noirq time if the resume_from_noirq flag is set. I wonder if patch 5 is necessary though, the acpi_lpss code only deals with a small set of devices, at least i2c-designware-platdrv.c uses SET_LATE_SYSTEM_SLEEP_PM_OPS so the poweroff and suspend methods are 1 and the same. If this is true for all involved drivers (I did not check) then patch 5 is not really necessary. I guess if fixed to deal with resume_from_noirq the same way the suspend methods do this patch cannot hurt and if we every add separate power_off methods to the device drivers later we need this. Regards, Hans > Also notice that acpi_subsys_suspend_late() is not used outside > of the file where it is defined, so make it static and drop its > header for the header file containing it. > > Fixes: 48402cee6889 (ACPI / LPSS: Resume BYT/CHT I2C controllers from resume_noirq) > Signed-off-by: Rafael J. Wysocki > --- > drivers/acpi/acpi_lpss.c | 23 ++++++++++------------- > drivers/acpi/device_pm.c | 3 +-- > include/linux/acpi.h | 2 -- > 3 files changed, 11 insertions(+), 17 deletions(-) > > Index: linux-pm/drivers/acpi/acpi_lpss.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpi_lpss.c > +++ linux-pm/drivers/acpi/acpi_lpss.c > @@ -1034,34 +1034,31 @@ static int acpi_lpss_resume(struct devic > } > > #ifdef CONFIG_PM_SLEEP > -static int acpi_lpss_do_suspend_late(struct device *dev) > +static int acpi_lpss_suspend_late(struct device *dev) > { > + struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > int ret; > > if (dev_pm_smart_suspend_and_suspended(dev)) > return 0; > > ret = pm_generic_suspend_late(dev); > - return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev)); > -} > + if (ret) > + return ret; > > -static int acpi_lpss_suspend_late(struct device *dev) > -{ > - struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > - > - if (pdata->dev_desc->resume_from_noirq) > - return 0; > + if (!pdata->dev_desc->resume_from_noirq) > + return acpi_lpss_suspend(dev, device_may_wakeup(dev)); > > - return acpi_lpss_do_suspend_late(dev); > + return 0; > } > > static int acpi_lpss_suspend_noirq(struct device *dev) > { > struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > - int ret; > > - if (pdata->dev_desc->resume_from_noirq) { > - ret = acpi_lpss_do_suspend_late(dev); > + if (!dev_pm_smart_suspend_and_suspended(dev) && > + pdata->dev_desc->resume_from_noirq) { > + int ret = acpi_lpss_suspend(dev, device_may_wakeup(dev)); > if (ret) > return ret; > } > Index: linux-pm/drivers/acpi/device_pm.c > =================================================================== > --- linux-pm.orig/drivers/acpi/device_pm.c > +++ linux-pm/drivers/acpi/device_pm.c > @@ -1069,7 +1069,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend); > * Carry out the generic late suspend procedure for @dev and use ACPI to put > * it into a low-power state during system transition into a sleep state. > */ > -int acpi_subsys_suspend_late(struct device *dev) > +static int acpi_subsys_suspend_late(struct device *dev) > { > int ret; > > @@ -1079,7 +1079,6 @@ int acpi_subsys_suspend_late(struct devi > ret = pm_generic_suspend_late(dev); > return ret ? ret : acpi_dev_suspend(dev, device_may_wakeup(dev)); > } > -EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late); > > /** > * acpi_subsys_suspend_noirq - Run the device driver's "noirq" suspend callback. > Index: linux-pm/include/linux/acpi.h > =================================================================== > --- linux-pm.orig/include/linux/acpi.h > +++ linux-pm/include/linux/acpi.h > @@ -916,7 +916,6 @@ static inline int acpi_dev_pm_attach(str > int acpi_dev_suspend_late(struct device *dev); > int acpi_subsys_prepare(struct device *dev); > void acpi_subsys_complete(struct device *dev); > -int acpi_subsys_suspend_late(struct device *dev); > int acpi_subsys_suspend_noirq(struct device *dev); > int acpi_subsys_suspend(struct device *dev); > int acpi_subsys_freeze(struct device *dev); > @@ -924,7 +923,6 @@ int acpi_subsys_freeze(struct device *de > static inline int acpi_dev_resume_early(struct device *dev) { return 0; } > static inline int acpi_subsys_prepare(struct device *dev) { return 0; } > static inline void acpi_subsys_complete(struct device *dev) {} > -static inline int acpi_subsys_suspend_late(struct device *dev) { return 0; } > static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; } > static inline int acpi_subsys_suspend(struct device *dev) { return 0; } > static inline int acpi_subsys_freeze(struct device *dev) { return 0; } > > > >