Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Wed, 13 Dec 2023 13:05:21 +0100
From:   Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To:     Christian Marangi <ansuelsmth@gmail.com>
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
Message-ID: <20231213130521.11513e0a@dellmb>
In-Reply-To: <20231209150724.25565-1-ansuelsmth@gmail.com>
References: <20231209150724.25565-1-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

Marek