* [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()
@ 2020-09-30 19:21 Andrew Gabbasov
2020-10-01 7:13 ` Andrew Gabbasov
2020-10-24 18:09 ` Sergei Shtylyov
0 siblings, 2 replies; 7+ messages in thread
From: Andrew Gabbasov @ 2020-09-30 19:21 UTC (permalink / raw)
To: linux-renesas-soc, netdev, linux-kernel, Sergei Shtylyov,
David S. Miller, geert+renesas, Julia Lawall, Dirk Behme,
Eugeniu Rosca, Andrew Gabbasov
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.
Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
drivers/net/ethernet/renesas/ravb_main.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index df89d09b253e..c0610b2d3b14 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1802,12 +1802,16 @@ static int ravb_hwtstamp_get(struct net_device *ndev, struct ifreq *req)
config.flags = 0;
config.tx_type = priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON :
HWTSTAMP_TX_OFF;
- if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT)
+ switch (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE) {
+ case 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)
+ break;
+ case RAVB_RXTSTAMP_TYPE_ALL:
config.rx_filter = HWTSTAMP_FILTER_ALL;
- else
+ break;
+ default:
config.rx_filter = HWTSTAMP_FILTER_NONE;
+ }
return copy_to_user(req->ifr_data, &config, sizeof(config)) ?
-EFAULT : 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-09-30 19:21 [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() Andrew Gabbasov @ 2020-10-01 7:13 ` Andrew Gabbasov 2020-10-17 19:49 ` Sergei Shtylyov 2020-10-24 18:09 ` Sergei Shtylyov 1 sibling, 1 reply; 7+ messages in thread From: Andrew Gabbasov @ 2020-10-01 7:13 UTC (permalink / raw) To: Sergei Shtylyov Cc: linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Behme, Dirk - Bosch, Eugeniu Rosca Hi Sergei, > -----Original Message----- > From: Gabbasov, Andrew > Sent: Wednesday, September 30, 2020 10:21 PM > To: linux-renesas-soc@vger.kernel.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Sergei Shtylyov <sergei.shtylyov@gmail.com>; David > S. Miller <davem@davemloft.net>; geert+renesas@glider.be; Julia Lawall > <julia.lawall@inria.fr>; Behme, Dirk - Bosch <dirk.behme@de.bosch.com>; > Eugeniu Rosca <erosca@de.adit-jv.com>; Gabbasov, Andrew > <Andrew_Gabbasov@mentor.com> > Subject: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() > > 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. Thanks! Best regards, Andrew > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Reported-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > b/drivers/net/ethernet/renesas/ravb_main.c > index df89d09b253e..c0610b2d3b14 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1802,12 +1802,16 @@ static int ravb_hwtstamp_get(struct net_device > *ndev, struct ifreq *req) > config.flags = 0; > config.tx_type = priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : > HWTSTAMP_TX_OFF; > - if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT) > + switch (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE) { > + case 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) > + break; > + case RAVB_RXTSTAMP_TYPE_ALL: > config.rx_filter = HWTSTAMP_FILTER_ALL; > - else > + break; > + default: > config.rx_filter = HWTSTAMP_FILTER_NONE; > + } > > return copy_to_user(req->ifr_data, &config, sizeof(config)) ? > -EFAULT : 0; > -- > 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-10-01 7:13 ` Andrew Gabbasov @ 2020-10-17 19:49 ` Sergei Shtylyov 2020-10-19 7:32 ` Andrew Gabbasov 0 siblings, 1 reply; 7+ messages in thread From: Sergei Shtylyov @ 2020-10-17 19:49 UTC (permalink / raw) To: Andrew Gabbasov Cc: linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Behme, Dirk - Bosch, Eugeniu Rosca 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)... [...] >> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") >> Reported-by: Julia Lawall <julia.lawall@inria.fr> >> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> >> --- >> drivers/net/ethernet/renesas/ravb_main.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c >> b/drivers/net/ethernet/renesas/ravb_main.c >> index df89d09b253e..c0610b2d3b14 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -1802,12 +1802,16 @@ static int ravb_hwtstamp_get(struct net_device >> *ndev, struct ifreq *req) >> config.flags = 0; >> config.tx_type = priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : >> HWTSTAMP_TX_OFF; >> - if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT) >> + switch (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE) { >> + case 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) >> + break; >> + case RAVB_RXTSTAMP_TYPE_ALL: >> config.rx_filter = HWTSTAMP_FILTER_ALL; >> - else >> + break; >> + default: >> config.rx_filter = HWTSTAMP_FILTER_NONE; 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? [...] MBR, Sergei ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-10-17 19:49 ` Sergei Shtylyov @ 2020-10-19 7:32 ` Andrew Gabbasov 2020-10-24 18:02 ` Sergei Shtylyov 0 siblings, 1 reply; 7+ messages in thread From: Andrew Gabbasov @ 2020-10-19 7:32 UTC (permalink / raw) To: 'Sergei Shtylyov' Cc: linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Behme, Dirk - Bosch, Eugeniu Rosca Hello Sergei, > -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtylyov@gmail.com] > Sent: Saturday, October 17, 2020 10:49 PM > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > Cc: linux-renesas-soc@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; David S. Miller > <davem@davemloft.net>; geert+renesas@glider.be; Julia Lawall <julia.lawall@inria.fr>; Behme, Dirk - Bosch > <dirk.behme@de.bosch.com>; Eugeniu Rosca <erosca@de.adit-jv.com> > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-10-19 7:32 ` Andrew Gabbasov @ 2020-10-24 18:02 ` Sergei Shtylyov 2020-10-26 10:29 ` Andrew Gabbasov 0 siblings, 1 reply; 7+ messages in thread From: Sergei Shtylyov @ 2020-10-24 18:02 UTC (permalink / raw) To: Andrew Gabbasov Cc: linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Behme, Dirk - Bosch, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-10-24 18:02 ` Sergei Shtylyov @ 2020-10-26 10:29 ` Andrew Gabbasov 0 siblings, 0 replies; 7+ messages in thread From: Andrew Gabbasov @ 2020-10-26 10:29 UTC (permalink / raw) To: 'Sergei Shtylyov' Cc: linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Behme, Dirk - Bosch, Eugeniu Rosca Hi Sergei, Thank you for the review. > -----Original Message----- > From: Sergei Shtylyov [mailto:sergei.shtylyov@gmail.com] > Sent: Saturday, October 24, 2020 9:02 PM > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > Cc: linux-renesas-soc@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; David S. Miller > <davem@davemloft.net>; geert+renesas@glider.be; Julia Lawall <julia.lawall@inria.fr>; Behme, Dirk - Bosch > <dirk.behme@de.bosch.com>; Eugeniu Rosca <erosca@de.adit-jv.com> > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() 2020-09-30 19:21 [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() Andrew Gabbasov 2020-10-01 7:13 ` Andrew Gabbasov @ 2020-10-24 18:09 ` Sergei Shtylyov 1 sibling, 0 replies; 7+ messages in thread From: Sergei Shtylyov @ 2020-10-24 18:09 UTC (permalink / raw) To: Andrew Gabbasov, linux-renesas-soc, netdev, linux-kernel, David S. Miller, geert+renesas, Julia Lawall, Dirk Behme, Eugeniu Rosca On 9/30/20 10:21 PM, Andrew Gabbasov wrote: > 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. > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Reported-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> [...] MBR, Sergei ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-26 10:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 19:21 [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get() Andrew Gabbasov 2020-10-01 7:13 ` Andrew Gabbasov 2020-10-17 19:49 ` Sergei Shtylyov 2020-10-19 7:32 ` Andrew Gabbasov 2020-10-24 18:02 ` Sergei Shtylyov 2020-10-26 10:29 ` Andrew Gabbasov 2020-10-24 18:09 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).