| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dfb57a6c-8db7-4ab5-9d51-eec40cf8662e@linux.dev>
Date: Thu, 8 May 2025 23:45:48 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc: Köry Maincent <kory.maincent@...tlin.com>,
Linus Walleij <linusw@...nel.org>, Imre Kaloz <kaloz@...nwrt.org>,
linux-arm-kernel@...ts.infradead.org, Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>, Richard Cochran <richardcochran@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: ixp4xx_eth: convert to ndo_hwtstamp_get()
and ndo_hwtstamp_set()
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@....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, ®s->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,
> ®s->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@...ux.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)
Powered by blists - more mailing lists