| 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: <CAA+D8ANLOxnfj9cWhbTUWHuFGt5Qb-upTmKfCyNxEb1ChJKjbg@mail.gmail.com>
Date: Tue, 1 Jul 2025 10:16:21 +0800
From: Shengjiu Wang <shengjiu.wang@...il.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Shengjiu Wang <shengjiu.wang@....com>, andersson@...nel.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
linux-remoteproc@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process
On Tue, Jul 1, 2025 at 12:49 AM Mathieu Poirier
<mathieu.poirier@...aro.org> wrote:
>
> On Thu, Jun 26, 2025 at 09:32:06AM +0800, Shengjiu Wang wrote:
> > On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
> > <mathieu.poirier@...aro.org> wrote:
> > >
> > > On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@...il.com> wrote:
> > > >
> > > > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > > > <mathieu.poirier@...aro.org> wrote:
> > > > >
> > > > > Good day,
> > > > >
> > > > > On Wed, Jun 18, 2025 at 02:26:43PM +0800, Shengjiu Wang wrote:
> > > > > > when recovery is triggered, rproc_stop() is called first then
> > > > > > rproc_start(), but there is no rproc_unprepare_device() and
> > > > > > rproc_prepare_device() in the flow.
> > > > > >
> > > > > > So power enablement needs to be moved from prepare callback to start
> > > > > > callback, power disablement needs to be moved from unprepare callback
> > > > > > to stop callback, loading elf function also needs to be moved to start
> > > > > > callback, the load callback only store the firmware handler.
> > > > > >
> > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@....com>
> > > > > > ---
> > > > > > drivers/remoteproc/imx_dsp_rproc.c | 58 ++++++++++++++++++------------
> > > > > > 1 file changed, 36 insertions(+), 22 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > index 5ee622bf5352..9b9cddb224b0 100644
> > > > > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > @@ -122,6 +122,7 @@ enum imx_dsp_rp_mbox_messages {
> > > > > > * @ipc_handle: System Control Unit ipc handle
> > > > > > * @rproc_work: work for processing virtio interrupts
> > > > > > * @pm_comp: completion primitive to sync for suspend response
> > > > > > + * @firmware: firmware handler
> > > > > > * @flags: control flags
> > > > > > */
> > > > > > struct imx_dsp_rproc {
> > > > > > @@ -139,6 +140,7 @@ struct imx_dsp_rproc {
> > > > > > struct imx_sc_ipc *ipc_handle;
> > > > > > struct work_struct rproc_work;
> > > > > > struct completion pm_comp;
> > > > > > + const struct firmware *firmware;
> > > > > > u32 flags;
> > > > > > };
> > > > > >
> > > > > > @@ -211,6 +213,7 @@ static const struct imx_rproc_att imx_dsp_rproc_att_imx8ulp[] = {
> > > > > >
> > > > > > /* Initialize the mailboxes between cores, if exists */
> > > > > > static int (*imx_dsp_rproc_mbox_init)(struct imx_dsp_rproc *priv);
> > > > > > +static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw);
> > > > > >
> > > > > > /* Reset function for DSP on i.MX8MP */
> > > > > > static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv)
> > > > > > @@ -402,8 +405,24 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
> > > > > > const struct imx_dsp_rproc_dcfg *dsp_dcfg = priv->dsp_dcfg;
> > > > > > const struct imx_rproc_dcfg *dcfg = dsp_dcfg->dcfg;
> > > > > > struct device *dev = rproc->dev.parent;
> > > > > > + struct rproc_mem_entry *carveout;
> > > > > > int ret;
> > > > > >
> > > > > > + pm_runtime_get_sync(dev);
> > > > > > +
> > > > > > + /*
> > > > > > + * Clear buffers after pm rumtime for internal ocram is not
> > > > > > + * accessible if power and clock are not enabled.
> > > > > > + */
> > > > > > + list_for_each_entry(carveout, &rproc->carveouts, node) {
> > > > > > + if (carveout->va)
> > > > > > + memset(carveout->va, 0, carveout->len);
> > > > > > + }
> > > > > > +
> > > > > > + ret = imx_dsp_rproc_elf_load_segments(rproc, priv->firmware);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > switch (dcfg->method) {
> > > > > > case IMX_RPROC_MMIO:
> > > > > > ret = regmap_update_bits(priv->regmap,
> > > > > > @@ -446,6 +465,7 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
> > > > > >
> > > > > > if (rproc->state == RPROC_CRASHED) {
> > > > > > priv->flags &= ~REMOTE_IS_READY;
> > > > > > + pm_runtime_put_sync(dev);
> > > > >
> > > > > From this patch I understand that for a recovery to be successful, the
> > > > > remote processor _has_ to go through a hard reset. But here the PM runtime API
> > > > > is used, meaning the remote processor won't be switched off if another device in
> > > > > the same power domain still neeeds power. If that is the case, the solution in
> > > > > tihs patch won't work.
> > > >
> > > > Thanks for reviewing.
> > > > With the case you mentioned, there is software reset to make the
> > > > recovery process work.
> > >
> > >
> > > Are you talking about a manual software reset of some other mechanism?
> > >
> > > If manual software reset, the recovery may or may not work and we
> > > simply don't know when that might be. If it's another mechanism, then
> > > that mechanism should be used in all cases. Either way, I don't see
> > > how we can move forward with this patch.
> >
> > Not manual software reset, in this driver we registered .reset() function.
> > it has been called at imx_dsp_runtime_resume(), I paste the function below.
> >
> > And I have tested the case you mentioned, the recovery works.
> >
> > /* pm runtime functions */
> > static int imx_dsp_runtime_resume(struct device *dev)
> > {
> > struct rproc *rproc = dev_get_drvdata(dev);
> > struct imx_dsp_rproc *priv = rproc->priv;
> > const struct imx_dsp_rproc_dcfg *dsp_dcfg = priv->dsp_dcfg;
> > int ret;
> >
> > /*
> > * There is power domain attached with mailbox, if setup mailbox
> > * in probe(), then the power of mailbox is always enabled,
> > * the power can't be saved.
> > * So move setup of mailbox to runtime resume.
> > */
> > ret = imx_dsp_rproc_mbox_init(priv);
> > if (ret) {
> > dev_err(dev, "failed on imx_dsp_rproc_mbox_init\n");
> > return ret;
> > }
> >
> > ret = clk_bulk_prepare_enable(DSP_RPROC_CLK_MAX, priv->clks);
> > if (ret) {
> > dev_err(dev, "failed on clk_bulk_prepare_enable\n");
> > return ret;
> > }
> >
> > /* Reset DSP if needed */
> > if (dsp_dcfg->reset)
> > dsp_dcfg->reset(priv);
> >
> > return 0;
> > }
> >
>
> Thanks for the clarification. I would have been nice to have that kind of
> explanation (not the copy paste of the imx_dsp_runtime_resume() function) in the
> changelog.
Ok, will add it.
>
> That said, that is just one aspect of the things I find bizarre with this
> patchset - see below.
>
> > >
> > > >
> > > >
> > > > best regards
> > > > Shengjiu Wang
> > > >
> > > > >
> > > > > Thanks,
> > > > > Mathieu
> > > > >
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > @@ -472,6 +492,8 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
> > > > > > else
> > > > > > priv->flags &= ~REMOTE_IS_READY;
> > > > > >
> > > > > > + pm_runtime_put_sync(dev);
> > > > > > +
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > @@ -774,7 +796,6 @@ static int imx_dsp_rproc_prepare(struct rproc *rproc)
> > > > > > {
> > > > > > struct imx_dsp_rproc *priv = rproc->priv;
> > > > > > struct device *dev = rproc->dev.parent;
> > > > > > - struct rproc_mem_entry *carveout;
> > > > > > int ret;
> > > > > >
> > > > > > ret = imx_dsp_rproc_add_carveout(priv);
> > > > > > @@ -783,25 +804,6 @@ static int imx_dsp_rproc_prepare(struct rproc *rproc)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > - pm_runtime_get_sync(dev);
> > > > > > -
> > > > > > - /*
> > > > > > - * Clear buffers after pm rumtime for internal ocram is not
> > > > > > - * accessible if power and clock are not enabled.
> > > > > > - */
> > > > > > - list_for_each_entry(carveout, &rproc->carveouts, node) {
> > > > > > - if (carveout->va)
> > > > > > - memset(carveout->va, 0, carveout->len);
> > > > > > - }
> > > > > > -
> > > > > > - return 0;
> > > > > > -}
> > > > > > -
> > > > > > -/* Unprepare function for rproc_ops */
> > > > > > -static int imx_dsp_rproc_unprepare(struct rproc *rproc)
> > > > > > -{
> > > > > > - pm_runtime_put_sync(rproc->dev.parent);
> > > > > > -
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > @@ -1022,13 +1024,25 @@ static int imx_dsp_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static int imx_dsp_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > > > > > +{
> > > > > > + struct imx_dsp_rproc *priv = rproc->priv;
> > > > > > +
> > > > > > + /*
> > > > > > + * Just save the fw handler, the firmware loading will be after
> > > > > > + * power enabled
> > > > > > + */
> > > > > > + priv->firmware = fw;
> > > > > > +
>
> Why is a new firwmare variable needed? Why can't you just use rproc->firmware?
The "firmware" in "rproc->firmware" is "const char *firmware;"
* @firmware: name of firmware file to be loaded
But "const struct firmware *fw" is the result after request_firmware()
ret = request_firmware(&firmware_p, rproc->firmware, dev);
"firmware_p" is the "fw".
As the remoteproc_core.c has called request_firmware(), so we don't need to call
the request_firmware() again. so use the new "const struct firmware
*firmware;" for
saving the "fw".
Best regards
Shengjiu Wang
>
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > static const struct rproc_ops imx_dsp_rproc_ops = {
> > > > > > .prepare = imx_dsp_rproc_prepare,
> > > > > > - .unprepare = imx_dsp_rproc_unprepare,
> > > > > > .start = imx_dsp_rproc_start,
> > > > > > .stop = imx_dsp_rproc_stop,
> > > > > > .kick = imx_dsp_rproc_kick,
> > > > > > - .load = imx_dsp_rproc_elf_load_segments,
> > > > > > + .load = imx_dsp_rproc_load,
> > > > > > .parse_fw = imx_dsp_rproc_parse_fw,
> > > > > > .handle_rsc = imx_dsp_rproc_handle_rsc,
> > > > > > .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
Powered by blists - more mailing lists