[PATCH net-next] net: phylink: require supported_interfaces to be filled

[PATCH net-next] net: phylink: require supported_interfaces to be filled

[Russell King (Oracle)]

We have been requiring the supported_interfaces bitmap to be filled in
by MAC drivers that have a mac_select_pcs() method. Now that all MAC
drivers fill in the supported_interfaces bitmap, it is time to enforce
this. We have already required supported_interfaces to be set in order
for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
use phy_interface_t bitmaps for optical modules").

Refuse phylink creation if supported_interfaces is empty, and remove
code to deal with cases where this mask is empty.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
I believe what I've said above is indeed the case, but there is always
the chance that something has been missed and this will cause breakage.
I would post as RFC and ask for testing, but in my experience that is
a complete waste of time as it doesn't result in any testing feedback.
So, it's probably better to get it merged into net-next and then wait
for any reports of breakage.

 drivers/net/phy/phylink.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a4dd5197355a..093b7b6e0263 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -712,14 +712,11 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
 {
 	const unsigned long *interfaces = pl->config->supported_interfaces;
 
-	if (!phy_interface_empty(interfaces)) {
-		if (state->interface == PHY_INTERFACE_MODE_NA)
-			return phylink_validate_mask(pl, supported, state,
-						     interfaces);
+	if (state->interface == PHY_INTERFACE_MODE_NA)
+		return phylink_validate_mask(pl, supported, state, interfaces);
 
-		if (!test_bit(state->interface, interfaces))
-			return -EINVAL;
-	}
+	if (!test_bit(state->interface, interfaces))
+		return -EINVAL;
 
 	return phylink_validate_mac_and_pcs(pl, supported, state);
 }
