Received-SPF: pass (google.com: domain of linux-kernel+bounces-174406-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 9 May 2024 17:41:45 +0700
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
Message-ID: <ZjyoaZ1zJhMVGjbS@archie.me>
References: <Zjra2GZIDC7BPoZx@archie.me>
 <2024050853-basin-salsa-32bb@gregkh>
 <20240509062456.GE3620298@mit.edu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="nKxqMyvWjc07RdJB"
Content-Disposition: inline
In-Reply-To: <20240509062456.GE3620298@mit.edu>


--nKxqMyvWjc07RdJB
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

[+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.
>=20
> 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).
>=20
> 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.
>=20
> 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 c=
lient 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 =3D &uport->state->port;
>         struct tty_struct *tty =3D port->tty;
>         struct tty_ldisc *ld;
>
>         lockdep_assert_held_once(&uport->lock);
>
>         if (tty) {
>                 ld =3D 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 =3D false;
>                                 uport->ops->start_tx(uport);
>                                 uart_write_wakeup(uport);
>                         }
>                 } else {
>                         if (!active) {
>                                 uport->hw_stopped =3D 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 *bu=
f,
>                         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 kterm=
ios *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.
>

>=20
> 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.

--=20
An old man doll... just what I always wanted! - Clara

--nKxqMyvWjc07RdJB
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSSYQ6Cy7oyFNCHrUH2uYlJVVFOowUCZjyoZAAKCRD2uYlJVVFO
oxUmAQDP92/mNAgiTMk/ZDg9tWCvhkHcwhlnjEUMlIZodcZ6AgEAv6dwG6ofkitj
gfQ4umEXVPnPvO2XNcFNvcUCm0PZ9A4=
=T0A1
-----END PGP SIGNATURE-----

--nKxqMyvWjc07RdJB--