Re: [PATCH] net/mlx5: fix metadata endianness in modify field action

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/16/2021 9:50 AM, Slava Ovsiienko wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, December 7, 2021 15:45
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
>> <rasland@nvidia.com>; stable@dpdk.org
>> Subject: Re: [PATCH] net/mlx5: fix metadata endianness in modify field action
>>
>> On 11/29/2021 12:32 PM, Viacheslav Ovsiienko wrote:
>>> As modify field action immediate source parameter the metadata should
>>> follow the CPU endianness (according to SET_META action structure
>>> format), and mlx5 PMD wrongly handled the immediate parameter
>> metadata
>>> buffer as big-endian, resulting in wrong metadata set action with
>>> incorrect endianness.
>>>
>>> Fixes: 40c8fb1fd3b3 ("net/mlx5: update modify field action")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>> ---
>>>    drivers/net/mlx5/mlx5_flow_dv.c | 23 +++++++++++------------
>>>    1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
>>> b/drivers/net/mlx5/mlx5_flow_dv.c index 4834c752d9..1c6cae8779 100644
>>> --- a/drivers/net/mlx5/mlx5_flow_dv.c
>>> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
>>> @@ -1465,7 +1465,7 @@ static void
>>>    mlx5_flow_field_id_to_modify_info
>>>    		(const struct rte_flow_action_modify_data *data,
>>>    		 struct field_modify_info *info, uint32_t *mask,
>>> -		 uint32_t width, uint32_t *shift, struct rte_eth_dev *dev,
>>> +		 uint32_t width, struct rte_eth_dev *dev,
>>
>> Hi Viacheslav,
>>
>> Is removing (unused) 'shift' variable related to the problem mentioned in the
>> commit log?
> Related indirectly to metadata, but not directly to the issue.
> "shift" is unused leftover after changing immediate value format.
> And this patch just provides collateral cleanup. Do you think we should
> separate into dedicated cleanup patch? Or mention this cleanup in commit
> message?
> 

if not directly related I think better to split on its own patch, this
makes it more clear to possible future references to the patches.