| Message ID | 20171213224111.17864-7-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > Sent: Wednesday, December 13, 2017 11:41 PM > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson > <bjorn.andersson@linaro.org> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 6/8] remoteproc: Move resource table load logic to find > > Extend the previous operation of finding the resource table in the ELF > with the extra step of populating the rproc struct with a copy and the > size. This allows drivers to override the mechanism used for acquiring > the resource table, or omit it for firmware that is known not to have a > resource table. > > This leaves the custom, dummy, find_rsc_table implementations found in > some drivers dangling. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 32 ++++++-------------------- > drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++-- > ---------- > drivers/remoteproc/remoteproc_internal.h | 16 +++++-------- > include/linux/remoteproc.h | 1 + > 4 files changed, 36 insertions(+), 50 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index 63d88d1d206e..cbd12382b219 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > { > struct device *dev = &rproc->dev; > const char *name = rproc->firmware; > - struct resource_table *table; > - int ret, tablesz; > + int ret; > > ret = rproc_fw_sanity_check(rproc, fw); > if (ret) > @@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > } > > rproc->bootaddr = rproc_get_boot_addr(rproc, fw); > - ret = -EINVAL; > - > - /* look for the resource table */ > - table = rproc_find_rsc_table(rproc, fw, &tablesz); > - if (!table) { > - dev_err(dev, "Failed to find resource table\n"); > - goto clean_up; > - } > - > - /* > - * Create a copy of the resource table. When a virtio device starts > - * and calls vring_new_virtqueue() the address of the allocated vring > - * will be stored in the cached_table. Before the device is started, > - * cached_table will be copied into device memory. > - */ > - rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); > - if (!rproc->cached_table) > - goto clean_up; > > - rproc->table_ptr = rproc->cached_table; > - rproc->table_sz = tablesz; > + /* load resource table */ > + ret = rproc_load_rsc_table(rproc, fw); > + if (ret) > + goto disable_iommu; > > /* reset max_notifyid */ > rproc->max_notifyid = -1; > @@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > > clean_up_resources: > rproc_resource_cleanup(rproc); > -clean_up: > kfree(rproc->cached_table); > rproc->cached_table = NULL; > rproc->table_ptr = NULL; > - > +disable_iommu: > rproc_disable_iommu(rproc); > return ret; > } > @@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const > char *name, > /* Default to ELF loader if no load function is specified */ > if (!rproc->ops->load) { > rproc->ops->load = rproc_elf_load_segments; > - rproc->ops->find_rsc_table = rproc_elf_find_rsc_table; > + rproc->ops->load_rsc_table = 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; > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c > b/drivers/remoteproc/remoteproc_elf_loader.c > index 822fa1bf893f..b17d72ec8603 100644 > --- a/drivers/remoteproc/remoteproc_elf_loader.c > +++ b/drivers/remoteproc/remoteproc_elf_loader.c > @@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr > *ehdr, size_t fw_size) > } > > /** > - * rproc_elf_find_rsc_table() - find the resource table > + * rproc_elf_load_rsc_table() - load the resource table > * @rproc: the rproc handle > * @fw: the ELF firmware image > - * @tablesz: place holder for providing back the table size > * > * This function finds the resource table inside the remote processor's > - * firmware. It is used both upon the registration of @rproc (in order > - * to look for and register the supported virito devices), and when the > - * @rproc is booted. > + * firmware, load it into the @cached_table and update @table_ptr. > * > - * Returns the pointer to the resource table if it is found, and write its > - * size into @tablesz. If a valid table isn't found, NULL is returned > - * (and @tablesz isn't set). > + * Return: 0 on success, negative errno on failure. > */ > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz) > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw) > { > struct elf32_hdr *ehdr; > struct elf32_shdr *shdr; > struct device *dev = &rproc->dev; > struct resource_table *table = NULL; > const u8 *elf_data = fw->data; > + size_t tablesz; > > ehdr = (struct elf32_hdr *)elf_data; > > shdr = find_table(dev, ehdr, fw->size); > if (!shdr) > - return NULL; > + return -EINVAL; > > table = (struct resource_table *)(elf_data + shdr->sh_offset); > - *tablesz = shdr->sh_size; > + tablesz = shdr->sh_size; > + > + /* > + * Create a copy of the resource table. When a virtio device starts > + * and calls vring_new_virtqueue() the address of the allocated vring > + * will be stored in the cached_table. Before the device is started, > + * cached_table will be copied into device memory. > + */ > + rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); > + if (!rproc->cached_table) > + return -ENOMEM; > > - return table; > + rproc->table_ptr = rproc->cached_table; > + rproc->table_sz = tablesz; > + > + return 0; > } > -EXPORT_SYMBOL(rproc_elf_find_rsc_table); > +EXPORT_SYMBOL(rproc_elf_load_rsc_table); [LPA] Hi Bjorn, I understand the simplification of the code, but I would like to keep rproc_elf_find_rsc_table function as helper one. Indeed if a driver wants to override the default rproc_elf_load_rsc_table, it may have to parse first the elf file to look up for resource table using rproc_elf_find_rsc_table as helper. Regards, Loic > > /** > * rproc_elf_find_loaded_rsc_table() - find the loaded resource table > diff --git a/drivers/remoteproc/remoteproc_internal.h > b/drivers/remoteproc/remoteproc_internal.h > index a42690c514e2..55a2950c5cb7 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc); > int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw); > u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware > *fw); > int rproc_elf_load_segments(struct rproc *rproc, const struct firmware > *fw); > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz); > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware > *fw); > struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc, > const struct firmware > *fw); > > @@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const > struct firmware *fw) > return -EINVAL; > } > > -static inline > -struct resource_table *rproc_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz) > +static inline int rproc_load_rsc_table(struct rproc *rproc, > + const struct firmware *fw) > { > - if (rproc->ops->find_rsc_table) > - return rproc->ops->find_rsc_table(rproc, fw, tablesz); > + if (rproc->ops->load_rsc_table) > + return rproc->ops->load_rsc_table(rproc, fw); > > - return NULL; > + return 0; > } > > static inline > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e21de55e19d1..ec1ada7cc6d7 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -344,6 +344,7 @@ struct rproc_ops { > int (*stop)(struct rproc *rproc); > void (*kick)(struct rproc *rproc, int vqid); > void * (*da_to_va)(struct rproc *rproc, u64 da, int len); > + int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); > struct resource_table *(*find_rsc_table)(struct rproc *rproc, > const struct firmware *fw, > int *tablesz); > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > Sent: Wednesday, December 13, 2017 11:41 PM > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson > <bjorn.andersson@linaro.org> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 6/8] remoteproc: Move resource table load logic to find > > Extend the previous operation of finding the resource table in the ELF > with the extra step of populating the rproc struct with a copy and the > size. This allows drivers to override the mechanism used for acquiring > the resource table, or omit it for firmware that is known not to have a > resource table. > > This leaves the custom, dummy, find_rsc_table implementations found in > some drivers dangling. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 32 ++++++-------------------- > drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++-- > ---------- > drivers/remoteproc/remoteproc_internal.h | 16 +++++-------- > include/linux/remoteproc.h | 1 + > 4 files changed, 36 insertions(+), 50 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index 63d88d1d206e..cbd12382b219 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > { > struct device *dev = &rproc->dev; > const char *name = rproc->firmware; > - struct resource_table *table; > - int ret, tablesz; > + int ret; > > ret = rproc_fw_sanity_check(rproc, fw); > if (ret) > @@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > } > > rproc->bootaddr = rproc_get_boot_addr(rproc, fw); > - ret = -EINVAL; > - > - /* look for the resource table */ > - table = rproc_find_rsc_table(rproc, fw, &tablesz); > - if (!table) { > - dev_err(dev, "Failed to find resource table\n"); > - goto clean_up; > - } > - > - /* > - * Create a copy of the resource table. When a virtio device starts > - * and calls vring_new_virtqueue() the address of the allocated vring > - * will be stored in the cached_table. Before the device is started, > - * cached_table will be copied into device memory. > - */ > - rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); > - if (!rproc->cached_table) > - goto clean_up; > > - rproc->table_ptr = rproc->cached_table; > - rproc->table_sz = tablesz; > + /* load resource table */ > + ret = rproc_load_rsc_table(rproc, fw); > + if (ret) > + goto disable_iommu; > > /* reset max_notifyid */ > rproc->max_notifyid = -1; > @@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > > clean_up_resources: > rproc_resource_cleanup(rproc); > -clean_up: > kfree(rproc->cached_table); > rproc->cached_table = NULL; > rproc->table_ptr = NULL; > - > +disable_iommu: > rproc_disable_iommu(rproc); > return ret; > } > @@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const > char *name, > /* Default to ELF loader if no load function is specified */ > if (!rproc->ops->load) { > rproc->ops->load = rproc_elf_load_segments; > - rproc->ops->find_rsc_table = rproc_elf_find_rsc_table; > + rproc->ops->load_rsc_table = 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; > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c > b/drivers/remoteproc/remoteproc_elf_loader.c > index 822fa1bf893f..b17d72ec8603 100644 > --- a/drivers/remoteproc/remoteproc_elf_loader.c > +++ b/drivers/remoteproc/remoteproc_elf_loader.c > @@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr > *ehdr, size_t fw_size) > } > > /** > - * rproc_elf_find_rsc_table() - find the resource table > + * rproc_elf_load_rsc_table() - load the resource table > * @rproc: the rproc handle > * @fw: the ELF firmware image > - * @tablesz: place holder for providing back the table size > * > * This function finds the resource table inside the remote processor's > - * firmware. It is used both upon the registration of @rproc (in order > - * to look for and register the supported virito devices), and when the > - * @rproc is booted. > + * firmware, load it into the @cached_table and update @table_ptr. > * > - * Returns the pointer to the resource table if it is found, and write its > - * size into @tablesz. If a valid table isn't found, NULL is returned > - * (and @tablesz isn't set). > + * Return: 0 on success, negative errno on failure. > */ > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz) > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw) > { > struct elf32_hdr *ehdr; > struct elf32_shdr *shdr; > struct device *dev = &rproc->dev; > struct resource_table *table = NULL; > const u8 *elf_data = fw->data; > + size_t tablesz; > > ehdr = (struct elf32_hdr *)elf_data; > > shdr = find_table(dev, ehdr, fw->size); > if (!shdr) > - return NULL; > + return -EINVAL; Reading patch 7, I don't catch how firmware without resource table are supported as if no table found, an error is returned here. In rproc_fw_boot, you have: ret = rproc_load_rsc_table(rproc, fw); if (ret) goto disable_iommu; So boot process is interrupted... Regards, Loic > > table = (struct resource_table *)(elf_data + shdr->sh_offset); > - *tablesz = shdr->sh_size; > + tablesz = shdr->sh_size; > + > + /* > + * Create a copy of the resource table. When a virtio device starts > + * and calls vring_new_virtqueue() the address of the allocated vring > + * will be stored in the cached_table. Before the device is started, > + * cached_table will be copied into device memory. > + */ > + rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); > + if (!rproc->cached_table) > + return -ENOMEM; > > - return table; > + rproc->table_ptr = rproc->cached_table; > + rproc->table_sz = tablesz; > + > + return 0; > } > -EXPORT_SYMBOL(rproc_elf_find_rsc_table); > +EXPORT_SYMBOL(rproc_elf_load_rsc_table); > > /** > * rproc_elf_find_loaded_rsc_table() - find the loaded resource table > diff --git a/drivers/remoteproc/remoteproc_internal.h > b/drivers/remoteproc/remoteproc_internal.h > index a42690c514e2..55a2950c5cb7 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc); > int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw); > u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware > *fw); > int rproc_elf_load_segments(struct rproc *rproc, const struct firmware > *fw); > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz); > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware > *fw); > struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc, > const struct firmware > *fw); > > @@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const > struct firmware *fw) > return -EINVAL; > } > > -static inline > -struct resource_table *rproc_find_rsc_table(struct rproc *rproc, > - const struct firmware *fw, > - int *tablesz) > +static inline int rproc_load_rsc_table(struct rproc *rproc, > + const struct firmware *fw) > { > - if (rproc->ops->find_rsc_table) > - return rproc->ops->find_rsc_table(rproc, fw, tablesz); > + if (rproc->ops->load_rsc_table) > + return rproc->ops->load_rsc_table(rproc, fw); > > - return NULL; > + return 0; > } > > static inline > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e21de55e19d1..ec1ada7cc6d7 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -344,6 +344,7 @@ struct rproc_ops { > int (*stop)(struct rproc *rproc); > void (*kick)(struct rproc *rproc, int vqid); > void * (*da_to_va)(struct rproc *rproc, u64 da, int len); > + int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); > struct resource_table *(*find_rsc_table)(struct rproc *rproc, > const struct firmware *fw, > int *tablesz); > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 14 Dec 03:25 PST 2017, Loic PALLARDY wrote: > > -----Original Message----- > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > > Sent: Wednesday, December 13, 2017 11:41 PM [..] > > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, > > - const struct firmware *fw, > > - int *tablesz) > > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw) > > { > > struct elf32_hdr *ehdr; > > struct elf32_shdr *shdr; > > struct device *dev = &rproc->dev; > > struct resource_table *table = NULL; > > const u8 *elf_data = fw->data; > > + size_t tablesz; > > > > ehdr = (struct elf32_hdr *)elf_data; > > > > shdr = find_table(dev, ehdr, fw->size); > > if (!shdr) > > - return NULL; > > + return -EINVAL; > > > > table = (struct resource_table *)(elf_data + shdr->sh_offset); > > - *tablesz = shdr->sh_size; > > + tablesz = shdr->sh_size; > > + > > + /* > > + * Create a copy of the resource table. When a virtio device starts > > + * and calls vring_new_virtqueue() the address of the allocated vring > > + * will be stored in the cached_table. Before the device is started, > > + * cached_table will be copied into device memory. > > + */ > > + rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); > > + if (!rproc->cached_table) > > + return -ENOMEM; > > > > - return table; > > + rproc->table_ptr = rproc->cached_table; > > + rproc->table_sz = tablesz; > > + > > + return 0; > > } > > -EXPORT_SYMBOL(rproc_elf_find_rsc_table); > > +EXPORT_SYMBOL(rproc_elf_load_rsc_table); > [LPA] Hi Bjorn, > > I understand the simplification of the code, but I would like to keep > rproc_elf_find_rsc_table function as helper one. Indeed if a driver > wants to override the default rproc_elf_load_rsc_table, it may have to > parse first the elf file to look up for resource table using > rproc_elf_find_rsc_table as helper. find_table() finds the .resource_table segment and verifies that this contains a valid, version 1, resource table. So AFAICT there are two scenarios here: 1) A driver wants to support custom entries in the resource table, for which Suman's proposal of adding a way for drivers to implement custom properties seems like a good solution. 2) A driver wants to support something other than a v1 resource table, in which case they would need to reimplement find_table() anyways. It's possible that the part of find_table() that looks up the segment by name could be exposed for drivers to use this, but half of find_table() deals with verifying that the content is a proper resource table. Please let me know if you have a use case beyond these. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 14 Dec 04:00 PST 2017, Loic PALLARDY wrote: > > -----Original Message----- > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson [..] > > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw) > > { > > struct elf32_hdr *ehdr; > > struct elf32_shdr *shdr; > > struct device *dev = &rproc->dev; > > struct resource_table *table = NULL; > > const u8 *elf_data = fw->data; > > + size_t tablesz; > > > > ehdr = (struct elf32_hdr *)elf_data; > > > > shdr = find_table(dev, ehdr, fw->size); > > if (!shdr) > > - return NULL; > > + return -EINVAL; > Reading patch 7, I don't catch how firmware without resource table are > supported as if no table found, an error is returned here. > In rproc_fw_boot, you have: > ret = rproc_load_rsc_table(rproc, fw); > if (ret) > goto disable_iommu; > So boot process is interrupted... > When a driver registers a remoteproc with load_rsc_table specified that implies that the resource table is not optional as such it's an error to not find one. A driver that expects to load firmware without a resource table, such as st_slim_rproc with it's dummy resource table would set load_rsc_table to NULL. So the purpose is to maintain the current requirements, with the addition of removing the dummy implementations of find_rsc_table. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > Sent: Thursday, December 14, 2017 8:48 PM > To: Loic PALLARDY <loic.pallardy@st.com> > Cc: Ohad Ben-Cohen <ohad@wizery.com>; linux- > remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 6/8] remoteproc: Move resource table load logic to find > > On Thu 14 Dec 04:00 PST 2017, Loic PALLARDY wrote: > > > -----Original Message----- > > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux- > remoteproc- > > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > [..] > > > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware > *fw) > > > { > > > struct elf32_hdr *ehdr; > > > struct elf32_shdr *shdr; > > > struct device *dev = &rproc->dev; > > > struct resource_table *table = NULL; > > > const u8 *elf_data = fw->data; > > > + size_t tablesz; > > > > > > ehdr = (struct elf32_hdr *)elf_data; > > > > > > shdr = find_table(dev, ehdr, fw->size); > > > if (!shdr) > > > - return NULL; > > > + return -EINVAL; > > Reading patch 7, I don't catch how firmware without resource table are > > supported as if no table found, an error is returned here. > > In rproc_fw_boot, you have: > > ret = rproc_load_rsc_table(rproc, fw); > > if (ret) > > goto disable_iommu; > > So boot process is interrupted... > > > > When a driver registers a remoteproc with load_rsc_table specified that > implies that the resource table is not optional as such it's an error to > not find one. > > A driver that expects to load firmware without a resource table, such as > st_slim_rproc with it's dummy resource table would set load_rsc_table to > NULL. Yes I saw that if no load_rsc_table, 0 is returned. Thanks for the explanations, I'll apply the series and review final code. Regards, Loic > > > So the purpose is to maintain the current requirements, with the > addition of removing the dummy implementations of find_rsc_table. > > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 63d88d1d206e..cbd12382b219 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) { struct device *dev = &rproc->dev; const char *name = rproc->firmware; - struct resource_table *table; - int ret, tablesz; + int ret; ret = rproc_fw_sanity_check(rproc, fw); if (ret) @@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) } rproc->bootaddr = rproc_get_boot_addr(rproc, fw); - ret = -EINVAL; - - /* look for the resource table */ - table = rproc_find_rsc_table(rproc, fw, &tablesz); - if (!table) { - dev_err(dev, "Failed to find resource table\n"); - goto clean_up; - } - - /* - * Create a copy of the resource table. When a virtio device starts - * and calls vring_new_virtqueue() the address of the allocated vring - * will be stored in the cached_table. Before the device is started, - * cached_table will be copied into device memory. - */ - rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); - if (!rproc->cached_table) - goto clean_up; - rproc->table_ptr = rproc->cached_table; - rproc->table_sz = tablesz; + /* load resource table */ + ret = rproc_load_rsc_table(rproc, fw); + if (ret) + goto disable_iommu; /* reset max_notifyid */ rproc->max_notifyid = -1; @@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) clean_up_resources: rproc_resource_cleanup(rproc); -clean_up: kfree(rproc->cached_table); rproc->cached_table = NULL; rproc->table_ptr = NULL; - +disable_iommu: rproc_disable_iommu(rproc); return ret; } @@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, /* Default to ELF loader if no load function is specified */ if (!rproc->ops->load) { rproc->ops->load = rproc_elf_load_segments; - rproc->ops->find_rsc_table = rproc_elf_find_rsc_table; + rproc->ops->load_rsc_table = 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; diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c index 822fa1bf893f..b17d72ec8603 100644 --- a/drivers/remoteproc/remoteproc_elf_loader.c +++ b/drivers/remoteproc/remoteproc_elf_loader.c @@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size) } /** - * rproc_elf_find_rsc_table() - find the resource table + * rproc_elf_load_rsc_table() - load the resource table * @rproc: the rproc handle * @fw: the ELF firmware image - * @tablesz: place holder for providing back the table size * * This function finds the resource table inside the remote processor's - * firmware. It is used both upon the registration of @rproc (in order - * to look for and register the supported virito devices), and when the - * @rproc is booted. + * firmware, load it into the @cached_table and update @table_ptr. * - * Returns the pointer to the resource table if it is found, and write its - * size into @tablesz. If a valid table isn't found, NULL is returned - * (and @tablesz isn't set). + * Return: 0 on success, negative errno on failure. */ -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, - const struct firmware *fw, - int *tablesz) +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw) { struct elf32_hdr *ehdr; struct elf32_shdr *shdr; struct device *dev = &rproc->dev; struct resource_table *table = NULL; const u8 *elf_data = fw->data; + size_t tablesz; ehdr = (struct elf32_hdr *)elf_data; shdr = find_table(dev, ehdr, fw->size); if (!shdr) - return NULL; + return -EINVAL; table = (struct resource_table *)(elf_data + shdr->sh_offset); - *tablesz = shdr->sh_size; + tablesz = shdr->sh_size; + + /* + * Create a copy of the resource table. When a virtio device starts + * and calls vring_new_virtqueue() the address of the allocated vring + * will be stored in the cached_table. Before the device is started, + * cached_table will be copied into device memory. + */ + rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL); + if (!rproc->cached_table) + return -ENOMEM; - return table; + rproc->table_ptr = rproc->cached_table; + rproc->table_sz = tablesz; + + return 0; } -EXPORT_SYMBOL(rproc_elf_find_rsc_table); +EXPORT_SYMBOL(rproc_elf_load_rsc_table); /** * rproc_elf_find_loaded_rsc_table() - find the loaded resource table diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h index a42690c514e2..55a2950c5cb7 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc); int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw); u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw); int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw); -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc, - const struct firmware *fw, - int *tablesz); +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw); struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw); @@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const struct firmware *fw) return -EINVAL; } -static inline -struct resource_table *rproc_find_rsc_table(struct rproc *rproc, - const struct firmware *fw, - int *tablesz) +static inline int rproc_load_rsc_table(struct rproc *rproc, + const struct firmware *fw) { - if (rproc->ops->find_rsc_table) - return rproc->ops->find_rsc_table(rproc, fw, tablesz); + if (rproc->ops->load_rsc_table) + return rproc->ops->load_rsc_table(rproc, fw); - return NULL; + return 0; } static inline diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index e21de55e19d1..ec1ada7cc6d7 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -344,6 +344,7 @@ struct rproc_ops { int (*stop)(struct rproc *rproc); void (*kick)(struct rproc *rproc, int vqid); void * (*da_to_va)(struct rproc *rproc, u64 da, int len); + int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); struct resource_table *(*find_rsc_table)(struct rproc *rproc, const struct firmware *fw, int *tablesz);
Extend the previous operation of finding the resource table in the ELF with the extra step of populating the rproc struct with a copy and the size. This allows drivers to override the mechanism used for acquiring the resource table, or omit it for firmware that is known not to have a resource table. This leaves the custom, dummy, find_rsc_table implementations found in some drivers dangling. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 32 ++++++-------------------- drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++------------ drivers/remoteproc/remoteproc_internal.h | 16 +++++-------- include/linux/remoteproc.h | 1 + 4 files changed, 36 insertions(+), 50 deletions(-)