Re: [PATCH v6 1/4] tty: serdev: Add mutex lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] tty: serdev: late close in serdev_device_remove() and missed
  driver unbind
- [High] tty: serdev: returning -EALREADY in ttyport_open() breaks
  hci_mrvl probe
- [Low] tty: serdev: dev_WARN() used instead of dev_warn() in
  serdev_device_remove()
--

commit 5353012a0aab4d9161723d4449dfeeb88d3f481c
Author: Markus Probst <markus.probst@posteo.de>

tty: serdev: Add mutex lock

This commit adds mutex locking to serdev-ttyport to serialize open, close,
and receive operations, returns -EALREADY on duplicate opens, makes closing
an already-closed device a no-op, and closes the device with a warning in
serdev_device_remove() if left open.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..86d9cf8215edc 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -142,6 +142,13 @@ void serdev_device_remove(struct serdev_device *serdev)
>  	struct serdev_controller *ctrl = serdev->ctrl;
>  
>  	device_unregister(&serdev->dev);
> +
> +	/* Warn if driver did not close the serial device. */
> +	if (ctrl->ops->close && ctrl->ops->close(ctrl)) {

[Severity: High]
Does closing the controller after device_unregister(&serdev->dev) leave a
use-after-free window on serdev and its devres data?

When a child driver leaves the device open, SERPORT_ACTIVE remains set
while device_unregister() runs device_unbind_cleanup() (freeing devm
resources and clearing drvdata) and put_device() -> serdev_device_release()
-> kfree(serdev), before ctrl->ops->close(ctrl) clears SERPORT_ACTIVE and
ctrl->serdev is set to NULL:

CPU0                                  CPU1
serdev_device_remove()
  device_unregister(&serdev->dev)
    ...
    device_unbind_cleanup()
    kfree(serdev)
                                      ttyport_receive_buf()
                                        serdev_controller_receive_buf()
                                          serdev = ctrl->serdev;
                                          serdev->ops->receive_buf(serdev, ...)
                                            <-- use-after-free
  ctrl->ops->close(ctrl)
  ctrl->serdev = NULL

In addition, serdev_device_remove() only runs when the struct serdev_device
itself is unregistered, not when a client driver is unbound via
serdev_drv_remove() (e.g. module unload or sysfs unbind).  If a driver
leaves the port open across unbind, this fallback close never runs,
leaving SERPORT_ACTIVE set with NULL drvdata and causing any later re-probe
to fail in serdev_device_open() with -EALREADY.

> +		dev_WARN(&ctrl->dev, "Child driver did not close the serial device");
> +		pm_runtime_put(&ctrl->dev);
> +	}

[Severity: Low]
As reported by Greg Kroah-Hartman on v4 [1], this should log a normal
kernel warning rather than a WARN_ON() splat.  dev_WARN() expands to
WARN(1, ...), which still triggers a backtrace and crashes systems with
panic_on_warn; dev_warn() (with a trailing newline) logs at KERN_WARNING
without a WARN trap.

[ ... ]

> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index b6638f9f40d2d..17c18d4b0cb8f 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
[ ... ]
> @@ -99,14 +103,23 @@ static int ttyport_open(struct serdev_controller *ctrl)
>  	struct ktermios ktermios;
>  	int ret;
>  
> +	mutex_lock(&serport->lock);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags)) {
> +		ret = -EALREADY;
> +		goto err_flags_unlock;
> +	}

[Severity: High]
Does returning -EALREADY here break probing of Marvell serdev Bluetooth
devices in drivers/bluetooth/hci_mrvl.c?

During probe, mrvl_serdev_probe() calls hci_uart_register_device() ->
hci_uart_register_device_priv(), which opens hu->serdev and then calls
p->open(hu):

drivers/bluetooth/hci_serdev.c:hci_uart_register_device_priv() {
	...
	err = serdev_device_open(hu->serdev);
	if (err)
		goto err_rwsem;

	err = p->open(hu);
	if (err)
		goto err_open;
	...
}

mrvl_open() then calls serdev_device_open(hu->serdev) a second time:

drivers/bluetooth/hci_mrvl.c:mrvl_open() {
	...
	if (hu->serdev) {
		ret = serdev_device_open(hu->serdev);
		if (ret)
			goto err;
	}
	...
}

With ttyport_open() returning -EALREADY when SERPORT_ACTIVE is already set,
mrvl_open() and mrvl_serdev_probe() will unconditionally fail.

[1] https://lore.kernel.org/r/2026101758-confidant-comprised-e1c0@gregkh

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-rust_serdev_probe_refactor-v6-0-3568ed56264c@posteo.de?part=1