Re: [PATCH] usb: serial: usb_wwan: Fix data races by protecting dtr/rts state with a mutex

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jun 26, 2025 at 09:01:56PM +0530, Abinash Singh wrote:
> Fix two previously noted locking-related issues in usb_wwan by introducing
> a mutex to serialize access to the shared `rts_state` and `dtr_state`
> fields in `struct usb_wwan_port_private`.
> 
> - In `usb_wwan_dtr_rts()`, the fields are now updated under the new
>   `portdata->lock` to prevent concurrent access.
> - In `usb_wwan_tiocmset()`, the same lock is used to protect both updates
>   to the modem control lines and the subsequent `usb_wwan_send_setup()`
>   call.
> 
> The mutex is initialized during `usb_wwan_port_probe()` when the port
> private data is allocated. This ensures consistent state and avoids
> data races when multiple threads attempt to modify control line state.
> 
> This change resolves the two old `FIXME` comments and improves thread
> safety for modem control signal handling.

How was this tested?

> 
> Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
> ---
> Thank You very much for your feedback .
> You don't have to say sorry , your feedback
> is valueable for me.
> 
> 
> v2 :
> 	initialized the mutex during probing
> 	droping lock after returning from usb_wwan_send_setup()

You didn't list "v2" in the subject line, which makes this hard for our
tools to track (and for you to track as well!)



> 
> Regards
> Abinash
> ---
>  drivers/usb/serial/usb-wwan.h |  1 +
>  drivers/usb/serial/usb_wwan.c | 12 ++++++++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
> index 519101945769..e8d042d9014f 100644
> --- a/drivers/usb/serial/usb-wwan.h
> +++ b/drivers/usb/serial/usb-wwan.h
> @@ -59,6 +59,7 @@ struct usb_wwan_port_private {
>  	int ri_state;
>  
>  	unsigned long tx_start_time[N_OUT_URB];
> +	struct mutex lock;

You might want to document what this lock is for somewhere, right?

>  };
>  
>  #endif /* __LINUX_USB_USB_WWAN */
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index 0017f6e969e1..cd80fbd1dc6f 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -80,11 +80,12 @@ void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
>  		return;
>  
>  	portdata = usb_get_serial_port_data(port);
> -	/* FIXME: locking */
> +	mutex_lock(&portdata->lock);
>  	portdata->rts_state = on;
>  	portdata->dtr_state = on;
>  
>  	usb_wwan_send_setup(port);

You are sure it's ok to call a function while the lock is held?  Is it
now required?  If so, please add the proper static and runtime checking
for that.  If not, then it's going to get messy very quickly :(

> +	mutex_unlock(&portdata->lock);
>  }
>  EXPORT_SYMBOL(usb_wwan_dtr_rts);
>  
> @@ -113,6 +114,7 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
>  	struct usb_serial_port *port = tty->driver_data;
>  	struct usb_wwan_port_private *portdata;
>  	struct usb_wwan_intf_private *intfdata;
> +	int ret;
>  
>  	portdata = usb_get_serial_port_data(port);
>  	intfdata = usb_get_serial_data(port->serial);
> @@ -120,7 +122,7 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
>  	if (!intfdata->use_send_setup)
>  		return -EINVAL;
>  
> -	/* FIXME: what locks portdata fields ? */
> +	mutex_lock(&portdata->lock);
>  	if (set & TIOCM_RTS)
>  		portdata->rts_state = 1;
>  	if (set & TIOCM_DTR)
> @@ -130,7 +132,9 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
>  		portdata->rts_state = 0;
>  	if (clear & TIOCM_DTR)
>  		portdata->dtr_state = 0;
> -	return usb_wwan_send_setup(port);
> +	ret = usb_wwan_send_setup(port);

Again, is this ok to hold a lock across?

> +	mutex_unlock(&portdata->lock);

Why not use the guard() style for all of this to make it simpler
overall?

thanks,

greg k-h