[PATCH v3 0/3] serial: max310x: honour per-channel DT RS485 properties

[PATCH v3 0/3] serial: max310x: honour per-channel DT RS485 properties

[Tapio Reijonen]

The MAX310x DT binding pulls in /schemas/serial/rs485.yaml via its allOf
list, but the driver has never actually called uart_get_rs485_mode(), so
none of the advertised rs485-* properties take effect at runtime.

This series wires per-channel RS485 DT configuration end to end:

Patch 1 reorders the probe so the gpio_chip is registered before
uart_add_one_port(). A port can then reference one of the chip's own
GPIOs (e.g. rs485-term-gpios = <&max310x ...>) without -EPROBE_DEFER
from its own provider - prerequisite for patch 3.

Patch 2 splits the binding per compatible. Single-channel parts
(max3107, max3108) keep RS485 on the chip node, which is itself the
serial port. Multi-channel parts (max3109, max14830) describe each
channel as a "serial@N" subnode carrying the standard rs485.yaml
properties; the chip node is then only a container and is no longer
itself a serial node, which avoids a serial node that is also the parent
of serial nodes. Being serial nodes, the channels may also host serial
slave devices. (max3109 has channels 0-1, max14830 has 0-3.)

Patch 3 reads each channel's RS485 properties from its own subnode by
temporarily retargeting dev->fwnode while uart_get_rs485_mode() runs.
For single-channel variants, falls back to the chip's own fwnode when no
subnode is present, so existing top-level rs485 DTs keep working.

Note for maintainers: patch 3 mutates the parent SPI/I2C device's
fwnode around the uart_get_rs485_mode() call so the underlying
property/GPIO lookups resolve against the per-channel DT subnode. Probe
is serialised, so the swap is locally safe, but I'd appreciate feedback
on whether this idiom is acceptable. If a cleaner shape is preferred (a
serial_core helper that takes a fwnode directly, or one struct device
per port), I'll respin accordingly.

Tested on max14830 (SPI, 4 ports): each ttyMAXn port comes up with the
rs485 flags and delays configured in its serial@N subnode, and the
termination GPIO sourced from the MAX310x's own gpio_chip is resolved
without probe deferral.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v3:
- dt-bindings: split per compatible so the chip node is a serial node only
  for the single-channel max3107/max3108; the multi-channel max3109/max14830
  are containers with serial@N child nodes and the chip node is no longer
  itself a serial node. This avoids a node that is simultaneously a serial
  node and the parent of serial nodes. (Krzysztof)
- dt-bindings: move the device/DT-model explanation into a top-level
  description.
- Rebase onto v7.1-rc7.
- Patches 1 and 3 (driver) are unchanged; patch 3 still uses the temporary
  fwnode retarget (idiom flagged above).
- Link to v2: https://lore.kernel.org/r/20260601-b4-max310x-rs485-dt-v2-0-a105105f8e70@vaisala.com

Changes in v2:
- dt-bindings: rename the per-port subnode "port@N" -> "serial@N" so each
  channel is a proper serial node (serial.yaml) that can also host a
  serial slave device; "port" is reserved for the graph binding. (Krzysztof)
