From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Lukas Magel <lukas.magel@posteo.net>,
Michal Sojka <michal.sojka@cvut.cz>,
Maxime Jayat <maxime.jayat@mobile-devices.fr>,
Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
"Dae R. Jeong" <threeearcat@gmail.com>,
Hillf Danton <hdanton@sina.com>
Subject: Re: can: isotp: epoll breaks isotp_sendmsg
Date: Tue, 22 Aug 2023 18:37:14 +0200 [thread overview]
Message-ID: <04fd32bc-b1c4-b9c3-3f8b-7987704a1f85@hartkopp.net> (raw)
In-Reply-To: <f4221e5e-8fee-4ed6-af54-46b8ac0e5c03@posteo.net>
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
prev parent reply other threads:[~2023-08-22 16:37 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-30 16:44 can: isotp: epoll breaks isotp_sendmsg Maxime Jayat
2023-06-30 22:35 ` Michal Sojka
2023-07-17 7:49 ` Marc Kleine-Budde
2023-07-17 13:05 ` Michal Sojka
2023-08-13 11:23 ` Lukas Magel
2023-08-18 11:53 ` Lukas Magel
2023-08-20 19:51 ` Oliver Hartkopp
2023-08-22 6:51 ` Lukas Magel
2023-08-22 16:37 ` Oliver Hartkopp [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=04fd32bc-b1c4-b9c3-3f8b-7987704a1f85@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=hdanton@sina.com \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lukas.magel@posteo.net \
--cc=maxime.jayat@mobile-devices.fr \
--cc=michal.sojka@cvut.cz \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=threeearcat@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).