[PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[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

RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Andrew Gabbasov]

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

Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Sergei Shtylyov]

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

RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Andrew Gabbasov]

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

Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Sergei Shtylyov]

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

Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Sergei Shtylyov]

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

RE: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()

[Andrew Gabbasov]

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