- dt-bindings: constrain channels per compatible - max3107/max3108 take no
  subnodes (nor #address-cells/#size-cells), max3109 allows 0-1, max14830
  allows 0-3; out-of-range channels now fail dt_binding_check. (Krzysztof)
- serial: max310x: match the "serial" child node name accordingly.
- No change to patch 1; patch 3 still reads rs485 via the temporary fwnode
  retarget (idiom flagged above).
- Link to v1: https://lore.kernel.org/r/20260525-b4-max310x-rs485-dt-v1-0-e6c19b4d5592@vaisala.com

---
Tapio Reijonen (3):
      serial: max310x: register GPIO controller before adding UART ports
      dt-bindings: serial: maxim,max310x: describe per-channel rs485 subnodes
      serial: max310x: honour rs485 properties from per-channel DT subnode

 .../devicetree/bindings/serial/maxim,max310x.yaml  | 92 +++++++++++++++++++++-
 drivers/tty/serial/max310x.c                       | 91 ++++++++++++++++-----
 2 files changed, 160 insertions(+), 23 deletions(-)
---
base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
change-id: 20260525-b4-max310x-rs485-dt-ebff12af9976

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

[PATCH v3 1/3] serial: max310x: register GPIO controller before adding UART ports

[Tapio Reijonen]

The MAX310x exposes four GPIOs per UART port via an in-driver
gpio_chip. devm_gpiochip_add_data() used to run after the per-port
uart_add_one_port() loop, so a device-tree consumer referencing one of
the chip's own GPIOs (for example rs485-term-gpios = <&max310x 0 ...>)
could not resolve it during port registration: the GPIO provider it
waits for is the very driver still trying to register, and the lookup
returns -EPROBE_DEFER on its own provider, deferring probe forever.

Split the per-port setup into two passes around the gpio_chip
registration:

  1. Initialise per-port state - port struct fields, regmap binding,
     IRQ disable, work queues. The gpio_chip callbacks dereference
     s->p[i].regmap via to_max310x_port() and become callable as soon
     as the chip is visible to gpiolib, so every entry must be
     populated first.
  2. devm_gpiochip_add_data() - register the gpio_chip.
  3. Allocate a line, uart_add_one_port(), set_bit(), max310x_power().
     Keeping line allocation, registration and set_bit() together
     preserves the existing "bit set <=> port registered" rollback
     invariant that out_uart relies on.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 54 +++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index ac7d3f197c3a5ce3531d5607f48e21a807314021..5cb7d01e404663dc25b88bc7b4f8df61be2135ec 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1364,17 +1364,12 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 
 	dev_dbg(dev, "Reference clock set to %i Hz\n", uartclk);
 
+	/*
+	 * Set up each port's state before registering the gpiochip,
+	 * since the gpiochip callbacks will read s->p[i].regmap as
+	 * soon as gpiolib exposes the controller.
+	 */
 	for (i = 0; i < devtype->nr; i++) {
-		unsigned int line;
-
-		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);
-		if (line == MAX310X_UART_NRMAX) {
-			ret = -ERANGE;
-			goto out_uart;
-		}
-
-		/* Initialize port data */
-		s->p[i].port.line	= line;
 		s->p[i].port.dev	= dev;
 		s->p[i].port.irq	= irq;
 		s->p[i].port.type	= PORT_MAX310X;
@@ -1404,20 +1399,16 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
 		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
-
-		/* Register port */
-		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
-		if (ret)
-			goto out_uart;
-
-		set_bit(line, max310x_lines);
-
-		/* Go to suspend mode */
-		max310x_power(&s->p[i].port, 0);
 	}
 
 #ifdef CONFIG_GPIOLIB
-	/* Setup GPIO controller */
+	/*
+	 * Register the GPIO controller before adding the UART ports so
+	 * that consumers referencing the chip's own GPIOs from device
+	 * tree (for example rs485-term-gpios = <&max310x ...>) can
+	 * resolve them at uart_add_one_port() time instead of receiving
+	 * -EPROBE_DEFER from their own provider.
+	 */
 	s->gpio.owner		= THIS_MODULE;
 	s->gpio.parent		= dev;
 	s->gpio.label		= devtype->name;
@@ -1434,6 +1425,27 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		goto out_uart;
 #endif
 
+	for (i = 0; i < devtype->nr; i++) {
+		unsigned int line;
+
+		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);
+		if (line == MAX310X_UART_NRMAX) {
+			ret = -ERANGE;
+			goto out_uart;
+		}
+		s->p[i].port.line = line;
+
+		/* Register port */
+		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
+		if (ret)
+			goto out_uart;
+
+		set_bit(line, max310x_lines);
+
+		/* Go to suspend mode */
+		max310x_power(&s->p[i].port, 0);
+	}
+
 	/* Setup interrupt */
 	ret = devm_request_threaded_irq(dev, irq, NULL, max310x_ist,
 					IRQF_ONESHOT | IRQF_SHARED, dev_name(dev), s);

-- 
2.47.3

Re: [PATCH v3 1/3] serial: max310x: register GPIO controller before adding UART ports

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] The `out_uart` error path relies on zero-initialized `port.line` values, causing it to erroneously clear line 0 from the global bitmap.
- [High] Concurrent allocation of UART lines using the global `max310x_lines` bitmap lacks locking, leading to race conditions and probe failures.
- [High] The shared IRQ handler is registered after the UART ports are exposed to userspace, creating an unhandled interrupt window.
- [High] Missing `cancel_work_sync()` in the `out_uart` error path leaks synchronously queued work items, leading to a Use-After-Free.
- [High] The driver unconditionally powers off the device after registering the port, introducing a TOCTOU race with concurrent userspace access.
- [High] The driver accesses and modifies the `uart_port` state completely outside of `port->lock`, violating fundamental serial core locking rules.
--

