[PATCH v2 0/3] rust: serdev: Refactor

[PATCH v2 0/3] rust: serdev: Refactor

[Markus Probst]

The following has been changed:
- introduce a new C serdev API to fix race conditions and simplify rust
  code. Should also be useful on the C side.
- fix race conditions
- simplify rust code

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v2:
- fix missing memory barriers in serdev apis
- use `tty_buffer_restart_work` instead of `tty_flip_buffer_push`
- use `tty_buffer_flush_work` instead of exclusively locking the mutex
- add `#[inline]` to `PrivateData::driver_data`
- remove patches for mutable references
- Link to v1: https://patch.msgid.link/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de

---
Markus Probst (3):
      tty: serdev: Export functions to pause receive_buf callback calls
      rust: serdev: Replace `active` mutex with receive pause
      rust: serdev: Simplify callbacks

 drivers/tty/serdev/core.c           |  50 ++++++++++-
 drivers/tty/serdev/serdev-ttyport.c |  38 ++++++++
 include/linux/serdev.h              |   6 ++
 rust/kernel/serdev.rs               | 167 ++++++++++++------------------------
 4 files changed, 147 insertions(+), 114 deletions(-)
---
base-commit: c2cd463d6ee7d55a3ec0719d93c49ff99022d58f
change-id: 20260905-rust_serdev_probe_refactor-0e2b044354a7

[PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[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           | 50 ++++++++++++++++++++++++++++++++++++-
 drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
 include/linux/serdev.h              |  6 +++++
 3 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 7500efcdfc21..7d24f16710cb 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -187,6 +187,51 @@ 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.
+ *
+ * The caller must guarantee that this does not run concurrently with
+ * `serdev_device_open` or `serdev_device_close`.
+ *
+ * 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.
+ *
+ * The caller must guarantee that this does not run concurrently with
+ * `serdev_device_open` or `serdev_device_close`.
+ *
+ * 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);
@@ -398,6 +443,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 |
@@ -405,7 +451,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 bab1b143b8a6..e8aa89e733bd 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;
@@ -32,6 +34,14 @@ 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;
+
+	/*
+	 * Ensure writes by the driver are visible before allowing traffic to resume.
+	 */
+	smp_mb__after_atomic();
+
 	ret = serdev_controller_receive_buf(ctrl, cp, count);
 
 	dev_WARN_ONCE(&ctrl->dev, ret > count,
@@ -156,6 +166,32 @@ static void ttyport_close(struct serdev_controller *ctrl)
 	tty_release_struct(tty, serport->tty_idx);
 }
 
+static void ttyport_pause_rx(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	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;
+
+	/*
+	 * Ensure writes by the driver are visible before allowing traffic to resume.
+	 */
+	smp_mb__before_atomic();
+	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);
@@ -260,6 +296,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 b6c3d957ec15..5cf05df17ddf 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

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Greg Kroah-Hartman]

On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
>  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
>  include/linux/serdev.h              |  6 +++++
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc21..7d24f16710cb 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -187,6 +187,51 @@ 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.
> + *
> + * The caller must guarantee that this does not run concurrently with
> + * `serdev_device_open` or `serdev_device_close`.
> + *
> + * 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.
> + *
> + * The caller must guarantee that this does not run concurrently with
> + * `serdev_device_open` or `serdev_device_close`.
> + *
> + * 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);
> @@ -398,6 +443,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 |
> @@ -405,7 +451,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 bab1b143b8a6..e8aa89e733bd 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;
> @@ -32,6 +34,14 @@ 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;
> +
> +	/*
> +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> +	 */
> +	smp_mb__after_atomic();

This scares me.  Why not use a real lock?  WHat's the issue here, you
need this to be "flushed" before this call:

> +
>  	ret = serdev_controller_receive_buf(ctrl, cp, count);

here?

And you just tested a bit, you didn't set a bit, so what are you trying
to ensure is written exactly?


>  
>  	dev_WARN_ONCE(&ctrl->dev, ret > count,
> @@ -156,6 +166,32 @@ static void ttyport_close(struct serdev_controller *ctrl)
>  	tty_release_struct(tty, serport->tty_idx);
>  }
>  
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))

What keeps this bit from being set right after you test it?



> +		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;
> +
> +	/*
> +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> +	 */
> +	smp_mb__before_atomic();
> +	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))

Same here.  Are you sure you don't need locks?  This feels wrong.

thanks,

greg k-h

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Markus Probst]

