| Message ID | 20240124082408.49138-1-kurt@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,iwl-next] igc: Add support for LEDs on i225/i226 | expand |
On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: ... > +static int igc_led_hw_control_set(struct led_classdev *led_cdev, > + unsigned long flags) > +{ > + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); > + struct igc_adapter *adapter = netdev_priv(ldev->netdev); > + bool blink = false; > + u32 mode; > + > + if (flags & BIT(TRIGGER_NETDEV_LINK_10)) > + mode = IGC_LEDCTL_MODE_LINK_10; > + if (flags & BIT(TRIGGER_NETDEV_LINK_100)) > + mode = IGC_LEDCTL_MODE_LINK_100; > + if (flags & BIT(TRIGGER_NETDEV_LINK_1000)) > + mode = IGC_LEDCTL_MODE_LINK_1000; > + if (flags & BIT(TRIGGER_NETDEV_LINK_2500)) > + mode = IGC_LEDCTL_MODE_LINK_2500; > + if ((flags & BIT(TRIGGER_NETDEV_TX)) || > + (flags & BIT(TRIGGER_NETDEV_RX))) > + mode = IGC_LEDCTL_MODE_ACTIVITY; Hi Kurt, I guess this can't happen in practice, but if none of the conditions above are met, then mode is used uninitialised below. Flagged by Smatch. > + > + /* blink is recommended for activity */ > + if (mode == IGC_LEDCTL_MODE_ACTIVITY) > + blink = true; > + > + igc_led_set(adapter, ldev->index, mode, blink); > + > + return 0; > +} ...
On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: > Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs > from user space using the netdev trigger. The LEDs are named as > igc-<bus><device>-<led> to be easily identified. > > Offloading activity and link speed is supported. Tested on Intel i225. Nice to see something not driver by phylib/DSA making use of LEDs. Is there no plain on/off support? Ideally we want that for software blinking for when a mode is not supported. Andrew
On Wed Jan 24 2024, Simon Horman wrote: > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: > > ... > >> +static int igc_led_hw_control_set(struct led_classdev *led_cdev, >> + unsigned long flags) >> +{ >> + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); >> + struct igc_adapter *adapter = netdev_priv(ldev->netdev); >> + bool blink = false; >> + u32 mode; >> + >> + if (flags & BIT(TRIGGER_NETDEV_LINK_10)) >> + mode = IGC_LEDCTL_MODE_LINK_10; >> + if (flags & BIT(TRIGGER_NETDEV_LINK_100)) >> + mode = IGC_LEDCTL_MODE_LINK_100; >> + if (flags & BIT(TRIGGER_NETDEV_LINK_1000)) >> + mode = IGC_LEDCTL_MODE_LINK_1000; >> + if (flags & BIT(TRIGGER_NETDEV_LINK_2500)) >> + mode = IGC_LEDCTL_MODE_LINK_2500; >> + if ((flags & BIT(TRIGGER_NETDEV_TX)) || >> + (flags & BIT(TRIGGER_NETDEV_RX))) >> + mode = IGC_LEDCTL_MODE_ACTIVITY; > > Hi Kurt, > > I guess this can't happen in practice, > but if none of the conditions above are met, > then mode is used uninitialised below. Yes, it shouldn't happen, because the supported modes are checked. However, mode can be initialized to off to avoid the warning. > > Flagged by Smatch. Out of curiosity how did you get the warning? I usually run `make W=1 C=1 M=...` before sending patches. Thanks, Kurt
On Wed Jan 24 2024, Andrew Lunn wrote: > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: >> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs >> from user space using the netdev trigger. The LEDs are named as >> igc-<bus><device>-<led> to be easily identified. >> >> Offloading activity and link speed is supported. Tested on Intel i225. > > Nice to see something not driver by phylib/DSA making use of LEDs. > > Is there no plain on/off support? Ideally we want that for software > blinking for when a mode is not supported. Plain on and off is supported is supported, too. Should be possible to implement brightness_set(). Thanks, Kurt
+ Dan Carpenter <carpenter@linaro.org> On Thu, Jan 25, 2024 at 08:20:40AM +0100, Kurt Kanzenbach wrote: > On Wed Jan 24 2024, Simon Horman wrote: > > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: > > > > ... > > > >> +static int igc_led_hw_control_set(struct led_classdev *led_cdev, > >> + unsigned long flags) > >> +{ > >> + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); > >> + struct igc_adapter *adapter = netdev_priv(ldev->netdev); > >> + bool blink = false; > >> + u32 mode; > >> + > >> + if (flags & BIT(TRIGGER_NETDEV_LINK_10)) > >> + mode = IGC_LEDCTL_MODE_LINK_10; > >> + if (flags & BIT(TRIGGER_NETDEV_LINK_100)) > >> + mode = IGC_LEDCTL_MODE_LINK_100; > >> + if (flags & BIT(TRIGGER_NETDEV_LINK_1000)) > >> + mode = IGC_LEDCTL_MODE_LINK_1000; > >> + if (flags & BIT(TRIGGER_NETDEV_LINK_2500)) > >> + mode = IGC_LEDCTL_MODE_LINK_2500; > >> + if ((flags & BIT(TRIGGER_NETDEV_TX)) || > >> + (flags & BIT(TRIGGER_NETDEV_RX))) > >> + mode = IGC_LEDCTL_MODE_ACTIVITY; > > > > Hi Kurt, > > > > I guess this can't happen in practice, > > but if none of the conditions above are met, > > then mode is used uninitialised below. > > Yes, it shouldn't happen, because the supported modes are > checked. However, mode can be initialized to off to avoid the warning. Thanks. > > Flagged by Smatch. > > Out of curiosity how did you get the warning? I usually run `make W=1 C=1 > M=...` before sending patches. Probably there is a better way, but I run Smatch like this: .../smatch/smatch_scripts/kchecker a.c Smatch can be found here: https://github.com/error27/smatch
On Thu, Jan 25, 2024 at 08:31:54AM +0100, Kurt Kanzenbach wrote: > On Wed Jan 24 2024, Andrew Lunn wrote: > > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: > >> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs > >> from user space using the netdev trigger. The LEDs are named as > >> igc-<bus><device>-<led> to be easily identified. > >> > >> Offloading activity and link speed is supported. Tested on Intel i225. > > > > Nice to see something not driver by phylib/DSA making use of LEDs. > > > > Is there no plain on/off support? Ideally we want that for software > > blinking for when a mode is not supported. > > Plain on and off is supported is supported, too. Should be possible to > implement brightness_set(). Great. Its actually better to first implement brightness_set(). That gives you full support for everything the netdev trigger has. Then add offload, which is optional, and will fall back to software for modes which cannot be offloaded. Andrew
On Thu Jan 25 2024, Andrew Lunn wrote: > On Thu, Jan 25, 2024 at 08:31:54AM +0100, Kurt Kanzenbach wrote: >> On Wed Jan 24 2024, Andrew Lunn wrote: >> > On Wed, Jan 24, 2024 at 09:24:08AM +0100, Kurt Kanzenbach wrote: >> >> Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs >> >> from user space using the netdev trigger. The LEDs are named as >> >> igc-<bus><device>-<led> to be easily identified. >> >> >> >> Offloading activity and link speed is supported. Tested on Intel i225. >> > >> > Nice to see something not driver by phylib/DSA making use of LEDs. >> > >> > Is there no plain on/off support? Ideally we want that for software >> > blinking for when a mode is not supported. >> >> Plain on and off is supported is supported, too. Should be possible to >> implement brightness_set(). > > Great. > > Its actually better to first implement brightness_set(). That gives > you full support for everything the netdev trigger has. Then add > offload, which is optional, and will fall back to software for modes > which cannot be offloaded. Understood. I'll do that. Thanks, Kurt
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig index d55638ad8704..767358b60507 100644 --- a/drivers/net/ethernet/intel/Kconfig +++ b/drivers/net/ethernet/intel/Kconfig @@ -368,6 +368,14 @@ config IGC To compile this driver as a module, choose M here. The module will be called igc. + +config IGC_LEDS + def_bool LEDS_TRIGGER_NETDEV + depends on IGC && LEDS_CLASS + help + Optional support for controlling the NIC LED's with the netdev + LED trigger. + config IDPF tristate "Intel(R) Infrastructure Data Path Function Support" depends on PCI_MSI diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile index 95d1e8c490a4..ebffd3054285 100644 --- a/drivers/net/ethernet/intel/igc/Makefile +++ b/drivers/net/ethernet/intel/igc/Makefile @@ -6,6 +6,7 @@ # obj-$(CONFIG_IGC) += igc.o +igc-$(CONFIG_IGC_LEDS) += igc_leds.o igc-objs := igc_main.o igc_mac.o igc_i225.o igc_base.o igc_nvm.o igc_phy.o \ igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o igc_xdp.o diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 45430e246e9c..914d5189b001 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -295,6 +295,9 @@ struct igc_adapter { struct timespec64 start; struct timespec64 period; } perout[IGC_N_PEROUT]; + + /* LEDs */ + struct mutex led_mutex; }; void igc_up(struct igc_adapter *adapter); @@ -720,6 +723,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter); void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts); void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter); +int igc_led_setup(struct igc_adapter *adapter); + #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring)) #define IGC_TXD_DCMD (IGC_ADVTXD_DCMD_EOP | IGC_ADVTXD_DCMD_RS) diff --git a/drivers/net/ethernet/intel/igc/igc_leds.c b/drivers/net/ethernet/intel/igc/igc_leds.c new file mode 100644 index 000000000000..7ca02b2903eb --- /dev/null +++ b/drivers/net/ethernet/intel/igc/igc_leds.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2024 Linutronix GmbH */ + +#include <linux/bits.h> +#include <linux/leds.h> +#include <linux/netdevice.h> +#include <linux/pm_runtime.h> +#include <uapi/linux/uleds.h> + +#include "igc.h" + +#define IGC_NUM_LEDS 3 + +#define IGC_LEDCTL_LED0_MODE_SHIFT 0 +#define IGC_LEDCTL_LED0_MODE_MASK GENMASK(3, 0) +#define IGC_LEDCTL_LED0_BLINK BIT(7) +#define IGC_LEDCTL_LED1_MODE_SHIFT 8 +#define IGC_LEDCTL_LED1_MODE_MASK GENMASK(11, 8) +#define IGC_LEDCTL_LED1_BLINK BIT(15) +#define IGC_LEDCTL_LED2_MODE_SHIFT 16 +#define IGC_LEDCTL_LED2_MODE_MASK GENMASK(19, 16) +#define IGC_LEDCTL_LED2_BLINK BIT(23) + +#define IGC_LEDCTL_MODE_LINK_10 0x05 +#define IGC_LEDCTL_MODE_LINK_100 0x06 +#define IGC_LEDCTL_MODE_LINK_1000 0x07 +#define IGC_LEDCTL_MODE_LINK_2500 0x08 +#define IGC_LEDCTL_MODE_ACTIVITY 0x0b + +#define IGC_SUPPORTED_MODES \ + (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK_1000) | \ + BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_10) | \ + BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX)) + +struct igc_led_classdev { + struct net_device *netdev; + struct led_classdev led; + int index; +}; + +#define lcdev_to_igc_ldev(lcdev) \ + container_of(lcdev, struct igc_led_classdev, led) + +static void igc_led_select(struct igc_adapter *adapter, int led, + u32 *mask, u32 *shift, u32 *blink) +{ + switch (led) { + case 0: + *mask = IGC_LEDCTL_LED0_MODE_MASK; + *shift = IGC_LEDCTL_LED0_MODE_SHIFT; + *blink = IGC_LEDCTL_LED0_BLINK; + break; + case 1: + *mask = IGC_LEDCTL_LED1_MODE_MASK; + *shift = IGC_LEDCTL_LED1_MODE_SHIFT; + *blink = IGC_LEDCTL_LED1_BLINK; + break; + case 2: + *mask = IGC_LEDCTL_LED2_MODE_MASK; + *shift = IGC_LEDCTL_LED2_MODE_SHIFT; + *blink = IGC_LEDCTL_LED2_BLINK; + break; + default: + *mask = *shift = *blink = 0; + netdev_err(adapter->netdev, "Unknown LED %d selected!\n", led); + } +} + +static void igc_led_set(struct igc_adapter *adapter, int led, u32 mode, + bool blink) +{ + u32 shift, mask, blink_bit, ledctl; + struct igc_hw *hw = &adapter->hw; + + igc_led_select(adapter, led, &mask, &shift, &blink_bit); + + pm_runtime_get_sync(&adapter->pdev->dev); + mutex_lock(&adapter->led_mutex); + + /* Set mode */ + ledctl = rd32(IGC_LEDCTL); + ledctl &= ~mask; + ledctl |= mode << shift; + + /* Configure blinking */ + if (blink) + ledctl |= blink_bit; + else + ledctl &= ~blink_bit; + wr32(IGC_LEDCTL, ledctl); + + mutex_unlock(&adapter->led_mutex); + pm_runtime_put(&adapter->pdev->dev); +} + +static u32 igc_led_get(struct igc_adapter *adapter, int led) +{ + u32 shift, mask, blink_bit, ledctl; + struct igc_hw *hw = &adapter->hw; + + igc_led_select(adapter, led, &mask, &shift, &blink_bit); + + pm_runtime_get_sync(&adapter->pdev->dev); + mutex_lock(&adapter->led_mutex); + ledctl = rd32(IGC_LEDCTL); + mutex_unlock(&adapter->led_mutex); + pm_runtime_put(&adapter->pdev->dev); + + return (ledctl & mask) >> shift; +} + +static int igc_led_hw_control_is_supported(struct led_classdev *led_cdev, + unsigned long flags) +{ + bool rx, tx; + + if (flags & ~IGC_SUPPORTED_MODES) + return -EOPNOTSUPP; + + rx = flags & BIT(TRIGGER_NETDEV_RX); + tx = flags & BIT(TRIGGER_NETDEV_TX); + if (rx != tx) + return -EOPNOTSUPP; + + return 0; +} + +static int igc_led_hw_control_set(struct led_classdev *led_cdev, + unsigned long flags) +{ + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); + struct igc_adapter *adapter = netdev_priv(ldev->netdev); + bool blink = false; + u32 mode; + + if (flags & BIT(TRIGGER_NETDEV_LINK_10)) + mode = IGC_LEDCTL_MODE_LINK_10; + if (flags & BIT(TRIGGER_NETDEV_LINK_100)) + mode = IGC_LEDCTL_MODE_LINK_100; + if (flags & BIT(TRIGGER_NETDEV_LINK_1000)) + mode = IGC_LEDCTL_MODE_LINK_1000; + if (flags & BIT(TRIGGER_NETDEV_LINK_2500)) + mode = IGC_LEDCTL_MODE_LINK_2500; + if ((flags & BIT(TRIGGER_NETDEV_TX)) || + (flags & BIT(TRIGGER_NETDEV_RX))) + mode = IGC_LEDCTL_MODE_ACTIVITY; + + /* blink is recommended for activity */ + if (mode == IGC_LEDCTL_MODE_ACTIVITY) + blink = true; + + igc_led_set(adapter, ldev->index, mode, blink); + + return 0; +} + +static int igc_led_hw_control_get(struct led_classdev *led_cdev, + unsigned long *flags) +{ + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); + struct igc_adapter *adapter = netdev_priv(ldev->netdev); + u32 mode; + + mode = igc_led_get(adapter, ldev->index); + + switch (mode) { + case IGC_LEDCTL_MODE_ACTIVITY: + *flags = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); + break; + case IGC_LEDCTL_MODE_LINK_10: + *flags = BIT(TRIGGER_NETDEV_LINK_10); + break; + case IGC_LEDCTL_MODE_LINK_100: + *flags = BIT(TRIGGER_NETDEV_LINK_100); + break; + case IGC_LEDCTL_MODE_LINK_1000: + *flags = BIT(TRIGGER_NETDEV_LINK_1000); + break; + case IGC_LEDCTL_MODE_LINK_2500: + *flags = BIT(TRIGGER_NETDEV_LINK_2500); + break; + } + + return 0; +} + +static struct device *igc_led_hw_control_get_device(struct led_classdev *led_cdev) +{ + struct igc_led_classdev *ldev = lcdev_to_igc_ldev(led_cdev); + + return &ldev->netdev->dev; +} + +static void igc_led_get_name(struct igc_adapter *adapter, int index, char *buf, + size_t buf_len) +{ + snprintf(buf, buf_len, "igc-%x%x-led%d", + pci_domain_nr(adapter->pdev->bus), + pci_dev_id(adapter->pdev), index); +} + +static void igc_setup_ldev(struct igc_led_classdev *ldev, + struct net_device *netdev, int index) +{ + struct igc_adapter *adapter = netdev_priv(netdev); + struct led_classdev *led_cdev = &ldev->led; + char led_name[LED_MAX_NAME_SIZE]; + + ldev->netdev = netdev; + ldev->index = index; + + igc_led_get_name(adapter, index, led_name, LED_MAX_NAME_SIZE); + led_cdev->name = led_name; + led_cdev->hw_control_trigger = "netdev"; + led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN; + led_cdev->hw_control_is_supported = igc_led_hw_control_is_supported; + led_cdev->hw_control_set = igc_led_hw_control_set; + led_cdev->hw_control_get = igc_led_hw_control_get; + led_cdev->hw_control_get_device = igc_led_hw_control_get_device; + + devm_led_classdev_register(&netdev->dev, led_cdev); +} + +int igc_led_setup(struct igc_adapter *adapter) +{ + struct net_device *netdev = adapter->netdev; + struct device *dev = &netdev->dev; + struct igc_led_classdev *leds; + int i; + + mutex_init(&adapter->led_mutex); + + leds = devm_kcalloc(dev, IGC_NUM_LEDS, sizeof(*leds), GFP_KERNEL); + if (!leds) + return -ENOMEM; + + for (i = 0; i < IGC_NUM_LEDS; i++) + igc_setup_ldev(leds + i, netdev, i); + + return 0; +} diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index ba8d3fe186ae..5ee26def75a7 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6977,6 +6977,12 @@ static int igc_probe(struct pci_dev *pdev, pm_runtime_put_noidle(&pdev->dev); + if (IS_ENABLED(CONFIG_IGC_LEDS)) { + err = igc_led_setup(adapter); + if (err) + goto err_register; + } + return 0; err_register: diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h index d38c87d7e5e8..e5b893fc5b66 100644 --- a/drivers/net/ethernet/intel/igc/igc_regs.h +++ b/drivers/net/ethernet/intel/igc/igc_regs.h @@ -12,6 +12,7 @@ #define IGC_MDIC 0x00020 /* MDI Control - RW */ #define IGC_CONNSW 0x00034 /* Copper/Fiber switch control - RW */ #define IGC_VET 0x00038 /* VLAN Ether Type - RW */ +#define IGC_LEDCTL 0x00E00 /* LED Control - RW */ #define IGC_I225_PHPM 0x00E14 /* I225 PHY Power Management */ #define IGC_GPHY_VERSION 0x0001E /* I225 gPHY Firmware Version */
Add support for LEDs on i225/i226. The LEDs can be controlled via sysfs from user space using the netdev trigger. The LEDs are named as igc-<bus><device>-<led> to be easily identified. Offloading activity and link speed is supported. Tested on Intel i225. Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- drivers/net/ethernet/intel/Kconfig | 8 + drivers/net/ethernet/intel/igc/Makefile | 1 + drivers/net/ethernet/intel/igc/igc.h | 5 + drivers/net/ethernet/intel/igc/igc_leds.c | 241 ++++++++++++++++++++++ drivers/net/ethernet/intel/igc/igc_main.c | 6 + drivers/net/ethernet/intel/igc/igc_regs.h | 1 + 6 files changed, 262 insertions(+) create mode 100644 drivers/net/ethernet/intel/igc/igc_leds.c