Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Sergei Shtylyov]

Hello!

On 10/19/20 10:32 AM, Andrew Gabbasov wrote:

   Sorry for the delay again, I keep forgetting about the mails I' couldn't reply
quickly. :-|

[...]
>>    The patch was set to the "Changes Requested" state -- most probably because of this
>> mail. Though unintentionally, it served to throttle actions on this patch. I did only
>> remember about this patch yesterday... :-)
>>
>> [...]
>>>> In the function ravb_hwtstamp_get() in ravb_main.c with the existing values
>>>> for RAVB_RXTSTAMP_TYPE_V2_L2_EVENT (0x2) and RAVB_RXTSTAMP_TYPE_ALL (0x6)
>>>>
>>>> if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT)
>>>> 	config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>>>> else if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_ALL)
>>>> 	config.rx_filter = HWTSTAMP_FILTER_ALL;
>>>>
>>>> if the test on RAVB_RXTSTAMP_TYPE_ALL should be true, it will never be
>>>> reached.
>>>>
>>>> This issue can be verified with 'hwtstamp_config' testing program
>>>> (tools/testing/selftests/net/hwtstamp_config.c). Setting filter type to ALL
>>>> and subsequent retrieving it gives incorrect value:
>>>>
>>>> $ hwtstamp_config eth0 OFF ALL
>>>> flags = 0
>>>> tx_type = OFF
>>>> rx_filter = ALL
>>>> $ hwtstamp_config eth0
>>>> flags = 0
>>>> tx_type = OFF
>>>> rx_filter = PTP_V2_L2_EVENT
>>>>
>>>> Correct this by converting if-else's to switch.
>>>
>>> Earlier you proposed to fix this issue by changing the value
>>> of RAVB_RXTSTAMP_TYPE_ALL constant to 0x4.
>>> Unfortunately, simple changing of the constant value will not
>>> be enough, since the code in ravb_rx() (actually determining
>>> if timestamp is needed)
>>>
>>> u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
>>> [...]
>>> get_ts &= (q == RAVB_NC) ?
>>>                 RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
>>>                 ~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
>>>
>>> will work incorrectly and will need to be fixed too, making this
>>> piece of code more complicated.

   Judging on the above code, we can only stamp RAVB_RXTSTAMP_TYPE_V2_L2_EVENT
on the NC queue, and the rest only on the BE queue, right?

>>> So, it's probably easier and safer to keep the constant value and
>>> the code in ravb_rx() intact, and just fix the get ioctl code,
>>> where the issue is actually located.
>>
>>    We have one more issue with the current driver: bit 2 of priv->tstamp_rx_ctrl
>> can only be set as a part of the ALL mask, not individually. I'm now thinking we
>> should set RAVB_RXTSTAMP_TYPE[_ALL] to 2 (and probably just drop the ALL mask)...
> 
> [skipped]
> 
>>    Yeah, that's better. But do we really need am anonymous bit 2 that can't be
>> toggled other than via passing the ALL mask?
> 
> The driver supports setting timestamps either for all packets or for some
> particular kind of packets (events). Bit 1 in internal mask corresponds
> to this selected kind. Bit 2 corresponds to all other packets, and ALL mask 
> combines both variants. Although bit 2 can't be controlled individually
> (since there is no much sense to Request stamping of only packets, other than
> events, moreover, there is no user-visible filter constant to represent it),
> and that's why is anonymous, it provides a convenient way to handle stamping
> logic in ravb_rx(), so I don't see an immediate need to get rid of it.

    OK, you convinced me. :-)
    I suggest that you repost the patch since it's now applying with a large offset.

[...]
> Best regards,
> Andrew

MBR, Sergei