[-- Attachment #1: Type: text/plain, Size: 6556 bytes --]

On Wed, 2026-09-23 at 12:35 +0200, Greg Kroah-Hartman wrote:
> On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
> >  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
> >  include/linux/serdev.h              |  6 +++++
> >  3 files changed, 93 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index 7500efcdfc21..7d24f16710cb 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -187,6 +187,51 @@ 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.
> > + *
> > + * The caller must guarantee that this does not run concurrently with
> > + * `serdev_device_open` or `serdev_device_close`.
> > + *
> > + * 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.
> > + *
> > + * The caller must guarantee that this does not run concurrently with
> > + * `serdev_device_open` or `serdev_device_close`.
> > + *
> > + * 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);
> > @@ -398,6 +443,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 |
> > @@ -405,7 +451,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 bab1b143b8a6..e8aa89e733bd 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;
> > @@ -32,6 +34,14 @@ 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;
> > +
> > +	/*
> > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> > +	 */
> > +	smp_mb__after_atomic();
> 
> This scares me.  Why not use a real lock? 
> 
I can use locks to make it less "fragile". But I don't I think I need
them.

> WHat's the issue here, you
> need this to be "flushed" before this call:
> 
> > +
> >  	ret = serdev_controller_receive_buf(ctrl, cp, count);
> 
> here?
> 
> And you just tested a bit, you didn't set a bit, so what are you trying
> to ensure is written exactly?
This should be an acquire load operation (paired with the release store
operation in `ttyport_resume_rx`).

It ensures that any writes before calling `ttyport_resume_rx` are
visible in the `serdev_controller_receive_buf` invocation.


For instance, in the Rust abstraction the following will be called in
order in probe (with the following patches):

- serdev_device_pause_rx
- serdev_device_open
- dev_set_drvdata
- serdev_device_resume_rx

This atomic lock effectively ensures in this example that the set
device driver data is visible to `ttyport_receive_buf` before
`SERPORT_PAUSE_RX` is unset in `ttyport_resume_rx`.

> 
> 
> >  
> >  	dev_WARN_ONCE(&ctrl->dev, ret > count,
> > @@ -156,6 +166,32 @@ static void ttyport_close(struct serdev_controller *ctrl)
> >  	tty_release_struct(tty, serport->tty_idx);
> >  }
> >  
> > +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> > +{
> > +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > +	struct tty_struct *tty = serport->tty;
> > +
> > +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> > +
> > +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> 
> What keeps this bit from being set right after you test it?
The statement "The caller must guarantee that this does not run
concurrently with `serdev_device_open` or `serdev_device_close`." in
the kdoc of `serdev_device_pause_rx` does.
> 
> 
> 
> > +		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;
> > +
> > +	/*
> > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> > +	 */
> > +	smp_mb__before_atomic();
> > +	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> > +
> > +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> 
> Same here.  Are you sure you don't need locks?  This feels wrong.
Same as above.

Thanks
- Markus Probst

> 
> thanks,
> 
> greg k-h

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Gary Guo]

On Wed Sep 23, 2026 at 2:31 PM BST, Markus Probst wrote:
> On Wed, 2026-09-23 at 12:35 +0200, Greg Kroah-Hartman wrote:
>> On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
>> >  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
>> >  include/linux/serdev.h              |  6 +++++
>> >  3 files changed, 93 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
>> > index bab1b143b8a6..e8aa89e733bd 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;
>> > @@ -32,6 +34,14 @@ 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;
>> > +
>> > +	/*
>> > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
>> > +	 */
>> > +	smp_mb__after_atomic();
>> 
>> This scares me.  Why not use a real lock? 
>> 
> I can use locks to make it less "fragile". But I don't I think I need
> them.
>
>> WHat's the issue here, you
>> need this to be "flushed" before this call:
>> 
>> > +
>> >  	ret = serdev_controller_receive_buf(ctrl, cp, count);
>> 
>> here?
>> 
>> And you just tested a bit, you didn't set a bit, so what are you trying
>> to ensure is written exactly?
> This should be an acquire load operation (paired with the release store
> operation in `ttyport_resume_rx`).

Then you should use `test_bit_acquire`.

However, this isn't sufficient because there's no synchronization between this
bit test with the bit set inside pause_rx.

Best,
Gary

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Markus Probst]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On Wed, 2026-09-23 at 14:52 +0100, Gary Guo wrote:
> On Wed Sep 23, 2026 at 2:31 PM BST, Markus Probst wrote:
> > On Wed, 2026-09-23 at 12:35 +0200, Greg Kroah-Hartman wrote:
> > > On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
> > > >  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
> > > >  include/linux/serdev.h              |  6 +++++
> > > >  3 files changed, 93 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> > > > index bab1b143b8a6..e8aa89e733bd 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;
> > > > @@ -32,6 +34,14 @@ 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;
> > > > +
> > > > +	/*
> > > > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> > > > +	 */
> > > > +	smp_mb__after_atomic();
> > > 
> > > This scares me.  Why not use a real lock? 
> > > 
> > I can use locks to make it less "fragile". But I don't I think I need
> > them.
> > 
> > > WHat's the issue here, you
> > > need this to be "flushed" before this call:
> > > 
> > > > +
> > > >  	ret = serdev_controller_receive_buf(ctrl, cp, count);
> > > 
> > > here?
> > > 
> > > And you just tested a bit, you didn't set a bit, so what are you trying
> > > to ensure is written exactly?
> > This should be an acquire load operation (paired with the release store
> > operation in `ttyport_resume_rx`).
> 
> Then you should use `test_bit_acquire`.
Ok. For some reason there is no equivalent `set_bit_release`.

