[Sound-open-firmware] [RFC PATCH] mailbox: move multi-line macros to inline functions
Detected with Coverity:
In do_notify: The indentation of this code suggests it is nested when it is not. (CWE-483)
multi_stmt_macro: The macro on this line expands into multiple statements, only the first of which is nested within the preceding parent while the rest are not.
if (msg->rx_size && msg->rx_size < SOF_IPC_MSG_MAX_SIZE) mailbox_dspbox_read(msg->rx_data, 0, msg->rx_size);
Move mailbox macros to inline functions to remove the issue, keep indentation the same and add typecasts as needed
FIXME: a. is type uint8_t * ok for data? b. is uint32_t ok for offset (renamed from dest since it was not a pointer) c. This code is full of integer/pointer conversions which will have to be cleaned-up for MISRA compliance
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- src/include/reef/mailbox.h | 44 ++++++++++++++++++++++++++-------------- src/ipc/byt-ipc.c | 2 +- src/ipc/intel-ipc.c | 23 +++++++++++---------- src/platform/baytrail/platform.c | 2 +- 4 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/src/include/reef/mailbox.h b/src/include/reef/mailbox.h index c0580a9..c3976a3 100644 --- a/src/include/reef/mailbox.h +++ b/src/include/reef/mailbox.h @@ -34,6 +34,7 @@ #include <platform/mailbox.h> #include <arch/cache.h> #include <stdint.h> +#include <reef/reef.h>
/* 4k should be enough for everyone ..... */ #define IPC_MAX_MAILBOX_BYTES 0x1000 @@ -62,20 +63,33 @@ #define mailbox_get_debug_size() \ MAILBOX_DEBUG_SIZE
-#define mailbox_dspbox_write(dest, src, bytes) \ - rmemcpy((void*)(MAILBOX_DSPBOX_BASE + dest), src, bytes); \ - dcache_writeback_region((void*)(MAILBOX_DSPBOX_BASE + dest), bytes); - -#define mailbox_dspbox_read(dest, src, bytes) \ - dcache_invalidate_region((void*)(MAILBOX_DSPBOX_BASE + src), bytes); \ - rmemcpy(dest, (void*)(MAILBOX_DSPBOX_BASE + src), bytes); - -#define mailbox_hostbox_write(dest, src, bytes) \ - rmemcpy((void*)(MAILBOX_HOSTBOX_BASE + dest), src, bytes); \ - dcache_writeback_region((void*)(MAILBOX_HOSTBOX_BASE + dest), bytes); - -#define mailbox_hostbox_read(dest, src, bytes) \ - dcache_invalidate_region((void*)(MAILBOX_HOSTBOX_BASE + src), bytes); \ - rmemcpy(dest, (void*)(MAILBOX_HOSTBOX_BASE + src), bytes); +static inline +void mailbox_dspbox_write(uint32_t offset, uint8_t *src, uint32_t bytes) +{ + rmemcpy((void *)(MAILBOX_DSPBOX_BASE + offset), src, bytes); + dcache_writeback_region((void *)(MAILBOX_DSPBOX_BASE + offset), bytes); +} + +static inline +void mailbox_dspbox_read(uint8_t *dest, uint32_t offset, uint32_t bytes) +{ + dcache_invalidate_region((void *)(MAILBOX_DSPBOX_BASE + offset), bytes); + rmemcpy(dest, (void *)(MAILBOX_DSPBOX_BASE + offset), bytes); +} + +static inline +void mailbox_hostbox_write(uint32_t offset, uint8_t *src, uint32_t bytes) +{ + rmemcpy((void *)(MAILBOX_HOSTBOX_BASE + offset), src, bytes); + dcache_writeback_region((void *)(MAILBOX_HOSTBOX_BASE + offset), bytes); +} + +static inline +void mailbox_hostbox_read(uint8_t *dest, uint32_t offset, uint32_t bytes) +{ + dcache_invalidate_region((void *)(MAILBOX_HOSTBOX_BASE + offset), + bytes); + rmemcpy(dest, (void *)(MAILBOX_HOSTBOX_BASE + offset), bytes); +}
#endif diff --git a/src/ipc/byt-ipc.c b/src/ipc/byt-ipc.c index 8897bb9..029d3a6 100644 --- a/src/ipc/byt-ipc.c +++ b/src/ipc/byt-ipc.c @@ -140,7 +140,7 @@ void ipc_platform_do_cmd(struct ipc *ipc) /* send std error/ok reply */ reply.hdr.cmd = SOF_IPC_GLB_REPLY; reply.hdr.size = sizeof(reply); - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply));
done: ipc->host_pending = 0; diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index 5a98e1f..99e0a90 100644 --- a/src/ipc/intel-ipc.c +++ b/src/ipc/intel-ipc.c @@ -69,7 +69,7 @@ static inline struct sof_ipc_hdr *mailbox_validate(void) struct sof_ipc_hdr *hdr = _ipc->comp_data;
/* read component values from the inbox */ - mailbox_hostbox_read(hdr, 0, sizeof(*hdr)); + mailbox_hostbox_read((uint8_t *)hdr, 0, sizeof(*hdr));
/* validate component header */ if (hdr->size > SOF_IPC_MSG_MAX_SIZE) { @@ -78,7 +78,8 @@ static inline struct sof_ipc_hdr *mailbox_validate(void) }
/* read rest of component data */ - mailbox_hostbox_read(hdr + 1, sizeof(*hdr), hdr->size - sizeof(*hdr)); + mailbox_hostbox_read((uint8_t *)(hdr + 1), sizeof(*hdr), + hdr->size - sizeof(*hdr)); return hdr; }
@@ -297,7 +298,7 @@ static int ipc_stream_pcm_params(uint32_t stream) reply.rhdr.error = 0; reply.comp_id = pcm_params->comp_id; reply.posn_offset = 0; /* TODO: set this up for mmaped components */ - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply)); return 1;
error: @@ -360,7 +361,7 @@ static int ipc_stream_position(uint32_t header) pipeline_get_timestamp(pcm_dev->cd->pipeline, pcm_dev->cd, &posn);
/* copy positions to outbox */ - mailbox_hostbox_write(0, &posn, sizeof(posn)); + mailbox_hostbox_write(0, (uint8_t *)&posn, sizeof(posn)); return 1; }
@@ -521,7 +522,7 @@ static int ipc_pm_context_size(uint32_t header) /* TODO: calculate the context and size of host buffers required */
/* write the context to the host driver */ - mailbox_hostbox_write(0, &pm_ctx, sizeof(pm_ctx)); + mailbox_hostbox_write(0, (uint8_t *)&pm_ctx, sizeof(pm_ctx)); return 1; }
@@ -554,7 +555,7 @@ static int ipc_pm_context_save(uint32_t header) //reply.entries_no = 0;
/* write the context to the host driver */ - mailbox_hostbox_write(0, pm_ctx, sizeof(*pm_ctx)); + mailbox_hostbox_write(0, (uint8_t *)pm_ctx, sizeof(*pm_ctx));
//iipc->pm_prepare_D3 = 1;
@@ -632,7 +633,7 @@ static int ipc_dma_trace_config(uint32_t header) reply.hdr.size = sizeof(reply); reply.hdr.cmd = header; reply.error = 0; - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply)); return 0;
error: @@ -702,7 +703,7 @@ static int ipc_comp_value(uint32_t header, uint32_t cmd) }
/* write component values to the outbox */ - mailbox_hostbox_write(0, data, data->rhdr.hdr.size); + mailbox_hostbox_write(0, (uint8_t *)data, data->rhdr.hdr.size); return 1; }
@@ -746,7 +747,7 @@ static int ipc_glb_tplg_comp_new(uint32_t header) reply.rhdr.hdr.cmd = header; reply.rhdr.error = 0; reply.offset = 0; /* TODO: set this up for mmaped components */ - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply)); return 1; }
@@ -769,7 +770,7 @@ static int ipc_glb_tplg_buffer_new(uint32_t header) reply.rhdr.hdr.cmd = header; reply.rhdr.error = 0; reply.offset = 0; /* TODO: set this up for mmaped components */ - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply)); return 1; }
@@ -792,7 +793,7 @@ static int ipc_glb_tplg_pipe_new(uint32_t header) reply.rhdr.hdr.cmd = header; reply.rhdr.error = 0; reply.offset = 0; /* TODO: set this up for mmaped components */ - mailbox_hostbox_write(0, &reply, sizeof(reply)); + mailbox_hostbox_write(0, (uint8_t *)&reply, sizeof(reply)); return 1; }
diff --git a/src/platform/baytrail/platform.c b/src/platform/baytrail/platform.c index 3114ac1..c5c9108 100644 --- a/src/platform/baytrail/platform.c +++ b/src/platform/baytrail/platform.c @@ -92,7 +92,7 @@ int platform_boot_complete(uint32_t boot_message) { uint64_t outbox = MAILBOX_HOST_OFFSET >> 3;
- mailbox_dspbox_write(0, &ready, sizeof(ready)); + mailbox_dspbox_write(0, (uint8_t *)&ready, sizeof(ready));
/* now interrupt host to tell it we are done booting */ shim_write(SHIM_IPCDL, SOF_IPC_FW_READY | outbox);
Coverity issue: 254842 Uninitialized scalar variable The variable will contain an arbitrary value left from earlier computations.
In parse_page_descriptors: Use of an uninitialized variable (CWE-457) The elem.src variable is not initialized, but in dma_trace_host_buffer() the code reads this initialized value.
Break
*e = *elem;
in e->dest = elem->dest; e->size = elem->size;
to only access relevant fields.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- src/lib/dma-trace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index 7569d5d..429c7b6 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -185,7 +185,10 @@ int dma_trace_host_buffer(struct dma_trace_data *d, struct dma_sg_elem *elem, if (e == NULL) return -ENOMEM;
- *e = *elem; + /* copy fields - excluding possibly non-initialized elem->src */ + e->dest = elem->dest; + e->size = elem->size; + d->host_size = host_size;
list_item_append(&e->list, &d->config.elem_list);
On Fri, 2018-01-19 at 16:33 -0600, Pierre-Louis Bossart wrote:
Detected with Coverity:
In do_notify: The indentation of this code suggests it is nested when it is not. (CWE-483)
multi_stmt_macro: The macro on this line expands into multiple statements, only the first of which is nested within the preceding parent while the rest are not.
if (msg->rx_size && msg->rx_size < SOF_IPC_MSG_MAX_SIZE) mailbox_dspbox_read(msg->rx_data, 0, msg->rx_size);
Move mailbox macros to inline functions to remove the issue, keep indentation the same and add typecasts as needed
FIXME: a. is type uint8_t * ok for data? b. is uint32_t ok for offset (renamed from dest since it was not a pointer) c. This code is full of integer/pointer conversions which will have to be cleaned-up for MISRA compliance
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
src/include/reef/mailbox.h | 44 ++++++++++++++++++++++++++-------------- src/ipc/byt-ipc.c | 2 +- src/ipc/intel-ipc.c | 23 +++++++++++---------- src/platform/baytrail/platform.c | 2 +- 4 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/src/include/reef/mailbox.h b/src/include/reef/mailbox.h index c0580a9..c3976a3 100644 --- a/src/include/reef/mailbox.h +++ b/src/include/reef/mailbox.h @@ -34,6 +34,7 @@ #include <platform/mailbox.h> #include <arch/cache.h> #include <stdint.h> +#include <reef/reef.h>
/* 4k should be enough for everyone ..... */ #define IPC_MAX_MAILBOX_BYTES 0x1000 @@ -62,20 +63,33 @@ #define mailbox_get_debug_size() \ MAILBOX_DEBUG_SIZE
-#define mailbox_dspbox_write(dest, src, bytes) \
- rmemcpy((void*)(MAILBOX_DSPBOX_BASE + dest), src, bytes); \
- dcache_writeback_region((void*)(MAILBOX_DSPBOX_BASE + dest), bytes);
-#define mailbox_dspbox_read(dest, src, bytes) \
- dcache_invalidate_region((void*)(MAILBOX_DSPBOX_BASE + src), bytes); \
- rmemcpy(dest, (void*)(MAILBOX_DSPBOX_BASE + src), bytes);
-#define mailbox_hostbox_write(dest, src, bytes) \
- rmemcpy((void*)(MAILBOX_HOSTBOX_BASE + dest), src, bytes); \
- dcache_writeback_region((void*)(MAILBOX_HOSTBOX_BASE + dest), bytes);
-#define mailbox_hostbox_read(dest, src, bytes) \
- dcache_invalidate_region((void*)(MAILBOX_HOSTBOX_BASE + src), bytes); \
- rmemcpy(dest, (void*)(MAILBOX_HOSTBOX_BASE + src), bytes);
+static inline +void mailbox_dspbox_write(uint32_t offset, uint8_t *src, uint32_t bytes)
probably best making this size_t offset, void *src, size_t bytes so it becomes more portable (and also follows the memcpy() convention a little more).
Liam --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Fri, 2018-01-19 at 16:33 -0600, Pierre-Louis Bossart wrote:
Coverity issue: 254842 Uninitialized scalar variable The variable will contain an arbitrary value left from earlier computations.
In parse_page_descriptors: Use of an uninitialized variable (CWE-457) The elem.src variable is not initialized, but in dma_trace_host_buffer() the code reads this initialized value.
Break
*e = *elem;
in e->dest = elem->dest; e->size = elem->size;
to only access relevant fields.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel .com>
Applied.
Thanks
Liam
On 1/22/18 6:15 AM, Liam Girdwood wrote:
On Fri, 2018-01-19 at 16:33 -0600, Pierre-Louis Bossart wrote:
Detected with Coverity:
In do_notify: The indentation of this code suggests it is nested when it is not. (CWE-483)
multi_stmt_macro: The macro on this line expands into multiple statements, only the first of which is nested within the preceding parent while the rest are not.
if (msg->rx_size && msg->rx_size < SOF_IPC_MSG_MAX_SIZE) mailbox_dspbox_read(msg->rx_data, 0, msg->rx_size);
Move mailbox macros to inline functions to remove the issue, keep indentation the same and add typecasts as needed
FIXME: a. is type uint8_t * ok for data? b. is uint32_t ok for offset (renamed from dest since it was not a pointer) c. This code is full of integer/pointer conversions which will have to be cleaned-up for MISRA compliance
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
src/include/reef/mailbox.h | 44 ++++++++++++++++++++++++++-------------- src/ipc/byt-ipc.c | 2 +- src/ipc/intel-ipc.c | 23 +++++++++++---------- src/platform/baytrail/platform.c | 2 +- 4 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/src/include/reef/mailbox.h b/src/include/reef/mailbox.h index c0580a9..c3976a3 100644 --- a/src/include/reef/mailbox.h +++ b/src/include/reef/mailbox.h @@ -34,6 +34,7 @@ #include <platform/mailbox.h> #include <arch/cache.h> #include <stdint.h> +#include <reef/reef.h>
/* 4k should be enough for everyone ..... */ #define IPC_MAX_MAILBOX_BYTES 0x1000 @@ -62,20 +63,33 @@ #define mailbox_get_debug_size() \ MAILBOX_DEBUG_SIZE
-#define mailbox_dspbox_write(dest, src, bytes) \
- rmemcpy((void*)(MAILBOX_DSPBOX_BASE + dest), src, bytes); \
- dcache_writeback_region((void*)(MAILBOX_DSPBOX_BASE + dest), bytes);
-#define mailbox_dspbox_read(dest, src, bytes) \
- dcache_invalidate_region((void*)(MAILBOX_DSPBOX_BASE + src), bytes); \
- rmemcpy(dest, (void*)(MAILBOX_DSPBOX_BASE + src), bytes);
-#define mailbox_hostbox_write(dest, src, bytes) \
- rmemcpy((void*)(MAILBOX_HOSTBOX_BASE + dest), src, bytes); \
- dcache_writeback_region((void*)(MAILBOX_HOSTBOX_BASE + dest), bytes);
-#define mailbox_hostbox_read(dest, src, bytes) \
- dcache_invalidate_region((void*)(MAILBOX_HOSTBOX_BASE + src), bytes); \
- rmemcpy(dest, (void*)(MAILBOX_HOSTBOX_BASE + src), bytes);
+static inline +void mailbox_dspbox_write(uint32_t offset, uint8_t *src, uint32_t bytes)
probably best making this size_t offset, void *src, size_t bytes so it becomes more portable (and also follows the memcpy() convention a little more).
Yeah, that's what I was thinking initially but then the msg->rx_data is uint8_t so what generates fewer typecasts Will use void and size_t
Liam
participants (3)
-
Liam Girdwood -
Liam Girdwood -
Pierre-Louis Bossart