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