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

[Hans de Goede]

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.

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