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

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jul 02, 2025 at 03:15:32AM +0530, Abinash Singh wrote:
> The rts_state and dtr_state fields in usb_wwan were updated without
> any locking, which could lead to data races if accessed from multiple
> threads.
> 
> Fixes proper locking using guard(mutex) to ensure
> safe access to these shared fields. To avoid holding the lock during
> USB control message transmission, the values are passed explicitly
> to usb_wwan_send_setup().
> 
> This resolves two previously marked FIXME comments and improves
> the thread safety of modem control line handling.
> 
> Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
> ---
> v2:
> - I missed the "v2" tag in the subject line earlier — added now, sorry about that.
> - Regarding the concern about locking while calling functions: I was unsure if
>   it’s safe to hold the lock across `usb_wwan_send_setup()`, since it may block.
>   To be safe, I’ve changed the function to take `rts_state` and `dtr_state` as
>   arguments, so it no longer accesses shared state directly.
> - I’ve now used `guard(mutex)` so the lock will automatically release when
>   `portdata` goes out of scope.
> 
>    Is this the correct way to use gaurd if we don't want the lock held during
>    usb_wwan_send_setup() ?
> 
> > How was this tested?
> 
> I haven’t been able to test this patch due to lack of hardware access. If you
> have any suggestions on how to test this kind of change without actual hardware,
> I’d appreciate your guidance.
> 
> Thanks for the feedback!
> ---
>  drivers/usb/serial/usb-wwan.h |  1 +
>  drivers/usb/serial/usb_wwan.c | 29 ++++++++++++++++-------------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
> index 519101945769..5a990fc2e140 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;  /* protects rts_state and dtr_state */
>  };
>  
>  #endif /* __LINUX_USB_USB_WWAN */
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index 0017f6e969e1..042d63aa8ec6 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -38,19 +38,16 @@
>   * Generate DTR/RTS signals on the port using the SET_CONTROL_LINE_STATE request
>   * in CDC ACM.
>   */
> -static int usb_wwan_send_setup(struct usb_serial_port *port)
> +static int usb_wwan_send_setup(struct usb_serial_port *port, int rts_state, int dtr_state)
>  {
>  	struct usb_serial *serial = port->serial;
> -	struct usb_wwan_port_private *portdata;
>  	int val = 0;
>  	int ifnum;
>  	int res;
>  
> -	portdata = usb_get_serial_port_data(port);
> -
> -	if (portdata->dtr_state)
> +	if (dtr_state)
>  		val |= USB_CDC_CTRL_DTR;
> -	if (portdata->rts_state)
> +	if (rts_state)
>  		val |= USB_CDC_CTRL_RTS;
>  
>  	ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
> @@ -80,11 +77,12 @@ void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
>  		return;
>  
>  	portdata = usb_get_serial_port_data(port);
> -	/* FIXME: locking */
> +	{
> +	guard(mutex)(&portdata->lock);

This use of a { } without indenting the code is not good, and should
have given you a coding style error, right?

Also, if you don't have the hardware to test this with, I wouldn't
recommend working on adding locking without being able to verify it
works in some way.

thanks,

greg k-h