Re: [PATCH 1/4] remoteproc: add common wc-ioremap carveout callbacks

[Ben Levinsky]

Hi Arnaud, Geert,

See my replies below

On 5/12/26 10:03 AM, Levinsky, Ben wrote:
> AMD General
> 
> 
> 
> 
> *From: *Arnaud POULIQUEN <arnaud.pouliquen@xxxxxxxxxxx>
> *Date: *Tuesday, May 12, 2026 at 2:45 AM
> *To: *Levinsky, Ben <ben.levinsky@xxxxxxx>; Bjorn Andersson 
> <andersson@xxxxxxxxxx>; Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; linux- 
> remoteproc@xxxxxxxxxxxxxxx <linux-remoteproc@xxxxxxxxxxxxxxx>
> *Cc: *Frank Li <Frank.Li@xxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; 
> Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam 
> <festevam@xxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Magnus Damm 
> <magnus.damm@xxxxxxxxx>; Patrice Chotard <patrice.chotard@xxxxxxxxxxx>; Maxime 
> Coquelin <mcoquelin.stm32@xxxxxxxxx>; Alexandre Torgue 
> <alexandre.torgue@xxxxxxxxxxx>; imx@xxxxxxxxxxxxxxx <imx@xxxxxxxxxxxxxxx>; 
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; 
> linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; linux-renesas- 
> soc@xxxxxxxxxxxxxxx <linux-renesas-soc@xxxxxxxxxxxxxxx>; linux-stm32@st-md- 
> mailman.stormreply.com <linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>; Shah, Tanmay 
> <tanmay.shah@xxxxxxx>
> *Subject: *Re: [PATCH 1/4] remoteproc: add common wc-ioremap carveout callbacks
> 
> 
> 
> On 5/11/26 23:18, Ben Levinsky wrote:
>  > Several remoteproc drivers open-code the same ioremap_wc() and
>  > iounmap() callbacks for carveout mappings. Add subsystem-private
>  > helpers in remoteproc_internal.h so those drivers can share the same
>  > implementation.
>  >
>  > Signed-off-by: Ben Levinsky <ben.levinsky@xxxxxxx>
>  > ---
>  >   drivers/remoteproc/remoteproc_internal.h | 26 +++++++++++++++++++++++-
>  >   1 file changed, 25 insertions(+), 1 deletion(-)
>  >
>  > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/ 
> remoteproc_internal.h
>  > index 0a5e15744b1d..3724a47a9748 100644
>  > --- a/drivers/remoteproc/remoteproc_internal.h
>  > +++ b/drivers/remoteproc/remoteproc_internal.h
>  > @@ -12,8 +12,9 @@
>  >   #ifndef REMOTEPROC_INTERNAL_H
>  >   #define REMOTEPROC_INTERNAL_H
>  >
>  > -#include <linux/irqreturn.h>
>  >   #include <linux/firmware.h>
>  > +#include <linux/io.h>
>  > +#include <linux/irqreturn.h>
>  >
>  >   struct rproc;
>  >
>  > @@ -122,6 +123,29 @@ rproc_find_carveout_by_name(struct rproc *rproc, const 
> char *name, ...);
>  >   void rproc_add_rvdev(struct rproc *rproc, struct rproc_vdev *rvdev);
>  >   void rproc_remove_rvdev(struct rproc_vdev *rvdev);
>  >
>  > +static inline int rproc_mem_entry_ioremap_wc(struct rproc *rproc,
>  > +                                          struct rproc_mem_entry *mem)
>  > +{
>  > +     void __iomem *va;
>  > +
>  > +     va = ioremap_wc(mem->dma, mem->len);
>  > +     if (!va)
>  > +             return -ENOMEM;
> 
> Could you add error message here to help for debug
> 
> +               dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> +                       &mem->dma, mem->len);
>  > +
>  > +     mem->va = (__force void *)va;
>  > +     mem->is_iomem = true;

Hi Geert,

Thanks for the review and the Reviewed-by for 4/4.

Here there is a real behavioral impact from not setting mem->is_iomem for carveouts backed by ioremap_wc(). In that case rproc_da_to_va() reports the region as normal memory, so the ELF load and
coredump paths can end up using memcpy/memset/memcpy_fromio incorrectly instead of the io accessors.

Given Arnaud's feedback, I'll split that out from the helper cleanup and explain it explicitly in a separate patch in v2.

Thanks,
Ben

> 
> HHere, you set mem->is_iomem, but this is not done in platform drivers.
> 
> It seems better to add this in a separate commit after patch 2/4, with
> an explanation of why it needs to be set.
> 
> Regards,
> Arnaud

Hi Arnaud,

Thanks for the review.

Agreed on both points. I'll add the missing map-failure error message in v2.

For mem->is_iomem, I agree it should not be folded into this cleanup patch without its own justification. I'll keep the helper conversion behavior-neutral here and split
it into a separate patch with an explanation of the impact on the remoteproc load/coredump paths.

Thanks,
Ben

> 
>  > +
>  > +     return 0;
>  > +}
>  > +
>  > +static inline int rproc_mem_entry_iounmap(struct rproc *rproc,
>  > +                                       struct rproc_mem_entry *mem)
>  > +{
>  > +     iounmap((__force __iomem void *)mem->va);
>  > +
>  > +     return 0;
>  > +}
>  > +
>  >   static inline int rproc_prepare_device(struct rproc *rproc)
>  >   {
>  >        if (rproc->ops->prepare)
>