Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: "Köry Maincent" <kory.maincent@bootlin.com>
Cc: netdev@vger.kernel.org, kuba@kernel.org, glipus@gmail.com,
	maxime.chevallier@bootlin.com, vadim.fedorenko@linux.dev,
	richardcochran@gmail.com, gerhard@engleder-embedded.com,
	thomas.petazzoni@bootlin.com, krzysztof.kozlowski+dt@linaro.org,
	robh+dt@kernel.org, linux@armlinux.org.uk
Subject: Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.
Date: Thu, 11 May 2023 16:48:07 +0300	[thread overview]
Message-ID: <20230511134807.v4u3ofn6jvgphqco@skbuf> (raw)
In-Reply-To: <20230502130525.02ade4a8@kmaincent-XPS-13-7390>

On Tue, May 02, 2023 at 01:05:25PM +0200, Köry Maincent wrote:
> On Sat, 29 Apr 2023 20:58:07 +0300
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> Thanks for the review and sorry for the coding style issues, I forgot to run the
> checkpatch script.
> 
> > >  
> > >  #if IS_ENABLED(CONFIG_MACSEC)
> > > @@ -2879,6 +2885,7 @@ enum netdev_cmd {
> > >  	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
> > >  	NETDEV_XDP_FEAT_CHANGE,
> > >  	NETDEV_PRE_CHANGE_HWTSTAMP,
> > > +	NETDEV_CHANGE_HWTSTAMP,  
> > 
> > Don't create new netdev notifiers with no users. Also,
> > NETDEV_PRE_CHANGE_HWTSTAMP has disappered.
> 
> Ok, right you move it on to dsa stub. What do you think of our case, should we
> continue with netdev notifier? 

I don't know.

AFAIU, the plan forward with this patch set is that, if the active
timestamping layer is the PHY, phy_mii_ioctl() gets called and the MAC
driver does not get notified in any way of that. That is an issue
because if it's a switch, it will want to trap PTP even if it doesn't
timestamp it, and with this proposal it doesn't get a chance to do that.

What is your need for this? Do you have this scenario? If not, just drop
this part from the patch.

Jakub, you said "nope" to netdev notifiers, what would you suggest here
instead? ndo_change_ptp_traps()?

> Just some thought:
> Maybe we could create a PHC_ID 0xXY where X is the layer and Y the PHCs number
> in this layer, but what will append if there is two MACs consecutively?
> Also in case of several MACs or PHYs in serial
> netdev->selected_timestamping_layer is inappropriate. 
> 
> Maybe using it like 0xABC where A is the MAC number, B the PHY number and C
> the PHC number in the device.
> For example if we select the second MAC and its third PHC, PHC_ID will be
> 0x203. Or if we select the second PHC of the PHY it will be 0x012.

I think the kernel has (or can have) enough information to deduce the
timestamping layer from just the PHC number, everything else is redundant
information (also, what's "MAC number"? what is "PHY number"? how can
user space correlate them to anything?).

If you want to design an UAPI where individual PHCs present in the
timestamping list of a netdev can individually have timestamping turned
on or off (for a future where a socket can communicate hardware
timestamps from multiple PHCs to user space in an identifiable way),
then that is fine by me and maybe even a good idea. Just be aware that
the current kernel will have to deny more than a single active
timestamping PHC for now, because of the lack of hwtstamp identification
in the SO_TIMESTAMPING API.