@@ -1513,19 +1510,18 @@ struct phylink *phylink_create(struct phylink_config *config,
 	struct phylink *pl;
 	int ret;
 
-	if (mac_ops->mac_select_pcs &&
-	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
-	      ERR_PTR(-EOPNOTSUPP))
-		using_mac_select_pcs = true;
-
 	/* Validate the supplied configuration */
-	if (using_mac_select_pcs &&
-	    phy_interface_empty(config->supported_interfaces)) {
+	if (phy_interface_empty(config->supported_interfaces)) {
 		dev_err(config->dev,
-			"phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
+			"phylink: error: empty supported_interfaces\n");
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (mac_ops->mac_select_pcs &&
+	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
+	      ERR_PTR(-EOPNOTSUPP))
+		using_mac_select_pcs = true;
+
 	pl = kzalloc(sizeof(*pl), GFP_KERNEL);
 	if (!pl)
 		return ERR_PTR(-ENOMEM);
-- 
2.30.2

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Andrew Lunn]

On Sat, May 20, 2023 at 11:41:42AM +0100, Russell King (Oracle) wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
> 
> Refuse phylink creation if supported_interfaces is empty, and remove
> code to deal with cases where this mask is empty.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

> I believe what I've said above is indeed the case, but there is always
> the chance that something has been missed and this will cause breakage.
> I would post as RFC and ask for testing, but in my experience that is
> a complete waste of time as it doesn't result in any testing feedback.
> So, it's probably better to get it merged into net-next and then wait
> for any reports of breakage.

Agreed.

    Andrew

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 20 May 2023 11:41:42 +0100 you wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
> 
> [...]

Here is the summary with links:
  - [net-next] net: phylink: require supported_interfaces to be filled
    https://git.kernel.org/netdev/net-next/c/de5c9bf40c45

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Greg Ungerer]

Hi Russell,

On 20/5/23 20:41, Russell King (Oracle) wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
> 
> Refuse phylink creation if supported_interfaces is empty, and remove
> code to deal with cases where this mask is empty.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> I believe what I've said above is indeed the case, but there is always
> the chance that something has been missed and this will cause breakage.
> I would post as RFC and ask for testing, but in my experience that is
> a complete waste of time as it doesn't result in any testing feedback.
> So, it's probably better to get it merged into net-next and then wait
> for any reports of breakage.

This commit breaks a platform I have with a Marvell 88e6350 switch.
During boot up it now fails with:

     ...
     mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
     mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
     error creating PHYLINK: -22
     mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
     ...

The 6350 looks to be similar to the 6352 in many respects, though it lacks
a SERDES interface, but it otherwise mostly seems compatible. 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:

     ...
     mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
     8<--- cut here ---
     Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
     [00000000] *pgd=00000000
     Internal error: Oops: 5 [#1] ARM
     Modules linked in:
     CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
     Hardware name: Marvell Armada 370/XP (Device Tree)
     Workqueue: events_unbound deferred_probe_work_func
     PC is at mv88e6xxx_port_setup+0x1c/0x44
     LR is at dsa_port_devlink_setup+0x74/0x154
     pc : [<c057ea24>]    lr : [<c0819598>]    psr: a0000013
     sp : c184fce0  ip : c542b8f4  fp : 00000000
     r10: 00000001  r9 : c542a540  r8 : c542bc00
     r7 : c542b838  r6 : c5244580  r5 : 00000005  r4 : c5244580
     r3 : 00000000  r2 : c542b840  r1 : 00000005  r0 : c1a02040
     ...

I can see that the mv88e6350_ops struct doesn't have an initializer for
pcs_ops. Based on the similarity with the 6352 again I tried using the
mv88e6352_pcs_ops for that:

--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5069,7 +5069,8 @@ 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 = mv88e6352_phylink_get_caps,
+       .pcs_ops = &mv88e6352_pcs_ops,
  };


This gets the 6350 switch back to working again (on current linux 6.7-rc2).
I am not entirely sure if this needs a dedicated phylink_get_caps
and pcs_ops for the 6350, or if it is safe to use the 6352 ones?

Looking at the mv88e6351_ops I am guessing it is going to suffer the
same problems too.

Regards
Greg



>   drivers/net/phy/phylink.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a4dd5197355a..093b7b6e0263 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -712,14 +712,11 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
>   {
>   	const unsigned long *interfaces = pl->config->supported_interfaces;
>   
> -	if (!phy_interface_empty(interfaces)) {
> -		if (state->interface == PHY_INTERFACE_MODE_NA)
> -			return phylink_validate_mask(pl, supported, state,
> -						     interfaces);
> +	if (state->interface == PHY_INTERFACE_MODE_NA)
> +		return phylink_validate_mask(pl, supported, state, interfaces);
>   
> -		if (!test_bit(state->interface, interfaces))
> -			return -EINVAL;
> -	}
> +	if (!test_bit(state->interface, interfaces))
> +		return -EINVAL;
>   
>   	return phylink_validate_mac_and_pcs(pl, supported, state);
>   }
> @@ -1513,19 +1510,18 @@ struct phylink *phylink_create(struct phylink_config *config,
>   	struct phylink *pl;
>   	int ret;
>   
> -	if (mac_ops->mac_select_pcs &&
> -	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> -	      ERR_PTR(-EOPNOTSUPP))
> -		using_mac_select_pcs = true;
> -
>   	/* Validate the supplied configuration */
> -	if (using_mac_select_pcs &&
> -	    phy_interface_empty(config->supported_interfaces)) {
> +	if (phy_interface_empty(config->supported_interfaces)) {
>   		dev_err(config->dev,
> -			"phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
> +			"phylink: error: empty supported_interfaces\n");
>   		return ERR_PTR(-EINVAL);
>   	}
>   
> +	if (mac_ops->mac_select_pcs &&
> +	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> +	      ERR_PTR(-EOPNOTSUPP))
> +		using_mac_select_pcs = true;
> +
>   	pl = kzalloc(sizeof(*pl), GFP_KERNEL);
>   	if (!pl)
>   		return ERR_PTR(-ENOMEM);

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Andrew Lunn]

> 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.

> 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.

	Andrew

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Greg Ungerer]

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

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Russell King (Oracle)]

