| Message ID | 20231019112809.881730-1-fe@dev.tdt.de |
|---|---|
| Headers | show |
| Series | ledtrig-tty: add additional tty state evaluation | expand |
On Thu, Oct 19, 2023 at 01:28:09PM +0200, Florian Eckert wrote: > Until now, the LED blinks when data is sent via the tty (rx/tx). > The serial tty interface also supports additional input signals, that can > also be evaluated within this trigger. This change is adding the following > additional input sources, which could be controlled > via the '/sys/class/<leds>/' sysfs interface. > > - line_cts: > DCE is ready to accept data from the DTE (CTS = Clear To Send). If the > line state is detected, the LED is switched on. > If set to 0 (default), the LED will not evaluate CTS. > If set to 1, the LED will evaluate CTS. > > - line_dsr: > DCE is ready to receive and send data (DSR = Data Set Ready). If the line > state is detected, the LED is switched on. > If set to 0 (default), the LED will not evaluate DSR. > If set to 1, the LED will evaluate DSR. > > - line_car: > DTE is receiving a carrier from the DCE (DCD = Data Carrier Detect). If > the line state is detected, the LED is switched on. > If set to 0 (default), the LED will not evaluate CAR (DCD). > If set to 1, the LED will evaluate CAR (DCD). > > - line_rng: > DCE has detected an incoming ring signal on the telephone line > (RI = Ring Indicator). If the line state is detected, the LED is > switched on. > If set to 0 (default), the LED will not evaluate RNG (RI). > If set to 1, the LED will evaluate RNG (RI). > > Explanation: > DCE = Data Communication Equipment (Modem) > DTE = Data Terminal Equipment (Computer) > > In addition to the new line_* entries in sysfs, the indication for the > direction of the transmitted data is independently controllable via the > new rx and tx sysfs entrie now too. These are on by default. Thus the > trigger behaves as before this change. > > - 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. > > Signed-off-by: Florian Eckert <fe@dev.tdt.de> > --- > .../ABI/testing/sysfs-class-led-trigger-tty | 54 +++++ > drivers/leds/trigger/ledtrig-tty.c | 192 +++++++++++++++++- > 2 files changed, 235 insertions(+), 11 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > index 2bf6b24e781b..08127b1a4602 100644 > --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty > @@ -4,3 +4,57 @@ KernelVersion: 5.10 > Contact: linux-leds@vger.kernel.org > Description: > Specifies the tty device name of the triggering tty > + > +What: /sys/class/leds/<led>/rx > +Date: October 2023 > +KernelVersion: 6.7 > +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: October 2023 > +KernelVersion: 6.7 > +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. > + > +car rng > +What: /sys/class/leds/<led>/line_cts > +Date: October 2023 > +KernelVersion: 6.7 > +Description: > + DCE is ready to accept data from the DTE (Clear To Send). If > + the line state is detected, the LED is switched on. > + If set to 0 (default), the LED will not evaluate CTS. > + If set to 1, the LED will evaluate CTS. > + > +What: /sys/class/leds/<led>/line_dsr > +Date: October 2023 > +KernelVersion: 6.7 > +Description: > + DCE is ready to receive and send data (Data Set Ready). If > + the line state is detected, the LED is switched on. > + If set to 0 (default), the LED will not evaluate DSR. > + If set to 1, the LED will evaluate DSR. > + > +What: /sys/class/leds/<led>/line_car > +Date: October 2023 > +KernelVersion: 6.7 > +Description: > + DTE is receiving a carrier from the DCE (Data Carrier Detect). > + If the line state is detected, the LED is switched on. > + If set to 0 (default), the LED will not evaluate CAR (DCD). > + If set to 1, the LED will evaluate CAR (DCD). > + > +What: /sys/class/leds/<led>/line_cts > +Date: October 2023 > +KernelVersion: 6.7 > +Description: > + DCE has detected an incoming ring signal on the telephone > + line (Ring Indicator). If the line state is detected, the > + LED is switched on. > + If set to 0 (default), the LED will not evaluate RNG (RI). > + If set to 1, the LED will evaluate RNG (RI). > 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? > +}; > + > +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. > 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. > +} > + > +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? thanks, greg k-h
On Thu, Oct 19, 2023 at 01:28:08PM +0200, Florian Eckert wrote: > The struct 'tty_struct' has a callback to read the status flags of the tty > if the tty driver provides them. So fare, the data is transferred directly > to userspace with the function 'tty_tiocmget'. This function cannot be > used to evaluate the status line of the tty interface in the ledtrig-tty > trigger. To make this possible, a new function must be added that does > not immediately pass the data on to userspace. > > The new function 'tty_get_tiocm' only returns the status register. > This information can then be processed further in the ledtrig-tty > trigger. Writing changelogs are hard. You are including a lot of information in here that really doesn't need to be, as you are focusing on your specific use case, which is fine, but you are creating a generic function. This can be simpler, how about something like this: There is no in-kernel function to get the status register of a tty device like the TIOCMGET ioctl returns to userspace. Create a new function, tty_get_tiocm(), to obtain the status register that other portions of the kernel can call if they need this information, and move the existing internal tty_tiocmget() function to use this interface. Sound good? The code portion looks fine to me, thanks for doing this. greg k-h
On 2023-10-21 18:15, Greg KH wrote: > On Thu, Oct 19, 2023 at 01:28:08PM +0200, Florian Eckert wrote: >> The struct 'tty_struct' has a callback to read the status flags of the >> tty >> if the tty driver provides them. So fare, the data is transferred >> directly >> to userspace with the function 'tty_tiocmget'. This function cannot be >> used to evaluate the status line of the tty interface in the >> ledtrig-tty >> trigger. To make this possible, a new function must be added that does >> not immediately pass the data on to userspace. >> >> The new function 'tty_get_tiocm' only returns the status register. >> This information can then be processed further in the ledtrig-tty >> trigger. > > Writing changelogs are hard. You are including a lot of information in > here that really doesn't need to be, as you are focusing on your > specific use case, which is fine, but you are creating a generic > function. Yes, that is absolutely right. I'll try to take that into account next time, thanks for your advice. > This can be simpler, how about something like this: > > There is no in-kernel function to get the status register of a > tty device like the TIOCMGET ioctl returns to userspace. Create > a new function, tty_get_tiocm(), to obtain the status register > that other portions of the kernel can call if they need this > information, and move the existing internal tty_tiocmget() > function to use this interface. I will replace the commit message with your suggestion in the next round of the patch series. Thanks! --- Florian
Changes in v4: - Merging patch 3/4 into patch number 4/4 from previous series, because it fixes a problem that does not exist upstream. This was a note from the build robot regarding my change that I added with previous series. This change was never upstream and therefore this is not relevant. - Update the commit message of patch 1/3 of this series, that this commit also changes the 'ndashes' to simple dashes. There were no changes, so I add the 'Reviewed-by' that the commit received before. - With this patchset version I have reworked my implementation for the evaluation of the additional line state, so that this changes becomes smaller. As basis I have used the staged commits from Christian Marangi that makes this changes to the netdev trigger. This has already been applied to 'for-leds-next-next' by Lee Jones. I adapted this to the tty trigger. Convert device attr to macro: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=509412749002f4bac4c29f2012fff90c08d8afca Unify sysfs and state handling: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=0fd93ac8582627bee9a3c824489f302dff722881 Changes in v3: - Add missing 'kernel test robot' information to the commit message. - Additional information added to the commit message Changes in v2: - rename new function from tty_get_mget() to tty_get_tiocm() as requested by 'Jiri Slaby'. - As suggested by 'Jiri Slaby', fixed tabs in function documentation throughout the file '/drivers/tty/tty_io.c' in a separate commit. - Move the variable definition to the top in function 'ledtrig_tty_work()'. This was reported by the 'kernel test robot' after my change in v1. - Also set the 'max_brightness' to 'blink_brightness' if no 'blink_brightness' was set. This fixes a problem at startup when the brightness is still set to 0 and only 'line_*' is evaluated. I looked in the netdev trigger and that's exactly how it's done there. v1: This is a follow-up patchset, based on the mailing list discussion from March 2023 based on the old patchset v8 [1]. I have changed, the LED trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König. [1] https://lore.kernel.org/linux-leds/20230306094113.273988-1-fe@dev.tdt.de/ Florian Eckert (3): tty: whitespaces in descriptions corrected by replacing tabs with spaces tty: add new helper function tty_get_tiocm leds: ledtrig-tty: add new line mode evaluation .../ABI/testing/sysfs-class-led-trigger-tty | 54 +++++ drivers/leds/trigger/ledtrig-tty.c | 192 +++++++++++++++++- drivers/tty/tty_io.c | 130 ++++++------ include/linux/tty.h | 1 + 4 files changed, 309 insertions(+), 68 deletions(-)