| 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: <20250509191308.6i3ydftzork3sv5c@hiago-nb>
Date: Fri, 9 May 2025 16:13:08 -0300
From: Hiago De Franco <hiagofranco@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>, linux-pm@...r.kernel.org,
linux-remoteproc@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Bjorn Andersson <andersson@...nel.org>,
Hiago De Franco <hiago.franco@...adex.com>, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Peng Fan <peng.fan@....nxp.com>, daniel.baluta@....com,
iuliana.prodan@....nxp.com, Fabio Estevam <festevam@...il.com>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for
remote core attachment
On Fri, May 09, 2025 at 12:37:02PM +0200, Ulf Hansson wrote:
> On Thu, 8 May 2025 at 22:28, Hiago De Franco <hiagofranco@...il.com> wrote:
> >
> > Hello,
> >
> > On Thu, May 08, 2025 at 12:03:33PM +0200, Ulf Hansson wrote:
> > > On Wed, 7 May 2025 at 18:02, Hiago De Franco <hiagofranco@...il.com> wrote:
> > > >
> > > > From: Hiago De Franco <hiago.franco@...adex.com>
> > > >
> > > > When the remote core is started before Linux boots (e.g., by the
> > > > bootloader), the driver currently is not able to attach because it only
> > > > checks for cores running in different partitions. If the core was kicked
> > > > by the bootloader, it is in the same partition as Linux and it is
> > > > already up and running.
> > > >
> > > > This adds power mode verification through the SCU interface, enabling
> > > > the driver to detect when the remote core is already running and
> > > > properly attach to it.
> > > >
> > > > Signed-off-by: Hiago De Franco <hiago.franco@...adex.com>
> > > > Suggested-by: Peng Fan <peng.fan@....com>
> > > > ---
> > > > v2: Dropped unecessary include. Removed the imx_rproc_is_on function, as
> > > > suggested.
> > > > ---
> > > > drivers/remoteproc/imx_rproc.c | 13 +++++++++++++
> > > > 1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > > > index 627e57a88db2..9b6e9e41b7fc 100644
> > > > --- a/drivers/remoteproc/imx_rproc.c
> > > > +++ b/drivers/remoteproc/imx_rproc.c
> > > > @@ -949,6 +949,19 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
> > > > if (of_property_read_u32(dev->of_node, "fsl,entry-address", &priv->entry))
> > > > return -EINVAL;
> > > >
> > > > + /*
> > > > + * If remote core is already running (e.g. kicked by
> > > > + * the bootloader), attach to it.
> > > > + */
> > > > + ret = imx_sc_pm_get_resource_power_mode(priv->ipc_handle,
> > > > + priv->rsrc_id);
> > > > + if (ret < 0)
> > > > + dev_err(dev, "failed to get power resource %d mode, ret %d\n",
> > > > + priv->rsrc_id, ret);
> > > > +
> > > > + if (ret == IMX_SC_PM_PW_MODE_ON)
> > > > + priv->rproc->state = RPROC_DETACHED;
> > > > +
> > > > return imx_rproc_attach_pd(priv);
> > >
> > > Why is it important to potentially set "priv->rproc->state =
> > > RPROC_DETACHED" before calling imx_rproc_attach_pd()?
> > >
> > > Would it be possible to do it the other way around? First calling
> > > imx_rproc_attach_pd() then get the power-mode to know if
> > > RPROC_DETACHED should be set or not?
> > >
> > > The main reason why I ask, is because of how we handle the single PM
> > > domain case. In that case, the PM domain has already been attached
> > > (and powered-on) before we reach this point.
> >
> > I am not sure if I understood correcly, let me know if I missed
> > something. From my understanding in this case it does not matter, since
> > the RPROC_DETACHED will only be a flag to trigger the attach callback
> > from rproc_validate(), when rproc_add() is called inside
> > remoteproc_core.c.
>
> Okay, I see.
>
> To me, it sounds like we should introduce a new genpd helper function
> instead. Something along the lines of this (drivers/pmdomain/core.c)
>
> bool dev_pm_genpd_is_on(struct device *dev)
> {
> struct generic_pm_domain *genpd;
> bool is_on;
>
> genpd = dev_to_genpd_safe(dev);
> if (!genpd)
> return false;
>
> genpd_lock(genpd);
> is_on = genpd_status_on(genpd);
> genpd_unlock(genpd);
>
> return is_on;
> }
>
> After imx_rproc_attach_pd() has run, we have the devices that
> correspond to the genpd(s). Those can then be passed as in-parameters
> to the above function to get the power-state of their PM domains
> (genpds). Based on that, we can decide if priv->rproc->state should be
> to RPROC_DETACHED or not. Right?
Got your idea, I think it should work yes, I am not so sure how. From
what I can see these power domains are managed by
drivers/pmdomain/imx/scu-pd.c and by enabling the debug messages I can
see the power mode is correct when the remote core is powered on:
[ 0.317369] imx-scu-pd system-controller:power-controller: cm40-pid0 : IMX_SC_PM_PW_MODE_ON
and powered off:
[ 0.314953] imx-scu-pd system-controller:power-controller: cm40-pid0 : IMX_SC_PM_PW_MODE_OFF
But I cannot see how to integrate this into the dev_pm_genpd_is_on() you
proposed. For a quick check, I added this function and it always return
NULL at dev_to_genpd_safe(). Can you help me to understand this part?
>
> In this way we don't need to export unnecessary firmware functions
> from firmware/imx/misc.c, as patch1/3 does.
>
> If you think it can work, I can help to cook a formal patch for the
> above helper that you can fold into your series. Let me know.
>
> >
> > With that we can correcly attach to the remote core running, which was
> > not possible before, where the function returns at "return
> > imx_rproc_attach_pd(priv);" with the RPROC_OFFLINE state to
> > rproc_validate().
>
> I see, thanks for clarifying!
>
> Kind regards
> Uffe
Thank you!
Hiago.
Powered by blists - more mailing lists