Re: [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable

[Greg KH]

On Thu, Nov 09, 2023 at 09:50:37AM +0100, Florian Eckert wrote:
> Until now, the LED blinks when data is sent via the tty (rx/tx).
> This is not configurable.
> 
> This change adds the possibility to make the indication for the direction
> of the transmitted data independently controllable via the new rx and
> tx sysfs entries.
> 
> - rx:
>   Signal reception (rx) of data on the named tty device.
>   If set to 0, the LED will not blink on reception.
>   If set to 1 (default), the LED will blink on reception.
> 
> - tx:
>   Signal transmission (tx) of data on the named tty device.
>   If set to 0, the LED will not blink on transmission.
>   If set to 1 (default), the LED will blink on transmission.
> 
> This new sysfs entry are on by default. Thus the trigger behaves as
> before this change.
> 
> Signed-off-by: Florian Eckert <fe@xxxxxxxxxx>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-tty   |  16 +++
>  drivers/leds/trigger/ledtrig-tty.c            | 124 ++++++++++++++++--
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> index 2bf6b24e781b..504dece151b8 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -4,3 +4,19 @@ KernelVersion:	5.10
>  Contact:	linux-leds@xxxxxxxxxxxxxxx
>  Description:
>  		Specifies the tty device name of the triggering tty
> +
> +What:		/sys/class/leds/<led>/rx
> +Date:		February 2024
> +KernelVersion:	6.8
> +Description:
> +		Signal reception (rx) of data on the named tty device.
> +		If set to 0, the LED will not blink on reception.
> +		If set to 1 (default), the LED will blink on reception.
> +
> +What:		/sys/class/leds/<led>/tx
> +Date:		February 2024
> +KernelVersion:	6.8
> +Description:
> +		Signal transmission (tx) of data on the named tty device.
> +		If set to 0, the LED will not blink on transmission.
> +		If set to 1 (default), the LED will blink on transmission.
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 3badf74fa420..1a40a78bf1ee 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -17,6 +17,19 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	bool mode_rx;
> +	bool mode_tx;
> +};
> +
> +/* Indicates which state the LED should now display */
> +enum led_trigger_tty_state {
> +	TTY_LED_BLINK,
> +	TTY_LED_DISABLE,
> +};
> +
> +enum led_trigger_tty_modes {
> +	TRIGGER_TTY_RX = 0,
> +	TRIGGER_TTY_TX,
>  };
>  
>  static int ledtrig_tty_waitforcompletion(struct device *dev)
> @@ -86,12 +99,82 @@ static ssize_t ttyname_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(ttyname);
>  
> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
> +				     enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int completion;
> +	bool state;
> +
> +	reinit_completion(&trigger_data->sysfs);
> +	completion = ledtrig_tty_waitforcompletion(dev);
> +	if (completion < 0)
> +		return completion;

Why do you need to wait for anything just to read the sysfs file?  What
does that sync up with?  And why would it matter?

> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +		state = trigger_data->mode_rx;
> +		break;
> +	case TRIGGER_TTY_TX:
> +		state = trigger_data->mode_tx;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", state);
> +}
> +
> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
> +				      size_t size, enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int completion;
> +	bool state;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&trigger_data->sysfs);
> +	completion = ledtrig_tty_waitforcompletion(dev);
> +	if (completion < 0)
> +		return completion;

Same here, why sync anything?

What am I missing as to why a completion is needed?

thanks,

greg k-h