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