Re: [PATCH net] net: lan966x: fix kernel oops on ioctl when I/F is down

[Michael Walle]

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