On Thu, Nov 28, 2024 at 09:42:12AM +0100, Arnaud Pouliquen wrote: > The two methods to load the firmware to memory should be exclusive. > > - make load_segments optional returning 0 if not set in > rproc_load_segments(), > - ensure that load_segments() and load_fw() are not both set, > - do not set default rproc::ops fields if load_fw() is set. This changelog is describing "what" the patch does rather than "why". I have commented on that before. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++++ > drivers/remoteproc/remoteproc_internal.h | 2 +- > include/linux/remoteproc.h | 7 +++++-- > 3 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index e4ad024efcda..deadec0f3474 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2477,6 +2477,10 @@ static int rproc_alloc_firmware(struct rproc *rproc, > > static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) > { > + /* A processor with a load_segments() and a load_fw() functions makes no sense. */ > + if (ops->load_segments && ops->load_fw) > + return -EINVAL; > + It is only a matter of time before someone needs both ->load_segments() and ->load_fw(). > rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > if (!rproc->ops) > return -ENOMEM; > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > index b898494600cf..3a4161eaf291 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -170,7 +170,7 @@ int rproc_load_segments(struct rproc *rproc, const struct firmware *fw) > if (rproc->ops->load_segments) > return rproc->ops->load_segments(rproc, fw); > > - return -EINVAL; > + return 0; Other than this hunk I would drop this patch completely. > } > > static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 55c20424a99f..4f4c65ce74af 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -374,8 +374,9 @@ enum rsc_handling_status { > * @find_loaded_rsc_table: find the loaded resource table from firmware image > * @get_loaded_rsc_table: get resource table installed in memory > * by external entity > - * @load_segments: load firmware ELF segment to memory, where the remote processor > - * expects to find it > + * @load_segments: optional load firmware ELF segments to memory, where the remote processor > + * expects to find it. > + * This operation is exclusive with the load_fw() > * @sanity_check: sanity check the fw image > * @get_boot_addr: get boot address to entry point specified in firmware > * @panic: optional callback to react to system panic, core will delay > @@ -383,8 +384,10 @@ enum rsc_handling_status { > * @coredump: collect firmware dump after the subsystem is shutdown > * @load_fw: optional function to load non-ELF firmware image to memory, where the remote > * processor expects to find it. > + * This operation is exclusive with the load_segments() > * @release_fw: optional function to release the firmware image from memories. > * This function is called after stopping the remote processor or in case of error > + * > */ > struct rproc_ops { > int (*prepare)(struct rproc *rproc); > -- > 2.25.1 >