lkml.org 
[lkml]   [2023]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: WARNING in isotp_tx_timer_handler and WARNING in print_tainted
From
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) {

\
 
 \ /
  Last update: 2023-03-31 12:28    [W:0.065 / U:1.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site