netdev - Re: Regression: supported_interfaces filling enforcement

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230710153827.jhdbl5xh3stslz3u@skbuf>
Date: Mon, 10 Jul 2023 18:38:27 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Sergei Antonov <saproj@...il.com>
Cc: netdev@...r.kernel.org, rmk+kernel@...linux.org.uk
Subject: Re: Regression: supported_interfaces filling enforcement

On Mon, Jul 10, 2023 at 05:35:35PM +0300, Sergei Antonov wrote:
> On Mon, 10 Jul 2023 at 15:35, Vladimir Oltean <olteanv@...il.com> wrote:
> > > +static void mv88e6060_get_caps(struct dsa_switch *ds, int port,
> > > +                              struct phylink_config *config)
> > > +{
> > > +       __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces);
> > > +       __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
> >
> > This is enough to fix phylink generic validation on the front-facing
> > ports with internal PHYs. But it is possible (and encouraged) to use
> > phylink on the CPU port too (rev-mii, rev-rmii); currently that's not
> > enforced for mv88e6060 because it's in the dsa_switches_apply_workarounds[]
> > array.
> >
> > Could you please modify your device tree to add a fixed-link and
> > phy-mode property on your CPU port so that phylink does get used
> 
> I already have fixed-link and phy-mode on CPU port. See below.

And with PHY_INTERFACE_MODE_MII missing from supported_interfaces, how
did phylink_generic_validate() not fail for the CPU port?

... goes to check dsa_port_phylink_validate() ...

ah, the call to phylink_generic_validate() is skipped because you're not
setting config->mac_capabilities in your mini implementation.
Please also set that - take other drivers as a model. But for
config->legacy_pre_march2020, I guess you don't have to explicitly set
that to false, because dsa_port_phylink_create() doesn't set it to true,
either.

> > , and
> > populate supported_interfaces and mac_capabilities properly on the MII
> > ports (4 and 5) as well (so that it doesn't fail validation)?
> 
> By setting bits in .phylink_get_caps function?

Yes, depending on which port it is and how it is configured to be used.

> Should I remove mv88e6060 from dsa_switches_apply_workarounds too?

If the device tree doesn't have fixed-link and phy-mode on the CPU port,
a phylink instance will fail to be created there, and the switch will
fail to probe. The presence in dsa_switches_apply_workarounds[] allows
phylink to be skipped for the CPU port.

I won't oppose to mv88e6060 getting removed from that array, but if
there exist production device trees with the above characteristics,
it would be unwise to break them.

That being said, given the kind of bugs I've seen uncovered in this
driver recently, I'd say it would be ridiculous to play pretend - you're
probably one of its only users. You can probably be a bit aggressive,
remove support for incomplete device trees, see if anyone complains, and
they do, revert the removal.

> &mdio1 {
>         status = "okay";
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         switch@10 {
>                 compatible = "marvell,mv88e6060";
>                 reg = <0x10>;
> 
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         port@0 {
>                                 reg = <0>;
>                                 label = "lan2";
>                         };
> 
>                         port@1 {
>                                 reg = <1>;
>                                 label = "lan3";
>                         };
> 
>                         port@2 {
>                                 reg = <2>;
>                                 label = "lan1";
>                         };
> 
>                         port@5 {
>                                 reg = <5>;
>                                 label = "cpu";
>                                 ethernet = <&mac1>;
>                                 phy-mode = "mii";

I think we have phy-mode = "rev-mii" for "MAC acting as MII PHY", rather
than "mii" which is "MAC acting as MII MAC".

> 
>                                 fixed-link {
>                                         speed = <100>;
>                                         full-duplex;
>                                 };
>                         };
>                 };
>         };
> };