Re: [PATCH RFC net-next v2 0/5] net: dsa: always use phylink

From: Florian Fainelli <f.fainelli@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>
Cc: "Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Alvin Šipraga" <alsi@bang-olufsen.dk>,
	"Claudiu Manoil" <claudiu.manoil@nxp.com>,
	"David S. Miller" <davem@davemloft.net>,
	"DENG Qingfang" <dqfext@gmail.com>,
	"Eric Dumazet" <edumazet@google.com>,
	"George McCollister" <george.mccollister@gmail.com>,
	"Hauke Mehrtens" <hauke@hauke-m.de>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Kurt Kanzenbach" <kurt@linutronix.de>,
	"Landen Chao" <Landen.Chao@mediatek.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	netdev@vger.kernel.org, "Paolo Abeni" <pabeni@redhat.com>,
	"Sean Wang" <sean.wang@mediatek.com>,
	UNGLinuxDriver@microchip.com,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	"Vladimir Oltean" <olteanv@gmail.com>,
	"Woojung Huh" <woojung.huh@microchip.com>
Subject: Re: [PATCH RFC net-next v2 0/5] net: dsa: always use phylink
Date: Tue, 5 Jul 2022 09:42:33 -0700	[thread overview]
Message-ID: <7fe6b661-06b9-96dd-e064-1db23a9eaae7@gmail.com> (raw)
In-Reply-To: <YsQIjC7UpcGWJovx@shell.armlinux.org.uk>

On 7/5/22 02:46, Russell King (Oracle) wrote:
> A new revision of the series which incorporates changes that Marek
> suggested. Specifically, the changes are:
> 
> 1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the
>     default interface rather than re-using port_max_speed_mode()
> 
> 2. Patch 4 - if no default interface is provided, use the supported
>     interface mask to search for the first interface that gives the
>     fastest speed.
> 
> 3. Patch 5 - now also removes the port_max_speed_mode() method

This was tested with bcm_sf2.c and b53_srab.b and did not cause 
regressions, however we do have a 'fixed-link' property for the CPU port 
(always have had one), so there was no regression expected.

See answers to your RFC v1 below.

