| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VfAfd7Ubhh1YxYH2fYJMv3wXUvUzrJSC1=V6HQ-wczYJg@mail.gmail.com>
Date: Mon, 1 Dec 2025 04:40:51 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Crescent Hsieh <crescentcy.hsieh@...a.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
ilpo.jarvinen@...ux.intel.com, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH v1 29/31] serial: 8250_mxpcie: Add sysfs to control terminator
On Sun, Nov 30, 2025 at 12:46 PM Crescent Hsieh
<crescentcy.hsieh@...a.com> wrote:
>
> Some Moxa PCIe serial board variants support software-controlled
> termination resistors on each port. Users currently have no standardized
> interface to enable or disable terminator state from userspace.
>
> This patch introduces the following:
>
> - EXPORT a new sysfs attribute `/sys/class/tty/*/mxpcie8250_terminator`
> to allow users to read or set the terminator state (enabled/disabled).
> - Implement both CPLD-based and GPIO-based paths depending on board model.
> - Track runtime terminator state in `struct mxpcie8250_port`.
> - Serialize CPLD modifications using a new `board_lock` spinlock.
>
> Example usage:
>
> # Enable terminator on ttyS*
> echo 1 > /sys/class/tty/ttyS*/mxpcie8250_terminator
>
> # Check current state
> cat /sys/class/tty/ttyS*/mxpcie8250_terminator
> enabled
This introduces an ABI without documentation update. It's no go.
...
> +#define MOXA_CPLD_STATE_MASK 0x0F
>
> #define MOXA_CPLD_DATA_MASK 0x1F /* Pin0 ~ Pin4 */
> #define MOXA_CPLD_CTRL_MASK 0xE0 /* Pin5 ~ Pin7 */
Use GENMASK() for the above.
...
> struct mxpcie8250_port {
Run `pahole`.
> int line;
> int interface;
> + int terminator;
> unsigned long event_flags;
> u8 rx_trig_level;
> struct uart_port *uport;
> struct mxpcie8250 {
> struct pci_dev *pdev;
> unsigned int supp_rs;
> unsigned int num_ports;
> + spinlock_t board_lock;
> struct mxpcie8250_port port[];
> };
...
> +/**
> + * mxpcie8250_cpld_get_terminator() - Get the terminator state of the specified port
> + * @iobar_addr: The base address of the GPIO I/O region
> + * @port_idx: The port index (0 to 7)
> + *
> + * Reads the terminator from the CPLD by accessing the appropriate GET_STATE
> + * register for the specified port using GPIO-based communication.
> + *
> + * Returns:
> + * MOXA_TERMINATOR_ENABLE if terminator is enabled,
> + * MOXA_TERMINATOR_DISABLE if terminator is disabled.
Have you rendered this with the kernel-doc script? I believe you want
a list, so you need something like
* * List item 1
* * List item 2
> + */
...
> +static void mxpcie8250_do_set_terminator(struct pci_dev *pdev,
> + unsigned int port_idx,
> + u8 state)
> +{
> + struct mxpcie8250 *priv = pci_get_drvdata(pdev);
> + resource_size_t iobar_addr = pci_resource_start(pdev, 2);
> + u8 cval;
> +
> + cval = inb(iobar_addr + MOXA_GPIO_INPUT);
> +
> + switch (pdev->device) {
> + case PCI_DEVICE_ID_MOXA_CP132EL:
> + cval &= ~(1 << (port_idx + 2));
BIT()
> + cval |= (state << (port_idx + 2));
state can be more than one bit set, is it a problem?
> + break;
> + case PCI_DEVICE_ID_MOXA_CP114EL:
> + case PCI_DEVICE_ID_MOXA_CP118EL_A:
> + cval &= ~(1 << port_idx);
> + cval |= (state << port_idx);
Same questions as above.
> + break;
> + default:
> + return;
> + }
> + outb(0xff, iobar_addr + MOXA_GPIO_DIRECTION);
> + outb(cval, iobar_addr + MOXA_GPIO_OUTPUT);
> +
> + priv->port[port_idx].terminator = state;
> +}
...
> +static int mxpcie8250_set_terminator(struct uart_port *port,
> + u8 state)
> +{
> + struct pci_dev *pdev = to_pci_dev(port->dev);
> + struct mxpcie8250 *priv = pci_get_drvdata(pdev);
> + resource_size_t iobar_addr = pci_resource_start(pdev, 2);
Why can't you use what is stored in the "port"? IIRC there are 3
variables: 2 for MMIO (membase/mapbase) and one for IO (iobase).
> + if (priv->port[port->port_id].interface == MOXA_UIR_RS232)
> + return -EINVAL;
> +
> + switch (pdev->device) {
> + case PCI_DEVICE_ID_MOXA_CP116E_A_A:
> + case PCI_DEVICE_ID_MOXA_CP116E_A_B:
> + case PCI_DEVICE_ID_MOXA_CP118E_A_I:
> + case PCI_DEVICE_ID_MOXA_CP134EL_A:
> + case PCI_DEVICE_ID_MOXA_CP138E_A:
> + spin_lock(&priv->board_lock);
> + mxpcie8250_cpld_set_terminator(iobar_addr, port->port_id, state);
> + spin_unlock(&priv->board_lock);
> + break;
> + /* RS232 only, no need to be set */
> + case PCI_DEVICE_ID_MOXA_CP104EL_A:
> + case PCI_DEVICE_ID_MOXA_CP102E:
> + case PCI_DEVICE_ID_MOXA_CP102EL:
> + case PCI_DEVICE_ID_MOXA_CP168EL_A:
> + /* CP100N series doesn't support SW control terminator */
> + case PCI_DEVICE_ID_MOXA_CP102N:
> + case PCI_DEVICE_ID_MOXA_CP132N:
> + case PCI_DEVICE_ID_MOXA_CP112N:
> + case PCI_DEVICE_ID_MOXA_CP104N:
> + case PCI_DEVICE_ID_MOXA_CP134N:
> + case PCI_DEVICE_ID_MOXA_CP114N:
> + return -ENODEV;
> + default:
> + mxpcie8250_do_set_terminator(pdev, port->port_id, state);
Missing break, but see below.
> + }
> +
> + return 0;
Just move it to each case (group of cases) where it's needed.
> +}
...
> +static ssize_t mxpcie8250_terminator_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct tty_port *tport = dev_get_drvdata(dev);
> + struct uart_state *ustate = container_of(tport, struct uart_state, port);
> + struct uart_port *port = ustate->uart_port;
> + int ret;
> + u8 state;
> + if (!count)
> + return -EINVAL;
Dead code?
> + ret = kstrtou8(buf, 10, &state);
Why is this strictly decimal?
> +
Unneeded blank line.
> + if (ret < 0)
> + return ret;
> + if (state != MOXA_TERMINATOR_ENABLE && state != MOXA_TERMINATOR_DISABLE)
> + return -EINVAL;
> +
> + ret = mxpcie8250_set_terminator(port, state);
> +
Ditto.
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
...
> + spin_lock(&priv->board_lock);
> + ret = mxpcie8250_cpld_get_terminator(iobar_addr, port->port_id);
> + spin_unlock(&priv->board_lock);
scoped_guard() from cleanup.h helps here.
...
> + if (ret == MOXA_TERMINATOR_ENABLE)
> + return sysfs_emit(buf, "enabled\n");
> +
> + return sysfs_emit(buf, "disabled\n");
str_enabled_disabled() from string_choices.h helps here.
> +}
> +
Unneeded blank line.
> +static DEVICE_ATTR_RW(mxpcie8250_terminator);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists