Messages in this thread | | | Date | Fri, 31 Mar 2023 12:25:42 +0200 | Subject | Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted | From | Oliver Hartkopp <> |
| |
Hey all,
looking at the code simplification from Hillf I reworked some of the cmpxchg() code and removed the old_state mechanism.
I posted a RFC patch here:
https://lore.kernel.org/linux-can/20230331102114.15164-1-socketcan@hartkopp.net/
Please comment on the patch whether you think if this could be an improvement.
Many thanks, Oliver
On 27.03.23 03:48, Hillf Danton wrote: > On Sun, 26 Mar 2023 18:17:17 +0200 Oliver Hartkopp <socketcan@hartkopp.net> >> On 26.03.23 13:55, Dae R. Jeong wrote: >>> >>> If I think correctly, this will make cmpxchg() work, and prevent the >>> problematic concurrent execution. Could you please check the patch >>> below? >> >> Hm, interesting idea. >> >> But in which state will so->tx.state be here: >> >> /* wait for complete transmission of current pdu */ >> err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); >> if (err) >> goto err_out; >> >> >> Should we better set the tx.state in the error case? >> >> if (err) { >> so->tx.state = ISOTP_IDLE; >> goto err_out; >> } >> >> Best regards, >> Oliver > > Another 2c only if cmpxchg is preferred. > > +++ b/net/can/isotp.c > @@ -932,19 +932,24 @@ static int isotp_sendmsg(struct socket * > return -EADDRNOTAVAIL; > > /* we do not support multiple buffers - for now */ > - if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE || > - wq_has_sleeper(&so->wait)) { > - if (msg->msg_flags & MSG_DONTWAIT) { > - err = -EAGAIN; > - goto err_out; > - } > - > + if (wq_has_sleeper(&so->wait)) { > + if (msg->msg_flags & MSG_DONTWAIT) > + return -EAGAIN; > /* wait for complete transmission of current pdu */ > err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); > if (err) > - goto err_out; > + return err; > + } > > - so->tx.state = ISOTP_SENDING; > +again: > + old_state = cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING); > + if (old_state != ISOTP_IDLE) { > + if (msg->msg_flags & MSG_DONTWAIT) > + return -EAGAIN; > + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); > + if (err) > + return err; > + goto again; > } > > if (!size || size > MAX_MSG_LENGTH) {
| |