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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240402095340.fnfjq3gtvjd4hakv@skbuf>
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