Hi,
On 16.09.24 14:44, Jeongjun Park wrote:
Oliver Neukum <oneukum@xxxxxxxx> wrote:
On 16.09.24 06:15, Greg KH wrote:
On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:
Please use the guard() form here, it makes the change much simpler and
easier to review and maintain.
That would break the O_NONBLOCK case.
Looking at the code it indeed looks like iowarrior_read() can race
with itself. Strictly speaking it always could happen if a task used
fork() after open(). The driver tries to restrict its usage to one
thread, but I doubt that the logic is functional.
It seems to me the correct fix is something like this:
Well, I don't know why it's necessary to modify it like this.
I think it would be more appropriate to patch it to make it
more maintainable by using guard() as Greg suggested.
Allow me to explain detail.
guard() internally uses mutex_lock(). That means that
a) it will block
b) having blocked it will sleep in the state TASK_UNINTERRUPTIBLE
The driver itself uses TASK_INTERRUPTIBLE in iowarrior_read(),
when it waits for IO. That is entirely correct, as it waits for
an external device doing an operation that may never occur. You
must use TASK_INTERRUPTIBLE.
Now, if you use mutex_lock() to wait for a task waiting for IO
to occur in the state TASK_INTERRUPTIBLE, you are indirectlywaiting for
an event that you must wait for in TASK_INTERRUPTIBLE in the state
TASK_UNINTERRUPTIBLE.
That is a bug. You have created a task that cannot be killed (uid may not match),
but may have to be killed. Furthermore you block even in case the
device has been opened with O_NONBLOCK, which is a second bug.
These limitations are inherent in guard(). Therefore you cannot use
guard here.
Regards
Oliver
