Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin

Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hi Greg, hi maintainers,

On Bugzilla, Elvis <elvisimprsntr@gmail.com> requested a new RJ45 UART
feature [1]:

> BACKGROUND
> 
> There are appliances that do not have a full DE9 RS-232 port and only RJ45 COM ports, which do not include the DCD pin required for PPS signal detection.  
> 
> RJ45 COM port do include the CTS input pin #8, which can be used for PPS signal detection.  This is a feature that has been available in FreeBSD for some time.  
> 
> https://man.freebsd.org/cgi/man.cgi?query=uart
> 
> FreeBSD allows switching from DCD to CTS pin on the fly using a system tunable in a loader.conf file.  Example: dev.uart.0.pps_mode="1"
> 
> This allow one to connect a serial GPS receiver with PPS output to a RJ45 COM port for a Stratum 0 GPS+PPS source.  
> 
> https://github.com/elvisimprsntr/pfsense-ntp-gps?tab=readme-ov-file
> 
> KERNAL PATCH HACKS
> 
> Some kernel patches have demonstrated receiving PPS on the CTS pin, but requires hacking up the kernel and will will not persist on kernel update to new installations.  
> 
> https://github.com/not1337/pps-stuff
> 
> FEATURE REQUEST
> 
> Add a tunable option to allow the end user to switch between PPS on DCD to CTS pin, similar to the way FreeBSD does.

What do you think about above feature request?

Thanks.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=218813

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin

[Greg Kroah-Hartman]

On Wed, May 08, 2024 at 08:52:24AM +0700, Bagas Sanjaya wrote:
> What do you think about above feature request?

We will be glad to review any submitted patches for any features.
patches in bugzilla are not viable for obvious reasons.

thanks,

greg k-h

Re: Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin

[Theodore Ts'o]

On Wed, May 08, 2024 at 07:28:35PM +0100, Greg Kroah-Hartman wrote:
> On Wed, May 08, 2024 at 08:52:24AM +0700, Bagas Sanjaya wrote:
> > What do you think about above feature request?
> 
> We will be glad to review any submitted patches for any features.
> patches in bugzilla are not viable for obvious reasons.

Bagas,

Note that the feature request is related to PPS, so per the
MAINTAINERS file entry:

PPS SUPPORT
M:	Rodolfo Giometti <giometti@enneenne.com>
L:	linuxpps@ml.enneenne.com (subscribers-only)
S:	Maintained
W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
F:	Documentation/ABI/testing/sysfs-pps
F:	Documentation/devicetree/bindings/pps/pps-gpio.yaml
F:	Documentation/driver-api/pps.rst
F:	drivers/pps/
F:	include/linux/pps*.h
F:	include/uapi/linux/pps.h

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.

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.

Cheers,

					- Ted

Re: Fwd: Add method to allow switching kernel level PPS signal from DCD to CTS serial pin

[Bagas Sanjaya]

[-- 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 --]