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

[Markus Probst]

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@xxxxxxxxx>
> > > > ---
> > > >  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: signature.asc
Description: This is a digitally signed message part