| 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: <aGPwPW_f1qAZGmzd@p14s>
Date: Tue, 1 Jul 2025 08:27:09 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Shengjiu Wang <shengjiu.wang@...il.com>
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 01, 2025 at 10:16:21AM +0800, Shengjiu Wang wrote:
> 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".
Ah yes, of course.
>
> 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