netdev - Re: [PATCH net-next] net: phylink: require supported

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]

Date: Wed, 22 Nov 2023 14:12:44 +1000
From: Greg Ungerer <gerg@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
 Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phylink: require supported_interfaces to be
 filled


On 22/11/23 00:29, Andrew Lunn wrote:
>> The 6350 looks to be similar to the 6352 in many respects, though it lacks
>> a SERDES interface, but it otherwise mostly seems compatible.
> 
> Not having the SERDES is important. Without that SERDES, the bit about
> Port 4 in mv88e6352_phylink_get_caps() is
> incorrect. mv88e61852_phylink_get_caps() looks reasonable for this
> hardware.
              ^^^^^^^^^^
The problem with mv88e6185_phylink_get_caps() is the cmode check fails
for me. For my 6350 hardware chip->ports[port].cmode is "9", so set to
MV88E6XXX_PORT_STS_CMODE_1000BASEX. But that is not part of the defines
used in mv88e6185_phy_interface_modes[].

Doesn't it need to be checking in mv88e6xxx_phy_interface_modes[]
for the cmode?

I see another similar function, mv88e6250_phylink_get_caps().
But that is only 10/100 capable.

So I am thinking that something like this actually makes more sense now:

--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -577,6 +577,18 @@ static void mv88e6250_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
         config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100;
  }
  
+static void mv88e6350_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
+                                      struct phylink_config *config)
+{
+       unsigned long *supported = config->supported_interfaces;
+
+       /* Translate the default cmode */
+       mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported);
+
+       config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 |
+                                  MAC_1000FD;
+}
+
  static int mv88e6352_get_port4_serdes_cmode(struct mv88e6xxx_chip *chip)
  {
         u16 reg, val;
@@ -5069,7 +5082,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
         .vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
         .stu_getnext = mv88e6352_g1_stu_getnext,
         .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
-       .phylink_get_caps = mv88e6185_phylink_get_caps,
+       .phylink_get_caps = mv88e6350_phylink_get_caps,
  };
  
  static const struct mv88e6xxx_ops mv88e6351_ops = {
@@ -5117,7 +5130,7 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
         .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
         .avb_ops = &mv88e6352_avb_ops,
         .ptp_ops = &mv88e6352_ptp_ops,
-       .phylink_get_caps = mv88e6185_phylink_get_caps,
+       .phylink_get_caps = mv88e6350_phylink_get_caps,
  };
  
  static const struct mv88e6xxx_ops mv88e6352_ops = {


>> Using the 6352
>> phylink_get_caps function instead of the 6185 one fixes this:
>>
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
>>          .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
>>          .stu_getnext = mv88e6352_g1_stu_getnext,
>>          .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
>> -       .phylink_get_caps = mv88e6185_phylink_get_caps,
>> +       .phylink_get_caps = mv88e6352_phylink_get_caps,
>>   };
>>
>>   static const struct mv88e6xxx_ops mv88e6351_ops = {
>>
>>
>> The story doesn't quite end here though. With this fix in place support
>> for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
>> mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
>> on boot up:
> 
> PCS is approximately another name of a SERDES. Since there is no
> SERDES, you don't don't want any of the pcs ops filled in.
> 
> Russell knows this code much better than i do. Let see what he says.

Ok, that makes sense. Russell had a suggestion for this one and I will
follow up with that.

Thanks
Greg