On 2020/08/12 22:20, Oliver Neukum wrote:
> CDC-WDM was not written with multithreaded users or
> uncooperative devices in mind.
> This leads to race conditions and hangs in flush().
>
In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", since multiple users can
share "desc", wouldn't someone's usb_submit_urb() from wdm_write() gets unexpectedly
interfered by someone else's usb_kill_urb(desc->command) from wdm_open() ?
In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", don't we need to similarly
apply timeout to wait_event_interruptible() in wdm_write(), for the same problem will
happen if hardware remained silent forever?
In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
from close(). But I think that returning -EINTR from close() is not recommended because
it can confuse multithreaded application (retrying close() upon -EINTR is not safe).
In patch "[RFC 5/5] CDC-WDM: remove use of intf->dev after potential disconnect",
/* cannot dereference desc->intf if WDM_DISCONNECTING */
if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
can be also removed, for this check is meant for not to dereference desc->intf
after disconnect ?
Guessing from symmetry, do we need to check WDM_DISCONNECTING or WDM_RESETTING
in wait_event_interruptible_timeout() from wdm_flush() when wait_event_interruptible()
in wdm_write() is not checking WDM_DISCONNECTING nor WDM_RESETTING ?
Does it make sense to wait for response of someone else's usb_submit_urb() when
someone is calling close(), for there is no guarantee that failure notice received
via wdm_flush() via some file descriptor corresponds to usb_submit_urb() request from
wdm_write() call from that file descriptor?
