* [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 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 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 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).