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

[Markus Probst <markus.probst@posteo.de>]

[-- 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 --]