Thanks Hans.
Below is the logic:
IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
Line 696: *desc = &vring->drop_desc;
Line 742: desc = &vring->drop_desc;
So line 634 below will never happen when IS_VRING_DROP() is TRUE due the checking of line 633.
633 if (!is_rx)
634 writeq(data, fifo->tx.data);
Please correct me if it's my misunderstanding.
Thanks,
Liming
> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: Friday, October 6, 2023 8:54 AM
> To: Liming Sun <limings@xxxxxxxxxx>; Vadim Pasternak
> <vadimp@xxxxxxxxxx>; David Thompson <davthompson@xxxxxxxxxx>; Mark
> Gross <markgross@xxxxxxxxxx>; Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> message
>
> Hi Liming,
>
> On 10/5/23 14:18, Liming Sun wrote:
> > This commit fixes the smatch static checker warning in
> > mlxbf_tmfifo_rxtx_word() which complains data not initialized at
> > line 634 when IS_VRING_DROP() is TRUE. This is not a real bug since
> > line 634 is for Tx while IS_VRING_DROP() is only set for Rx. So there
> > is no case that line 634 is executed when IS_VRING_DROP() is TRUE.
> >
> > This commit initializes the local data variable to avoid unnecessary
> > confusion to those static analyzing tools.
> >
> > Signed-off-by: Liming Sun <limings@xxxxxxxxxx>
> > ---
> > drivers/platform/mellanox/mlxbf-tmfifo.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > index f3696a54a2bd..ccc4b51d3379 100644
> > --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> > +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > @@ -595,8 +595,8 @@ static void mlxbf_tmfifo_rxtx_word(struct
> mlxbf_tmfifo_vring *vring,
> > {
> > struct virtio_device *vdev = vring->vq->vdev;
> > struct mlxbf_tmfifo *fifo = vring->fifo;
> > + u64 data = 0;
> > void *addr;
> > - u64 data;
> >
> > /* Get the buffer address of this desc. */
> > addr = phys_to_virt(virtio64_to_cpu(vdev, desc->addr));
>
>
> This will fix the warning but not the issue at hand. As Dan pointed
> out in his original bug report, the issue is that after:
>
> 78034cbece79 ("platform/mellanox: mlxbf-tmfifo: Drop the Rx packet if no
> descriptors")
>
> We now have this IS_VRING_DROP() check in the path, which despite
> the subject writeq(data, fifo->tx.data);is currently being applied to both rx and
> tx vring-s
> and when this returns true the memcpy from the ring to &data
> will not happen, but the code will still do:
>
> writeq(data, fifo->tx.data);
>
> So you may have silenced the warning now, but you will still write
> data not coming from the vring to transmit. The only difference
> is you are now guaranteed to write all zeroes.
>
> Note another older issue is that if you hit the not enough space
> path:
>
> } else {
> /* Leftover bytes. */
> if (!IS_VRING_DROP(vring)) {
> if (is_rx)
> memcpy(addr + vring->cur_len, &data,
> len - vring->cur_len);
> else
> memcpy(&data, addr + vring->cur_len,
> len - vring->cur_len);
> }
> vring->cur_len = len;
> }
>
> Then even if IS_VRING_DROP() returns true you are only initializing some bytes
> of the 8 bytes data variable and the other bytes will stay at whatever random
> value they had before and you end up writing this random bytes when doing:
>
> writeq(data, fifo->tx.data);
>
> Regards,
>
> Hans
>
>
>