On Wed, Nov 22, 2023 at 02:12:44PM +1000, Greg Ungerer wrote:
> 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;
> +}
> +

Looks sensible to me - but I do notice that a black line has been lost
between mv88e6250_phylink_get_caps() and your new function - probably
down to your email client being stupid with whitespace because it's
broken the patch context. Just be aware of that when you come to send
the patch for real.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Greg Ungerer]

On 22/11/23 19:27, Russell King (Oracle) wrote:
> On Wed, Nov 22, 2023 at 02:12:44PM +1000, Greg Ungerer wrote:
>> 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;
>> +}
>> +
> 
> Looks sensible to me - but I do notice that a black line has been lost
> between mv88e6250_phylink_get_caps() and your new function - probably
> down to your email client being stupid with whitespace because it's
> broken the patch context. Just be aware of that when you come to send
> the patch for real.

Oh, yes, for sure. The above was a cut and paste into email client.
I'll send proper patches via git send-email soon.

Regards
Greg

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Russell King (Oracle)]

On Wed, Nov 22, 2023 at 12:19:38AM +1000, Greg Ungerer wrote:
> Hi Russell,
> 
> On 20/5/23 20:41, Russell King (Oracle) wrote:
> > We have been requiring the supported_interfaces bitmap to be filled in
> > by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> > drivers fill in the supported_interfaces bitmap, it is time to enforce
> > this. We have already required supported_interfaces to be set in order
> > for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> > use phy_interface_t bitmaps for optical modules").
> > 
> > Refuse phylink creation if supported_interfaces is empty, and remove
> > code to deal with cases where this mask is empty.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > I believe what I've said above is indeed the case, but there is always
> > the chance that something has been missed and this will cause breakage.
> > I would post as RFC and ask for testing, but in my experience that is
> > a complete waste of time as it doesn't result in any testing feedback.
> > So, it's probably better to get it merged into net-next and then wait
> > for any reports of breakage.
> 
> This commit breaks a platform I have with a Marvell 88e6350 switch.
> During boot up it now fails with:
> 
>     ...
>     mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>     mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
>     error creating PHYLINK: -22
>     mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
>     ...
> 
> The 6350 looks to be similar to the 6352 in many respects, though it lacks
> a SERDES interface, but it otherwise mostly seems compatible. 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,
>  };

Based on what you say, this is probably correct, but I've no idea as I
don't have any data on the 88e6350.

