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

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(-)

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

On Tue, 10 Nov 2020 at 19:37, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> On Tuesday 10 of November 2020 18:06:02 Peter Maydell wrote:
> > @@ -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);

I mentioned this in a reply to my v1 series. The buffer status
in the tx_status field is already TXT_RDY, so there is no state
change happening here to document as far as I can tell ?

thanks
-- PMM