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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serdev: returning -EALREADY in ttyport_open() breaks hci_mrvl probe
- [High] tty: serdev: late close in serdev_device_remove() and missed driver unbind
--

commit 792f1e31002a2c366a7d3535ec38a8a39ce21b0c
Author: Markus Probst <markus.probst@posteo.de>

tty: serdev: Add mutex lock

This commit adds mutexes to serialize serport state changes and receive
buffer handling in serdev-ttyport. It also rejects repeated opens with
-EALREADY, ignores redundant closes, and warns if a device is left open on
removal.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..77e8e1d4d2a66 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -142,6 +142,11 @@ 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 && WARN_ON(ctrl->ops->close(ctrl)))
> +		pm_runtime_put(&ctrl->dev);
> +
>  	ctrl->serdev = NULL;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_remove);

[Severity: High]
Is it safe to call ctrl->ops->close(ctrl) after device_unregister(&serdev->dev)
has already completed?

In serdev_device_remove(), device_unregister(&serdev->dev) calls device_del(),
which unbinds the driver via serdev_drv_remove() and runs
device_unbind_cleanup() (releasing devres resources and clearing drvdata),
followed by put_device(&serdev->dev), which invokes serdev_device_release()
and frees serdev:

drivers/tty/serdev/core.c:serdev_device_release() {
	struct serdev_device *serdev = to_serdev_device(dev);
	kfree(serdev);
}

When a driver leaves the port open across remove, SERPORT_ACTIVE remains set
and ctrl->serdev points to the freed serdev_device throughout
device_unbind_cleanup() and put_device() until ctrl->ops->close(ctrl) clears
SERPORT_ACTIVE. Could a concurrent ttyport_receive_buf() or
ttyport_write_wakeup() during this window dereference the freed ctrl->serdev
or invoke serdev->ops callbacks with NULL or freed drvdata?

Also, serdev_device_remove() is only called when unregistering the
serdev_device itself, not when unbinding a driver via serdev_drv_remove():

drivers/tty/serdev/core.c:serdev_drv_remove() {
	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
	if (sdrv->remove)
		sdrv->remove(to_serdev_device(dev));
}

If a driver leaves the device open on unbind (or after a failed probe),
wouldn't SERPORT_ACTIVE stay set while drvdata is cleared, leaving the port
open and causing any subsequent re-probe to fail in serdev_device_open() with
-EALREADY?

[ ... ]

> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bab1b143b8a68..7d2d50d7e1a48 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]
Won't returning -EALREADY here cause a regression when probing Marvell
Bluetooth serdev devices in drivers/bluetooth/hci_mrvl.c?

During mrvl_serdev_probe(), hci_uart_register_device() calls
hci_uart_register_device_priv(), which opens hu->serdev before invoking the
protocol open callback 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;
	...
}

In drivers/bluetooth/hci_mrvl.c, 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,
won't mrvl_open() fail unconditionally and abort mrvl_serdev_probe()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=1