Re: can: isotp: epoll breaks isotp_sendmsg

[Oliver Hartkopp]

Hi Lukas,

On 22.08.23 08:51, Lukas Magel wrote:

> > > @Oliver I adjusted the exit path for the case where the initial wait is
> > > interrupted to return immediately instead of jumping to err_event_drop.
> > > Could you please check if you would agree with this change?
> > The code has really won with your change! Thanks!
> >
> > But as you already assumed I have a problem with the handling of the
> > cleanup when a signal interrupts the wait_event_interruptible() statement.
> >
> > I think it should still be:
> >
> > /* wait for complete transmission of current pdu */
> > err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> > if (err)
> >           goto err_event_drop;
> >
> > as we need to make sure that the state machine is set to defined values
> > and states for the next isotp_sendmsg() attempt.
> >
> > Best regards,
> > Oliver
>
>
> Thank you for the feedback! Can you elaborate why the state needs to be
> reset here? For me, the loop is basically a "let's wait until we win
> arbitration for the tx.state", which means that the task is allowed
> to send. I'm imagining an application that has two threads, both sending
> at the same time (because maybe they don't care about reading). So one
> would always be waiting in the loop until the send operation of the other
> has concluded. My motivation for not going to err_event_drop was that if
> one thread was interrupted in its wait_event_interruptible, why would we
> need to change tx.state that is currently being occupied by the other
> thread? The thread waiting in the loop has not done any state manipulation
> of the socket.

Please don't only look at the isotp_sendmsg() function but the other 
possibilities e.g. from timeouts.

Look for the documentation from the commit 051737439eaee. This patch has 
been added recently as it was needed.

Best regards,
Oliver