Re: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Russell King (Oracle)]

On Sat, Mar 04, 2023 at 04:43:47PM +0100, Andrew Lunn wrote:
> On Fri, Mar 03, 2023 at 05:42:40PM +0100, Köry Maincent wrote:
> > From: Richard Cochran <richardcochran@xxxxxxxxx>
> > 
> > Make the sysfs knob writable, and add checks in the ioctl and time
> > stamping paths to respect the currently selected time stamping layer.
> 
> Although it probably works, i think the ioctl code is ugly.
> 
> I think it would be better to pull the IOCTL code into the PTP object
> interface. Add an ioctl member to struct ptp_clock_info. The PTP core
> can then directly call into the PTP object.

Putting it into ptp_clock_info makes little sense to me, because this
is for the PHC, which is exposed via /dev/ptp*, and that's what the
various methods in that structure are for

The timestamping part is via the netdev, which is a separate entity,
and its that entity which is responsible for identifying which PHC it
is connected to (normally by filling in the phc_index field of
ethtool_ts_info.)

Think of is as:

  netdev ---- timestamping ---- PHC

since we can have:

  netdev1 ---- timestamping \
  netdev2 ---- timestamping -*--- PHC
  netdev3 ---- timestamping /

Since the ioctl is to do with requesting what we want the timestamping
layer to be doing with packets, putting it in ptp_clock_info makes
very little sense.

> You now have a rather odd semantic that calling the .ndo_eth_ioctl
> means operate on the MAC PTP. If you look at net_device_ops, i don't
> think any of the other members have this semantic. They all look at
> the netdev as a whole, and ask the netdev to do something, without
> caring what level it operates at. So a PTP ioctl should operate on
> 'the' PTP of the netdev, whichever that might be, MAC or PHY.

Well, what we have today is:

int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
{
...
        if (phy_has_tsinfo(phydev))
                return phy_ts_info(phydev, info);
        if (ops->get_ts_info)
                return ops->get_ts_info(dev, info);
}

So, one can argue that we already have this "odd" semantic, in that
calling get_ts_info() means to operate on the MAC PTP implementation.
Making the ioctl also do that merely brings it into line with this
existing code!

If we want in general for the netdev to always be called, then we need
to remove the above, but then we need to go through all the networking
drivers working out which need to provide a get_ts_info() and forward
that to phylib. Maybe that's a good thing in the longer run though?

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