Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6273d0c3.1c69fb81.19fc.3eaf@mx.google.com>
Date:   Thu, 5 May 2022 15:27:29 +0200
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>, Pavel Machek <pavel@ucw.cz>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [RFC PATCH v6 06/11] leds: trigger: netdev: add hardware control
 support
References: <20220503151633.18760-1-ansuelsmth@gmail.com>
 <20220503151633.18760-7-ansuelsmth@gmail.com>
 <YnMhk1F0LrIMK5hp@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YnMhk1F0LrIMK5hp@lunn.ch>
Precedence: bulk

On Thu, May 05, 2022 at 03:00:03AM +0200, Andrew Lunn wrote:
> > +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},
> 
> hardware_only is never set. Maybe it is used in a later patch? If so,
> please introduce it there.
>

Is it better to introduce the hardware_only bool in the patch where the
additional "hardware only" modes are added?

> >  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;
> 
> This file mostly keeps with reverse christmas tree, probably because
> it was written by a netdev developer. It is probably not required for
> the LED subsystem, but it would be nice to keep the file consistent.
> 

The order is a bit mixed as you notice. Ok will stick to reverse
christmas.

> > @@ -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)) {
> 
> You probably want to validate trigger_data->net_dev is not NULL first. The current code
> is a little odd with that, 
> 

The thing is that net_dev can be NULL and actually is a requirement for
hardware_mode to be triggered. (net_dev must be NULL or software mode is
forced)

> > +		/* Restore old net_dev and device_name */
> > +		if (trigger_data->net_dev)
> > +			dev_put(trigger_data->net_dev);
> > +
> > +		dev_hold(old_net);
> 
> This dev_hold() looks wrong. It is trying to undo a dev_put()
> somewhere? You should not actually do a put until you know you really
> do not old_net, otherwise there is a danger it disappears and you
> cannot undo.
> 

Yes if you notice some lines above, the first thing done is to dev_put
the current net_dev set. So on validation fail we restore the old state
with holding the old_net again and restoring the device_name.

But thanks for poiting it out... I should check if old_net is not NULL.
Also should i change the logic and just dev_put if all goes well? (for
example before the return size?) That way I should be able to skip this
additional dev_hold.

> > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> >  		return ret;
> >  
> >  	/* impose some basic bounds on the timer interval */
> > -	if (value >= 5 && value <= 10000) {
> > -		cancel_delayed_work_sync(&trigger_data->work);
> > +	if (value < 5 || value > 10000)
> > +		return -EINVAL;
> > +
> > +	cancel_delayed_work_sync(&trigger_data->work);
> > +
> > +	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> >  
> > -		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> > -		set_baseline_state(trigger_data);	/* resets timer */
> > +	if (!validate_baseline_state(trigger_data)) {
> > +		/* Restore old interval on validation error */
> > +		atomic_set(&trigger_data->interval, old_interval);
> > +		trigger_data->mode = old_mode;
> 
> I think you need to schedule the work again, since you cancelled
> it. It is at the end of the work that the next work is scheduled, and
> so it will not self recover.
> 

Ok I assume the correct way to handle this is to return error and still
use the set_baseline_state... Or Also move the validate_baseline_state
up before the cancel_delayed_work_sync. But considering we require
atomic_set for the validation to work I think the right way is to
set_baseline_state even with errors (as it will reschedule the work)

>    Andrew

-- 
	Ansuel