| Message ID | 20240406-v6-8-0-net-next-mv88e6xxx-leds-v4-v4-7-eb97665e7f96@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add generic support for netdev LEDs | expand |
Nitpick: title: net: dsa: mv88e6xxx On Sat, Apr 06, 2024 at 03:13:34PM -0500, Andrew Lunn wrote: > Make use of the helpers to add LEDs to the user ports when the port is > setup. Also remove the LEDs when the port is destroyed to avoid any > race conditions with users of /sys/class/leds and LED triggers after > the driver has been removed. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/dsa/mv88e6xxx/Kconfig | 1 + > drivers/net/dsa/mv88e6xxx/chip.c | 119 +++++++++++++++++++++++++++++++++++++- > drivers/net/dsa/mv88e6xxx/chip.h | 13 +++++ > 3 files changed, 132 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig > index e3181d5471df..ded5c6b9132b 100644 > --- a/drivers/net/dsa/mv88e6xxx/Kconfig > +++ b/drivers/net/dsa/mv88e6xxx/Kconfig > @@ -5,6 +5,7 @@ config NET_DSA_MV88E6XXX > select IRQ_DOMAIN > select NET_DSA_TAG_EDSA > select NET_DSA_TAG_DSA > + select NETDEV_LEDS Do we want mv88e6xxx to select NETDEV_LEDS, or also to be able to build it without LED control? If we want to select, do we always want drivers to select NETDEV_LEDS? Maybe hide the help text from the NETDEV_LEDS option, make it invisible to the user? > help > This driver adds support for most of the Marvell 88E6xxx models of > Ethernet switch chips, except 88E6060. > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 3d7e4aa9293a..4e4031f11088 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -31,6 +31,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/phylink.h> > #include <net/dsa.h> > +#include <net/netdev_leds.h> > > #include "chip.h" > #include "devlink.h" > @@ -3129,6 +3130,105 @@ static int mv88e6xxx_switch_reset(struct mv88e6xxx_chip *chip) > return mv88e6xxx_software_reset(chip); > } > > +static int mv88e6xxx_led_brightness_set(struct net_device *ndev, > + u8 led, enum led_brightness value) > +{ > + struct dsa_port *dp = dsa_port_from_netdev(ndev); > + struct mv88e6xxx_chip *chip = dp->ds->priv; > + int port = dp->index; > + int err; > + > + if (chip->info->ops->led_brightness_set) { > + mv88e6xxx_reg_lock(chip); > + err = chip->info->ops->led_brightness_set(chip, port, led, > + value); > + mv88e6xxx_reg_unlock(chip); > + return err; > + } > + return -EOPNOTSUPP; The opposite looks more natural, if (!ops) return -EOPNOTSUPP, followed by the normal code with 1 level of indentation less. Comment applicable to all netdev_leds_ops below. > +} > + > static int mv88e6xxx_set_port_mode(struct mv88e6xxx_chip *chip, int port, > enum mv88e6xxx_frame_mode frame, > enum mv88e6xxx_egress_mode egress, u16 etype) > @@ -4006,7 +4106,9 @@ 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; > + struct mv88e6xxx_port *p; > int err; > > if (chip->info->ops->pcs_ops && > @@ -4016,13 +4118,28 @@ static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port) > return err; > } > > - return mv88e6xxx_setup_devlink_regions_port(ds, port); Unfortunate, but there is an existing bug here. chip->info->ops->pcs_ops->pcs_init() allocates memory. If mv88e6xxx_setup_devlink_regions_port() fails here, that's memory we're not freeing, by not calling chip->info->ops->pcs_ops->pcs_teardown(). > + err = mv88e6xxx_setup_devlink_regions_port(ds, port); > + if (err) > + return err; > + > + if (dp->dn && dsa_is_user_port(ds, port)) { > + p = &chip->ports[port]; > + INIT_LIST_HEAD(&p->leds); > + err = netdev_leds_setup(dp->user, dp->dn, &p->leds, > + &mv88e6xxx_netdev_leds_ops, 2); Does anything bad happen with netdev_leds_setup() on platform_data? OF APIs are pretty NULL-tolerant. > + if (err) > + mv88e6xxx_teardown_devlink_regions_port(ds, port); > + } > + return err; > } > > static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port) > { > struct mv88e6xxx_chip *chip = ds->priv; > > + if (dsa_is_user_port(ds, port)) > + netdev_leds_teardown(&chip->ports[port].leds); On platform_data, we are calling netdev_leds_teardown() when we never called netdev_leds_setup() and never initialized the port leds list. > + > mv88e6xxx_teardown_devlink_regions_port(ds, port); > > if (chip->info->ops->pcs_ops && > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > index 64f8bde68ccf..d15bb5810831 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.h > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -291,6 +291,9 @@ struct mv88e6xxx_port { > > /* MacAuth Bypass control flag */ > bool mab; > + > + /* LEDs associated to the port */ > + struct list_head leds; > }; > > enum mv88e6xxx_region_id { > @@ -432,6 +435,7 @@ struct mv88e6xxx_chip { > > /* Bridge MST to SID mappings */ > struct list_head msts; > + Stray change. > }; > > struct mv88e6xxx_bus_ops { > @@ -668,6 +672,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 >
diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig index e3181d5471df..ded5c6b9132b 100644 --- a/drivers/net/dsa/mv88e6xxx/Kconfig +++ b/drivers/net/dsa/mv88e6xxx/Kconfig @@ -5,6 +5,7 @@ config NET_DSA_MV88E6XXX select IRQ_DOMAIN select NET_DSA_TAG_EDSA select NET_DSA_TAG_DSA + select NETDEV_LEDS help This driver adds support for most of the Marvell 88E6xxx models of Ethernet switch chips, except 88E6060. diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 3d7e4aa9293a..4e4031f11088 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -31,6 +31,7 @@ #include <linux/gpio/consumer.h> #include <linux/phylink.h> #include <net/dsa.h> +#include <net/netdev_leds.h> #include "chip.h" #include "devlink.h" @@ -3129,6 +3130,105 @@ static int mv88e6xxx_switch_reset(struct mv88e6xxx_chip *chip) return mv88e6xxx_software_reset(chip); } +static int mv88e6xxx_led_brightness_set(struct net_device *ndev, + u8 led, enum led_brightness value) +{ + struct dsa_port *dp = dsa_port_from_netdev(ndev); + struct mv88e6xxx_chip *chip = dp->ds->priv; + int port = dp->index; + int err; + + if (chip->info->ops->led_brightness_set) { + mv88e6xxx_reg_lock(chip); + err = chip->info->ops->led_brightness_set(chip, port, led, + value); + mv88e6xxx_reg_unlock(chip); + return err; + } + return -EOPNOTSUPP; +} + +static int mv88e6xxx_led_blink_set(struct net_device *ndev, u8 led, + unsigned long *delay_on, + unsigned long *delay_off) +{ + struct dsa_port *dp = dsa_port_from_netdev(ndev); + struct mv88e6xxx_chip *chip = dp->ds->priv; + int port = dp->index; + int err; + + if (chip->info->ops->led_blink_set) { + mv88e6xxx_reg_lock(chip); + err = chip->info->ops->led_blink_set(chip, port, led, + delay_on, delay_off); + mv88e6xxx_reg_unlock(chip); + return err; + } + return -EOPNOTSUPP; +} + +static int mv88e6xxx_led_hw_control_is_supported(struct net_device *ndev, + u8 led, unsigned long flags) +{ + struct dsa_port *dp = dsa_port_from_netdev(ndev); + struct mv88e6xxx_chip *chip = dp->ds->priv; + int port = dp->index; + int err; + + if (chip->info->ops->led_hw_control_is_supported) { + mv88e6xxx_reg_lock(chip); + err = chip->info->ops->led_hw_control_is_supported(chip, port, + led, flags); + mv88e6xxx_reg_unlock(chip); + return err; + } + return -EOPNOTSUPP; +} + +static int mv88e6xxx_led_hw_control_set(struct net_device *ndev, u8 led, + unsigned long flags) +{ + struct dsa_port *dp = dsa_port_from_netdev(ndev); + struct mv88e6xxx_chip *chip = dp->ds->priv; + int port = dp->index; + int err; + + if (chip->info->ops->led_hw_control_set) { + mv88e6xxx_reg_lock(chip); + err = chip->info->ops->led_hw_control_set(chip, port, + led, flags); + mv88e6xxx_reg_unlock(chip); + return err; + } + return -EOPNOTSUPP; +} + +static int mv88e6xxx_led_hw_control_get(struct net_device *ndev, + u8 led, unsigned long *flags) +{ + struct dsa_port *dp = dsa_port_from_netdev(ndev); + struct mv88e6xxx_chip *chip = dp->ds->priv; + int port = dp->index; + int err; + + if (chip->info->ops->led_hw_control_get) { + mv88e6xxx_reg_lock(chip); + err = chip->info->ops->led_hw_control_get(chip, port, + led, flags); + mv88e6xxx_reg_unlock(chip); + return err; + } + return -EOPNOTSUPP; +} + +static struct netdev_leds_ops mv88e6xxx_netdev_leds_ops = { + .brightness_set = mv88e6xxx_led_brightness_set, + .blink_set = mv88e6xxx_led_blink_set, + .hw_control_is_supported = mv88e6xxx_led_hw_control_is_supported, + .hw_control_set = mv88e6xxx_led_hw_control_set, + .hw_control_get = mv88e6xxx_led_hw_control_get, +}; + static int mv88e6xxx_set_port_mode(struct mv88e6xxx_chip *chip, int port, enum mv88e6xxx_frame_mode frame, enum mv88e6xxx_egress_mode egress, u16 etype) @@ -4006,7 +4106,9 @@ 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; + struct mv88e6xxx_port *p; int err; if (chip->info->ops->pcs_ops && @@ -4016,13 +4118,28 @@ 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 && dsa_is_user_port(ds, port)) { + p = &chip->ports[port]; + INIT_LIST_HEAD(&p->leds); + err = netdev_leds_setup(dp->user, dp->dn, &p->leds, + &mv88e6xxx_netdev_leds_ops, 2); + if (err) + mv88e6xxx_teardown_devlink_regions_port(ds, port); + } + return err; } static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port) { struct mv88e6xxx_chip *chip = ds->priv; + if (dsa_is_user_port(ds, port)) + netdev_leds_teardown(&chip->ports[port].leds); + mv88e6xxx_teardown_devlink_regions_port(ds, port); if (chip->info->ops->pcs_ops && diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 64f8bde68ccf..d15bb5810831 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -291,6 +291,9 @@ struct mv88e6xxx_port { /* MacAuth Bypass control flag */ bool mab; + + /* LEDs associated to the port */ + struct list_head leds; }; enum mv88e6xxx_region_id { @@ -432,6 +435,7 @@ struct mv88e6xxx_chip { /* Bridge MST to SID mappings */ struct list_head msts; + }; struct mv88e6xxx_bus_ops { @@ -668,6 +672,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 {
Make use of the helpers to add LEDs to the user ports when the port is setup. Also remove the LEDs when the port is destroyed to avoid any race conditions with users of /sys/class/leds and LED triggers after the driver has been removed. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/mv88e6xxx/Kconfig | 1 + drivers/net/dsa/mv88e6xxx/chip.c | 119 +++++++++++++++++++++++++++++++++++++- drivers/net/dsa/mv88e6xxx/chip.h | 13 +++++ 3 files changed, 132 insertions(+), 1 deletion(-)