Re: [PATCH v8 net-next 06/12] net: fec: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Jul 17, 2023 at 06:27:03PM +0300, Vladimir Oltean wrote:
> -static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
> -{
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct phy_device *phydev = ndev->phydev;
> -
> -	if (!netif_running(ndev))
> -		return -EINVAL;
> -
> -	if (!phydev)
> -		return -ENODEV;
> -
... process hwtstamp calls

So if the network device is not running, ioctl() returns -EINVAL. From
what I can see in fec_enet_mii_probe() called from fec_enet_open(), we
guarantee that phydev will not be NULL once the first open has
succeeded, so I don't think the second if() statement has any effect.

> +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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!