RE: [PATCH] ASoC: renesas: rz-ssi: Add a check for negative sample_space

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 08 January 2025 10:58
> Subject: Re: [PATCH] ASoC: renesas: rz-ssi: Add a check for negative sample_space
> 
> Hi Dan,
> 
> On Wed, Jan 8, 2025 at 10:29 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > My static checker rule complains about this code.  The concern is that
> > if "sample_space" is negative then the "sample_space >= runtime->channels"
> > condition will not work as intended because it will be type promoted
> > to a high unsigned int value.
> >
> > strm->fifo_sample_size is SSI_FIFO_DEPTH (32).  The SSIFSR_TDC_MASK is
> > 0x3f.  Without any further context it does seem like a reasonable
> > warning and it can't hurt to add a check for negatives.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 03e786bd4341 ("ASoC: sh: Add RZ/G2L SSIF-2 driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/sound/soc/renesas/rz-ssi.c
> > +++ b/sound/soc/renesas/rz-ssi.c
> > @@ -521,6 +521,8 @@ static int rz_ssi_pio_send(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
> >         sample_space = strm->fifo_sample_size;
> >         ssifsr = rz_ssi_reg_readl(ssi, SSIFSR);
> >         sample_space -= (ssifsr >> SSIFSR_TDC_SHIFT) &
> > SSIFSR_TDC_MASK;
> > +       if (sample_space < 0)
> > +               return -EINVAL;
> >
> >         /* Only add full frames at a time */
> >         while (frames_left && (sample_space >= runtime->channels)) {
> 
> In practice this cannot happen, as the maximum value of the field is 0x20 (= SSI_FIFO_DEPTH), but
> better safe than sorry, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> 
> Biju/Prabhakar: The documentation for the TDC bits in the FIFO Status Register seems to be incorrect
> (in all of the RZ/G2L, RZ/G2UL, RZ/G3S, and RZ/A2M documentation):
> 
>     TDC[5:0] Bits
>     These bits indicate the number of valid data that are stored in
>     the transmit FIFO data register (SSIFTDR). With this flag as H’0,
>     there is no data to be transmitted. With H’10, there is no space to
>     write data.
> 
> As the FIFO size is 4 bytes x 32 stages for both the transmit and receive FIFOs, the above should be
> H'20 instead of H'10.

Thanks for pointing it out. Will check this with HW documentation team about this issue.

Cheers,
Biju