> 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:
> 
>     ...
>     mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>     8<--- cut here ---
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
>     [00000000] *pgd=00000000
>     Internal error: Oops: 5 [#1] ARM
>     Modules linked in:
>     CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
>     Hardware name: Marvell Armada 370/XP (Device Tree)
>     Workqueue: events_unbound deferred_probe_work_func
>     PC is at mv88e6xxx_port_setup+0x1c/0x44
>     LR is at dsa_port_devlink_setup+0x74/0x154
>     pc : [<c057ea24>]    lr : [<c0819598>]    psr: a0000013
>     sp : c184fce0  ip : c542b8f4  fp : 00000000
>     r10: 00000001  r9 : c542a540  r8 : c542bc00
>     r7 : c542b838  r6 : c5244580  r5 : 00000005  r4 : c5244580
>     r3 : 00000000  r2 : c542b840  r1 : 00000005  r0 : c1a02040
>     ...
> 
> I can see that the mv88e6350_ops struct doesn't have an initializer for
> pcs_ops.

You mentioned that the 6350 doesn't have serdes, so there should be no
PCS. mv88e6xxx_port_setup() probably should be doing:

-	if (chip->info->ops->pcs_ops->pcs_init) {
+	if (chip->info->ops->pcs_ops &&
+	    chip->info->ops->pcs_ops->pcs_init) {

> This gets the 6350 switch back to working again (on current linux 6.7-rc2).
> I am not entirely sure if this needs a dedicated phylink_get_caps
> and pcs_ops for the 6350, or if it is safe to use the 6352 ones?

Without knowing what the 6350 cmode values are, I can't comment.

> Looking at the mv88e6351_ops I am guessing it is going to suffer the
> same problems too.

Again, it's a similar problem - I have no information for the 6351
so I could only make guesses.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled

[Greg Ungerer]

On 22/11/23 00:41, Russell King (Oracle) wrote:
> On Wed, Nov 22, 2023 at 12:19:38AM +1000, Greg Ungerer wrote:
>> Hi Russell,
>>
>> On 20/5/23 20:41, Russell King (Oracle) wrote:
>>> We have been requiring the supported_interfaces bitmap to be filled in
>>> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
>>> drivers fill in the supported_interfaces bitmap, it is time to enforce
>>> this. We have already required supported_interfaces to be set in order
>>> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
>>> use phy_interface_t bitmaps for optical modules").
>>>
>>> Refuse phylink creation if supported_interfaces is empty, and remove
>>> code to deal with cases where this mask is empty.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>> I believe what I've said above is indeed the case, but there is always
>>> the chance that something has been missed and this will cause breakage.
>>> I would post as RFC and ask for testing, but in my experience that is
>>> a complete waste of time as it doesn't result in any testing feedback.
>>> So, it's probably better to get it merged into net-next and then wait
>>> for any reports of breakage.
>>
>> This commit breaks a platform I have with a Marvell 88e6350 switch.
>> During boot up it now fails with:
>>
>>      ...
>>      mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>>      mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
>>      error creating PHYLINK: -22
>>      mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
>>      ...
>>
>> The 6350 looks to be similar to the 6352 in many respects, though it lacks
>> a SERDES interface, but it otherwise mostly seems compatible. 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,
>>   };
> 
> Based on what you say, this is probably correct, but I've no idea as I
> don't have any data on the 88e6350.

I am thinking a dedicated 6350 phylink_get_caps may be better here now,
since the 6350 does not have a SERDES lane like the 6352.


>> 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:
>>
>>      ...
>>      mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>>      8<--- cut here ---
>>      Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
>>      [00000000] *pgd=00000000
>>      Internal error: Oops: 5 [#1] ARM
>>      Modules linked in:
>>      CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
>>      Hardware name: Marvell Armada 370/XP (Device Tree)
>>      Workqueue: events_unbound deferred_probe_work_func
>>      PC is at mv88e6xxx_port_setup+0x1c/0x44
>>      LR is at dsa_port_devlink_setup+0x74/0x154
>>      pc : [<c057ea24>]    lr : [<c0819598>]    psr: a0000013
>>      sp : c184fce0  ip : c542b8f4  fp : 00000000
>>      r10: 00000001  r9 : c542a540  r8 : c542bc00
>>      r7 : c542b838  r6 : c5244580  r5 : 00000005  r4 : c5244580
>>      r3 : 00000000  r2 : c542b840  r1 : 00000005  r0 : c1a02040
>>      ...
>>
>> I can see that the mv88e6350_ops struct doesn't have an initializer for
>> pcs_ops.
> 
> You mentioned that the 6350 doesn't have serdes, so there should be no
> PCS. mv88e6xxx_port_setup() probably should be doing:
> 
> -	if (chip->info->ops->pcs_ops->pcs_init) {
> +	if (chip->info->ops->pcs_ops &&
> +	    chip->info->ops->pcs_ops->pcs_init) {

Yes, that probably makes more sense.
With that change in place the probing works again, and does not need
a pcs_ops entry for the 6350.

Thanks
Greg



>> This gets the 6350 switch back to working again (on current linux 6.7-rc2).
>> I am not entirely sure if this needs a dedicated phylink_get_caps
>> and pcs_ops for the 6350, or if it is safe to use the 6352 ones?
> 
> Without knowing what the 6350 cmode values are, I can't comment.
> 
>> Looking at the mv88e6351_ops I am guessing it is going to suffer the
>> same problems too.
> 
> Again, it's a similar problem - I have no information for the 6351
> so I could only make guesses.
>