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

[Andrew Gabbasov]

Hi Sergei,

Thank you for the review.

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov@xxxxxxxxx]
> Sent: Saturday, October 24, 2020 9:02 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@xxxxxxxxxx>
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; David S. Miller
> <davem@xxxxxxxxxxxxx>; geert+renesas@xxxxxxxxx; Julia Lawall <julia.lawall@xxxxxxxx>; Behme, Dirk - Bosch
> <dirk.behme@xxxxxxxxxxxx>; Eugeniu Rosca <erosca@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()
> 
> 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?

Yes, this is how it is implemented now. Frankly speaking, I didn't dig
too deeply into the deriver code to understand whether it is correct
and if there could be any other variants.

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

I've resubmitted the patch as v2. It is re-based on top of the latest linux master.
Since you sent your "Reviewed-by:" for this patch and there were no changes other
than file offsets, I took the liberty to add "Reviewed-by:" with your name too.


Thanks!

Best regards,
Andrew