commit e3ed4a711cf7a4f6df5863f1c7baec8925661b6f
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: register GPIO controller before adding UART ports

The MAX310x exposes four GPIOs per UART port via an in-driver
gpio_chip. devm_gpiochip_add_data() used to run after the per-port
uart_add_one_port() loop, so a device-tree consumer referencing one of
the chip's own GPIOs (for example rs485-term-gpios = <&max310x 0 ...>)
could not resolve it during port registration: the GPIO provider it
waits for is the very driver still trying to register, and the lookup
returns -EPROBE_DEFER on its own provider, deferring probe forever.

Split the per-port setup into two passes around the gpio_chip
registration.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index ac7d3f197c3a5..5cb7d01e40466 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -1434,6 +1425,27 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  		goto out_uart;
>  #endif
>  
> +	for (i = 0; i < devtype->nr; i++) {
> +		unsigned int line;
> +
> +		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);

[Severity: High]
This is a pre-existing issue, but does the allocation of UART lines using
the global max310x_lines bitmap lack locking?

Multiple instances of the driver probing concurrently on different buses
could simultaneously find the same zero bit and assign the same line number.

Since set_bit is deferred until after uart_add_one_port completes, the
second instance to call uart_add_one_port would fail with -EINVAL. Could
this cause the probe to abort and jump to the out_uart cleanup path?

> +		if (line == MAX310X_UART_NRMAX) {
> +			ret = -ERANGE;
> +			goto out_uart;
> +		}
> +		s->p[i].port.line = line;
> +
> +		/* Register port */
> +		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
> +		if (ret)
> +			goto out_uart;
> +
> +		set_bit(line, max310x_lines);
> +
> +		/* Go to suspend mode */
> +		max310x_power(&s->p[i].port, 0);

[Severity: High]
This is a pre-existing issue, but is there a race condition with userspace
access when unconditionally powering off the device here?

The port is exposed to userspace via uart_add_one_port just before this.
If userspace opens the port in the exact window between these two calls,
max310x_startup will properly power the device on, but then this thread
will immediately execute the power-off sequence.

Could this leave an active port unpowered and completely unresponsive?

> +	}
> +
>  	/* Setup interrupt */
>  	ret = devm_request_threaded_irq(dev, irq, NULL, max310x_ist,
>  					IRQF_ONESHOT | IRQF_SHARED, dev_name(dev), s);

[Severity: High]
This is a pre-existing issue, but does registering the shared IRQ handler
after exposing the UART ports to userspace create an unhandled interrupt
window?

Userspace can immediately open the port, invoking max310x_startup, which
synchronously enables chip-level hardware interrupts via MAX310X_IRQEN_REG.
If the hardware asserts the IRQ line before devm_request_threaded_irq is
called here, the kernel receives an unhandled interrupt.

Since the interrupt is requested with IRQF_SHARED, could this unhandled
interrupt trigger the spurious interrupt detector to permanently disable
the shared interrupt line, causing the device to hang?

[Severity: High]
This is another pre-existing issue, but can the out_uart cleanup path clear
the wrong bit in the max310x_lines bitmap?

If find_first_zero_bit or devm_gpiochip_add_data fails, the code jumps
to out_uart. Because the port structures are zero-initialized via
devm_kzalloc, the port.line field defaults to 0.

The cleanup loop iterates over all ports:

