linux-kernel - Re: [PATCH v2 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250527000510.fofehmsdhifcwlys@hiago-nb>
Date: Mon, 26 May 2025 21:05:10 -0300
From: Hiago De Franco <hiagofranco@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Peng Fan <peng.fan@....nxp.com>,
	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,
	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 Mon, May 26, 2025 at 12:07:49PM +0200, Ulf Hansson wrote:
> On Fri, 23 May 2025 at 21:17, Hiago De Franco <hiagofranco@...il.com> wrote:
> >
> > Hi Ulf,
> >
> > On Wed, May 21, 2025 at 02:11:02PM +0200, Ulf Hansson wrote:
> > > You should not provide any flag (or attach_data to
> > > dev_pm_domain_attach_list()) at all. In other words just call
> > > dev_pm_domain_attach_list(dev, NULL, &priv->pd_list), similar to how
> > > drivers/remoteproc/imx_dsp_rproc.c does it.
> > >
> > > In this way, the device_link is created by making the platform->dev
> > > the consumer and by keeping the supplier-devices (corresponding to the
> > > genpds) in RPM_SUSPENDED state.
> > >
> > > The PM domains (genpds) are then left in their current state, which
> > > should allow us to call dev_pm_genpd_is_on() for the corresponding
> > > supplier-devices, to figure out whether the bootloader turned them on
> > > or not, I think.
> > >
> > > Moreover, to make sure the genpds are turned on when needed, we also
> > > need to call pm_runtime_enable(platform->dev) and
> > > pm_runtime_get_sync(platform->dev). The easiest approach is probably
> > > to do that during ->probe() - and then as an improvement on top you
> > > may want to implement more fine-grained support for runtime PM.
> > >
> > > [...]
> > >
> > > Kind regards
> > > Uffe
> >
> > I did some tests here and I might be missing something. I used the
> > dev_pm_genpd_is_on() inside imx_rproc.c with the following changes:
> >
> > @@ -902,7 +902,12 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> >         if (dev->pm_domain)
> >                 return 0;
> >
> >         ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list);
> > +       printk("hfranco: returned pd devs is %d", ret);
> > +       for (int i = 0; i < ret; i++) {
> > +               test = dev_pm_genpd_is_on(priv->pd_list->pd_devs[i]);
> > +               printk("hfranco: returned value is %d", test);
> > +       }
> >         return ret < 0 ? ret : 0;
> >  }
> >
> > This was a quick test to check the returned value, and it always return
> > 1 for both pds, even if I did not boot the remote core.
> >
> > So I was wondering if it was because of PD_FLAG_DEV_LINK_ON, I removed
> > it and passed NULL to dev_pm_domain_attach_list().
> 
> Right, that's exactly what we should be doing.
> 
> > Booting the kernel
> > now it correctly reports 0 for both pds, however when I start the
> > remote core with a hello world firmware and boot the kernel, the CPU
> > resets with a fault reset ("Reset cause: SCFW fault reset").
> >
> > I added both pm functions to probe, just to test:
> >
> > @@ -1152,6 +1158,9 @@ static int imx_rproc_probe(struct platform_device *pdev)
> >                 goto err_put_clk;
> >         }
> >
> > +       pm_runtime_enable(dev);
> > +       pm_runtime_get_sync(dev);
> > +
> 
> Indeed, calling pm_runtime_enable() and then pm_runtime_get_sync()
> should turn on the PM domains for the device, which I assume is needed
> at some point.
> 
> Although, I wonder if this may be a bit too late, I would expect that
> you at least need to call these *before* the call to rproc_add(), as I
> assume the rproc-core may start using the device/driver beyond that
> point.
> 
> >         return 0
> >
> > Now the kernel boot with the remote core running, but it still returns
> > 0 from dev_pm_genpd_is_on(). So basically now it always returns 0, with
> > or without the remote core running.
> 
> dev_pm_genpd_is_on() is returning the current status of the PM domain
> (genpd) for the device.
> 
> Could it be that the genpd provider doesn't register its PM domains
> with the state that the HW is really in? pm_genpd_init() is the call
> that allows the genpd provider to specify the initial state.
> 
> I think we need Peng's help here to understand what goes on.
> 
> >
> > I tried to move pm_runtime_get_sync() to .prepare function but it make
> > the kernel not boot anymore (with the SCU fault reset).
> 
> Try move pm_runtime_enable() before rproc_add().

Thanks Ulf, that indeed made it work, at least now the kernel does not
reset anymore with the SCU fault reset. However I am still only getting
0 from dev_pm_genpd_is_on(), no matter what the state of the remote
core. Maybe I am missing something in between?

Peng, do you know what could be the issue here?

> 
> >
> > Do you have any suggestions? Am I doing something wrong with these PDs?
> >
> > Best regards,
> > Hiago.
> 
> Kind regards
> Uffe

Best regards,
Hiago