[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b4ebfd3770ffa5ad1233d2b5e79499ee@walle.cc>
Date: Fri, 10 Mar 2023 13:15:16 +0100
From: Michael Walle <michael@...le.cc>
To: Vladimir Oltean <vladimir.oltean@....com>,
Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: Köry Maincent <kory.maincent@...tlin.com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-omap@...r.kernel.org,
Maxime Chevallier <maxime.chevallier@...tlin.com>,
Richard Cochran <richardcochran@...il.com>,
thomas.petazzoni@...tlin.com, Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com, Minghao Chi <chi.minghao@....com.cn>,
Jie Wang <wangjie125@...wei.com>,
Oleksij Rempel <linux@...pel-privat.de>,
Sean Anderson <sean.anderson@...o.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>,
Marco Bonelli <marco@...eim.net>
Subject: Re: [PATCH v3 3/5] net: Let the active time stamping layer be
selectable.
[+ Horatiu]
Am 2023-03-10 12:35, schrieb Vladimir Oltean:
> On Fri, Mar 10, 2023 at 11:48:52AM +0100, Köry Maincent wrote:
>> > From previous discussions, I believe that a device tree property was
>> > added in order to prevent perceived performance regressions when
>> > timestamping support is added to a PHY driver, correct?
>>
>> Yes, i.e. to select the default and better timestamp on a board.
>
> Is there a way to unambiguously determine the "better" timestamping on
> a board?
>
> Is it plausible that over time, when PTP timestamping matures and,
> for example, MDIO devices get support for PTP_SYS_OFFSET_EXTENDED
> (an attempt was here: https://lkml.org/lkml/2019/8/16/638), the
> relationship between PTP clock qualities changes, and so does the
> preference change?
>
>> > I have a dumb question: if updating the device trees is needed in order
>> > to prevent these behavior changes, then how is the regression problem
>> > addressed for those device trees which don't contain this new property
>> > (all device trees)?
>>
>> On that case there is not really solution,
>
> If it's not really a solution, then doesn't this fail at its primary
> purpose of preventing regressions?
>
>> but be aware that CONFIG_PHY_TIMESTAMPING need to be activated to
>> allow timestamping on the PHY. Currently in mainline only few (3)
>> defconfig have it enabled so it is really not spread,
>
> Do distribution kernels use the defconfigs from the kernel, or do they
> just enable as many options that sound good as possible?
>
>> maybe I could add more documentation to prevent further regression
>> issue when adding support of timestamp to a PHY driver.
>
> My opinion is that either the problem was not correctly identified,
> or the proposed solution does not address that problem.
>
> What I believe is the problem is that adding support for PHY
> timestamping
> to a PHY driver will cause a behavior change for existing systems which
> are deployed with that PHY.
>
> If I had a multi-port NIC where all ports share the same PHC, I would
> want to create a boundary clock with it. I can do that just fine when
> using MAC timestamping. But assume someone adds support for PHY
> timestamping and the kernel switches to using PHY timestamps by
> default.
> Now I need to keep in sync the PHCs of the PHYs, something which was
> implicit before (all ports shared the same PHC). I have done nothing
> incorrectly, yet my deployment doesn't work anymore. This is just an
> example. It doesn't sound like a good idea in general for new features
> to cause a behavior change by default.
>
> Having identified that as the problem, I guess the solution should be
> to stop doing that (and even though a PHY driver supports timestamping,
> keep using the MAC timestamping by default).
>
> There is a slight inconvenience caused by the fact that there are
> already PHY drivers using PHY timestamping, and those may have been
> introduced into deployments with PHY timestamping. We cannot change the
> default behavior for those either. There are 5 such PHY drivers today
> (I've grepped for mii_timestamper in drivers/net/phy).
>
> I would suggest that the kernel implements a short whitelist of 5
> entries containing PHY driver names, which are compared against
> netdev->phydev->drv->name (with the appropriate NULL pointer checks).
> Matches will default to PHY timestamping. Otherwise, the new default
> will be to keep the behavior as if PHY timestamping doesn't exist
> (MAC still provides the timestamps), and the user needs to select the
> PHY as the timestamping source explicitly.
>
> Thoughts?
While I agree in principle (I have suggested to make MAC timestamping
the default before), I see a problem with the recent LAN8814 PHY
timestamping support, which will likely be released with 6.3. That
would now switch the timestamping to PHY timestamping for our board
(arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt-8g.dts). I could
argue that is a regression for our board iff NETWORK_PHY_TIMESTAMPING
is enabled. Honestly, I don't know how to proceed here and haven't
tried to replicate the regression due to limited time. Assuming,
that I can show it is a regression, what would be the solution then,
reverting the commit? Horatiu, any ideas?
I digress from the original problem a bit. But if there would be such
a whitelist, I'd propose that it won't contain the lan8814 driver.
Other than that, I guess I have to put some time into testing
before it's too late.
-michael
Powered by blists - more mailing lists