Re: [PATCH v3 2/4] tty: serdev: Export functions to pause receive_buf callback calls

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

[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]

On Wed, 2026-09-30 at 17:32 +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           | 44 ++++++++++++++++++++++++++++++++++++-
>  drivers/tty/serdev/serdev-ttyport.c | 33 ++++++++++++++++++++++++++++
>  include/linux/serdev.h              |  6 +++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 77e8e1d4d2a6..af1470e33620 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -191,6 +191,45 @@ 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.
> + *
> + * 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.
> + *
> + * 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);
> @@ -402,6 +441,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 |
> @@ -409,7 +449,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 c11908f5e1ce..2bed0e54202a 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;
> @@ -35,6 +37,9 @@ 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;
> +
>  	ret = serdev_controller_receive_buf(ctrl, cp, count);
>  
>  	dev_WARN_ONCE(&ctrl->dev, ret > count,
> @@ -177,6 +182,32 @@ static bool ttyport_close(struct serdev_controller *ctrl)
>  	return true;
>  }
>  
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	guard(mutex)(&serport->lock);
> +
> +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_buffer_flush_work(tty->port);
I just realised this could result in a deadlock.

Since the mutex already prevents calling of
`serdev_controller_receive_buf`, it is safe to completely remove this
section (if statement + `tty_buffer_flush_work`).

Thanks
- Markus Probst

> +}
> +
> +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	guard(mutex)(&serport->lock);
> +
> +	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_buffer_restart_work(tty->port);
> +}
> +
>  static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
>  {
>  	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> @@ -281,6 +312,8 @@ static const struct serdev_controller_ops ctrl_ops = {
>  	.get_tiocm = ttyport_get_tiocm,
>  	.set_tiocm = ttyport_set_tiocm,
>  	.break_ctl = ttyport_break_ctl,
> +	.pause_rx = ttyport_pause_rx,
> +	.resume_rx = ttyport_resume_rx,
>  };
>  
>  struct device *serdev_tty_port_register(struct tty_port *port,
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 0f4e81c0950d..02773554e759 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -89,6 +89,8 @@ struct serdev_controller_ops {
>  	int (*get_tiocm)(struct serdev_controller *);
>  	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
>  	int (*break_ctl)(struct serdev_controller *ctrl, unsigned int break_state);
> +	void (*pause_rx)(struct serdev_controller *ctrl);
> +	void (*resume_rx)(struct serdev_controller *ctrl);
>  };
>  
>  /**
> @@ -194,6 +196,8 @@ static inline size_t serdev_controller_receive_buf(struct serdev_controller *ctr
>  int serdev_device_open(struct serdev_device *);
>  void serdev_device_close(struct serdev_device *);
>  int devm_serdev_device_open(struct device *, struct serdev_device *);
> +void serdev_device_pause_rx(struct serdev_device *serdev);
> +void serdev_device_resume_rx(struct serdev_device *serdev);
>  unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
>  void serdev_device_set_flow_control(struct serdev_device *, bool);
>  int serdev_device_write_buf(struct serdev_device *, const u8 *, size_t);
> @@ -233,6 +237,8 @@ static inline int serdev_device_open(struct serdev_device *sdev)
>  	return -ENODEV;
>  }
>  static inline void serdev_device_close(struct serdev_device *sdev) {}
> +static inline void serdev_device_pause_rx(struct serdev_device *serdev) {}
> +static inline void serdev_device_resume_rx(struct serdev_device *serdev) {}
>  static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev, unsigned int baudrate)
>  {
>  	return 0;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]