Message-ID: <639ca665.1c0a0220.ae24f.9d06@mx.google.com>
Date:   Fri, 16 Dec 2022 18:09:58 +0100
From:   Christian Marangi <ansuelsmth@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>, Pavel Machek <pavel@....cz>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-leds@...r.kernel.org,
        Tim Harvey <tharvey@...eworks.com>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Rasmus Villemoes <rasmus.villemoes@...vas.dk>
Subject: Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control
 support

On Thu, Dec 15, 2022 at 05:07:31PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:33AM +0100, Christian Marangi wrote:
> > Add hardware control support for the Netdev trigger.
> > The trigger on config change will check if the requested trigger can set
> > to blink mode using LED hardware mode and if every blink mode is supported,
> > the trigger will enable hardware mode with the requested configuration.
> > If there is at least one trigger that is not supported and can't run in
> > hardware mode, then software mode will be used instead.
> > A validation is done on every value change and on fail the old value is
> > restored and -EINVAL is returned.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> >  1 file changed, 149 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index dd63cadb896e..ed019cb5867c 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> >   */
> >  
> >  struct led_netdev_data {
> > +	enum led_blink_modes blink_mode;
> >  	spinlock_t lock;
> >  
> >  	struct delayed_work work;
> > @@ -53,11 +54,105 @@ struct led_netdev_data {
> >  	bool carrier_link_up;
> >  };
> >  
> > +struct netdev_led_attr_detail {
> > +	char *name;
> > +	bool hardware_only;
> > +	enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > +};
> > +
> > +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> > +{
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	struct netdev_led_attr_detail *detail;
> > +	u32 hw_blink_mode_supported = 0;
> > +	bool force_sw = false;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +		detail = &attr_details[i];
> > +
> > +		/* Mode not active, skip */
> > +		if (!test_bit(detail->bit, &trigger_data->mode))
> > +			continue;
> > +
> > +		/* Hardware only mode enabled on software controlled led */
> > +		if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > +		    detail->hardware_only)
> > +			return false;
> > +
> > +		/* Check if the mode supports hardware mode */
> > +		if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > +			/* With a net dev set, force software mode.
> > +			 * With modes are handled by hardware, led will blink
> > +			 * based on his own events and will ignore any event
> > +			 * from the provided dev.
> > +			 */
> > +			if (trigger_data->net_dev) {
> > +				force_sw = true;
> > +				continue;
> > +			}
> > +
> > +			/* With empty dev, check if the mode is supported */
> > +			if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > +				hw_blink_mode_supported |= BIT(detail->bit);
> > +		}
> > +	}
> > +
> > +	/* We can't run modes handled by both software and hardware.
> > +	 * Check if we run hardware modes and check if all the modes
> > +	 * can be handled by hardware.
> > +	 */
> > +	if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
> > +		return false;
> > +
> > +	/* Modes are valid. Decide now the running mode to later
> > +	 * set the baseline.
> > +	 * Software mode is enforced with net_dev set. With an empty
> > +	 * one hardware mode is selected by default (if supported).
> > +	 */
> > +	if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> > +		trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > +	else
> > +		trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > +
> > +	return true;
> > +}
> > +
> >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  {
> > +	int i;
> >  	int current_brightness;
> > +	struct netdev_led_attr_detail *detail;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> >  
> > +	/* Modes already validated. Directly apply hw trigger modes */
> > +	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > +		/* We are refreshing the blink modes. Reset them */
> > +		led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> > +					       BLINK_MODE_ZERO);
> > +
> > +		for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > +			detail = &attr_details[i];
> > +
> > +			if (!test_bit(detail->bit, &trigger_data->mode))
> > +				continue;
> > +
> > +			led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> > +						       BLINK_MODE_ENABLE);
> > +		}
> > +
> > +		led_cdev->hw_control_start(led_cdev);
> > +
> > +		return;
> > +	}
> > +
> > +	/* Handle trigger modes by software */
> >  	current_brightness = led_cdev->brightness;
> >  	if (current_brightness)
> >  		led_cdev->blink_brightness = current_brightness;
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> >  				 size_t size)
> >  {
> >  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct net_device *old_net = trigger_data->net_dev;
> > +	char old_device_name[IFNAMSIZ];
> >  
> >  	if (size >= IFNAMSIZ)
> >  		return -EINVAL;
> >  
> > +	/* Backup old device name */
> > +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> >  	cancel_delayed_work_sync(&trigger_data->work);
> >  
> >  	spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> >  		trigger_data->net_dev =
> >  		    dev_get_by_name(&init_net, trigger_data->device_name);
> >  
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old net_dev and device_name */
> > +		if (trigger_data->net_dev)
> > +			dev_put(trigger_data->net_dev);
> > +
> > +		dev_hold(old_net);
> > +		trigger_data->net_dev = old_net;
> > +		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> > +
> > +		spin_unlock_bh(&trigger_data->lock);
> > +		return -EINVAL;
> 
> I'm not sure this is the best way... putting the net_dev but holding a
> reference, to leter regain the reference via dev_hold() just feels
> wrong. Also, I wonder what happens if two threads try to change the
> netdev together - will the read of the old device name be potentially
> corrupted (since we're not holding the trigger's lock?)
> 
> Maybe instead:
> 
> +	struct net_device *old_net;
> ...
> -	if (trigger_data->net_dev) {
> -		dev_put(trigger_data->net_dev);
> -		trigger_data->net_dev = NULL;
> -	}
> +	old_net = trigger_data->net_dev;
> +	trigger_data->net_dev = NULL;
> +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> ...
> 	... extract out the setup of trigger_data->device_name
> ...
> +	if (!validate_baseline_state(trigger_data)) {
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		/* Restore device settings */
> +		trigger_data->net_dev = old_dev;
> +		memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> +		spin_unlock_bh(&trigger_data->lock);
> +		return -EINVAL;
> +	} else {
> +		dev_put(old_net);
> +	}
> 
> would be safer all round?

Need to check but if I'm not wrong all this thing was to handle the very
corner case where net can be removed while we are changing trigger and
something goes wrong down the line... Holding that means it won't get
actually removed till everything is ok.

> 
> One thought on this approach though - if one has a PHY that supports
> "activity" but not independent "rx" and "tx" activity indications
> and it doesn't support software control, how would one enable activity
> mode? There isn't a way to simultaneously enable both at the same
> time... However, I need to check whether there are any PHYs that fall
> into this category.
>

Problem is that for such feature and to have at least something working
we need to face compromise. We really can't support each switch feature
and have a generic API for everything. My original idea was to have
something VERY dynamic with a totally dedicated and dumb trigger... But
that was NACK as netdev was the correct way to handle these stuff...

But adapting everything to netdev trigger is hard since you have just
another generic abstraction layer. My idea at times was that in such
case the trigger rule will be rejected and only enabled if both tx and
rx were enabled. An alternative is to add another flag for activity
rule. (for switch supporting independent tx and rx with activity rule
enable both tx and rx event are enabled. for switch not supporting
independent tx and rx just fallback to sw and say that the mode is not
suported.)

I already had the idea of Documenting all this case but if we decide to
follow this approach then creating a schema file is a must at this
point. (but wanted to introduce that later if and ever this feature will
be accepted to permit to set trigger rules directly in DT following
something like linux,default-trigger.

-- 
	Ansuel

Powered by blists - more mailing lists