> 
> However, this isn't sufficient because there's no synchronization between this
> bit test with the bit set inside pause_rx.
Will be replaced with a mutex in the next revision.

Thanks
- Markus Probst

> 
> Best,
> Gary

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Greg Kroah-Hartman]

On Wed, Sep 23, 2026 at 01:31:49PM +0000, Markus Probst wrote:
> On Wed, 2026-09-23 at 12:35 +0200, Greg Kroah-Hartman wrote:
> > On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
> > >  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
> > >  include/linux/serdev.h              |  6 +++++
> > >  3 files changed, 93 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > > index 7500efcdfc21..7d24f16710cb 100644
> > > --- a/drivers/tty/serdev/core.c
> > > +++ b/drivers/tty/serdev/core.c
> > > @@ -187,6 +187,51 @@ 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.
> > > + *
> > > + * The caller must guarantee that this does not run concurrently with
> > > + * `serdev_device_open` or `serdev_device_close`.
> > > + *
> > > + * 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.
> > > + *
> > > + * The caller must guarantee that this does not run concurrently with
> > > + * `serdev_device_open` or `serdev_device_close`.
> > > + *
> > > + * 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);
> > > @@ -398,6 +443,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 |
> > > @@ -405,7 +451,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 bab1b143b8a6..e8aa89e733bd 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;
> > > @@ -32,6 +34,14 @@ 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;
> > > +
> > > +	/*
> > > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> > > +	 */
> > > +	smp_mb__after_atomic();
> > 
> > This scares me.  Why not use a real lock? 
> > 
> I can use locks to make it less "fragile". But I don't I think I need
> them.

Always use them first, and then remove and do "tricky" things if you
really can measure the need and can prove that they are not needed.

> > WHat's the issue here, you
> > need this to be "flushed" before this call:
> > 
> > > +
> > >  	ret = serdev_controller_receive_buf(ctrl, cp, count);
> > 
> > here?
> > 
> > And you just tested a bit, you didn't set a bit, so what are you trying
> > to ensure is written exactly?
> This should be an acquire load operation (paired with the release store
> operation in `ttyport_resume_rx`).

Where is the load?  This feels wrong.

> It ensures that any writes before calling `ttyport_resume_rx` are
> visible in the `serdev_controller_receive_buf` invocation.

writes from where?  There wasn't a write before this that I can see in
the diff, hence my confusion.

> For instance, in the Rust abstraction the following will be called in
> order in probe (with the following patches):
> 
> - serdev_device_pause_rx
> - serdev_device_open
> - dev_set_drvdata
> - serdev_device_resume_rx
> 
> This atomic lock effectively ensures in this example that the set
> device driver data is visible to `ttyport_receive_buf` before
> `SERPORT_PAUSE_RX` is unset in `ttyport_resume_rx`.

This feels rough.  In talking with others today, serdev really should be
reworked to be a "real" bus here, which should solve these issues,
right?  Perhaps that's the better idea overall instead of these fragile
links?  That might also solve the other issues with serdev where people
want to use it for dynamic devices (i.e. USB devices).

Thoughts?

