| Message ID | 20200415204858.2448-7-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: Refactor function rproc_alloc() | expand |
On 4/15/20 3:48 PM, Mathieu Poirier wrote: > Make the rproc_ops allocation a function on its own in an effort > to clean up function rproc_alloc(). > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++----------- > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 0bfa6998705d..a5a0ceb86b3f 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc, > return 0; > } > > +static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) > +{ > + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > + if (!rproc->ops) > + return -ENOMEM; > + > + /* Default to ELF loader if no load function is specified */ > + if (!rproc->ops->load) { > + rproc->ops->load = rproc_elf_load_segments; > + rproc->ops->parse_fw = rproc_elf_load_rsc_table; > + rproc->ops->find_loaded_rsc_table = > + rproc_elf_find_loaded_rsc_table; > + rproc->ops->sanity_check = rproc_elf_sanity_check; So, the conditional check on sanity check is dropped and the callback switched here without the changelog reflecting anything why. You should just rebase this patch on top of Clement's patch [1] that removes the conditional flag, and also usage from the remoteproc platform drivers. regards Suman [1] https://patchwork.kernel.org/patch/11462013/ > + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > + } > + > + return 0; > +} > + > /** > * rproc_alloc() - allocate a remote processor handle > * @dev: the underlying device > @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > if (rproc_alloc_firmware(rproc, name, firmware)) > goto free_rproc; > > - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > - if (!rproc->ops) > + if (rproc_alloc_ops(rproc, ops)) > goto free_firmware; > > rproc->name = name; > @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > atomic_set(&rproc->power, 0); > > - /* Default to ELF loader if no load function is specified */ > - if (!rproc->ops->load) { > - rproc->ops->load = rproc_elf_load_segments; > - rproc->ops->parse_fw = rproc_elf_load_rsc_table; > - rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table; > - if (!rproc->ops->sanity_check) > - rproc->ops->sanity_check = rproc_elf32_sanity_check; > - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > - } > - > mutex_init(&rproc->lock); > > INIT_LIST_HEAD(&rproc->carveouts); >
On 4/17/20 8:49 AM, Suman Anna wrote: > On 4/15/20 3:48 PM, Mathieu Poirier wrote: >> Make the rproc_ops allocation a function on its own in an effort >> to clean up function rproc_alloc(). >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> Reviewed-by: Alex Elder <elder@linaro.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index 0bfa6998705d..a5a0ceb86b3f 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc >> *rproc, >> return 0; >> } >> +static int rproc_alloc_ops(struct rproc *rproc, const struct >> rproc_ops *ops) >> +{ >> + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); >> + if (!rproc->ops) >> + return -ENOMEM; >> + >> + /* Default to ELF loader if no load function is specified */ >> + if (!rproc->ops->load) { >> + rproc->ops->load = rproc_elf_load_segments; >> + rproc->ops->parse_fw = rproc_elf_load_rsc_table; >> + rproc->ops->find_loaded_rsc_table = >> + rproc_elf_find_loaded_rsc_table; >> + rproc->ops->sanity_check = rproc_elf_sanity_check; > > So, the conditional check on sanity check is dropped and the callback > switched here without the changelog reflecting anything why. You should > just rebase this patch on top of Clement's patch [1] that removes the > conditional flag, and also usage from the remoteproc platform drivers. > > regards > Suman > > [1] https://patchwork.kernel.org/patch/11462013/ Sorry, gave the wrong link, that was v1. This is the latest, https://patchwork.kernel.org/patch/11466955/ > > >> + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; >> + } >> + >> + return 0; >> +} >> + >> /** >> * rproc_alloc() - allocate a remote processor handle >> * @dev: the underlying device >> @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, >> const char *name, >> if (rproc_alloc_firmware(rproc, name, firmware)) >> goto free_rproc; >> - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); >> - if (!rproc->ops) >> + if (rproc_alloc_ops(rproc, ops)) >> goto free_firmware; >> rproc->name = name; >> @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, >> const char *name, >> atomic_set(&rproc->power, 0); >> - /* Default to ELF loader if no load function is specified */ >> - if (!rproc->ops->load) { >> - rproc->ops->load = rproc_elf_load_segments; >> - rproc->ops->parse_fw = rproc_elf_load_rsc_table; >> - rproc->ops->find_loaded_rsc_table = >> rproc_elf_find_loaded_rsc_table; >> - if (!rproc->ops->sanity_check) >> - rproc->ops->sanity_check = rproc_elf32_sanity_check; >> - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; >> - } >> - >> mutex_init(&rproc->lock); >> INIT_LIST_HEAD(&rproc->carveouts); >> >
On Fri, Apr 17, 2020 at 08:49:25AM -0500, Suman Anna wrote: > On 4/15/20 3:48 PM, Mathieu Poirier wrote: > > Make the rproc_ops allocation a function on its own in an effort > > to clean up function rproc_alloc(). > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Reviewed-by: Alex Elder <elder@linaro.org> > > --- > > drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++----------- > > 1 file changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 0bfa6998705d..a5a0ceb86b3f 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc, > > return 0; > > } > > +static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) > > +{ > > + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > > + if (!rproc->ops) > > + return -ENOMEM; > > + > > + /* Default to ELF loader if no load function is specified */ > > + if (!rproc->ops->load) { > > + rproc->ops->load = rproc_elf_load_segments; > > + rproc->ops->parse_fw = rproc_elf_load_rsc_table; > > + rproc->ops->find_loaded_rsc_table = > > + rproc_elf_find_loaded_rsc_table; > > + rproc->ops->sanity_check = rproc_elf_sanity_check; > > So, the conditional check on sanity check is dropped and the callback > switched here without the changelog reflecting anything why. You should just > rebase this patch on top of Clement's patch [1] that removes the conditional > flag, and also usage from the remoteproc platform drivers. That's a rebase that went very wrong... Thanks for pointing it out, Mathieu > > regards > Suman > > [1] https://patchwork.kernel.org/patch/11462013/ > > > > + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > > + } > > + > > + return 0; > > +} > > + > > /** > > * rproc_alloc() - allocate a remote processor handle > > * @dev: the underlying device > > @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > if (rproc_alloc_firmware(rproc, name, firmware)) > > goto free_rproc; > > - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > > - if (!rproc->ops) > > + if (rproc_alloc_ops(rproc, ops)) > > goto free_firmware; > > rproc->name = name; > > @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > atomic_set(&rproc->power, 0); > > - /* Default to ELF loader if no load function is specified */ > > - if (!rproc->ops->load) { > > - rproc->ops->load = rproc_elf_load_segments; > > - rproc->ops->parse_fw = rproc_elf_load_rsc_table; > > - rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table; > > - if (!rproc->ops->sanity_check) > > - rproc->ops->sanity_check = rproc_elf32_sanity_check; > > - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > > - } > > - > > mutex_init(&rproc->lock); > > INIT_LIST_HEAD(&rproc->carveouts); > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 0bfa6998705d..a5a0ceb86b3f 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc, return 0; } +static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) +{ + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); + if (!rproc->ops) + return -ENOMEM; + + /* Default to ELF loader if no load function is specified */ + if (!rproc->ops->load) { + rproc->ops->load = rproc_elf_load_segments; + rproc->ops->parse_fw = rproc_elf_load_rsc_table; + rproc->ops->find_loaded_rsc_table = + rproc_elf_find_loaded_rsc_table; + rproc->ops->sanity_check = rproc_elf_sanity_check; + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; + } + + return 0; +} + /** * rproc_alloc() - allocate a remote processor handle * @dev: the underlying device @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, if (rproc_alloc_firmware(rproc, name, firmware)) goto free_rproc; - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); - if (!rproc->ops) + if (rproc_alloc_ops(rproc, ops)) goto free_firmware; rproc->name = name; @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, atomic_set(&rproc->power, 0); - /* Default to ELF loader if no load function is specified */ - if (!rproc->ops->load) { - rproc->ops->load = rproc_elf_load_segments; - rproc->ops->parse_fw = rproc_elf_load_rsc_table; - rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table; - if (!rproc->ops->sanity_check) - rproc->ops->sanity_check = rproc_elf32_sanity_check; - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; - } - mutex_init(&rproc->lock); INIT_LIST_HEAD(&rproc->carveouts);