Re: [PATCH net-next v3 6/7] dsa: mv88e6xxx: Create port/netdev LEDs

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Apr 01, 2024 at 08:35:51AM -0500, Andrew Lunn wrote:
> @@ -4006,6 +4106,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>  
>  static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
>  {
> +	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err;
>  
> @@ -4016,13 +4117,26 @@ static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
>  			return err;
>  	}
>  
> -	return mv88e6xxx_setup_devlink_regions_port(ds, port);
> +	err  = mv88e6xxx_setup_devlink_regions_port(ds, port);
> +	if (err)
> +		return err;
> +
> +	if (dp->dn) {
> +		err = netdev_leds_setup(dp->user, dp->dn, &chip->leds,
> +					&mv88e6xxx_netdev_leds_ops, 2);

This (dereferencing dp->user regardless of dp->type) is a dangerous
thing to do. Let alone the fact that dp->user is NULL for DSA ports. It
is actually in a union with struct net_device *conduit, for CPU ports.
So you're also setting up LEDs for the conduit interface here...

Please make it conditional on dsa_port_is_user(), and same for teardown.

> +		if (err)
> +			mv88e6xxx_teardown_devlink_regions_port(ds, port);
> +	}
> +	return err;
>  }
>  
>  static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
>  {
> +	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> +	netdev_leds_teardown(&chip->leds, dp->user);
> +
>  	mv88e6xxx_teardown_devlink_regions_port(ds, port);
>  
>  	if (chip->info->ops->pcs_ops &&
> @@ -6397,6 +6511,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>  	INIT_LIST_HEAD(&chip->mdios);
>  	idr_init(&chip->policies);
>  	INIT_LIST_HEAD(&chip->msts);
> +	INIT_LIST_HEAD(&chip->leds);
>  
>  	return chip;
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 64f8bde68ccf..b70e74203b31 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -432,6 +432,9 @@ struct mv88e6xxx_chip {
>  
>  	/* Bridge MST to SID mappings */
>  	struct list_head msts;
> +
> +	/* LEDs associated to the ports */
> +	struct list_head leds;
>  };
>  
>  struct mv88e6xxx_bus_ops {
> @@ -668,6 +671,15 @@ struct mv88e6xxx_ops {
>  	int (*led_blink_set)(struct mv88e6xxx_chip *chip, int port, u8 led,
>  			     unsigned long *delay_on,
>  			     unsigned long *delay_off);
> +	int (*led_hw_control_is_supported)(struct mv88e6xxx_chip *chip,
> +					   int port, u8 led,
> +					   unsigned long flags);
> +	int (*led_hw_control_set)(struct mv88e6xxx_chip *chip,
> +				  int port, u8 led,
> +				  unsigned long flags);
> +	int (*led_hw_control_get)(struct mv88e6xxx_chip *chip,
> +				  int port, u8 led,
> +				  unsigned long *flags);
>  };
>  
>  struct mv88e6xxx_irq_ops {
> 
> -- 
> 2.43.0
>