| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 2 Apr 2024 12:53:40 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Russell King <linux@...linux.org.uk>,
Gregory Clement <gregory.clement@...tlin.com>,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/7] net: Add helpers for netdev LEDs
On Mon, Apr 01, 2024 at 08:35:47AM -0500, Andrew Lunn wrote:
> Add a set of helpers for parsing the standard device tree properties
> for LEDs as part of an ethernet device, and registering them with the
> LED subsystem. This code can be used by any sort of netdev driver,
> including plain MAC, DSA switches or pure switchdev switch driver.
>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
> include/net/netdev_leds.h | 45 +++++++++++
> net/Kconfig | 11 +++
> net/core/Makefile | 1 +
> net/core/netdev-leds.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 258 insertions(+)
>
> diff --git a/include/net/netdev_leds.h b/include/net/netdev_leds.h
> new file mode 100644
> index 000000000000..239f492f29f5
> --- /dev/null
> +++ b/include/net/netdev_leds.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Helpers used for creating and managing LEDs on a netdev MAC
> + * driver.
> + */
> +
> +#ifndef _NET_NETDEV_LEDS_H
> +#define _NET_NETDEV_LEDS_H
> +
An object file which has "#include <net/netdev_leds.h>" as its only
line of source code does not compile. All headers should be
self-contained in terms of #include dependencies.
../include/net/netdev_leds.h:11:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int (*brightness_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:11:49: error: unknown type name 'u8'
int (*brightness_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:12:15: warning: declaration of 'enum led_brightness' will not be visible outside of this function [-Wvisibility]
enum led_brightness brightness);
^
../include/net/netdev_leds.h:13:26: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int (*blink_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:13:44: error: unknown type name 'u8'
int (*blink_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:15:40: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:15:58: error: unknown type name 'u8'
int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:17:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int (*hw_control_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:17:49: error: unknown type name 'u8'
int (*hw_control_set)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:19:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int (*hw_control_get)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:19:49: error: unknown type name 'u8'
int (*hw_control_get)(struct net_device *ndev, u8 led,
^
../include/net/netdev_leds.h:24:30: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
^
../include/net/netdev_leds.h:24:55: warning: declaration of 'struct device_node' will not be visible outside of this function [-Wvisibility]
int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
^
../include/net/netdev_leds.h:25:16: warning: declaration of 'struct list_head' will not be visible outside of this function [-Wvisibility]
struct list_head *list, struct netdev_leds_ops *ops,
^
../include/net/netdev_leds.h:28:34: warning: declaration of 'struct list_head' will not be visible outside of this function [-Wvisibility]
void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
^
../include/net/netdev_leds.h:28:58: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
> +struct netdev_leds_ops {
> + int (*brightness_set)(struct net_device *ndev, u8 led,
> + enum led_brightness brightness);
> + int (*blink_set)(struct net_device *ndev, u8 led,
> + unsigned long *delay_on, unsigned long *delay_off);
One single space between arguments.
> + int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
> + unsigned long flags);
> + int (*hw_control_set)(struct net_device *ndev, u8 led,
> + unsigned long flags);
> + int (*hw_control_get)(struct net_device *ndev, u8 led,
> + unsigned long *flags);
> +};
> +
> +#ifdef CONFIG_NETDEV_LEDS
> +int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
> + struct list_head *list, struct netdev_leds_ops *ops,
> + int max_leds);
Should we be even more opaque in the API, aka instead of requiring the
user to explicitly hold a struct list_head, just give it an opaque
struct netdev_led_group * which holds that (as a hidden implementation
detail)?
The API structure could be simply styled as
"struct netdev_led_group *netdev_led_group_create()" and
"void netdev_led_group_destroy(const struct netdev_led_group *group)",
and it would allow for future editing of the actual implementation.
Just an idea.
> +
> +void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
> +
> +#else
> +static inline int netdev_leds_setup(struct net_device *ndev,
> + struct device_node *np,
> + struct list_head *list,
> + struct netdev_leds_ops *ops)
> +{
> + return 0;
> +}
> +
> +static inline void netdev_leds_teardown(struct list_head *list,
> + struct net_device *ndev)
> +{
> +}
> +#endif /* CONFIG_NETDEV_LEDS */
> +
> +#endif /* _NET_PORT_LEDS_H */
> diff --git a/net/Kconfig b/net/Kconfig
> index 3e57ccf0da27..753dfd11f014 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -516,4 +516,15 @@ config NET_TEST
>
> If unsure, say N.
>
> +config NETDEV_LEDS
> + bool "NETDEV helper code for MAC LEDs"
> + select LEDS_CLASS
> + select LEDS_TRIGGERS
> + select LEDS_TRIGGER_NETDEV
> + help
> + NICs and Switches often contain LED controllers. When the LEDs
switches
> + are part of the MAC, the MAC driver, aka netdev driver, should
> + make the LEDs available. NETDEV_LEDS offers a small library
> + of code to help MAC drivers do this.
> +
> endif # if NET
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 21d6fbc7e884..d04ce07541b5 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -42,3 +42,4 @@ obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
> obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
> obj-$(CONFIG_OF) += of_net.o
> obj-$(CONFIG_NET_TEST) += net_test.o
> +obj-$(CONFIG_NETDEV_LEDS) += netdev-leds.o
> diff --git a/net/core/netdev-leds.c b/net/core/netdev-leds.c
> new file mode 100644
> index 000000000000..802dd819a991
> --- /dev/null
> +++ b/net/core/netdev-leds.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <net/netdev_leds.h>
> +
> +struct netdev_led {
> + struct list_head led_list;
> + struct led_classdev led_cdev;
> + struct netdev_leds_ops *ops;
> + struct net_device *ndev;
> + u8 index;
> +};
> +
> +#define to_netdev_led(d) container_of(d, struct netdev_led, led_cdev)
> +
> +static int netdev_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return netdev_led->ops->brightness_set(netdev_led->ndev,
> + netdev_led->index,
> + value);
> +}
> +
> +static int netdev_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on, unsigned long *delay_off)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return netdev_led->ops->blink_set(netdev_led->ndev,
> + netdev_led->index,
> + delay_on, delay_off);
> +}
> +
> +static __maybe_unused int
> +netdev_hw_control_is_supported(struct led_classdev *led_cdev,
> + unsigned long flags)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return netdev_led->ops->hw_control_is_supported(netdev_led->ndev,
> + netdev_led->index,
> + flags);
> +}
> +
> +static __maybe_unused int netdev_hw_control_set(struct led_classdev *led_cdev,
> + unsigned long flags)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return netdev_led->ops->hw_control_set(netdev_led->ndev,
> + netdev_led->index,
> + flags);
> +}
> +
> +static __maybe_unused int netdev_hw_control_get(struct led_classdev *led_cdev,
> + unsigned long *flags)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return netdev_led->ops->hw_control_get(netdev_led->ndev,
> + netdev_led->index,
> + flags);
> +}
> +
> +static struct device *
> +netdev_hw_control_get_device(struct led_classdev *led_cdev)
> +{
> + struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> + return &netdev_led->ndev->dev;
> +}
> +
> +static int netdev_led_setup(struct net_device *ndev, struct device_node *led,
> + struct list_head *list, struct netdev_leds_ops *ops,
> + int max_leds)
> +{
> + struct led_init_data init_data = {};
> + struct device *dev = &ndev->dev;
> + struct netdev_led *netdev_led;
> + struct led_classdev *cdev;
> + u32 index;
> + int err;
> +
> + netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
> + if (!netdev_led)
> + return -ENOMEM;
> +
> + netdev_led->ndev = ndev;
> + netdev_led->ops = ops;
> + cdev = &netdev_led->led_cdev;
> +
> + err = of_property_read_u32(led, "reg", &index);
> + if (err)
> + return err;
> +
> + if (index >= max_leds)
> + return -EINVAL;
> +
> + netdev_led->index = index;
> +
> + if (ops->brightness_set)
> + cdev->brightness_set_blocking = netdev_brightness_set;
> + if (ops->blink_set)
> + cdev->blink_set = netdev_blink_set;
> +#ifdef CONFIG_LEDS_TRIGGERS
> + if (ops->hw_control_is_supported)
> + cdev->hw_control_is_supported = netdev_hw_control_is_supported;
> + if (ops->hw_control_set)
> + cdev->hw_control_set = netdev_hw_control_set;
> + if (ops->hw_control_get)
> + cdev->hw_control_get = netdev_hw_control_get;
> + cdev->hw_control_trigger = "netdev";
> +#endif
> + cdev->hw_control_get_device = netdev_hw_control_get_device;
> + cdev->max_brightness = 1;
> + init_data.fwnode = of_fwnode_handle(led);
> + init_data.devname_mandatory = true;
> +
> + init_data.devicename = dev_name(dev);
> + err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> + if (err)
> + return err;
> +
> + INIT_LIST_HEAD(&netdev_led->led_list);
> + list_add(&netdev_led->led_list, list);
> +
> + return 0;
> +}
> +
> +/**
> + * netdev_leds_setup - Parse DT node and create LEDs for netdev
> + *
> + * @ndev: struct netdev for the MAC
> + * @np: ethernet-node in device tree
> + * @list: list to add LEDs to
> + * @ops: structure of ops to manipulate the LED.
> + * @max_leds: maximum number of LEDs support by netdev.
> + *
> + * Parse the device tree node, as described in
> + * ethernet-controller.yaml, and find any LEDs. For each LED found,
> + * ensure the reg value is less than max_leds, create an LED and
> + * register it with the LED subsystem. The LED will be added to the
> + * list, which can be shared by all netdevs of the device. The ops
> + * structure contains the callbacks needed to control the LEDs.
> + *
> + * Return 0 in success, otherwise an negative error code.
> + */
> +int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
> + struct list_head *list, struct netdev_leds_ops *ops,
> + int max_leds)
> +{
> + struct device_node *leds, *led;
> + int err;
> +
> + leds = of_get_child_by_name(np, "leds");
> + if (!leds)
> + return 0;
> +
> + for_each_available_child_of_node(leds, led) {
> + err = netdev_led_setup(ndev, led, list, ops, max_leds);
> + if (err) {
> + of_node_put(led);
What is the refcounting scheme for the "leds" node? Its refcount is left
incremented both on success, and on error here. It is not decremented
anywhere.
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_leds_setup);
> +
> +/**
> + * netdev_leds_teardown - Remove LEDs for a netdev
> + *
> + * @list: list to add LEDs to teardown
> + * @ndev: The netdev for which LEDs should be removed
> + *
> + * Unregister all LEDs for a given netdev, freeing up any allocated
> + * memory.
> + */
> +void netdev_leds_teardown(struct list_head *list, struct net_device *ndev)
> +{
> + struct netdev_led *netdev_led;
> + struct led_classdev *cdev;
> + struct device *dev;
> +
> + list_for_each_entry(netdev_led, list, led_list) {
> + if (netdev_led->ndev != ndev)
> + continue;
I don't exactly see what's the advantage, in your proposal, of allowing
the API to bundle up the LED groups of multiple netdevs into a single
struct list_head. I see switches like mv88e6xxx have a single list for
the entire chip rather than one per port. It makes for a less
straightforward implementation here (we could have just wiped out the
entire list, if we knew there's a single group in it).
Also, what's the locking scheme expected by the API? The setup and
teardown methods are not reentrant.
> + dev = &netdev_led->ndev->dev;
> + cdev = &netdev_led->led_cdev;
> + devm_led_classdev_unregister(dev, cdev);
I think devres-using API functions should be prefixed with devm_ in
their name for callers' awareness, rather than this aspect being silent.
And, if a netdev_leds_teardown() exists, I suspect devres usage isn't
even necessary, you could use list_for_each_entry_safe() and also
kfree() the netdev_led.
> + }
> +}
> +EXPORT_SYMBOL_GPL(netdev_leds_teardown);
>
> --
> 2.43.0
>
Powered by blists - more mailing lists