Re: [PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Oct 22, 2023 at 12:24:27PM +0200, Florian Eckert wrote:
> On 2023-10-21 18:07, Greg KH wrote:
> > > diff --git a/drivers/leds/trigger/ledtrig-tty.c
> > > b/drivers/leds/trigger/ledtrig-tty.c
> > > index 8ae0d2d284af..6a96439a7e55 100644
> > > --- a/drivers/leds/trigger/ledtrig-tty.c
> > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > @@ -16,6 +16,24 @@ struct ledtrig_tty_data {
> > >  	const char *ttyname;
> > >  	struct tty_struct *tty;
> > >  	int rx, tx;
> > > +	unsigned long mode;
> > 
> > Why is mode "unsigned long" when the tty layer treats it as an int?  And
> > really, this should be set to an explit size, u32 perhaps?  Or am I
> > confused as to exactly what this is?
> 
> This is about the line state that the LED should show "altogether".
> All states that the LED is to display are stored here.
> 
> For example:
> Via the sysfs of the LED I can set the flags rx, tx and line_cts to
> a "not" zero value. That means that the led is enable if the CTS of the
> tty ist set, and the LED flashes if rx/tx data are transmitted via
> this tty.
> 
> Therefore, the bits 0 (TRIGGER_TTY_RX), 1 (TRIGGER_TTY_TX) and
> 2 (TRIGGER_TTY_CTS) are set in the variable. As defined in the
> enum led_trigger_tty_modes

So the enum is a bitfield value?  That's not obvious either, a comment
for the enum might be good to help describe that.

> I think I have not chosen the correct name for the variable there.
> Maybe line_state, would be a better choice?

Or "trigger_modes"?  "mode" feels odd, these are values, so maybe just
"triggers"?

Naming is hard :(

> > > +};
> > > +
> > > +enum led_trigger_tty_state {
> > > +	TTY_LED_BLINK,
> > > +	TTY_LED_ENABLE,
> > > +	TTY_LED_DISABLE,
> > > +};
> > > +
> > > +enum led_trigger_tty_modes {
> > > +	TRIGGER_TTY_RX = 0,
> > > +	TRIGGER_TTY_TX,
> > > +	TRIGGER_TTY_CTS,
> > > +	TRIGGER_TTY_DSR,
> > > +	TRIGGER_TTY_CAR,
> > > +	TRIGGER_TTY_RNG,
> > > +	/* Keep last */
> > > +	__TRIGGER_TTY_MAX,
> > >  };
> > > 
> > 
> > Oh wait, is "mode" this?  If so, why not define it as an enum?  Or if
> > not, I'm totally confused as to what is going on here, sorry.
> 
> See explanation above. I can not set this to an enum because I could
> set more then one Flag via the sysfs.

Ah, then say they are bits, enums are usually not used for that, or if
they are, they are documented better :)

> > >  static void ledtrig_tty_restart(struct ledtrig_tty_data
> > > *trigger_data)
> > > @@ -78,13 +96,106 @@ 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 bit;
> > > +
> > > +	switch (attr) {
> > > +	case TRIGGER_TTY_RX:
> > > +	case TRIGGER_TTY_TX:
> > > +	case TRIGGER_TTY_CTS:
> > > +	case TRIGGER_TTY_DSR:
> > > +	case TRIGGER_TTY_CAR:
> > > +	case TRIGGER_TTY_RNG:
> > > +		bit = attr;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> > 
> > sysfs_emit() for all new sysfs attributes please.
> 
> Correct. Thanks for the hint will use sysf_emit() function in the next
> patchset round.
> 
> > 
> > > +}
> > > +
> > > +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);
> > > +	unsigned long state;
> > > +	int ret;
> > > +	int bit;
> > > +
> > > +	ret = kstrtoul(buf, 0, &state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	switch (attr) {
> > > +	case TRIGGER_TTY_RX:
> > > +	case TRIGGER_TTY_TX:
> > > +	case TRIGGER_TTY_CTS:
> > > +	case TRIGGER_TTY_DSR:
> > > +	case TRIGGER_TTY_CAR:
> > > +	case TRIGGER_TTY_RNG:
> > > +		bit = attr;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (state)
> > > +		set_bit(bit, &trigger_data->mode);
> > > +	else
> > > +		clear_bit(bit, &trigger_data->mode);
> > 
> > I think your test of "state" here is wrong, if you write in "40000" you
> > are treating it as "1", which I don't think you want, right?
> 
> If I have understood your question correctly, then I would say that your
> assumption is not correct. I just want to check here whether it is a number
> greater than zero or not. If the number is greater than zero then the bit
> should be set in the 'mode' variable of the struct and if it is zero then
> it should be cleared.

"greater than 0" can be any number, that's not a good api.  Use the
sysfs api that can handle a boolean, it will deal with "y/N" and 0/1 and
all sorts of other options that way for you automatically.

> The LED could indicate more then one state there. As described above.
> This was requested by Uwe Kleine-König in the old v7 patch series [1].

That's fine, but you need to fix up the userspace api a bit here.

thanks,

greg k-h