Re: [PATCH v4] remoteproc: imx_dsp_rproc: Add support for DSP-specific features

From: Mathieu Poirier
Date: Tue Apr 15 2025 - 09:35:28 EST

Next message: Eric Huang: "Re: [PATCH next] drm/amdkfd: Fix kfd_smi_event_process()"
Previous message: Mark Brown: "[PATCH] staging: bcm2835-audio: Validate values written to controls"
In reply to: Iuliana Prodan: "Re: [PATCH v4] remoteproc: imx_dsp_rproc: Add support for DSP-specific features"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 15 Apr 2025 at 05:04, Iuliana Prodan <iuliana.prodan@xxxxxxx> wrote:
>
> On 4/14/2025 6:41 PM, Mathieu Poirier wrote:
> > On Thu, Apr 10, 2025 at 12:30:30AM +0300, Iuliana Prodan (OSS) wrote:
> >> From: Iuliana Prodan <iuliana.prodan@xxxxxxx>
> >>
> >> Some DSP firmware requires a FW_READY signal before proceeding, while
> >> others do not.
> >> Therefore, add support to handle i.MX DSP-specific features.
> >>
> >> Implement handle_rsc callback to handle resource table parsing and to
> >> process DSP-specific resource, to determine if waiting is needed.
> >>
> >> Update imx_dsp_rproc_start() to handle this condition accordingly.
> >>
> >> Signed-off-by: Iuliana Prodan <iuliana.prodan@xxxxxxx>
> >> ---
> >> Changes in v4:
> >> - Reviews from Mathieu Poirier:
> >> - Adjusted len to include the size of struct fw_rsc_imx_dsp.
> >> - Updated len validation checks.
> >> - Review from Frank Li:
> >> - In imx_dsp_rproc_handle_rsc(), removed the goto ignored statement.
> >> - In probe(), set flags to WAIT_FW_READY to ensure the host waits
> >> for fw_ready when no vendor-specific resource is defined.
> >> - Link to v3: https://lore.kernel.org/all/20250403100124.637889-1-iuliana.prodan@xxxxxxxxxxx/
> >>
> >> Changes in v3:
> >> - Reviews from Mathieu Poirier:
> >> - Added version and magic number to vendor-specific resource table entry.
> >> - Updated defines to maintain backward compatibility with a resource table that doesn't have a vendor-specific resource.
> >> - By default, wait for `fw_ready`, unless specified otherwise.
> >> - Link to v2: https://lore.kernel.org/all/20250318215007.2109726-1-iuliana.prodan@xxxxxxxxxxx
> >>
> >> Changes in v2:
> >> - Reviews from Mathieu Poirier:
> >> - Use vendor-specific resource table entry.
> >> - Implement resource handler specific to the i.MX DSP.
> >> - Revise commit message to include recent updates.
> >> - Link to v1: https://lore.kernel.org/all/20250305123923.514386-1-iuliana.prodan@xxxxxxxxxxx/
> >>
> >> drivers/remoteproc/imx_dsp_rproc.c | 98 +++++++++++++++++++++++++++++-
> >> 1 file changed, 96 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> >> index b9bb15970966..e4212e624a91 100644
> >> --- a/drivers/remoteproc/imx_dsp_rproc.c
> >> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> >> @@ -35,9 +35,18 @@ module_param_named(no_mailboxes, no_mailboxes, int, 0644);
> >> MODULE_PARM_DESC(no_mailboxes,
> >> "There is no mailbox between cores, so ignore remote proc reply after start, default is 0 (off).");
> >>
> >> +/* Flag indicating that the remote is up and running */
> >> #define REMOTE_IS_READY BIT(0)
> >> +/* Flag indicating that the host should wait for a firmware-ready response */
> >> +#define WAIT_FW_READY BIT(1)
> >> #define REMOTE_READY_WAIT_MAX_RETRIES 500
> >>
> >> +/*
> >> + * This flag is set in the DSP resource table's features field to indicate
> >> + * that the firmware requires the host NOT to wait for a FW_READY response.
> >> + */
> >> +#define FEATURE_DONT_WAIT_FW_READY BIT(0)
> >> +
> >> /* att flags */
> >> /* DSP own area */
> >> #define ATT_OWN BIT(31)
> >> @@ -72,6 +81,10 @@ MODULE_PARM_DESC(no_mailboxes,
> >>
> >> #define IMX8ULP_SIP_HIFI_XRDC 0xc200000e
> >>
> >> +#define FW_RSC_NXP_S_MAGIC ((uint32_t)'n' << 24 | \
> >> + (uint32_t)'x' << 16 | \
> >> + (uint32_t)'p' << 8 | \
> >> + (uint32_t)'s')
> >> /*
> >> * enum - Predefined Mailbox Messages
> >> *
> >> @@ -136,6 +149,24 @@ struct imx_dsp_rproc_dcfg {
> >> int (*reset)(struct imx_dsp_rproc *priv);
> >> };
> >>
> >> +/**
> >> + * struct fw_rsc_imx_dsp - i.MX DSP specific info
> >> + *
> >> + * @len: length of the resource entry
> >> + * @magic_num: 32-bit magic number
> >> + * @version: version of data structure
> >> + * @features: feature flags supported by the i.MX DSP firmware
> >> + *
> >> + * This represents a DSP-specific resource in the firmware's
> >> + * resource table, providing information on supported features.
> >> + */
> >> +struct fw_rsc_imx_dsp {
> >> + uint32_t len;
> >> + uint32_t magic_num;
> >> + uint32_t version;
> >> + uint32_t features;
> >> +} __packed;
> >> +
> >> static const struct imx_rproc_att imx_dsp_rproc_att_imx8qm[] = {
> >> /* dev addr , sys addr , size , flags */
> >> { 0x596e8000, 0x556e8000, 0x00008000, ATT_OWN },
> >> @@ -300,6 +331,66 @@ static int imx_dsp_rproc_ready(struct rproc *rproc)
> >> return -ETIMEDOUT;
> >> }
> >>
> >> +/**
> >> + * imx_dsp_rproc_handle_rsc() - Handle DSP-specific resource table entries
> >> + * @rproc: remote processor instance
> >> + * @rsc_type: resource type identifier
> >> + * @rsc: pointer to the resource entry
> >> + * @offset: offset of the resource entry
> >> + * @avail: available space in the resource table
> >> + *
> >> + * Parse the DSP-specific resource entry and update flags accordingly.
> >> + * If the WAIT_FW_READY feature is set, the host must wait for the firmware
> >> + * to signal readiness before proceeding with execution.
> >> + *
> >> + * Return: RSC_HANDLED if processed successfully, RSC_IGNORED otherwise.
> >> + */
> >> +static int imx_dsp_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type,
> >> + void *rsc, int offset, int avail)
> >> +{
> >> + struct imx_dsp_rproc *priv = rproc->priv;
> >> + struct fw_rsc_imx_dsp *imx_dsp_rsc = rsc;
> >> + struct device *dev = rproc->dev.parent;
> >> +
> >> + if (!imx_dsp_rsc) {
> >> + dev_dbg(dev, "Invalid fw_rsc_imx_dsp.\n");
> >> + return RSC_IGNORED;
> >> + }
> >> +
> >> + /* Make sure resource isn't truncated */
> >> + if (sizeof(struct fw_rsc_imx_dsp) > avail ||
> > We agree on that part.
> >
> >> + sizeof(struct fw_rsc_imx_dsp) < imx_dsp_rsc->len) {
> > From the above, "sizeof(struct fw_rsc_imx_dsp) > imx_dsp_rsc->len" would be a
> > valid condition when it clearly isn't. I am still convinced the only
> > valid option is:
> >
> > sizeof(struct fw_rsc_imx_dsp) != imx_dsp_rsc->len)
> >
> > I am happy to change my mind but would need more information.
>
> You're right, it should be sizeof(struct fw_rsc_imx_dsp) > imx_dsp_rsc->len.
> The ->len comes from the remote size, while in Linux we need to check if
> the length is at least sizeof(struct fw_rsc_imx_dsp).

To be clear, I think it should _not_ be sizeof(struct fw_rsc_imx_dsp)
> imx_dsp_rsc->len, this is an error condition.

> This is for backwards compatibility - if someone changes the structure
> on the remote side and increases the length, in Linux we can still load
> that firmware, but probably not all features will be checked.

And nothing good can come out of that situation.

>
> If you agree with this, I'll send a v5 with this fix - s/</>.

To me, the only valid condition is:

sizeof(struct fw_rsc_imx_dsp) != imx_dsp_rsc->len)

