On 31.03.23 15:37, Dae R. Jeong wrote:
On Fri, Mar 31, 2023 at 9:23 PM Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
Hi Dae,
(...)
/* wait for complete transmission of current pdu */
while (wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE) ==
0 &&
cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE);
I'm not sure, but your intention in this condition is probably
while (wait_event_interruptible() != 0 || cmpxchg() != ISOTP_IDLE)?
So, release() can get out of the loop only if
wait_event_interruptible() returns 0 and cmpxchg() successes?
No it is the other way around.
When wait_event_interruptible() returns 0 the state has been properly
set to ISOTP_IDLE (the good case).
And THEN we try to grab the tx.state to be ISOTP_SHUTDOWN.
The while() statement is left when either the tx.state is ISOTP_SHUTDOWN
(after it was ISOTP_IDLE) 'OR' when a signal occurred which terminated
wait_event_interruptible().
In the latter case we do not know the value from tx.state.
Therefore I set the tx.state to ISOTP_IDLE/ISOTP_SHUTDOWN in the V4
patch after checking each wait_event_interruptible() return value:
https://lore.kernel.org/linux-can/20230331131935.21465-1-socketcan@xxxxxxxxxxxx/
I think this is the best way to handle the signal interrupt case for
wait_event_interruptible() ?!?
Best regards,
Oliver
/* force state machines to be idle also when a signal occurred */
so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE;
When wait_event_interruptible() does not return '0' we can neither rely
on tx.state to be ISOTP_IDLE nor on anybody else taking care for that.
So I would suggest to continue removing the socket.
Thank you for your hard work!
Thanks for the review and the request to take an additional look at the
code!
Best regards,
Oliver
Best regards,
Dae R. Jeong
