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

[Hans de Goede]

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.

Just setting data = 0 is simply papering over the warning
without actually fixing anything.

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