[+Cc Rodolfo and not1337 (kernel hack author)]
On Thu, May 09, 2024 at 02:24:56AM -0400, Theodore Ts'o wrote:
> I'd suggest that you reach out to Rondolfo as the maintainer, or to
> the linuxpps mailing list.
>
> First of all, looking at the patch referenced in the bugzilla (which
> is actually found in github), it appears that the person who made the
> request via Bugzilla is different from the the person who authored the
> patch (apparently, github.com/not1337).
>
> Secondly, the patch is really quite hacky. First, the termonology
> used of "4wire" is non-standard (e.g., uised nowhere but at
> github.com/not1337/pss-stuff), and misleading. A cable which only has
> RxD, TxD, RTS, and CTS is not going to work well without GND, so "4
> wire" is quite the misnomer". This termonology is also not used by
> FreeBSD, BTW. Secondly, unconditionally mapping CTS to DCD when
> setting a magic UART-level attribute is a bit hacky, since it will do
> this magic ad-hoc mapping all of the time, not only if the PPS line
> discpline is selected.
>
> Now, I haven't been the tty maintainer in quite a while, but in my
> opinion, a much cleaner way would be to plumb a new tty ldisc
> function, cts_change, which is analogous to the dcd_change function
> (which was introduced specifically for pps_ldisc). Then for bonus
> points, consider using the pps capture mode mde that FreeeBSD's UART
> driver, including the invert option and narrow pulse mode, and eschew
> using the non-standard "4wire" naming terminology.
I have pinged Rodolfo (and also Cc: linuxpps ML but the ML address bounces,
essentially turned into private mail) and here's his comments about the
feature request:
> The DCD-change information is delivered via struct tty_ldisc to the PPS client pps-ldisc.c (file serial_core.c):
>
> /**
> * uart_handle_dcd_change - handle a change of carrier detect state
> * @uport: uart_port structure for the open port
> * @active: new carrier detect status
> *
> * Caller must hold uport->lock.
> */
> void uart_handle_dcd_change(struct uart_port *uport, bool active)
> {
> struct tty_port *port = &uport->state->port;
> struct tty_struct *tty = port->tty;
> struct tty_ldisc *ld;
>
> lockdep_assert_held_once(&uport->lock);
>
> if (tty) {
> ld = tty_ldisc_ref(tty);
> if (ld) {
> if (ld->ops->dcd_change)
> ld->ops->dcd_change(tty, active);
> tty_ldisc_deref(ld);
> }
> }
>
> uport->icount.dcd++;
>
> if (uart_dcd_enabled(uport)) {
> if (active)
> wake_up_interruptible(&port->open_wait);
> else if (tty)
> tty_hangup(tty);
> }
> }
> EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
>
> But for CTS this is not (serial_core.c):
>
> /**
> * uart_handle_cts_change - handle a change of clear-to-send state
> * @uport: uart_port structure for the open port
> * @active: new clear-to-send status
> *
> * Caller must hold uport->lock.
> */
> void uart_handle_cts_change(struct uart_port *uport, bool active)
> {
> lockdep_assert_held_once(&uport->lock);
>
> uport->icount.cts++;
>
> if (uart_softcts_mode(uport)) {
> if (uport->hw_stopped) {
> if (active) {
> uport->hw_stopped = false;
> uport->ops->start_tx(uport);
> uart_write_wakeup(uport);
> }
> } else {
> if (!active) {
> uport->hw_stopped = true;
> uport->ops->stop_tx(uport);
> }
> }
>
> }
> }
> EXPORT_SYMBOL_GPL(uart_handle_cts_change);
>
> This is because the struct tty_ldisc has no cts_change() method (file tty_ldisc.h):
>
> struct tty_ldisc_ops {
> char *name;
> int num;
>
> /*
> * The following routines are called from above.
> */
> int (*open)(struct tty_struct *tty);
> void (*close)(struct tty_struct *tty);
> void (*flush_buffer)(struct tty_struct *tty);
> ssize_t (*read)(struct tty_struct *tty, struct file *file, u8 *buf,
> size_t nr, void **cookie, unsigned long offset);
> ssize_t (*write)(struct tty_struct *tty, struct file *file,
> const u8 *buf, size_t nr);
> int (*ioctl)(struct tty_struct *tty, unsigned int cmd,
> unsigned long arg);
> int (*compat_ioctl)(struct tty_struct *tty, unsigned int cmd,
> unsigned long arg);
> void (*set_termios)(struct tty_struct *tty, const struct ktermios *old);
> __poll_t (*poll)(struct tty_struct *tty, struct file *file,
> struct poll_table_struct *wait);
> void (*hangup)(struct tty_struct *tty);
>
> /*
> * The following routines are called from below.
> */
> void (*receive_buf)(struct tty_struct *tty, const u8 *cp,
> const u8 *fp, size_t count);
> void (*write_wakeup)(struct tty_struct *tty);
> void (*dcd_change)(struct tty_struct *tty, bool active);
> size_t (*receive_buf2)(struct tty_struct *tty, const u8 *cp,
> const u8 *fp, size_t count);
> void (*lookahead_buf)(struct tty_struct *tty, const u8 *cp,
> const u8 *fp, size_t count);
>
> struct module *owner;
> };
>
> So, in order to do what you suggest you have to add this feature first.
>
>
> Finally, note that the way kernel development works is that it's not
> enough for a user to ask for a feature. Someone has to create a high
> quality, clean, maintainable patch. Note all random hacks found in
> random Bugzilla or Github git trees are suitable for inclusion in the
> upstream kernel. And if you don't know how to evaluate the patch for
> quality, it might not be best thing to just ask the bugzilla requester
> to follow the Submitting Patches procedure, given that (a) they might
> not be a kernel developer, and (b) it might just frustrate the
> bugzilla requester and maintainer if the patch isn't sufficient high
> quality, especially if you've managed to set expectations that all the
> bugzilla requestor needs to do is to submit the patch and it will be
> accepted.
I also expected the same (provide patches).
Thanks.
--
An old man doll... just what I always wanted! - Clara
Attachment:
signature.asc
Description: PGP signature
