| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFpm1nMEeUjygapfwtBcr25AQerX4j14MPKd5NW2-BW33g@mail.gmail.com>
Date: Mon, 18 Jul 2022 14:26:35 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Martin Kepplinger <martin.kepplinger@...i.sm>
Cc: rafael@...nel.org, khilman@...nel.org, robh@...nel.org,
krzysztof.kozlowski@...aro.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, festevam@...il.com, pavel@....cz,
kernel@...i.sm, linux-imx@....com, broonie@...nel.org,
l.stach@...gutronix.de, aford173@...il.com,
linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] power: domain: handle power supplies that need interrupts
On Mon, 18 Jul 2022 at 14:08, Martin Kepplinger
<martin.kepplinger@...i.sm> wrote:
>
> Am Montag, dem 18.07.2022 um 12:54 +0200 schrieb Ulf Hansson:
> > On Tue, 12 Jul 2022 at 14:19, Martin Kepplinger
> > <martin.kepplinger@...i.sm> wrote:
> > >
> > > If the power-domains' power-supply node (regulator) needs
> > > interrupts to work, the current setup with noirq callbacks cannot
> > > work; for example a pmic regulator on i2c, when suspending, usually
> > > already
> > > times out during suspend_noirq:
> > >
> > > [ 41.024193] buck4: failed to disable: -ETIMEDOUT
> > >
> > > So fix system suspend and resume for these power-domains by using
> > > the
> > > "outer" suspend/resume callbacks instead. Tested on the imx8mq-
> > > librem5
> > > board, but by looking at the dts, this will fix imx8mq-evk and
> > > possibly
> > > other boards too.
> > >
> > > Possibly one can find more changes than suspend/resume for this
> > > case. They
> > > can be added later when testing them.
> > >
> > > Initially system suspend problems had been discussed at
> > > https://lore.kernel.org/linux-arm-kernel/20211002005954.1367653-8-l.stach@pengutronix.de/
> > > which led to discussing the pmic that contains the regulators which
> > > serve as power-domain power-supplies:
> > > https://lore.kernel.org/linux-pm/573166b75e524517782471c2b7f96e03fd93d175.camel@puri.sm/T/
> > >
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@...i.sm>
> > > ---
> > >
> > > revision history
> > > ----------------
> > > v2: (thank you Krzysztof)
> > > * rewrite: find possible regulators' interrupts property in parents
> > > instead of inventing a new property.
> > >
> > > v1: (initial idea)
> > > https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t
> > >
> > >
> > > drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/base/power/domain.c
> > > b/drivers/base/power/domain.c
> > > index 3e86772d5fac..ca3e3500939d 100644
> > > --- a/drivers/base/power/domain.c
> > > +++ b/drivers/base/power/domain.c
> > > @@ -2298,6 +2298,28 @@ static bool genpd_present(const struct
> > > generic_pm_domain *genpd)
> > > return ret;
> > > }
> > >
> > > +/**
> > > + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs
> > > interrupts
> > > + * @genpd: Pointer to PM domain associated with the PM domain
> > > provider.
> > > + */
> > > +static void of_genpd_get_power_supply_irq(struct generic_pm_domain
> > > *pd)
> > > +{
> > > + struct device_node *dn;
> > > +
> > > + dn = of_parse_phandle(pd->dev.of_node, "power-supply", 0);
> > > + if (!dn)
> > > + return;
> > > +
> > > + while ((dn = of_get_next_parent(dn))) {
> > > + if (of_get_property(dn, "interrupts", NULL)) {
> > > + pd->domain.ops.suspend =
> > > genpd_suspend_noirq;
> > > + pd->domain.ops.resume = genpd_resume_noirq;
> > > + pd->domain.ops.suspend_noirq = NULL;
> > > + pd->domain.ops.resume_noirq = NULL;
> > > + }
> > > + }
> > > +}
> > > +
> > > /**
> > > * of_genpd_add_provider_simple() - Register a simple PM domain
> > > provider
> > > * @np: Device node pointer associated with the PM domain
> > > provider.
> > > @@ -2343,6 +2365,8 @@ int of_genpd_add_provider_simple(struct
> > > device_node *np,
> > > genpd->provider = &np->fwnode;
> > > genpd->has_provider = true;
> > >
> > > + of_genpd_get_power_supply_irq(genpd);
> > > +
> > > return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> > > @@ -2394,6 +2418,8 @@ int of_genpd_add_provider_onecell(struct
> > > device_node *np,
> > >
> > > genpd->provider = &np->fwnode;
> > > genpd->has_provider = true;
> > > +
> > > + of_genpd_get_power_supply_irq(genpd);
> > > }
> > >
> > > ret = genpd_add_provider(np, data->xlate, data);
> >
> > Overall I understand the need for this, but let me suggest a slightly
> > different approach to solve this. See below.
> >
> > I think the OF parsing looks quite platform specific. Rather than
> > adding this in the generic layer of genpd, I suggest that we move the
> > OF parsing into the genpd provider code.
> >
> > Moreover, to inform genpd that it should use the other set of
> > callbacks for system suspend/resume, let's add a new genpd
> > configuration bit. The genpd provider should then set the genpd-
> > >flag,
> > prior to calling pm_genpd_init(), to let it know that it should pick
> > the other callbacks.
> >
> > Does it make sense?
>
> the provider here would be gpcv2, right?
Correct.
> Conceptually I know what you
> mean and will try to make it work later. thanks a lot!
Great! Feel free to ping me if you need some help to put it together.
Kind regards
Uffe
Powered by blists - more mailing lists