Re: WARNING in wdm_write/usb_submit_urb

["Bjørn Mork" <bjorn@mork.no>]

Oliver Neukum <oneukum@suse.com> writes:

> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 1656f5155ab8..a341081a5f47 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -588,14 +588,24 @@ static int wdm_flush(struct file *file, fl_owner_t id)
>  {
>  	struct wdm_device *desc = file->private_data;
>  
> -	wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
> +	wait_event(desc->wait,
> +			/*
> +			 * needs both flags. We cannot do with one
> +			 * because resetting it would cause a race
> +			 * with write() yet we need to signal
> +			 * a disconnect
> +			 */
> +			!test_bit(WDM_IN_USE, &desc->flags) &&
> +			!test_bit(WDM_DISCONNECTING, &desc->flags));


Makes sense.  But isn't the WDM_DISCONNECTING test inverted?

>  	/* cannot dereference desc->intf if WDM_DISCONNECTING */
>  	if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
>  		dev_err(&desc->intf->dev, "Error in flush path: %d\n",
>  			desc->werr);
>  
> -	return usb_translate_errors(desc->werr);
> +	return test_bit(WDM_DISCONNECTING, &desc->flags) ? 
> +			-ENODEV : 
> +			usb_translate_errors(desc->werr);
>  }

Minor detail, but there's an awful lot of test_bit(WDM_DISCONNECTING)
here.  How about

  if (test_bit(WDM_DISCONNECTING, &desc->flags))
    return -ENODEV;
  if (desc->werr < 0)
    dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr);
  return usb_translate_errors(desc->werr);



>  static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
> @@ -975,8 +985,6 @@ static void wdm_disconnect(struct usb_interface *intf)
>  	spin_lock_irqsave(&desc->iuspin, flags);
>  	set_bit(WDM_DISCONNECTING, &desc->flags);
>  	set_bit(WDM_READ, &desc->flags);
> -	/* to terminate pending flushes */
> -	clear_bit(WDM_IN_USE, &desc->flags);
>  	spin_unlock_irqrestore(&desc->iuspin, flags);
>  	wake_up_all(&desc->wait);
>  	mutex_lock(&desc->rlock);


Yes, this looks much better. 


Bjørn