Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <65799e8d.df0a0220.1c142.2743@mx.google.com>
Date:   Wed, 13 Dec 2023 13:07:39 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>
Cc:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Andrew Lunn <andrew@lunn.ch>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Daniel Golle <daniel@makrotopia.org>,
        Li Zetao <lizetao1@huawei.com>, linux-leds@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] leds: trigger: netdev: display only supported link speed
 attribute
References: <20231209150724.25565-1-ansuelsmth@gmail.com>
 <20231213130521.11513e0a@dellmb>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231213130521.11513e0a@dellmb>
Precedence: bulk

On Wed, Dec 13, 2023 at 01:05:21PM +0100, Marek Beh?n wrote:
> On Sat,  9 Dec 2023 16:07:24 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > With the addition of more link speed mode to the netdev trigger, it was
> > pointed out that there may be a problem with bloating the attribute list
> > with modes that won't ever be supported by the trigger as the attached
> > device name doesn't support them.
> > 
> > To clear and address this problem, change the logic where these
> > additional trigger modes are added.
> > 
> > Since the netdev trigger REQUIRE a device name to be set, attach to the
> > device name change function additional logic to parse the supported link
> > speed modes using ethtool APIs and add only the supported link speed
> > modes attribute.
> > 
> > This only apply to the link speed modes and every other mode is still
> > provided by default.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 56 +++++++++++++++++++++++----
> >  1 file changed, 49 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index 09e75fd9f2bc..ce84808e231c 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -61,6 +61,8 @@ struct led_netdev_data {
> >  	bool hw_control;
> >  };
> >  
> > +static int add_link_speed_attr(struct led_netdev_data *trigger_data);
> > +
> >  static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  {
> >  	int current_brightness;
> > @@ -262,8 +264,10 @@ static int set_device_name(struct led_netdev_data *trigger_data,
> >  	trigger_data->carrier_link_up = false;
> >  	trigger_data->link_speed = SPEED_UNKNOWN;
> >  	trigger_data->duplex = DUPLEX_UNKNOWN;
> > -	if (trigger_data->net_dev)
> > +	if (trigger_data->net_dev) {
> >  		get_device_state(trigger_data);
> > +		add_link_speed_attr(trigger_data);
> > +	}
> >  
> >  	trigger_data->last_activity = 0;
> >  
> > @@ -396,6 +400,50 @@ DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
> >  DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
> >  DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
> >  
> > +static int add_link_speed_attr(struct led_netdev_data *trigger_data)
> > +{
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	struct device *dev = led_cdev->dev;
> > +	struct ethtool_link_ksettings cmd;
> > +	int ret;
> > +
> > +	/* First remove any entry previously added */
> > +	device_remove_file(dev, &dev_attr_link_10);
> > +	device_remove_file(dev, &dev_attr_link_100);
> > +	device_remove_file(dev, &dev_attr_link_1000);
> > +	device_remove_file(dev, &dev_attr_link_2500);
> > +	device_remove_file(dev, &dev_attr_link_5000);
> > +	device_remove_file(dev, &dev_attr_link_10000);
> 
> Noooooo!
> 
> > +	ret = __ethtool_get_link_ksettings(trigger_data->net_dev, &cmd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Add only supported entry */
> > +	if (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, cmd.link_modes.supported) ||
> > +	    test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_10);
> > +
> > +	if (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, cmd.link_modes.supported) ||
> > +	    test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_100);
> > +
> > +	if (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, cmd.link_modes.supported) ||
> > +	    test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_1000);
> > +
> > +	if (test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_2500);
> > +
> > +	if (test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_5000);
> > +
> > +	if (test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, cmd.link_modes.supported))
> > +		device_create_file(dev, &dev_attr_link_10000);
> > +
> > +	return 0;
> > +}
> 
> This should be done via the is_visible sysfs attribute_group method.
> 
> Also documentation for the link_* files should be changed so that it
> says that the files are present only if those speeds are available.
>

Just working on doing this change!

Just need to do some test on how is_visible behave with changing the
device_name.

-- 
	Ansuel