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

[Andrew Gabbasov <andrew_gabbasov@mentor.com>]

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