>
> Thanks,
> Iulia
>
> > Thanks,
> > Mathieu
> >
> >> + dev_dbg(dev, "Resource fw_rsc_imx_dsp is truncated.\n");
> >> + return RSC_IGNORED;
> >> + }
> >> +
> >> + /*
> >> + * If FW_RSC_NXP_S_MAGIC number is not found then
> >> + * wait for fw_ready reply (default work flow)
> >> + */
> >> + if (imx_dsp_rsc->magic_num != FW_RSC_NXP_S_MAGIC) {
> >> + dev_dbg(dev, "Invalid resource table magic number.\n");
> >> + return RSC_IGNORED;
> >> + }
> >> +
> >> + /*
> >> + * For now, in struct fw_rsc_imx_dsp, version 0,
> >> + * only FEATURE_DONT_WAIT_FW_READY is valid.
> >> + *
> >> + * When adding new features, please upgrade version.
> >> + */
> >> + if (imx_dsp_rsc->version > 0) {
> >> + dev_warn(dev, "Unexpected fw_rsc_imx_dsp version %d.\n",
> >> + imx_dsp_rsc->version);
> >> + return RSC_IGNORED;
> >> + }
> >> +
> >> + if (imx_dsp_rsc->features & FEATURE_DONT_WAIT_FW_READY)
> >> + priv->flags &= ~WAIT_FW_READY;
> >> +
> >> + return RSC_HANDLED;
> >> +}
> >> +
> >> /*
> >> * Start function for rproc_ops
> >> *
> >> @@ -335,8 +426,8 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
> >>
> >> if (ret)
> >> dev_err(dev, "Failed to enable remote core!\n");
> >> - else
> >> - ret = imx_dsp_rproc_ready(rproc);
> >> + else if (priv->flags & WAIT_FW_READY)
> >> + return imx_dsp_rproc_ready(rproc);
> >>
> >> return ret;
> >> }
> >> @@ -936,6 +1027,7 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
> >> .kick = imx_dsp_rproc_kick,
> >> .load = imx_dsp_rproc_elf_load_segments,
> >> .parse_fw = imx_dsp_rproc_parse_fw,
> >> + .handle_rsc = imx_dsp_rproc_handle_rsc,
> >> .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> >> .sanity_check = rproc_elf_sanity_check,
> >> .get_boot_addr = rproc_elf_get_boot_addr,
> >> @@ -1053,6 +1145,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> >> priv = rproc->priv;
> >> priv->rproc = rproc;
> >> priv->dsp_dcfg = dsp_dcfg;
> >> + /* By default, host waits for fw_ready reply */
> >> + priv->flags |= WAIT_FW_READY;
> >>
> >> if (no_mailboxes)
> >> imx_dsp_rproc_mbox_init = imx_dsp_rproc_mbox_no_alloc;
> >> --
> >> 2.25.1
> >>

Next message: Eric Huang: "Re: [PATCH next] drm/amdkfd: Fix kfd_smi_event_process()"
Previous message: Mark Brown: "[PATCH] staging: bcm2835-audio: Validate values written to controls"
In reply to: Iuliana Prodan: "Re: [PATCH v4] remoteproc: imx_dsp_rproc: Add support for DSP-specific features"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]