| Message ID | 20200930192124.25060-1-andrew_gabbasov@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [net] ravb: Fix bit fields checking in ravb_hwtstamp_get() | expand |
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
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
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
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
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
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
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;
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(-)