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