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: Jakub Kicinski <kuba@kernel.org>
Cc: "Köry Maincent" <kory.maincent@bootlin.com>,
	netdev@vger.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 18:56:40 +0300	[thread overview]
Message-ID: <20230511155640.3nqanqpczz5xwxae@skbuf> (raw)
In-Reply-To: <20230511083620.15203ebe@kernel.org>

On Thu, May 11, 2023 at 08:36:20AM -0700, Jakub Kicinski wrote:
> On Thu, 11 May 2023 16:48:07 +0300 Vladimir Oltean wrote:
> > > 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()?
> 
> More importantly "monolithic" drivers have DMA/MAC/PHY all under 
> the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
> is not going to work.
> 
> We need a more complex calling convention for the NDO.

It's the first time I become aware of the issue of PHY timestamping in
monolithic drivers that don't use phylib, and it's actually a very good
point. I guess that input gives a clearer set of constraints for Köry to
design an API where the selected timestamping layer is maybe passed to
ndo_hwtstamp_set() and MAC drivers are obliged to look at it.

OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
code path was that phylib PHY drivers need to have the explicit blessing
from the MAC driver in order to enable timestamping. This patch set
attempts to circumvent that, and you're basically saying that it shouldn't.