[PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Vladimir Oltean]

New timestamping API was introduced in commit 66f7223039c0 ("net: add
NDOs for configuring hardware timestamping") from kernel v6.6. It is
time to convert the intel ixp4xx ethernet driver to the new API, so that
the ndo_eth_ioctl() path can be removed completely.

hwtstamp_get() and hwtstamp_set() are only called if netif_running()
when the code path is engaged through the legacy ioctl. As I don't
want to make an unnecessary functional change which I can't test,
preserve that restriction when going through the new operations.

When cpu_is_ixp46x() is false, the execution of SIOCGHWTSTAMP and
SIOCSHWTSTAMP falls through to phy_mii_ioctl(), which may process it in
case of a timestamping PHY, or may return -EOPNOTSUPP. In the new API,
the core handles timestamping PHYs directly and does not call the netdev
driver, so just return -EOPNOTSUPP directly for equivalent logic.

A gratuitous change I chose to do anyway is prefixing hwtstamp_get() and
hwtstamp_set() with the driver name, ipx4xx. This reflects modern coding
sensibilities, and we are touching the involved lines anyway.

The remainder of eth_ioctl() is exactly equivalent to
phy_do_ioctl_running(), so use that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/xscale/ixp4xx_eth.c | 61 +++++++++++-------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index a2ab1c150822..e1e7f65553e7 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -394,16 +394,20 @@ static void ixp_tx_timestamp(struct port *port, struct sk_buff *skb)
 	__raw_writel(TX_SNAPSHOT_LOCKED, &regs->channel[ch].ch_event);
 }
 