out_uart:
    for (i = 0; i < devtype->nr; i++) {
        if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
            ...

If another max310x device successfully claimed line 0, would its bit be
wrongfully cleared, permanently corrupting the resource tracking for future
allocations?

[Severity: High]
This is also a pre-existing issue, but does the out_uart error path leak
synchronously queued work items?

During port registration, uart_add_one_port invokes max310x_rs485_config
if SER_RS485_ENABLED is set, which unconditionally schedules one->rs_work.

If a subsequent initialization step fails and the probe jumps to out_uart,
the driver structure is freed by devres. However, cancel_work_sync is not
called for rs_work.

When the workqueue scheduler later executes the pending work, will it
dereference freed memory?

[Severity: High]
Finally, this is a pre-existing issue, but does the driver modify the
uart_port state outside of the port->lock?

In max310x_handle_tx, the transmit FIFO is modified using
kfifo_out_linear_ptr and uart_xmit_advance is called. Similarly,
max310x_port_irq calls uart_handle_cts_change.

None of these execution paths acquire spin_lock(&port->lock). Could this
lead to data corruption and inconsistent state when racing with concurrent
TTY core operations, such as tty_write modifying the FIFO?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-max310x-rs485-dt-v3-0-7e79f064bdd7@vaisala.com?part=1

[PATCH v3 2/3] dt-bindings: serial: maxim,max310x: describe per-channel rs485 subnodes

[Tapio Reijonen]

The MAX310x is a family of one- (max3107, max3108), two- (max3109) and
four-channel (max14830) UARTs. The binding pulls in
/schemas/serial/rs485.yaml at the chip level, describing a single set of
RS-485 properties - enough for the single-channel parts, but a
multi-channel chip can wire RS-485 differently on each channel.

Split the binding per compatible:

 - single-channel parts (max3107, max3108): the chip node is itself the
   serial port and carries the RS-485 properties, as before;

 - multi-channel parts (max3109, max14830): the chip node is only a
   container and is no longer a serial node; each channel is a "serial@N"
   subnode that carries the standard serial.yaml/rs485.yaml properties
   (and may host a serial slave device). max3109 has channels 0-1,
   max14830 has 0-3.

This avoids a chip node that is simultaneously a serial node and the
parent of serial nodes. The driver still reads chip-level RS-485 for
single-channel and legacy device trees, so existing users are unaffected.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 .../devicetree/bindings/serial/maxim,max310x.yaml  | 92 +++++++++++++++++++++-
 1 file changed, 90 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/maxim,max310x.yaml b/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
index 889eeaca64a027b4d9e8ec87bcf63fcc8fd9d55b..e598dda4d13f514820ed8012f2fb2fc2aba3e669 100644
--- a/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
+++ b/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
@@ -9,6 +9,13 @@ title: Maxim MAX310X Advanced Universal Asynchronous Receiver-Transmitter (UART)
 maintainers:
   - Hugo Villeneuve <hvilleneuve@dimonoff.com>
 
+description:
+  The MAX310X is a family of SPI/I2C UARTs with one (max3107, max3108),
+  two (max3109) or four (max14830) channels. Single-channel parts are
+  described as a serial node with RS-485 properties on the chip node;
+  multi-channel parts use one "serial@N" child node per channel, each
+  carrying its own serial/RS-485 properties.
+
 properties:
   compatible:
     enum:
@@ -49,8 +56,55 @@ required:
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
-  - $ref: /schemas/serial/serial.yaml#
-  - $ref: /schemas/serial/rs485.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max3107
+              - maxim,max3108
+    then:
+      allOf:
+        - $ref: /schemas/serial/serial.yaml#
+        - $ref: /schemas/serial/rs485.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max3109
+              - maxim,max14830
+    then:
+      properties:
+        "#address-cells":
+          const: 1
+        "#size-cells":
+          const: 0
+      patternProperties:
+        "^serial@[0-3]$":
+          type: object
+          description: A single UART channel of the chip.
+          allOf:
+            - $ref: /schemas/serial/serial.yaml#
+            - $ref: /schemas/serial/rs485.yaml#
+          properties:
+            reg:
+              description: UART channel number on the chip.
+              maximum: 3
+          required:
+            - reg
+          unevaluatedProperties: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: maxim,max3109
+    then:
+      patternProperties:
+        "^serial@[23]$": false
 
 unevaluatedProperties: false
 
@@ -70,5 +124,39 @@ examples:
             interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
             gpio-controller;
             #gpio-cells = <2>;
+            rs485-rts-active-low;
+            linux,rs485-enabled-at-boot-time;
+        };
+    };
+
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        serial@0 {
+            compatible = "maxim,max14830";
+            reg = <0>;
+            spi-max-frequency = <26000000>;
+            clocks = <&xtal4m>;
+            clock-names = "xtal";
+            interrupt-parent = <&gpio3>;
+            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+            gpio-controller;
+            #gpio-cells = <2>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            serial@0 {
+                reg = <0>;
+                rs485-rts-active-low;
+                linux,rs485-enabled-at-boot-time;
+            };
+
+            serial@2 {
+                reg = <2>;
+                rs485-rts-active-low;
+            };
         };
     };

-- 
2.47.3

Re: [PATCH v3 2/3] dt-bindings: serial: maxim,max310x: describe per-channel rs485 subnodes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Schema backward compatibility break for legacy multi-channel device trees by removing $refs to serial/rs485 schemas at the chip level.
--

commit d832a7d0f42d71a0c331f81efe607eabea305da1
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

dt-bindings: serial: maxim,max310x: describe per-channel rs485 subnodes

