Re: [PATCH] USB: cdc-wdm: Call wake_up_all() when clearing WDM_IN_USE bit.

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2020/07/02 16:24, Oliver Neukum wrote:
> Am Donnerstag, den 02.07.2020, 14:44 +0900 schrieb Tetsuo Handa:
> 
>>  
>>  	usb_autopm_put_interface(desc->intf)
>>  	mutex_unlock(&desc->wlock);
>> +	if (rv >= 0 &&
>> +	    /*
>> +	     * needs both flags. We cannot do with one
>> +	     * because resetting it would cause a race
>> +	     * with write() yet we need to signal
>> +	     * a disconnect
>> +	     */
>> +	    wait_event_killable_timeout(desc->wait,
>> +					!test_bit(WDM_IN_USE, &desc->flags) ||
>> +					test_bit(WDM_DISCONNECTING, &desc->flags), 30 * HZ) == 0) {
>> +		if (mutex_lock_killable(&desc->wlock) == 0) {
>> +			if (!test_bit(WDM_DISCONNECTING, &desc->flags))
>> +				dev_err(&desc->intf->dev,
>> +					"Tx URB not responding index=%d\n",
>> +					le16_to_cpu(req->wIndex));
>> +			mutex_unlock(&desc->wlock);
>> +		}
>> +	}
> 
> Hi,
> 
> I am afraid this would
> 
> 1. serialize the driver, harming performance

Is wdm_write() called so frequently (e.g. 1000 times per one second) ?

> 2. introduce a race with every timer a task is running

What is estimated response time from usb_submit_urb() to wdm_out_callback() ?
Can it be many seconds?

I didn't try your patches at https://lkml.kernel.org/r/1593078968.28236.15.camel@suse.com
because it seems to me that your patch does not answer my 3 concerns:

(1) wdm_flush() says
    
            /* cannot dereference desc->intf if WDM_DISCONNECTING */
            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);

    but it seems to me that nothing guarantees that test_bit(WDM_DISCONNECTING) == false
    indicates dereferencing desc->intf->dev is safe, for wdm_flush() tests WDM_DISCONNECTING
    without any lock whereas wdm_disconnect() sets WDM_DISCONNECTING under wdm_mutex and
    desc->iuspin held. It might be safe to dereference from wdm_release() which holds wdm_mutex.

(2) If wait_event() in wdm_flush() might fail to wake up (due to close() dependency
    problem this crash report is focusing on), wait_event_interruptible() in wdm_write() might
    also fail to wake up (unless interrupted) due to the same dependency. Then, why can't we
    wait for completion of wdm_out_callback() (with reasonable timeout) inside wdm_write() ?

(3) While wdm_flush() waits for clearing of WDM_IN_USE using wait_event(), concurrently
    executed wdm_write() also waits for clearing of WDM_IN_USE using wait_event_interruptible(),
    and wdm_write() can immediately set WDM_IN_USE again as soon as returning from
    wait_event_interruptible() even if somebody was already waiting at wdm_flush() to clear
    WDM_IN_USE.

    That is, wait_event() in wdm_flush() does not know whether there is usb_submit_urb()
    request which is started after wait_event() found that WDM_IN_USE was cleared. Then,
    why does this wait_event() in wdm_flush() want to flush which current thread might not
    have issued?

Current thread synchronously waits for completion of wdm_out_callback() issued by current
thread's usb_submit_urb() request might make sense. But how much value is there for current
thread waits for completion of wdm_out_callback() issued by other thread's usb_submit_urb()
request? Multiple threads can use the same "desc" pointer, and trying to flush upon close()
by each thread using that "desc" pointer...

If synchronous waiting harms performance so much, do we want to know the error at all?
wdm_write() already returned success (the number of bytes passed to write()). And there is
no guarantee that the error code which current thread will receive from wmd_flush()
corresponds with a request current thread has issued?

I'm skeptical about the value of trying to synchronously return an error code for
wmd_write() request to the caller. I'm really inclined to remove wdm_flush() completely.