-static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
+static int ixp4xx_hwtstamp_set(struct net_device *netdev,
+			       struct kernel_hwtstamp_config *cfg,
+			       struct netlink_ext_ack *extack)
 {
-	struct hwtstamp_config cfg;
 	struct ixp46x_ts_regs *regs;
 	struct port *port = netdev_priv(netdev);
 	int ret;
 	int ch;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
+	if (!cpu_is_ixp46x())
+		return -EOPNOTSUPP;
+
+	if (!netif_running(netdev))
+		return -EINVAL;
 
 	ret = ixp46x_ptp_find(&port->timesync_regs, &port->phc_index);
 	if (ret)
@@ -412,10 +416,10 @@ static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
 	ch = PORT2CHANNEL(port);
 	regs = port->timesync_regs;
 
-	if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
+	if (cfg->tx_type != HWTSTAMP_TX_OFF && cfg->tx_type != HWTSTAMP_TX_ON)
 		return -ERANGE;
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		port->hwts_rx_en = 0;
 		break;
@@ -431,39 +435,45 @@ static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	port->hwts_tx_en = cfg.tx_type == HWTSTAMP_TX_ON;
+	port->hwts_tx_en = cfg->tx_type == HWTSTAMP_TX_ON;
 
 	/* Clear out any old time stamps. */
 	__raw_writel(TX_SNAPSHOT_LOCKED | RX_SNAPSHOT_LOCKED,
 		     &regs->channel[ch].ch_event);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
-static int hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
+static int ixp4xx_hwtstamp_get(struct net_device *netdev,
+			       struct kernel_hwtstamp_config *cfg)
 {
-	struct hwtstamp_config cfg;
 	struct port *port = netdev_priv(netdev);
 
-	cfg.flags = 0;
-	cfg.tx_type = port->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+	if (!cpu_is_ixp46x())
+		return -EOPNOTSUPP;
+
+	if (!netif_running(netdev))
+		return -EINVAL;
+
+	cfg->flags = 0;
+	cfg->tx_type = port->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
 
 	switch (port->hwts_rx_en) {
 	case 0:
-		cfg.rx_filter = HWTSTAMP_FILTER_NONE;
+		cfg->rx_filter = HWTSTAMP_FILTER_NONE;
 		break;
 	case PTP_SLAVE_MODE:
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_SYNC;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_SYNC;
 		break;
 	case PTP_MASTER_MODE:
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ;
 		break;
 	default:
 		WARN_ON_ONCE(1);
 		return -ERANGE;
 	}
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static int ixp4xx_mdio_cmd(struct mii_bus *bus, int phy_id, int location,
@@ -985,21 +995,6 @@ static void eth_set_mcast_list(struct net_device *dev)
 }
 
 
-static int eth_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
-{
-	if (!netif_running(dev))
-		return -EINVAL;
-
-	if (cpu_is_ixp46x()) {
-		if (cmd == SIOCSHWTSTAMP)
-			return hwtstamp_set(dev, req);
-		if (cmd == SIOCGHWTSTAMP)
-			return hwtstamp_get(dev, req);
-	}
-
-	return phy_mii_ioctl(dev->phydev, req, cmd);
-}
-
 /* ethtool support */
 
 static void ixp4xx_get_drvinfo(struct net_device *dev,
@@ -1433,9 +1428,11 @@ static const struct net_device_ops ixp4xx_netdev_ops = {
 	.ndo_change_mtu = ixp4xx_eth_change_mtu,
 	.ndo_start_xmit = eth_xmit,
 	.ndo_set_rx_mode = eth_set_mcast_list,
-	.ndo_eth_ioctl = eth_ioctl,
+	.ndo_eth_ioctl = phy_do_ioctl_running,
 	.ndo_set_mac_address = eth_mac_addr,
 	.ndo_validate_addr = eth_validate_addr,
+	.ndo_hwtstamp_get = ixp4xx_hwtstamp_get,
+	.ndo_hwtstamp_set = ixp4xx_hwtstamp_set,
 };
 
 static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev)
-- 
2.43.0

Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Vadim Fedorenko]

On 08/05/2025 22:10, Vladimir Oltean wrote:
> New timestamping API was introduced in commit 66f7223039c0 ("net: add
> NDOs for configuring hardware timestamping") from kernel v6.6. It is
> time to convert the intel ixp4xx ethernet driver to the new API, so that
> the ndo_eth_ioctl() path can be removed completely.
> 
> hwtstamp_get() and hwtstamp_set() are only called if netif_running()
> when the code path is engaged through the legacy ioctl. As I don't
> want to make an unnecessary functional change which I can't test,
> preserve that restriction when going through the new operations.
> 
> When cpu_is_ixp46x() is false, the execution of SIOCGHWTSTAMP and
> SIOCSHWTSTAMP falls through to phy_mii_ioctl(), which may process it in
> case of a timestamping PHY, or may return -EOPNOTSUPP. In the new API,
> the core handles timestamping PHYs directly and does not call the netdev
> driver, so just return -EOPNOTSUPP directly for equivalent logic.
> 
> A gratuitous change I chose to do anyway is prefixing hwtstamp_get() and
> hwtstamp_set() with the driver name, ipx4xx. This reflects modern coding
> sensibilities, and we are touching the involved lines anyway.
> 
> The remainder of eth_ioctl() is exactly equivalent to
> phy_do_ioctl_running(), so use that.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>   drivers/net/ethernet/xscale/ixp4xx_eth.c | 61 +++++++++++-------------
>   1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> index a2ab1c150822..e1e7f65553e7 100644
> --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
> +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> @@ -394,16 +394,20 @@ static void ixp_tx_timestamp(struct port *port, struct sk_buff *skb)
>   	__raw_writel(TX_SNAPSHOT_LOCKED, &regs->channel[ch].ch_event);
>   }
>   
> -static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
> +static int ixp4xx_hwtstamp_set(struct net_device *netdev,
> +			       struct kernel_hwtstamp_config *cfg,
> +			       struct netlink_ext_ack *extack)
>   {
> -	struct hwtstamp_config cfg;
>   	struct ixp46x_ts_regs *regs;
>   	struct port *port = netdev_priv(netdev);
>   	int ret;
>   	int ch;
>   
> -	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> -		return -EFAULT;
> +	if (!cpu_is_ixp46x())
> +		return -EOPNOTSUPP;
> +
> +	if (!netif_running(netdev))
> +		return -EINVAL;
>   
>   	ret = ixp46x_ptp_find(&port->timesync_regs, &port->phc_index);
>   	if (ret)
> @@ -412,10 +416,10 @@ static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
>   	ch = PORT2CHANNEL(port);
>   	regs = port->timesync_regs;
>   
> -	if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
> +	if (cfg->tx_type != HWTSTAMP_TX_OFF && cfg->tx_type != HWTSTAMP_TX_ON)
>   		return -ERANGE;
>   
> -	switch (cfg.rx_filter) {
> +	switch (cfg->rx_filter) {
>   	case HWTSTAMP_FILTER_NONE:
>   		port->hwts_rx_en = 0;
>   		break;
> @@ -431,39 +435,45 @@ static int hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
>   		return -ERANGE;
>   	}
>   
> -	port->hwts_tx_en = cfg.tx_type == HWTSTAMP_TX_ON;
> +	port->hwts_tx_en = cfg->tx_type == HWTSTAMP_TX_ON;
>   
>   	/* Clear out any old time stamps. */
>   	__raw_writel(TX_SNAPSHOT_LOCKED | RX_SNAPSHOT_LOCKED,
>   		     &regs->channel[ch].ch_event);
>   
> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +	return 0;
>   }
>   
> -static int hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
> +static int ixp4xx_hwtstamp_get(struct net_device *netdev,
> +			       struct kernel_hwtstamp_config *cfg)
>   {
> -	struct hwtstamp_config cfg;
>   	struct port *port = netdev_priv(netdev);
>   
> -	cfg.flags = 0;
> -	cfg.tx_type = port->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
> +	if (!cpu_is_ixp46x())
> +		return -EOPNOTSUPP;
> +
> +	if (!netif_running(netdev))
> +		return -EINVAL;

One interesting fact is that phy_do_ioctl_running() will return -ENODEV
in case of !netif_running(netdev) while previous code would return
-EINVAL. Probably it's ok, but may be it's better to have consistent
error path for both options.

Otherwise LGTM,
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>


> +
> +	cfg->flags = 0;
> +	cfg->tx_type = port->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
>   
>   	switch (port->hwts_rx_en) {
>   	case 0:
> -		cfg.rx_filter = HWTSTAMP_FILTER_NONE;
> +		cfg->rx_filter = HWTSTAMP_FILTER_NONE;
>   		break;
>   	case PTP_SLAVE_MODE:
> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_SYNC;
> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_SYNC;
>   		break;
>   	case PTP_MASTER_MODE:
> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ;
> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ;
>   		break;
>   	default:
>   		WARN_ON_ONCE(1);
>   		return -ERANGE;
>   	}
>   
> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +	return 0;
>   }
>   
>   static int ixp4xx_mdio_cmd(struct mii_bus *bus, int phy_id, int location,
> @@ -985,21 +995,6 @@ static void eth_set_mcast_list(struct net_device *dev)
>   }
>   
>   
> -static int eth_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
> -{
> -	if (!netif_running(dev))
> -		return -EINVAL;
> -
> -	if (cpu_is_ixp46x()) {
> -		if (cmd == SIOCSHWTSTAMP)
> -			return hwtstamp_set(dev, req);
> -		if (cmd == SIOCGHWTSTAMP)
> -			return hwtstamp_get(dev, req);
> -	}
> -
> -	return phy_mii_ioctl(dev->phydev, req, cmd);
> -}
> -
>   /* ethtool support */
>   
>   static void ixp4xx_get_drvinfo(struct net_device *dev,
> @@ -1433,9 +1428,11 @@ static const struct net_device_ops ixp4xx_netdev_ops = {
>   	.ndo_change_mtu = ixp4xx_eth_change_mtu,
>   	.ndo_start_xmit = eth_xmit,
>   	.ndo_set_rx_mode = eth_set_mcast_list,
> -	.ndo_eth_ioctl = eth_ioctl,
> +	.ndo_eth_ioctl = phy_do_ioctl_running,
>   	.ndo_set_mac_address = eth_mac_addr,
>   	.ndo_validate_addr = eth_validate_addr,
> +	.ndo_hwtstamp_get = ixp4xx_hwtstamp_get,
> +	.ndo_hwtstamp_set = ixp4xx_hwtstamp_set,
>   };
>   
>   static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev)

Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Vladimir Oltean]

On Thu, May 08, 2025 at 11:45:48PM +0100, Vadim Fedorenko wrote:
> > The remainder of eth_ioctl() is exactly equivalent to
> > phy_do_ioctl_running(), so use that.
> 
> One interesting fact is that phy_do_ioctl_running() will return -ENODEV
> in case of !netif_running(netdev) while previous code would return
> -EINVAL. Probably it's ok, but may be it's better to have consistent
> error path for both options.
> 
> Otherwise LGTM,
> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Thanks for the review. Indeed, I hadn't noticed the -EINVAL vs -ENODEV
difference.

Are you suggesting that I first create a patch which replaces -EINVAL
with -ENODEV in eth_ioctl(), so that ixp4xx_hwtstamp_get/set() is
consistent with phy_do_ioctl_running() in returning -ENODEV?

Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Vadim Fedorenko]

