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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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