Re: [PATCH net-next 1/3] net: cpsw: return proper RX timestamping filter in cpsw_hwtstamp_get()

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 08/05/2025 20:48, Vladimir Oltean wrote:
> priv->rx_ts_enabled is a boolean variable (0 or 1). Overlapped over enum
> hwtstamp_rx_filters, it makes cfg.rx_filter take the value of either
> HWTSTAMP_FILTER_NONE (when 0) or HWTSTAMP_FILTER_ALL (when 1).

Hmm.. I have to disagree here. rx_ts_enabled is int, not bool:

struct cpsw_priv {
         struct net_device               *ndev;
         struct device                   *dev;
         u32                             msg_enable;
         u8                              mac_addr[ETH_ALEN];
         bool                            rx_pause;
         bool                            tx_pause;
         bool                            mqprio_hw;
         int                             fifo_bw[CPSW_TC_NUM];
         int                             shp_cfg_speed;
         int                             tx_ts_enabled;
         int                             rx_ts_enabled;
         struct bpf_prog                 *xdp_prog;
	....


And it's assigned a value of HWTSTAMP_FILTER_PTP_V2_EVENT in
cpsw_hwtstamp_set(). Not sure this change is actually needed.

> 
> But this is inconsistent with what is returned in cpsw_hwtstamp_set().
> There, HWTSTAMP_FILTER_ALL is refused (-ERANGE), and a subset of the RX
> filters requestable by user space are all replaced with
> HWTSTAMP_FILTER_PTP_V2_EVENT. So the driver should be reporting this
> value during SIOCGHWTSTAMP as well.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>   drivers/net/ethernet/ti/cpsw_priv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
> index 6fe4edabba44..68d8f7ea0e44 100644
> --- a/drivers/net/ethernet/ti/cpsw_priv.c
> +++ b/drivers/net/ethernet/ti/cpsw_priv.c
> @@ -687,7 +687,8 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
>   
>   	cfg.flags = 0;
>   	cfg.tx_type = priv->tx_ts_enabled ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
> -	cfg.rx_filter = priv->rx_ts_enabled;
> +	cfg.rx_filter = priv->rx_ts_enabled ? HWTSTAMP_FILTER_PTP_V2_EVENT :
> +			HWTSTAMP_FILTER_NONE;
>   
>   	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
>   }