On 12/05/2025 13:06, Vladimir Oltean wrote:
> On Thu, May 08, 2025 at 11:45:48PM +0100, Vadim Fedorenko wrote:
>>> The remainder of eth_ioctl() is exactly equivalent to
>>> phy_do_ioctl_running(), so use that.
>>
>> One interesting fact is that phy_do_ioctl_running() will return -ENODEV
>> in case of !netif_running(netdev) while previous code would return
>> -EINVAL. Probably it's ok, but may be it's better to have consistent
>> error path for both options.
>>
>> Otherwise LGTM,
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> 
> Thanks for the review. Indeed, I hadn't noticed the -EINVAL vs -ENODEV
> difference.
> 
> Are you suggesting that I first create a patch which replaces -EINVAL
> with -ENODEV in eth_ioctl(), so that ixp4xx_hwtstamp_get/set() is
> consistent with phy_do_ioctl_running() in returning -ENODEV?

The patch to net to make things consistent would be great, but no strong
opinion as there were no complains I believe.

Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[Linus Walleij]

On Thu, May 8, 2025 at 11:12 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> New timestamping API was introduced in commit 66f7223039c0 ("net: add
> NDOs for configuring hardware timestamping") from kernel v6.6. It is
> time to convert the intel ixp4xx ethernet driver to the new API, so that
> the ndo_eth_ioctl() path can be removed completely.
>
> hwtstamp_get() and hwtstamp_set() are only called if netif_running()
> when the code path is engaged through the legacy ioctl. As I don't
> want to make an unnecessary functional change which I can't test,
> preserve that restriction when going through the new operations.
>
> When cpu_is_ixp46x() is false, the execution of SIOCGHWTSTAMP and
> SIOCSHWTSTAMP falls through to phy_mii_ioctl(), which may process it in
> case of a timestamping PHY, or may return -EOPNOTSUPP. In the new API,
> the core handles timestamping PHYs directly and does not call the netdev
> driver, so just return -EOPNOTSUPP directly for equivalent logic.
>
> A gratuitous change I chose to do anyway is prefixing hwtstamp_get() and
> hwtstamp_set() with the driver name, ipx4xx. This reflects modern coding
> sensibilities, and we are touching the involved lines anyway.
>
> The remainder of eth_ioctl() is exactly equivalent to
> phy_do_ioctl_running(), so use that.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Overall looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  9 May 2025 00:10:42 +0300 you wrote:
> New timestamping API was introduced in commit 66f7223039c0 ("net: add
> NDOs for configuring hardware timestamping") from kernel v6.6. It is
> time to convert the intel ixp4xx ethernet driver to the new API, so that
> the ndo_eth_ioctl() path can be removed completely.
> 
> hwtstamp_get() and hwtstamp_set() are only called if netif_running()
> when the code path is engaged through the legacy ioctl. As I don't
> want to make an unnecessary functional change which I can't test,
> preserve that restriction when going through the new operations.
> 
> [...]

Here is the summary with links:
  - [net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()
    https://git.kernel.org/netdev/net-next/c/c14e1ecefd9e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html