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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Sat, 26 Mar 2022 14:59:53 +0100
From:   Michael Walle <michael@walle.cc>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Horatiu Vultur <horatiu.vultur@microchip.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, UNGLinuxDriver@microchip.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is
 down
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <1ee5aec079e3eefff8475017ff1044bf@walle.cc>
Precedence: bulk

Am 2022-03-26 03:17, schrieb Andrew Lunn:
> On Sat, Mar 26, 2022 at 01:02:51AM +0100, Michael Walle wrote:
>> A SIOCGMIIPHY ioctl will cause a kernel oops when the interface is 
>> down.
>> Fix it by checking the state and if it's no running, return an error.
> 
> s/no/not/
> 
> I don't think it is just SIOCGMIIPHY. phy_has_hwtstamp(dev->phydev) is
> probably also an issue. The phy is connected in open, and disconnected
> in stop. So dev->phydev is not valid outside of that time.

phy_has_hwtstamp() handles NULL gracefully. And I guess the MAC 
timestamp
handling is working if there is no phydev. Not sure if the interface
has to be up though.

> But i'm also not sure it is guaranteed to be valid while the interface
> is up. The driver uses phylink, so there could be an SFP attached to a
> port, in which case, dev->phydev will not be set.

I wonder if we should use phylink_mii_ioctl() here. Maybe as a seperate
patch for the net-next if its open again?

> So rather than testing of running, it would be better to test if the
> phydev is NULL or not.

What about the following:

static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
			     int cmd)
{
	struct lan966x_port *port = netdev_priv(dev);

	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
		switch (cmd) {
		case SIOCSHWTSTAMP:
			return lan966x_ptp_hwtstamp_set(port, ifr);
		case SIOCGHWTSTAMP:
			return lan966x_ptp_hwtstamp_get(port, ifr);
		}
	}

	if (!dev->phydev)
		return -ENODEV;

	return phy_mii_ioctl(dev->phydev, ifr, cmd);
}

-michael