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

[Pavel Pisa]

Hello Peter,

On Tuesday 10 of November 2020 18:06:02 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 538270e62f9..a400ad13a43 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;

I would prefer to add there next line even that it has no real effect

 +        s->tx_status.u32 = deposit32(s->tx_status.u32,
 +                                     buff2tx_idx * 4, 4, TXT_RDY);

But if it generates warning or you have some other reason not to put
it there, I add my

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

When we separated processsing to call of message submit for Tx
and then separate callback to confirm arbitration win,
we would need to reintroduce this assignment. But there would
be much moresignificant changes that this small notice is not
so important. 

>          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);
>  }


-- 
Yours sincerely

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/