From: Bagas Sanjaya <bagasdotme@gmail.com>
To: Theodore Ts'o <tytso@mit.edu>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh@kernel.org>, Jiri Slaby <jirislaby@kernel.org>,
Rodolfo Giometti <giometti@enneenne.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux Serial <linux-serial@vger.kernel.org>,
Elvis <elvisimprsntr@gmail.com>,
"A. Steinmetz" <anstein99@googlemail.com>
Subject: Re: Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin
Date: Thu, 9 May 2024 17:41:45 +0700 [thread overview]
Message-ID: <ZjyoaZ1zJhMVGjbS@archie.me> (raw)
In-Reply-To: <20240509062456.GE3620298@mit.edu>
[-- Attachment #1: Type: text/plain, Size: 6946 bytes --]
[+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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
prev parent reply other threads:[~2024-05-09 10:41 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 1:52 Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin Bagas Sanjaya
2024-05-08 18:28 ` Greg Kroah-Hartman
2024-05-09 6:24 ` Theodore Ts'o
2024-05-09 10:41 ` Bagas Sanjaya [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjyoaZ1zJhMVGjbS@archie.me \
--to=bagasdotme@gmail.com \
--cc=anstein99@googlemail.com \
--cc=elvisimprsntr@gmail.com \
--cc=giometti@enneenne.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=robh@kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).