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

Vladimir Oltean
2024-04-02T11:03:59Z

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
>