Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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-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

* 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

end of thread, back to index

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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git