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

[Greg Ungerer <gerg@kernel.org>]

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);