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

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

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