[RFC PATCH] usb_wwan : add locking around shared port data in two FIXME-marked places

[RFC PATCH] usb_wwan : add locking around shared port data in two FIXME-marked places

[Abinash Singh]

Fix two locking-related FIXME comments by adding a mutex
 to protect shared fields in `usb_wwan_port_private`.

- In `usb_wwan_dtr_rts()`, access to `rts_state`
and `dtr_state` is now protected by `portdata->lock`.
- In `usb_wwan_tiocmset()`, access to `rts_state`
 and `dtr_state` is now also synchronized with the same mutex.

These changes prevent possible data races
and inconsistent state updates when the port is written concurrently.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>

---
usb_wwan_chars_in_buffer() have this:
/* FIXME: This locking is insufficient as this_urb may
		   go unused during the test */
How can we do proper locking there ?
Do we need to lock portdata in other places also ? I see no other FIXME related to locking

Thank You

Regards

Abinash
---
 drivers/usb/serial/usb-wwan.h | 3 +++
 drivers/usb/serial/usb_wwan.c | 7 ++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
index 519101945..3cc0d4ef4 100644
--- a/drivers/usb/serial/usb-wwan.h
+++ b/drivers/usb/serial/usb-wwan.h
@@ -59,6 +59,9 @@ struct usb_wwan_port_private {
 	int ri_state;
 
 	unsigned long tx_start_time[N_OUT_URB];
+
+	/* Locking */
+	struct mutex lock;
 };
 
 #endif /* __LINUX_USB_USB_WWAN */
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 0017f6e96..4bd81b21f 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -80,10 +80,10 @@ 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;
-
+	mutex_unlock(&portdata->lock);
 	usb_wwan_send_setup(port);
 }
 EXPORT_SYMBOL(usb_wwan_dtr_rts);
@@ -120,7 +120,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,6 +130,7 @@ int usb_wwan_tiocmset(struct tty_struct *tty,
 		portdata->rts_state = 0;
 	if (clear & TIOCM_DTR)
 		portdata->dtr_state = 0;
+	mutex_unlock(&portdata->lock);
 	return usb_wwan_send_setup(port);
 }
 EXPORT_SYMBOL(usb_wwan_tiocmset);
-- 
2.43.0

Re: [RFC PATCH] usb_wwan : add locking around shared port data in two FIXME-marked places

[Oliver Neukum]

Hi,

On 20.06.25 12:17, Abinash Singh wrote:
> Fix two locking-related FIXME comments by adding a mutex
>   to protect shared fields in `usb_wwan_port_private`.
> 
> - In `usb_wwan_dtr_rts()`, access to `rts_state`
> and `dtr_state` is now protected by `portdata->lock`.
> - In `usb_wwan_tiocmset()`, access to `rts_state`
>   and `dtr_state` is now also synchronized with the same mutex.
> 
> These changes prevent possible data races
> and inconsistent state updates when the port is written concurrently.

unfortunately this patch is rather problematic because

1. you never initialize the mutex
2. these values are read in usb_wwan_send_setup(), where you don't take the lock

Now, as usb_wwan_send_setup() is called right after you drop
the mutex, this patch is kind of inelegant.

	Sorry
		Oliver

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

[Abinash Singh]

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.

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()

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;
 };
 
 #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);
+	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);
+	mutex_unlock(&portdata->lock);
+	return ret;
 }
 EXPORT_SYMBOL(usb_wwan_tiocmset);
 
@@ -452,7 +456,7 @@ int usb_wwan_port_probe(struct usb_serial_port *port)
 	portdata = kzalloc(sizeof(*portdata), GFP_KERNEL);
 	if (!portdata)
 		return -ENOMEM;
-
+	mutex_init(&portdata->lock);
 	init_usb_anchor(&portdata->delayed);
 
 	for (i = 0; i < N_IN_URB; i++) {
-- 
2.43.0

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

[Greg KH]

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

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

[Abinash Singh]

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);
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
-
-	usb_wwan_send_setup(port);
+	}
+	usb_wwan_send_setup(port,on,on);
 }
 EXPORT_SYMBOL(usb_wwan_dtr_rts);
 
@@ -113,14 +111,15 @@ 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 rts, dtr;
 
 	portdata = usb_get_serial_port_data(port);
 	intfdata = usb_get_serial_data(port->serial);
 
 	if (!intfdata->use_send_setup)
 		return -EINVAL;
-
-	/* FIXME: what locks portdata fields ? */
+	{
+	guard(mutex)(&portdata->lock);
 	if (set & TIOCM_RTS)
 		portdata->rts_state = 1;
 	if (set & TIOCM_DTR)
@@ -130,7 +129,11 @@ 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);
+
+	rts = portdata->rts_state;
+	dtr = portdata->dtr_state;
+	}
+	return usb_wwan_send_setup(port, rts, dtr);
 }
 EXPORT_SYMBOL(usb_wwan_tiocmset);
 
@@ -452,7 +455,7 @@ int usb_wwan_port_probe(struct usb_serial_port *port)
 	portdata = kzalloc(sizeof(*portdata), GFP_KERNEL);
 	if (!portdata)
 		return -ENOMEM;
-
+	mutex_init(&portdata->lock);
 	init_usb_anchor(&portdata->delayed);
 
 	for (i = 0; i < N_IN_URB; i++) {
-- 
2.43.0

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

[Greg KH]

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