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
