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

From: Ben Levinsky <[email-masked]>
Date: Tue, 12 May 2026 10:15:01 -0700

Hi Arnaud, Geert,

See my replies below

On 5/12/26 10:03 AM, Levinsky, Ben wrote:
> AMD General
> 
> 
> 
> 
> *From: *Arnaud POULIQUEN <[email-masked]>
> *Date: *Tuesday, May 12, 2026 at 2:45 AM
> *To: *Levinsky, Ben <[email-masked]>; Bjorn Andersson 
> <[email-masked]>; Mathieu Poirier <[email-masked]>; linux- 
> [email-masked] <[email-masked]>
> *Cc: *Frank Li <[email-masked]>; Sascha Hauer <[email-masked]>; 
> Pengutronix Kernel Team <[email-masked]>; Fabio Estevam 
> <[email-masked]>; Geert Uytterhoeven <[email-masked]>; Magnus Damm 
> <[email-masked]>; Patrice Chotard <[email-masked]>; Maxime 
> Coquelin <[email-masked]>; Alexandre Torgue 
> <[email-masked]>; [email-masked] <[email-masked]>; 
> [email-masked] <[email-masked]>; 
> [email-masked] <[email-masked]>; linux-renesas- 
> [email-masked] <[email-masked]>; linux-stm32@st-md- 
> mailman.stormreply.com <[email-masked]>; Shah, Tanmay 
> <[email-masked]>
> *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 <[email-masked]>
>  > ---
>  >   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)
>