Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751957AbdLNMAS convert rfc822-to-8bit (ORCPT ); Thu, 14 Dec 2017 07:00:18 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:35951 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751729AbdLNMAR (ORCPT ); Thu, 14 Dec 2017 07:00:17 -0500 From: Loic PALLARDY To: Bjorn Andersson , Ohad Ben-Cohen CC: "linux-remoteproc@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH 6/8] remoteproc: Move resource table load logic to find Thread-Topic: [PATCH 6/8] remoteproc: Move resource table load logic to find Thread-Index: AQHTdGObfFo+ft+zlUuMb6aI6llU+aNCvKug Date: Thu, 14 Dec 2017 12:00:13 +0000 Message-ID: <04d54471292f446898fd162afd15388f@SFHDAG7NODE2.st.com> References: <20171213224111.17864-1-bjorn.andersson@linaro.org> <20171213224111.17864-7-bjorn.andersson@linaro.org> In-Reply-To: <20171213224111.17864-7-bjorn.andersson@linaro.org> Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.44] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-14_06:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8871 Lines: 245 > -----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 ; Bjorn Andersson > > 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 > --- > 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