lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <6451339f.5d0a0220.88ec5.8829@mx.google.com> Date: Tue, 2 May 2023 18:00:29 +0200 From: Christian Marangi <ansuelsmth@...il.com> To: Andrew Lunn <andrew@...n.ch> Cc: Jonathan Corbet <corbet@....net>, Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org, netdev@...r.kernel.org Subject: Re: [PATCH 08/11] leds: trigger: netdev: add support for LED hw control On Sun, Apr 30, 2023 at 07:55:13PM +0200, Andrew Lunn wrote: > On Thu, Apr 27, 2023 at 02:15:38AM +0200, Christian Marangi wrote: > > Add support for LED hw control for the netdev trigger. > > > > The trigger on calling set_baseline_state to configure a new mode, will > > do various check to verify if hw control can be used for the requested > > mode in the validate_requested_mode() function. > > > > It will first check if the LED driver supports hw control for the netdev > > trigger, then will check if the requested mode are in the trigger mode > > mask and finally will call hw_control_set() to apply the requested mode. > > > > To use such mode, interval MUST be set to the default value and net_dev > > MUST be empty. If one of these 2 value are not valid, hw control will > > never be used and normal software fallback is used. > > > > Signed-off-by: Christian Marangi <ansuelsmth@...il.com> > > --- > > drivers/leds/trigger/ledtrig-netdev.c | 52 +++++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c > > index 8cd876647a27..61bc19fd0c7a 100644 > > --- a/drivers/leds/trigger/ledtrig-netdev.c > > +++ b/drivers/leds/trigger/ledtrig-netdev.c > > @@ -68,6 +68,13 @@ static void set_baseline_state(struct led_netdev_data *trigger_data) > > int current_brightness; > > struct led_classdev *led_cdev = trigger_data->led_cdev; > > > > + /* Already validated, hw control is possible with the requested mode */ > > + if (trigger_data->hw_control) { > > + led_cdev->hw_control_set(led_cdev, trigger_data->mode); > > + > > + return; > > + } > > + > > current_brightness = led_cdev->brightness; > > if (current_brightness) > > led_cdev->blink_brightness = current_brightness; > > @@ -95,6 +102,51 @@ static void set_baseline_state(struct led_netdev_data *trigger_data) > > static int validate_requested_mode(struct led_netdev_data *trigger_data, > > unsigned long mode, bool *can_use_hw_control) > > { > > + unsigned int interval = atomic_read(&trigger_data->interval); > > + unsigned long hw_supported_mode, hw_mode = 0, sw_mode = 0; > > + struct led_classdev *led_cdev = trigger_data->led_cdev; > > + unsigned long default_interval = msecs_to_jiffies(50); > > + bool force_sw = false; > > + int i, ret; > > + > > + hw_supported_mode = led_cdev->trigger_supported_flags_mask; > > + > > > + if (interval == default_interval && !trigger_data->net_dev && > > + !force_sw && test_bit(i, &hw_supported_mode)) > > + set_bit(i, &hw_mode); > > + else > > + set_bit(i, &sw_mode); > > + } > > + > > > + /* Check if the requested mode is supported */ > > + ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode); > > + if (ret) > > + return ret; > > Hi Christian > > What is the purpose of led_cdev->trigger_supported_flags_mask? I don't > see why it is needed when you are also going to ask the PHY if it can > support the specific blink pattern the user is requesting. The idea is to have a place where a trigger can quickly check the single mode supported before the entire mode map is validated, but I understand that this can totally be dropped with some extra code from both trigger and LED driver. While refactoring the netdev triger mode validation I notice it was very handy and simplified the check logic having a mask of the single mode supported. But this might be not needed for now and we can think of a better approach later when we will introduce hardware only modes. > > The problem i have with the Marvell PHY, and other PHYs i've looked at > datasheets for, is that hardware does not work like this. It has a > collection of blinking modes, which are a mixture of link speeds, rx > activity, and tx activity. It supports just a subset of all > possibilities. > > I think this function can be simplified. Simply ask the LED via > hw_control_is_supported() does it support this mode. If yes, offload > it, if not use software blinking. Yep, I will consider dropping it to slim this series even further. > > Andrew -- Ansuel
Powered by blists - more mailing lists