Messages in this thread | | | Date | Tue, 1 Aug 2023 16:12:54 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH v8 net-next 06/12] net: fec: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set() |
| |
On Tue, Jul 18, 2023 at 03:20:13PM +0100, Russell King (Oracle) wrote: > > +static int fec_hwtstamp_get(struct net_device *ndev, > > + struct kernel_hwtstamp_config *config) > > +{ > > + struct fec_enet_private *fep = netdev_priv(ndev); > > + struct phy_device *phydev = ndev->phydev; > > + > > + if (phy_has_hwtstamp(phydev)) > > + return phy_mii_ioctl(phydev, config->ifr, SIOCGHWTSTAMP); > > + > > + if (!fep->bufdesc_ex) > > + return -EOPNOTSUPP; > > If the interface hasn't been brought up at least once, then phydev > here will be NULL, and we'll drop through to this test. If the FEC > doesn't support extended buffer descriptors, userspace will see > -EOPNOTSUPP rather than -EINVAL. This could be misleading to userspace. > > Does this need something like: > > if (!netif_running(ndev)) > return -EINVAL; > > before, or maybe just after phy_has_hwtstamp() to give equivalent > behaviour? > > > +static int fec_hwtstamp_set(struct net_device *ndev, > > + struct kernel_hwtstamp_config *config, > > + struct netlink_ext_ack *extack) > > +{ > > + struct fec_enet_private *fep = netdev_priv(ndev); > > + struct phy_device *phydev = ndev->phydev; > > + > > + if (phy_has_hwtstamp(phydev)) { > > + fec_ptp_disable_hwts(ndev); > > + > > + return phy_mii_ioctl(phydev, config->ifr, SIOCSHWTSTAMP); > > + } > > + > > + if (!fep->bufdesc_ex) > > + return -EOPNOTSUPP; > > Same comment here.
I will add back the netif_running() check to continue to not permit the operation to go through, as before.
| |