Re: [PATCH for-5.2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()

[Pavel Pisa]

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