From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:10
This is another attempt in adding support for PHY LEDs. Most of the
times Switch/PHY have connected multiple LEDs that are controlled by HW
based on some rules/event. Currently we lack any support for a generic
way to control the HW part and normally we either never implement the
feature or only add control for brightness or hw blink.
This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.
The current idea is:
- LED driver implement 2 API (trigger and configure). They are used to
put the LED in offload mode and to configure the various trigger.
- We have offload triggers that are used to expose to userspace the
supported offload triggers and set the offload mode on trigger
activation.
- The LED driver will declare each supported trigger and each supported
offloaded trigger and communicate it to the trigger that will expose them
via sysfs.
- Only supported triggers from linux,supported-offload-triggers will be
exposed by the offload trigger to userspace via sysfs. Other won't be
configurable as they are not supported by the LED driver.
- The LED driver will set how the Offload mode is activated/disabled and
will set a configure function to set each supported offload triggers.
This function provide 3 different mode enable/disable/read that
will enable or disable the offload trigger or read the current status.
- On offload trigger activation, only the offload mode is triggered but
the offload triggers are not configured. This means that the LED will
run in offload mode but will have the default rules/event set by the
device by default. To change this the user will have to operate via
userspace (or we can consider adding another binding in the dts to
declare also a default trigger configuration)
Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we pass a u32 flag that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).
I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.
v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
Change the logic, now the LED driver declare support for them
using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
implementation work.
Ansuel Smith (4):
leds: add function to configure offload leds
leds: trigger: add offload-phy-activity trigger
net: dsa: qca8k: add LEDs support
dt-bindings: net: dsa: qca8k: add LEDs definition example
Marek Behún (1):
leds: trigger: add API for HW offloading of triggers
.../devicetree/bindings/net/dsa/qca8k.yaml | 20 +
Documentation/leds/leds-class.rst | 44 +++
drivers/leds/led-triggers.c | 1 +
drivers/leds/trigger/Kconfig | 39 ++
drivers/leds/trigger/Makefile | 1 +
.../trigger/ledtrig-offload-phy-activity.c | 145 +++++++
drivers/net/dsa/Kconfig | 9 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/qca8k-leds.c | 364 ++++++++++++++++++
drivers/net/dsa/qca8k.c | 8 +-
drivers/net/dsa/qca8k.h | 64 +++
include/linux/leds.h | 85 ++++
12 files changed, 779 insertions(+), 2 deletions(-)
create mode 100644 drivers/leds/trigger/ledtrig-offload-phy-activity.c
create mode 100644 drivers/net/dsa/qca8k-leds.c
--
2.32.0
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:16
From: Marek Behún <kabel@kernel.org>
Add method trigger_offload() and member variable `offloaded` to struct
led_classdev. Add helper functions led_trigger_offload() and
led_trigger_offload_stop().
The trigger_offload() method, when implemented by the LED driver, should
be called (via led_trigger_offload() function) from trigger code wanting
to be offloaded at the moment when configuration of the trigger changes.
If the trigger is successfully offloaded, this method returns 0 and the
trigger does not have to blink the LED in software.
If the trigger with given configuration cannot be offloaded, the method
should return -EOPNOTSUPP, in which case the trigger must blink the LED
in SW.
The second argument to trigger_offload() being false means that the
offloading is being disabled. In this case the function must return 0,
errors are not permitted.
An additional config CONFIG_LEDS_OFFLOAD_TRIGGERS is added to add support
for these special trigger offload driven.
Signed-off-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
Documentation/leds/leds-class.rst | 20 +++++++++++++++++++
drivers/leds/led-triggers.c | 1 +
drivers/leds/trigger/Kconfig | 10 ++++++++++
include/linux/leds.h | 33 +++++++++++++++++++++++++++++++
4 files changed, 64 insertions(+)
@@ -169,6 +169,26 @@ Setting the brightness to zero with brightness_set() callback function should completely turn off the LED and cancel the previously programmed hardware blinking function, if any.+Hardware offloading of LED triggers+===================================++Some LEDs can be driven by hardware triggers (for example a LED connected to+an ethernet PHY or an ethernet switch can be configured to blink on activity on+the network, which in software is done by the netdev trigger).++To do such offloading, LED driver must support this and a deficated offload+trigger must be used. The LED must implement the trigger_offload() method and+the trigger code must try to call this method (via led_trigger_offload()+function) to set the LED to this particular mode. (and disable any software+blinking)++The implementation of the trigger_offload() method by the LED driver must return+0 if the offload is successful and -EOPNOTSUPP if the requested trigger+configuration is not supported.++If the second argument (enable) to the trigger_offload() method is false, any+active HW offloading must be deactivated. In this case errors are not permitted+in the trigger_offload() method and the driver will be set to the new trigger. Known Issues ============
@@ -154,6 +154,13 @@ struct led_classdev {/* LEDs that have private triggers have this set */structled_hw_trigger_type*trigger_type;++#ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS+intoffloaded;+/* some LEDs cne be driven by HW */+int(*trigger_offload)(structled_classdev*led_cdev,+boolenable);+#endif#endif#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:17
Add configure_offload helper to configure how the LED should work in
offload mode. The function require to support the particular trigger and
will use the passed offload_flags to elaborate the data and apply the
correct configuration. This function will then be used by the offload
trigger to request and update offload configuration.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
Documentation/leds/leds-class.rst | 24 ++++++++++++++++++++++++
include/linux/leds.h | 28 ++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
@@ -190,6 +190,30 @@ If the second argument (enable) to the trigger_offload() method is false, any active HW offloading must be deactivated. In this case errors are not permitted in the trigger_offload() method and the driver will be set to the new trigger.+The offload trigger will use the function configure_offload() provided by the driver+that will configure the offloaded mode for the LED.+This function passes as the first argument (offload_flags) a u32 flag.+The second argument (cmd) of the configure_offload() method can be used to do various+operations for the specific trigger. We currently support ENABLE, DISABLE, READ and+SUPPORTED to enable, disable, read the state of the offload trigger and ask the LED+driver supports the specific offload trigger.++In ENABLE/DISABLE configure_offload() should configure the LED to enable/disable the+requested trigger (flags).+In READ configure_offload() should return 0 or 1 based on the status of the requested+trigger (flags).+In SUPPORTED configure_offload() should return 0 or 1 if the LED driver supports the+requested trigger (flags) or not.++The u32 flag is specific to the trigger and change across them. It's in the LED+driver interest know how to elaborate this flag and to declare support for a+particular offload trigger. For this exact reason explicit support for the specific+trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode+with a not supported trigger.+If the driver returns -EOPNOTSUPP on configure_offload(), the trigger activation will+fail as the driver doesn't support that specific offload trigger or doesn't know+how to handle the provided flags.+ Known Issues ============
@@ -67,6 +67,15 @@ struct led_hw_trigger_type {intdummy;};+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS+enumoffload_trigger_cmd{+TRIGGER_ENABLE,/* Enable the offload trigger */+TRIGGER_DISABLE,/* Disable the offload trigger */+TRIGGER_READ,/* Read the status of the offload trigger */+TRIGGER_SUPPORTED,/* Ask the driver if the trigger is supported */+};+#endif+structled_classdev{constchar*name;unsignedintbrightness;
@@ -160,6 +169,14 @@ struct led_classdev {/* some LEDs cne be driven by HW */int(*trigger_offload)(structled_classdev*led_cdev,boolenable);+/* Function to configure how the LEDs should work in offload mode.+*Thefunctionrequiretosupportthetriggerandwillusethe+*passedflagstoelaboratethetriggerrequestedandapplythe+*correctconfiguration.+*/+int(*configure_offload)(structled_classdev*led_cdev,+u32offload_flags,+enumoffload_trigger_cmdcmd);#endif#endif
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:20
Add Offload Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each offload trigger can be configured by sysfs and on trigger
activation the offload mode is enabled.
This currently implement these hw triggers:
- blink_tx: Blink LED on tx packet receive
- blink_rx: Blink LED on rx packet receive
- blink_collision: Blink LED on collision detection
- link_10m: Keep LED on with 10m link speed
- link_100m: Keep LED on with 100m link speed
- link_1000m: Keep LED on with 1000m link speed
- half_duplex: Keep LED on with half duplex link
- full_duplex: Keep LED on with full duplex link
- linkup_over: Keep LED on with link speed and blink on rx/tx traffic
- power_on_reset: Keep LED on with switch reset
- blink_2hz: Set blink speed at 2hz for every blink event
- blink_4hz: Set blink speed at 4hz for every blink event
- blink_8hz: Set blink speed at 8hz for every blink event
- blink_auto: Set blink speed at 2hz for 10m link speed,
4hz for 100m and 8hz for 1000m
The trigger will check if the LED driver support the offload trigger and
will expose the offload triggers in sysfs.
It will finally enable hw mode for the LED without configuring any rule.
This mean that the LED will blink/follow whatever offload trigger is
active by default and the user needs to manually configure the desired
offload triggers using sysfs.
A flag is passed to configure_offload with the related rule from this
trigger to active or disable.
It's in the led driver interest the detection and knowing how to
elaborate the passed flags and should report -EOPNOTSUPP otherwise.
The different hw triggers are exposed in the led sysfs dir under the
offload-phy-activity subdir.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
drivers/leds/trigger/Kconfig | 29 ++++
drivers/leds/trigger/Makefile | 1 +
.../trigger/ledtrig-offload-phy-activity.c | 145 ++++++++++++++++++
include/linux/leds.h | 24 +++
4 files changed, 199 insertions(+)
create mode 100644 drivers/leds/trigger/ledtrig-offload-phy-activity.c
@@ -0,0 +1,145 @@+// SPDX-License-Identifier: GPL-2.0-only++#include<linux/kernel.h>+#include<linux/init.h>+#include<linux/leds.h>+#include<linux/slab.h>++#define PHY_ACTIVITY_MAX_TRIGGERS 14++#define DEFINE_OFFLOAD_TRIGGER(trigger_name, trigger) \+staticssize_ttrigger_name##_show(structdevice*dev,\+structdevice_attribute*attr,char*buf)\+{\+structled_classdev*led_cdev=led_trigger_get_led(dev);\+intval;\+val=led_cdev->configure_offload(led_cdev,trigger,TRIGGER_READ);\+returnsprintf(buf,"%d\n",val?1:0);\+}\+staticssize_ttrigger_name##_store(structdevice*dev,\+structdevice_attribute*attr,\+constchar*buf,size_tsize)\+{\+structled_classdev*led_cdev=led_trigger_get_led(dev);\+unsignedlongstate;\+intcmd,ret;\+ret=kstrtoul(buf,0,&state);\+if(ret)\+returnret;\+cmd=!!state?TRIGGER_ENABLE:TRIGGER_DISABLE;\+/* Update the configuration with every change */\+led_cdev->configure_offload(led_cdev,trigger,cmd);\+returnsize;\+}\+DEVICE_ATTR_RW(trigger_name)++/* Expose sysfs for every blink to be configurable from userspace */+DEFINE_OFFLOAD_TRIGGER(blink_tx,BLINK_TX);+DEFINE_OFFLOAD_TRIGGER(blink_rx,BLINK_RX);+DEFINE_OFFLOAD_TRIGGER(blink_collision,BLINK_COLLISION);+DEFINE_OFFLOAD_TRIGGER(keep_link_10m,KEEP_LINK_10M);+DEFINE_OFFLOAD_TRIGGER(keep_link_100m,KEEP_LINK_100M);+DEFINE_OFFLOAD_TRIGGER(keep_link_1000m,KEEP_LINK_1000M);+DEFINE_OFFLOAD_TRIGGER(keep_half_duplex,KEEP_HALF_DUPLEX);+DEFINE_OFFLOAD_TRIGGER(keep_full_duplex,KEEP_FULL_DUPLEX);+DEFINE_OFFLOAD_TRIGGER(option_linkup_over,OPTION_LINKUP_OVER);+DEFINE_OFFLOAD_TRIGGER(option_power_on_reset,OPTION_POWER_ON_RESET);+DEFINE_OFFLOAD_TRIGGER(option_blink_2hz,OPTION_BLINK_2HZ);+DEFINE_OFFLOAD_TRIGGER(option_blink_4hz,OPTION_BLINK_4HZ);+DEFINE_OFFLOAD_TRIGGER(option_blink_8hz,OPTION_BLINK_8HZ);+DEFINE_OFFLOAD_TRIGGER(option_blink_auto,OPTION_BLINK_AUTO);++/* The attrs will be placed dynamically based on the supported triggers */+staticstructattribute*phy_activity_attrs[PHY_ACTIVITY_MAX_TRIGGERS+1];++staticintoffload_phy_activity_activate(structled_classdev*led_cdev)+{+inti,trigger,ret;++/* This triggered is supported only by LEDs that can be offload driven */+if(!led_cdev->trigger_offload||!led_cdev->trigger_offload)+return-EOPNOTSUPP;++/* Scan the supported offload triggers and expose them in sysfs if supported */+for(trigger=0,i=0;trigger<PHY_ACTIVITY_MAX_TRIGGERS;trigger++){+if(led_trigger_offload_is_supported(led_cdev,BLINK_TX))+phy_activity_attrs[i++]=&dev_attr_blink_tx.attr;++if(led_trigger_offload_is_supported(led_cdev,BLINK_RX))+phy_activity_attrs[i++]=&dev_attr_blink_rx.attr;++if(led_trigger_offload_is_supported(led_cdev,BLINK_COLLISION))+phy_activity_attrs[i++]=&dev_attr_blink_collision.attr;++if(led_trigger_offload_is_supported(led_cdev,KEEP_LINK_10M))+phy_activity_attrs[i++]=&dev_attr_keep_link_10m.attr;++if(led_trigger_offload_is_supported(led_cdev,KEEP_LINK_100M))+phy_activity_attrs[i++]=&dev_attr_keep_link_100m.attr;++if(led_trigger_offload_is_supported(led_cdev,KEEP_LINK_1000M))+phy_activity_attrs[i++]=&dev_attr_keep_link_1000m.attr;++if(led_trigger_offload_is_supported(led_cdev,KEEP_HALF_DUPLEX))+phy_activity_attrs[i++]=&dev_attr_keep_half_duplex.attr;++if(led_trigger_offload_is_supported(led_cdev,KEEP_FULL_DUPLEX))+phy_activity_attrs[i++]=&dev_attr_keep_full_duplex.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_LINKUP_OVER))+phy_activity_attrs[i++]=&dev_attr_option_linkup_over.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_POWER_ON_RESET))+phy_activity_attrs[i++]=&dev_attr_option_power_on_reset.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_BLINK_2HZ))+phy_activity_attrs[i++]=&dev_attr_option_blink_2hz.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_BLINK_4HZ))+phy_activity_attrs[i++]=&dev_attr_option_blink_4hz.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_BLINK_8HZ))+phy_activity_attrs[i++]=&dev_attr_option_blink_8hz.attr;++if(led_trigger_offload_is_supported(led_cdev,OPTION_BLINK_AUTO))+phy_activity_attrs[i++]=&dev_attr_option_blink_auto.attr;+}++/* Enable offload mode. No custom configuration is applied,+*theLEDdriverwillusewhateverdefaultconfigurationis+*currentlyconfigured.+*/+ret=led_cdev->trigger_offload(led_cdev,true);+if(ret)+returnret;++return0;+}++staticvoidoffload_phy_activity_deactivate(structled_classdev*led_cdev)+{+led_cdev->trigger_offload(led_cdev,false);+}++staticconststructattribute_groupphy_activity_group={+.name="offload-phy-activity",+.attrs=phy_activity_attrs,+};++staticconststructattribute_group*phy_activity_groups[]={+&phy_activity_group,+NULL,+};++staticstructled_triggeroffload_phy_activity_trigger={+.name="offload-phy-activity",+.activate=offload_phy_activity_activate,+.deactivate=offload_phy_activity_deactivate,+.groups=phy_activity_groups,+};++staticint__initoffload_phy_activity_init(void)+{+returnled_trigger_register(&offload_phy_activity_trigger);+}+device_initcall(offload_phy_activity_init);
@@ -546,6 +546,30 @@ static inline void ledtrig_flash_ctrl(bool on) {}staticinlinevoidledtrig_torch_ctrl(boolon){}#endif+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGER_PHY_ACTIVITY+/* 3 main category for offload trigger:+*-blink:theledwillblinkontrigger+*-keep:theledwillbekeptonwithcondition+*-option:aconfigurationvalueinternaltotheledonhowoffloadworks+*/+enumoffload_phy_activity{+BLINK_TX=BIT(0),/* Blink on TX traffic */+BLINK_RX=BIT(1),/* Blink on RX traffic */+BLINK_COLLISION=BIT(2),/* Blink on Collision packet */+KEEP_LINK_10M=BIT(3),/* LED on with 10M link */+KEEP_LINK_100M=BIT(4),/* LED on with 100M link */+KEEP_LINK_1000M=BIT(5),/* LED on with 1000M link */+KEEP_HALF_DUPLEX=BIT(6),/* LED on with Half-Duplex link */+KEEP_FULL_DUPLEX=BIT(7),/* LED on with Full-Duplex link */+OPTION_LINKUP_OVER=BIT(8),/* LED will be on with KEEP offload_triggers and blink on BLINK traffic */+OPTION_POWER_ON_RESET=BIT(9),/* LED will be on Switch reset */+OPTION_BLINK_2HZ=BIT(10),/* LED will blink with any offload_trigger at 2Hz */+OPTION_BLINK_4HZ=BIT(11),/* LED will blink with any offload_trigger at 4Hz */+OPTION_BLINK_8HZ=BIT(12),/* LED will blink with any offload_trigger at 8Hz */+OPTION_BLINK_AUTO=BIT(13),/* LED will blink differently based on the Link Speed */+};+#endif+/**GenericLEDplatformdatafordescribingLEDnamesanddefaulttriggers.*/
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:22
Add LEDs support for qca8k Switch Family. This will provide the LEDs
offload API to permit the PHY LED to be driven offloaded.
Each port have at least 3 LEDs and they can HW blink, set on/off or
set to be driven by some rules applied by the offload trigger.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
drivers/net/dsa/Kconfig | 9 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/qca8k-leds.c | 364 +++++++++++++++++++++++++++++++++++
drivers/net/dsa/qca8k.c | 8 +-
drivers/net/dsa/qca8k.h | 64 ++++++
5 files changed, 444 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/dsa/qca8k-leds.c
@@ -0,0 +1,364 @@+// SPDX-License-Identifier: GPL-2.0+#include<net/dsa.h>++#include"qca8k.h"++staticint+qca8k_get_enable_led_reg(intport_num,intled_num,structqca8k_led_pattern_en*reg_info)+{+intshift;++switch(port_num){+case0:+reg_info->reg=QCA8K_LED_CTRL_REG(led_num);+reg_info->shift=14;+break;+case1:+case2:+case3:+reg_info->reg=QCA8K_LED_CTRL_REG(3);+shift=2*led_num+(6*(port_num-1));++reg_info->shift=8+shift;++break;+case4:+reg_info->reg=QCA8K_LED_CTRL_REG(led_num);+reg_info->shift=30;+break;+default:+return-EINVAL;+}++return0;+}++staticint+qca8k_get_control_led_reg(intport_num,intled_num,structqca8k_led_pattern_en*reg_info)+{+reg_info->reg=QCA8K_LED_CTRL_REG(led_num);++/* 6 total control rule:+*3controlrulesforphy0-3thatappliestoalltheirleds+*3controlrulesforphy4+*/+if(port_num==4)+reg_info->shift=16;+else+reg_info->shift=0;++return0;+}++staticint+qca8k_cled_configure_offload(structled_classdev*ldev,u32rules,enumoffload_trigger_cmdcmd)+{+structqca8k_led*led=container_of(ldev,structqca8k_led,cdev);+structled_trigger*trigger=ldev->trigger;+structqca8k_led_pattern_enreg_info;+structqca8k_priv*priv=led->priv;+u32offload_trigger=0,mask,val;+intret;++/* Check trigger compatibility */+if(strcmp(trigger->name,"offload-phy-activity"))+return-EOPNOTSUPP;++switch(rules){+caseBLINK_TX:+offload_trigger=QCA8K_LED_TX_BLINK_MASK;+break;+caseBLINK_RX:+offload_trigger=QCA8K_LED_RX_BLINK_MASK;+break;+caseBLINK_COLLISION:+offload_trigger=QCA8K_LED_COL_BLINK_MASK;+break;+caseKEEP_LINK_10M:+offload_trigger=QCA8K_LED_LINK_10M_EN_MASK;+break;+caseKEEP_LINK_100M:+offload_trigger=QCA8K_LED_LINK_100M_EN_MASK;+break;+caseKEEP_LINK_1000M:+offload_trigger=QCA8K_LED_LINK_1000M_EN_MASK;+break;+caseKEEP_HALF_DUPLEX:+offload_trigger=QCA8K_LED_HALF_DUPLEX_MASK;+break;+caseKEEP_FULL_DUPLEX:+offload_trigger=QCA8K_LED_FULL_DUPLEX_MASK;+break;+caseOPTION_LINKUP_OVER:+offload_trigger=QCA8K_LED_LINKUP_OVER_MASK;+break;+caseOPTION_BLINK_2HZ:+offload_trigger=QCA8K_LED_BLINK_2HZ;+break;+caseOPTION_BLINK_4HZ:+offload_trigger=QCA8K_LED_BLINK_4HZ;+break;+caseOPTION_BLINK_8HZ:+offload_trigger=QCA8K_LED_BLINK_8HZ;+break;+caseOPTION_BLINK_AUTO:+offload_trigger=QCA8K_LED_BLINK_AUTO;+break;+default:+return-EOPNOTSUPP;+}++if(rules==OPTION_BLINK_2HZ||rules==OPTION_BLINK_4HZ||+rules==OPTION_BLINK_8HZ||rules==OPTION_BLINK_AUTO){+mask=QCA8K_LED_BLINK_FREQ_MASK;+}else{+mask=offload_trigger;+}++qca8k_get_control_led_reg(led->port_num,led->led_num,®_info);++switch(cmd){+caseTRIGGER_SUPPORTED:+/* We reache this point, we are sure the trigger is supported */+return1;+caseTRIGGER_ENABLE:+ret=qca8k_rmw(priv,reg_info.reg,+mask<<reg_info.shift,+offload_trigger<<reg_info.shift);+break;+caseTRIGGER_DISABLE:+ret=qca8k_rmw(priv,reg_info.reg,+mask<<reg_info.shift,+0);+break;+caseTRIGGER_READ:+ret=qca8k_read(priv,reg_info.reg,&val);+if(ret)+returnret;++val>>=reg_info.shift;+val&=offload_trigger;++/* Special handling for LED_BLINK_2HZ */+if(!val&&offload_trigger==QCA8K_LED_BLINK_2HZ)+val=1;++returnval;+default:+return-EOPNOTSUPP;+}++returnret;+}++staticvoid+qca8k_led_brightness_set(structqca8k_led*led,+enumled_brightnessb)+{+structqca8k_led_pattern_enreg_info;+structqca8k_priv*priv=led->priv;+u32val=QCA8K_LED_ALWAYS_OFF;++qca8k_get_enable_led_reg(led->port_num,led->led_num,®_info);++if(b)+val=QCA8K_LED_ALWAYS_ON;++qca8k_rmw(priv,reg_info.reg,+GENMASK(1,0)<<reg_info.shift,+val<<reg_info.shift);+}++staticvoid+qca8k_cled_brightness_set(structled_classdev*ldev,+enumled_brightnessb)+{+structqca8k_led*led=container_of(ldev,structqca8k_led,cdev);++returnqca8k_led_brightness_set(led,b);+}++staticenumled_brightness+qca8k_led_brightness_get(structqca8k_led*led)+{+structqca8k_led_pattern_enreg_info;+structqca8k_priv*priv=led->priv;+u32val;+intret;++qca8k_get_enable_led_reg(led->port_num,led->led_num,®_info);++ret=qca8k_read(priv,reg_info.reg,&val);+if(ret)+return0;++val>>=reg_info.shift;+val&=GENMASK(1,0);++returnval>0?1:0;+}++staticenumled_brightness+qca8k_cled_brightness_get(structled_classdev*ldev)+{+structqca8k_led*led=container_of(ldev,structqca8k_led,cdev);++returnqca8k_led_brightness_get(led);+}++staticint+qca8k_cled_blink_set(structled_classdev*ldev,+unsignedlong*delay_on,+unsignedlong*delay_off)+{+structqca8k_led*led=container_of(ldev,structqca8k_led,cdev);+structqca8k_led_pattern_enreg_info;+structqca8k_priv*priv=led->priv;++if(*delay_on==0&&*delay_off==0){+*delay_on=125;+*delay_off=125;+}++if(*delay_on!=125||*delay_off!=125){+/* The hardware only supports blinking at 4Hz. Fall back+*tosoftwareimplementationinothercases.+*/+return-EINVAL;+}++qca8k_get_enable_led_reg(led->port_num,led->led_num,®_info);++qca8k_rmw(priv,reg_info.reg,+GENMASK(1,0)<<reg_info.shift,+QCA8K_LED_ALWAYS_BLINK_4HZ<<reg_info.shift);++return0;+}++staticint+qca8k_cled_trigger_offload(structled_classdev*ldev,boolenable)+{+structqca8k_led*led=container_of(ldev,structqca8k_led,cdev);++structqca8k_led_pattern_enreg_info;+structqca8k_priv*priv=led->priv;+u32val=QCA8K_LED_ALWAYS_OFF;++qca8k_get_enable_led_reg(led->port_num,led->led_num,®_info);++if(enable)+val=QCA8K_LED_RULE_CONTROLLED;++returnqca8k_rmw(priv,reg_info.reg,+GENMASK(1,0)<<reg_info.shift,+val<<reg_info.shift);+}++staticint+qca8k_parse_port_leds(structqca8k_priv*priv,structfwnode_handle*port,intport_num)+{+structfwnode_handle*led=NULL,*leds=NULL;+structled_init_datainit_data={};+structqca8k_led*port_led;+intled_num,port_index;+constchar*state;+intret;++leds=fwnode_get_named_child_node(port,"leds");+if(!leds){+dev_dbg(priv->dev,"No Leds node specified in device tree for port %d!\n",+port_num);+return0;+}++fwnode_for_each_child_node(leds,led){+/* Reg rapresent the led number of the port.+*Eachportcanhaveatleast3ledsattached+*Commonly:+*1.isgigabitled+*2.ismbitled+*3.additionalstatusled+*/+if(fwnode_property_read_u32(led,"reg",&led_num))+continue;++if(led_num>=QCA8K_LED_PORT_COUNT){+dev_warn(priv->dev,"Invalid LED reg defined %d",port_num);+continue;+}++port_index=3*port_num+led_num;++port_led=&priv->ports_led[port_index];+port_led->port_num=port_num;+port_led->led_num=led_num;+port_led->priv=priv;++ret=fwnode_property_read_string(led,"default-state",&state);+if(!ret){+if(!strcmp(state,"on")){+port_led->cdev.brightness=1;+qca8k_led_brightness_set(port_led,1);+}elseif(!strcmp(state,"off")){+port_led->cdev.brightness=0;+qca8k_led_brightness_set(port_led,0);+}elseif(!strcmp(state,"keep")){+port_led->cdev.brightness=+qca8k_led_brightness_get(port_led);+}+}++/* 3 brightness settings can be applied from Documentation:+*0alwaysoff+*1blinkat4Hz+*2alwayson+*3rulecontrolled+*Suppotsonly2mode:(pcblimitation,withalwaysonandblink+*onlythelastledissettothismode)+*0alwaysoff(setsallledsoff)+*3rulecontrolled+*/+port_led->cdev.max_brightness=1;+port_led->cdev.brightness_set=qca8k_cled_brightness_set;+port_led->cdev.brightness_get=qca8k_cled_brightness_get;+port_led->cdev.blink_set=qca8k_cled_blink_set;+port_led->cdev.configure_offload=qca8k_cled_configure_offload;+port_led->cdev.trigger_offload=qca8k_cled_trigger_offload;+init_data.default_label=":port";+init_data.devicename="qca8k";+init_data.fwnode=led;++ret=devm_led_classdev_register_ext(priv->dev,&port_led->cdev,&init_data);+if(ret)+dev_warn(priv->dev,"Failed to int led");+}++return0;+}++int+qca8k_setup_led_ctrl(structqca8k_priv*priv)+{+structfwnode_handle*mdio,*port;+intport_num;+intret;++mdio=device_get_named_child_node(priv->dev,"mdio");+if(!mdio){+dev_info(priv->dev,"No MDIO node specified in device tree!\n");+return0;+}++fwnode_for_each_child_node(mdio,port){+if(fwnode_property_read_u32(port,"reg",&port_num))+continue;++/* Each port can have at least 3 different leds attached */+ret=qca8k_parse_port_leds(priv,port,port_num);+if(ret)+returnret;+}++return0;+}
@@ -308,4 +359,17 @@ struct qca8k_fdb {u8mac[6];};+/* Common function used by the LEDs module */+intqca8k_read(structqca8k_priv*priv,u32reg,u32*val);+intqca8k_rmw(structqca8k_priv*priv,u32reg,u32mask,u32write_val);++#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT+intqca8k_setup_led_ctrl(structqca8k_priv*priv);+#else+staticinlineintqca8k_setup_led_ctrl(structqca8k_priv*priv)+{+return0;+}+#endif+#endif /* __QCA8K_H */
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 00:25:26
Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
.../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
@@ -64,6 +64,8 @@ properties:internal mdio access is used.With the legacy mapping the reg corresponding to the internalmdio is the switch reg with an offset of -1.+Each phy have at least 3 LEDs connected and can be declared+using the standard LEDs structure.properties:'#address-cells':
@@ -190,6 +190,30 @@ If the second argument (enable) to the trigger_offload() method is false, any active HW offloading must be deactivated. In this case errors are not permitted in the trigger_offload() method and the driver will be set to the new trigger.+The offload trigger will use the function configure_offload() provided by the driver+that will configure the offloaded mode for the LED.+This function passes as the first argument (offload_flags) a u32 flag.+The second argument (cmd) of the configure_offload() method can be used to do various+operations for the specific trigger. We currently support ENABLE, DISABLE, READ and+SUPPORTED to enable, disable, read the state of the offload trigger and ask the LED+driver supports the specific offload trigger.++In ENABLE/DISABLE configure_offload() should configure the LED to enable/disable the+requested trigger (flags).+In READ configure_offload() should return 0 or 1 based on the status of the requested+trigger (flags).+In SUPPORTED configure_offload() should return 0 or 1 if the LED driver supports the+requested trigger (flags) or not.++The u32 flag is specific to the trigger and change across them. It's in the LED
changes
+driver interest know how to elaborate this flag and to declare support for a
driver's
+particular offload trigger. For this exact reason explicit support for the specific
+trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
+with a not supported trigger.
+If the driver returns -EOPNOTSUPP on configure_offload(), the trigger activation will
+fail as the driver doesn't support that specific offload trigger or doesn't know
+how to handle the provided flags.
@@ -169,6 +169,26 @@ Setting the brightness to zero with brightness_set() callback function should completely turn off the LED and cancel the previously programmed hardware blinking function, if any.+Hardware offloading of LED triggers+===================================++Some LEDs can be driven by hardware triggers (for example a LED connected to+an ethernet PHY or an ethernet switch can be configured to blink on activity on+the network, which in software is done by the netdev trigger).++To do such offloading, LED driver must support this and a deficated offload
dedicated
+trigger must be used. The LED must implement the trigger_offload() method and
+the trigger code must try to call this method (via led_trigger_offload()
+function) to set the LED to this particular mode. (and disable any software
+blinking)
+
+The implementation of the trigger_offload() method by the LED driver must return
+0 if the offload is successful and -EOPNOTSUPP if the requested trigger
+configuration is not supported.
+
+If the second argument (enable) to the trigger_offload() method is false, any
+active HW offloading must be deactivated. In this case errors are not permitted
Preferably s/HW/hardware/ and s/SW/software/ throughout documentation and Kconfig
files.
+in the trigger_offload() method and the driver will be set to the new trigger.
Known Issues
============
+ can be driven in hardware by declaring some specific triggers.
+ A offload trigger will expose a sysfs dir to configure the
+ different blinking trigger and the available hardware triggers.
+
+ If unsure, say Y.
I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().
There are also a number of PHYs which don't allow software blinking of
the LED. So for them, trigger_offload_stop() is going to return
-EOPNOTSUPP. And you need to handle that correctly.
It would be go to also document the expectations of
trigger_offload_stop(). Should it leave the LED in whatever state it
was, or force it off?
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-11-08 14:17:46
On Mon, Nov 08, 2021 at 01:24:58AM +0100, Ansuel Smith wrote:
Add Offload Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each offload trigger can be configured by sysfs and on trigger
activation the offload mode is enabled.
This currently implement these hw triggers:
- blink_tx: Blink LED on tx packet receive
- blink_rx: Blink LED on rx packet receive
- blink_collision: Blink LED on collision detection
When did you last see a collision? Do you really have a 1/2 duplex
link? Just because the PHY can, does not mean we should support
it. Lets restrict this to the most useful modes.
- link_10m: Keep LED on with 10m link speed
- link_100m: Keep LED on with 100m link speed
- link_1000m: Keep LED on with 1000m link speed
- half_duplex: Keep LED on with half duplex link
- full_duplex: Keep LED on with full duplex link
- linkup_over: Keep LED on with link speed and blink on rx/tx traffic
- power_on_reset: Keep LED on with switch reset
- blink_2hz: Set blink speed at 2hz for every blink event
- blink_4hz: Set blink speed at 4hz for every blink event
- blink_8hz: Set blink speed at 8hz for every blink event
These seems like attributes, not blink modes. They need to be
specified somehow differently, or not at all. Do we really need them?
- blink_auto: Set blink speed at 2hz for 10m link speed,
4hz for 100m and 8hz for 1000m
Another attribute, and one i've not seen any other PHY do.
Andrew
I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().
To not add too much function to the struct, can we introduce one
function that both enable and disable the hw mode?
There are also a number of PHYs which don't allow software blinking of
the LED. So for them, trigger_offload_stop() is going to return
-EOPNOTSUPP. And you need to handle that correctly.
So we have PHYs that can only work in offload or off. Correct?
It would be go to also document the expectations of
trigger_offload_stop(). Should it leave the LED in whatever state it
was, or force it off?
I think it should be put off. Do you agree? (also the brightness should
be set to 0 in this case)
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 15:19:21
On Mon, Nov 08, 2021 at 03:17:33PM +0100, Andrew Lunn wrote:
On Mon, Nov 08, 2021 at 01:24:58AM +0100, Ansuel Smith wrote:
quoted
Add Offload Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each offload trigger can be configured by sysfs and on trigger
activation the offload mode is enabled.
This currently implement these hw triggers:
- blink_tx: Blink LED on tx packet receive
- blink_rx: Blink LED on rx packet receive
- blink_collision: Blink LED on collision detection
When did you last see a collision? Do you really have a 1/2 duplex
link? Just because the PHY can, does not mean we should support
it. Lets restrict this to the most useful modes.
Ok will drop this. In my case (qca8k) I also never see a device using it
so I agree on the fact that should be dropped.
quoted
- link_10m: Keep LED on with 10m link speed
- link_100m: Keep LED on with 100m link speed
- link_1000m: Keep LED on with 1000m link speed
- half_duplex: Keep LED on with half duplex link
- full_duplex: Keep LED on with full duplex link
- linkup_over: Keep LED on with link speed and blink on rx/tx traffic
- power_on_reset: Keep LED on with switch reset
quoted
- blink_2hz: Set blink speed at 2hz for every blink event
- blink_4hz: Set blink speed at 4hz for every blink event
- blink_8hz: Set blink speed at 8hz for every blink event
These seems like attributes, not blink modes. They need to be
specified somehow differently, or not at all. Do we really need them?
Sorry I didn't update the commit. In sysfs they are exposed as option
like the power_on_reset and linkup_over. So they are option on how the
LED behave on the event.
quoted
- blink_auto: Set blink speed at 2hz for 10m link speed,
4hz for 100m and 8hz for 1000m
Another attribute, and one i've not seen any other PHY do.
Yes we can consider dropping this but I think the other 3 should be
keeped.
I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().
To not add too much function to the struct, can we introduce one
function that both enable and disable the hw mode?
Dear Ansuel,
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().
To not add too much function to the struct, can we introduce one
function that both enable and disable the hw mode?
Dear Ansuel,
(just to make sure, I don't want to look rude, I just want this feature
finally introduced and supported since AFAIK many tried to add support
for LEDs in PHY but everyone failed. For a reason or another, too
specific, not generic...)
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
But the final step was adding LEDs support for PHY. The idea was to find
a clean way by passing the idea of offloading a trigger... But fact is
that LEDs in PHY operate by themself so it would add overhead selecting
a trigger that will use event/works just to then be ignored as the
trigger is offloaded.
Also I think we are missing the fact that most of PHY can operate in
software or in hardware. NOT BOTH. So the entire concept of offloading
some trigger won't work as we were not able to simulate with SW
unsupported trigger. (not the case with netdev as it does only support
link, rx/tx but that was to explain the concept that SW and HW mode are
mutually exclusive.)
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
Again the idea here is that a LED can offer a way to run by HW and then
a trigger configure them and enables the mode. I see the offload and
configure function needed anyway side from the implementation.
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
Can we try to find a common way to introduce this?
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
Can't be done. If in HW mode, we should just declare a trigger not
supported in SW if we really want to follow the netdev expanding path.
But still that would mean filling the netdev trigger with extra code and
condition that will only apply in HW mode. At this point just create a
dedicated trigger.
We can consider introducing the same sysfs used by netdev trigger but
nothing else.
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
How would this work in HW mode? The PHY blink only with packet in his
port. We can't tell the PHY to HW blink based on an interface.
That is the main problem by using netdev for PHYs, netdev is flexible
but an offload trigger is not and would work only based on some
condition. We should hardcode the device_name in HW and again pollute
the netdev trigger more.
An idea would be add tons of check to netdev on every event to check the
interface but that wouldn't go against any offload idea? The system will
be loaded anyway with all these checks.
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
The rest of the implementation is very similar. Except we just NOT use
netdev. And to me it does seems the most sane way to handle offload for
a LED. (considering the specific situation of a PHY)
From what I can see we have 2 path:
- Pollute netdev trigger to add entire different function for HW. (that
will end up in complex condition and more load/overhead)
- Introduce a new way to entirely offload some triggers.
Could be that here I'm just using the wrong word and I should use
hardware instead offload. But considering we are offloading some trigger
(example rx/tx/link) it's not that wrong.
The thing is why trying to expand a trigger that will just remove some
flexibility when we can solve the problem at the source with some
additional API that currently we lack any support (leds can be
configured to run by hw) and some dedicated trigger that will do the
task in a cleaner way (and without adding extra load/overhead by really
offloading the task) ?
Again hope I didn't seem rude in this message but I just want to find a
solution and proposing a new idea/explaining my concern.
--
Ansuel
From: Marek Behún <kabel@kernel.org> Date: 2021-11-08 17:35:48
Dear Ansuel,
On Mon, 8 Nov 2021 17:46:02 +0100
Ansuel Smith [off-list ref] wrote:
quoted
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
But the final step was adding LEDs support for PHY. The idea was to find
a clean way by passing the idea of offloading a trigger... But fact is
that LEDs in PHY operate by themself so it would add overhead selecting
a trigger that will use event/works just to then be ignored as the
trigger is offloaded.
I don't understand what kind of overhead in events/works are you
talking about. If the trigger is successfully offloaded, there are no
events/works. Everything is done in hardware. If you look at my last
attempt
https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
which adds the offloading support to netdev trigger, you can see
if (!led_trigger_offload(led_cdev))
return;
so if the trigger is succesfully offloaded, the subsequen
schedule_delayed_work() is not called and netdev trigger does not do anything.
The blinking of the LED on rx/tx activity is done purely in hardware.
No software overhead.
Also I think we are missing the fact that most of PHY can operate in
software or in hardware. NOT BOTH. So the entire concept of offloading
some trigger won't work as we were not able to simulate with SW
unsupported trigger. (not the case with netdev as it does only support
link, rx/tx but that was to explain the concept that SW and HW mode are
mutually exclusive.)
I am not missing this fact, I know that there are LEDs that cannot be
set into purely SW mode. Which brings another problem: Pavel currely
isn't convinced that these LEDs should be exported via LED classdev
API. We need to persuade him about this, because otherwise we would
need to create another subsystem for this.
But there are PHYs which do allow purely SW LED control, i.e. I can set
the LED to be just ON or OFF. These are for example marvell PHYs.
The fact that there are LEDs that can't be controlled purely in SW does
not seem a good reason for to not implement it via netdev trigger.
Either these LEDs shouldn't be exported as LED classdevs, or they
should somehow make use of existing trigger API (so netdev).
The fact that the LED cannot be controlled in SW can be simply
implemented by refusing to disable the netdev trigger on the LED.
quoted
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
Again the idea here is that a LED can offer a way to run by HW and then
a trigger configure them and enables the mode. I see the offload and
configure function needed anyway side from the implementation.
There is already LED private trigger API for this. You don't need
offloading of existing trigger if you are going to create a new trigger
anyway.
quoted
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
Can we try to find a common way to introduce this?
quoted
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
Can't be done. If in HW mode, we should just declare a trigger not
supported in SW if we really want to follow the netdev expanding path.
But still that would mean filling the netdev trigger with extra code and
condition that will only apply in HW mode. At this point just create a
dedicated trigger.
The netdev trigger already needs to be expanded with extra code: it
doesn't allow indicating different link types, for example.
For offloading, the extra code can be quite small. So I don't think
this is an argument.
We can consider introducing the same sysfs used by netdev trigger but
nothing else.
Not true.
quoted
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
How would this work in HW mode? The PHY blink only with packet in his
port. We can't tell the PHY to HW blink based on an interface.
That is the main problem by using netdev for PHYs, netdev is flexible
but an offload trigger is not and would work only based on some
condition. We should hardcode the device_name in HW and again pollute
the netdev trigger more.
netdev trigger does not need to be poluted by this much. All this can be
implemented by one callback to offload method, and everything else can
be done in the LED driver itself.
An idea would be add tons of check to netdev on every event to check the
interface but that wouldn't go against any offload idea? The system will
be loaded anyway with all these checks.
The checks are done only at the moment of configuring the trigger.
Surely you don't see that as "loaded system".
quoted
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
The rest of the implementation is very similar. Except we just NOT use
netdev. And to me it does seems the most sane way to handle offload for
a LED. (considering the specific situation of a PHY)
From what I can see we have 2 path:
- Pollute netdev trigger to add entire different function for HW. (that
will end up in complex condition and more load/overhead)
- Introduce a new way to entirely offload some triggers.
The check conditions would be all in LED driver (PHY driver in this
case). The code could be a little complicated, but it only needs to be
written once, and the PHY drivers can reuse it. netdev trigger is not
polluted, only a few calls to offload() method are done.
Could be that here I'm just using the wrong word and I should use
hardware instead offload. But considering we are offloading some trigger
(example rx/tx/link) it's not that wrong.
The thing is why trying to expand a trigger that will just remove some
flexibility when we can solve the problem at the source with some
additional API that currently we lack any support (leds can be
configured to run by hw) and some dedicated trigger that will do the
task in a cleaner way (and without adding extra load/overhead by really
offloading the task) ?
As I explained above, there is no extra load/overhead. The only thing
that is extra is that netdev trigger gets a little more new code, but
that is already needed anyway.
Again hope I didn't seem rude in this message but I just want to find a
solution and proposing a new idea/explaining my concern.
I don't think you're rude, we are just discussing a topic for which we
have different opinions.
Marek
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-11-08 17:47:01
Dear Ansuel,
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
Hi Marek
The problem here is, you are not making much progress. People are
giving up on you ever getting this done, and doing their own
implementation. Ansuel code is not mature enough yet, it has problems,
but he is responsive, he is dealing with comments, progress is being
made. At some point, it is going to be good enough, and it will get
merged, unless you actual get your code to a point it can be merged.
Andrew
From: Marek Behún <kabel@kernel.org> Date: 2021-11-08 17:56:45
On Mon, 8 Nov 2021 18:46:26 +0100
Andrew Lunn [off-list ref] wrote:
quoted
Dear Ansuel,
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
Hi Marek
The problem here is, you are not making much progress. People are
giving up on you ever getting this done, and doing their own
implementation. Ansuel code is not mature enough yet, it has problems,
but he is responsive, he is dealing with comments, progress is being
made. At some point, it is going to be good enough, and it will get
merged, unless you actual get your code to a point it can be merged.
Andrew
Hello Andrew,
you are right that this has been taking too long on my side. I am sorry
for that.
I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.
Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?
Marek
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 17:58:48
On Mon, Nov 08, 2021 at 06:35:37PM +0100, Marek Behún wrote:
Dear Ansuel,
On Mon, 8 Nov 2021 17:46:02 +0100
Ansuel Smith [off-list ref] wrote:
quoted
quoted
what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.
But the final step was adding LEDs support for PHY. The idea was to find
a clean way by passing the idea of offloading a trigger... But fact is
that LEDs in PHY operate by themself so it would add overhead selecting
a trigger that will use event/works just to then be ignored as the
trigger is offloaded.
I don't understand what kind of overhead in events/works are you
talking about. If the trigger is successfully offloaded, there are no
events/works. Everything is done in hardware. If you look at my last
attempt
https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
which adds the offloading support to netdev trigger, you can see
if (!led_trigger_offload(led_cdev))
return;
so if the trigger is succesfully offloaded, the subsequen
schedule_delayed_work() is not called and netdev trigger does not do anything.
The blinking of the LED on rx/tx activity is done purely in hardware.
No software overhead.
quoted
Also I think we are missing the fact that most of PHY can operate in
software or in hardware. NOT BOTH. So the entire concept of offloading
some trigger won't work as we were not able to simulate with SW
unsupported trigger. (not the case with netdev as it does only support
link, rx/tx but that was to explain the concept that SW and HW mode are
mutually exclusive.)
I am not missing this fact, I know that there are LEDs that cannot be
set into purely SW mode. Which brings another problem: Pavel currely
isn't convinced that these LEDs should be exported via LED classdev
API. We need to persuade him about this, because otherwise we would
need to create another subsystem for this.
But there are PHYs which do allow purely SW LED control, i.e. I can set
the LED to be just ON or OFF. These are for example marvell PHYs.
The fact that there are LEDs that can't be controlled purely in SW does
not seem a good reason for to not implement it via netdev trigger.
Either these LEDs shouldn't be exported as LED classdevs, or they
should somehow make use of existing trigger API (so netdev).
The fact that the LED cannot be controlled in SW can be simply
implemented by refusing to disable the netdev trigger on the LED.
Are you aware of any device that can have some trigger offloaded and
still have the led triggered manually? Talking about mixed mode, so HW
and SW. Asking to understand as currently the only way to impement all
of this in netdev trigger is that:
IF any hw offload trigger is supported (and enabled) then the entire
netdev trigger can't work as it won't be able to simulate missing
trigger in SW. And that would leave some flexibility.
We need to understand how to operate in this condition. Should netdev
detect that and ""hide"" the sysfs triggers? Should we report error?
quoted
quoted
If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.
Again the idea here is that a LED can offer a way to run by HW and then
a trigger configure them and enables the mode. I see the offload and
configure function needed anyway side from the implementation.
There is already LED private trigger API for this. You don't need
offloading of existing trigger if you are going to create a new trigger
anyway.
quoted
quoted
And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).
Can we try to find a common way to introduce this?
quoted
The current plan is to:
- add support for offloading existing LED triggers to HW (LED
controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
it fails, netdev trigger will blink the LED in SW as it does now)
Can't be done. If in HW mode, we should just declare a trigger not
supported in SW if we really want to follow the netdev expanding path.
But still that would mean filling the netdev trigger with extra code and
condition that will only apply in HW mode. At this point just create a
dedicated trigger.
The netdev trigger already needs to be expanded with extra code: it
doesn't allow indicating different link types, for example.
For offloading, the extra code can be quite small. So I don't think
this is an argument.
quoted
We can consider introducing the same sysfs used by netdev trigger but
nothing else.
Not true.
quoted
quoted
- create LED classdevices in a PHY driver that have the offload()
methods implemented. The offload method looks at what trigger is
being enabled for the LED, and it if it is a netdev trigger with such
settings that are possible to offload, it will be offloaded.
This whole thing makes use of the existing sysfs ABI.
So for example if I do
cd /sys/class/net/eth0/phydev/leds/<LED>
echo netdev >trigger
echo eth0 >device_name
How would this work in HW mode? The PHY blink only with packet in his
port. We can't tell the PHY to HW blink based on an interface.
That is the main problem by using netdev for PHYs, netdev is flexible
but an offload trigger is not and would work only based on some
condition. We should hardcode the device_name in HW and again pollute
the netdev trigger more.
netdev trigger does not need to be poluted by this much. All this can be
implemented by one callback to offload method, and everything else can
be done in the LED driver itself.
quoted
An idea would be add tons of check to netdev on every event to check the
interface but that wouldn't go against any offload idea? The system will
be loaded anyway with all these checks.
The checks are done only at the moment of configuring the trigger.
Surely you don't see that as "loaded system".
quoted
quoted
echo 1 >rx
echo 1 >tx
The netdev trigger is activated, and it calls the offload() method.
The offload() method is implemented in the PHY driver, and it checks
that it can offload these settings (blink on rx/tx), and will enable
this.
- extend netdev trigger to support more settings:
- indicate link for specific link modes only (for example 1g, 100m)
- ...
- extend PHY drivers to support offloading of these new settings
Marek
The rest of the implementation is very similar. Except we just NOT use
netdev. And to me it does seems the most sane way to handle offload for
a LED. (considering the specific situation of a PHY)
From what I can see we have 2 path:
- Pollute netdev trigger to add entire different function for HW. (that
will end up in complex condition and more load/overhead)
- Introduce a new way to entirely offload some triggers.
The check conditions would be all in LED driver (PHY driver in this
case). The code could be a little complicated, but it only needs to be
written once, and the PHY drivers can reuse it. netdev trigger is not
polluted, only a few calls to offload() method are done.
quoted
Could be that here I'm just using the wrong word and I should use
hardware instead offload. But considering we are offloading some trigger
(example rx/tx/link) it's not that wrong.
The thing is why trying to expand a trigger that will just remove some
flexibility when we can solve the problem at the source with some
additional API that currently we lack any support (leds can be
configured to run by hw) and some dedicated trigger that will do the
task in a cleaner way (and without adding extra load/overhead by really
offloading the task) ?
As I explained above, there is no extra load/overhead. The only thing
that is extra is that netdev trigger gets a little more new code, but
that is already needed anyway.
quoted
Again hope I didn't seem rude in this message but I just want to find a
solution and proposing a new idea/explaining my concern.
I don't think you're rude, we are just discussing a topic for which we
have different opinions.
Marek
From: Marek Behún <kabel@kernel.org> Date: 2021-11-08 18:41:53
On Mon, 8 Nov 2021 18:58:38 +0100
Ansuel Smith [off-list ref] wrote:
Are you aware of any device that can have some trigger offloaded and
still have the led triggered manually?
I don't understand why we would need such a thing.
Look, just to make it clear via an example: I have a device with a
Marvell PHY chip inside. There is a LED connected to one of the PHY LED
pins.
Marvell PHY has LED[0] control register, which supports the following
modes:
LED is OFF
LED is ON
LED is ON when Link is up
LED blinks on RX activity
LED blinks on TX activity
LED blinks on RX/TX activity
LED is ON and blinks on RX/TX activity
...
I have code that exports this LED as a LED classdev
When I activate netdev trigger on this LED, the netdev trigger currently
just blinks the LED in software, by calling the .brightness_set()
method, which configures LED[0] control register to one of the first
two modes above (LED is OFF, LED is ON).
But I have also another patch that adds support to offloading netdev
trigger upon offloadable settings. The netdev trigger code calls the
.trigger_offload() method, which is implemented in PHY driver. This
method checks whether it is a netdev trigger that is to be offloaded,
and whether device_name is the name of the device attached to the PHY,
and then chooses one of the modes above, according to netdev trigger
settings.
So when I request netdev trigger for eth0, to indicate link and blink
on activity, the netdev trigger doesn't do anything in software. It
just calls the offload method ONCE (at the moment I am changing netdev
trigger settings). The blinking is then done by the PHY chip. Netdev
trigger doesn't do anything, at least not until I change the settings
again.
Talking about mixed mode, so HW and SW.
What exactly do you mean by mixed mode? There is no mixed mode.
Asking to understand as currently the only way to impement all
of this in netdev trigger is that:
IF any hw offload trigger is supported (and enabled) then the entire
netdev trigger can't work as it won't be able to simulate missing
trigger in SW. And that would leave some flexibility.
What do you mean by missing trigger here? I think we need to clarify
what we mean by the word "trigger". Are you talking about the various
blinking modes that the PHY supports? If so, please let's call them HW
control modes, and not triggers. By "triggers" I understand triggers
that can be enabled on a LED via /sys/class/leds/<LED>/trigger.
We need to understand how to operate in this condition. Should netdev
detect that and ""hide"" the sysfs triggers? Should we report error?
So if I understand you correctly, you are asking about what should we
do if user asked for netdev trigger settings (currently only link, rx,
tx, interval) that can't be offloaded to the PHY chip.
Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.
If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.
For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.
Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.
$ cd /sys/class/leds/<LED>
$ cat device_name
eth0
$ echo eth1 >device_name
Operation not supported.
$ echo none >trigger
Operation not supported.
Now suppose that the driver by default enabled link indication, so we
have:
$ cat link
1
$ cat rx
0
$ cat tx
0
We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.
Currently the only way to enable this is to do
$ echo 1 >rx
$ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.
There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
is created, which lists modes that are actually supported, and allows
to select between them. When a mode is selected, link,rx,tx are
filled automatically, so that user may read them to know what the LED
is actually doing
- something different?
Marek
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 19:09:02
On Mon, Nov 08, 2021 at 07:41:42PM +0100, Marek Behún wrote:
On Mon, 8 Nov 2021 18:58:38 +0100
Ansuel Smith [off-list ref] wrote:
quoted
Are you aware of any device that can have some trigger offloaded and
still have the led triggered manually?
I don't understand why we would need such a thing.
Look, just to make it clear via an example: I have a device with a
Marvell PHY chip inside. There is a LED connected to one of the PHY LED
pins.
Marvell PHY has LED[0] control register, which supports the following
modes:
LED is OFF
LED is ON
LED is ON when Link is up
LED blinks on RX activity
LED blinks on TX activity
LED blinks on RX/TX activity
LED is ON and blinks on RX/TX activity
...
I have code that exports this LED as a LED classdev
When I activate netdev trigger on this LED, the netdev trigger currently
just blinks the LED in software, by calling the .brightness_set()
method, which configures LED[0] control register to one of the first
two modes above (LED is OFF, LED is ON).
But I have also another patch that adds support to offloading netdev
trigger upon offloadable settings. The netdev trigger code calls the
.trigger_offload() method, which is implemented in PHY driver. This
method checks whether it is a netdev trigger that is to be offloaded,
and whether device_name is the name of the device attached to the PHY,
and then chooses one of the modes above, according to netdev trigger
settings.
So when I request netdev trigger for eth0, to indicate link and blink
on activity, the netdev trigger doesn't do anything in software. It
just calls the offload method ONCE (at the moment I am changing netdev
trigger settings). The blinking is then done by the PHY chip. Netdev
trigger doesn't do anything, at least not until I change the settings
again.
quoted
Talking about mixed mode, so HW and SW.
What exactly do you mean by mixed mode? There is no mixed mode.
Ok.
quoted
Asking to understand as currently the only way to impement all
of this in netdev trigger is that:
IF any hw offload trigger is supported (and enabled) then the entire
netdev trigger can't work as it won't be able to simulate missing
trigger in SW. And that would leave some flexibility.
What do you mean by missing trigger here? I think we need to clarify
what we mean by the word "trigger". Are you talking about the various
blinking modes that the PHY supports? If so, please let's call them HW
control modes, and not triggers. By "triggers" I understand triggers
that can be enabled on a LED via /sys/class/leds/<LED>/trigger.
offload triggers = blinking modes supported
quoted
We need to understand how to operate in this condition. Should netdev
detect that and ""hide"" the sysfs triggers? Should we report error?
So if I understand you correctly, you are asking about what should we
do if user asked for netdev trigger settings (currently only link, rx,
tx, interval) that can't be offloaded to the PHY chip.
Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.
Are all these sysfs entry OK? I mean if we want to add support for he
main blinking modes, the number will increase to at least 10 additional
entry.
If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.
For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.
Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.
$ cd /sys/class/leds/<LED>
$ cat device_name
eth0
$ echo eth1 >device_name
Operation not supported.
$ echo none >trigger
Operation not supported.
Now suppose that the driver by default enabled link indication, so we
have:
$ cat link
1
$ cat rx
0
$ cat tx
0
We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.
Currently the only way to enable this is to do
$ echo 1 >rx
$ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.
There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
is created, which lists modes that are actually supported, and allows
to select between them. When a mode is selected, link,rx,tx are
filled automatically, so that user may read them to know what the LED
is actually doing
- something different?
Expose only the supported blinking modes? (in conjunciong with a generic
traffic blinking mode)
The initial question was Should we support a mixed mode offloaed
blinking modes and blinking modes simulated by sw? I assume no as i
don't think a device that supports that exist.
From: Marek Behún <kabel@kernel.org> Date: 2021-11-08 19:17:21
On Mon, 8 Nov 2021 20:08:53 +0100
Ansuel Smith [off-list ref] wrote:
quoted
Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.
Are all these sysfs entry OK? I mean if we want to add support for he
main blinking modes, the number will increase to at least 10 additional
entry.
See below.
quoted
If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.
For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.
Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.
$ cd /sys/class/leds/<LED>
$ cat device_name
eth0
$ echo eth1 >device_name
Operation not supported.
$ echo none >trigger
Operation not supported.
Now suppose that the driver by default enabled link indication, so we
have:
$ cat link
1
$ cat rx
0
$ cat tx
0
We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.
Currently the only way to enable this is to do
$ echo 1 >rx
$ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.
There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
is created, which lists modes that are actually supported, and allows
to select between them. When a mode is selected, link,rx,tx are
filled automatically, so that user may read them to know what the LED
is actually doing
- something different?
Expose only the supported blinking modes? (in conjunciong with a generic
traffic blinking mode)
The problem with exposing only supported blinking modes is that you
can't hide rx and tx files and create a new, rxtx file. That would
break netdev trigger ABI.
The initial question was Should we support a mixed mode offloaed
blinking modes and blinking modes simulated by sw? I assume no as i
don't think a device that supports that exist.
If you mean something like: the PHY can blink on tx, but not on rx, and
I want to blink on rx/tx, do I will enable HW blinking on tx, and on rx
I will blink from software, then no. If I can blink in software, then
this case should be all done in software. If software blinking is not
supported, then this setting is simply unsupported by the LED.
Let's at first implement offloading of only those things that are
supported by netdev trigger already in SW, so link, rx and tx.
When this works OK and gets merged, we can try to extend the netdev
trigger to support more modes in SW, and then implement offloading of
these modes.
Marek
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-11-08 19:53:48
I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.
Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?
It is a solution that fits the general pattern, do it in software, and
offload it if possible.
However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.
We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.
Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?
Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.
Andrew
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-11-08 20:03:15
On Mon, Nov 08, 2021 at 08:53:36PM +0100, Andrew Lunn wrote:
quoted
I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.
Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?
It is a solution that fits the general pattern, do it in software, and
offload it if possible.
However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.
No it wasn't, I lost the interest.
I feel pretty uneasy hooking up .ndo_get_stats64() to my switches, and
basically opening the flood gates for random processes and kernel
threads to send SPI transactions back and forth like it's nothing.
Latency for programs like ptp4l and phc2sys is actually more important.
We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.
Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?
Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.
Andrew
We could use some of the newer stats APIs exposed by Jakub if
.ndo_get_stats64 is not implemented, like ethtool_ops->get_eth_mac_stats.
Although.. see above.
From: Marek Behún <kabel@kernel.org> Date: 2021-11-08 20:11:20
On Mon, 8 Nov 2021 20:53:36 +0100
Andrew Lunn [off-list ref] wrote:
quoted
I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.
Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?
It is a solution that fits the general pattern, do it in software, and
offload it if possible.
However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.
We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.
Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?
Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.
I am working with Marvell PHYs and Marvell switches. I am aware of the
problem that SW netdev does not work for switches because we don't have
uptodate data.
It seems to me that yes, if the user wants to blink the LEDs on
activity on switch port, the netdev trigger should only work in offload
mode and only on LEDs that are connected to switch pins, unless we
implement a mechanism to get statistics at lest 10 times a second
(which could maybe be done on marvell switches by reading the registers
via ethernet frames instead of MDIO).
Marvell switches don't seem to support rx only / tx only activity
blinking, only rx/tx. I think this could be solved by making rx/tx
sysfs files for netdev trigger behave so that writing to one would also
write to another.
But since currently netdev trigger does not work for these switches, I
think making it so that it works at least to some fashion (in HW
supported modes) would still be better that current situation.
I will try to look into this, maybe work together with Ansuel.
Marek
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-11-08 20:15:14
On Mon, Nov 08, 2021 at 09:11:10PM +0100, Marek Behún wrote:
On Mon, 8 Nov 2021 20:53:36 +0100
Andrew Lunn [off-list ref] wrote:
quoted
quoted
I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.
Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?
It is a solution that fits the general pattern, do it in software, and
offload it if possible.
However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.
We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.
Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?
Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.
I am working with Marvell PHYs and Marvell switches. I am aware of the
problem that SW netdev does not work for switches because we don't have
uptodate data.
It seems to me that yes, if the user wants to blink the LEDs on
activity on switch port, the netdev trigger should only work in offload
mode and only on LEDs that are connected to switch pins, unless we
implement a mechanism to get statistics at lest 10 times a second
(which could maybe be done on marvell switches by reading the registers
via ethernet frames instead of MDIO).
Marvell switches don't seem to support rx only / tx only activity
blinking, only rx/tx. I think this could be solved by making rx/tx
sysfs files for netdev trigger behave so that writing to one would also
write to another.
But since currently netdev trigger does not work for these switches, I
think making it so that it works at least to some fashion (in HW
supported modes) would still be better that current situation.
I will try to look into this, maybe work together with Ansuel.
Marek
I'm polishing some api with Andrew comments and will try to extend
netdev trigger while still proposing the additional trigger. Better
discuss in v3.
I think the main target here is first have a good generic solution for
offload and then actually thinking about working on the triggers/offload
part.
--
Ansuel