Re: [PATCH V2] PCI: rcar: Add the initialization of PCIe link in resume_noirq
On Friday, March 8, 2019 10:35:49 PM CET Marek Vasut wrote:
> On 3/8/19 6:17 PM, Bjorn Helgaas wrote:
> > [+cc linux-pm, Rafael for SET_NOIRQ_SYSTEM_SLEEP_PM_OPS question at the end]
> >
> > On Thu, Mar 07, 2019 at 11:49:34PM +0100, Marek Vasut wrote:
> >> On 3/7/19 9:50 PM, Bjorn Helgaas wrote:
> >>> On Sun, Feb 17, 2019 at 02:24:41PM +0100, marek.va...@gmail.com wrote:
> From: Kazufumi Ikeda
>
> Reestablish the PCIe link very early in the resume process in case it
> went down to prevent PCI accesses from hanging the bus. Such accesses
> can happen early in the PCI resume process, in the resume_noirq, thus
> the link must be reestablished in the resume_noirq callback of the
> driver.
>
> Signed-off-by: Kazufumi Ikeda
> Signed-off-by: Gaku Inami
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: - Use BIT() macro for (1 << n)
> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not
> add extra changes to this function anymore
> - Make resume_noirq return early and clean up parenthesis therein
> ---
> drivers/pci/controller/pcie-rcar.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-rcar.c
> b/drivers/pci/controller/pcie-rcar.c
> index c8febb009454..b8f8fb3bc640 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -46,6 +46,7 @@
>
> /* Transfer control */
> #define PCIETCTLR 0x02000
> +#define DL_DOWNBIT(3)
> #define CFINIT 1
> >>>
> >>> I saw discussion after the V1 patch about using BIT() and making
> >>> similar constants also use BIT() for consistency. That makes sense to
> >>> me, and I think the best way would be:
> >>>
> >>> 1) in *this* patch, use "#define DL_DOWN 8"
> >>> 2) in a followup patch, convert them all to BIT()
> >>>
> >>> That way each revision of pcie-rcar.c is self-consistent.
> >>
> >> But the BIT() macros are already cleaned , see commit
> >> 0ee40820989b330e24926d82953ffb9e1c7a8425
> >>
> >> PCI: rcar: Clean up the macros
> >
> > Hmmm. Maybe I'm missing something, but it looks like 0ee40820989b
> > didn't touch CFINIT, DATA_LINK_ACTIVE, or MSIFE. Arguably,
> > LINK_SPEED_2_5GTS and LINK_SPEED_5_0GTS could use BIT() also.
> >
> > I guess I'm just old-school, but in my personal opinion, BIT() is more
> > trouble than it's worth. I'd rather see a complete bitmask because I
> > can easily match it with the typical pictures in a spec, multi-bit
> > fields are easy (you don't have to mix BIT() and GENMASK()), it gives
> > a hint about the register width, it's easy to match with a hexdump,
> > etc, e.g.,
> >
> > #define DL_DOWN0x0008
> > #define CFINIT 0x0001
> >
> > But I'm not suggesting that you get rid of BIT() in this driver. I'm
> > fine with it as long as it's used consistently.
> >
> > BTW, while we're looking at it, I think rcar_pci_read_reg() and
> > rcar_pci_write_reg() really should use "u32" instead of "unsigned
> > long", since they deal with hardware registers that are explicitly
> > 32 bits wide.
>
> OK, I can send those as separate patches.
>
> #define PCIETSTR0x02004
> #define DATA_LINK_ACTIVE 1
> @@ -1130,6 +1131,7 @@ static int rcar_pcie_probe(struct platform_device
> *pdev)
> pcie = pci_host_bridge_priv(bridge);
>
> pcie->dev = dev;
> +platform_set_drvdata(pdev, pcie);
>
> err = pci_parse_request_of_pci_ranges(dev, &pcie->resources,
> NULL);
> if (err)
> @@ -1221,10 +1223,28 @@ static int rcar_pcie_probe(struct
> platform_device *pdev)
> return err;
> }
>
> +static int rcar_pcie_resume_noirq(struct device *dev)
> +{
> +struct rcar_pcie *pcie = dev_get_drvdata(dev);
> +
> +if (rcar_pci_read_reg(pcie, PMSR) &&
> +!(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> +return 0;
> +
> +/* Re-establish the PCIe link */
> +rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> +return rcar_pcie_wait_for_dl(pcie);
> +}
> +
> +static const struct dev_pm_ops rcar_pcie_pm_ops = {
> +.resume_noirq = rcar_pcie_resume_noirq,
> +};
> >>>
> >>> I think there's the beginning of a convention to use #ifdef
> >>> CONFIG_PM_SLEEP around the ops themselves [1]. Otherwise I think
> >>> we'll get a warning about unused code when CONFIG_PM_SLEEP is unset.
> >>
> >> Only if I used SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() , but I set the
> >> resume_noirq directly.
Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
On Thu, Feb 7, 2019 at 8:42 PM Geert Uytterhoeven
wrote:
>
> When unbinding the (IOMMU-enabled) R-Car SATA device on Salvator-XS
> (R-Car H3 ES2.0), in preparation of rebinding against vfio-platform for
> device pass-through for virtualization:
>
> echo ee30.sata > /sys/bus/platform/drivers/sata_rcar/unbind
>
> the kernel crashes with:
>
> Unable to handle kernel paging request at virtual address ffbf029c
> Mem abort info:
> ESR = 0x9606
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x0006
> CM = 0, WnR = 0
> swapper pgtable: 4k pages, 39-bit VAs, pgdp = 7e8c586c
> [ffbf029c] pgd=00073bfc6003, pud=00073bfc6003,
> pmd=
> Internal error: Oops: 9606 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1098 Comm: bash Not tainted
> 5.0.0-rc5-salvator-x-00452-g37596f884f4318ef #287
> Hardware name: Renesas Salvator-X 2nd version board based on r8a7795
> ES2.0+ (DT)
> pstate: 6045 (nZCv daif +PAN -UAO)
> pc : __free_pages+0x8/0x58
> lr : __dma_direct_free_pages+0x50/0x5c
> sp : ff801268baa0
> x29: ff801268baa0 x28:
> x27: ffc6f9c60bf0 x26: ffc6f9c60bf0
> x25: ffc6f9c60810 x24:
> x23: f000 x22: ff8012145000
> x21: 0800 x20: ffbf029fffc8
> x19: x18: ffc6f86c42c8
> x17: x16: 0070
> x15: 0003 x14:
> x13: ff801103d7f8 x12: 0028
> x11: ff807604 x10: 9ad8
> x9 : ff80110126d0 x8 : ffc6f7563000
> x7 : 6b6b6b6b6b6b6b6b x6 : 0018
> x5 : ff8011cf3cc8 x4 : 4000
> x3 : 0008 x2 : 0001
> x1 : x0 : ffbf029fffc8
> Process bash (pid: 1098, stack limit = 0xc38e3e32)
> Call trace:
> __free_pages+0x8/0x58
> __dma_direct_free_pages+0x50/0x5c
> arch_dma_free+0x1c/0x98
> dma_direct_free+0x14/0x24
> dma_free_attrs+0x9c/0xdc
> dmam_release+0x18/0x20
> release_nodes+0x25c/0x28c
> devres_release_all+0x48/0x4c
> device_release_driver_internal+0x184/0x1f0
> device_release_driver+0x14/0x1c
> unbind_store+0x70/0xb8
> drv_attr_store+0x24/0x34
> sysfs_kf_write+0x4c/0x64
> kernfs_fop_write+0x154/0x1c4
> __vfs_write+0x34/0x164
> vfs_write+0xb4/0x16c
> ksys_write+0x5c/0xbc
> __arm64_sys_write+0x14/0x1c
> el0_svc_common+0x98/0x114
> el0_svc_handler+0x1c/0x24
> el0_svc+0x8/0xc
> Code: d51b4234 17fa a9bf7bfd 910003fd (b9403404)
> ---[ end trace 8c564cdd3a1a840f ]---
>
> While I've bisected this to commit e8e683ae9a736407 ("iommu/of: Fix
> probe-deferral"), and reverting that commit on post-v5.0-rc4 kernels
> does fix the problem, this turned out to be a red herring.
>
> On arm64, arch_teardown_dma_ops() resets dev->dma_ops to NULL.
> Hence if a driver has used a managed DMA allocation API, the allocated
> DMA memory will be freed using the direct DMA ops, while it may have
> been allocated using a custom DMA ops (iommu_dma_ops in this case).
>
> Fix this by reversing the order of the calls to devres_release_all() and
> arch_teardown_dma_ops().
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Rafael J. Wysocki
> ---
> Question:
> Is this safe on arm32, which calls arm_teardown_iommu_dma_ops() instead
> of resetting dev->dma_ops?
>
> ---
> Adding some debug code, and comparing before/after commit
> e8e683ae9a736407:
>
> sata_rcar ee30.sata: of_iommu_configure:227: err = -517
> -sata_rcar ee30.sata: of_iommu_configure:230: calling
> iommu_probe_device()
> -sata_rcar ee30.sata: iommu_probe_device:122: Calling ipmmu_add_device
> -sata_rcar ee30.sata: ipmmu_add_device:893
> -sata_rcar ee30.sata: of_iommu_configure:232: iommu_probe_device()
> returned -19
> -sata_rcar ee30.sata: dma_alloc_attrs:257: size 2048, ops =
> (null)
> -sata_rcar ee30.sata: __dma_direct_alloc_pages:104: size 4096
> -sata_rcar ee30.sata: __dma_direct_alloc_pages:115: trying
> dma_alloc_from_contiguous()
> -sata_rcar ee30.sata: dma_alloc_from_contiguous:202: cma_alloc(1)
> returned page ffbf00d20e00
> -sata_rcar ee30.sata: dma_alloc_attrs:271: allocated using
> dma_direct_alloc()
> -sata_rcar ee30.sata: dmam_alloc_attrs
Re: [PATCH] PM/runtime: Optimize pm_runtime_autosuspend_expiration()
On Wednesday, January 30, 2019 10:40:17 PM CET Ladislav Michl wrote:
> pm_runtime_autosuspend_expiration calls ktime_get_mono_fast_ns
> even when its returned value may be unused. Therefore get
> current time later and remove gotos while there.
>
> Signed-off-by: Ladislav Michl
> Acked-by: Tony Lindgren
> Acked-by: Vincent Guittot
> ---
> This patch is based on top of bleeding-edge branch, where
> "[PATCH v2 ] PM-runtime: fix deadlock with ktime" is sitting.
> I expect v3 of that patch, which should not harm this one.
> It is meant to replace "PM/runtime: Do not needlessly call ktime_get"
> sent earlier.
>
> drivers/base/power/runtime.c | 19 ---
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 65e2b5f48e0c..7bbe28faca8d 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -145,24 +145,21 @@ static void pm_runtime_cancel_pending(struct device
> *dev)
> u64 pm_runtime_autosuspend_expiration(struct device *dev)
> {
> int autosuspend_delay;
> - u64 last_busy, expires = 0;
> - u64 now = ktime_get_mono_fast_ns();
> + u64 expires;
>
> if (!dev->power.use_autosuspend)
> - goto out;
> + return 0;
>
> autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> if (autosuspend_delay < 0)
> - goto out;
> -
> - last_busy = READ_ONCE(dev->power.last_busy);
> + return 0;
>
> - expires = last_busy + (u64)autosuspend_delay * NSEC_PER_MSEC;
> - if (expires <= now)
> - expires = 0;/* Already expired. */
> + expires = READ_ONCE(dev->power.last_busy);
> + expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> + if (expires > ktime_get_mono_fast_ns())
> + return expires; /* Expires in the future */
>
> - out:
> - return expires;
> + return 0;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
>
>
Applied, thanks!
Re: [PATCH 0/2] PM-runtime: fix and clean up of time accounting
On Monday, February 4, 2019 5:25:51 PM CET Vincent Guittot wrote: > Fix time accounting which has the same lock contraint as for using hrtimer > and update accounting_timestamp only when useful. > > Vincent Guittot (2): > PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() > PM-runtime: update time accounting only when enabled > > drivers/base/power/runtime.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > Both applied, thanks!
Re: [PATCH v3] PM-runtime: fix deadlock with ktime
On Wed, Jan 30, 2019 at 6:26 PM Vincent Guittot
wrote:
>
> A deadlock has been seen when swicthing clocksources which use PM runtime.
> The call path is:
> change_clocksource
> ...
> write_seqcount_begin
> ...
> timekeeping_update
> ...
> sh_cmt_clocksource_enable
> ...
> rpm_resume
> pm_runtime_mark_last_busy
> ktime_get
> do
> read_seqcount_begin
> while read_seqcount_retry
>
> write_seqcount_end
>
> Although we should be safe because we haven't yet changed the clocksource
> at that time, we can't because of seqcount protection.
>
> Use ktime_get_mono_fast_ns() instead which is lock safe for such case
>
> With ktime_get_mono_fast_ns, the timestamp is not guaranteed to be
> monotonic across an update and as a result can goes backward. According to
> update_fast_timekeeper() description: "In the worst case, this can
> result is a slightly wrong timestamp (a few nanoseconds)". For
> PM runtime autosuspend, this means only that the suspend decision can
> be slightly sub optimal.
>
> Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers")
> Reported-by: Biju Das
> Signed-off-by: Vincent Guittot
> ---
>
> Hi Rafael,
>
> Sorry, I sent the version with the typo mistake that generated the
> compilation error
> reported by kbuild-test-robot
>
> This version doesn't have the typo.
OK, I've applied this one, thanks!
Re: [PATCH v2 ] PM-runtime: fix deadlock with ktime
On Wed, Jan 30, 2019 at 12:16 PM Vincent Guittot
wrote:
>
> A deadlock has been seen when swicthing clocksources which use PM runtime.
> The call path is:
> change_clocksource
> ...
> write_seqcount_begin
> ...
> timekeeping_update
> ...
> sh_cmt_clocksource_enable
> ...
> rpm_resume
> pm_runtime_mark_last_busy
> ktime_get
> do
> read_seqcount_begin
> while read_seqcount_retry
>
> write_seqcount_end
>
> Although we should be safe because we haven't yet changed the clocksource
> at that time, we can't because of seqcount protection.
>
> Use ktime_get_mono_fast_ns() instead which is lock safe for such case
>
> With ktime_get_mono_fast_ns, the timestamp is not guaranteed to be
> monotonic across an update and as a result can goes backward. According to
> update_fast_timekeeper() description: "In the worst case, this can
> result is a slightly wrong timestamp (a few nanoseconds)". For
> PM runtime autosuspend, this means only that the suspend decision can
> be slightly sub optimal.
>
> Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers")
> Reported-by: Biju Das
> Signed-off-by: Vincent Guittot
I've queued this one up as a fix for 5.0, but unfortunately it clashes
with the patch from Ladislav Michl at
https://patchwork.kernel.org/patch/10755477/ which has been dropped
now.
Can you or Ladislav please rebase that patch on top of this one and repost?
> ---
>
> - v2: Updated commit message to explain the impact of using
> ktime_get_mono_fast_ns()
>
> drivers/base/power/runtime.c | 10 +-
> include/linux/pm_runtime.h | 2 +-
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 457be03..708a13f 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
> {
> int autosuspend_delay;
> u64 last_busy, expires = 0;
> - u64 now = ktime_to_ns(ktime_get());
> + u64 now = ktime_get_mono_fast_ns();
>
> if (!dev->power.use_autosuspend)
> goto out;
> @@ -909,7 +909,7 @@ static enum hrtimer_restart pm_suspend_timer_fn(struct
> hrtimer *timer)
> * If 'expires' is after the current time, we've been called
> * too early.
> */
> - if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> + if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> dev->power.timer_expires = 0;
> rpm_suspend(dev, dev->power.timer_autosuspends ?
> (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC);
> @@ -928,7 +928,7 @@ static enum hrtimer_restart pm_suspend_timer_fn(struct
> hrtimer *timer)
> int pm_schedule_suspend(struct device *dev, unsigned int delay)
> {
> unsigned long flags;
> - ktime_t expires;
> + u64 expires;
> int retval;
>
> spin_lock_irqsave(&dev->power.lock, flags);
> @@ -945,8 +945,8 @@ int pm_schedule_suspend(struct device *dev, unsigned int
> delay)
> /* Other scheduled or pending requests need to be canceled. */
> pm_runtime_cancel_pending(dev);
>
> - expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> - dev->power.timer_expires = ktime_to_ns(expires);
> + expires = ktime_get_mono_fast_ns() + (u64)delay * NSEC_PER_MSEC);
> + dev->power.timer_expires = expires;
> dev->power.timer_autosuspends = 0;
> hrtimer_start(&dev->power.suspend_timer, expires, HRTIMER_MODE_ABS);
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 54af4ee..fed5be7 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -105,7 +105,7 @@ static inline bool pm_runtime_callbacks_present(struct
> device *dev)
>
> static inline void pm_runtime_mark_last_busy(struct device *dev)
> {
> - WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
> + WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> }
>
> static inline bool pm_runtime_is_irq_safe(struct device *dev)
> --
> 2.7.4
>
Re: [PATCH] PM-runtime: fix deadlock with ktime
On Wed, Jan 30, 2019 at 10:14 AM Vincent Guittot
wrote:
>
> Hi Geert,
>
> On Wed, 30 Jan 2019 at 09:21, Geert Uytterhoeven wrote:
> >
> > Hi Vincent,
> >
> > On Wed, Jan 30, 2019 at 9:16 AM Vincent Guittot
> > wrote:
> > > A deadlock has been seen when swicthing clocksources which use PM runtime.
> > > The call path is:
> > > change_clocksource
> > > ...
> > > write_seqcount_begin
> > > ...
> > > timekeeping_update
> > > ...
> > > sh_cmt_clocksource_enable
> > > ...
> > > rpm_resume
> > > pm_runtime_mark_last_busy
> > > ktime_get
> > > do
> > > read_seqcount_begin
> > > while read_seqcount_retry
> > >
> > > write_seqcount_end
> > >
> > > Although we should be safe because we haven't yet changed the clocksource
> > > at that time, we can't because of seqcount protection.
> > >
> > > Use ktime_get_mono_fast_ns instead which is lock safe for such case
> > >
> > > Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using
> > > hrtimers")
> > > Reported-by: Biju Das
> > > Signed-off-by: Vincent Guittot
> >
> > Thanks for your patch!
> >
> > /**
> > * ktime_get_mono_fast_ns - Fast NMI safe access to clock monotonic
> > *
> > * This timestamp is not guaranteed to be monotonic across an update.
> > * The timestamp is calculated by:
> > *
> > * now = base_mono + clock_delta * slope
> > *
> > * So if the update lowers the slope, readers who are forced to the
> > * not yet updated second array are still using the old steeper slope.
> > *
> > * tmono
> > * ^
> > * |o n
> > * | o n
> > * | u
> > * | o
> > * |o
> > * |12345678---> reader order
> > *
> > * o = old slope
> > * u = update
> > * n = new slope
> > *
> > * So reader 6 will observe time going backwards versus reader 5.
> > *
> > * While other CPUs are likely to be able observe that, the only way
> > * for a CPU local observation is when an NMI hits in the middle of
> > * the update. Timestamps taken from that NMI context might be ahead
> > * of the following timestamps. Callers need to be aware of that and
> > * deal with it.
> > */
> >
> > As this function is not guaranteed to be monotonic, have you checked how
> > the Runtime PM code behaves if time goes backwards? Does it just make
> > a suboptimal decision or does it crash?
>
> As a worst case this will generate a suboptimal decision around the update
So that should be explained in the changelog of the patch. In detail,
if poss, please.
Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c adapters
On Thursday, December 20, 2018 11:00:29 AM CET Hans de Goede wrote:
> Hi,
>
> On 19-12-18 23:33, Wolfram Sang wrote:
> > Hi Lukas, Hans,
> >
> > On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 19-12-18 18:22, Lukas Wunner wrote:
> >>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> +{
> +i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> +i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}
> >>>
> >>> This looks like a duplication of the is_suspended flag in struct
> >>> dev_pm_info.
> >>> Any reason why you can't use that? If so, it would be good to document
> >>> the
> >>> reason in the commit message.
> >>
> >> Oh, that is a very good point and that one only gets set on system suspend
> >> and not on resume suspend, working around the problems with the
> >> i2c-designware
> >
> > Just to make it clear: you mean runtime suspend, not resume suspend, or?
>
> Yes I mean runtime-suspend, sorry.
The power.is_suspended flag is about system-wide suspend, however.
Re: [PATCH 10/10] cpufreq: dt: Add support for r8a7744
On Tuesday, September 25, 2018 10:16:12 AM CEST Biju Das wrote: > Hi Rafeal, > > Sorry to bother you. Are you happy to merge the below patch with 4.20 > > https://patchwork.kernel.org/patch/10595451/ Yes. You had told me that you wanted me to apply it, so I queued it up. Thanks, Rafael
Re: [PATCH 10/10] cpufreq: dt: Add support for r8a7744
On Tue, Sep 11, 2018 at 12:20 PM Biju Das wrote:
>
> Add the compatible strings for supporting the generic cpufreq driver on
> the Renesas RZ/G1N (R8A7744) SoC.
>
> Signed-off-by: Biju Das
> Reviewed-by: Fabrizio Castro
> ---
> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> b/drivers/cpufreq/cpufreq-dt-platdev.c
> index fe14c57..805f8a0 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -58,6 +58,7 @@ static const struct of_device_id whitelist[] __initconst = {
> { .compatible = "renesas,r8a73a4", },
> { .compatible = "renesas,r8a7740", },
> { .compatible = "renesas,r8a7743", },
> + { .compatible = "renesas,r8a7744", },
> { .compatible = "renesas,r8a7745", },
> { .compatible = "renesas,r8a7778", },
> { .compatible = "renesas,r8a7779", },
> --
Since this is the last patch in a series of 10, do you want me to
apply it or should it go in along with the rest?
Thanks,
Rafael
Re: [PATCH 0/2] Revert explicit support for Renesas R-Car Gen 3 r8a779[56] SoCs
On Tue, May 15, 2018 at 9:35 AM, Simon Horman wrote:
> On Thu, May 10, 2018 at 11:51:38AM +0200, Rafael J. Wysocki wrote:
>> On Wednesday, May 2, 2018 11:58:04 AM CEST Simon Horman wrote:
>> > Revert commits that added explicit support for Renesas R-Car Gen 3
>> > r8a779[56] SoCs to the generic cpufreq driver.
>> >
>> > This is no longer needed since the flowing commit and to the best of my
>> > knowledge is not relied on by any upstream DTS: edeec420de24 ("cpufreq:
>> > dt-platdev: Automatically create cpufreq device with OPP v2")
>> >
>> > Simon Horman (2):
>> > Revert "cpufreq: dt: Add r8a7796 support to to use generic cpufreq
>> > driver"
>> > Revert "cpufreq: rcar: Add support for R8A7795 SoC"
>> >
>> > drivers/cpufreq/cpufreq-dt-platdev.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> >
>>
>> Am I expected to pick up this series?
>
> Hi Rafael,
>
> that would be ideal from my point of view.
OK, I'll queue them up, then.
Thanks!
Re: [PATCH 0/2] Revert explicit support for Renesas R-Car Gen 3 r8a779[56] SoCs
On Wednesday, May 2, 2018 11:58:04 AM CEST Simon Horman wrote:
> Revert commits that added explicit support for Renesas R-Car Gen 3
> r8a779[56] SoCs to the generic cpufreq driver.
>
> This is no longer needed since the flowing commit and to the best of my
> knowledge is not relied on by any upstream DTS: edeec420de24 ("cpufreq:
> dt-platdev: Automatically create cpufreq device with OPP v2")
>
> Simon Horman (2):
> Revert "cpufreq: dt: Add r8a7796 support to to use generic cpufreq
> driver"
> Revert "cpufreq: rcar: Add support for R8A7795 SoC"
>
> drivers/cpufreq/cpufreq-dt-platdev.c | 2 --
> 1 file changed, 2 deletions(-)
>
>
Am I expected to pick up this series?
Thanks,
Rafael
Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification
On Mon, Apr 23, 2018 at 11:32 AM, Geert Uytterhoeven wrote: > Hi Rafael, > > On Mon, Apr 23, 2018 at 11:18 AM, Rafael J. Wysocki > wrote: >> On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote: >>> Add a callback to inform a device that his wake-up setting has been >>> changed. This allows a device to synchronize device configuration with >>> an external user action. >>> >>> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power >>> switch, the system suspend procedure is: >>> 1. Configure PMIC for DDR backup mode, which changes the role of the >>> accessory power switch from a power to a wake-up switch, >>> 2. Switch accessory power switch off, to prepare for system suspend, >>> 3. Suspend system. >>> >>> Hence step 1 cannot be done in the PMIC's suspend callback, >> >> I don't quite understand this, so can you please explain? >> >> What can't it be done from ->prepare() or even from a suspend notifier? >> >>> but it can be done in the new callback, in response to the user writing >>> "enabled" >>> to the PMIC's wakeup virtual file in sysfs. > > Step 2 (flip a switch on the board) is a manual step, to be done by the user. > So it cannot be done from any suspend callback or notifier. I still find it somewhat difficult to follow you here. :-) How is this expected to work? Is the user expected to flip the switch on the board and then write to "wakeup" or is that the other way around?
Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification
First, sorry for the huge delay. On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote: > Add a callback to inform a device that his wake-up setting has been > changed. This allows a device to synchronize device configuration with > an external user action. > > E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power > switch, the system suspend procedure is: > 1. Configure PMIC for DDR backup mode, which changes the role of the > accessory power switch from a power to a wake-up switch, > 2. Switch accessory power switch off, to prepare for system suspend, > 3. Suspend system. > > Hence step 1 cannot be done in the PMIC's suspend callback, I don't quite understand this, so can you please explain? What can't it be done from ->prepare() or even from a suspend notifier? > but it can be done in the new callback, in response to the user writing > "enabled" > to the PMIC's wakeup virtual file in sysfs.
Re: [PATCH] dmaengine: rcar-dmac: Make DMAC reinit during system resume explicit
On Wednesday, January 17, 2018 11:19:17 AM CET Vinod Koul wrote:
> On Wed, Jan 17, 2018 at 10:38:28AM +0100, Geert Uytterhoeven wrote:
> > The current (empty) system sleep callbacks rely on the PM core to force
> > a runtime resume to reinitialize the DMAC registers during system
> > resume. Without a reinitialization, e.g. SCIF DMA will hang silently
> > after a system resume on R-Car Gen3.
> >
> > Make this explicit by using pm_runtime_force_{suspend,resume}() as the
> > system sleep callbacks instead. Use SET_LATE_SYSTEM_SLEEP_PM_OPS() as
> > DMA engines must be initialized before all DMA slave devices.
> >
> > Suggested-by: Ulf Hansson
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > This is a dependency for "[PATCH 1/2] PM / genpd: Stop/start devices
> > without pm_runtime_force_suspend/resume()"
> > (https://www.spinics.net/lists/kernel/msg2696802.html), so perhaps it
> > makes most sense if Rafael takes it through the PM tree?
>
> Sounds okay to me.
>
> Acked-By: Vinod Koul
OK, so I'll take it.
Thanks!
Re: [PATCH 0/2] PM / core: genpd fix and pm_runtime_force_suspend|resume() rework
On Wednesday, January 17, 2018 10:14:23 AM CET Geert Uytterhoeven wrote: > Hi Rafael, > > On Mon, Jan 15, 2018 at 5:17 PM, Rafael J. Wysocki wrote: > > On Mon, Jan 15, 2018 at 3:26 PM, Ulf Hansson wrote: > >> On 15 January 2018 at 14:22, Geert Uytterhoeven > >> wrote: > > > > [cut] > > > >>> > >>> I did miss a small difference in topology: in pm/linux-next, H3 has DMA > >>> enabled for SCIF2, while M3 hasn't (yet). > >>> With DMA enabled on M3, it fails in the same way. > >>> > >>> As genpd_resume_noirq() no longer calls pm_runtime_force_resume(), > >>> rcar_dmac_runtime_resume() is no longer called, and the DMAC's registers > >>> are no longer reinitialized after system resume, breaking the serial port. > >> > >> In drivers/dma/sh/rcar-dmac.c, I would try to replace the below line: > >> SET_SYSTEM_SLEEP_PM_OPS(rcar_dmac_sleep_suspend, rcar_dmac_sleep_resume) > >> > >> with: > >> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) > > > > Yes, that probably is the least intrusive thing that can be done to > > address the issue. > > > >> in case that may be too early to suspend the dma device (which is > >> rather common for dma devices) then try; > >> > >> SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > >> pm_runtime_force_resume) > > > > Good suggestion, and I would go straight for it anyway. > > > > Geert, can you try if this works, please? > > Works. Both using SET_SYSTEM_SLEEP_PM_OPS() and > SET_LATE_SYSTEM_SLEEP_PM_OPS(). But given this is a DMA engine > driver, I'd settle for the latter. > > And I did verify doing so doesn't break the system without the patch > in $subject. > > Thanks! > > Will send a patch... Thank you!
Re: [PATCH 0/2] PM / core: genpd fix and pm_runtime_force_suspend|resume() rework
On Mon, Jan 15, 2018 at 3:26 PM, Ulf Hansson wrote: > On 15 January 2018 at 14:22, Geert Uytterhoeven wrote: [cut] >> >> I did miss a small difference in topology: in pm/linux-next, H3 has DMA >> enabled for SCIF2, while M3 hasn't (yet). >> With DMA enabled on M3, it fails in the same way. >> >> As genpd_resume_noirq() no longer calls pm_runtime_force_resume(), >> rcar_dmac_runtime_resume() is no longer called, and the DMAC's registers >> are no longer reinitialized after system resume, breaking the serial port. > > In drivers/dma/sh/rcar-dmac.c, I would try to replace the below line: > SET_SYSTEM_SLEEP_PM_OPS(rcar_dmac_sleep_suspend, rcar_dmac_sleep_resume) > > with: > SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) Yes, that probably is the least intrusive thing that can be done to address the issue. > in case that may be too early to suspend the dma device (which is > rather common for dma devices) then try; > > SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume) Good suggestion, and I would go straight for it anyway. Geert, can you try if this works, please? Thanks, Rafael
Re: [PATCH 0/2] PM / core: genpd fix and pm_runtime_force_suspend|resume() rework
On Sun, Jan 14, 2018 at 10:48 AM, Geert Uytterhoeven wrote: > Hi Rafael, > > On Sat, Jan 13, 2018 at 1:38 AM, Rafael J. Wysocki wrote: >> On Friday, January 12, 2018 3:31:09 PM CET Geert Uytterhoeven wrote: >>> On Fri, Jan 12, 2018 at 2:00 PM, Rafael J. Wysocki >>> wrote: >>> > This comes from the recent discussion/testing effort that ensued after my >>> > pm_runtime_force_suspend|resume() changes proposal: >>> > >>> > https://marc.info/?t=15149777204&r=1&w=2 >>> > >>> > Patch [1/2] basically is https://patchwork.kernel.org/patch/10152873/ >>> > rebased >>> > on top of the current linux-next branch of the linux-pm.git tree (the >>> > relevant >>> > part should be there in the linux-next tree proper ATM). It applies on >>> > top >>> > of https://patchwork.kernel.org/patch/10156077/ which should apply to the >>> > Linus' >>> > tree cleanly. >>> > >>> > Patch [2/2] is a resend of https://patchwork.kernel.org/patch/10142047/ >>> > with >>> > a very minor changelog modification and the R-b tag from Ulf. >>> > >>> > Geert, if possible, please test this on the Renesas systems that had the >>> > problem with https://patchwork.kernel.org/patch/10142047/ previously and >>> > let >>> > me know if you still see issues. >>> >>> I've tested this on two very similar systems: Salvator-XS with R-Car H3 >>> ES2.0, >>> and Salvator-X with R-Car M3-W ES1.0. >>> >>> On the M3-based system, everything seems to work fine. >> >> Good. >> >>> On the H3-based system, the serial console (the /dev/ttySC0 device, not >>> kernel >>> serial output) is dead after resume from s2ram, with and without >>> no_console_suspend. >>> >>> With no_console_suspend, I see: >>> >>> ttySC ttySC0: 1 input overrun(s) >>> >>> after typing on the serial console, so it looks like an interrupt problem. >>> >>> This issue seems to be caused by patch [1/2]. But I have no idea what's >>> really happening, and why the two systems behave differently. > > Could be a firmware issue, too. > While the kernel images are identical, the ARM trusted firmware configs aren't > (same version, though). > > I'll do some more investigation... OK, thanks! It also would be good to know the topology of the device hierarchy and how that maps to the domains on the failing system (and which UART clocks are operated by genpd). >> Well, that's not dramatic. >> >> Let's make a deal that we'll fix this on top of [1/2]. > > ;-) > >> Which driver is this BTW? sh-sci? That one doesn't even support runtime >> PM, confusingly enough. > > Yes, sh-sci. It does make pm_runtime_*() calls. Hmm. I overlooked that part. This is sort of unusual, because the driver doesn't provide any runtime PM callbacks, but still it does provided system suspend ones. It looks like the idea is to never put it into runtime suspend if any ports are enabled and always put it into runtime suspend otherwise. Which one is the case in your testing? Is the port disabled or enabled during system-wide suspend? > And of course there's uart_ops.pm, which is driven from serial_core... What does this point to for that particular device?
Re: [PATCH 0/2] PM / core: genpd fix and pm_runtime_force_suspend|resume() rework
On Friday, January 12, 2018 3:31:09 PM CET Geert Uytterhoeven wrote: > Hi Rafael, > > On Fri, Jan 12, 2018 at 2:00 PM, Rafael J. Wysocki wrote: > > This comes from the recent discussion/testing effort that ensued after my > > pm_runtime_force_suspend|resume() changes proposal: > > > > https://marc.info/?t=15149777204&r=1&w=2 > > > > Patch [1/2] basically is https://patchwork.kernel.org/patch/10152873/ > > rebased > > on top of the current linux-next branch of the linux-pm.git tree (the > > relevant > > part should be there in the linux-next tree proper ATM). It applies on top > > of https://patchwork.kernel.org/patch/10156077/ which should apply to the > > Linus' > > tree cleanly. > > > > Patch [2/2] is a resend of https://patchwork.kernel.org/patch/10142047/ with > > a very minor changelog modification and the R-b tag from Ulf. > > > > Geert, if possible, please test this on the Renesas systems that had the > > problem with https://patchwork.kernel.org/patch/10142047/ previously and let > > me know if you still see issues. > > I've tested this on two very similar systems: Salvator-XS with R-Car H3 ES2.0, > and Salvator-X with R-Car M3-W ES1.0. > > On the M3-based system, everything seems to work fine. Good. > On the H3-based system, the serial console (the /dev/ttySC0 device, not kernel > serial output) is dead after resume from s2ram, with and without > no_console_suspend. > > With no_console_suspend, I see: > > ttySC ttySC0: 1 input overrun(s) > > after typing on the serial console, so it looks like an interrupt problem. > > This issue seems to be caused by patch [1/2]. But I have no idea what's > really happening, and why the two systems behave differently. Well, that's not dramatic. Let's make a deal that we'll fix this on top of [1/2]. Which driver is this BTW? sh-sci? That one doesn't even support runtime PM, confusingly enough. > Oh well, have a nice weekend! Thanks, you too!
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Friday, January 12, 2018 12:26:56 PM CET Geert Uytterhoeven wrote: > Hi Rafael, > > On Tue, Jan 9, 2018 at 5:28 PM, Rafael J. Wysocki wrote: > > On Tuesday, January 9, 2018 5:03:18 PM CET Rafael J. Wysocki wrote: > >> On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven > >> wrote: > >> > On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki > >> > wrote: > >> >> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven > >> >> wrote: > >> >>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki > >> >>> wrote: > >> >>>> From: Rafael J. Wysocki > >> >>>> > >> >>>> One of the limitations of pm_runtime_force_suspend/resume() is that > >> >>>> if a parent driver wants to use these functions, all of its child > >> >>>> drivers have to do that too because of the parent usage counter > >> >>>> manipulations necessary to get the correct state of the parent during > >> >>>> system-wide transitions to the working state (system resume). > >> >>>> However, that limitation turns out to be artificial, so remove it. > >> >>>> > >> >>>> Namely, pm_runtime_force_suspend() only needs to update the children > >> >>>> counter of its parent (if there's is a parent) when the device can > >> >>>> stay in suspend after the subsequent system resume transition, as > >> >>>> that counter is correct already otherwise. Now, if the parent's > >> >>>> children counter is not updated, it is not necessary to increment > >> >>>> the parent's usage counter in that case any more, as long as the > >> >>>> children counters of devices are checked along with their usage > >> >>>> counters in order to decide whether or not the devices may be left > >> >>>> in suspend after the subsequent system resume transition. > >> >>>> > >> >>>> Accordingly, modify pm_runtime_force_suspend() to only call > >> >>>> pm_runtime_set_suspended() for devices whose usage and children > >> >>>> counters are at the "no references" level (the runtime PM status > >> >>>> of the device needs to be updated to "suspended" anyway in case > >> >>>> this function is called once again for the same device during the > >> >>>> transition under way), drop the parent usage counter incrementation > >> >>>> from it and update pm_runtime_force_resume() to compensate for these > >> >>>> changes. > >> >>>> > >> >>>> Signed-off-by: Rafael J. Wysocki > >> >>> > >> >>> This patch causes a regression during system resume on Renesas > >> >>> Salvator-XS > >> >>> with R-Car H3 ES2.0: > >> >> > >> >> I have dropped it for now, but we need to address the underlying issue. > >> >> > >> >>> SError Interrupt on CPU3, code 0xbf02 -- SError > >> >>> SError Interrupt on CPU2, code 0xbf02 -- SError > >> >>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted > >> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97 > >> >>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted > >> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97 > >> >>> Hardware name: Renesas Salvator-X 2nd version board based on > >> >>> r8a7795 ES2.0+ (DT) > >> >>> Hardware name: Renesas Salvator-X 2nd version board based on > >> >>> r8a7795 ES2.0+ (DT) > >> >>> Workqueue: events_unbound async_run_entry_fn > >> >>> Workqueue: events_unbound async_run_entry_fn > >> >>> pstate: 6005 (nZCv daif -PAN -UAO) > >> >>> pstate: 6005 (nZCv daif -PAN -UAO) > >> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8 > >> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8 > >> >>> lr : phy_init+0x64/0xcc > >> >>> lr : phy_init+0x64/0xcc > >> >>> ... > >> >>> Kernel panic - not syncing: Asynchronous SError Interrupt > >> >>> > >> >>> Note that before, it printed a warning instead: > >> >>>
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Thu, Jan 11, 2018 at 1:32 PM, Ulf Hansson wrote: [cut] >>> >>> The point is, we can go for this solution, but is it good enough? >> >> I would like to treat it as a step away from what is there today, >> retaining some of the existing functionality. >> >> From a quick look at the existing users of genpd that use the device >> start/stop functionality, running genpd_stop_dev()/genpd_start_dev() >> in the "noirq" phases should not be problematic for them at least. > > I guess so. > > Still, the error report by Geert worries me, but I don't think it > worth to speculate, but rather test and see. Agreed. >>> Another option, is to simply to remove (and not replace with something >>> else) the calls to pm_runtime_force_ suspend|resume() from genpd. >> >> Right. >> >>> This moves the entire responsibility to the driver, to put its device into >>> low power state during system suspend, but with the benefit of that it >>> can decide to use the correct level of suspend/resume callbacks. >> >> Drivers of the devices in the "stop/start" domains will have to use >> pm_runtime_force_ suspend|resume() internally then to benefit from the >> domain's management of clocks, but that just may be the only way to >> avoid more problems. > > Agree. But this doesn't look particularly attractive to me, because it would add a requirement for drivers to implement their system-wide PM callbacks in a specific way and, in my view, no such requirements should be imposed by code that advertises itself as "generic" (by the name at least). In particular, drivers written to this requirement might not be able to work correctly with other middle-layer code. If genpd was a bus type, that wouldn't be particularly objectionable, even though it is not particularly straightforward either, because drivers register for a particular bus type and so they need to take its specifics into account. Drivers usually don't register for a specific PM domain, however, and at least some of them may need to work with different PM domain code on different platforms. >>> No matter how we do it, changing this may introduce some potential >>> regressions from a power consumption point of view, however although >>> only for those SoCs that uses the ->start|stop() callbacks. To >>> mitigate these power regressions, some drivers needs to be fixed, but >>> that seems inevitable anyway, doesn't it? >> >> Yes, it does. >> >> I would say let's try to go with this patch of mine as submitted and >> see how far we can get with that. >> >> That essentially doesn't prevent us from dropping the >> genpd_stop_dev()/genpd_start_dev() from the "noirq" stages if need be >> after all, but dropping them right away may cause the fallout to be >> more dramatic, at least in theory. >> > > Well, my guesses is that would be a step to make the behavior a bit > more deterministic and perhaps less fragile than today. > > On the other hand, I am also concerned about the future users of the > ->stop|start() callbacks, including related drivers dealing with the > affected devices. That in a sense, that converting the code in genpd > to what you suggest, would still not encourage related drivers to > behave correctly during system suspend/resume. I'm not sure what you mean here. Drivers in principle should not need to worry about the ->stop|start() part, because that belongs to a different code layer. They only need to be able to expect certain things (like clocks) to be there (or in a specific state) when their code is running in certain cases. Now, I realize that this is pure theory and that in practice, if ->stop|start() are there, drivers working with genpd are probably platform-specific anyway, more or less, so it is not likely that they will ever have to work with any other middle-layer code. >Then down the road, when additional new users of the ->stop|start() callbacks > may have been added, it may be even harder to drop calling them in from the > noirq phases. > > So to conclude, I would prefer to drop calling > pm_runtime_force_suspend|resume() from genpd, without a replacement, > but I am willing to accept also your suggested alternative. OK Thanks, Rafael
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Wed, Jan 10, 2018 at 10:26 AM, Ulf Hansson wrote:
> On 9 January 2018 at 17:28, Rafael J. Wysocki wrote:
>> On Tuesday, January 9, 2018 5:03:18 PM CET Rafael J. Wysocki wrote:
>>> On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven
>>> wrote:
[cut]
>>> That's because of the pm_runtime_force_suspend/resume() called by
>>> genpd. These functions generally may cause devices active before
>>> system suspend to be left in suspend after it. That generally is a
>>> good idea if the device was not really in use before the system
>>> suspend, but there is a problem that the driver of it may not be
>>> prepared for that to happen (and also the way to determine the "not
>>> really in use" case may not be as expected by the driver).
>>
>> So below is something to try (untested).
>>
>> I know that Ulf will be protesting, but that's what I would do unless there
>> are really *really* good reasons not to.
>
> I am not protesting! :-)
OK
That makes things a lot easier in principle. :-)
> Instead I think we are rather in agreement that we are concerned about
> that genpd are invoking pm_runtime_force_suspend|resume().
>
>>
>>
>> ---
>> drivers/base/power/domain.c | 34 +-
>> 1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/domain.c
>> ===
>> --- linux-pm.orig/drivers/base/power/domain.c
>> +++ linux-pm/drivers/base/power/domain.c
>> @@ -1048,8 +1048,9 @@ static int genpd_finish_suspend(struct d
>> if (ret)
>> return ret;
>>
>> - if (genpd->dev_ops.stop && genpd->dev_ops.start) {
>> - ret = pm_runtime_force_suspend(dev);
>> + if (genpd->dev_ops.stop && genpd->dev_ops.start &&
>> + !pm_runtime_status_suspended(dev)) {
>> + ret = genpd_stop_dev(genpd, dev);
>
> Something like this existed there before, but because of other
> internal genpd reasons. It's an option but there are issues with it.
OK
> I think it's fragile because invoking the ->stop() callback may
> trigger calls to "disable" resources (like clocks, pinctrls, etc) for
> the device. Doing this at ->suspend_noirq() may be too late, as that
> subsystem/driver for the resource(s) may already be system suspended.
> This is the classic problem of suspending (and later resuming) devices
> in in-correct order.
Well, this is what happens with the current code too.
I mean if unmodified genpd_finish_suspend() runs, it will call
pm_runtime_force_suspend() and that will check the device's runtime PM
status in the first place. If that is "suspended", it will return
right away without doing anything (after disabling runtime PM, but
that is irrelevant here as runtime PM is already disabled). In turn,
if the runtime PM status is "active", it will run
genpd_runtime_suspend() (as the callback) and that will run the
driver's runtime PM callback and the genpd_stop_dev() right after it.
Thus, if calling genpd_stop_dev() at that point is problematic, it is
problematic already today.
What the patch does is essentially to drop the driver's runtime PM
callback (should not be necessary) and the domain power off (certainly
not necessary) from that path, but to keep the genpd_stop_dev()
invocation that may be necessary in principle (the device is "active",
so genpd_runtime_suspend() has not run for it, so genpd_stop_dev() has
not been called for it yet, but it may be good to stop the clocks
before powering off the domain).
The resume part is basically running genpd_start_dev() for the devices
for which genpd_stop_dev() was run by genpd_finish_suspend(), which is
because the subsequent driver callbacks may expect the clocks to be
enabled.
> Today we deal with this, by drivers/subsystem assigning the right
> level of callback, as well as making sure the "dpm_list" is sorted
> correctly (yeah, we have device links as well).
>
> The point is, we can go for this solution, but is it good enough?
I would like to treat it as a step away from what is there today,
retaining some of the existing functionality.
>From a quick look at the existing users of genpd that use the device
start/stop functionality, running genpd_stop_dev()/genpd_start_dev()
in the "noirq" phases should not be problematic for them at least.
> Another option, is to simply to remove (and not replace with something
> else) the calls to pm_runtime_force_ suspend|resume() from genpd.
Right.
> This moves the entire responsibility to the driver, to put its
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tue, Jan 9, 2018 at 7:46 PM, Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
> wrote:
>> Hi Rafael,
>>
>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
>> wrote:
>>> From: Rafael J. Wysocki
>>>
[cut]
>>>
>>> Signed-off-by: Rafael J. Wysocki
>>
>> This patch causes a regression during system resume on Renesas Salvator-XS
>> with R-Car H3 ES2.0:
>>
>> SError Interrupt on CPU3, code 0xbf02 -- SError
>> SError Interrupt on CPU2, code 0xbf02 -- SError
>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>> Hardware name: Renesas Salvator-X 2nd version board based on
>> r8a7795 ES2.0+ (DT)
>> Hardware name: Renesas Salvator-X 2nd version board based on
>> r8a7795 ES2.0+ (DT)
>> Workqueue: events_unbound async_run_entry_fn
>> Workqueue: events_unbound async_run_entry_fn
>> pstate: 6005 (nZCv daif -PAN -UAO)
>> pstate: 6005 (nZCv daif -PAN -UAO)
>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>> lr : phy_init+0x64/0xcc
>> lr : phy_init+0x64/0xcc
>> ...
>> Kernel panic - not syncing: Asynchronous SError Interrupt
>>
>> Note that before, it printed a warning instead:
>>
>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
>> active children
>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
>> pm_runtime_enable+0x94/0xd8
>>
>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
>> pm_runtime_force_suspend/resume()") fixes the crash.
>
> It looks like what happens without the Ulf's patch is as follows.
>
> usb-phy has children with runtime PM enabled that are not in the
> domain, so without the $subject patch the pm_runtime_force_suspend()
> in genpd_finish_suspend() checks the usage counter of usb-phy and
> since it is 1, the parent's usage counter is not incremented and
> genpd_runtime_suspend() is run for usb-phy. On resume, the
> pm_runtime_force_resume() in genpd_resume_noirq() finds that the usage
> counter of usb-phy is 1, so the parent's usage counter is not
> decremented (correctly) and the function returns (arguably incorrectly
> if the runtime PM status of the children is "active", because it is
> necessary to resume the children in that case, but the children have
> no PM callbacks and even if they had had them, they would have been
> run later anyway). The parent of usb-phy is skipped by the
> pm_runtime_force_resume() too, because its usage counter is 1 when it
> is checked by this function.
And note that it shouldn't be skipped by pm_runtime_force_resume() in
principle, because there are active children under usb-phy.
> With the $subject patch pm_runtime_force_suspend() in
> genpd_finish_suspend() calls pm_runtime_need_not_resume() and that
> returns "false" for usb-phy if the runtime PM status or at least one
> of its children is "active" (which it is for the "phy" devices). That
> is correct, but for this reason the parent's children counter is not
> decremented and both usb-phy and its parent will be resumed by the
> subsequent pm_runtime_force_resume().
>
> The pm_runtime_force_resume() in genpd_resume(_noirq) now finds that
> pm_runtime_need_not_resume() returns "false" for both usb-phy and its
> parent and attempts to resume them both via genpd_runtime_resume()
> which is too early, because stuff they depend on hasn't been resumed
> yet. That triggers the crash (so
> https://patchwork.kernel.org/patch/10152767/ will cause the crash to
> happen too).
>
> In conclusion, without the $subject patch it all works pretty much by
> accident, basically because the pm_runtime_force_resume()
> inadvertently decides to skip the resume of some devices which avoids
> the premature execution of genpd_runtime_resume() for them.
>
>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
>> deployment and fix an issue"
>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
>> does not fix the crash.
>
> I'm not sure why the crash is still there in this case,
It is there, because usb-phy itself is now reference-counted by the
phy layer, so its usage counter is greater than 1 in
genpd_finish_suspend() and its parent's children counter is not
decremented then.
Next, on resume, the pm_runtime_force_resume() in genpd_resume_noirq()
will attempt to resume both usb-phy and its parent via
genpd_runtime_suspend() and that (again) is too early.
I'm not sure, however, why the crash isn't there with
https://www.spinics.net/lists/linux-renesas-soc/msg21719.html alone,
because in theory it should be there too in that case.
Thanks,
Rafael
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven wrote:
> Hi Rafael,
>
> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki
>>
>> One of the limitations of pm_runtime_force_suspend/resume() is that
>> if a parent driver wants to use these functions, all of its child
>> drivers have to do that too because of the parent usage counter
>> manipulations necessary to get the correct state of the parent during
>> system-wide transitions to the working state (system resume).
>> However, that limitation turns out to be artificial, so remove it.
>>
>> Namely, pm_runtime_force_suspend() only needs to update the children
>> counter of its parent (if there's is a parent) when the device can
>> stay in suspend after the subsequent system resume transition, as
>> that counter is correct already otherwise. Now, if the parent's
>> children counter is not updated, it is not necessary to increment
>> the parent's usage counter in that case any more, as long as the
>> children counters of devices are checked along with their usage
>> counters in order to decide whether or not the devices may be left
>> in suspend after the subsequent system resume transition.
>>
>> Accordingly, modify pm_runtime_force_suspend() to only call
>> pm_runtime_set_suspended() for devices whose usage and children
>> counters are at the "no references" level (the runtime PM status
>> of the device needs to be updated to "suspended" anyway in case
>> this function is called once again for the same device during the
>> transition under way), drop the parent usage counter incrementation
>> from it and update pm_runtime_force_resume() to compensate for these
>> changes.
>>
>> Signed-off-by: Rafael J. Wysocki
>
> This patch causes a regression during system resume on Renesas Salvator-XS
> with R-Car H3 ES2.0:
>
> SError Interrupt on CPU3, code 0xbf02 -- SError
> SError Interrupt on CPU2, code 0xbf02 -- SError
> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> Hardware name: Renesas Salvator-X 2nd version board based on
> r8a7795 ES2.0+ (DT)
> Hardware name: Renesas Salvator-X 2nd version board based on
> r8a7795 ES2.0+ (DT)
> Workqueue: events_unbound async_run_entry_fn
> Workqueue: events_unbound async_run_entry_fn
> pstate: 6005 (nZCv daif -PAN -UAO)
> pstate: 6005 (nZCv daif -PAN -UAO)
> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> lr : phy_init+0x64/0xcc
> lr : phy_init+0x64/0xcc
> ...
> Kernel panic - not syncing: Asynchronous SError Interrupt
>
> Note that before, it printed a warning instead:
>
> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
> active children
> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
> pm_runtime_enable+0x94/0xd8
>
> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
> pm_runtime_force_suspend/resume()") fixes the crash.
It looks like what happens without the Ulf's patch is as follows.
usb-phy has children with runtime PM enabled that are not in the
domain, so without the $subject patch the pm_runtime_force_suspend()
in genpd_finish_suspend() checks the usage counter of usb-phy and
since it is 1, the parent's usage counter is not incremented and
genpd_runtime_suspend() is run for usb-phy. On resume, the
pm_runtime_force_resume() in genpd_resume_noirq() finds that the usage
counter of usb-phy is 1, so the parent's usage counter is not
decremented (correctly) and the function returns (arguably incorrectly
if the runtime PM status of the children is "active", because it is
necessary to resume the children in that case, but the children have
no PM callbacks and even if they had had them, they would have been
run later anyway). The parent of usb-phy is skipped by the
pm_runtime_force_resume() too, because its usage counter is 1 when it
is checked by this function.
With the $subject patch pm_runtime_force_suspend() in
genpd_finish_suspend() calls pm_runtime_need_not_resume() and that
returns "false" for usb-phy if the runtime PM status or at least one
of its children is "active" (which it is for the "phy" devices). That
is correct, but for this reason the parent's children counter is not
decremented and both usb-phy and its parent will be resumed by the
subsequent pm_runtime_force_resume().
The pm_runtime_force_resume() in genpd_resume(_noirq) now finds that
pm
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tuesday, January 9, 2018 5:03:18 PM CET Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven
> wrote:
> > Hi Rafael,
> >
> > CC usb
> >
> > On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki wrote:
> >> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
> >> wrote:
> >>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
> >>> wrote:
> >>>> From: Rafael J. Wysocki
> >>>>
> >>>> One of the limitations of pm_runtime_force_suspend/resume() is that
> >>>> if a parent driver wants to use these functions, all of its child
> >>>> drivers have to do that too because of the parent usage counter
> >>>> manipulations necessary to get the correct state of the parent during
> >>>> system-wide transitions to the working state (system resume).
> >>>> However, that limitation turns out to be artificial, so remove it.
> >>>>
> >>>> Namely, pm_runtime_force_suspend() only needs to update the children
> >>>> counter of its parent (if there's is a parent) when the device can
> >>>> stay in suspend after the subsequent system resume transition, as
> >>>> that counter is correct already otherwise. Now, if the parent's
> >>>> children counter is not updated, it is not necessary to increment
> >>>> the parent's usage counter in that case any more, as long as the
> >>>> children counters of devices are checked along with their usage
> >>>> counters in order to decide whether or not the devices may be left
> >>>> in suspend after the subsequent system resume transition.
> >>>>
> >>>> Accordingly, modify pm_runtime_force_suspend() to only call
> >>>> pm_runtime_set_suspended() for devices whose usage and children
> >>>> counters are at the "no references" level (the runtime PM status
> >>>> of the device needs to be updated to "suspended" anyway in case
> >>>> this function is called once again for the same device during the
> >>>> transition under way), drop the parent usage counter incrementation
> >>>> from it and update pm_runtime_force_resume() to compensate for these
> >>>> changes.
> >>>>
> >>>> Signed-off-by: Rafael J. Wysocki
> >>>
> >>> This patch causes a regression during system resume on Renesas Salvator-XS
> >>> with R-Car H3 ES2.0:
> >>
> >> I have dropped it for now, but we need to address the underlying issue.
> >>
> >>> SError Interrupt on CPU3, code 0xbf02 -- SError
> >>> SError Interrupt on CPU2, code 0xbf02 -- SError
> >>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> >>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> >>> Hardware name: Renesas Salvator-X 2nd version board based on
> >>> r8a7795 ES2.0+ (DT)
> >>> Hardware name: Renesas Salvator-X 2nd version board based on
> >>> r8a7795 ES2.0+ (DT)
> >>> Workqueue: events_unbound async_run_entry_fn
> >>> Workqueue: events_unbound async_run_entry_fn
> >>> pstate: 6005 (nZCv daif -PAN -UAO)
> >>> pstate: 6005 (nZCv daif -PAN -UAO)
> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> >>> lr : phy_init+0x64/0xcc
> >>> lr : phy_init+0x64/0xcc
> >>> ...
> >>> Kernel panic - not syncing: Asynchronous SError Interrupt
> >>>
> >>> Note that before, it printed a warning instead:
> >>>
> >>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
> >>> active children
> >>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
> >>> pm_runtime_enable+0x94/0xd8
> >>>
> >>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
> >>> pm_runtime_force_suspend/resume()") fixes the crash.
> >>>
> >>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
> >>> deployment and fix an issue"
> >>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
> >>> does not fix the crash.
>
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tuesday, January 9, 2018 4:00:35 PM CET Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
> wrote:
> > Hi Rafael,
> >
> > On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
> > wrote:
> >> From: Rafael J. Wysocki
> >>
> >> One of the limitations of pm_runtime_force_suspend/resume() is that
> >> if a parent driver wants to use these functions, all of its child
> >> drivers have to do that too because of the parent usage counter
> >> manipulations necessary to get the correct state of the parent during
> >> system-wide transitions to the working state (system resume).
> >> However, that limitation turns out to be artificial, so remove it.
> >>
> >> Namely, pm_runtime_force_suspend() only needs to update the children
> >> counter of its parent (if there's is a parent) when the device can
> >> stay in suspend after the subsequent system resume transition, as
> >> that counter is correct already otherwise. Now, if the parent's
> >> children counter is not updated, it is not necessary to increment
> >> the parent's usage counter in that case any more, as long as the
> >> children counters of devices are checked along with their usage
> >> counters in order to decide whether or not the devices may be left
> >> in suspend after the subsequent system resume transition.
> >>
> >> Accordingly, modify pm_runtime_force_suspend() to only call
> >> pm_runtime_set_suspended() for devices whose usage and children
> >> counters are at the "no references" level (the runtime PM status
> >> of the device needs to be updated to "suspended" anyway in case
> >> this function is called once again for the same device during the
> >> transition under way), drop the parent usage counter incrementation
> >> from it and update pm_runtime_force_resume() to compensate for these
> >> changes.
> >>
> >> Signed-off-by: Rafael J. Wysocki
> >
> > This patch causes a regression during system resume on Renesas Salvator-XS
> > with R-Car H3 ES2.0:
>
> I have dropped it for now, but we need to address the underlying issue.
>
> > SError Interrupt on CPU3, code 0xbf02 -- SError
> > SError Interrupt on CPU2, code 0xbf02 -- SError
> > CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
> > 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> > CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
> > 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> > Hardware name: Renesas Salvator-X 2nd version board based on
> > r8a7795 ES2.0+ (DT)
> > Hardware name: Renesas Salvator-X 2nd version board based on
> > r8a7795 ES2.0+ (DT)
> > Workqueue: events_unbound async_run_entry_fn
> > Workqueue: events_unbound async_run_entry_fn
> > pstate: 6005 (nZCv daif -PAN -UAO)
> > pstate: 6005 (nZCv daif -PAN -UAO)
> > pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> > pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> > lr : phy_init+0x64/0xcc
> > lr : phy_init+0x64/0xcc
> > ...
> > Kernel panic - not syncing: Asynchronous SError Interrupt
> >
> > Note that before, it printed a warning instead:
> >
> > Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
> > active children
> > WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
> > pm_runtime_enable+0x94/0xd8
> >
> > Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
> > pm_runtime_force_suspend/resume()") fixes the crash.
> >
> > Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
> > deployment and fix an issue"
> > (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
> > does not fix the crash.
>
> OK
>
> > With more debug code added, it seems the EHCI module clocks (701-703) are
> > enabled earlier than before. I guess this triggers the workqueue to perform
> > an operation while another related device (HSUSB 704?) is still disabled?
>
> Probably.
>
> Likely a device that wasn't resumed before resumes now and that causes
> the issue to appear.
>
> I'm wondering if adding the ignore_children check to the patch helps.
> If not, there clearly is a resume ordering issue that is papered over
> by the current code.
Below is an alternative version of the patch that caused the problem
to happen which checks the ignore_children flag of the device. It may
avoid the issue by accident, bu
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven wrote:
> Hi Rafael,
>
> CC usb
>
> On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki wrote:
>> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
>> wrote:
>>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
>>> wrote:
>>>> From: Rafael J. Wysocki
>>>>
>>>> One of the limitations of pm_runtime_force_suspend/resume() is that
>>>> if a parent driver wants to use these functions, all of its child
>>>> drivers have to do that too because of the parent usage counter
>>>> manipulations necessary to get the correct state of the parent during
>>>> system-wide transitions to the working state (system resume).
>>>> However, that limitation turns out to be artificial, so remove it.
>>>>
>>>> Namely, pm_runtime_force_suspend() only needs to update the children
>>>> counter of its parent (if there's is a parent) when the device can
>>>> stay in suspend after the subsequent system resume transition, as
>>>> that counter is correct already otherwise. Now, if the parent's
>>>> children counter is not updated, it is not necessary to increment
>>>> the parent's usage counter in that case any more, as long as the
>>>> children counters of devices are checked along with their usage
>>>> counters in order to decide whether or not the devices may be left
>>>> in suspend after the subsequent system resume transition.
>>>>
>>>> Accordingly, modify pm_runtime_force_suspend() to only call
>>>> pm_runtime_set_suspended() for devices whose usage and children
>>>> counters are at the "no references" level (the runtime PM status
>>>> of the device needs to be updated to "suspended" anyway in case
>>>> this function is called once again for the same device during the
>>>> transition under way), drop the parent usage counter incrementation
>>>> from it and update pm_runtime_force_resume() to compensate for these
>>>> changes.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki
>>>
>>> This patch causes a regression during system resume on Renesas Salvator-XS
>>> with R-Car H3 ES2.0:
>>
>> I have dropped it for now, but we need to address the underlying issue.
>>
>>> SError Interrupt on CPU3, code 0xbf02 -- SError
>>> SError Interrupt on CPU2, code 0xbf02 -- SError
>>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
>>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
>>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>>> Hardware name: Renesas Salvator-X 2nd version board based on
>>> r8a7795 ES2.0+ (DT)
>>> Hardware name: Renesas Salvator-X 2nd version board based on
>>> r8a7795 ES2.0+ (DT)
>>> Workqueue: events_unbound async_run_entry_fn
>>> Workqueue: events_unbound async_run_entry_fn
>>> pstate: 6005 (nZCv daif -PAN -UAO)
>>> pstate: 6005 (nZCv daif -PAN -UAO)
>>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>>> lr : phy_init+0x64/0xcc
>>> lr : phy_init+0x64/0xcc
>>> ...
>>> Kernel panic - not syncing: Asynchronous SError Interrupt
>>>
>>> Note that before, it printed a warning instead:
>>>
>>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
>>> active children
>>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
>>> pm_runtime_enable+0x94/0xd8
>>>
>>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
>>> pm_runtime_force_suspend/resume()") fixes the crash.
>>>
>>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
>>> deployment and fix an issue"
>>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
>>> does not fix the crash.
>>
>> OK
>>
>>> With more debug code added, it seems the EHCI module clocks (701-703) are
>>> enabled earlier than before. I guess this triggers the workqueue to perform
>>> an operation while another related device (HSUSB 704?) is still disabled?
>>
>> Probably.
>>
>> Likely a device that wasn't resumed before resumes now and that causes
>> the issue to appear.
>>
>> I'm wondering if a
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven wrote:
> Hi Rafael,
>
> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki
>>
>> One of the limitations of pm_runtime_force_suspend/resume() is that
>> if a parent driver wants to use these functions, all of its child
>> drivers have to do that too because of the parent usage counter
>> manipulations necessary to get the correct state of the parent during
>> system-wide transitions to the working state (system resume).
>> However, that limitation turns out to be artificial, so remove it.
>>
>> Namely, pm_runtime_force_suspend() only needs to update the children
>> counter of its parent (if there's is a parent) when the device can
>> stay in suspend after the subsequent system resume transition, as
>> that counter is correct already otherwise. Now, if the parent's
>> children counter is not updated, it is not necessary to increment
>> the parent's usage counter in that case any more, as long as the
>> children counters of devices are checked along with their usage
>> counters in order to decide whether or not the devices may be left
>> in suspend after the subsequent system resume transition.
>>
>> Accordingly, modify pm_runtime_force_suspend() to only call
>> pm_runtime_set_suspended() for devices whose usage and children
>> counters are at the "no references" level (the runtime PM status
>> of the device needs to be updated to "suspended" anyway in case
>> this function is called once again for the same device during the
>> transition under way), drop the parent usage counter incrementation
>> from it and update pm_runtime_force_resume() to compensate for these
>> changes.
>>
>> Signed-off-by: Rafael J. Wysocki
>
> This patch causes a regression during system resume on Renesas Salvator-XS
> with R-Car H3 ES2.0:
I have dropped it for now, but we need to address the underlying issue.
> SError Interrupt on CPU3, code 0xbf02 -- SError
> SError Interrupt on CPU2, code 0xbf02 -- SError
> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> Hardware name: Renesas Salvator-X 2nd version board based on
> r8a7795 ES2.0+ (DT)
> Hardware name: Renesas Salvator-X 2nd version board based on
> r8a7795 ES2.0+ (DT)
> Workqueue: events_unbound async_run_entry_fn
> Workqueue: events_unbound async_run_entry_fn
> pstate: 6005 (nZCv daif -PAN -UAO)
> pstate: 6005 (nZCv daif -PAN -UAO)
> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> lr : phy_init+0x64/0xcc
> lr : phy_init+0x64/0xcc
> ...
> Kernel panic - not syncing: Asynchronous SError Interrupt
>
> Note that before, it printed a warning instead:
>
> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
> active children
> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
> pm_runtime_enable+0x94/0xd8
>
> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
> pm_runtime_force_suspend/resume()") fixes the crash.
>
> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
> deployment and fix an issue"
> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
> does not fix the crash.
OK
> With more debug code added, it seems the EHCI module clocks (701-703) are
> enabled earlier than before. I guess this triggers the workqueue to perform
> an operation while another related device (HSUSB 704?) is still disabled?
Probably.
Likely a device that wasn't resumed before resumes now and that causes
the issue to appear.
I'm wondering if adding the ignore_children check to the patch helps.
If not, there clearly is a resume ordering issue that is papered over
by the current code.
Thanks,
Rafael
Re: [PATCH] PM / wakeup: Enable option to specify wakeup as a non in-band wakeup
On Tuesday, January 9, 2018 11:54:11 AM CET Ulf Hansson wrote:
> In some cases, a driver may not require its device to be powered on to be
> able to deliver wakeup signals during system suspend.
That quite often is the case.
It is the standard way wakeup signaling works in PCI and in ACPI.
> Likely the most common scenario when this is the case, is a driver routing
> the wakeup signal through a GPIO IRQ, instead of using the regular in-band
> IRQ logic, via the device itself.
>
> Obviously the driver may put its device into a low power state during
> system suspend in cases like this. For example it may gate clocks, put
> pinctrls into sleep state, etc.
>
> However, for middle-layers and PM domains (like genpd), which checks the
> return value from device_may_wakeup() and/or the ->dev.power.wakeup_path
> status flag, there is information missing about scenarios like these.
>
> In the case of genpd, when it finds the ->dev.power.wakeup_path status flag
> being set for a device during system suspend, it leaves the device in
> powered on state including its PM domain. In other words, wasting power.
>
> To address this issue, add a new ->dev.power.no_inband_wakeup flag for the
> device, which drivers may set/clear to inform the PM core, in case
> device_may_wakeup() returns true, whether that shall make the
> ->dev.power.wakeup_path status flag to be set or not for the device.
>
> The PM core checks the ->dev.power.no_inband_wakeup flag in the
> __device_suspend() phase, after invoking the ->suspend() callback for the
> device. At that point, the driver is responsible that it has set a correct
> value to the flag.
>
> Let's also introduce a helper function, device_set_no_inband_wakeup(),
> which drivers shall use to set/clear the ->dev.power.no_inband_wakeup flag.
Wait, wait.
> Signed-off-by: Ulf Hansson
> ---
>
> To get some additional background, one may look at earlier discussions from
> various threads. The most recent is in and RFD [1] from Rafael and from an mmc
> series by Adrian [2].
Let's first conclude that discussion, please.
> [1]
> https://marc.info/?l=linux-pm&m=151541229425689&w=2
>
> [2]
> https://www.spinics.net/lists/linux-mmc/msg47549.html
>
> ---
> drivers/base/power/main.c | 2 +-
> include/linux/pm.h| 1 +
> include/linux/pm_wakeup.h | 8
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 02a497e..376c275 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1794,7 +1794,7 @@ static int __device_suspend(struct device *dev,
> pm_message_t state, bool async)
> End:
> if (!error) {
> dev->power.is_suspended = true;
> - if (device_may_wakeup(dev))
> + if (device_may_wakeup(dev) && !dev->power.no_inband_wakeup)
That is not just a system-wide PM concept. It affects runtime PM potentially
too.
> dev->power.wakeup_path = true;
>
> dpm_propagate_wakeup_to_parent(dev);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78..d131834 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -600,6 +600,7 @@ struct dev_pm_info {
> struct completion completion;
> struct wakeup_source*wakeup;
> boolwakeup_path:1;
> + boolno_inband_wakeup:1;
> boolsyscore:1;
> boolno_pm_callbacks:1; /* Owned by the PM core
> */
> unsigned intmust_resume:1; /* Owned by the PM core */
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 4238dde..5d40887 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -93,6 +93,11 @@ static inline void device_set_wakeup_path(struct device
> *dev)
> dev->power.wakeup_path = true;
> }
>
> +static inline void device_set_no_inband_wakeup(struct device *dev, bool
> wakeup)
> +{
> + dev->power.no_inband_wakeup = wakeup;
> +}
> +
> /* drivers/base/power/wakeup.c */
> extern void wakeup_source_prepare(struct wakeup_source *ws, const char
> *name);
> extern struct wakeup_source *wakeup_source_create(const char *name);
> @@ -181,6 +186,9 @@ static inline bool device_may_wakeup(struct device *dev)
>
> static inline void device_set_wakeup_path(struct device *dev) {}
>
> +static inline void device_set_no_inband_wakeup(struct device *dev,
> +bool wakeup) {}
> +
> static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>
> static inline void pm_stay_awake(struct device *dev) {}
>
Re: [PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep
On Mon, Jan 8, 2018 at 12:13 PM, Ulf Hansson wrote: > On 6 January 2018 at 00:57, Rafael J. Wysocki wrote: >> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson wrote: >>> In general, wakeup settings are not supposed to be changed during any of >>> the system wide PM phases. The reason is simply that it would break >>> guarantees provided by the PM core, to properly act on active wakeup >>> sources. >>> >>> However, there are exceptions to when, in particular, disabling a device as >>> wakeup source makes sense. For example, in cases when a driver realizes >>> that its device is dead during system suspend. For these scenarios, we >>> don't need to care about acting on the wakeup source correctly, because a >>> dead device shouldn't deliver wakeup signals. >>> >>> To this reasoning and to help users to properly manage wakeup settings, >>> let's print a warning in cases someone calls device_wakeup_enable() during >>> system sleep. >>> >>> Suggested-by: Rafael J. Wysocki >>> Signed-off-by: Ulf Hansson >>> --- >>> drivers/base/power/wakeup.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c >>> index b7b8b2f..272c44b 100644 >>> --- a/drivers/base/power/wakeup.c >>> +++ b/drivers/base/power/wakeup.c >>> @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev) >>> if (!dev || !dev->power.can_wakeup) >>> return -EINVAL; >>> >>> + if (pm_suspend_target_state != PM_SUSPEND_ON) >>> + dev_warn(dev, "don't enable as wakup source during >>> sleep!\n"); >>> + >>> ws = wakeup_source_register(dev_name(dev)); >>> if (!ws) >>> return -ENOMEM; >>> -- >> >> It looks like we need to defer this until we fix a couple of issues ... > > Yes. > > Or, perhaps changing from dev_warn() to dev_dbg(), as it could provide > us with some more detailed information, compared to a "git grep" > research? That would work too. Thanks, Rafael
Re: [PATCH v3 3/4] PM / wakeup: Add device_set_wakeup_path() helper to control wakeup path
On Tuesday, January 2, 2018 5:08:52 PM CET Ulf Hansson wrote:
> During system suspend, a driver may find that the wakeup setting is enabled
> for its device and therefore configures it to deliver system wakeup
> signals.
>
> Additionally, sometimes the driver and its device, relies on some further
> consumed resource, like an irqchip or a phy for example, to stay powered
> on, as to be able to deliver system wakeup signals.
>
> In general the driver deals with this, via raising an "enable count" of the
> consumed resource or via a subsystem specific API, like irq_set_irq_wake()
> or enable|disable_irq_wake() for an irqchip. However, this may not be
> sufficient in cases when the resource's device may be attached to a PM
> domain (genpd for example) or is attached to a non-trivial middle layer
> (PCI for example).
>
> To address cases like these, the existing ->dev.power.wakeup_path status
> flag is there to help. As a matter of fact, genpd already monitors the flag
> during system suspend and acts accordingly.
>
> However, so far it has not been clear, if anybody else but the PM core is
> allowed to set the ->dev.power.wakeup_path status flag, which is required
> to make this work. For this reason, let's introduce a new helper function,
> device_set_wakeup_path().
>
> Typically a driver that manages a resource needed in the wakeup path,
> should call device_set_wakeup_path() from its ->suspend() or
> ->suspend_late() callback.
>
> Signed-off-by: Ulf Hansson
> ---
> include/linux/pm_wakeup.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 4c2cba7..4238dde 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -88,6 +88,11 @@ static inline bool device_may_wakeup(struct device *dev)
> return dev->power.can_wakeup && !!dev->power.wakeup;
> }
>
> +static inline void device_set_wakeup_path(struct device *dev)
> +{
> + dev->power.wakeup_path = true;
> +}
> +
> /* drivers/base/power/wakeup.c */
> extern void wakeup_source_prepare(struct wakeup_source *ws, const char
> *name);
> extern struct wakeup_source *wakeup_source_create(const char *name);
> @@ -174,6 +179,8 @@ static inline bool device_may_wakeup(struct device *dev)
> return dev->power.can_wakeup && dev->power.should_wakeup;
> }
>
> +static inline void device_set_wakeup_path(struct device *dev) {}
> +
> static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>
> static inline void pm_stay_awake(struct device *dev) {}
>
Applied, thanks!
Re: [PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep
On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson wrote: > In general, wakeup settings are not supposed to be changed during any of > the system wide PM phases. The reason is simply that it would break > guarantees provided by the PM core, to properly act on active wakeup > sources. > > However, there are exceptions to when, in particular, disabling a device as > wakeup source makes sense. For example, in cases when a driver realizes > that its device is dead during system suspend. For these scenarios, we > don't need to care about acting on the wakeup source correctly, because a > dead device shouldn't deliver wakeup signals. > > To this reasoning and to help users to properly manage wakeup settings, > let's print a warning in cases someone calls device_wakeup_enable() during > system sleep. > > Suggested-by: Rafael J. Wysocki > Signed-off-by: Ulf Hansson > --- > drivers/base/power/wakeup.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index b7b8b2f..272c44b 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev) > if (!dev || !dev->power.can_wakeup) > return -EINVAL; > > + if (pm_suspend_target_state != PM_SUSPEND_ON) > + dev_warn(dev, "don't enable as wakup source during sleep!\n"); > + > ws = wakeup_source_register(dev_name(dev)); > if (!ws) > return -ENOMEM; > -- It looks like we need to defer this until we fix a couple of issues ... Thanks, Rafael
Re: [PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()
On Fri, Jan 5, 2018 at 6:12 PM, Ulf Hansson wrote:
> On 5 January 2018 at 13:57, Rafael J. Wysocki wrote:
>> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson wrote:
>>> Currently the wakeup_path status flag becomes propagated from a child
>>> device to its parent device at __device_suspend(). This allows a driver
>>> dealing with a parent device to act on the flag from its ->suspend()
>>> callback.
>>>
>>> However, in situations when the wakeup_path status flag needs to be set
>>> from a ->suspend_late() callback, its value doesn't get propagated to the
>>> parent by the PM core. Let's address this limitation, by also propagating
>>> the flag at __device_suspend_late().
>>
>> This doesn't need to be done twice, so doing it once in
>> __device_suspend_late() should be sufficient.
>
> Unfortunately no.
>
> For example that would break drivers/ssb/pcihost_wrapper.c, as it uses
> the flag from its ->suspend() callback.
But what drivers/ssb/pcihost_wrapper.c does is just wrong!
I guess we'd need a special variant of pci_prepare_to_sleep() with an
extra "wakeup" arg for it to do the right thing and I don't see why it
cannot do all that in ->suspend_late.
> Also, I expect there may be more cases when the flag may be useful to
> check from a ->suspend() callback, rather than from a ->suspend_late()
> (or in later phase).
I'm not inclined to believe it until I see a convincing example. :-)
And it obviously won't take the later updates of power.wakeup_path into account.
Anyway, for the time being please at least add a comment next to the
first invocation of this routine to explain why it is needed in there.
>
>>
>>> Signed-off-by: Ulf Hansson
>>> ---
>>> drivers/base/power/main.c | 33 +
>>> 1 file changed, 17 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index 7aeb91d..612bf1b 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1476,6 +1476,22 @@ static pm_callback_t
>>> dpm_subsys_suspend_late_cb(struct device *dev,
>>> return callback;
>>> }
>>>
>>> +static void dpm_propagate_to_parent(struct device *dev)
>>> +{
>>> + struct device *parent = dev->parent;
>>> +
>>> + if (!parent)
>>> + return;
>>> +
>>> + spin_lock_irq(&parent->power.lock);
>>> +
>>> + parent->power.direct_complete = false;
>>> + if (dev->power.wakeup_path && !parent->power.ignore_children)
>>> + parent->power.wakeup_path = true;
>>> +
>>> + spin_unlock_irq(&parent->power.lock);
>>> +}
>>
>> Clearing the parent's direct_complete in __device_suspend_late() is
>> incorrect, because it potentially overwrites the value previously used
>> by __device_suspend() for the parent.
>
> That is already taken care of in __device_suspend_late(), with the below
> check:
>
> if (dev->power.syscore || dev->power.direct_complete)
> goto Complete;
>
> In other words, the dpm_propagate_to_parent() isn't called when
> dev->power.direct_complete is set.
Which means that the parent's direct_complete is only cleared when it
is unset already, so this isn't incorrect, but just pointless.
Well, "pointless" doesn't really sound good to me too ...
>>
>> IMO it also need not be done under parent->power.lock, however,
>> because the other parent's power. flags should not be updated in
>> parallel with this update anyway, so maybe just move the parent's
>> direct_complete update to __device_suspend(), rename
>> dpm_propagate_to_parent() to something like
>> dpm_propagate_wakeup_to_parent() and call it from
>> __device_suspend_late() only?
>
> I can do this, if you think this is more clear, but according to the
> above, it's shouldn't be needed.
Yes, please.
Apart from avoiding a pointless update of the parent's direct_complete
I think that it's better to keep it close to the update of
direct_complete for suppliers.
Thanks,
Rafael
Re: [PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()
On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson wrote:
> Currently the wakeup_path status flag becomes propagated from a child
> device to its parent device at __device_suspend(). This allows a driver
> dealing with a parent device to act on the flag from its ->suspend()
> callback.
>
> However, in situations when the wakeup_path status flag needs to be set
> from a ->suspend_late() callback, its value doesn't get propagated to the
> parent by the PM core. Let's address this limitation, by also propagating
> the flag at __device_suspend_late().
This doesn't need to be done twice, so doing it once in
__device_suspend_late() should be sufficient.
> Signed-off-by: Ulf Hansson
> ---
> drivers/base/power/main.c | 33 +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 7aeb91d..612bf1b 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1476,6 +1476,22 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct
> device *dev,
> return callback;
> }
>
> +static void dpm_propagate_to_parent(struct device *dev)
> +{
> + struct device *parent = dev->parent;
> +
> + if (!parent)
> + return;
> +
> + spin_lock_irq(&parent->power.lock);
> +
> + parent->power.direct_complete = false;
> + if (dev->power.wakeup_path && !parent->power.ignore_children)
> + parent->power.wakeup_path = true;
> +
> + spin_unlock_irq(&parent->power.lock);
> +}
Clearing the parent's direct_complete in __device_suspend_late() is
incorrect, because it potentially overwrites the value previously used
by __device_suspend() for the parent.
IMO it also need not be done under parent->power.lock, however,
because the other parent's power. flags should not be updated in
parallel with this update anyway, so maybe just move the parent's
direct_complete update to __device_suspend(), rename
dpm_propagate_to_parent() to something like
dpm_propagate_wakeup_to_parent() and call it from
__device_suspend_late() only?
Thanks,
Rafael
Re: [PATCH v3 1/4] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson wrote:
> The PM core in the device_prepare() phase, resets the wakeup_path status
> flag to the value of device_may_wakeup(). This means if a ->prepare() or a
> ->suspend() callback for the device would update the device's wakeup
> setting, this doesn't become reflected in the wakeup_path status flag.
>
> In general this isn't a problem, because wakeup settings are not supposed
> to be changed (via for example calling device_set_wakeup_enable()) during
> any system wide suspend/resume phase. Nevertheless there are some users,
> which can be considered as legacy, that don't conform to this behaviour.
>
> These legacy cases should be corrected, however until that is done, let's
> address the issue from the PM core, by moving the assignment of the
> wakeup_path status flag to the __device_suspend() phase and after the
> ->suspend() callback has been invoked.
>
> Signed-off-by: Ulf Hansson
I'm queuing this one up with an extra empty line added.
> ---
> drivers/base/power/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 70398e7..7aeb91d 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1788,6 +1788,8 @@ static int __device_suspend(struct device *dev,
> pm_message_t state, bool async)
> End:
> if (!error) {
> dev->power.is_suspended = true;
> + if (device_may_wakeup(dev))
> + dev->power.wakeup_path = true;
> dpm_propagate_to_parent(dev);
> dpm_clear_suppliers_direct_complete(dev);
> }
> @@ -1912,7 +1914,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state)
>
> device_lock(dev);
>
> - dev->power.wakeup_path = device_may_wakeup(dev);
> + dev->power.wakeup_path = false;
>
> if (dev->power.no_pm_callbacks) {
> ret = 1;/* Let device go direct_complete */
> --
> 2.7.4
>
Re: [PATCH v2 3/4] PM / Domains: Take WAKEUP_PATH driver flag into account in genpd
On Tue, Jan 2, 2018 at 12:46 PM, Ulf Hansson wrote: > On 30 December 2017 at 01:47, Rafael J. Wysocki wrote: >> On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson wrote: >>> In case the WAKEUP_PATH flag has been set in a later phase than from the >>> ->suspend() callback, the PM core don't set the ->power.wakeup_path status >>> flag for the device. Therefore, let's be safe and check it explicitly. >>> >>> Signed-off-by: Ulf Hansson >>> --- >>> drivers/base/power/domain.c | 8 ++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >>> index f9dcc98..32b4ba7 100644 >>> --- a/drivers/base/power/domain.c >>> +++ b/drivers/base/power/domain.c >>> @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, >>> bool poweroff) >>> if (IS_ERR(genpd)) >>> return -EINVAL; >>> >>> - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) >>> + if ((dev->power.wakeup_path || >>> + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && >> >> Shouldn't dev->power.wakeup_path be always set if DPM_FLAG_WAKEUP_PATH >> is set as per the second patch in the series? > > Not if DPM_FLAG_WAKEUP_PATH is set from a driver's ->suspend_late() callback. > > To do that, the PM core would need to be adopted to set/propagate the > "wakeup_path" flag at __device_suspend_late(), similar to what is done > at __device_suspend(). Why not? The wakeup_path flag is only used during the "noirq" phase, so it can be propagated to parent at the end of __device_suspend_late() instead of doing that in __device_suspend() even. Thanks, Rafael
Re: [PATCH v2 1/4] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Tuesday, January 2, 2018 12:36:55 PM CET Ulf Hansson wrote:
> On 30 December 2017 at 01:44, Rafael J. Wysocki wrote:
> > On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson
> > wrote:
> >> The PM core in the device_prepare() phase, resets the wakeup_path status
> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a
> >> ->suspend() callback for the device would update the device's wakeup
> >> setting, this doesn't become reflected in the wakeup_path status flag.
> >>
> >> In general this isn't a problem, because wakeup settings are not supposed
> >> to be changed (via for example calling device_set_wakeup_enable()) during
> >> any system wide suspend/resume phase. Nevertheless there are some users,
> >> which can be considered as legacy, that don't conform to this behaviour.
> >>
> >> These legacy cases should be corrected, however until that is done, let's
> >> address the issue from the PM core, by moving the assignment of the
> >> wakeup_path status flag to the __device_suspend() phase and after the
> >> ->suspend() callback has been invoked.
> >>
> >> Signed-off-by: Ulf Hansson
> >> ---
> >> drivers/base/power/main.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index 6e8cc5d..810e5fb 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev,
> >> pm_message_t state, bool async)
> >> End:
> >> if (!error) {
> >> dev->power.is_suspended = true;
> >> + if (device_may_wakeup(dev))
> >> + dev->power.wakeup_path = true;
> >> dpm_propagate_to_parent(dev);
> >> dpm_clear_suppliers_direct_complete(dev);
> >> }
> >> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev,
> >> pm_message_t state)
> >>
> >> device_lock(dev);
> >>
> >> - dev->power.wakeup_path = device_may_wakeup(dev);
> >> + dev->power.wakeup_path = false;
> >
> > If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in
> > __device_suspend(), it wouldn't need to be cleared here I guess?
> >
>
> No that doesn't work, because it may override the value of the flag
> wrongly, in case a child's wakeup_path flag that is set and has been
> propagated to the parent.
I see, OK.
So it looks like the new driver flag is not really necessary.
The wakeup_path propagation may be done even later, like at the end of
__device_suspend_late(), which is fine, because it is not going to be used
before __device_suspend_noirq() anyway.
Then, drivers can set it from their ->suspend and ->suspend_late callbacks.
Thanks,
Rafael
Re: [PATCH v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag
On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven
wrote:
> Hi Rafael,
>
> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki wrote:
>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote:
>>> From: Geert Uytterhoeven
>>>
>>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable
>>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's
>>> module clock (if exists) is manually kept running during system suspend, to
>>> make sure the device stays active.
>>>
>>> However, this explicit clock handling is merely a workaround for a failure
>>> to properly communicate wakeup information to the PM core. Instead, set the
>>> WAKEUP_PATH driver PM flag to indicate that the device is part of the
>>> wakeup path, which further also enables middle-layers and PM domains (like
>>> genpd) to act on this.
>>>
>>> In case the device is attached to genpd and depending on if it has an
>>> active wakeup configuration, genpd will keep the device active (the clock
>>> running) during system suspend when needed. This enables us to remove all
>>> explicit clock handling code from the driver, so let's do that as well.
>>>
>>> Signed-off-by: Geert Uytterhoeven
>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]
>
> Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n?
>
>>> Signed-off-by: Ulf Hansson
>
>>> --- a/drivers/gpio/gpio-rcar.c
>>> +++ b/drivers/gpio/gpio-rcar.c
>
>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
>>> *p, unsigned int *npins)
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int gpio_rcar_suspend(struct device *dev)
>>> +{
>>> + struct gpio_rcar_priv *p = dev_get_drvdata(dev);
>>> +
>>> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH
>>> : 0);
>>
>> Why don't you simply set dev->power.wakeup_path here?
>
> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/).
I very much prefer this one. :-)
What's wrong with it?
Thanks,
Rafael
Re: [PATCH v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag
On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote:
> From: Geert Uytterhoeven
>
> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable
> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's
> module clock (if exists) is manually kept running during system suspend, to
> make sure the device stays active.
>
> However, this explicit clock handling is merely a workaround for a failure
> to properly communicate wakeup information to the PM core. Instead, set the
> WAKEUP_PATH driver PM flag to indicate that the device is part of the
> wakeup path, which further also enables middle-layers and PM domains (like
> genpd) to act on this.
>
> In case the device is attached to genpd and depending on if it has an
> active wakeup configuration, genpd will keep the device active (the clock
> running) during system suspend when needed. This enables us to remove all
> explicit clock handling code from the driver, so let's do that as well.
>
> Signed-off-by: Geert Uytterhoeven
> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]
> Signed-off-by: Ulf Hansson
> ---
> drivers/gpio/gpio-rcar.c | 40 +++-
> 1 file changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index e76de57..d414511 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -14,7 +14,6 @@
> * GNU General Public License for more details.
> */
>
> -#include
> #include
> #include
> #include
> @@ -37,10 +36,9 @@ struct gpio_rcar_priv {
> struct platform_device *pdev;
> struct gpio_chip gpio_chip;
> struct irq_chip irq_chip;
> - struct clk *clk;
> unsigned int irq_parent;
> bool has_both_edge_trigger;
> - bool needs_clk;
> + bool wakeup_path;
> };
>
> #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */
> @@ -186,14 +184,7 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d,
> unsigned int on)
> }
> }
>
> - if (!p->clk)
> - return 0;
> -
> - if (on)
> - clk_enable(p->clk);
> - else
> - clk_disable(p->clk);
> -
> + p->wakeup_path = on;
> return 0;
> }
>
> @@ -330,17 +321,14 @@ static int gpio_rcar_direction_output(struct gpio_chip
> *chip, unsigned offset,
>
> struct gpio_rcar_info {
> bool has_both_edge_trigger;
> - bool needs_clk;
> };
>
> static const struct gpio_rcar_info gpio_rcar_info_gen1 = {
> .has_both_edge_trigger = false,
> - .needs_clk = false,
> };
>
> static const struct gpio_rcar_info gpio_rcar_info_gen2 = {
> .has_both_edge_trigger = true,
> - .needs_clk = true,
> };
>
> static const struct of_device_id gpio_rcar_of_table[] = {
> @@ -403,7 +391,6 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p,
> unsigned int *npins)
> ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
> &args);
> *npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
> p->has_both_edge_trigger = info->has_both_edge_trigger;
> - p->needs_clk = info->needs_clk;
>
> if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
> dev_warn(&p->pdev->dev,
> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p,
> unsigned int *npins)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int gpio_rcar_suspend(struct device *dev)
> +{
> + struct gpio_rcar_priv *p = dev_get_drvdata(dev);
> +
> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH :
> 0);
Why don't you simply set dev->power.wakeup_path here?
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, NULL);
> +
> static int gpio_rcar_probe(struct platform_device *pdev)
> {
> struct gpio_rcar_priv *p;
Thanks,
Rafael
Re: [PATCH v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag
On Tue, Jan 2, 2018 at 10:37 AM, Geert Uytterhoeven
wrote:
> Hi Linus,
>
> On Tue, Jan 2, 2018 at 10:27 AM, Linus Walleij
> wrote:
>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote:
>>> From: Geert Uytterhoeven
>>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable
>>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's
>>> module clock (if exists) is manually kept running during system suspend, to
>>> make sure the device stays active.
>>>
>>> However, this explicit clock handling is merely a workaround for a failure
>>> to properly communicate wakeup information to the PM core. Instead, set the
>>> WAKEUP_PATH driver PM flag to indicate that the device is part of the
>>> wakeup path, which further also enables middle-layers and PM domains (like
>>> genpd) to act on this.
>>>
>>> In case the device is attached to genpd and depending on if it has an
>>> active wakeup configuration, genpd will keep the device active (the clock
>>> running) during system suspend when needed. This enables us to remove all
>>> explicit clock handling code from the driver, so let's do that as well.
>>>
>>> Signed-off-by: Geert Uytterhoeven
>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]
>>> Signed-off-by: Ulf Hansson
>>
>> Acked-by: Linus Walleij
>>
>> I guess it is dependent on the other patches?
>
> Yes, it depends on (a) a clock patch queued in clk-next for v4.16, and (b) a
> PM
> patch introducing WAKEUP_PATH. Applying it prematurely will cause build
> or runtime issues.
Plus, I'm not going to apply the WAKEUP_PATH thing just yet.
At least not in its current version.
Thanks,
Rafael
Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag
On Sun, Dec 31, 2017 at 10:22 AM, Geert Uytterhoeven wrote: > Hi Rafael, > > On Sun, Dec 31, 2017 at 1:56 AM, Rafael J. Wysocki wrote: >> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote: >>> From: Geert Uytterhoeven >>> >>> Changes in v2: [By Ulf Hansson] >>> - I have picked up the series from Geert [1] and converted it into >>> use >>> the WAKEUP_PATH driver PM flag. This includes some minor changes to >>> each >>> patch and updates to the changelogs. >>> - An important note, the WAKEUP_PATH driver PM flag is introduced >>> in a >>> separate series [2], not yet applied, so @subject series depends on >>> it. >>> - One more note, two of the patches has a checkpatch error, however >>> I >>> did not fix them, becuase I think that should be done separate. >>> >>> [1] >>> https://lkml.org/lkml/2017/11/9/382 >>> [2] >>> https://marc.info/?l=linux-pm&m=151454744124661&w=2 >>> >>> More information below, picked from Geert's previous cover letter. >>> >>> Kind regards >>> Uffe >>> >>> >>> Hi all, >>> >>> If an interrupt controller in a Renesas ARM SoC is part of a Clock >>> Domain, and it is part of the wakeup path, it must be kept active during >>> system suspend. >>> >>> Currently this is handled in all interrupt controller drivers by >>> explicitly increasing the use count of the module clock when the device >>> is part of the wakeup path. However, this explicit clock handling is >>> merely a workaround for a failure to properly communicate wakeup >>> information to the device core. >>> >>> Hence this series fixes the affected drivers by setting the devices' >>> power.wakeup_path fields instead, to indicate they are part of the >>> wakeup path. Depending on the PM Domain's active_wakeup configuration, >>> the genpd core code will keep the device enabled (and the clock running) >>> during system suspend when needed. >> >> However, there is a convention, documented in the kerneldoc comment of >> device_init_wakeup(), by which devices participating in system wakeup >> "passively" (like USB controllers and hubs) are expected to have it >> enabled by default. >> >> If that convention was followed by the devices in question here, the >> wakeup_path bit would be set for them and no other code changes would >> be necessary. So is there any reason for not following it? > > Yes there is. The need to stay enabled during system suspend depends > on the consumer of the interrupt. It is controlled by the consumer using > the irq_chip.irq_set_wake() callback at runtime, and may change at runtime. > > If the wakeup_path flag is always set, the interrupt controller will > never be suspended during system suspend, and thus waste power. OK For IRQ chips in particular, I think, you don't need add new fields to struct dev_pm_info to make it work. In ->suspend (or ->suspend_late, which may be better) you can check the IRQD_WAKEUP_STATE flag of the irq_desc associated with the pin. If that is set, you can simply set power.wakeup_path for the device and that will make genpd skip it. Wouldn't that work? Thanks, Rafael
Re: [PATCH v2 1/3] irqchip/renesas-intc-irqpin: Use WAKEUP_PATH driver PM flag
On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote:
> From: Geert Uytterhoeven
>
> Since commit 705bc96c2c15313c ("irqchip: renesas-intc-irqpin: Add minimal
> runtime PM support"), when an IRQ is used for wakeup, the INTC block's
> module clock (if exists) is manually kept running during system suspend, to
> make sure the device stays active.
>
> However, this explicit clock handling is merely a workaround for a failure
> to properly communicate wakeup information to the PM core. Instead, set the
> WAKEUP_PATH driver PM flag to indicate that the device is part of the
> wakeup path, which further also enables middle-layers and PM domains (like
> genpd) to act on this.
>
> In case the device is attached to genpd and depending on if it has an
> active wakeup configuration, genpd will keep the device active (the clock
> running) during system suspend when needed. This enables us to remove all
> explicit clock handling code from the driver, so let's do that as well.
>
> Signed-off-by: Geert Uytterhoeven
> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]
> Signed-off-by: Ulf Hansson
> ---
> drivers/irqchip/irq-renesas-intc-irqpin.c | 42
> +++
> 1 file changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c
> b/drivers/irqchip/irq-renesas-intc-irqpin.c
> index 06f29cf..bfc2c5c 100644
> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> @@ -17,7 +17,6 @@
> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> */
>
> -#include
> #include
> #include
> #include
> @@ -78,16 +77,14 @@ struct intc_irqpin_priv {
> struct platform_device *pdev;
> struct irq_chip irq_chip;
> struct irq_domain *irq_domain;
> - struct clk *clk;
> unsigned shared_irqs:1;
> - unsigned needs_clk:1;
> + unsigned wakeup_path:1;
> u8 shared_irq_mask;
> };
>
> struct intc_irqpin_config {
> unsigned int irlm_bit;
> unsigned needs_irlm:1;
> - unsigned needs_clk:1;
> };
>
> static unsigned long intc_irqpin_read32(void __iomem *iomem)
> @@ -287,15 +284,7 @@ static int intc_irqpin_irq_set_wake(struct irq_data *d,
> unsigned int on)
> int hw_irq = irqd_to_hwirq(d);
>
> irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
> -
> - if (!p->clk)
> - return 0;
> -
> - if (on)
> - clk_enable(p->clk);
> - else
> - clk_disable(p->clk);
> -
> + p->wakeup_path = on;
> return 0;
> }
>
> @@ -365,12 +354,10 @@ static const struct irq_domain_ops
> intc_irqpin_irq_domain_ops = {
> static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
> .irlm_bit = 23, /* ICR0.IRLM0 */
> .needs_irlm = 1,
> - .needs_clk = 0,
> };
>
> static const struct intc_irqpin_config intc_irqpin_rmobile = {
> .needs_irlm = 0,
> - .needs_clk = 1,
> };
>
> static const struct of_device_id intc_irqpin_dt_ids[] = {
> @@ -422,18 +409,6 @@ static int intc_irqpin_probe(struct platform_device
> *pdev)
> platform_set_drvdata(pdev, p);
>
> config = of_device_get_match_data(dev);
> - if (config)
> - p->needs_clk = config->needs_clk;
> -
> - p->clk = devm_clk_get(dev, NULL);
> - if (IS_ERR(p->clk)) {
> - if (p->needs_clk) {
> - dev_err(dev, "unable to get clock\n");
> - ret = PTR_ERR(p->clk);
> - goto err0;
> - }
> - p->clk = NULL;
> - }
>
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> @@ -602,12 +577,25 @@ static int intc_irqpin_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int intc_irqpin_suspend(struct device *dev)
> +{
> + struct intc_irqpin_priv *p = dev_get_drvdata(dev);
> +
> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH :
> 0);
If you want that thing to be a DPM_FLAG_, then please follow the rule
that these flags are only set once at the probe time.
> + return 0;
> +}
> +#endif
Thanks,
Rafael
Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag
On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson wrote: > From: Geert Uytterhoeven > > Changes in v2: [By Ulf Hansson] > - I have picked up the series from Geert [1] and converted it into use > the WAKEUP_PATH driver PM flag. This includes some minor changes to > each > patch and updates to the changelogs. > - An important note, the WAKEUP_PATH driver PM flag is introduced in a > separate series [2], not yet applied, so @subject series depends on > it. > - One more note, two of the patches has a checkpatch error, however I > did not fix them, becuase I think that should be done separate. > > [1] > https://lkml.org/lkml/2017/11/9/382 > [2] > https://marc.info/?l=linux-pm&m=151454744124661&w=2 > > More information below, picked from Geert's previous cover letter. > > Kind regards > Uffe > > > Hi all, > > If an interrupt controller in a Renesas ARM SoC is part of a Clock > Domain, and it is part of the wakeup path, it must be kept active during > system suspend. > > Currently this is handled in all interrupt controller drivers by > explicitly increasing the use count of the module clock when the device > is part of the wakeup path. However, this explicit clock handling is > merely a workaround for a failure to properly communicate wakeup > information to the device core. > > Hence this series fixes the affected drivers by setting the devices' > power.wakeup_path fields instead, to indicate they are part of the > wakeup path. Depending on the PM Domain's active_wakeup configuration, > the genpd core code will keep the device enabled (and the clock running) > during system suspend when needed. However, there is a convention, documented in the kerneldoc comment of device_init_wakeup(), by which devices participating in system wakeup "passively" (like USB controllers and hubs) are expected to have it enabled by default. If that convention was followed by the devices in question here, the wakeup_path bit would be set for them and no other code changes would be necessary. So is there any reason for not following it? Thanks, Rafael
Re: [PATCH v2 4/4] PM / core: Print warn if device gets enabled as wakeup source during sleep
On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson wrote:
> In general, wakeup settings are not supposed to be changed during any of
> the system wide PM phases. The reason is simply that it would break
> guarantees provided by the PM core, to properly act on active wakeup
> sources.
>
> However, there are exceptions to when, in particular, disabling a device as
> wakeup source makes sense. For example, in cases when a driver realizes
> that its device is dead during system suspend. For these scenarios, we
> don't need to care about acting on the wakeup source correctly, because a
> dead device shouldn't deliver wakeup signals.
>
> To this reasoning and to help users to properly manage wakeup settings,
> let's print a warning in cases someone calls device_wakeup_enable() during
> system sleep.
>
> Signed-off-by: Ulf Hansson
> ---
> drivers/base/power/main.c | 4
> drivers/base/power/wakeup.c | 4
> include/linux/pm.h | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 1327726..6b0d312 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1030,6 +1030,8 @@ static void device_complete(struct device *dev,
> pm_message_t state)
> callback(dev);
> }
>
> + dev->power.may_set_wakeup = dev->power.can_wakeup;
> +
> device_unlock(dev);
>
> pm_runtime_put(dev);
> @@ -1747,6 +1749,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state)
>
> device_lock(dev);
>
> + dev->power.may_set_wakeup = false;
> dev->power.wakeup_path = false;
>
> if (dev->power.no_pm_callbacks) {
> @@ -1775,6 +1778,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state)
> if (ret < 0) {
> suspend_report_result(callback, ret);
> pm_runtime_put(dev);
> + dev->power.may_set_wakeup = dev->power.can_wakeup;
> return ret;
> }
> /*
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index cb72965..5445bea 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev)
> if (!dev || !dev->power.can_wakeup)
> return -EINVAL;
>
> + if (!dev->power.may_set_wakeup)
> + dev_warn(dev, "don't enable as wakup source during sleep!\n");
> +
> ws = wakeup_source_register(dev_name(dev));
> if (!ws)
> return -ENOMEM;
> @@ -413,6 +416,7 @@ void device_set_wakeup_capable(struct device *dev, bool
> capable)
> return;
>
> dev->power.can_wakeup = capable;
> + dev->power.may_set_wakeup = capable;
> if (device_is_registered(dev) && !list_empty(&dev->power.entry)) {
> if (capable) {
> int ret = wakeup_sysfs_add(dev);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index ebc6ef8..106fb15 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -606,6 +606,7 @@ struct dev_pm_info {
> struct list_headentry;
> struct completion completion;
> struct wakeup_source*wakeup;
> + boolmay_set_wakeup:1;
> boolwakeup_path:1;
> boolsyscore:1;
> boolno_pm_callbacks:1; /* Owned by the PM
> core */
> --
I didn't mean anything so complicated. :-)
Just a warning message if (pm_suspend_target_state != PM_SUSPEND_ON)
in device_wakeup_enable() (or equivalent) should be sufficient.
Re: [PATCH v2 3/4] PM / Domains: Take WAKEUP_PATH driver flag into account in genpd
On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson wrote: > In case the WAKEUP_PATH flag has been set in a later phase than from the > ->suspend() callback, the PM core don't set the ->power.wakeup_path status > flag for the device. Therefore, let's be safe and check it explicitly. > > Signed-off-by: Ulf Hansson > --- > drivers/base/power/domain.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index f9dcc98..32b4ba7 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, > bool poweroff) > if (IS_ERR(genpd)) > return -EINVAL; > > - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) > + if ((dev->power.wakeup_path || > + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && Shouldn't dev->power.wakeup_path be always set if DPM_FLAG_WAKEUP_PATH is set as per the second patch in the series? > + genpd_is_active_wakeup(genpd)) > return 0; > > if (poweroff) > @@ -1093,7 +1095,9 @@ static int genpd_resume_noirq(struct device *dev) > if (IS_ERR(genpd)) > return -EINVAL; > > - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) > + if ((dev->power.wakeup_path || > + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && > + genpd_is_active_wakeup(genpd)) > return 0; > > genpd_lock(genpd); > -- > 2.7.4 >
Re: [PATCH v2 1/4] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson wrote:
> The PM core in the device_prepare() phase, resets the wakeup_path status
> flag to the value of device_may_wakeup(). This means if a ->prepare() or a
> ->suspend() callback for the device would update the device's wakeup
> setting, this doesn't become reflected in the wakeup_path status flag.
>
> In general this isn't a problem, because wakeup settings are not supposed
> to be changed (via for example calling device_set_wakeup_enable()) during
> any system wide suspend/resume phase. Nevertheless there are some users,
> which can be considered as legacy, that don't conform to this behaviour.
>
> These legacy cases should be corrected, however until that is done, let's
> address the issue from the PM core, by moving the assignment of the
> wakeup_path status flag to the __device_suspend() phase and after the
> ->suspend() callback has been invoked.
>
> Signed-off-by: Ulf Hansson
> ---
> drivers/base/power/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 6e8cc5d..810e5fb 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev,
> pm_message_t state, bool async)
> End:
> if (!error) {
> dev->power.is_suspended = true;
> + if (device_may_wakeup(dev))
> + dev->power.wakeup_path = true;
> dpm_propagate_to_parent(dev);
> dpm_clear_suppliers_direct_complete(dev);
> }
> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state)
>
> device_lock(dev);
>
> - dev->power.wakeup_path = device_may_wakeup(dev);
> + dev->power.wakeup_path = false;
If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in
__device_suspend(), it wouldn't need to be cleared here I guess?
>
> if (dev->power.no_pm_callbacks) {
> ret = 1;/* Let device go direct_complete */
> --
> 2.7.4
>
Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Saturday, December 23, 2017 4:17:58 PM CET Ulf Hansson wrote: > [...] > > How many drivers actually do call device_set_wakeup_enable() during > suspend? > >>> > >>> There are some ethernet/wifi drivers, although it hard to say how many > >>> without a more thorough investigation. > >>> > >>> Besides those I found these more obvious ones: > >>> drivers/ssb/pcihost_wrapper.c > >>> drivers/staging/rtlwifi/core.c > >>> drivers/tty/serial/atmel_serial.c > >>> drivers/usb/core/hcd-pci.c > >> > >> Ugh. I need to look at the last one at least ... > > > > The drivers/usb/core/hcd-pci.c instance is actually valid, because it > > calls device_set_wakeup_enable() to remove the wakeup source object in > > case the device is dead. Moreover, it does that in the "noirq" phase > > which is not covered by your patch anyway. > > Right, but this puzzles me a bit. > > Could you explain what you mean by valid here? Is it okay to call > device_set_wakeup_enable() in the suspend phase after all? No? This is an exception and it would be better to call device_wakeup_disable() directly here. In general, calling device_set_wakeup_enable() during system suspend (to "enable" device wakeup) and then during system resume (to "disable" it) may be problematic. First, if device_wakeup_enable() is run during system suspend, it will create a wakeup source object, which then, quite obviously, can only cover the time after its creation, so there is a window (between the start of the system suspend transition and the creation of the wakeup source) during which wakeup events may be missed. That is not only relevant for devices supporting runtime PM, because input device drivers, say, may choose to report system wakeup events every time they see input (eg. from their interrupt handlers or similar) and wakeup sources used for that should be present before system suspend begins. Also, if the wakeup source is not present already, this effectively overrides the choice made by user space which didn't set the device's "wakeup" attribute in sysfs to "enabled", so it should not wake up the system. Second, if device_wakeup_disable() is run during system resume, it effectively destroys information that can be used to figure out why the system woke up and, again, it may cause the choice made by user space to be effectively overridden. However, if the device is dead, calling device_wakeup_disable() on it is fine at any time (as dead devices do not wake up the system as a rule). > My very vague idea at this point is, if we find cases where > device_set_wakeup_enable() makes sense to call during system suspend, > probably those could/should be replaces by using the "wakeup_path" > flag instead, somehow. Not really in this particular case. Clearing the wakeup enable bit for the device plays the role of "don't bother any more" here. Thanks, Rafael
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote: > [...] > > > > > So IMO the changes you are proposing make sense regardless of the > > genpd issue, because they generally simplify the phy code, but the > > additional use_runtime_pm field in struct phy represents redundant > > information (manipulating reference counters shouldn't matter if > > runtime PM is disabled), so it doesn't appear to be necessary. > > > > Actually, the first version I posted treated the return codes from > pm_runtime_get_sync() according to your suggestion above. > > However, Kishon pointed out that it didn't work. That's because, there > are phy provider drivers that enables runtime PM *after* calling > phy_create(). And in those cases, that is because they want to treat > runtime PM themselves. > > I think that's probably something we should look into to change, but I > find it being a separate issue, that I didn't want to investigate as > part of this series. > > See more about the thread here: > https://www.spinics.net/lists/linux-renesas-soc/msg21711.html Even so, it shouldn't matter when the provider enables runtime PM. Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as long as they are balanced properly regardless of whether or not runtime PM is enabled for the device or it is enabled in the meantime. Of course, the initial runtime PM status of the device must reflect the values of the usage counters, but that should not be too hard to ensure. The reason why it matters here is because the phy core tries to be smart about manipulating reference counters by itself and that's a mistake. So it looks to me like the whole thing needs to be reworked and yes, that should be done in the first place IMO, because it will make the issue with genpd go away automatically. [Why is phy_pm_runtime_get() there at all, for instance? It seems to have no users and I kind of don't see use cases for it. Also phy_pm_runtime_get_sync() is only used by the phy core in three places - shouldn't be too hard to fix that.] So the issue with genpd really seems to be a messenger here. :-) Thanks, Rafael
Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Sat, Dec 23, 2017 at 1:50 PM, Rafael J. Wysocki wrote: > On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson wrote: >> On 22 December 2017 at 20:12, Rafael J. Wysocki wrote: >>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: >>>> On 21 December 2017 at 02:43, Rafael J. Wysocki wrote: >>>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson >>>> > wrote: >>>> >> The PM core in the device_prepare() phase, resets the wakeup_path status >>>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() >>>> >> or a >>>> >> ->suspend() callback for the device would update the device's wakeup >>>> >> setting, this doesn't become reflected in the wakeup_path status flag. >>>> >> >>>> >> In general this isn't a problem, because wakeup settings isn't supposed >>>> >> to >>>> >> be changed during those system suspend phases. Nevertheless, there are a >>>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >>>> >> indeed called from ->suspend() callbacks. >>>> > >>>> > And why is this regarded as correct? >>>> >>>> I am not saying that this behavior is correct. However, I am trying to >>>> improve the situation, which doesn't hurt or does it? >>> >>> Adding a workaround for them kind of encourages new code to do the same >>> thing, which actually may really hurt. So I assume that these call sites >>> are >>> all legacy and that's why you don't want to touch them for now, but in that >>> case the commit message should make it very clear that this is about legacy >>> only and new code should not call device_set_wakeup_enable() during suspend. >> >> Yeah, makes sense. Let me clarify that! >> >>> >>> [Something should be printed to the log if wakeup source objects >>> are created while system suspend is in progress I guess or similar.] >> >> Yes, good idea. Let me cook a patch for that as well. >> >>> >>>> More importantly, the next patch, which is about the wakeup path, >>>> depends on this. >>> >>> Honestly, this sounds like "We have this change we really really would like >>> to >>> make, but there's incorrect code getting in the way, so let's paper over it >>> and be done." Not very nice. :-/ >> >> Yeah it sounds a bit weird, I agree. >> >> However, maybe if I update the changelog and fold in a patch printing >> an error in case the APIs is being abused, that would sort out the >> confusion? > > That should be sufficient IMO. > >> Another option is simply to squash patch 1 and patch2, what do you prefer? >> >>> >>> How many drivers actually do call device_set_wakeup_enable() during suspend? >> >> There are some ethernet/wifi drivers, although it hard to say how many >> without a more thorough investigation. >> >> Besides those I found these more obvious ones: >> drivers/ssb/pcihost_wrapper.c >> drivers/staging/rtlwifi/core.c >> drivers/tty/serial/atmel_serial.c >> drivers/usb/core/hcd-pci.c > > Ugh. I need to look at the last one at least ... The drivers/usb/core/hcd-pci.c instance is actually valid, because it calls device_set_wakeup_enable() to remove the wakeup source object in case the device is dead. Moreover, it does that in the "noirq" phase which is not covered by your patch anyway. Thanks, Rafael
Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson wrote: > On 22 December 2017 at 20:12, Rafael J. Wysocki wrote: >> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: >>> On 21 December 2017 at 02:43, Rafael J. Wysocki wrote: >>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson >>> > wrote: >>> >> The PM core in the device_prepare() phase, resets the wakeup_path status >>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or >>> >> a >>> >> ->suspend() callback for the device would update the device's wakeup >>> >> setting, this doesn't become reflected in the wakeup_path status flag. >>> >> >>> >> In general this isn't a problem, because wakeup settings isn't supposed >>> >> to >>> >> be changed during those system suspend phases. Nevertheless, there are a >>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >>> >> indeed called from ->suspend() callbacks. >>> > >>> > And why is this regarded as correct? >>> >>> I am not saying that this behavior is correct. However, I am trying to >>> improve the situation, which doesn't hurt or does it? >> >> Adding a workaround for them kind of encourages new code to do the same >> thing, which actually may really hurt. So I assume that these call sites are >> all legacy and that's why you don't want to touch them for now, but in that >> case the commit message should make it very clear that this is about legacy >> only and new code should not call device_set_wakeup_enable() during suspend. > > Yeah, makes sense. Let me clarify that! > >> >> [Something should be printed to the log if wakeup source objects >> are created while system suspend is in progress I guess or similar.] > > Yes, good idea. Let me cook a patch for that as well. > >> >>> More importantly, the next patch, which is about the wakeup path, >>> depends on this. >> >> Honestly, this sounds like "We have this change we really really would like >> to >> make, but there's incorrect code getting in the way, so let's paper over it >> and be done." Not very nice. :-/ > > Yeah it sounds a bit weird, I agree. > > However, maybe if I update the changelog and fold in a patch printing > an error in case the APIs is being abused, that would sort out the > confusion? That should be sufficient IMO. > Another option is simply to squash patch 1 and patch2, what do you prefer? > >> >> How many drivers actually do call device_set_wakeup_enable() during suspend? > > There are some ethernet/wifi drivers, although it hard to say how many > without a more thorough investigation. > > Besides those I found these more obvious ones: > drivers/ssb/pcihost_wrapper.c > drivers/staging/rtlwifi/core.c > drivers/tty/serial/atmel_serial.c > drivers/usb/core/hcd-pci.c Ugh. I need to look at the last one at least ... Thanks, Rafael
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson wrote: > On 23 December 2017 at 02:35, Rafael J. Wysocki wrote: >> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson wrote: >>> On 21 December 2017 at 02:39, Rafael J. Wysocki wrote: >>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson >>>> wrote: >>>>> The runtime PM deployment in the phy core is deployed using the phy core >>>>> device, which is created by the phy core and assigned as a child device of >>>>> the phy provider device. >>>>> >>>>> The behaviour around the runtime PM deployment cause some issues during >>>>> system suspend, in cases when the phy provider device is put into a low >>>>> power state via a call to the pm_runtime_force_suspend() helper, as is the >>>>> case for a Renesas SoC, which has its phy provider device attached to the >>>>> generic PM domain. >>>>> >>>>> In more detail, the problem in this case is that >>>>> pm_runtime_force_suspend() >>>>> expects the child device of the provider device, which is the phy core >>>>> device, to be runtime suspended, else a WARN splat will be printed >>>>> (correctly) when runtime PM gets re-enabled at system resume. >>>> >>>> So we are now trying to work around issues with >>>> pm_runtime_force_suspend(). Lovely. :-/ >>> >>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or >>> are you saying we should just ignore all issues related to it? >> >> Or get rid of it? >> >> Seriously, is the resulting pain worth it? > > I am not sure what pain you refer to? :-) So far I haven't got much > errors being reported because of its use, have you? > > Moreover, to me, this small series fixes the problems in a rather trivial way. Well, I agree here (please see the message regarding that I've just sent in this thread). >> >>> Of course, if we had something that could replace >>> pm_runtime_force_suspend(), that would be great, but there isn't. >> >> I beg to differ. >> >> At least some of it could be replaced with the driver flags. > > Yes, true. > > On the other hand that is exactly the problem, only *some*. And more > importantly, genpd can't use them, because it can't cope with have > system wide PM callbacks to be skipped. Its own system-wide PM callbacks cannot be skipped, but it already skips driver callbacks in some cases. :-) > In other words, so far, the driver PM flags can't solve this issue. It is unclear which issue you are talking about, but anyway, yes, they aren't equivalent to pm_runtime_force_suspend/resume() which is quite intentional, honestly ... >>>>> In the current scenario, even if a call to phy_power_off() triggers it to >>>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't >>>>> get runtime suspended, because this is prevented in the system suspend >>>>> phases by the PM core. >>>>> >>>>> To solve this problem, let's move the runtime PM deployment from the phy >>>>> core device to the phy provider device, as this provides the similar >>>>> behaviour. Changing this makes it redundant to enable runtime PM for the >>>>> phy core device, so let's avoid doing that. >>>> >>>> I'm not really convinced that this approach is the best one to be honest. >>>> >>>> I'll have a deeper look at this in the next few days, stay tuned. >>> >>> There is different ways to solve this, for sure. I picked this one, >>> because I think it's the most trivial thing to do, and it shouldn't >>> cause any other problems. >>> >>> I think any other option would involve assigning ->suspend|resume() >>> callbacks to the phy core device, but that's fine too, if you prefer >>> that. >>> >>> Also, I have considered how to deal with wakeup paths for phys, >>> although I didn't want to post changes as a part of this series, but >>> maybe I should to give a more complete picture? >> >> Yes, you should. > > Okay! > >> >> The point is that without genpd using pm_runtime_force_suspend() the >> phy code could very well stay the way it is. And it is logical, >> because having a parent with enabled runtime PM without enabling >> runtime PM for its children is at least conceptually questionable. > > I agree that the phy core is today logical okay. But I think that's > also the case, if we pick up this small series. > > There are many reasons and cases where a children is runtime PM > disabled, while the parent is runtime PM enabled. Moreover the runtime > PM core copes fine with that. Fair enough. >> >> So the conclusion may be that the phy code is OK, but calling >> pm_runtime_force_suspend() from genpd isn't. > > So I am worried that changing genpd in this regards, may introduce > other regressions. @subject series seems far less risky - and again, > to me, the changes are trivial. > > Anyway, I will keep your suggestion in mind and think about, how/if > genpd can be changed. Thanks a lot! Rafael
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Thu, Dec 21, 2017 at 2:39 AM, Rafael J. Wysocki wrote: > On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson wrote: >> The runtime PM deployment in the phy core is deployed using the phy core >> device, which is created by the phy core and assigned as a child device of >> the phy provider device. >> >> The behaviour around the runtime PM deployment cause some issues during >> system suspend, in cases when the phy provider device is put into a low >> power state via a call to the pm_runtime_force_suspend() helper, as is the >> case for a Renesas SoC, which has its phy provider device attached to the >> generic PM domain. >> >> In more detail, the problem in this case is that pm_runtime_force_suspend() >> expects the child device of the provider device, which is the phy core >> device, to be runtime suspended, else a WARN splat will be printed >> (correctly) when runtime PM gets re-enabled at system resume. > > So we are now trying to work around issues with > pm_runtime_force_suspend(). Lovely. :-/ > >> In the current scenario, even if a call to phy_power_off() triggers it to >> invoke pm_runtime_put() during system suspend, the phy core device doesn't >> get runtime suspended, because this is prevented in the system suspend >> phases by the PM core. >> >> To solve this problem, let's move the runtime PM deployment from the phy >> core device to the phy provider device, as this provides the similar >> behaviour. Changing this makes it redundant to enable runtime PM for the >> phy core device, so let's avoid doing that. > > I'm not really convinced that this approach is the best one to be honest. > > I'll have a deeper look at this in the next few days, stay tuned. So IMO the changes you are proposing make sense regardless of the genpd issue, because they generally simplify the phy code, but the additional use_runtime_pm field in struct phy represents redundant information (manipulating reference counters shouldn't matter if runtime PM is disabled), so it doesn't appear to be necessary. [On a related note, I'm not sure why phy tries to intercept runtime PM errors and "fix up" the reference counters. That doesn't look right to me at all.] That said, the current phy code is not strictly invalid. While it looks more complicated than necessary, it doesn't do things documented as invalid in principle, so saying "The behaviour around the runtime PM deployment cause some issues during system suspend" in the changelog is describing the problem from a very specific angle. Simply put, pm_runtime_force_suspend() and the current phy code cannot work together and so using them together is a bug. None of them individually is at fault, but combining them is incorrect. Fortunately enough, the phy code can be modified so that it can be used with pm_runtime_force_suspend() without problems, but picturing it as "problematic", because it cannot do that today is not entirely fair IMO. Thanks, Rafael
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Sat, Dec 23, 2017 at 2:35 AM, Rafael J. Wysocki wrote: > On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson wrote: >> On 21 December 2017 at 02:39, Rafael J. Wysocki wrote: >>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson wrote: >>>> The runtime PM deployment in the phy core is deployed using the phy core >>>> device, which is created by the phy core and assigned as a child device of >>>> the phy provider device. [cut] >> >> Also, I have considered how to deal with wakeup paths for phys, >> although I didn't want to post changes as a part of this series, but >> maybe I should to give a more complete picture? > > Yes, you should. > > The point is that without genpd using pm_runtime_force_suspend() the > phy code could very well stay the way it is. And it is logical, > because having a parent with enabled runtime PM without enabling > runtime PM for its children is at least conceptually questionable. Actually, I sort of agree that the phy's usage of runtime PM is too convoluted. For example, it uses pm_runtime_enabled() unnecessarily at least in some places, but that doesn't seem to be fixed by your patches. Thanks, Rafael
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson wrote: > On 21 December 2017 at 02:39, Rafael J. Wysocki wrote: >> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson wrote: >>> The runtime PM deployment in the phy core is deployed using the phy core >>> device, which is created by the phy core and assigned as a child device of >>> the phy provider device. >>> >>> The behaviour around the runtime PM deployment cause some issues during >>> system suspend, in cases when the phy provider device is put into a low >>> power state via a call to the pm_runtime_force_suspend() helper, as is the >>> case for a Renesas SoC, which has its phy provider device attached to the >>> generic PM domain. >>> >>> In more detail, the problem in this case is that pm_runtime_force_suspend() >>> expects the child device of the provider device, which is the phy core >>> device, to be runtime suspended, else a WARN splat will be printed >>> (correctly) when runtime PM gets re-enabled at system resume. >> >> So we are now trying to work around issues with >> pm_runtime_force_suspend(). Lovely. :-/ > > Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or > are you saying we should just ignore all issues related to it? Or get rid of it? Seriously, is the resulting pain worth it? > Of course, if we had something that could replace > pm_runtime_force_suspend(), that would be great, but there isn't. I beg to differ. At least some of it could be replaced with the driver flags. >>> In the current scenario, even if a call to phy_power_off() triggers it to >>> invoke pm_runtime_put() during system suspend, the phy core device doesn't >>> get runtime suspended, because this is prevented in the system suspend >>> phases by the PM core. >>> >>> To solve this problem, let's move the runtime PM deployment from the phy >>> core device to the phy provider device, as this provides the similar >>> behaviour. Changing this makes it redundant to enable runtime PM for the >>> phy core device, so let's avoid doing that. >> >> I'm not really convinced that this approach is the best one to be honest. >> >> I'll have a deeper look at this in the next few days, stay tuned. > > There is different ways to solve this, for sure. I picked this one, > because I think it's the most trivial thing to do, and it shouldn't > cause any other problems. > > I think any other option would involve assigning ->suspend|resume() > callbacks to the phy core device, but that's fine too, if you prefer > that. > > Also, I have considered how to deal with wakeup paths for phys, > although I didn't want to post changes as a part of this series, but > maybe I should to give a more complete picture? Yes, you should. The point is that without genpd using pm_runtime_force_suspend() the phy code could very well stay the way it is. And it is logical, because having a parent with enabled runtime PM without enabling runtime PM for its children is at least conceptually questionable. So the conclusion may be that the phy code is OK, but calling pm_runtime_force_suspend() from genpd isn't. Thanks, Rafael
Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: > On 21 December 2017 at 02:43, Rafael J. Wysocki wrote: > > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson wrote: > >> The PM core in the device_prepare() phase, resets the wakeup_path status > >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a > >> ->suspend() callback for the device would update the device's wakeup > >> setting, this doesn't become reflected in the wakeup_path status flag. > >> > >> In general this isn't a problem, because wakeup settings isn't supposed to > >> be changed during those system suspend phases. Nevertheless, there are a > >> cases not conforming to that behaviour, as device_set_wakeup_enable() is > >> indeed called from ->suspend() callbacks. > > > > And why is this regarded as correct? > > I am not saying that this behavior is correct. However, I am trying to > improve the situation, which doesn't hurt or does it? Adding a workaround for them kind of encourages new code to do the same thing, which actually may really hurt. So I assume that these call sites are all legacy and that's why you don't want to touch them for now, but in that case the commit message should make it very clear that this is about legacy only and new code should not call device_set_wakeup_enable() during suspend. [Something should be printed to the log if wakeup source objects are created while system suspend is in progress I guess or similar.] > More importantly, the next patch, which is about the wakeup path, > depends on this. Honestly, this sounds like "We have this change we really really would like to make, but there's incorrect code getting in the way, so let's paper over it and be done." Not very nice. :-/ How many drivers actually do call device_set_wakeup_enable() during suspend? Thanks, Rafael
Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson wrote: > The PM core in the device_prepare() phase, resets the wakeup_path status > flag to the value of device_may_wakeup(). This means if a ->prepare() or a > ->suspend() callback for the device would update the device's wakeup > setting, this doesn't become reflected in the wakeup_path status flag. > > In general this isn't a problem, because wakeup settings isn't supposed to > be changed during those system suspend phases. Nevertheless, there are a > cases not conforming to that behaviour, as device_set_wakeup_enable() is > indeed called from ->suspend() callbacks. And why is this regarded as correct?
Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device
On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson wrote: > The runtime PM deployment in the phy core is deployed using the phy core > device, which is created by the phy core and assigned as a child device of > the phy provider device. > > The behaviour around the runtime PM deployment cause some issues during > system suspend, in cases when the phy provider device is put into a low > power state via a call to the pm_runtime_force_suspend() helper, as is the > case for a Renesas SoC, which has its phy provider device attached to the > generic PM domain. > > In more detail, the problem in this case is that pm_runtime_force_suspend() > expects the child device of the provider device, which is the phy core > device, to be runtime suspended, else a WARN splat will be printed > (correctly) when runtime PM gets re-enabled at system resume. So we are now trying to work around issues with pm_runtime_force_suspend(). Lovely. :-/ > In the current scenario, even if a call to phy_power_off() triggers it to > invoke pm_runtime_put() during system suspend, the phy core device doesn't > get runtime suspended, because this is prevented in the system suspend > phases by the PM core. > > To solve this problem, let's move the runtime PM deployment from the phy > core device to the phy provider device, as this provides the similar > behaviour. Changing this makes it redundant to enable runtime PM for the > phy core device, so let's avoid doing that. I'm not really convinced that this approach is the best one to be honest. I'll have a deeper look at this in the next few days, stay tuned. Thanks, Rafael
Re: [PATCH] bus: simple-pm-bus: convert bool SIMPLE_PM_BUS to tristate
On Friday, December 1, 2017 8:02:33 AM CET Simon Horman wrote: > On Thu, Nov 30, 2017 at 12:57:00PM +0100, Geert Uytterhoeven wrote: > > From: Paul Gortmaker > > > > The Kconfig currently controlling compilation of this code is: > > > > config SIMPLE_PM_BUS > > bool "Simple Power-Managed Bus Driver" > > > > ...meaning that it currently is not being built as a module by anyone. > > > > In removing the orphaned modular support in a previous patch set, > > Geert indicated he'd rather see this code converted to tristate. > > > > I normally don't do that because it extends functionality that I > > can't easily run time test or even know if the use case makes sense, > > but since in this case the author has nominated it as such, we do > > the conversion here. > > > > Note that doesn't change the lack of run time testing ; this change > > is only tested for sucessful compile and modpost. > > > > [geert: Ethernet is probed successfully on sh73a0/kzm9g after > > insmodding simple-pm-bus.ko] > > > > Cc: Kevin Hilman > > Cc: Simon Horman > > Cc: linux-arm-ker...@lists.infradead.org > > Signed-off-by: Paul Gortmaker > > Tested-by: Geert Uytterhoeven > > Signed-off-by: Geert Uytterhoeven > > Acked-by: Simon Horman Applied, thanks!
Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag
On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
> For some bus types and PM domains, it's not sufficient to only check the
> return value from device_may_wakeup(), to fully understand how to configure
> wakeup settings for the device during system suspend.
>
> In particular, sometimes the device may need to remain in its power state,
> in case the driver has configured it for in band IRQs, as to be able to
> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
> driver flag, to enable drivers to instruct bus types and PM domains about
> this setting.
>
> Of course, in case the bus type and PM domain has additional information
> about wakeup settings of the device, they may override the behaviour.
>
> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
> types and PM domains should treat the device's parent during system
> suspend. Therefore, in __device_suspend(), let the PM core propagate the
> wakeup setting by using a status flag in the struct dev_pm_info for the
> parent. This also makes it consistent with how the existing "wakeup_path"
> status flag is being assigned.
I've been thinking about this quite a bit recently and my conclusion is that
the flag makes perfect sence (as it covers a valid use case), but I would
define it and design the handling of it a bit differently.
First off, the genpd changes in patch [3/3] effectively skip the "noirq"
driver callbacks for the device, but question is why this is just "noirq".
Arguably, the "late suspend" callback should be skipped too (the regular
"suspend" one not necessarily, because the state of the device can still
be changed via runtime PM at that point). Moreover, if we go for the
skipping at the suspend time, all of the resume should be skipped for the
device too, because it has not been suspended in the first place.
But, if the device is in suspend at the suspend time already, the
resume part should not be skipped for it at all (unless it has
LEAVE_SUSPENDED set too), so it looks like the skipping should only
happen if the device has not been suspended when the system transition
starts.
Second, it looks like the flag should be handled in the core in
accordance with what genpd does with it or drivers will have to
check the middle layer configuration.
So below is my version of the core part (on top of the series I posted
earlier today: https://marc.info/?l=linux-pm&m=151286423304445&w=2)
and the genpd one should be analogous IMO.
---
Documentation/driver-api/pm/devices.rst | 21 -
drivers/base/power/main.c | 71 +---
include/linux/pm.h |8 +++
3 files changed, 82 insertions(+), 18 deletions(-)
Index: linux-pm/include/linux/pm.h
===
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -560,6 +560,7 @@ struct pm_subsys_data {
* SMART_PREPARE: Check the return value of the driver's ->prepare callback.
* SMART_SUSPEND: No need to resume the device from runtime suspend.
* LEAVE_SUSPENDED: Avoid resuming the device during system resume if possible.
+ * IN_BAND_WAKEUP: Avoid suspending the device if configured for system wakeup.
*
* Setting SMART_PREPARE instructs bus types and PM domains which may want
* system suspend/resume callbacks to be skipped for the device to return 0
from
@@ -576,11 +577,16 @@ struct pm_subsys_data {
*
* Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the
* driver prefers the device to be left in suspend after system resume.
+ *
+ * Setting IN_BAND_WAKEUP informs the PM core and middle-layer code that, as
far
+ * as the driver knows, the device cannot be suspended to be able to wake up
the
+ * system from sleep.
*/
#define DPM_FLAG_NEVER_SKIPBIT(0)
#define DPM_FLAG_SMART_PREPARE BIT(1)
#define DPM_FLAG_SMART_SUSPEND BIT(2)
#define DPM_FLAG_LEAVE_SUSPENDED BIT(3)
+#define DPM_FLAG_IN_BAND_WAKEUPBIT(4)
struct dev_pm_info {
pm_message_tpower_state;
@@ -604,6 +610,7 @@ struct dev_pm_info {
boolno_pm_callbacks:1; /* Owned by the PM core
*/
unsigned intmust_resume:1; /* Owned by the PM core */
unsigned intmay_skip_resume:1; /* Set by subsystems */
+ unsigned intskip_suspend:1; /* Owned by the PM core */
#else
unsigned intshould_wakeup:1;
#endif
@@ -774,6 +781,7 @@ extern void pm_generic_complete(struct d
extern void dev_pm_skip_next_resume_phases(struct device *dev);
extern bool dev_pm_may_skip_resume(struct device *dev);
+extern bool dev_pm_skip_suspend_and_not_suspended(struct device *dev);
extern bool dev_pm_smart_suspend_and_suspended(struct device *dev);
#else /* !CONFIG_PM_SLEEP */
Index: linux-pm/Documentation/driver-api/pm/devices.rst
=
Re: [PATCH v2 1/3] PM / core: Re-factor some code dealing with parents in __device_suspend()
On Monday, November 13, 2017 4:46:41 PM CET Ulf Hansson wrote: > Let's make the code a bit more readable by moving some of the code, which > deals with adjustments for parent devices in __device_suspend(), into its > own function. > > Signed-off-by: Ulf Hansson > Reviewed-by: Geert Uytterhoeven Applied, thanks!
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
On Tue, Dec 5, 2017 at 4:03 PM, Alan Stern wrote: > On Tue, 5 Dec 2017, Yoshihiro Shimoda wrote: > >> Hi, >> >> > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM >> > >> > On 1 December 2017 at 12:03, Yoshihiro Shimoda >> > wrote: >> >> > > Sure! I tested your patch, and then the following message disappeared! >> > > >> > >Enabling runtime PM for inactive device (ee080200.usb-phy) with >> > > active children >> > >> > Great, that confirms my theory. >> > >> > I will re-work the patch and re-post it to see what people thinks about it. >> >> Thank you! >> >> > > >> > > However, the following message still exists. >> > > >> > >Enabling runtime PM for inactive device (ee08.usb) with active >> > > children >> > > >> > > So, I guess ohci-platform.c also has similar issue. >> > >> > Yes, very likely! >> > >> > However, I need some more time to look into this to be able to suggest >> > a solution. >> >> I found a solution and sent a report as below: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html >> >> What do you think about using pm_runtime_forbid()? > > In general, drivers should not use pm_runtime_forbid(). I'd rather say that it's not very useful to them. :-) > It is meant for use by userspace, through the /sys/.../power/control file. Or when the driver wants to change the default setting. > Drivers cannot rely on the result of calling pm_runtime_forbid() > or pm_runtime_allow(), because the user can change it at any time. Right. > If you really want to prevent the OHCI controller from going into > runtime suspend, the proper approach is to call pm_runtime_get() in the > probe routine and pm_runtime_put() in the release routine. However, > this will waste energy because it will force the controller to remain > at full power even when no active devices are attached. > > In this case, there probably is a better to solution to your problem > (such as fixing the runtime PM support in the phy driver). Agreed. Thanks, Rafael
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
On Tue, Nov 28, 2017 at 11:58 AM, Geert Uytterhoeven
wrote:
> Hi Rafael, Shimoda-san,
>
> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki
>>
>> The check for "active" children in __pm_runtime_set_status(), when
>> trying to set the parent device status to "suspended", doesn't
>> really make sense, because in fact it is not invalid to set the
>> status of a device with runtime PM disabled to "suspended" in any
>> case. It is invalid to enable runtime PM for a device with its
>> status set to "suspended" while its child_count reference counter
>> is nonzero, but the check in __pm_runtime_set_status() doesn't
>> really cover that situation.
>>
>> For this reason, drop the children check from __pm_runtime_set_status()
>> and add a check against child_count reference counters of "suspended"
>> devices to pm_runtime_enable().
>>
>> Signed-off-by: Rafael J. Wysocki
>> ---
>> drivers/base/power/runtime.c | 30 ++
>> 1 file changed, 10 insertions(+), 20 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/runtime.c
>> ===
>> --- linux-pm.orig/drivers/base/power/runtime.c
>> +++ linux-pm/drivers/base/power/runtime.c
>> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
>> goto out;
>> }
>>
>> - if (dev->power.runtime_status == status)
>> + if (dev->power.runtime_status == status || !parent)
>> goto out_set;
>>
>> if (status == RPM_SUSPENDED) {
>> - /*
>> -* It is invalid to suspend a device with an active child,
>> -* unless it has been set to ignore its children.
>> -*/
>> - if (!dev->power.ignore_children &&
>> - atomic_read(&dev->power.child_count)) {
>> - dev_err(dev, "runtime PM trying to suspend device
>> but active child\n");
>
> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> R-Car H3:
>
> ohci-platform ee08.usb: runtime PM trying to suspend device
> but active child
> phy_rcar_gen3_usb2 ee080200.usb-phy: runtime PM trying to suspend
> device but active child
> ohci-platform ee0c.usb: runtime PM trying to suspend device
> but active child
> ohci-platform ee0a.usb: runtime PM trying to suspend device
> but active child
> phy_rcar_gen3_usb2 ee0c0200.usb-phy: runtime PM trying to suspend
> device but active child
> phy_rcar_gen3_usb2 ee0a0200.usb-phy: runtime PM trying to suspend
> device but active child
>
> so this was an existing issue with USB before.
>
>> - error = -EBUSY;
>> - goto out;
>> - }
>> -
>> - if (parent) {
>> - atomic_add_unless(&parent->power.child_count, -1, 0);
>> - notify_parent = !parent->power.ignore_children;
>> - }
>> - goto out_set;
>> - }
>> -
>> - if (parent) {
>> + atomic_add_unless(&parent->power.child_count, -1, 0);
>> + notify_parent = !parent->power.ignore_children;
>> + } else {
>> spin_lock_nested(&parent->power.lock, SINGLE_DEPTH_NESTING);
>>
>> /*
>> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
>> else
>> dev_warn(dev, "Unbalanced %s!\n", __func__);
>>
>> + WARN(dev->power.runtime_status == RPM_SUSPENDED &&
>> +!dev->power.ignore_children &&
>> +atomic_read(&dev->power.child_count) > 0,
>> +"Enabling runtime PM for inactive device (%s) with active
>> children\n",
>> +dev_name(dev));
>
> And now it became a bit more noisy:
Well, it's all existing issues, although the WARN() doesn't provide
additional information in this particular case.
I'm considering changing it to print a message without a stack trace.
Thanks,
Rafael
Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag
On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven wrote: > Hi Ulf, > > On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson wrote: >> [...] >> > The Ethernet driver can still call device_set_wakeup_enable(... , false) > to control this. If WoL is disabled by the user (or deemed not usable, > see > below), it already does so. I don't think that API is intended to be used like that and I wonder if it even works as expected. Quoting the doc: "If device wakeup mechanisms are enabled or disabled directly by drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do during a system sleep transition. Device drivers, however, are not expected to call :c:func:`device_set_wakeup_enable()` directly in any case." Rafael, can you comment on this? >>> >>> There are ca. 100 callers in drivers. >> >> Yeah, but those doesn't normally use it to toggle the setting, but >> instead only to set a default value during ->probe() or whatever >> initialization code that runs. >> >> I think that makes a big difference, doesn't it? > > The few Ethernet drivers I looked at change the state in their > ethtool_ops.set_wol() callback, not during probe. > This is to be configured from userspace using ethtool. Which is the case I was talking about. Since changing the WoL setting via ethtool is expected to cause the sysfs knob to reflect its status, using device_set_wakeup_enable() in there is not actually confusing. The same applies to setting the default from ->probe(). It will be confusing in all of the other cases, though. Thanks, Rafael
Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag
On Thu, Nov 9, 2017 at 12:59 PM, Rafael J. Wysocki wrote: > On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson wrote: >> On 9 November 2017 at 10:02, Geert Uytterhoeven wrote: >>> Hi Ulf, >>> >>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson wrote: >>>> On 8 November 2017 at 16:41, Geert Uytterhoeven >>>> wrote: >>>>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson >>>>> wrote: >>>>>> The generic problem this series is trying to solve, is that for some bus >>>>>> types >>>>>> and PM domains, it's not sufficient to only check the return value from >>>>>> device_may_wakeup(), to fully understand how to treat the device during >>>>>> system >>>>>> suspend. >>>>>> >>>>>> One particular case that suffers from this, is the generic PM domain >>>>>> (aka genpd) >>>>>> and that is taken care of in the final change in this series. >>>>>> >>>>>> The special case this series address, is to enable drivers to instruct >>>>>> bus types >>>>>> and PM domains, that the device need to stay powered in case wakeup >>>>>> signals >>>>>> is enabled for it. >>> >>>>>> Geert Uytterhoeven, has been working on some related problems for some >>>>>> Renesas >>>>>> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet >>>>>> devices/drivers, which are used together with genpd. My intent is that >>>>>> this >>>>>> series enables a solution for those problems. >>>>>> >>>>>> [1] >>>>>> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html >>>>> >>>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think >>>>> it's the right solution for the Renesas SoCs. I can just set the recently >>>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain >>>>> drivers to fix the issue for all Renesas drivers. After all, all devices >>>>> in >>>>> the clock/power domain must be kept enabled if they're a wakeup source, or >>>>> part of the wakeup path. >>>> >>>> Right, that would work! However, to me, I don't think it's fine grained >>>> enough. >>>> >>>> Let's take the Ethernet device/driver using WoL as an example, similar >>>> to your cases. >>>> >>>> First, let's assume device_may_wakeup() returns true, meaning that the >>>> Ethernet device is wakeup capable and that userspace has requested >>>> wakeup to be enabled. >>>> >>>> Then we have three scenarios to consider when the Ethernet driver >>>> becomes suspended (typically when its ->suspend() callback gets >>>> invoked). >>>> 1) The Ethernet interface is down. >>>> 2) The Ethernet interface is up, but no connection established. >>>> 3) The Ethernet interface is up, connection established. >>>> >>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean >>>> that we can't distinguish between any of the the scenarios above, but >>>> instead always keep the Ethernet device powered on and thus the PM >>>> domain also. >>>> >>>> In the more fine grained solution, we can change the Ethernet driver >>>> to consider under what scenario it's being suspended. For 1) and 2), >>>> there is no need to keep the Ethernet device being powered, but >>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED >>>> flag. >>> >>> The Ethernet driver can still call device_set_wakeup_enable(... , false) >>> to control this. If WoL is disabled by the user (or deemed not usable, see >>> below), it already does so. >> >> I don't think that API is intended to be used like that and I wonder >> if it even works as expected. >> >> Quoting the doc: >> "If device wakeup mechanisms are enabled or disabled directly by >> drivers, they also should use :c:func:`device_may_wakeup()` to decide what >> to do >> during a system sleep transition. Device drivers, however, are not expected >> to >> call :c:func:`device_set_wakeup_enable()` directly in any case." >> >> Rafael, can you comment on this? > > Well, it means what it says. > > If you call device_set_wakeup_enable() from a driver, user space will > see a change in sysfs and may be confused by that and that's why doing > so is not recommended. > > Not that people listen to recommendations too much, though. :-) But setting up WoL via ethtool from user space is an exception, because user space actually *does* expect to see a change in sysfs in this particular case. It's basically two different ways to change the same setting and both should result in the same behavior, ideally. Thanks, Rafael
Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag
On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson wrote: > On 9 November 2017 at 10:02, Geert Uytterhoeven wrote: >> Hi Ulf, >> >> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson wrote: >>> On 8 November 2017 at 16:41, Geert Uytterhoeven >>> wrote: On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson wrote: > The generic problem this series is trying to solve, is that for some bus > types > and PM domains, it's not sufficient to only check the return value from > device_may_wakeup(), to fully understand how to treat the device during > system > suspend. > > One particular case that suffers from this, is the generic PM domain (aka > genpd) > and that is taken care of in the final change in this series. > > The special case this series address, is to enable drivers to instruct > bus types > and PM domains, that the device need to stay powered in case wakeup > signals > is enabled for it. >> > Geert Uytterhoeven, has been working on some related problems for some > Renesas > SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet > devices/drivers, which are used together with genpd. My intent is that > this > series enables a solution for those problems. > > [1] > https://www.spinics.net/lists/linux-renesas-soc/msg19319.html While your new WAKEUP_POWERED definitely serves a purpose, I don't think it's the right solution for the Renesas SoCs. I can just set the recently added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain drivers to fix the issue for all Renesas drivers. After all, all devices in the clock/power domain must be kept enabled if they're a wakeup source, or part of the wakeup path. >>> >>> Right, that would work! However, to me, I don't think it's fine grained >>> enough. >>> >>> Let's take the Ethernet device/driver using WoL as an example, similar >>> to your cases. >>> >>> First, let's assume device_may_wakeup() returns true, meaning that the >>> Ethernet device is wakeup capable and that userspace has requested >>> wakeup to be enabled. >>> >>> Then we have three scenarios to consider when the Ethernet driver >>> becomes suspended (typically when its ->suspend() callback gets >>> invoked). >>> 1) The Ethernet interface is down. >>> 2) The Ethernet interface is up, but no connection established. >>> 3) The Ethernet interface is up, connection established. >>> >>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean >>> that we can't distinguish between any of the the scenarios above, but >>> instead always keep the Ethernet device powered on and thus the PM >>> domain also. >>> >>> In the more fine grained solution, we can change the Ethernet driver >>> to consider under what scenario it's being suspended. For 1) and 2), >>> there is no need to keep the Ethernet device being powered, but >>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED >>> flag. >> >> The Ethernet driver can still call device_set_wakeup_enable(... , false) >> to control this. If WoL is disabled by the user (or deemed not usable, see >> below), it already does so. > > I don't think that API is intended to be used like that and I wonder > if it even works as expected. > > Quoting the doc: > "If device wakeup mechanisms are enabled or disabled directly by > drivers, they also should use :c:func:`device_may_wakeup()` to decide what to > do > during a system sleep transition. Device drivers, however, are not expected > to > call :c:func:`device_set_wakeup_enable()` directly in any case." > > Rafael, can you comment on this? Well, it means what it says. If you call device_set_wakeup_enable() from a driver, user space will see a change in sysfs and may be confused by that and that's why doing so is not recommended. Not that people listen to recommendations too much, though. :-) >> >> In addition, keeping WoL enabled for cases 1 and 2 may be desirable >> (e.g allow wake-up if a cable is plugged in during system suspend and >> a magic packet is received afterwards), depending on the hardware. >> But the driver can already control that through device_set_wakeup_enable(). >> > > Ehh, that sounds weird. :-) If the Ethernet interface is down, how > would such packet be received? In PCI NICs, if wakeup power is provided, the NIC can detect activity on the port and generate a PCI PME even if the I/F is down otherwise. Thanks, Rafael
Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag
On Thu, Nov 9, 2017 at 9:53 AM, Ulf Hansson wrote:
> On 9 November 2017 at 01:41, Rafael J. Wysocki wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson wrote:
>>> For some bus types and PM domains, it's not sufficient to only check the
>>> return value from device_may_wakeup(), to fully understand how to treat the
>>> device during system suspend.
>>>
>>> In particular, sometimes the device may need to stay in full power state,
>>> to have wakeup signals enabled for it. Therefore, define and document a
>>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>>> exactly about that.
>>>
>>> During __device_suspend() in the PM core, let's make sure to also propagate
>>> the setting of the flag to the parent device, when wakeup signals are
>>> enabled and unless the parent has the "ignore_children" flag set. This
>>> makes it also consistent with how the existing "wakeup_path" flag is being
>>> assigned.
>>>
>>> Signed-off-by: Ulf Hansson
>>> ---
>>> Documentation/driver-api/pm/devices.rst | 12
>>> drivers/base/power/main.c | 6 +-
>>> include/linux/pm.h | 5 +
>>> 3 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/driver-api/pm/devices.rst
>>> b/Documentation/driver-api/pm/devices.rst
>>> index 53c1b0b..1ca2d0f 100644
>>> --- a/Documentation/driver-api/pm/devices.rst
>>> +++ b/Documentation/driver-api/pm/devices.rst
>>> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example,
>>> :c:func:`enable_irq_wake()`
>>> might identify GPIO signals hooked up to a switch or other external
>>> hardware,
>>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME
>>> signal.
>>>
>>> +Moreover, in case wakeup signals are enabled for a device, some bus types
>>> and
>>> +PM domains may manage the device slightly differently during system
>>> suspend. For
>>> +example, sometimes the device needs to stay in full power state, to have
>>> wakeup
>>> +signals enabled for it. In cases when the wakeup settings are mostly
>>> managed by
>>> +the driver, it may not be sufficient for bus types and PM domains to only
>>> check
>>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what
>>> actions
>>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>>> +:c:member:`power.driver_flags`, by passing the flag to
>>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>>> +domains to leave the device in full power state, when wakeup signals are
>>> enabled
>>> +for it.
>>> +
>>> If any of these callbacks returns an error, the system won't enter the
>>> desired
>>> low-power state. Instead, the PM core will unwind its actions by resuming
>>> all
>>> the devices that were suspended.
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index 8089e72..f64f945 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device
>>> *dev)
>>> spin_lock_irq(&parent->power.lock);
>>>
>>> parent->power.direct_complete = false;
>>> - if (dev->power.wakeup_path && !parent->power.ignore_children)
>>> + if (dev->power.wakeup_path && !parent->power.ignore_children) {
>>> parent->power.wakeup_path = true;
>>>
>>> + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
>>> + parent->power.driver_flags |=
>>> DPM_FLAG_WAKEUP_POWERED;
>>
>> No, sorry.
>>
>> The flag cannot be propagated this way, because that effectively
>> overrides the choices made by the parent driver and up.
>
> Yes, but that is the hole point.
>
> If a child device needs to stay powered as to deal with wakeup, so is
> required by the parent.
So you need a flag and a status bit.
The flag says what the driver wants (and what it wants for a
particular device) and the status bit reflects the current situation
(taking dependencies into account).
Say a device has two children, A and B, and both of them have the new flag set.
If either A or B is configured for system wakeup, po
Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag
On Thu, Nov 9, 2017 at 9:44 AM, Ulf Hansson wrote: > On 9 November 2017 at 01:24, Rafael J. Wysocki wrote: >> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson wrote: >>> For some bus types and PM domains, it's not sufficient to only check the >>> return value from device_may_wakeup(), to fully understand how to treat the >>> device during system suspend. >>> >>> In particular, sometimes the device may need to stay in full power state, >>> to have wakeup signals enabled for it. Therefore, define and document a >>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains >>> exactly about that. >>> >>> During __device_suspend() in the PM core, let's make sure to also propagate >>> the setting of the flag to the parent device, when wakeup signals are >>> enabled and unless the parent has the "ignore_children" flag set. This >>> makes it also consistent with how the existing "wakeup_path" flag is being >>> assigned. >>> >>> Signed-off-by: Ulf Hansson >>> --- >>> Documentation/driver-api/pm/devices.rst | 12 >>> drivers/base/power/main.c | 6 +- >>> include/linux/pm.h | 5 + >>> 3 files changed, 22 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/driver-api/pm/devices.rst >>> b/Documentation/driver-api/pm/devices.rst >>> index 53c1b0b..1ca2d0f 100644 >>> --- a/Documentation/driver-api/pm/devices.rst >>> +++ b/Documentation/driver-api/pm/devices.rst >>> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, >>> :c:func:`enable_irq_wake()` >>> might identify GPIO signals hooked up to a switch or other external >>> hardware, >>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME >>> signal. >>> >>> +Moreover, in case wakeup signals are enabled for a device, some bus types >>> and >>> +PM domains may manage the device slightly differently during system >>> suspend. For >>> +example, sometimes the device needs to stay in full power state, to have >>> wakeup >>> +signals enabled for it. In cases when the wakeup settings are mostly >>> managed by >>> +the driver, it may not be sufficient for bus types and PM domains to only >>> check >>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what >>> actions >>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in >>> +:c:member:`power.driver_flags`, by passing the flag to >>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM >>> +domains to leave the device in full power state, when wakeup signals are >>> enabled >>> +for it. >> >> IMO this is a bit unclear. >> >> First off, how does the driver know that the device has to be in full >> power for wakeup to work? > > Because the device is accessed as part of dealing with the wakeup. Yes, it is, In the working state of the system. In the system wakeup case it may not be. Essentially, what happens then is that driver-provided interrupt handlers don't run as a rule and system wakeup is triggered by the low-level handler at the IRQ chip level. Next, the PM callbacks invoked for the device are expected to clean up the wakeup status etc. Of course, power still is necessary for that to work, but it may be not be in-band. That may be either in-band power used for normal operations and provided through the interconnect used by the device or it may be special wakeup power provided out-of-band. Also the wakeup signal itself may be an in-band device interrupt (like the ones used for signaling IO events during normal operation) or it may be a special wakeup signal (like PCI PME) in which case, from the driver's perspective, the wakeup signaling is out-of-band. Usually, the driver doesn't know how this is set up for the particular device in the particular platform and hence my question. :-) The case at hand seems to be when the wakeup power is in-band or the wakeup signal is an in-band interrupt (in which case power needs to be provided to the interconnect at least). If they both are out-of-band, the middle layer should know that, because as a rule it will be involved in setting up both of them. Otherwise, in principle, it should assume that in-band power needs to be provided for wakeup to work and avoid turning things off if wakeup is enabled. >> >> Second, this requirement sort of implies that the device cannot go >> into a low-power state during runt
Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag
On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson wrote:
> For some bus types and PM domains, it's not sufficient to only check the
> return value from device_may_wakeup(), to fully understand how to treat the
> device during system suspend.
>
> In particular, sometimes the device may need to stay in full power state,
> to have wakeup signals enabled for it. Therefore, define and document a
> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
> exactly about that.
>
> During __device_suspend() in the PM core, let's make sure to also propagate
> the setting of the flag to the parent device, when wakeup signals are
> enabled and unless the parent has the "ignore_children" flag set. This
> makes it also consistent with how the existing "wakeup_path" flag is being
> assigned.
>
> Signed-off-by: Ulf Hansson
> ---
> Documentation/driver-api/pm/devices.rst | 12
> drivers/base/power/main.c | 6 +-
> include/linux/pm.h | 5 +
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/pm/devices.rst
> b/Documentation/driver-api/pm/devices.rst
> index 53c1b0b..1ca2d0f 100644
> --- a/Documentation/driver-api/pm/devices.rst
> +++ b/Documentation/driver-api/pm/devices.rst
> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example,
> :c:func:`enable_irq_wake()`
> might identify GPIO signals hooked up to a switch or other external hardware,
> and :c:func:`pci_enable_wake()` does something similar for the PCI PME
> signal.
>
> +Moreover, in case wakeup signals are enabled for a device, some bus types and
> +PM domains may manage the device slightly differently during system suspend.
> For
> +example, sometimes the device needs to stay in full power state, to have
> wakeup
> +signals enabled for it. In cases when the wakeup settings are mostly managed
> by
> +the driver, it may not be sufficient for bus types and PM domains to only
> check
> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what
> actions
> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
> +:c:member:`power.driver_flags`, by passing the flag to
> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
> +domains to leave the device in full power state, when wakeup signals are
> enabled
> +for it.
> +
> If any of these callbacks returns an error, the system won't enter the
> desired
> low-power state. Instead, the PM core will unwind its actions by resuming
> all
> the devices that were suspended.
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 8089e72..f64f945 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev)
> spin_lock_irq(&parent->power.lock);
>
> parent->power.direct_complete = false;
> - if (dev->power.wakeup_path && !parent->power.ignore_children)
> + if (dev->power.wakeup_path && !parent->power.ignore_children) {
> parent->power.wakeup_path = true;
>
> + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
> + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
No, sorry.
The flag cannot be propagated this way, because that effectively
overrides the choices made by the parent driver and up.
Besides, wakeup_path already had a similar purpose. What has happened to that?
Thanks,
Rafael
Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag
On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson wrote:
> For some bus types and PM domains, it's not sufficient to only check the
> return value from device_may_wakeup(), to fully understand how to treat the
> device during system suspend.
>
> In particular, sometimes the device may need to stay in full power state,
> to have wakeup signals enabled for it. Therefore, define and document a
> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
> exactly about that.
>
> During __device_suspend() in the PM core, let's make sure to also propagate
> the setting of the flag to the parent device, when wakeup signals are
> enabled and unless the parent has the "ignore_children" flag set. This
> makes it also consistent with how the existing "wakeup_path" flag is being
> assigned.
>
> Signed-off-by: Ulf Hansson
> ---
> Documentation/driver-api/pm/devices.rst | 12
> drivers/base/power/main.c | 6 +-
> include/linux/pm.h | 5 +
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/pm/devices.rst
> b/Documentation/driver-api/pm/devices.rst
> index 53c1b0b..1ca2d0f 100644
> --- a/Documentation/driver-api/pm/devices.rst
> +++ b/Documentation/driver-api/pm/devices.rst
> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example,
> :c:func:`enable_irq_wake()`
> might identify GPIO signals hooked up to a switch or other external hardware,
> and :c:func:`pci_enable_wake()` does something similar for the PCI PME
> signal.
>
> +Moreover, in case wakeup signals are enabled for a device, some bus types and
> +PM domains may manage the device slightly differently during system suspend.
> For
> +example, sometimes the device needs to stay in full power state, to have
> wakeup
> +signals enabled for it. In cases when the wakeup settings are mostly managed
> by
> +the driver, it may not be sufficient for bus types and PM domains to only
> check
> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what
> actions
> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
> +:c:member:`power.driver_flags`, by passing the flag to
> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
> +domains to leave the device in full power state, when wakeup signals are
> enabled
> +for it.
IMO this is a bit unclear.
First off, how does the driver know that the device has to be in full
power for wakeup to work?
Second, this requirement sort of implies that the device cannot go
into a low-power state during runtime suspend too, so it basically
remains stays at full power even when runtime-suspended.
Does it then mean that the middle layer is expected to simply avoid
changing the power state of the device when enabled to wake up the
system, or is there more to that? In the former case, it may be
better to rename the flag to something along the lines of "don't
change power state if wakeup enabled".
> +
> If any of these callbacks returns an error, the system won't enter the
> desired
> low-power state. Instead, the PM core will unwind its actions by resuming
> all
> the devices that were suspended.
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 8089e72..f64f945 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev)
> spin_lock_irq(&parent->power.lock);
>
> parent->power.direct_complete = false;
> - if (dev->power.wakeup_path && !parent->power.ignore_children)
> + if (dev->power.wakeup_path && !parent->power.ignore_children) {
> parent->power.wakeup_path = true;
>
> + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
> + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
> + }
> +
> spin_unlock_irq(&parent->power.lock);
> }
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 65d3911..34c2404 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -559,6 +559,7 @@ struct pm_subsys_data {
> * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
> * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
> * SMART_SUSPEND: No need to resume the device from runtime suspend.
> + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled.
> *
> * Setting SMART_PREPARE instructs bus types and PM domains which may want
> * system suspend/resume callbacks to be skipped for the device to return 0
> from
> @@ -572,10 +573,14 @@ struct pm_subsys_data {
> * necessary from the driver's perspective. It also may cause them to skip
> * invocations of the ->suspend_late and ->suspend_noirq callbacks provided
> by
> * the driver if they decide to leave the device in runtime suspend.
> + *
> + * Setting WAKEUP_POWERED instructs bus types and
Re: [PATCH v3 0/5] PM / Domains: Remove gpd_dev_ops.active_wakeup() callback
On Tuesday, November 7, 2017 1:48:10 PM CET Geert Uytterhoeven wrote: > Hi Rafael, Ulf, Kevin, > > It is quite common for PM Domains to require slave devices to be kept > active during system suspend if they are to be used as wakeup sources. > To enable this, currently each PM Domain or driver has to provide its > own gpd_dev_ops.active_wakeup() callback. > > All existing callbacks either return always true, or a fixed value > depending on the PM Domain. > > Hence this patch series simplifies active wakeup handling by replacing > the callback by a flag: > - Patch 1 adds a new new flag GENPD_FLAG_ACTIVE_WAKEUP, to be set by > PM Domain drivers that want to use the new handling, > - Patches 2-4 convert all existing users of the callback to the new > flag, > - Patch 5 removes the callback. > > This series was extracted from "[PATCH 00/10] PM / Domain: renesas: Fix > active wakeup behavior", and retains only PM Domain changes to existing > drivers. > > Changes compared to v2: > - Add Acked-by, Reviewed-by, > - Drop RFC status from dependent patches, as everything can go in > through the pm tree. > > Changes compared to v1 (most suggested by Ulf): > - Use the flag in se instead of setting up an "always true" callback, > - Convert the mediatek and rockchip PM Domain drivers, > - Remove the callback. > > Thanks for applying for v4.15! Queued up for 4.15, thanks! Rafael
Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency
[cut]
>> It seems the default values for pm_qos have changed with the patch, and that
>> breaks genpd at least. I only fixed PM runtime initially, but you could try
>> this diff to fix the genpd part also:
>>
>> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
>> index d68b056..7c8f643 100644
>> --- a/include/linux/pm_qos.h
>> +++ b/include/linux/pm_qos.h
>> @@ -34,9 +34,9 @@ enum pm_qos_flags_status {
>> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE0
>> #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
>> -#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE0
>> +#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUEPM_QOS_LATENCY_ANY
>> #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINTPM_QOS_LATENCY_ANY
>> -#define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0
>> +#define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE (-1)
>> #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1)
>>
>> #define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
>
> This is the original change in pm_qos.h (up to the GMail-induced
> whitespace breakage):
>
> -#define PM_QOS_DEFAULT_VALUE -1
> +#define PM_QOS_DEFAULT_VALUE (-1)
> +#define PM_QOS_LATENCY_ANY S32_MAX
>
> #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
> #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
> #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE 0
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
OK, so I should have changed PM_QOS_RESUME_LATENCY_DEFAULT_VALUE to
PM_QOS_LATENCY_ANY too, so that the default is still "no restriction".
> #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0
> #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1)
>
> -#define PM_QOS_LATENCY_ANY ((s32)(~(__u32)0 >> 1))
>
> #define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
> #define PM_QOS_FLAG_REMOTE_WAKEUP (1 << 1)
Thanks,
Rafael
Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency
On Wed, Nov 1, 2017 at 11:28 AM, Tero Kristo wrote:
> On 01/11/17 00:32, Rafael J. Wysocki wrote:
>>
>> On Tue, Oct 31, 2017 at 7:07 PM, Geert Uytterhoeven
>> wrote:
>>>
>>> Hi Rafael,
>>>
>>> On Tue, Oct 31, 2017 at 6:22 PM, Rafael J. Wysocki
>>> wrote:
>>>>
>>>> On Tue, Oct 31, 2017 at 2:55 PM, Geert Uytterhoeven
>>>> wrote:
>>>>>
>>>>> Hi Rafael, Tero,
>>>>>
>>>>> CC pinchartl, dri-devel
>>>>>
>>>>> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven
>>>>> wrote:
>>>>>>
>>>>>> CC linux-renesas-soc
>>>>>>
>>>>>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven
>>>>>> wrote:
>>>>>>>
>>>>>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> The recent change to the PM QoS framework to introduce a proper
>>>>>>>>>> no constraint value overlooked to handle the devices which don't
>>>>>>>>>> implement PM QoS OPS. Runtime PM is one of the more severely
>>>>>>>>>> impacted subsystems, failing every attempt to runtime suspend
>>>>>>>>>> a device. This leads into some nasty second level issues like
>>>>>>>>>> probe failures and increased power consumption among other things.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Oh, that's bad.
>>>>>>>>>
>>>>>>>>> Sorry about breaking it and thanks for the fix!
>>>>>>>>>
>>>>>>>>>> Fix this by adding a proper return value for devices that don't
>>>>>>>>>> implement PM QoS implicitly.
>>>>>>>>>>
>>>>>>>>>> Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS")
>>>>>>>>>> Signed-off-by: Tero Kristo
>>>>>>>>>> Cc: Rafael J. Wysocki
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Applied.
>>>>>>>>
>>>>>>>>
>>>>>>>> And pushed to Linus.
>>>>>>>
>>>>>>>
>>>>>>> I'm afraid it is not sufficient.
>>>>>>>
>>>>>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM
>>>>>>> QoS")
>>>>>>> introduced two issues on Renesas platforms:
>>>>>>> 1. After boot up, many devices have changed their state from
>>>>>>> "suspended"
>>>>>>> to "active", according to
>>>>>>> /sys/kernel/debug/pm_genpd/pm_genpd_summary
>>>>>>> (comparing that file across boots is one of my standard tests).
>>>>>>> Interestingly, doing a system suspend/resume cycle restores
>>>>>>> their state
>>>>>>> to "suspended".
>>>>>>>
>>>>>>> 2. During system suspend, the following warning is printed on
>>>>>>> r8a7791/koelsch:
>>>>>>>
>>>>>>> i2c-rcar e653.i2c: runtime PM trying to suspend device
>>>>>>> but
>>>>>>> active child
>>>>>
>>>>>
>>>>> 3. I've just bisected a seemingly unrelated issue to the same commit.
>>>>> On Salvator-XS with R-Car H3, initialization of the rcar-du driver
>>>>> now
>>>>> takes more than 1 minute due to flip_done time outs, while it took
>>>>> 0.12s
>>>>> before:
>>>>>
>>>>> [3.015035] [drm] Supports vblank timestamp caching Rev 2
>>>>> (21.10.2013).
>>>>> [3.021721] [drm] No driver support for vblank timestamp query.
>>>&g
Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency
On Tue, Oct 31, 2017 at 7:07 PM, Geert Uytterhoeven
wrote:
> Hi Rafael,
>
> On Tue, Oct 31, 2017 at 6:22 PM, Rafael J. Wysocki wrote:
>> On Tue, Oct 31, 2017 at 2:55 PM, Geert Uytterhoeven
>> wrote:
>>> Hi Rafael, Tero,
>>>
>>> CC pinchartl, dri-devel
>>>
>>> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven
>>> wrote:
>>>> CC linux-renesas-soc
>>>>
>>>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven
>>>> wrote:
>>>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki
>>>>> wrote:
>>>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote:
>>>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo wrote:
>>>>>>> > The recent change to the PM QoS framework to introduce a proper
>>>>>>> > no constraint value overlooked to handle the devices which don't
>>>>>>> > implement PM QoS OPS. Runtime PM is one of the more severely
>>>>>>> > impacted subsystems, failing every attempt to runtime suspend
>>>>>>> > a device. This leads into some nasty second level issues like
>>>>>>> > probe failures and increased power consumption among other things.
>>>>>>>
>>>>>>> Oh, that's bad.
>>>>>>>
>>>>>>> Sorry about breaking it and thanks for the fix!
>>>>>>>
>>>>>>> > Fix this by adding a proper return value for devices that don't
>>>>>>> > implement PM QoS implicitly.
>>>>>>> >
>>>>>>> > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS")
>>>>>>> > Signed-off-by: Tero Kristo
>>>>>>> > Cc: Rafael J. Wysocki
>>>>>>>
>>>>>>> Applied.
>>>>>>
>>>>>> And pushed to Linus.
>>>>>
>>>>> I'm afraid it is not sufficient.
>>>>>
>>>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM QoS")
>>>>> introduced two issues on Renesas platforms:
>>>>> 1. After boot up, many devices have changed their state from "suspended"
>>>>> to "active", according to /sys/kernel/debug/pm_genpd/pm_genpd_summary
>>>>> (comparing that file across boots is one of my standard tests).
>>>>> Interestingly, doing a system suspend/resume cycle restores their
>>>>> state
>>>>> to "suspended".
>>>>>
>>>>> 2. During system suspend, the following warning is printed on
>>>>> r8a7791/koelsch:
>>>>>
>>>>> i2c-rcar e653.i2c: runtime PM trying to suspend device but
>>>>> active child
>>>
>>> 3. I've just bisected a seemingly unrelated issue to the same commit.
>>> On Salvator-XS with R-Car H3, initialization of the rcar-du driver now
>>> takes more than 1 minute due to flip_done time outs, while it took 0.12s
>>> before:
>>>
>>> [3.015035] [drm] Supports vblank timestamp caching Rev 2
>>> (21.10.2013).
>>> [3.021721] [drm] No driver support for vblank timestamp query.
>>> [ 13.280738] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 23.520707] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 33.760708] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 44.000755] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 44.003597] Console: switching to colour frame buffer device 128x48
>>> [ 54.240707] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 64.480706] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>>> [CRTC:58:crtc-3] flip_done timed out
>>> [ 64.544876] rcar-du feb0.display: fb0: frame buffer device
>>> [ 64.552013] [drm] Initialized rcar-du 1.0.0 20130110 for
>>> feb0.display on minor 0
>>> [ 64.559873] [drm] Device feb0.display probed
>>>
>>>>> Commit 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm device resume
>>>>> latency") fixes the second issue, but not the first.
>>>
>>> ... nor the third.
>>>
>>>>> Reverting commits 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm
>>>>> device resume latency") and 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume
>>>>> latency PM QoS") fixes both.
>>>
>>> ... all three.
>>
>> Sorry for the breakage.
>>
>> OK, I'll just push the reverts to Linus later today.
>>
>>>>> Do you have a clue?
>>
>> Well, kind of.
>>
>> There is a change in behavior in domain_governor.c that should not
>> have made any difference to my eyes, but maybe that's it.
>>
>> Can you please check if the attached patch makes any difference?
>
> Thanks, but it doesn't seem to fix the issues.
Thanks for testing!
I've just pushed the reverts, but the PM QoS still needs to be fixed,
so we have to get to the bottom of this.
The current theory goes that the changes in domain_governor.c are to
blame. Is genpd involved in all of the issues with the PM QoS fix you
have seen?
Thanks,
Rafael
Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency
On Tue, Oct 31, 2017 at 2:55 PM, Geert Uytterhoeven
wrote:
> Hi Rafael, Tero,
>
> CC pinchartl, dri-devel
>
> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven
> wrote:
>> CC linux-renesas-soc
>>
>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven
>> wrote:
>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki
>>> wrote:
>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote:
>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo wrote:
>>>>> > The recent change to the PM QoS framework to introduce a proper
>>>>> > no constraint value overlooked to handle the devices which don't
>>>>> > implement PM QoS OPS. Runtime PM is one of the more severely
>>>>> > impacted subsystems, failing every attempt to runtime suspend
>>>>> > a device. This leads into some nasty second level issues like
>>>>> > probe failures and increased power consumption among other things.
>>>>>
>>>>> Oh, that's bad.
>>>>>
>>>>> Sorry about breaking it and thanks for the fix!
>>>>>
>>>>> > Fix this by adding a proper return value for devices that don't
>>>>> > implement PM QoS implicitly.
>>>>> >
>>>>> > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS")
>>>>> > Signed-off-by: Tero Kristo
>>>>> > Cc: Rafael J. Wysocki
>>>>>
>>>>> Applied.
>>>>
>>>> And pushed to Linus.
>>>
>>> I'm afraid it is not sufficient.
>>>
>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM QoS")
>>> introduced two issues on Renesas platforms:
>>> 1. After boot up, many devices have changed their state from "suspended"
>>> to "active", according to /sys/kernel/debug/pm_genpd/pm_genpd_summary
>>> (comparing that file across boots is one of my standard tests).
>>> Interestingly, doing a system suspend/resume cycle restores their state
>>> to "suspended".
>>>
>>> 2. During system suspend, the following warning is printed on
>>> r8a7791/koelsch:
>>>
>>> i2c-rcar e653.i2c: runtime PM trying to suspend device but
>>> active child
>
> 3. I've just bisected a seemingly unrelated issue to the same commit.
> On Salvator-XS with R-Car H3, initialization of the rcar-du driver now
> takes more than 1 minute due to flip_done time outs, while it took 0.12s
> before:
>
> [3.015035] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [3.021721] [drm] No driver support for vblank timestamp query.
> [ 13.280738] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 23.520707] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 33.760708] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 44.000755] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 44.003597] Console: switching to colour frame buffer device 128x48
> [ 54.240707] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 64.480706] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:58:crtc-3] flip_done timed out
> [ 64.544876] rcar-du feb0.display: fb0: frame buffer device
> [ 64.552013] [drm] Initialized rcar-du 1.0.0 20130110 for
> feb0.display on minor 0
> [ 64.559873] [drm] Device feb0.display probed
>
>>> Commit 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm device resume
>>> latency") fixes the second issue, but not the first.
>
> ... nor the third.
>
>>> Reverting commits 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm
>>> device resume latency") and 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume
>>> latency PM QoS") fixes both.
>
> ... all three.
Sorry for the breakage.
OK, I'll just push the reverts to Linus later today.
>>> Do you have a clue?
Well, kind of.
There is a change in behavior in domain_governor.c that should not
have made any difference to my eyes, but maybe that's it.
Can you please check if the attached patch makes any difference?
Thanks,
Rafael
---
drivers/base/power/domain_governor.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/base/power/domain_governor.c
===
--- linux-pm.orig/drivers/base/power/domain_governor.c
+++ linux-pm/drivers/base/power/domain_governor.c
@@ -83,12 +83,11 @@ static bool default_suspend_ok(struct de
td->cached_suspend_ok = true;
} else {
constraint_ns -= td->suspend_latency_ns + td->resume_latency_ns;
- if (constraint_ns > 0) {
- td->effective_constraint_ns = constraint_ns;
- td->cached_suspend_ok = true;
- } else {
- td->effective_constraint_ns = 0;
- }
+ if (constraint_ns == 0)
+ return false;
+
+ td->effective_constraint_ns = constraint_ns;
+ td->cached_suspend_ok = constraint_ns >= 0;
}
/*
Re: [PATCH] PM / QoS: Fix default runtime_pm device resume latency
On Tue, Oct 31, 2017 at 3:04 PM, Ulf Hansson wrote:
> On 31 October 2017 at 14:55, Geert Uytterhoeven wrote:
>> Hi Rafael, Tero,
>>
>> CC pinchartl, dri-devel
>>
>> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven
>> wrote:
>>> CC linux-renesas-soc
>>>
>>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven
>>> wrote:
>>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki
>>>> wrote:
>>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote:
>>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo wrote:
>>>>>> > The recent change to the PM QoS framework to introduce a proper
>>>>>> > no constraint value overlooked to handle the devices which don't
>>>>>> > implement PM QoS OPS. Runtime PM is one of the more severely
>>>>>> > impacted subsystems, failing every attempt to runtime suspend
>>>>>> > a device. This leads into some nasty second level issues like
>>>>>> > probe failures and increased power consumption among other things.
>>>>>>
>>>>>> Oh, that's bad.
>>>>>>
>>>>>> Sorry about breaking it and thanks for the fix!
>>>>>>
>>>>>> > Fix this by adding a proper return value for devices that don't
>>>>>> > implement PM QoS implicitly.
>>>>>> >
>>>>>> > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS")
>>>>>> > Signed-off-by: Tero Kristo
>>>>>> > Cc: Rafael J. Wysocki
>>>>>>
>>>>>> Applied.
>>>>>
>>>>> And pushed to Linus.
>>>>
>>>> I'm afraid it is not sufficient.
>>>>
>>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM QoS")
>>>> introduced two issues on Renesas platforms:
>>>> 1. After boot up, many devices have changed their state from "suspended"
>>>> to "active", according to /sys/kernel/debug/pm_genpd/pm_genpd_summary
>>>> (comparing that file across boots is one of my standard tests).
>>>> Interestingly, doing a system suspend/resume cycle restores their state
>>>> to "suspended".
>>>>
>>>> 2. During system suspend, the following warning is printed on
>>>> r8a7791/koelsch:
>>>>
>>>> i2c-rcar e653.i2c: runtime PM trying to suspend device but
>>>> active child
>>
>> 3. I've just bisected a seemingly unrelated issue to the same commit.
>> On Salvator-XS with R-Car H3, initialization of the rcar-du driver now
>> takes more than 1 minute due to flip_done time outs, while it took 0.12s
>> before:
>>
>> [3.015035] [drm] Supports vblank timestamp caching Rev 2
>> (21.10.2013).
>> [3.021721] [drm] No driver support for vblank timestamp query.
>> [ 13.280738] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 23.520707] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 33.760708] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 44.000755] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 44.003597] Console: switching to colour frame buffer device 128x48
>> [ 54.240707] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 64.480706] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
>> [CRTC:58:crtc-3] flip_done timed out
>> [ 64.544876] rcar-du feb0.display: fb0: frame buffer device
>> [ 64.552013] [drm] Initialized rcar-du 1.0.0 20130110 for
>> feb0.display on minor 0
>> [ 64.559873] [drm] Device feb0.display probed
>>
>>>> Commit 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm device resume
>>>> latency") fixes the second issue, but not the first.
>>
>> ... nor the third.
>>
>>>> Reverting commits 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm
>>>> device resume latency") and 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume
>>>> latency PM QoS") fixes both.
>>
>> ... all three.
>>
>>>> Do you have a clue?
>>>> Thanks!
>
> As I didn't have the time to review the original commit, before it got
> pushed as a fix, I am planning to review it now instead.
>
> A vague guess is that the genpd governor prevents the device from
> being suspended. That was also the most tricky part of the changes
> from the original commit, which is causing problems.
I think you are right.
> I get back to this when I have reviewed it more thoroughly.
Thanks, and sorry for breaking stuff.
In retrospect I should just have not pushed it so late in the cycle.
Rafael
Re: [PATCH] cpufreq: dt: Add r8a7796 support to to use generic cpufreq driver
On Wednesday, August 16, 2017 5:25:18 AM CEST Viresh Kumar wrote:
> On 11-08-17, 17:36, Simon Horman wrote:
> > From: Khiem Nguyen
> >
> > This patch adds the r8a7796 support the generic cpufreq driver
> > by adding an appropriate compat string. This is in keeping
> > with support for other Renesas ARM and arm64 based SoCs.
> >
> > Signed-off-by: Khiem Nguyen
> > [simon: new changelog]
> > Signed-off-by: Simon Horman
> > ---
> > drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > This is a follow-up for a similar change that has already been accepted
> > for the r8a7795.
> >
> >
> > I have provided an integration branch that includes with this patch, those
> > DTS updates that make use of opp-v2 bindings that depend on this change,
> > and Renesas clock updates also depended on by the DTS changes. The result
> > is working CPUFreq for the r8a7796 (R-Car M3-W).
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git
> > topic/r8a7796-cpufreq
> >
> > A description of steps taken to lightly exercise the same feature for the
> > r88a7795 the above can be found at the link below. The results are the same
> > for the r8a7796 with the exception that it has two active CPU cores rather
> > than four.
> >
> > http://elinux.org/Tests:R-CAR-GEN3-CPUFreq
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index bcee384b3251..233e18ad3948 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -68,6 +68,7 @@ static const struct of_device_id machines[] __initconst =
> > {
> > { .compatible = "renesas,r8a7793", },
> > { .compatible = "renesas,r8a7794", },
> > { .compatible = "renesas,r8a7795", },
> > + { .compatible = "renesas,r8a7796", },
> > { .compatible = "renesas,sh73a0", },
> >
> > { .compatible = "rockchip,rk2928", },
>
> Acked-by: Viresh Kumar
>
>
Applied, thanks!
Re: [PATCH] cpufreq: rcar: Add support for R8A7795 SoC
On Monday, August 7, 2017 5:37:05 AM CEST Viresh Kumar wrote:
> On 04-08-17, 15:18, Simon Horman wrote:
> > From: Khiem Nguyen
> >
> > After the commit "a399dc9fc50 cpufreq: shmobile: Use generic platdev
> > driver", will use cpufreq-dt-platdev driver to enable cpufreq-dt support.
> > Hence, follow the implementation to support new R8A7795 SoC.
> >
> > Signed-off-by: Khiem Nguyen
> > Signed-off-by: Simon Horman
> > ---
> > drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > This is identical to a patch posted by Khiem last year.
> > At the time it was asked if using opp-v2 was the preferred approach.
> >
> > https://patchwork.kernel.org/patch/9054011/
> >
> > An inspection of the current upstream kernel code seems to indicate
> > that adding a binding as this patch does is compatibile with using opp-v2
> > and I plan to post DTS patches separately which make use of the opp-v2
> > bindings - they depend on this driver change to work.
> >
> > I have provided an integration patch with this patch, those DTS changes,
> > and Renesas clock updates also depended on by the DTS changes. The result
> > is working CPUFreq for the r8a7795 (R-Car H3) ES1.0.
> >
> > If this work is acceptable I plan to follow up with patches to
> > enable CPUFreq on the r8a7796 (R-Car M3-W).
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git
> > topic/r8a7795-cpufreq
> >
> > A description of steps taken to lightly exercise the above can be found
> > here:
> >
> > http://elinux.org/Tests:R-CAR-GEN3-CPUFreq
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 096aea7fcb67..13b72f3c420b 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -67,6 +67,7 @@ static const struct of_device_id machines[] __initconst =
> > {
> > { .compatible = "renesas,r8a7792", },
> > { .compatible = "renesas,r8a7793", },
> > { .compatible = "renesas,r8a7794", },
> > + { .compatible = "renesas,r8a7795", },
> > { .compatible = "renesas,sh73a0", },
> >
> > { .compatible = "rockchip,rk2928", },
>
> Acked-by: Viresh Kumar
>
>
Applied, thanks!
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 10:20 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski wrote: > >> >> > Thanks for report! >> >> >> > >> >> >> > Damn it, although I couldn't find this in the code, but I was fearing >> >> >> > that this ends up in atomic section. That would kind of explain why >> >> >> > mutex was not there [1]. >> >> >> > >> >> >> > Anyway, the buggy code was there already. Instead of "sleeping in >> >> >> > atomic >> >> >> > section" there was no locking at all... In this context this was >> >> >> > probably safe because it was executed *after* disabling non-boot CPUs >> >> >> > but then the function cannot be called in other contexts. >> >> >> > >> >> >> > I am not sure I will be capable of developing the proper fix as I do >> >> >> > not >> >> >> > have the hardware and I do not know all stuff happening in sh >> >> >> > suspend. >> >> >> > Probably reverting this and living with non-locked path would be the >> >> >> > safest choice. >> >> >> > >> >> >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> >> >> >> >> AFAIU, all syscore stuff runs in atomic context. >> >> > >> >> > Indeed... The confusing part is that this code is syscore only from >> >> > the name, it is not hooked in to syscore_ops. Although going by call >> >> > chain (through sh clocksource drivers) we end up in >> >> > timekeeping_suspend() which is a syscore op. >> >> > >> >> > I wonder whether it would be useful - after reverting my commit - to add >> >> > an assert (which is a stronger API requirement than only documentation >> >> > "may >> >> > only be called during the system core (syscore) suspend") like: >> >> > WARN_ON(num_online_cpus() > 1)); >> >> > as without mutexes this should not be executed with more than one online >> >> > CPU. >> >> >> >> Or maybe WARN_ON_ONCE(!in_atomic())? >> > >> > You could be in atomic section on this CPU and still have other CPUs >> > online playing with gpd_list (without any protection from locking). >> > This function is safe only on non-SMP case. >> >> Well, not quite. >> >> It is safe if you can guarantee that no other CPUs will touch the data >> structure in question concurrently, which pretty much is the case for >> timekeeping_suspend() even though it may be invoked without taking the >> other CPUs offline (from the suspend-to-idle core path). > > Right, that would work fine for that case. > > However I was rather thinking that we have an in-kernel API (exported) > so someone might by mistake try to use it in different contexts. For > example in some atomic section but on a platform which offlines CPUs > later. Thus it would be called in some imaginary suspend path but with > CPUs still being online. Partially it is already mentioned in documentation > although I am not sure that on every possible architecture syscore ops > are called after disabling non-boot CPUs... Yes, they are. Nonboot CPUs are disabled by the core. Anyway, while I see your point, it would be rather hard to find an assertion that would also work for the suspend-to-idle timekeeping_suspend() invocation case. Thanks, Rafael
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski wrote: >> > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote: >> >> Hi Krzysztof, >> >> >> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski >> >> wrote: >> >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote: >> >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski >> >> >> wrote: >> >> >> > genpd_syscore_switch() had two problems: >> >> >> > 1. It silently assumed that device, it is being called for, belongs >> >> >> > to >> >> >> >generic power domain and used container_of() on its power domain >> >> >> >pointer. Such assumption might not be true always. >> >> >> > >> >> >> > 2. It iterated over list of generic power domains without holding >> >> >> >gpd_list_lock mutex thus list could have been modified in the same >> >> >> >time. >> >> >> > >> >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call >> >> >> > for non-generic power domains and uses mutex when iterating. >> >> >> > >> >> >> > Reported-by: Ulf Hansson >> >> >> > Signed-off-by: Krzysztof Kozlowski >> >> >> > Acked-by: Ulf Hansson >> >> >> >> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull >> >> >> request "[GIT PULL] Power management updates for v4.13-rc1". >> >> >> >> >> >> > Not tested on real hardware. >> >> >> >> >> >> This causes the following BUG during s2ram on all my Renesas arm32 >> >> >> boards, >> >> >> where the system timer is an IRQ safe device: >> >> >> >> >> >> PM: Syncing filesystems ... done. >> >> >> PM: Preparing system for sleep (mem) >> >> >> Freezing user space processes ... (elapsed 0.001 seconds) done. >> >> >> OOM killer disabled. >> >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. >> >> >> PM: Suspending system (mem) >> >> >> PM: suspend of devices complete after 122.841 msecs >> >> >> PM: suspend devices took 0.130 seconds >> >> >> PM: late suspend of devices complete after 2.682 msecs >> >> >> PM: noirq suspend of devices complete after 29.951 msecs >> >> >> Disabling non-boot CPUs ... >> >> >> BUG: sleeping function called from invalid context at >> >> >> kernel/locking/mutex.c:238 >> >> > >> >> > Thanks for report! >> >> > >> >> > Damn it, although I couldn't find this in the code, but I was fearing >> >> > that this ends up in atomic section. That would kind of explain why >> >> > mutex was not there [1]. >> >> > >> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic >> >> > section" there was no locking at all... In this context this was >> >> > probably safe because it was executed *after* disabling non-boot CPUs >> >> > but then the function cannot be called in other contexts. >> >> > >> >> > I am not sure I will be capable of developing the proper fix as I do not >> >> > have the hardware and I do not know all stuff happening in sh suspend. >> >> > Probably reverting this and living with non-locked path would be the >> >> > safest choice. >> >> > >> >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> >> >> AFAIU, all syscore stuff runs in atomic context. >> > >> > Indeed... The confusing part is that this code is syscore only from >> > the name, it is not hooked in to syscore_ops. Although going by call >> > chain (through sh clocksource drivers) we end up in >> > timekeeping_suspend() which is a syscore op. >> > >> > I wonder whether it would be useful - after reverting my commit - to add >> > an assert (which is a stronger API requirement than only documentation "may >> > only be called during the system core (syscore) suspend") like: >> > WARN_ON(num_online_cpus() > 1)); >> > as without mutexes this should not be executed with more than one online >> > CPU. >> >> Or maybe WARN_ON_ONCE(!in_atomic())? > > You could be in atomic section on this CPU and still have other CPUs > online playing with gpd_list (without any protection from locking). > This function is safe only on non-SMP case. Well, not quite. It is safe if you can guarantee that no other CPUs will touch the data structure in question concurrently, which pretty much is the case for timekeeping_suspend() even though it may be invoked without taking the other CPUs offline (from the suspend-to-idle core path). Thanks, Rafael
Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device
On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski wrote: > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote: >> Hi Krzysztof, >> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski wrote: >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote: >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski >> >> wrote: >> >> > genpd_syscore_switch() had two problems: >> >> > 1. It silently assumed that device, it is being called for, belongs to >> >> >generic power domain and used container_of() on its power domain >> >> >pointer. Such assumption might not be true always. >> >> > >> >> > 2. It iterated over list of generic power domains without holding >> >> >gpd_list_lock mutex thus list could have been modified in the same >> >> >time. >> >> > >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call >> >> > for non-generic power domains and uses mutex when iterating. >> >> > >> >> > Reported-by: Ulf Hansson >> >> > Signed-off-by: Krzysztof Kozlowski >> >> > Acked-by: Ulf Hansson >> >> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull >> >> request "[GIT PULL] Power management updates for v4.13-rc1". >> >> >> >> > Not tested on real hardware. >> >> >> >> This causes the following BUG during s2ram on all my Renesas arm32 boards, >> >> where the system timer is an IRQ safe device: >> >> >> >> PM: Syncing filesystems ... done. >> >> PM: Preparing system for sleep (mem) >> >> Freezing user space processes ... (elapsed 0.001 seconds) done. >> >> OOM killer disabled. >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. >> >> PM: Suspending system (mem) >> >> PM: suspend of devices complete after 122.841 msecs >> >> PM: suspend devices took 0.130 seconds >> >> PM: late suspend of devices complete after 2.682 msecs >> >> PM: noirq suspend of devices complete after 29.951 msecs >> >> Disabling non-boot CPUs ... >> >> BUG: sleeping function called from invalid context at >> >> kernel/locking/mutex.c:238 >> > >> > Thanks for report! >> > >> > Damn it, although I couldn't find this in the code, but I was fearing >> > that this ends up in atomic section. That would kind of explain why >> > mutex was not there [1]. >> > >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic >> > section" there was no locking at all... In this context this was >> > probably safe because it was executed *after* disabling non-boot CPUs >> > but then the function cannot be called in other contexts. >> > >> > I am not sure I will be capable of developing the proper fix as I do not >> > have the hardware and I do not know all stuff happening in sh suspend. >> > Probably reverting this and living with non-locked path would be the >> > safest choice. >> > >> > [1] https://patchwork.kernel.org/patch/9778903/ >> >> AFAIU, all syscore stuff runs in atomic context. > > Indeed... The confusing part is that this code is syscore only from > the name, it is not hooked in to syscore_ops. Although going by call > chain (through sh clocksource drivers) we end up in > timekeeping_suspend() which is a syscore op. > > I wonder whether it would be useful - after reverting my commit - to add > an assert (which is a stronger API requirement than only documentation "may > only be called during the system core (syscore) suspend") like: > WARN_ON(num_online_cpus() > 1)); > as without mutexes this should not be executed with more than one online > CPU. Or maybe WARN_ON_ONCE(!in_atomic())? I'm queuing up a revert of the $subject commit. Thanks, Rafael
Re: PCI / PM: Crashes in PME scan during system suspend
On Tuesday, April 18, 2017 08:49:39 AM Geert Uytterhoeven wrote:
> Hi Lukas,
>
> On Sun, Apr 16, 2017 at 9:55 AM, Lukas Wunner wrote:
> > On Sat, Apr 15, 2017 at 12:27:31AM +0200, Rafael J. Wysocki wrote:
> >> On Friday, April 14, 2017 10:22:49 AM Lukas Wunner wrote:
> >> > Below is a tentative patch which moves PME polling to a freezable
> >> > workqueue, so it is frozen before the host bridge is suspended.
> >> > Geert, Laurent, could you test this?
> >> >
> >> > The patch may be problematic in that pci_pme_list_scan() acquires
> >> > pci_pme_list_mutex, which is also acquired by pci_pme_active(),
> >> > which gets called when devices are suspended -- *after* the worker
> >> > has been frozen. I'm not really familiar with the freezer, can it
> >> > happen that the worker is frozen while holding the mutex? If so
> >> > this would deadlock. Rafael?
> >>
> >> That depends on the worker, precisely on where it calls try_to_freeze().
> >
> > pci_pme_list_scan() doesn't call try_to_freeze() at all, that's what had
> > me confused. Neither do many other workers that are scheduled to
> > system_freezable_wq.
> >
> > However after familiarizing myself a bit more with the freezer it seems
> > that is fine. The freezer prevents new workers from being scheduled and
> > then schedules the existing workers for up to 20 seconds to allow them
> > to finish. The call to try_to_freeze() is thus only needed for long
> > running workers or for threads which are in some infinite loop.
> >
> > Below is the same patch with a proper commit message, it's mostly copied
> > and pasted from Geert's comprehensive report. Still needs to be tested
> > by Geert and/or Laurent as I was unable reproduce the issue on my x86
> > laptop. Perhaps on x86 the OS is not responsible for disabling the PCI
> > clock (but presumably the BIOS) or x86 doesn't emit this kind of fault.
>
> Thanks, works fine (see below)!
>
> > -- >8 --
> > Subject: [PATCH] PCI: Freeze PME scan before suspending devices
> >
> > Laurent Pinchart reported that the Renesas R-Car H2 Lager board
> > (r8a7790) crashes during suspend tests. Geert Uytterhoeven managed to
> > reproduce the issue on an M2-W Koelsch board (r8a7791):
> >
> > It occurs when the PME scan runs, once per second. During PME scan, the
> > PCI host bridge (rcar-pci) registers are accessed while its module clock
> > has already been disabled, leading to the crash.
> >
> > The issue only occurs during suspend tests, after writing either
> > "platform" or "processors" to /sys/power/pm_test. It does not (or is
> > less likely) to happen during full system suspend ("core" or "none")
> > because system suspend also disables timers, and thus the workqueue
> > handling PME scans no longer runs. Geert believes the issue may still
> > happen in the small window between disabling module clocks and disabling
> > timers.
>
> It can also be reproduced easily by configuring s2ram to use s2idle instead
> of deep suspend, which is a real usecase:
>
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo s2idle > /sys/power/mem_sleep
> # echo mem > /sys/power/state
>
> > Rafael Wysocki agrees that PME scans should be suspended before the host
> > bridge registers become inaccessible. To that end, queue the task on a
> > workqueue that gets frozen before devices suspend.
> >
> > How to reproduce:
> >
> > # echo 0 > /sys/module/printk/parameters/console_suspend
> > # echo platform > /sys/power/pm_test# Or "processors"
> > # echo mem > /sys/power/state
> >
> > Make sure CONFIG_PCI_RCAR_GEN2 and CONFIG_USB_OHCI_HCD_PCI are
> > enabled.
> >
> > PM: Syncing filesystems ... [ 38.566237] done.
> > PM: Preparing system for sleep (mem)
> > Freezing user space processes ... [ 38.579813] (elapsed 0.001 seconds)
> > done.
> > Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> > PM: Suspending system (mem)
> > PM: suspend of devices complete after 152.456 msecs
> > PM: late suspend of devices complete after 2.809 msecs
> > PM: noirq suspend of devices complete after 29.863 msecs
> > suspend debug: Waiting for 5 second(s).
> > Unhandled fault: asynchronous external abort (0x1211) at 0x
> > pgd = c0003000
> > [] *pgd=8040004003, *pmd=
> > Internal error: : 1211 [#1] SMP ARM
> > Modules lin
Re: PCI / PM: Crashes in PME scan during system suspend
On Friday, April 14, 2017 10:22:49 AM Lukas Wunner wrote:
> On Tue, Feb 14, 2017 at 12:26:01PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, February 14, 2017 10:31:38 AM Geert Uytterhoeven wrote:
> > > Laurent Pinchart reported that r8a7790/Lager crashes during suspend tests.
> > >
> > > I managed to reproduce the issue on r8a7791/koelsch:
> > > - It only happens during suspend tests, after writing either "platform"
> > > or "processors" to /sys/power/pm_test,
> > > - It does not (or is less likely) to happen during full system suspend
> > > ("core" or "none").
> > >
> > > More investigation shows this happens when the PME scan runs, once per
> > > second. During PME scan, the PCI host bridge (rcar-pci) registers are
> > > accessed while the host bridge's module clock has already been disabled,
> > > leading to a crash.
> >
> > OK, so clearly PME scans should be suspended before the host bridge
> > registers become inaccessible.
> >
> > Another question, though, is whether or not PME scans are actually necessary
> > on the affected platforms at all.
>
> I'm not seeing a fix for this in linux-next, am I missing something?
> Has anyone looked into it or is the issue still open?
It is still open AFAICS.
> Below is a tentative patch which moves PME polling to a freezable
> workqueue, so it is frozen before the host bridge is suspended.
> Geert, Laurent, could you test this?
>
> The patch may be problematic in that pci_pme_list_scan() acquires
> pci_pme_list_mutex, which is also acquired by pci_pme_active(),
> which gets called when devices are suspended -- *after* the worker
> has been frozen. I'm not really familiar with the freezer, can it
> happen that the worker is frozen while holding the mutex? If so
> this would deadlock. Rafael?
That depends on the worker, precisely on where it calls try_to_freeze().
That said I think it won't do that while holding any locks. :-)
Thanks,
Rafael
Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven
wrote:
> Hi Mark,
>
> On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland wrote:
>> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote:
>>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t
>>> state)
>>> static int psci_system_suspend_enter(suspend_state_t state)
>>> {
>>> switch (state) {
>>> + case PM_SUSPEND_MEM:
>>> + if (!psci_system_suspend_is_power_down ||
>>> + !wakeup_source_available())
>>> + return cpu_suspend(0, psci_system_suspend);
>>> + /* fall through */
>>
>> I don't believe that this is the correct place to handle this.
>>
>> The wakeup_source_available() check *might* be ok, though even with that
>> I'd rather we rejected the request rather than trying to fall back to a
>> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression.
>
> If we reject the request here, I think the PM core has to be modified to
> try again using a shallower state. Note that it would be better to reject
> the state in the .valid() callback instead of in .enter().
>
> You also have to consider this is dynamic not static.
> I.e. the availability of other wake-up sources may change at runtime (cfr.
> the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls
> which states are available) is initialized from suspend_set_ops(), and not
> changed later.
>
> Perhaps pm_sleep_states[] should be updated every time the wakeup_sources
> list is changed?
No, the definitions of sleep states *are* static. They have to be, or
user space won't know what sleep state it is asking for.
And, as I said in my last reply to Sudeep, the list of possible wakeup
devices for the given state is part of that definition. The sysfs
"wakeup" interface is on top of that, not the other way around (which
seems seems to be what you would want).
Thanks,
Rafael
Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
On Wed, Feb 22, 2017 at 3:32 PM, Sudeep Holla wrote:
>
>
> On 22/02/17 13:38, Geert Uytterhoeven wrote:
>> Hi Sudeep,
>>
>> On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla wrote:
>>> On 22/02/17 01:14, Rafael J. Wysocki wrote:
>>>> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:
>>>
>>> [...]
>>>
>>>>> I take this back, you have everything you need in place, nothing needs
>>>>> to be done. I just checked again. If I don't register PSCI suspend_ops,
>>>>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
>>>>> which is exactly what we need. Again we don't support standby/shallow
>>>>> state on ARM64/PSCI.
>>>>
>>>> Except for one thing which may or may not be a concern here.
>>>>
>>>> Suspend to idle should only go into states in which all of the available
>>>> wakeup
>>>> devices work. If there are devices that cannot wake you up from a given
>>>> state,
>>>> this isn't "idle" any more, is it?
>>>>
>>>
>>> True. In this Renasas platform, since the platform doesn't have PSCI
>>> system suspend, we can only support s2idle and not s2ram. In this case
>>
>> Not correct: this Renesas platform does have PSCI system suspend.
>> So s2ram "works" (it suspends the system, which can be resumed by a switch)
>>
>
> Ah OK. Sorry for misunderstanding the platform support.
>
>>> we don't ask platform to enter some system state whereas we suspend all
>>> the devices(leaving wakeup capable devices active) and ask platform to
>>> enter deepest idle state on all the CPUs. I still don't understand the
>>> issue Geert is facing.
>>
>> PSCI system suspend does not support wake-up sources configured from Linux.
>> Hence I cannot use PSCI system suspend if any wake-up sources have been
>> configured from Linux, and I expect to be able to use them for wake-up.
>>
>
> OK, I thought I had told this before. What do you mean by PSCI system
> suspend can't wakeup from the configured wakeup source. You just said
> above that you can wake up from the switch.
>
> Just enabling the wakeup sources in Linux doesn't mean you can enter
> system suspend anytime. You must enter only the state from which you can
> resume. And in your case if you can't wakeup from WLAN or wakeup source
> you have configured then simply don't enter system suspend.
Well, not quite.
The sysfs wakeup setting for devices only means whether or not to
enable the generation of wakeup signals for them while suspending. It
allows to *prevent* devices from waking up the system, but it doesn't
guarantee that they will actually wake up if enabled.
Now, the platform doesn't decide on the sleep state it will go to on
the basis of what devices have been enabled to wake up the system.
The states ("shallow", "deep") have to be defined upfront, including
what devices can wake up from the "shallow" and what devices can wake
up from the "deep" states (these lists need not be the same). [As I
said before, the assumption is that all of them will be able to wake
up the system from suspend-to-idle.]
So, if user space triggers a transition to the "shallow" state, say,
it will be possible to wake up the system from it by devices that (a)
can wake it up from the "shallow" state as defined for the given
platform and (b) have been enabled to wake up the system via sysfs.
Conversely, if you have a system power state such that only a subset
of devices can wake up from it, it needs to be defined as either
"shallow" or "deep" and the list of possible wakeup sources is part of
that definition.
Thanks,
Rafael
Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
On Wed, Feb 22, 2017 at 2:14 PM, Geert Uytterhoeven wrote: > Hi Rafael, > > On Wed, Feb 22, 2017 at 2:14 AM, Rafael J. Wysocki wrote: >> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote: >>> On 21/02/17 18:27, Sudeep Holla wrote: >>> > On 21/02/17 17:51, Sudeep Holla wrote: >>> >> On 21/02/17 17:34, Geert Uytterhoeven wrote: >>> >>> That's more or less what /sys/power/mem_sleep does, though. >>> >> >>> >> OK, I will go through that in detail. >>> > >>> > OK, I went through the patch and the main intention is was added. >>> > So I will begin by summarizing my understanding: >>> > >>> > A new suspend interface(/sys/power/mem_sleep) is added to allow the >>> > "mem" string in /sys/power/state to represent multiple things that can >>> > be selected. >>> > >>> > Before: >>> > A. echo freeze > /sys/power/state ---> Enters s2idle >>> > B. echo mem > /sys/power/state ---> Enters s2r(a.k.a now deep mem sleep) >>> > >>> > After: >>> > 1. echo freeze > /sys/power/state ---> Enters s2idle still same >>> > 2. echo s2idle > /sys/power/mem_sleep >>> >echo mem > /sys/power/state ---> Also enter s2idle >>> > 3. echo deep > /sys/power/mem_sleep >>> >echo mem > /sys/power/state ---> Also enter s2r(same as [B] above) >>> > >>> > Please note I have carefully dropped standby/shallow as we will not >>> > support that state on ARM64 platforms(refer previous discussions for the >>> > same) >>> > >>> > Now IIUC, you need 2 above. So, since this new interface allow mem to >>> > mean "s2idle", we need to fix the core to register default suspend_ops >>> > to achieve what you need. >>> >>> I take this back, you have everything you need in place, nothing needs >>> to be done. I just checked again. If I don't register PSCI suspend_ops, >>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep >>> which is exactly what we need. Again we don't support standby/shallow >>> state on ARM64/PSCI. >> >> Except for one thing which may or may not be a concern here. >> >> Suspend to idle should only go into states in which all of the available >> wakeup >> devices work. If there are devices that cannot wake you up from a given >> state, >> this isn't "idle" any more, is it? > > Indeed. And I have no problem with handling wake-up sources from Linux, > as Linux knows how to handle them. > >> As for the device wakeup disable/enable interface, it is for controlling >> whether or not a given device should be allowed to generate wakeup signals at >> all. > > OK. So it's not guaranteed that it will actually work... No, it is not. Enabling generation of wakeup signals at a device doesn't guarantee that the interrupt (or GPIO etc) controller will be functional when those signals reach it, for example. There actually is no way to guarantee that in general. In the ACPI land, for example, devices may be able to wake up the system from S3, but not from S4 or S5, and you can't say "I want that device to wake up the system from S4", because that may be physically impossible to achieve. >> The information on what states a given device can wake up the system from is >> platform-specific and generally would need to be taken into consideration at >> the platform level. > > So that's PSCI on arm64? > But the PSCI specification doesn't handle that. In theory, that should be some code that knows how the platform is configured and can set up things to work as expected. That's why we have all of the platform hooks, syscore operations etc (of course, all of that is not needed for suspend to idle, because it is entered via the idle path and wakeup signals for wakeup devices should be handled then). Thanks, Rafael
Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote: > > On 21/02/17 18:27, Sudeep Holla wrote: > > > > > > On 21/02/17 17:51, Sudeep Holla wrote: > >> > >> > >> On 21/02/17 17:34, Geert Uytterhoeven wrote: > > > > [...] > > > >>> > >>> The SoC can wake-up. It's just not guaranteed that it can wake-up using > >>> the wakeup-source configured from Linux. Which wakeup-sources are > >>> available > >>> depends on the actual PSCI implementation. It's not specified by the PSCI > >>> specification. > >>> > Just botching whatever shallow state you can enter on a particular SoC > into standard "mem" state sounds *horrible* to me. > >>> > >>> That's more or less what /sys/power/mem_sleep does, though. > >>> > >> > >> OK, I will go through that in detail. > >> > > > > OK, I went through the patch and the main intention is was added. > > So I will begin by summarizing my understanding: > > > > A new suspend interface(/sys/power/mem_sleep) is added to allow the > > "mem" string in /sys/power/state to represent multiple things that can > > be selected. > > > > Before: > > A. echo freeze > /sys/power/state ---> Enters s2idle > > B. echo mem > /sys/power/state ---> Enters s2r(a.k.a now deep mem sleep) > > > > After: > > 1. echo freeze > /sys/power/state ---> Enters s2idle still same > > 2. echo s2idle > /sys/power/mem_sleep > >echo mem > /sys/power/state ---> Also enter s2idle > > 3. echo deep > /sys/power/mem_sleep > >echo mem > /sys/power/state ---> Also enter s2r(same as [B] above) > > > > Please note I have carefully dropped standby/shallow as we will not > > support that state on ARM64 platforms(refer previous discussions for the > > same) > > > > Now IIUC, you need 2 above. So, since this new interface allow mem to > > mean "s2idle", we need to fix the core to register default suspend_ops > > to achieve what you need. > > I take this back, you have everything you need in place, nothing needs > to be done. I just checked again. If I don't register PSCI suspend_ops, > I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep > which is exactly what we need. Again we don't support standby/shallow > state on ARM64/PSCI. Except for one thing which may or may not be a concern here. Suspend to idle should only go into states in which all of the available wakeup devices work. If there are devices that cannot wake you up from a given state, this isn't "idle" any more, is it? As for the device wakeup disable/enable interface, it is for controlling whether or not a given device should be allowed to generate wakeup signals at all. The information on what states a given device can wake up the system from is platform-specific and generally would need to be taken into consideration at the platform level. Thanks, Rafael
Re: PCI / PM: Crashes in PME scan during system suspend
On Tuesday, February 14, 2017 10:31:38 AM Geert Uytterhoeven wrote:
> Hi all,
>
> Laurent Pinchart reported that r8a7790/Lager crashes during suspend tests.
>
> I managed to reproduce the issue on r8a7791/koelsch:
> - It only happens during suspend tests, after writing either "platform"
> or "processors" to /sys/power/pm_test,
> - It does not (or is less likely) to happen during full system suspend
> ("core" or "none").
>
> More investigation shows this happens when the PME scan runs, once per
> second. During PME scan, the PCI host bridge (rcar-pci) registers are
> accessed while the host bridge's module clock has already been disabled,
> leading to a crash.
OK, so clearly PME scans should be suspended before the host bridge
registers become inaccessible.
Another question, though, is whether or not PME scans are actually necessary
on the affected platforms at all.
> With "core" or "none", system suspend also disables timers, and thus the
> workqueue handling PME scan no longer runs. I believe the issue can still
> happen, as there's a small window between disabling module clocks and
> disabling timers.
>
> Lukas' patch "[PATCH v2] PCI: pciehp: Don't enable PME on runtime suspend"
> (http://lkml.iu.edu/hypermail/linux/kernel/1702.0/03245.html) is not
> sufficient to fix the issue.
>
> Note that the issue was not introduced by commit 68db9bc81436 ("PCI:
> pciehp: Add runtime PM support for PCIe hotplug ports"): I managed to
> trigger it on 68db9bc81436^ too, albeit not at first try.
>
> Do you have a clue?
Pretty much. :-)
The PME scans cannot run on a suspended host bridge.
> Shall I bisect it? I have no idea when the issue appeared first, or if it ever
> worked.
It's never worked IMO.
Thanks,
Rafael
Re: [PATCH] PM / Domains: Do not print PM domain add error message if EPROBE_DEFER
On Wednesday, November 30, 2016 01:24:56 PM Geert Uytterhoeven wrote:
> EPROBE_DEFER is not an error, hence printing an error message like
>
> renesas_irqc e61c.interrupt-controller: failed to add to PM domain
> always-on: -517
>
> may confuse the user.
>
> Suppress the error message in case of EPROBE_DEFER to fix this.
>
> Reported-by: Yoshihiro Shimoda
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/base/power/domain.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 64164aac6ae3990a..f22137b52dcb641b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2032,8 +2032,9 @@ int genpd_dev_pm_attach(struct device *dev)
> mutex_unlock(&gpd_list_lock);
>
> if (ret < 0) {
> - dev_err(dev, "failed to add to PM domain %s: %d",
> - pd->name, ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to add to PM domain %s: %d",
> + pd->name, ret);
> goto out;
> }
>
>
Applied (with ACKs etc).
Thanks,
Rafael
Re: [PATCH] cpufreq: dt: Add support for r8a7743 and r8a7745
On Wednesday, November 16, 2016 03:39:06 PM Viresh Kumar wrote: > On 16-11-16, 11:05, Geert Uytterhoeven wrote: > > Add the compatible strings for supporting the generic cpufreq driver on > > the Renesas RZ/G1M (r8a7743) and RZ/G1E (r8a7745) SoCs. > > > > Signed-off-by: Geert Uytterhoeven > > --- > > Support for RZ/G1M and RZ/G1E is expected to land in v4.10. > > Acked-by: Viresh Kumar Applied. Thanks, Rafael
Re: [PATCH] cpufreq: dt: Add support for r8a7792
On Wednesday, September 07, 2016 10:32:59 AM Viresh Kumar wrote:
> On 06-09-16, 14:18, Geert Uytterhoeven wrote:
> > Add the compatible string for supporting the generic cpufreq driver on
> > the Renesas R-Car V2H (r8a7792) SoC.
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > Untested due to lack of hardware.
> > ---
> > drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 285ed3e6494e9d22..276378a43321a4e0 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -52,6 +52,7 @@ static const struct of_device_id machines[] __initconst =
> > {
> > { .compatible = "renesas,r8a7779", },
> > { .compatible = "renesas,r8a7790", },
> > { .compatible = "renesas,r8a7791", },
> > + { .compatible = "renesas,r8a7792", },
> > { .compatible = "renesas,r8a7793", },
> > { .compatible = "renesas,r8a7794", },
> > { .compatible = "renesas,sh73a0", },
>
> Acked-by: Viresh Kumar
Applied.
Thanks,
Rafael
Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()
On Tuesday, June 28, 2016 06:26:36 PM Geert Uytterhoeven wrote: > Hi Ulf, Rafael, > > On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson wrote: > > When the pm_runtime_force_suspend|resume() helpers were invented, we still > > had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options. > > > > To make sure these helpers worked for all combinations and without > > introducing too much of complexity, the device was always resumed in > > pm_runtime_force_resume(). > > > > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was > > unset, we needed to resume the device as the subsystem/driver couldn't > > rely on using runtime PM to do it. > > > > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it > > removed this combination, of using CONFIG_PM_SLEEP without the earlier > > CONFIG_PM_RUNTIME. > > > > For this reason we can now rely on the subsystem/driver to use runtime PM > > to resume the device, instead of forcing that to be done in all cases. In > > other words, let's defer this to a later point when it's actually needed. > > > > Signed-off-by: Ulf Hansson > > --- > > drivers/base/power/runtime.c | 11 +++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 09e4eb1..1db7b46 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev) > > if (!pm_runtime_status_suspended(dev)) > > goto out; > > > > + /* > > +* The PM core increases the runtime PM usage count in the system PM > > +* prepare phase. If the count is greather than 1 at this point, > > someone > > +* else has also increased it. In that case, invoke the runtime > > resume > > +* callback for the device as that is likely what is expected. In > > other > > +* case we trust the subsystem/driver to runtime resume the device > > when > > +* it's actually needed. > > +*/ > > + if (atomic_read(&dev->power.usage_count) < 2) > > + goto out; > > + > > ret = pm_runtime_set_active(dev); > > if (ret) > > goto out; > > This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on > sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is > a > child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and > simple-pm-bus, cfr. > > arch/arm/boot/dts/r8a73a4-ape6evm.dts > arch/arm/boot/dts/r8a73a4.dtsi > arch/arm/boot/dts/sh73a0-kzm9g.dts > arch/arm/boot/dts/sh73a0.dtsi > > During resume, the bus clock is not enabled, causing an imprecise abort > when accessing the Ethernet controller's registers. With some debug code > added: > > PM: Syncing filesystems ... done. > PM: Preparing system for sleep (mem) > Freezing user space processes ... (elapsed 0.001 seconds) done. > Double checking all user space processes after OOM killer > disable... (elapsed 0.000 seconds) > Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > PM: Suspending system (mem) > smsc911x: smsc911x_suspend > simple-pm-bus fec1.bus: simple_pm_bus_suspend > PM: suspend of devices complete after 21.484 msecs > PM: suspend devices took 0.030 seconds > PM: late suspend of devices complete after 0.488 msecs > cpg_div6_clock_disable: zb > rmobile_pd_power_down: a3sp > PM: noirq suspend of devices complete after 8.300 msecs > Disabling non-boot CPUs ... > CPU1: shutdown > > PM: early resume of devices complete after 0.488 msecs > simple-pm-bus fec1.bus: simple_pm_bus_resume > smsc911x: smsc911x_resume > Unhandled fault: imprecise external abort (0x1406) at 0xb6f40068 > pgd = deedc000 > [b6f40068] *pgd=5f774831, *pte=4087659f, *ppte=40876e7e > Internal error: : 1406 [#1] SMP ARM > Modules linked in: > CPU: 0 PID: 1062 Comm: s2ram Not tainted > 4.7.0-rc5-kzm9g-00391-g8f5105e9303a-dirty #765 > Hardware name: Generic SH73A0 (Flattened Device Tree) > task: dedc0840 ti: deee6000 task.ti: deee6000 > PC is at __smsc911x_reg_read+0x1c/0x60 > LR is at smsc911x_resume+0x78/0xd0 > > After reverting this patch, resume succeeds again, as the zb clock is enabled > first: > > PM: Syncing filesystems ... done. > PM: Preparing system for sleep (mem) > Freezing user space processes ... (elapsed 0.001 seconds) done. > Double checking all user space processes after OOM killer > disable... (elapsed 0.000 seconds) > Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. > PM: Suspending system (mem) > smsc911x: smsc911x_suspend > simple-pm-bus fec1.bus: simple_pm_bus_suspend > PM: suspend of devices complete after 22.460 msecs > PM: suspend devices took 0.030 seconds > PM: late suspend of d
Re: [PATCH v2] PM / Runtime: Only force-resume device if it has been force-suspended
On Thu, Apr 21, 2016 at 10:57 PM, Laurent Pinchart wrote: > Hi Rafael, > > On Thursday 21 Apr 2016 21:52:56 Rafael J. Wysocki wrote: >> On Thursday, April 21, 2016 02:52:55 AM Laurent Pinchart wrote: >> > The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are >> > designed to help driver being RPM-centric by offering an easy way to >> > manage runtime PM state during system suspend and resume. The first >> > function will force the device into runtime suspend at system suspend >> > time, while the second one will perform the reverse operation at system >> > resume time. >> > >> > However, the pm_runtime_force_resume() really forces resume, regardless >> > of whether the device was running or already suspended before the call >> > to pm_runtime_force_suspend(). This results in devices being runtime >> > resumed at system resume time when they shouldn't. >> > >> > Fix this by recording whether the device has been forcefully suspended >> > in pm_runtime_force_suspend() and condition resume in >> > pm_runtime_force_resume() to that state. >> > >> > All current users of pm_runtime_force_resume() call the function >> > unconditionally in their system resume handler (some actually set it as >> > the resume handler), all after calling pm_runtime_force_suspend() at >> > system suspend time. The change in behaviour should thus be safe. >> > >> > Signed-off-by: Laurent Pinchart >> > >> > Reviewed-by: Kevin Hilman >> >> Ulf, any comments? > > Ulf has proposed a different approach in "[PATCH] PM / Runtime: Defer resuming > of the device in pm_runtime_force_resume()". I agree that using usage_count is > better than introducing a new state flag in struct dev_pm_info, with a caveat: > it doesn't work properly :-). We would have to fix genpd first, as commented > in a reply to Ulf's patch. OK, thanks! Since I'd prefer to avoid adding more state flags too, I'll let you guys noodle around this for a while more. :-)
Re: [PATCH v2] PM / Runtime: Only force-resume device if it has been force-suspended
On Thursday, April 21, 2016 02:52:55 AM Laurent Pinchart wrote:
> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> designed to help driver being RPM-centric by offering an easy way to
> manage runtime PM state during system suspend and resume. The first
> function will force the device into runtime suspend at system suspend
> time, while the second one will perform the reverse operation at system
> resume time.
>
> However, the pm_runtime_force_resume() really forces resume, regardless
> of whether the device was running or already suspended before the call
> to pm_runtime_force_suspend(). This results in devices being runtime
> resumed at system resume time when they shouldn't.
>
> Fix this by recording whether the device has been forcefully suspended
> in pm_runtime_force_suspend() and condition resume in
> pm_runtime_force_resume() to that state.
>
> All current users of pm_runtime_force_resume() call the function
> unconditionally in their system resume handler (some actually set it as
> the resume handler), all after calling pm_runtime_force_suspend() at
> system suspend time. The change in behaviour should thus be safe.
>
> Signed-off-by: Laurent Pinchart
> Reviewed-by: Kevin Hilman
Ulf, any comments?
> ---
> drivers/base/power/runtime.c | 19 ---
> include/linux/pm.h | 1 +
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> Changes since v1:
>
> - Fix typos
> - Protect the is_force_suspended flag modifications with power.lock
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 4c7055009bd6..8fc7fba811fa 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1400,6 +1400,7 @@ void pm_runtime_init(struct device *dev)
> pm_suspend_ignore_children(dev, false);
> dev->power.runtime_auto = true;
>
> + dev->power.is_force_suspended = false;
> dev->power.request_pending = false;
> dev->power.request = RPM_REQ_NONE;
> dev->power.deferred_resume = false;
> @@ -1457,6 +1458,7 @@ void pm_runtime_remove(struct device *dev)
> int pm_runtime_force_suspend(struct device *dev)
> {
> int (*callback)(struct device *);
> + unsigned long flags;
> int ret = 0;
>
> pm_runtime_disable(dev);
> @@ -1475,6 +1477,10 @@ int pm_runtime_force_suspend(struct device *dev)
> goto err;
>
> pm_runtime_set_suspended(dev);
> + spin_lock_irqsave(&dev->power.lock, flags);
> + dev->power.is_force_suspended = true;
> + spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> return 0;
> err:
> pm_runtime_enable(dev);
> @@ -1483,13 +1489,13 @@ err:
> EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
>
> /**
> - * pm_runtime_force_resume - Force a device into resume state.
> + * pm_runtime_force_resume - Force a device into resume state if needed.
> * @dev: Device to resume.
> *
> * Prior invoking this function we expect the user to have brought the device
> * into low power state by a call to pm_runtime_force_suspend(). Here we
> reverse
> - * those actions and brings the device into full power. We update the
> runtime PM
> - * status and re-enables runtime PM.
> + * those actions and bring the device back to its runtime PM state before
> forced
> + * suspension. We update the runtime PM status and re-enable runtime PM.
> *
> * Typically this function may be invoked from a system resume callback to
> make
> * sure the device is put into full power state.
> @@ -1497,8 +1503,12 @@ EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
> int pm_runtime_force_resume(struct device *dev)
> {
> int (*callback)(struct device *);
> + unsigned long flags;
> int ret = 0;
>
> + if (!dev->power.is_force_suspended)
> + goto out;
> +
> callback = RPM_GET_CALLBACK(dev, runtime_resume);
>
> if (!callback) {
> @@ -1510,6 +1520,9 @@ int pm_runtime_force_resume(struct device *dev)
> if (ret)
> goto out;
>
> + spin_lock_irqsave(&dev->power.lock, flags);
> + dev->power.is_force_suspended = false;
> + spin_unlock_irqrestore(&dev->power.lock, flags);
> pm_runtime_set_active(dev);
> pm_runtime_mark_last_busy(dev);
> out:
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 6a5d654f4447..bec15e0f244e 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -596,6 +596,7 @@ struct dev_pm_info {
> unsigned intuse_autosuspend:1;
> unsigned inttimer_autosuspends:1;
> unsigned intmemalloc_noio:1;
> + unsigned intis_force_suspended:1;
> enum rpm_requestrequest;
> enum rpm_status runtime_status;
> int runtime_error;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
