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@xxxxxxxx
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.
