* [PATCH 1/2] remoteproc: bail out if firmware has different endianess @ 2012-01-31 16:52 Ohad Ben-Cohen 2012-01-31 16:52 ` [PATCH 2/2] remoteproc: s/big switch/lookup table/ Ohad Ben-Cohen 2012-01-31 20:09 ` [PATCH 1/2] remoteproc: bail out if firmware has different endianess Grant Likely 0 siblings, 2 replies; 7+ messages in thread From: Ohad Ben-Cohen @ 2012-01-31 16:52 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: Grant Likely, Mark Grosen, Ohad Ben-Cohen At this point we don't support remote processors that have a different endianess than the host. Look out for these unsupported scenarios, and bail out if encountered. Reported-by: Grant Likely <grant.likely@secretlab.ca> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> Cc: Mark Grosen <mgrosen@ti.com> --- drivers/remoteproc/remoteproc_core.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 6212b82..567a3c5 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -39,6 +39,7 @@ #include <linux/elf.h> #include <linux/virtio_ids.h> #include <linux/virtio_ring.h> +#include <asm/byteorder.h> #include "remoteproc_internal.h" @@ -851,6 +852,16 @@ static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw) ehdr = (struct elf32_hdr *)fw->data; + /* We assume the firmware has the same endianess as the host */ +# ifdef __LITTLE_ENDIAN + if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB) { +# else /* BIG ENDIAN */ + if (ehdr->e_ident[EI_DATA] != ELFDATA2MSB) { +# endif + dev_err(dev, "Unsupported firmware endianess\n"); + return -EINVAL; + } + if (fw->size < ehdr->e_shoff + sizeof(struct elf32_shdr)) { dev_err(dev, "Image is too small\n"); return -EINVAL; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] remoteproc: s/big switch/lookup table/ 2012-01-31 16:52 [PATCH 1/2] remoteproc: bail out if firmware has different endianess Ohad Ben-Cohen @ 2012-01-31 16:52 ` Ohad Ben-Cohen 2012-01-31 17:58 ` Jack Stone 2012-01-31 20:09 ` [PATCH 1/2] remoteproc: bail out if firmware has different endianess Grant Likely 1 sibling, 1 reply; 7+ messages in thread From: Ohad Ben-Cohen @ 2012-01-31 16:52 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: Grant Likely, Mark Grosen, Ohad Ben-Cohen A lookup table would be easier to extend, and the resulting code is a bit cleaner. Reported-by: Grant Likely <grant.likely@secretlab.ca> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> --- drivers/remoteproc/remoteproc_core.c | 45 +++++++++++++++++---------------- include/linux/remoteproc.h | 7 +++++ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 567a3c5..94473bd 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -64,6 +64,8 @@ static DEFINE_KLIST(rprocs, klist_rproc_get, klist_rproc_put); typedef int (*rproc_handle_resources_t)(struct rproc *rproc, struct fw_resource *rsc, int len); +typedef int (*rproc_handle_resource_t)(struct rproc *rproc, + struct fw_resource *rsc); /* * This is the IOMMU fault handler we register with the IOMMU API @@ -658,44 +660,43 @@ free_mapping: return ret; } +/* + * A lookup table for resource handlers. Always keep in sync with + * enum fw_resource_type. + */ +static rproc_handle_resource_t rproc_handle_rsc[] = { + rproc_handle_carveout, /* RSC_CARVEOUT */ + rproc_handle_devmem, /* RSC_DEVMEM */ + rproc_handle_trace, /* RSC_TRACE */ + rproc_handle_vring, /* RSC_VRING */ + NULL, /* RSC_VIRTIO_DEV is handled early upon registration */ +}; + /* handle firmware resource entries before booting the remote processor */ static int rproc_handle_boot_rsc(struct rproc *rproc, struct fw_resource *rsc, int len) { struct device *dev = rproc->dev; + rproc_handle_resource_t handler; int ret = 0; - while (len >= sizeof(*rsc)) { + for (; len >= sizeof(*rsc); rsc++, len -= sizeof(*rsc)) { dev_dbg(dev, "rsc: type %d, da 0x%llx, pa 0x%llx, len 0x%x, " "id %d, name %s, flags %x\n", rsc->type, rsc->da, rsc->pa, rsc->len, rsc->id, rsc->name, rsc->flags); - switch (rsc->type) { - case RSC_CARVEOUT: - ret = rproc_handle_carveout(rproc, rsc); - break; - case RSC_DEVMEM: - ret = rproc_handle_devmem(rproc, rsc); - break; - case RSC_TRACE: - ret = rproc_handle_trace(rproc, rsc); - break; - case RSC_VRING: - ret = rproc_handle_vring(rproc, rsc); - break; - case RSC_VIRTIO_DEV: - /* this one is handled early upon registration */ - break; - default: + if (rsc->type >= RSC_LAST) { dev_warn(dev, "unsupported resource %d\n", rsc->type); - break; + continue; } + handler = rproc_handle_rsc[rsc->type]; + if (!handler) + continue; + + ret = handler(rproc, rsc); if (ret) break; - - rsc++; - len -= sizeof(*rsc); } return ret; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index b52f784..2544d2e 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -103,6 +103,7 @@ struct fw_resource { * the virtio device features, 'pa' holds the virtio guest * features, 'len' holds the virtio status, and 'flags' holds * the virtio id (currently only VIRTIO_ID_RPMSG is supported). + * @RSC_LAST: just keep this one at the end * * Most of the resource entries share the basic idea of address/length * negotiation with the host: the firmware usually asks (on behalf of the @@ -115,6 +116,11 @@ struct fw_resource { * will contain the expected device addresses (today we actually only support * this scheme, as there aren't yet any use cases for dynamically allocated * device addresses). + * + * Please note that these values are used as indices to the + * rproc_handle_rsc lookup table, so please keep the two synchronized. + * @RSC_LAST is used to check the validity of an index before the lookup table + * is accessed, so please update it as needed too. */ enum fw_resource_type { RSC_CARVEOUT = 0, @@ -122,6 +128,7 @@ enum fw_resource_type { RSC_TRACE = 2, RSC_VRING = 3, RSC_VIRTIO_DEV = 4, + RSC_LAST = 5, }; /** -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] remoteproc: s/big switch/lookup table/ 2012-01-31 16:52 ` [PATCH 2/2] remoteproc: s/big switch/lookup table/ Ohad Ben-Cohen @ 2012-01-31 17:58 ` Jack Stone 2012-01-31 20:08 ` Grant Likely 0 siblings, 1 reply; 7+ messages in thread From: Jack Stone @ 2012-01-31 17:58 UTC (permalink / raw) To: Ohad Ben-Cohen; +Cc: linux-kernel, linux-arm-kernel, Grant Likely, Mark Grosen On 31/01/12 16:52, Ohad Ben-Cohen wrote: > +/* > + * A lookup table for resource handlers. Always keep in sync with > + * enum fw_resource_type. > + */ > +static rproc_handle_resource_t rproc_handle_rsc[] = { > + rproc_handle_carveout, /* RSC_CARVEOUT */ > + rproc_handle_devmem, /* RSC_DEVMEM */ > + rproc_handle_trace, /* RSC_TRACE */ > + rproc_handle_vring, /* RSC_VRING */ > + NULL, /* RSC_VIRTIO_DEV is handled early upon registration */ > +}; > + You could change this to [RSC_CARVEOUT] = rproc_handle_carveout, Then you would be safe against renumbering and would only need to worry about addition (which your RSC_LAST check copes with). Thanks, Jack ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] remoteproc: s/big switch/lookup table/ 2012-01-31 17:58 ` Jack Stone @ 2012-01-31 20:08 ` Grant Likely 2012-01-31 20:21 ` Ohad Ben-Cohen 0 siblings, 1 reply; 7+ messages in thread From: Grant Likely @ 2012-01-31 20:08 UTC (permalink / raw) To: Jack Stone; +Cc: Ohad Ben-Cohen, linux-kernel, linux-arm-kernel, Mark Grosen On Tue, Jan 31, 2012 at 05:58:10PM +0000, Jack Stone wrote: > On 31/01/12 16:52, Ohad Ben-Cohen wrote: > > +/* > > + * A lookup table for resource handlers. Always keep in sync with > > + * enum fw_resource_type. > > + */ > > +static rproc_handle_resource_t rproc_handle_rsc[] = { > > + rproc_handle_carveout, /* RSC_CARVEOUT */ > > + rproc_handle_devmem, /* RSC_DEVMEM */ > > + rproc_handle_trace, /* RSC_TRACE */ > > + rproc_handle_vring, /* RSC_VRING */ > > + NULL, /* RSC_VIRTIO_DEV is handled early upon registration */ > > +}; > > + > > You could change this to > > [RSC_CARVEOUT] = rproc_handle_carveout, > > Then you would be safe against renumbering and would only need to worry > about addition (which your RSC_LAST check copes with). Yes, that is the right thing to do. Then the compiler can catch ordering issues for you. g. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] remoteproc: s/big switch/lookup table/ 2012-01-31 20:08 ` Grant Likely @ 2012-01-31 20:21 ` Ohad Ben-Cohen 0 siblings, 0 replies; 7+ messages in thread From: Ohad Ben-Cohen @ 2012-01-31 20:21 UTC (permalink / raw) To: Grant Likely; +Cc: Jack Stone, linux-kernel, linux-arm-kernel, Mark Grosen On Tue, Jan 31, 2012 at 10:08 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Tue, Jan 31, 2012 at 05:58:10PM +0000, Jack Stone wrote: >> On 31/01/12 16:52, Ohad Ben-Cohen wrote: >> [RSC_CARVEOUT] = rproc_handle_carveout, That looks great. Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] remoteproc: bail out if firmware has different endianess 2012-01-31 16:52 [PATCH 1/2] remoteproc: bail out if firmware has different endianess Ohad Ben-Cohen 2012-01-31 16:52 ` [PATCH 2/2] remoteproc: s/big switch/lookup table/ Ohad Ben-Cohen @ 2012-01-31 20:09 ` Grant Likely 2012-02-08 21:01 ` Ohad Ben-Cohen 1 sibling, 1 reply; 7+ messages in thread From: Grant Likely @ 2012-01-31 20:09 UTC (permalink / raw) To: Ohad Ben-Cohen; +Cc: linux-kernel, linux-arm-kernel, Mark Grosen On Tue, Jan 31, 2012 at 06:52:41PM +0200, Ohad Ben-Cohen wrote: > At this point we don't support remote processors that have > a different endianess than the host. > > Look out for these unsupported scenarios, and bail out if > encountered. > > Reported-by: Grant Likely <grant.likely@secretlab.ca> > Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> > Cc: Mark Grosen <mgrosen@ti.com> Acked-by: Grant Likely <grant.likely@secretlab.ca> > --- > drivers/remoteproc/remoteproc_core.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 6212b82..567a3c5 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -39,6 +39,7 @@ > #include <linux/elf.h> > #include <linux/virtio_ids.h> > #include <linux/virtio_ring.h> > +#include <asm/byteorder.h> > > #include "remoteproc_internal.h" > > @@ -851,6 +852,16 @@ static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw) > > ehdr = (struct elf32_hdr *)fw->data; > > + /* We assume the firmware has the same endianess as the host */ > +# ifdef __LITTLE_ENDIAN > + if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB) { > +# else /* BIG ENDIAN */ > + if (ehdr->e_ident[EI_DATA] != ELFDATA2MSB) { > +# endif > + dev_err(dev, "Unsupported firmware endianess\n"); > + return -EINVAL; > + } > + > if (fw->size < ehdr->e_shoff + sizeof(struct elf32_shdr)) { > dev_err(dev, "Image is too small\n"); > return -EINVAL; > -- > 1.7.5.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] remoteproc: bail out if firmware has different endianess 2012-01-31 20:09 ` [PATCH 1/2] remoteproc: bail out if firmware has different endianess Grant Likely @ 2012-02-08 21:01 ` Ohad Ben-Cohen 0 siblings, 0 replies; 7+ messages in thread From: Ohad Ben-Cohen @ 2012-02-08 21:01 UTC (permalink / raw) To: Grant Likely; +Cc: linux-kernel, linux-arm-kernel, Mark Grosen On Tue, Jan 31, 2012 at 10:09 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Tue, Jan 31, 2012 at 06:52:41PM +0200, Ohad Ben-Cohen wrote: >> At this point we don't support remote processors that have >> a different endianess than the host. >> >> Look out for these unsupported scenarios, and bail out if >> encountered. >> >> Reported-by: Grant Likely <grant.likely@secretlab.ca> >> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> >> Cc: Mark Grosen <mgrosen@ti.com> > > Acked-by: Grant Likely <grant.likely@secretlab.ca> Thanks, applied both patches to remoteproc's for-next. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-02-08 21:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-31 16:52 [PATCH 1/2] remoteproc: bail out if firmware has different endianess Ohad Ben-Cohen 2012-01-31 16:52 ` [PATCH 2/2] remoteproc: s/big switch/lookup table/ Ohad Ben-Cohen 2012-01-31 17:58 ` Jack Stone 2012-01-31 20:08 ` Grant Likely 2012-01-31 20:21 ` Ohad Ben-Cohen 2012-01-31 20:09 ` [PATCH 1/2] remoteproc: bail out if firmware has different endianess Grant Likely 2012-02-08 21:01 ` Ohad Ben-Cohen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).