This commit splits the binding for the MAX310x UART family, introducing
per-channel child nodes for multi-channel parts while keeping single-channel
parts as serial nodes. It removes the top-level serial and RS-485 references
and conditionally maps them based on the compatible string.

> diff --git a/Documentation/devicetree/bindings/serial/maxim,max310x.yaml b/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
> index 889eeaca64a02..e598dda4d13f5 100644
> --- a/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
> +++ b/Documentation/devicetree/bindings/serial/maxim,max310x.yaml
[ ... ]
> @@ -49,8 +56,55 @@ required:
>  
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> -  - $ref: /schemas/serial/serial.yaml#
> -  - $ref: /schemas/serial/rs485.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - maxim,max3107
> +              - maxim,max3108
> +    then:
> +      allOf:
> +        - $ref: /schemas/serial/serial.yaml#
> +        - $ref: /schemas/serial/rs485.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - maxim,max3109
> +              - maxim,max14830
> +    then:
> +      properties:

[Severity: Medium]
The commit message notes that existing users are unaffected because the driver
still reads chip-level RS-485 for single-channel and legacy device trees.

However, does this change break schema validation for legacy multi-channel
device trees?

By removing the top-level references to serial.yaml and rs485.yaml for
multi-channel parts, and because the binding uses unevaluatedProperties: false
at the top level, any legacy multi-channel device trees that still declare
serial or RS-485 properties at the chip level will now trigger unevaluated
property validation errors during dtbs_check.

Could we retain the top-level references for multi-channel parts, perhaps with
a deprecation note, to preserve ABI backward compatibility for existing device
trees?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-max310x-rs485-dt-v3-0-7e79f064bdd7@vaisala.com?part=2

[PATCH v3 3/3] serial: max310x: honour rs485 properties from per-channel DT subnode

[Tapio Reijonen]

The MAX310x DT binding pulls in /schemas/serial/rs485.yaml via its allOf
list, advertising the rs485-* properties defined there - none of which
were honoured at runtime, because the driver never called
uart_get_rs485_mode().

All channels share the parent SPI/I2C device, so uart_get_rs485_mode()
called directly on each port would read the same chip-level fwnode for
every call. Walk dev->of_node's children for the "serial@N" subnode
with matching reg, and temporarily retarget the parent device's fwnode
while uart_get_rs485_mode() runs, so each channel picks up its own
subnode's properties. Probe is serialised, so the swap is safe.

For single-channel variants (max3107, max3108), fall back to the chip's
own fwnode when no subnode is present, so existing DTs that declare
rs485 properties at the top level keep working.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 5cb7d01e404663dc25b88bc7b4f8df61be2135ec..aee3b75fff000385a7543f099663c8a0e4a7d014 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1426,6 +1426,9 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 #endif
 
 	for (i = 0; i < devtype->nr; i++) {
+		struct fwnode_handle *saved_fwnode = dev_fwnode(dev);
+		struct device_node *port_np = NULL;
+		struct device_node *child;
 		unsigned int line;
 
 		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);
@@ -1435,6 +1438,40 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		}
 		s->p[i].port.line = line;
 
+		/* Locate the matching "serial@i" DT subnode, if any. */
+		for_each_available_child_of_node(dev->of_node, child) {
+			u32 reg;
+
+			if (!of_node_name_eq(child, "serial"))
+				continue;
+			if (of_property_read_u32(child, "reg", &reg))
+				continue;
+			if (reg == i) {
+				port_np = child;
+				break;
+			}
+		}
+
+		/*
+		 * Temporarily retarget dev's fwnode to the per-port subnode
+		 * so uart_get_rs485_mode() picks up the per-port properties.
+		 * For single-port variants, fall back to the chip's own
+		 * fwnode so legacy DTs that declare rs485 properties at the
+		 * top level keep working.
+		 */
+		if (port_np) {
+			device_set_node(dev, of_fwnode_handle(port_np));
+			ret = uart_get_rs485_mode(&s->p[i].port);
+			device_set_node(dev, saved_fwnode);
+			of_node_put(port_np);
+			if (ret)
+				goto out_uart;
+		} else if (devtype->nr == 1) {
+			ret = uart_get_rs485_mode(&s->p[i].port);
+			if (ret)
+				goto out_uart;
+		}
+
 		/* Register port */
 		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
 		if (ret)

-- 
2.47.3