> > >  	dev_WARN_ONCE(&ctrl->dev, ret > count,
> > > @@ -156,6 +166,32 @@ static void ttyport_close(struct serdev_controller *ctrl)
> > >  	tty_release_struct(tty, serport->tty_idx);
> > >  }
> > >  
> > > +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> > > +{
> > > +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > > +	struct tty_struct *tty = serport->tty;
> > > +
> > > +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> > > +
> > > +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> > 
> > What keeps this bit from being set right after you test it?
> The statement "The caller must guarantee that this does not run
> concurrently with `serdev_device_open` or `serdev_device_close`." in
> the kdoc of `serdev_device_pause_rx` does.

Oh that's going to be impossible to keep working :)

thanks,

greg k-h

Re: [PATCH v2 1/3] tty: serdev: Export functions to pause receive_buf callback calls

[Markus Probst]

[-- Attachment #1: Type: text/plain, Size: 8238 bytes --]

On Wed, 2026-09-23 at 16:26 +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 23, 2026 at 01:31:49PM +0000, Markus Probst wrote:
> > On Wed, 2026-09-23 at 12:35 +0200, Greg Kroah-Hartman wrote:
> > > On Sun, Sep 20, 2026 at 02:29:58PM +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           | 50 ++++++++++++++++++++++++++++++++++++-
> > > >  drivers/tty/serdev/serdev-ttyport.c | 38 ++++++++++++++++++++++++++++
> > > >  include/linux/serdev.h              |  6 +++++
> > > >  3 files changed, 93 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > > > index 7500efcdfc21..7d24f16710cb 100644
> > > > --- a/drivers/tty/serdev/core.c
> > > > +++ b/drivers/tty/serdev/core.c
> > > > @@ -187,6 +187,51 @@ 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.
> > > > + *
> > > > + * The caller must guarantee that this does not run concurrently with
> > > > + * `serdev_device_open` or `serdev_device_close`.
> > > > + *
> > > > + * 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.
> > > > + *
> > > > + * The caller must guarantee that this does not run concurrently with
> > > > + * `serdev_device_open` or `serdev_device_close`.
> > > > + *
> > > > + * 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);
> > > > @@ -398,6 +443,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 |
> > > > @@ -405,7 +451,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 bab1b143b8a6..e8aa89e733bd 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;
> > > > @@ -32,6 +34,14 @@ 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;
> > > > +
> > > > +	/*
> > > > +	 * Ensure writes by the driver are visible before allowing traffic to resume.
> > > > +	 */
> > > > +	smp_mb__after_atomic();
> > > 
> > > This scares me.  Why not use a real lock? 
> > > 
> > I can use locks to make it less "fragile". But I don't I think I need
> > them.
> 
> Always use them first, and then remove and do "tricky" things if you
> really can measure the need and can prove that they are not needed.
Ok.

> 
> > > WHat's the issue here, you
> > > need this to be "flushed" before this call:
> > > 
> > > > +
> > > >  	ret = serdev_controller_receive_buf(ctrl, cp, count);
> > > 
> > > here?
> > > 
> > > And you just tested a bit, you didn't set a bit, so what are you trying
> > > to ensure is written exactly?
> > This should be an acquire load operation (paired with the release store
> > operation in `ttyport_resume_rx`).
> 
> Where is the load?  This feels wrong.

`test_bit(SERPORT_PAUSE_RX, &serport->flags)`.

As Gary pointed out, there is `test_bit_acquire`, but `test_bit` and a
memory barrier should have the same effect.

> 
> > It ensures that any writes before calling `ttyport_resume_rx` are
> > visible in the `serdev_controller_receive_buf` invocation.
> 
> writes from where?  There wasn't a write before this that I can see in
> the diff, hence my confusion.
Writes by the caller of `serdev_device_resume_rx`.
> 
> > For instance, in the Rust abstraction the following will be called in
> > order in probe (with the following patches):
> > 
> > - serdev_device_pause_rx
> > - serdev_device_open
> > - dev_set_drvdata
> > - serdev_device_resume_rx
> > 
> > This atomic lock effectively ensures in this example that the set
> > device driver data is visible to `ttyport_receive_buf` before
> > `SERPORT_PAUSE_RX` is unset in `ttyport_resume_rx`.
> 
> This feels rough.  In talking with others today, serdev really should be
> reworked to be a "real" bus here, which should solve these issues,
> right?  Perhaps that's the better idea overall instead of these fragile
> links?
> 
So that there wouldn't be a tty layer in between anymore?

I would be interested in how this would be implemented?

If every serial port driver needs to register to both tty and serdev,
it would likely introduce a lot of boilerplate code.

> That might also solve the other issues with serdev where people
> want to use it for dynamic devices (i.e. USB devices).
First time I read serdev having issues with dynamic devices.

Can you share details on this issue?

> 
> Thoughts?
> 
> > > >  	dev_WARN_ONCE(&ctrl->dev, ret > count,
> > > > @@ -156,6 +166,32 @@ static void ttyport_close(struct serdev_controller *ctrl)
> > > >  	tty_release_struct(tty, serport->tty_idx);
> > > >  }
> > > >  
> > > > +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> > > > +{
> > > > +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > > > +	struct tty_struct *tty = serport->tty;
> > > > +
> > > > +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> > > > +
> > > > +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> > > 
> > > What keeps this bit from being set right after you test it?
> > The statement "The caller must guarantee that this does not run
> > concurrently with `serdev_device_open` or `serdev_device_close`." in
> > the kdoc of `serdev_device_pause_rx` does.
> 
> Oh that's going to be impossible to keep working :)
I will add a mutex lock in the next revision, which removes this
requirement.

It should also allow to call `serdev_device_close` with the device
already closed without causing a use-after-free.

Thanks
- Markus Probst

> 
> thanks,
> 
> greg k-h

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

[PATCH v2 2/3] rust: serdev: Replace `active` mutex with receive pause

[Markus Probst]

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

[PATCH v2 3/3] rust: serdev: Simplify callbacks

[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