| Message ID | 20250331132614.51902-3-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: wdm: fix WWAN integration issue | expand |
On Mon, Mar 31, 2025 at 03:25:02PM +0200, Oliver Neukum wrote: > Clearing WDM_WWAN_IN_USE must be the last action or > we can open a chardev whose URBs are still poisoned > > Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration") > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/usb/class/cdc-wdm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c > index 12038aa43942..e67844618da6 100644 > --- a/drivers/usb/class/cdc-wdm.c > +++ b/drivers/usb/class/cdc-wdm.c > @@ -870,8 +870,9 @@ static void wdm_wwan_port_stop(struct wwan_port *port) > poison_urbs(desc); > desc->manage_power(desc->intf, 0); > clear_bit(WDM_READ, &desc->flags); > - clear_bit(WDM_WWAN_IN_USE, &desc->flags); > unpoison_urbs(desc); > + /* this must be last lest we open a poisoned device */ > + clear_bit(WDM_WWAN_IN_USE, &desc->flags); > } This is a good example of a place where a memory barrier is needed. Neither unpoison_urbs() nor clear_bit() includes an explicit memory barrier. So even though patch ensures that unpoison_urb() occurs before clear_bit() in the source code, there is still no guarantee that a CPU will execute them in that order. Or even if they are executed in order, there is no guarantee that a different CPU will see their effects occurring in that order. In this case you almost certainly need to have an smp_wmb() between the two statements, with a corresponding smp_rmb() (or equivalent) in the code that checks whether the WDM_WWAIN_IN_USE flag is set. Alan Stern
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 12038aa43942..e67844618da6 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -870,8 +870,9 @@ static void wdm_wwan_port_stop(struct wwan_port *port) poison_urbs(desc); desc->manage_power(desc->intf, 0); clear_bit(WDM_READ, &desc->flags); - clear_bit(WDM_WWAN_IN_USE, &desc->flags); unpoison_urbs(desc); + /* this must be last lest we open a poisoned device */ + clear_bit(WDM_WWAN_IN_USE, &desc->flags); } static void wdm_wwan_port_tx_complete(struct urb *urb)
Clearing WDM_WWAN_IN_USE must be the last action or we can open a chardev whose URBs are still poisoned Fixes: cac6fb015f71 ("usb: class: cdc-wdm: WWAN framework integration") Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/usb/class/cdc-wdm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)