mbox series

[net-next,0/3] Convert cpsw and cpsw_new to ndo_hwtstamp_get() and ndo_hwtstamp_set()

Message ID 20250508194825.3058929-1-vladimir.oltean@nxp.com
Headers show
Series Convert cpsw and cpsw_new to ndo_hwtstamp_get() and ndo_hwtstamp_set() | expand

Message

Vladimir Oltean May 8, 2025, 7:48 p.m. UTC 
This is probably the most difficult conversion to the new timestamping
API, because the cpsw driver makes some efforts to hide the existence of
two slave interfaces with two attached PHYs from the network stack, and
the whole point of the new timestamping API is to let the core stack
make more decisions. The cpsw_new driver doesn't do that.

Patch 2/3 is the main conversion.

Patch 3/3 is a subsequent cleanup/ simplification. It would have helped
my understanding if the code dealing with PHY ioctls wasn't common
between cpsw and cpsw_new, because there are important differences
between them.

Patch 1/3 is probably a minor fix, but in lack of a real-life report
I've targeted it to net-next (leaving the possibility for someone to
later request a backport). It would be very disruptive to my flow, at
this stage, to send this patch to 'net' and resend the rest after the
merge back into net-next.

I hope the changes are correct and reasonable, they are purely based on
static analysis and have only been compile-tested.

Vladimir Oltean (3):
  net: cpsw: return proper RX timestamping filter in cpsw_hwtstamp_get()
  net: cpsw: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()
  net: cpsw: isolate cpsw_ndo_ioctl() to just the old driver

 drivers/net/ethernet/ti/cpsw.c      | 25 ++++++++++
 drivers/net/ethernet/ti/cpsw_new.c  |  4 +-
 drivers/net/ethernet/ti/cpsw_priv.c | 71 +++++++++++------------------
 drivers/net/ethernet/ti/cpsw_priv.h |  6 ++-
 4 files changed, 59 insertions(+), 47 deletions(-)

Comments

Vadim Fedorenko May 8, 2025, 10:33 p.m. UTC | #1 
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;
>   }
Vladimir Oltean May 9, 2025, 1:19 p.m. UTC | #2 
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.