Re: [PATCH v3 3/3] serial: max310x: honour rs485 properties from per-channel DT subnode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Temporary modification of registered device's `fwnode` creates a data race with sysfs/bus iterators.

Pre-existing issues:
- [Critical] Uninitialized port line values in `out_uart` cleanup path cause deregistration of other devices' serial ports.
- [High] Unprotected global bitmap `max310x_lines` allows concurrent probes to allocate the same UART line, leading to state corruption.
- [High] Missing registration ordering: UART port is exposed to userspace before its interrupt handler is registered.
--

commit 0bd4955b2ec9441be6e7a42bfe7ca700f31859d2
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: honour rs485 properties from per-channel DT subnode

This commit updates the MAX310x driver to parse rs485 properties from
per-channel device tree subnodes by temporarily retargeting the parent
device's fwnode. It falls back to the top-level node for single-channel
variants.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -1426,6 +1426,9 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  #endif
>  
>  	for (i = 0; i < devtype->nr; i++) {
> +		struct fwnode_handle *saved_fwnode = dev_fwnode(dev);
> +		struct device_node *port_np = NULL;
> +		struct device_node *child;
>  		unsigned int line;
>  
>  		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);

[Severity: High]
This is a pre-existing issue, but does this lockless read of the global
bitmap allow concurrent probes to allocate the same UART line?

Since find_first_zero_bit() is called here, but set_bit() is only called
much later after port registration and DT parsing, multiple asynchronous
probes could observe the same zero bit. 

Could the second thread fail in uart_add_one_port() due to a collision, jump
to the cleanup path, and erroneously clear the bit that the first thread
successfully registered?

>  		if (line == MAX310X_UART_NRMAX) {
>  			ret = -ERANGE;
>  			goto out_uart;
>  		}
>  		s->p[i].port.line = line;
>  
> +		/* Locate the matching "serial@i" DT subnode, if any. */
> +		for_each_available_child_of_node(dev->of_node, child) {
[ ... ]
> +		/*
> +		 * Temporarily retarget dev's fwnode to the per-port subnode
> +		 * so uart_get_rs485_mode() picks up the per-port properties.
> +		 * For single-port variants, fall back to the chip's own
> +		 * fwnode so legacy DTs that declare rs485 properties at the
> +		 * top level keep working.
> +		 */
> +		if (port_np) {
> +			device_set_node(dev, of_fwnode_handle(port_np));

[Severity: Medium]
Does temporarily swapping the fwnode on a live device create a data race
with sysfs or bus iterators?

Since the parent SPI/I2C device is already registered with the driver core
and visible in sysfs at this stage, locklessly modifying dev->fwnode and
dev->of_node could expose a torn pointer state to concurrent udev events or
subsystem iterators.

> +			ret = uart_get_rs485_mode(&s->p[i].port);
> +			device_set_node(dev, saved_fwnode);
> +			of_node_put(port_np);
> +			if (ret)
> +				goto out_uart;
> +		} else if (devtype->nr == 1) {
> +			ret = uart_get_rs485_mode(&s->p[i].port);
> +			if (ret)
> +				goto out_uart;
> +		}
>  
>  		/* Register port */
>  		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);

[Severity: High]
This is a pre-existing issue, but could this ordering lead to unhandled
interrupts if the port is opened immediately?

Because uart_add_one_port() makes the port available to userspace, udev or a
getty could open it before the probe loop finishes. If the port is opened
and powers on, the hardware might assert the IRQ line before
devm_request_threaded_irq() is called later in max310x_probe(), potentially
causing the IRQ core to permanently disable the shared interrupt line.

>  		if (ret)
>  			goto out_uart;

[ ... ]

>  out_uart:
>  	for (i = 0; i < devtype->nr; i++) {
>  		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
>  			uart_remove_one_port(&max310x_uart, &s->p[i].port);
>  	}

[Severity: Critical]
This is a pre-existing issue, but can this cleanup path unregister active
ports belonging to completely different devices?

During early probe failure, the allocated port structures are zeroed out,
meaning uninitialized elements have their port.line set to 0. Since this
loop unconditionally iterates over all devtype->nr elements, it will call
test_and_clear_bit() on line 0.

If another max310x instance has already probed and owns line 0, couldn't
this accidentally clear the other device's bit and forcefully unregister its
active port via uart_remove_one_port(), leading to a kernel panic or
use-after-free?

>  out_clk:
>  	clk_disable_unprepare(s->clk);
>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-max310x-rs485-dt-v3-0-7e79f064bdd7@vaisala.com?part=3