On Tue, Jul 18, 2023 at 03:20:13PM +0100, Russell King (Oracle) wrote:

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel