* [PATCH v3 1/4] tty: serdev: Add mutex lock
2026-09-30 17:32 [PATCH v3 0/4] rust: serdev: Refactor Markus Probst
@ 2026-09-30 17:32 ` Markus Probst
2026-09-30 17:50 ` sashiko-bot
2026-09-30 17:32 ` [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls Markus Probst
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:32 UTC (permalink / raw)
To: Ayush Singh, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Jiri Slaby, Rafael J. Wysocki
Cc: greybus-dev, linux-serial, rust-for-linux, linux-kernel,
driver-core, Markus Probst
Besides more predictable behaviour, this allows for several hardened
behaviour changes:
Return -EALREADY in `serdev_device_open` if the device is already open
instead of causing undefined behaviour.
Allow calling `serdev_device_close`, even if the device is already
closed instead of causing a null pointer dereference.
If the device is left open by the driver after remove, close it and warn
instead of leaving it in a invalid state.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
drivers/tty/serdev/core.c | 10 +++++++---
drivers/tty/serdev/serdev-ttyport.c | 35 +++++++++++++++++++++++++++++------
include/linux/serdev.h | 2 +-
3 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 7500efcdfc21..77e8e1d4d2a6 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);
@@ -181,9 +186,8 @@ void serdev_device_close(struct serdev_device *serdev)
if (!ctrl || !ctrl->ops->close)
return;
- pm_runtime_put(&ctrl->dev);
-
- ctrl->ops->close(ctrl);
+ if (ctrl->ops->close(ctrl))
+ pm_runtime_put(&ctrl->dev);
}
EXPORT_SYMBOL_GPL(serdev_device_close);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index bab1b143b8a6..c11908f5e1ce 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -16,6 +16,7 @@ struct serport {
struct tty_driver *tty_drv;
int tty_idx;
unsigned long flags;
+ struct mutex lock; /* lock preventing modification of flags */
};
/*
@@ -29,6 +30,8 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
struct serport *serport = serdev_controller_get_drvdata(ctrl);
size_t ret;
+ guard(mutex)(&serport->lock);
+
if (!test_bit(SERPORT_ACTIVE, &serport->flags))
return 0;
@@ -99,14 +102,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;
+ }
+
tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
- if (IS_ERR(tty))
- return PTR_ERR(tty);
+ if (IS_ERR(tty)) {
+ ret = PTR_ERR(tty);
+ goto err_flags_unlock;
+ }
serport->tty = tty;
if (!tty->ops->open || !tty->ops->close) {
ret = -ENODEV;
- goto err_unlock;
+ goto err_tty_unlock;
}
ret = tty->ops->open(serport->tty, NULL);
@@ -130,23 +142,30 @@ static int ttyport_open(struct serdev_controller *ctrl)
set_bit(SERPORT_ACTIVE, &serport->flags);
+ mutex_unlock(&serport->lock);
+
return 0;
err_close:
tty->ops->close(tty, NULL);
-err_unlock:
+err_tty_unlock:
tty_unlock(tty);
tty_release_struct(tty, serport->tty_idx);
+err_flags_unlock:
+ mutex_unlock(&serport->lock);
return ret;
}
-static void ttyport_close(struct serdev_controller *ctrl)
+static bool ttyport_close(struct serdev_controller *ctrl)
{
struct serport *serport = serdev_controller_get_drvdata(ctrl);
struct tty_struct *tty = serport->tty;
- clear_bit(SERPORT_ACTIVE, &serport->flags);
+ guard(mutex)(&serport->lock);
+
+ if (!__test_and_clear_bit(SERPORT_ACTIVE, &serport->flags))
+ return false;
tty_lock(tty);
if (tty->ops->close)
@@ -154,6 +173,8 @@ static void ttyport_close(struct serdev_controller *ctrl)
tty_unlock(tty);
tty_release_struct(tty, serport->tty_idx);
+
+ return true;
}
static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
@@ -288,6 +309,8 @@ struct device *serdev_tty_port_register(struct tty_port *port,
port->client_ops = &client_ops;
port->client_data = ctrl;
+ mutex_init(&serport->lock);
+
ret = serdev_controller_add(ctrl);
if (ret)
goto err_reset_data;
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index b6c3d957ec15..0f4e81c0950d 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -81,7 +81,7 @@ struct serdev_controller_ops {
ssize_t (*write_buf)(struct serdev_controller *, const u8 *, size_t);
void (*write_flush)(struct serdev_controller *);
int (*open)(struct serdev_controller *);
- void (*close)(struct serdev_controller *);
+ bool (*close)(struct serdev_controller *);
void (*set_flow_control)(struct serdev_controller *, bool);
int (*set_parity)(struct serdev_controller *, enum serdev_parity);
unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/4] tty: serdev: Add mutex lock
2026-09-30 17:32 ` [PATCH v3 1/4] tty: serdev: Add mutex lock Markus Probst
@ 2026-09-30 17:50 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-09-30 17:50 UTC (permalink / raw)
To: Markus Probst; +Cc: ojeda, linux-serial, gary
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serdev: ABBA deadlock between serport->lock and TTY workqueue
- [Medium] pm: serdev: cancelled async pm_runtime_put leaks power state
--
commit f8d69b3702358f814be05474626c0ebf27b17939
Author: Markus Probst <markus.probst@posteo.de>
tty: serdev: Add mutex lock
Besides more predictable behaviour, this allows for several hardened
behaviour changes:
Return -EALREADY in serdev_device_open if the device is already open
instead of causing undefined behaviour.
> 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);
> +
[Severity: Medium]
When a driver fails to close the device, this calls an asynchronous
pm_runtime_put(). If this removal occurs during controller teardown,
serdev_controller_remove() will immediately call
pm_runtime_disable(&ctrl->dev).
Disabling runtime PM explicitly cancels any pending asynchronous idle/suspend
work. Does this leave the device permanently powered on? Would a synchronous
pm_runtime_put_sync() prevent this leak?
> ctrl->serdev = NULL;
> }
> EXPORT_SYMBOL_GPL(serdev_device_remove);
[ ... ]
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bab1b143b8a68..c11908f5e1ce3 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
[ ... ]
> @@ -29,6 +30,8 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> struct serport *serport = serdev_controller_get_drvdata(ctrl);
> size_t ret;
>
> + guard(mutex)(&serport->lock);
> +
[Severity: High]
This is a pre-existing issue, but does attempting to acquire the lock here
trigger a use-after-free if the device is actively receiving data when
unregistered?
When serdev_tty_port_unregister() drops the reference to ctrl, it frees both
ctrl and serport without synchronously cancelling the flush_to_ldisc TTY
workqueue. If that workqueue is executing concurrently, it will call
ttyport_receive_buf() and try to acquire serport->lock on the freed memory.
Also, does this lock attempt cause an ABBA deadlock?
ttyport_close() acquires serport->lock, then calls tty_release_struct().
tty_release_struct() synchronously waits for the flush_to_ldisc workqueue
to drain via cancel_work_sync(). If that workqueue is running concurrently,
it calls ttyport_receive_buf() and blocks here trying to acquire the same
serport->lock, deadlocking the system.
> if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> return 0;
>
[ ... ]
> @@ -130,23 +142,30 @@ static int ttyport_open(struct serdev_controller *ctrl)
>
> set_bit(SERPORT_ACTIVE, &serport->flags);
>
> + mutex_unlock(&serport->lock);
> +
> return 0;
>
> err_close:
> tty->ops->close(tty, NULL);
> -err_unlock:
> +err_tty_unlock:
> tty_unlock(tty);
> tty_release_struct(tty, serport->tty_idx);
> +err_flags_unlock:
> + mutex_unlock(&serport->lock);
[Severity: High]
Does this error path trigger the same deadlock described above? It calls
tty_release_struct() while holding serport->lock.
>
> return ret;
> }
>
> -static void ttyport_close(struct serdev_controller *ctrl)
> +static bool ttyport_close(struct serdev_controller *ctrl)
> {
> struct serport *serport = serdev_controller_get_drvdata(ctrl);
> struct tty_struct *tty = serport->tty;
>
> - clear_bit(SERPORT_ACTIVE, &serport->flags);
> + guard(mutex)(&serport->lock);
> +
[Severity: High]
This is a pre-existing issue, but does reading serport->tty into a local
variable before acquiring the lock bypass the lock's protection?
If ttyport_close() runs concurrently with ttyport_open(), it may read a stale
or NULL tty pointer before acquiring the lock. After ttyport_open() finishes
and releases the lock, ttyport_close() acquires it, passes the
SERPORT_ACTIVE check, and then dereferences the stale tty pointer.
> + if (!__test_and_clear_bit(SERPORT_ACTIVE, &serport->flags))
> + return false;
>
> tty_lock(tty);
> if (tty->ops->close)
> @@ -154,6 +173,8 @@ static void ttyport_close(struct serdev_controller *ctrl)
> tty_unlock(tty);
>
> tty_release_struct(tty, serport->tty_idx);
> +
> + return true;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de?part=1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls
2026-09-30 17:32 [PATCH v3 0/4] rust: serdev: Refactor Markus Probst
2026-09-30 17:32 ` [PATCH v3 1/4] tty: serdev: Add mutex lock Markus Probst
@ 2026-09-30 17:32 ` Markus Probst
2026-09-30 17:41 ` Markus Probst
2026-09-30 17:43 ` sashiko-bot
2026-09-30 17:32 ` [PATCH v3 3/4] rust: serdev: Replace `active` mutex with receive pause Markus Probst
2026-09-30 17:32 ` [PATCH v3 4/4] rust: serdev: Simplify callbacks Markus Probst
3 siblings, 2 replies; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:32 UTC (permalink / raw)
To: Ayush Singh, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Jiri Slaby, Rafael J. Wysocki
Cc: greybus-dev, linux-serial, rust-for-linux, linux-kernel,
driver-core, Markus Probst
These functions will be used to simply the serdev rust abstraction. It
also contributes to the fixing of 2 race conditions in the serdev rust
abstraction.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
drivers/tty/serdev/core.c | 44 ++++++++++++++++++++++++++++++++++++-
drivers/tty/serdev/serdev-ttyport.c | 33 ++++++++++++++++++++++++++++
include/linux/serdev.h | 6 +++++
3 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 77e8e1d4d2a6..af1470e33620 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -191,6 +191,45 @@ void serdev_device_close(struct serdev_device *serdev)
}
EXPORT_SYMBOL_GPL(serdev_device_close);
+/**
+ * serdev_device_pause_rx() - pause data receive
+ * @serdev: serdev device
+ *
+ * Pause calls to receive_buf.
+ *
+ * Note that if a call to receive_buf is currently executed, the function will
+ * sleep until it has finished.
+ */
+void serdev_device_pause_rx(struct serdev_device *serdev)
+{
+ struct serdev_controller *ctrl = serdev->ctrl;
+
+ if (!ctrl || !ctrl->ops->pause_rx)
+ return;
+
+ ctrl->ops->pause_rx(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_pause_rx);
+
+/**
+ * serdev_device_resume_rx() - resume data receive
+ * @serdev: serdev device
+ *
+ * Resume calls to receive_buf.
+ *
+ * This can be called even if not paused to ensure data receive is active.
+ */
+void serdev_device_resume_rx(struct serdev_device *serdev)
+{
+ struct serdev_controller *ctrl = serdev->ctrl;
+
+ if (!ctrl || !ctrl->ops->resume_rx)
+ return;
+
+ ctrl->ops->resume_rx(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_resume_rx);
+
static void devm_serdev_device_close(void *serdev)
{
serdev_device_close(serdev);
@@ -402,6 +441,7 @@ EXPORT_SYMBOL_GPL(serdev_device_break_ctl);
static int serdev_drv_probe(struct device *dev)
{
const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+ struct serdev_device *sdev = to_serdev_device(dev);
int ret;
ret = dev_pm_domain_attach(dev, PD_FLAG_ATTACH_POWER_ON |
@@ -409,7 +449,9 @@ static int serdev_drv_probe(struct device *dev)
if (ret)
return ret;
- return sdrv->probe(to_serdev_device(dev));
+ serdev_device_resume_rx(sdev);
+
+ return sdrv->probe(sdev);
}
static void serdev_drv_remove(struct device *dev)
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index c11908f5e1ce..2bed0e54202a 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -7,8 +7,10 @@
#include <linux/tty.h>
#include <linux/tty_driver.h>
#include <linux/poll.h>
+#include "../tty.h"
#define SERPORT_ACTIVE 1
+#define SERPORT_PAUSE_RX 2
struct serport {
struct tty_port *port;
@@ -35,6 +37,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
if (!test_bit(SERPORT_ACTIVE, &serport->flags))
return 0;
+ if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
+ return 0;
+
ret = serdev_controller_receive_buf(ctrl, cp, count);
dev_WARN_ONCE(&ctrl->dev, ret > count,
@@ -177,6 +182,32 @@ static bool ttyport_close(struct serdev_controller *ctrl)
return true;
}
+static void ttyport_pause_rx(struct serdev_controller *ctrl)
+{
+ struct serport *serport = serdev_controller_get_drvdata(ctrl);
+ struct tty_struct *tty = serport->tty;
+
+ guard(mutex)(&serport->lock);
+
+ set_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+ if (test_bit(SERPORT_ACTIVE, &serport->flags))
+ tty_buffer_flush_work(tty->port);
+}
+
+static void ttyport_resume_rx(struct serdev_controller *ctrl)
+{
+ struct serport *serport = serdev_controller_get_drvdata(ctrl);
+ struct tty_struct *tty = serport->tty;
+
+ guard(mutex)(&serport->lock);
+
+ clear_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+ if (test_bit(SERPORT_ACTIVE, &serport->flags))
+ tty_buffer_restart_work(tty->port);
+}
+
static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
{
struct serport *serport = serdev_controller_get_drvdata(ctrl);
@@ -281,6 +312,8 @@ static const struct serdev_controller_ops ctrl_ops = {
.get_tiocm = ttyport_get_tiocm,
.set_tiocm = ttyport_set_tiocm,
.break_ctl = ttyport_break_ctl,
+ .pause_rx = ttyport_pause_rx,
+ .resume_rx = ttyport_resume_rx,
};
struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 0f4e81c0950d..02773554e759 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -89,6 +89,8 @@ struct serdev_controller_ops {
int (*get_tiocm)(struct serdev_controller *);
int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
int (*break_ctl)(struct serdev_controller *ctrl, unsigned int break_state);
+ void (*pause_rx)(struct serdev_controller *ctrl);
+ void (*resume_rx)(struct serdev_controller *ctrl);
};
/**
@@ -194,6 +196,8 @@ static inline size_t serdev_controller_receive_buf(struct serdev_controller *ctr
int serdev_device_open(struct serdev_device *);
void serdev_device_close(struct serdev_device *);
int devm_serdev_device_open(struct device *, struct serdev_device *);
+void serdev_device_pause_rx(struct serdev_device *serdev);
+void serdev_device_resume_rx(struct serdev_device *serdev);
unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
void serdev_device_set_flow_control(struct serdev_device *, bool);
int serdev_device_write_buf(struct serdev_device *, const u8 *, size_t);
@@ -233,6 +237,8 @@ static inline int serdev_device_open(struct serdev_device *sdev)
return -ENODEV;
}
static inline void serdev_device_close(struct serdev_device *sdev) {}
+static inline void serdev_device_pause_rx(struct serdev_device *serdev) {}
+static inline void serdev_device_resume_rx(struct serdev_device *serdev) {}
static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev, unsigned int baudrate)
{
return 0;
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls
2026-09-30 17:32 ` [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls Markus Probst
@ 2026-09-30 17:41 ` Markus Probst
2026-09-30 17:43 ` sashiko-bot
1 sibling, 0 replies; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:41 UTC (permalink / raw)
To: Ayush Singh, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Jiri Slaby, Rafael J. Wysocki
Cc: greybus-dev, linux-serial, rust-for-linux, linux-kernel,
driver-core
[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]
On Wed, 2026-09-30 at 17:32 +0000, Markus Probst wrote:
> These functions will be used to simply the serdev rust abstraction. It
> also contributes to the fixing of 2 race conditions in the serdev rust
> abstraction.
>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> drivers/tty/serdev/core.c | 44 ++++++++++++++++++++++++++++++++++++-
> drivers/tty/serdev/serdev-ttyport.c | 33 ++++++++++++++++++++++++++++
> include/linux/serdev.h | 6 +++++
> 3 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 77e8e1d4d2a6..af1470e33620 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -191,6 +191,45 @@ void serdev_device_close(struct serdev_device *serdev)
> }
> EXPORT_SYMBOL_GPL(serdev_device_close);
>
> +/**
> + * serdev_device_pause_rx() - pause data receive
> + * @serdev: serdev device
> + *
> + * Pause calls to receive_buf.
> + *
> + * Note that if a call to receive_buf is currently executed, the function will
> + * sleep until it has finished.
> + */
> +void serdev_device_pause_rx(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->pause_rx)
> + return;
> +
> + ctrl->ops->pause_rx(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_pause_rx);
> +
> +/**
> + * serdev_device_resume_rx() - resume data receive
> + * @serdev: serdev device
> + *
> + * Resume calls to receive_buf.
> + *
> + * This can be called even if not paused to ensure data receive is active.
> + */
> +void serdev_device_resume_rx(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->resume_rx)
> + return;
> +
> + ctrl->ops->resume_rx(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_resume_rx);
> +
> static void devm_serdev_device_close(void *serdev)
> {
> serdev_device_close(serdev);
> @@ -402,6 +441,7 @@ EXPORT_SYMBOL_GPL(serdev_device_break_ctl);
> static int serdev_drv_probe(struct device *dev)
> {
> const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> + struct serdev_device *sdev = to_serdev_device(dev);
> int ret;
>
> ret = dev_pm_domain_attach(dev, PD_FLAG_ATTACH_POWER_ON |
> @@ -409,7 +449,9 @@ static int serdev_drv_probe(struct device *dev)
> if (ret)
> return ret;
>
> - return sdrv->probe(to_serdev_device(dev));
> + serdev_device_resume_rx(sdev);
> +
> + return sdrv->probe(sdev);
> }
>
> static void serdev_drv_remove(struct device *dev)
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index c11908f5e1ce..2bed0e54202a 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -7,8 +7,10 @@
> #include <linux/tty.h>
> #include <linux/tty_driver.h>
> #include <linux/poll.h>
> +#include "../tty.h"
>
> #define SERPORT_ACTIVE 1
> +#define SERPORT_PAUSE_RX 2
>
> struct serport {
> struct tty_port *port;
> @@ -35,6 +37,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> return 0;
>
> + if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
> + return 0;
> +
> ret = serdev_controller_receive_buf(ctrl, cp, count);
>
> dev_WARN_ONCE(&ctrl->dev, ret > count,
> @@ -177,6 +182,32 @@ static bool ttyport_close(struct serdev_controller *ctrl)
> return true;
> }
>
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
> + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
> +
> + guard(mutex)(&serport->lock);
> +
> + set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> + tty_buffer_flush_work(tty->port);
I just realised this could result in a deadlock.
Since the mutex already prevents calling of
`serdev_controller_receive_buf`, it is safe to completely remove this
section (if statement + `tty_buffer_flush_work`).
Thanks
- Markus Probst
> +}
> +
> +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> +{
> + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
> +
> + guard(mutex)(&serport->lock);
> +
> + clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> + tty_buffer_restart_work(tty->port);
> +}
> +
> static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
> {
> struct serport *serport = serdev_controller_get_drvdata(ctrl);
> @@ -281,6 +312,8 @@ static const struct serdev_controller_ops ctrl_ops = {
> .get_tiocm = ttyport_get_tiocm,
> .set_tiocm = ttyport_set_tiocm,
> .break_ctl = ttyport_break_ctl,
> + .pause_rx = ttyport_pause_rx,
> + .resume_rx = ttyport_resume_rx,
> };
>
> struct device *serdev_tty_port_register(struct tty_port *port,
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 0f4e81c0950d..02773554e759 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -89,6 +89,8 @@ struct serdev_controller_ops {
> int (*get_tiocm)(struct serdev_controller *);
> int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
> int (*break_ctl)(struct serdev_controller *ctrl, unsigned int break_state);
> + void (*pause_rx)(struct serdev_controller *ctrl);
> + void (*resume_rx)(struct serdev_controller *ctrl);
> };
>
> /**
> @@ -194,6 +196,8 @@ static inline size_t serdev_controller_receive_buf(struct serdev_controller *ctr
> int serdev_device_open(struct serdev_device *);
> void serdev_device_close(struct serdev_device *);
> int devm_serdev_device_open(struct device *, struct serdev_device *);
> +void serdev_device_pause_rx(struct serdev_device *serdev);
> +void serdev_device_resume_rx(struct serdev_device *serdev);
> unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
> void serdev_device_set_flow_control(struct serdev_device *, bool);
> int serdev_device_write_buf(struct serdev_device *, const u8 *, size_t);
> @@ -233,6 +237,8 @@ static inline int serdev_device_open(struct serdev_device *sdev)
> return -ENODEV;
> }
> static inline void serdev_device_close(struct serdev_device *sdev) {}
> +static inline void serdev_device_pause_rx(struct serdev_device *serdev) {}
> +static inline void serdev_device_resume_rx(struct serdev_device *serdev) {}
> static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev, unsigned int baudrate)
> {
> return 0;
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls
2026-09-30 17:32 ` [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls Markus Probst
2026-09-30 17:41 ` Markus Probst
@ 2026-09-30 17:43 ` sashiko-bot
2026-09-30 17:48 ` Markus Probst
1 sibling, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-09-30 17:43 UTC (permalink / raw)
To: Markus Probst; +Cc: ojeda, gary, linux-serial
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: serdev: AB-BA deadlock between ttyport_pause_rx() and ttyport_receive_buf()
- [High] tty: serdev: Self-deadlock calling pause_rx or resume_rx from receive_buf
- [High] tty: serdev: TOCTOU race leading to NULL pointer dereference in ttyport_close/pause/resume
--
commit f462dbe8ac06c8a92e0fb79ce15c1e6444a27110
Author: Markus Probst <markus.probst@posteo.de>
tty: serdev: Export functions to pause receive_buf callback calls
These functions will be used to simply the serdev rust abstraction. It
also contributes to the fixing of 2 race conditions in the serdev rust
abstraction.
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index c11908f5e1ce3..2bed0e54202a8 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
[ ... ]
> @@ -35,6 +37,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> return 0;
>
> + if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
> + return 0;
> +
> ret = serdev_controller_receive_buf(ctrl, cp, count);
[Severity: High]
Can this cause a self-deadlock if the driver attempts to pause or resume
reception during data reception?
If the downstream driver's receive callback calls serdev_device_pause_rx()
or serdev_device_resume_rx(), they will attempt to acquire the non-recursive
serport->lock which is already held by ttyport_receive_buf().
[ ... ]
> @@ -177,6 +182,32 @@ static bool ttyport_close(struct serdev_controller *ctrl)
> return true;
> }
>
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
> + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
[Severity: High]
Is it possible for this unprotected read of serport->tty to introduce a TOCTOU
race leading to a NULL pointer dereference?
If ttyport_pause_rx() races with ttyport_open(), the local tty pointer could
be captured as NULL before acquiring the lock. After acquiring the lock, the
code might observe SERPORT_ACTIVE as true (set by the concurrent open) and
proceed to dereference the stale, NULL tty pointer.
(Note: ttyport_close() also appears to have this pre-existing issue).
> +
> + guard(mutex)(&serport->lock);
> +
> + set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> + tty_buffer_flush_work(tty->port);
[Severity: High]
Could this tty_buffer_flush_work() cause an AB-BA deadlock with
ttyport_receive_buf()?
ttyport_pause_rx() acquires serport->lock and then calls
tty_buffer_flush_work(), which blocks waiting for the currently executing work
to finish. If the current work is flush_to_ldisc(), it calls
ttyport_receive_buf(), which attempts to acquire serport->lock, resulting in
a deadlock.
> +}
> +
> +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> +{
> + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
[Severity: High]
Can this also cause a NULL pointer dereference if it races with ttyport_open()
and captures a NULL tty pointer before acquiring the lock?
> +
> + guard(mutex)(&serport->lock);
> +
> + clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> + tty_buffer_restart_work(tty->port);
> +}
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls
2026-09-30 17:43 ` sashiko-bot
@ 2026-09-30 17:48 ` Markus Probst
0 siblings, 0 replies; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: ojeda, gary, linux-serial
[-- Attachment #1: Type: text/plain, Size: 3947 bytes --]
On Wed, 2026-09-30 at 17:43 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] tty: serdev: AB-BA deadlock between ttyport_pause_rx() and ttyport_receive_buf()
> - [High] tty: serdev: Self-deadlock calling pause_rx or resume_rx from receive_buf
> - [High] tty: serdev: TOCTOU race leading to NULL pointer dereference in ttyport_close/pause/resume
> --
>
> commit f462dbe8ac06c8a92e0fb79ce15c1e6444a27110
> Author: Markus Probst <markus.probst@posteo.de>
>
> tty: serdev: Export functions to pause receive_buf callback calls
>
> These functions will be used to simply the serdev rust abstraction. It
> also contributes to the fixing of 2 race conditions in the serdev rust
> abstraction.
>
> > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> > index c11908f5e1ce3..2bed0e54202a8 100644
> > --- a/drivers/tty/serdev/serdev-ttyport.c
> > +++ b/drivers/tty/serdev/serdev-ttyport.c
>
> [ ... ]
>
> > @@ -35,6 +37,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> > if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> > return 0;
> >
> > + if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
> > + return 0;
> > +
> > ret = serdev_controller_receive_buf(ctrl, cp, count);
>
> [Severity: High]
> Can this cause a self-deadlock if the driver attempts to pause or resume
> reception during data reception?
>
> If the downstream driver's receive callback calls serdev_device_pause_rx()
> or serdev_device_resume_rx(), they will attempt to acquire the non-recursive
> serport->lock which is already held by ttyport_receive_buf().
It was never intended to be called from inside receive_buf.
>
> [ ... ]
>
> > @@ -177,6 +182,32 @@ static bool ttyport_close(struct serdev_controller *ctrl)
> > return true;
> > }
> >
> > +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> > +{
> > + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > + struct tty_struct *tty = serport->tty;
>
> [Severity: High]
> Is it possible for this unprotected read of serport->tty to introduce a TOCTOU
> race leading to a NULL pointer dereference?
>
> If ttyport_pause_rx() races with ttyport_open(), the local tty pointer could
> be captured as NULL before acquiring the lock. After acquiring the lock, the
> code might observe SERPORT_ACTIVE as true (set by the concurrent open) and
> proceed to dereference the stale, NULL tty pointer.
>
> (Note: ttyport_close() also appears to have this pre-existing issue).
True.
>
> > +
> > + guard(mutex)(&serport->lock);
> > +
> > + set_bit(SERPORT_PAUSE_RX, &serport->flags);
> > +
> > + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> > + tty_buffer_flush_work(tty->port);
>
> [Severity: High]
> Could this tty_buffer_flush_work() cause an AB-BA deadlock with
> ttyport_receive_buf()?
>
> ttyport_pause_rx() acquires serport->lock and then calls
> tty_buffer_flush_work(), which blocks waiting for the currently executing work
> to finish. If the current work is flush_to_ldisc(), it calls
> ttyport_receive_buf(), which attempts to acquire serport->lock, resulting in
> a deadlock.
Already mentioned in a comment (true).
>
> > +}
> > +
> > +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> > +{
> > + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > + struct tty_struct *tty = serport->tty;
>
> [Severity: High]
> Can this also cause a NULL pointer dereference if it races with ttyport_open()
> and captures a NULL tty pointer before acquiring the lock?
True.
>
> > +
> > + guard(mutex)(&serport->lock);
> > +
> > + clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> > +
> > + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> > + tty_buffer_restart_work(tty->port);
> > +}
> > +
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] rust: serdev: Replace `active` mutex with receive pause
2026-09-30 17:32 [PATCH v3 0/4] rust: serdev: Refactor Markus Probst
2026-09-30 17:32 ` [PATCH v3 1/4] tty: serdev: Add mutex lock Markus Probst
2026-09-30 17:32 ` [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls Markus Probst
@ 2026-09-30 17:32 ` Markus Probst
2026-09-30 17:42 ` sashiko-bot
2026-09-30 17:32 ` [PATCH v3 4/4] rust: serdev: Simplify callbacks Markus Probst
3 siblings, 1 reply; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:32 UTC (permalink / raw)
To: Ayush Singh, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Jiri Slaby, Rafael J. Wysocki
Cc: greybus-dev, linux-serial, rust-for-linux, linux-kernel,
driver-core, Markus Probst, Sashiko Bot
There are currently 2 race conditions:
- in probe if `Driver::probe` returns Err
- in unbind
. In those cases the driver data will be set to NULL before the serdev
device was closed. If data is received while the driver data is dropped,
the `receive_buf_callback` might try to access the `active` mutex on a
null pointer.
Removing the need for `receive_buf_callback` to lock the `active` mutex
fixes these.
Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-serial/20260905000836.C8FC91F00A3D@smtp.kernel.org/
Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
rust/kernel/serdev.rs | 60 ++++++++++++---------------------------------------
1 file changed, 14 insertions(+), 46 deletions(-)
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..c16d6593a8d2 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -13,13 +13,9 @@
to_result,
VTABLE_DEFAULT_ERROR, //
},
- new_mutex,
of,
prelude::*,
- sync::{
- aref::AlwaysRefCounted,
- Mutex, //
- },
+ sync::aref::AlwaysRefCounted,
time::Jiffies,
types::{
Opaque,
@@ -103,40 +99,11 @@ pub struct PrivateData<'bound, T: Driver> {
#[pin]
driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
open: UnsafeCell<bool>,
- /// Whether `receive_buf_callback` is allowed to call `Driver::receive`.
- ///
- /// If locked, the receive_buf_callback will be blocked on data reception.
- /// This is the case while the driver is being probed or while [`PrivateData`] is being dropped.
- /// This is necessary, because we need to open the serdev device before the driver has been
- /// probed in order to allow it to be configured, which allows `receive_buf_callback` to be
- /// called. Thus we need to block data until probe completes and the driver data becomes
- /// initialized.
- ///
- /// If unlocked and true, the receive_buf_callback will forward the data to
- /// `Driver::receive`. This is the normal state of operation.
- ///
- /// If unlocked and false, the receive_buf_callback will throw away the data.
- /// This is only the case, if the serdev device is open and
- /// - the driver returned an error in probe
- /// or
- /// - the driver data already has been dropped, because it was unbound.
- #[pin]
- active: Mutex<bool>,
}
#[pinned_drop]
impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
fn drop(self: Pin<&mut Self>) {
- let mut active = self.active.lock();
- if *active {
- // SAFETY:
- // - We have exclusive access to `self.driver`.
- // - `self.driver` is guaranteed to be initialized.
- unsafe { (*self.driver.get()).assume_init_drop() };
- *active = false;
- }
- drop(active);
-
// SAFETY: We have exclusive access to `self.open`.
if unsafe { *self.open.get() } {
// SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
@@ -170,7 +137,6 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
sdev: &**sdev,
driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
open: false.into(),
- active <- new_mutex!(false),
}))?;
// SAFETY: We just set drvdata to `PrivateData<'_, T>`.
let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
@@ -178,11 +144,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
// SAFETY: We just set drvdata to `PrivateData<'_, T>`.
drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
});
- let mut active = private_data.active.lock();
-
// SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+ unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
// SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
// to a `serdev_device`.
to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
@@ -199,12 +166,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
// - `private_data.driver` is pinned.
let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
- *active = result.is_ok();
-
- drop(active);
-
result.map(|()| {
private_data.dismiss();
+
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+ unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
+
0
})
})
@@ -231,6 +198,12 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
T::unbind(sdev, data_pinned);
+
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+ unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
+ // SAFETY: We already established that `data` is guaranteed to be initialized.
+ unsafe { data.assume_init_drop() };
}
extern "C" fn receive_buf_callback(
@@ -248,11 +221,6 @@ extern "C" fn receive_buf_callback(
// `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
// and stored a `Pin<KBox<PrivateData<'_, T>>>`.
let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
- let active = private_data.active.lock();
-
- if !*active {
- return length;
- }
// SAFETY: No one has exclusive access to `private_data.driver`.
let data = unsafe { &*private_data.driver.get() };
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 4/4] rust: serdev: Simplify callbacks
2026-09-30 17:32 [PATCH v3 0/4] rust: serdev: Refactor Markus Probst
` (2 preceding siblings ...)
2026-09-30 17:32 ` [PATCH v3 3/4] rust: serdev: Replace `active` mutex with receive pause Markus Probst
@ 2026-09-30 17:32 ` Markus Probst
2026-09-30 17:53 ` sashiko-bot
3 siblings, 1 reply; 11+ messages in thread
From: Markus Probst @ 2026-09-30 17:32 UTC (permalink / raw)
To: Ayush Singh, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Eric Biggers, Ard Biesheuvel,
Lorenzo Stoakes, Vlastimil Babka, Liam R. Howlett,
Uladzislau Rezki, Jiri Slaby, Rafael J. Wysocki
Cc: greybus-dev, linux-serial, rust-for-linux, linux-kernel,
driver-core, Markus Probst
Initialize the driver's private data directly on `PrivateData`.
Introduce `OpenGuard` for resource cleanup.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
rust/kernel/serdev.rs | 131 ++++++++++++++++++++------------------------------
1 file changed, 52 insertions(+), 79 deletions(-)
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index c16d6593a8d2..3f9165e16776 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -17,16 +17,12 @@
prelude::*,
sync::aref::AlwaysRefCounted,
time::Jiffies,
- types::{
- Opaque,
- ScopeGuard, //
- }, //
+ types::Opaque, //
};
use core::{
- cell::UnsafeCell,
marker::PhantomData,
- mem::{offset_of, MaybeUninit},
+ mem::offset_of,
ptr::NonNull, //
};
@@ -92,24 +88,36 @@ unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
}
}
+struct OpenGuard<'bound> {
+ sdev: &'bound Device<device::Bound>,
+}
+
+impl Drop for OpenGuard<'_> {
+ fn drop(&mut self) {
+ // SAFETY:
+ // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
+ // `struct serdev_device`.
+ // - The existence of self proves that the device is open.
+ unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
+ }
+}
+
#[doc(hidden)]
-#[pin_data(PinnedDrop)]
+#[pin_data]
pub struct PrivateData<'bound, T: Driver> {
- sdev: &'bound Device<device::Bound>,
#[pin]
- driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
- open: UnsafeCell<bool>,
+ driver: T::Data<'bound>,
+ open: OpenGuard<'bound>,
}
-#[pinned_drop]
-impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
- fn drop(self: Pin<&mut Self>) {
- // SAFETY: We have exclusive access to `self.open`.
- if unsafe { *self.open.get() } {
- // SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
- // `struct serdev_device`.
- unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
- }
+impl<'bound, T: Driver> PrivateData<'bound, T> {
+ #[inline]
+ fn driver_data(self: Pin<&Self>) -> Pin<&T::Data<'bound>> {
+ // SAFETY: We treat the result as pinned.
+ let inner = unsafe { Pin::into_inner_unchecked(self) };
+
+ // SAFETY: `self.driver` is pinned.
+ unsafe { Pin::new_unchecked(&inner.driver) }
}
}
@@ -134,46 +142,30 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
from_result(|| {
sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
- sdev: &**sdev,
- driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
- open: false.into(),
+ open: {
+ // SAFETY:
+ // - `sdev.as_raw()` is guaranteed to be a valid pointer to
+ // `serdev_device`.
+ // - It is safe to call before open.
+ unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
+
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
+ // `serdev_device`.
+ unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
+ // `serdev_device`.
+ to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
+
+ OpenGuard { sdev }
+ },
+ driver <- T::probe(sdev, info),
}))?;
- // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
- let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
- let private_data = ScopeGuard::new_with_data(private_data, |_| {
- // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
- drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
- });
- // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
- unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
// SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
- unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
-
- // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
- // to a `serdev_device`.
- to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
-
- // SAFETY: We have exclusive access to `private_data.open`.
- unsafe { *private_data.open.get() = true };
-
- let data = T::probe(sdev, info);
+ unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
- // SAFETY: We have exclusive access to `private_data.driver`.
- let driver = unsafe { &mut *private_data.driver.get() };
- // SAFETY:
- // - `driver.as_mut_ptr()` is a valid pointer to uninitialized data.
- // - `private_data.driver` is pinned.
- let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
-
- result.map(|()| {
- private_data.dismiss();
-
- // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
- unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
-
- 0
- })
+ Ok(0)
})
}
@@ -189,21 +181,10 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
// and stored a `Pin<KBox<PrivateData<'_, T>>>`.
let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
- // SAFETY: No one has exclusive access to `private_data.driver`.
- let data = unsafe { &*private_data.driver.get() };
- // SAFETY:
- // - `private_data.driver` is pinned.
- // - `remove_callback` is only ever called after a successful call to `probe_callback`,
- // hence it's guaranteed that `private_data.driver` was initialized.
- let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
-
- T::unbind(sdev, data_pinned);
+ T::unbind(sdev, private_data.driver_data());
// SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
-
- // SAFETY: We already established that `data` is guaranteed to be initialized.
- unsafe { data.assume_init_drop() };
}
extern "C" fn receive_buf_callback(
@@ -211,6 +192,9 @@ extern "C" fn receive_buf_callback(
buf: *const u8,
length: usize,
) -> usize {
+ // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
+ let buf = unsafe { core::slice::from_raw_parts(buf, length) };
+
// SAFETY: The serial device bus only ever calls the receive buf callback with a valid
// pointer to a `struct serdev_device`.
//
@@ -222,18 +206,7 @@ extern "C" fn receive_buf_callback(
// and stored a `Pin<KBox<PrivateData<'_, T>>>`.
let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
- // SAFETY: No one has exclusive access to `private_data.driver`.
- let data = unsafe { &*private_data.driver.get() };
- // SAFETY:
- // - `private_data.driver` is pinned.
- // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
- // hence it's guaranteed that `private_data.driver` was initialized.
- let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
-
- // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
- let buf = unsafe { core::slice::from_raw_parts(buf, length) };
-
- T::receive(sdev, data_pinned, buf)
+ T::receive(sdev, private_data.driver_data(), buf)
}
}
--
2.55.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/4] rust: serdev: Simplify callbacks
2026-09-30 17:32 ` [PATCH v3 4/4] rust: serdev: Simplify callbacks Markus Probst
@ 2026-09-30 17:53 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-09-30 17:53 UTC (permalink / raw)
To: Markus Probst; +Cc: gary, linux-serial, ojeda
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: missing #[inline] on OpenGuard::drop
--
commit bed85685e823335d80519c5fa72de2689c661486
Author: Markus Probst <markus.probst@posteo.de>
rust: serdev: Simplify callbacks
This commit simplifies the serdev callbacks by initializing the driver's
private data directly on PrivateData. It also introduces the OpenGuard
struct to handle device resource cleanup.
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index c16d6593a8d2d..3f9165e167764 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -92,24 +88,36 @@ unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
> }
> }
>
> +struct OpenGuard<'bound> {
> + sdev: &'bound Device<device::Bound>,
> +}
> +
> +impl Drop for OpenGuard<'_> {
> + fn drop(&mut self) {
[Severity: Low]
Does this small abstraction method forwarding to a C binding need the
#[inline] annotation? The Rust subsystem guidelines recommend #[inline]
for small functions or functions forwarding to a C binding call in
abstractions to ensure compiler optimizations across crate boundaries.
> + // SAFETY:
> + // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> + // `struct serdev_device`.
> + // - The existence of self proves that the device is open.
> + unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> + }
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread