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
