Coverity points out that in ctucan_send_ready_buffers() we
set buff_st_mask = 0xf << (i * 4) inside the loop, but then
we never use it before overwriting it later.
The only thing we use the mask for is as part of the code that is
inserting the new buff_st field into tx_status. That is more
comprehensibly written using deposit32(), so do that and drop the
mask variable entirely.
We also update the buff_st local variable at multiple points
during this function, but nothing can ever see these
intermediate values, so just drop those, write the final
TXT_TOK as a fixed constant value, and collapse the only
remaining set/use of buff_st down into an extract32().
Fixes: Coverity CID 1432869
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/net/can/ctucan_core.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index ea09bf71a0c..f2ce978e5ec 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
uint8_t *pf;
int buff2tx_idx;
uint32_t tx_prio_max;
- unsigned int buff_st;
- uint32_t buff_st_mask;
if (!s->mode_settings.s.ena) {
return;
@@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
uint32_t prio;
- buff_st_mask = 0xf << (i * 4);
- buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
-
- if (buff_st != TXT_RDY) {
+ if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
continue;
}
prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
@@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
if (buff2tx_idx == -1) {
break;
}
- buff_st_mask = 0xf << (buff2tx_idx * 4);
- buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
int_stat.u32 = 0;
- buff_st = TXT_RDY;
pf = s->tx_buffer[buff2tx_idx].data;
ctucan_buff2frame(pf, &frame);
s->status.s.idle = 0;
@@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
s->status.s.idle = 1;
s->status.s.txs = 0;
s->tx_fr_ctr.s.tx_fr_ctr_val++;
- buff_st = TXT_TOK;
int_stat.s.txi = 1;
int_stat.s.txbhci = 1;
s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
- s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
- (buff_st << (buff2tx_idx * 4));
+ s->tx_status.u32 = deposit32(s->tx_status.u32,
+ buff2tx_idx * 4, 4, TXT_TOK);
} while (1);
}
--
2.20.1
Hello Peter,
this one is a little problematic. I understand that you want
to have clean code and no warnings reports from coverity.
On Friday 06 of November 2020 18:11:51 Peter Maydell wrote:
> Coverity points out that in ctucan_send_ready_buffers() we
> set buff_st_mask = 0xf << (i * 4) inside the loop, but then
> we never use it before overwriting it later.
>
> The only thing we use the mask for is as part of the code that is
> inserting the new buff_st field into tx_status. That is more
> comprehensibly written using deposit32(), so do that and drop the
> mask variable entirely.
>
> We also update the buff_st local variable at multiple points
> during this function, but nothing can ever see these
> intermediate values, so just drop those, write the final
> TXT_TOK as a fixed constant value, and collapse the only
> remaining set/use of buff_st down into an extract32().
>
> Fixes: Coverity CID 1432869
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/net/can/ctucan_core.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index ea09bf71a0c..f2ce978e5ec 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) uint8_t *pf;
> int buff2tx_idx;
> uint32_t tx_prio_max;
> - unsigned int buff_st;
> - uint32_t buff_st_mask;
>
> if (!s->mode_settings.s.ena) {
> return;
> @@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
> uint32_t prio;
>
> - buff_st_mask = 0xf << (i * 4);
> - buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
> -
> - if (buff_st != TXT_RDY) {
> + if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
> continue;
> }
It is right replacement. Initially buff_st kept location of bits of the buffer
found to be processed later. But after priorization of Tx this cannot
be used without recompute.
> prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
> @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) if (buff2tx_idx == -1) {
> break;
> }
> - buff_st_mask = 0xf << (buff2tx_idx * 4);
> - buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
There I would kept extracted state in the variable. Actual model is really
simplified to real hardware. Tx succeeds in zero time.
> int_stat.u32 = 0;
> - buff_st = TXT_RDY;
This is why the TXT_RDY state immediately changes to TXT_TOK. It works well
for actual simple CAN subsystem implementation. But if we want to implement
priorization of messages on emulated bus and even simulate real bus latency
by delay to state change and interrut delivery, then we need to proceed
through TXT_RDY state. If it is a problem for release, that your want to have
coverity clean source tree, then please left the line as a comment there
or use some trick with
(void)buff_st;
Or if you prefer, use
+ s->tx_status.u32 = deposit32(s->tx_status.u32,
+ buff2tx_idx * 4, 4, TXT_RDY);
if it silent the coverity.
> pf = s->tx_buffer[buff2tx_idx].data;
> ctucan_buff2frame(pf, &frame);
> s->status.s.idle = 0;
> @@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> *s) s->status.s.idle = 1;
> s->status.s.txs = 0;
> s->tx_fr_ctr.s.tx_fr_ctr_val++;
> - buff_st = TXT_TOK;
> int_stat.s.txi = 1;
> int_stat.s.txbhci = 1;
> s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
> - s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
> - (buff_st << (buff2tx_idx * 4));
> + s->tx_status.u32 = deposit32(s->tx_status.u32,
> + buff2tx_idx * 4, 4, TXT_TOK);
> } while (1);
> }
Thanks for your time, I have planned to look and these and attempt
to provide solution which is acceptable but documents our intentions,
but it is on my tasks queue still,
Pavel
On Fri, 6 Nov 2020 at 18:12, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> this one is a little problematic. I understand that you want
> to have clean code and no warnings reports from coverity.
>
> On Friday 06 of November 2020 18:11:51 Peter Maydell wrote:
> > @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> > *s) if (buff2tx_idx == -1) {
> > break;
> > }
> > - buff_st_mask = 0xf << (buff2tx_idx * 4);
> > - buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
>
> There I would kept extracted state in the variable. Actual model is really
> simplified to real hardware. Tx succeeds in zero time.
>
> > int_stat.u32 = 0;
> > - buff_st = TXT_RDY;
>
> This is why the TXT_RDY state immediately changes to TXT_TOK. It works well
> for actual simple CAN subsystem implementation. But if we want to implement
> priorization of messages on emulated bus and even simulate real bus latency
> by delay to state change and interrut delivery, then we need to proceed
> through TXT_RDY state. If it is a problem for release, that your want to have
> coverity clean source tree, then please left the line as a comment there
> or use some trick with
> (void)buff_st;
>
> Or if you prefer, use
>
> + s->tx_status.u32 = deposit32(s->tx_status.u32,
> + buff2tx_idx * 4, 4, TXT_RDY);
>
> if it silent the coverity.
I was going to put a comment in v2 of this patch series to
document that this is where the status goes to TXT_RDY,
but looking at the code, at this point the buffer status field
is *already* TXT_RDY -- the preceding loop does not allow
us to get to this point for an entry which is in any other
state. So while I agree with your suggestion that it's worth
having at least a documentation comment to indicate where the
state is changing, I think there is no intermediate state
transition to document here.
thanks
-- PMM
© 2016 - 2026 Red Hat, Inc.