Hello Sergei, > -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtylyov@xxxxxxxxx] > Sent: Saturday, October 17, 2020 10:49 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/1/20 10:13 AM, Andrew Gabbasov wrote: > > 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. > > > > 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. Thanks. Best regards, Andrew
