RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning message

[Liming Sun]

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: Tuesday, October 10, 2023 4:06 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,
> 
> On 10/9/23 21:28, Liming Sun wrote:
> >
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> Sent: Friday, October 6, 2023 1:07 PM
> >> 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/6/23 17:50, Liming Sun wrote:
> >>> 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.
> >>
> >> If IS_VRING_DROP() is ONLY set to TRUE for Rx, then it
> >> should simply *not* be checked *at all* in the tx paths.
> >
> > IS_VRING_DROP() itself is actually not checked in the Tx path.  It is the "!
> IS_VRING_DROP()" that checks the Rx/Tx, something like:
> >
> > if (!IS_VRING_DROP(vring)) {
> > 	if (is_rx)
> > 		...
> > 	else
> > 		...
> > }
> >
> > The reason is that I thought we might reuse the ' IS_VRING_DROP' for Tx
> later.
> >
> > However, if the logic looks confusing, I could revise it to something like:
> >
> > if (is_rx) {
> > 	if (!IS_VRING_DROP(vring))
> > 		...
> > } else {
> > 		...
> > }
> 
> Yes please revise the log to look like this, this should also
> fix the warning without needing to initialize data to 0.
> 
> Since now in the tx path you are guaranteed to first fill
> data before sending it.

Thanks, done. Updated in v2.

> 
> Regards,
> 
> Hans
> 
> 
> 
> >>>> -----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
> >>>>
> >>>>
> >>>>
> >>>
> >