> 
>   drivers/net/dsa/b53/b53_common.c       |   3 +-
>   drivers/net/dsa/bcm_sf2.c              |   3 +-
>   drivers/net/dsa/hirschmann/hellcreek.c |   3 +-
>   drivers/net/dsa/lantiq_gswip.c         |   6 +-
>   drivers/net/dsa/microchip/ksz_common.c |   3 +-
>   drivers/net/dsa/mt7530.c               |   3 +-
>   drivers/net/dsa/mv88e6xxx/chip.c       | 136 +++++++++++++++---------------
>   drivers/net/dsa/mv88e6xxx/chip.h       |   6 +-
>   drivers/net/dsa/mv88e6xxx/port.c       |  32 -------
>   drivers/net/dsa/mv88e6xxx/port.h       |   5 --
>   drivers/net/dsa/ocelot/felix.c         |   3 +-
>   drivers/net/dsa/qca/ar9331.c           |   3 +-
>   drivers/net/dsa/qca8k.c                |   3 +-
>   drivers/net/dsa/realtek/rtl8365mb.c    |   3 +-
>   drivers/net/dsa/sja1105/sja1105_main.c |   3 +-
>   drivers/net/dsa/xrs700x/xrs700x.c      |   3 +-
>   drivers/net/phy/phylink.c              | 150 ++++++++++++++++++++++++++++++---
>   include/linux/phylink.h                |   5 ++
>   include/net/dsa.h                      |   3 +-
>   net/dsa/port.c                         |  47 +++++++----
>   20 files changed, 270 insertions(+), 153 deletions(-)
> 
> On Wed, Jun 29, 2022 at 01:49:57PM +0100, Russell King (Oracle) wrote:
>> Mostly the same as the previous RFC, except:
>>
>> 1) incldues the phylink_validate_mask_caps() function
>> 2) has Marek's idea of searching the supported_interfaces bitmap for the
>>     fastest interface we can use
>> 3) includes a final patch to add a print which will be useful to hear
>>     from people testing it.
>>
>> Some of the questions from the original RFC remain though, so I've
>> included that text below. I'm guessing as they remain unanswered that
>> no one has any opinions on them?
>>
>>   drivers/net/dsa/b53/b53_common.c       |   3 +-
>>   drivers/net/dsa/bcm_sf2.c              |   3 +-
>>   drivers/net/dsa/hirschmann/hellcreek.c |   3 +-
>>   drivers/net/dsa/lantiq_gswip.c         |   6 +-
>>   drivers/net/dsa/microchip/ksz_common.c |   3 +-
>>   drivers/net/dsa/mt7530.c               |   3 +-
>>   drivers/net/dsa/mv88e6xxx/chip.c       |  53 ++++--------
>>   drivers/net/dsa/ocelot/felix.c         |   3 +-
>>   drivers/net/dsa/qca/ar9331.c           |   3 +-
>>   drivers/net/dsa/qca8k.c                |   3 +-
>>   drivers/net/dsa/realtek/rtl8365mb.c    |   3 +-
>>   drivers/net/dsa/sja1105/sja1105_main.c |   3 +-
>>   drivers/net/dsa/xrs700x/xrs700x.c      |   3 +-
>>   drivers/net/phy/phylink.c              | 148 ++++++++++++++++++++++++++++++---
>>   include/linux/phylink.h                |   5 ++
>>   include/net/dsa.h                      |   3 +-
>>   net/dsa/port.c                         |  47 +++++++----
>>   17 files changed, 215 insertions(+), 80 deletions(-)
>>
>> On Fri, Jun 24, 2022 at 12:41:26PM +0100, Russell King (Oracle) wrote:
>>> Hi,
>>>
>>> Currently, the core DSA code conditionally uses phylink for CPU and DSA
>>> ports depending on whether the firmware specifies a fixed-link or a PHY.
>>> If either of these are specified, then phylink is used for these ports,
>>> otherwise phylink is not, and we rely on the DSA drivers to "do the
>>> right thing". However, this detail is not mentioned in the DT binding,
>>> but Andrew has said that this behaviour has always something that DSA
>>> wants.
>>>
>>> mv88e6xxx has had support for this for a long time with its "SPEED_MAX"
>>> thing, which I recently reworked to make use of the mac_capabilities in
>>> preparation to solving this more fully.
>>>
>>> This series is an experiment to solve this properly, and it does this
>>> in two steps.
>>>
>>> The first step consists of the first two patches. Phylink needs to
>>> know the PHY interface mode that is being used so it can (a) pass the
>>> right mode into the MAC/PCS etc and (b) know the properties of the
>>> link and therefore which speeds can be supported across it.
>>>
>>> In order to achieve this, the DSA phylink_get_caps() method has an
>>> extra argument added to it so that DSA drivers can report the
>>> interface mode that they will be using for this port back to the core
>>> DSA code, thereby allowing phylink to be initialised with the correct
>>> interface mode.
>>>
>>> Note that this can only be used for CPU and DSA ports as "user" ports
>>> need a different behaviour - they rely on getting the interface mode
>>> from phylib, which will only happen if phylink is initialised with
>>> PHY_INTERFACE_MODE_NA. Unfortunately, changing this behaviour is likely
>>> to cause widespread regressions.
>>>
>>> Obvious questions:
>>> 1. Should phylink_get_caps() be augmented in this way, or should it be
>>>     a separate method?
>>>
>>> 2. DSA has traditionally used "interface mode for the maximum supported
>>>     speed on this port" where the interface mode is programmable (via
>>>     its internal port_max_speed_mode() method) but this is only present
>>>     for a few of the sub-drivers. Is reporting the current interface
>>>     mode correct where this method is not implemented?
>>>
>>> The second step is to introduce a function that allows phylink to be
>>> reconfigured after creation time to operate at max-speed fixed-link
>>> mode for the PHY interface mode, also using the MAC capabilities to
>>> determine the speed and duplex mode we should be using.
>>>
>>> Obvious questions:
>>> 1. Should we be allowing half-duplex for this?

Except for testing, not sure I do see a point as it should not be a 
configuration being used at all?

>>> 2. If we do allow half-duplex, should we prefer fastest speed over
>>>     duplex setting, or should we prefer fastest full-duplex speed
>>>     over any half-duplex?

I would opt for fastest speed over duplex setting.

>>> 3. How do we sanely switch DSA from its current behaviour to always
>>>     using phylink for these ports without breakage - this is the
>>>     difficult one, because it's not obvious which drivers have been
>>>     coded to either work around this quirk of the DSA implementation.
>>>     For example, if we start forcing the link down before calling
>>>     dsa_port_phylink_create(), and we then fail to set max-fixed-link,
>>>     then the CPU/DSA port is going to fail, and we're going to have
>>>     lots of regressions.

Good question, we already have a legacy_pre_march2020 behavior for a 
piece of infrastructure code that is not so old, I doubt that we would 
want to add more of that type of quirk.
-- 
Florian