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
