Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 15:20:13 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Maxim Georgiev <glipus@gmail.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        =?iso-8859-1?Q?K=F6ry?= Maincent <kory.maincent@bootlin.com>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Richard Cochran <richardcochran@gmail.com>,
        Vadim Fedorenko <vadim.fedorenko@linux.dev>,
        Gerhard Engleder <gerhard@engleder-embedded.com>,
        Hangbin Liu <liuhangbin@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Jacob Keller <jacob.e.keller@intel.com>,
        Jay Vosburgh <j.vosburgh@gmail.com>,
        Andy Gospodarek <andy@greyhouse.net>,
        Wei Fang <wei.fang@nxp.com>,
        Shenwei Wang <shenwei.wang@nxp.com>,
        Clark Wang <xiaoning.wang@nxp.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        UNGLinuxDriver@microchip.com,
        Lars Povlsen <lars.povlsen@microchip.com>,
        Steen Hegelund <Steen.Hegelund@microchip.com>,
        Daniel Machon <daniel.machon@microchip.com>,
        Simon Horman <simon.horman@corigine.com>,
        Casper Andersson <casper.casan@gmail.com>,
        Sergey Organov <sorganov@gmail.com>,
        Michal Kubecek <mkubecek@suse.cz>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 net-next 06/12] net: fec: convert to
 ndo_hwtstamp_get() and ndo_hwtstamp_set()
Message-ID: <ZLafnWuAlytSN7B+@shell.armlinux.org.uk>
References: <20230717152709.574773-1-vladimir.oltean@nxp.com>
 <20230717152709.574773-7-vladimir.oltean@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230717152709.574773-7-vladimir.oltean@nxp.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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!