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).

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;
 }
-- 
2.43.0

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;
>   }

On Thu, May 08, 2025 at 11:33:06PM +0100, Vadim Fedorenko wrote:
> 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.

You're right, thanks for pointing it out. I had searched for
"rx_ts_enabled" and mistook the first occurrence, in am65-cpsw-nuss.h,
as the definition for this driver. The patch is not needed in that case.