From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org
Cc: "Sottas Guillaume (LMB)" <Guillaume.Sottas@liebherr.com>
Subject: Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path
Date: Wed, 28 Apr 2021 12:24:32 +0200 [thread overview]
Message-ID: <f81170bc-b6d2-37c0-a6b0-86fb9570407c@hartkopp.net> (raw)
In-Reply-To: <f18cb9ce-4bab-80b2-ccc7-37fbafe04995@hartkopp.net>
On 28.04.21 11:26, Oliver Hartkopp wrote:
> On 28.04.21 11:12, Marc Kleine-Budde wrote:
>> On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:
>>> @@ -756,6 +762,10 @@ static enum hrtimer_restart
>>> isotp_tx_timer_handler(struct hrtimer *hrtimer)
>>
>>> sk->sk_err = ECOMM;
>>> if (!sock_flag(sk, SOCK_DEAD))
>>> sk->sk_error_report(sk);
>>
>> I think the idea is that sk_error_report takes care of propagation of
>> the error to the user space. I don't know why it's not working as
>> expected.
>
> Yes, I fact I was thinking about this question too.
>
>>
>>> + /* set error flag in order to consume it later in the
>>> + * isotp_sendmsg function */
>>> + so->tx.error = ISOTP_ERR_FC_TIMEOUT;
>>> +
>>> /* reset tx state */
>>> so->tx.state = ISOTP_IDLE;
>>> wake_up_interruptible(&so->wait);
>>> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock,
>>> struct msghdr *msg, size_t size)
>>> if (wait_tx_done) {
>>> /* wait for complete transmission of current pdu */
>>> wait_event_interruptible(so->wait, so->tx.state ==
>>> ISOTP_IDLE);
>>> +
>>> + /* check if an error has been raised in the timer
>>> + * function handler */
>>> + if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
>>> + so->tx.error = ISOTP_ERR_NO_ERROR;
>>> + return -ECOMM;
>
> Maybe the way to trigger the sk_error_report(sk) we might return '-1'
> while the error is then propagated inside errno.
I added some debug print in isotpsend:
diff --git a/isotpsend.c b/isotpsend.c
index 3ea574c..c2937fa 100644
--- a/isotpsend.c
+++ b/isotpsend.c
@@ -45,10 +45,11 @@
#include <libgen.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <errno.h>
#include <net/if.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/types.h>
@@ -252,10 +253,11 @@ int main(int argc, char **argv)
buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
}
retval = write(s, buf, buflen);
+ printf("retval %d errno %d\n", retval, errno);
if (retval < 0) {
perror("write");
return retval;
}
$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
43.269173590
retval 44 errno 0
44.271162277
So it waits for the timeout as required by the '-b' option - but the
errno is not set :-/
>
> But I haven't checked that myself so far.
>
> The reason why this return value is not that important is that ISO-TP is
> an (unreliable!) UDP like protocol where the application checks for an
> application response timeout.
>
> Having the information of a FF timeout only makes sense when
> CAN_ISOTP_WAIT_TX_DONE is enabled to assign the timeout to a that
> specific PDU.
>
> Best,
> Oliver
>
>>> + }
>>> }
>>> return size;
>>> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>>> so->tx.state = ISOTP_IDLE;
>>> hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC,
>>> HRTIMER_MODE_REL_SOFT);
>>> +
>>> + so->tx.error = ISOTP_ERR_NO_ERROR;
>>> + so->rx.error = ISOTP_ERR_NO_ERROR;
>>> so->rxtimer.function = isotp_rx_timer_handler;
>>> hrtimer_init(&so->txtimer, CLOCK_MONOTONIC,
>>> HRTIMER_MODE_REL_SOFT);
>>> so->txtimer.function = isotp_tx_timer_handler;
>>
>>
>> Marc
>>
next prev parent reply other threads:[~2021-04-28 10:24 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-28 9:09 [PATCH] can: isotp: return -ECOMM on FC timeout on TX path Marc Kleine-Budde
2021-04-28 9:12 ` Marc Kleine-Budde
2021-04-28 9:26 ` Oliver Hartkopp
2021-04-28 10:24 ` Oliver Hartkopp [this message]
2021-04-28 10:47 ` Marc Kleine-Budde
2021-04-28 11:45 ` Oliver Hartkopp
2021-04-28 12:07 ` Marc Kleine-Budde
2021-04-28 11:43 Sottas Guillaume (LMB)
2021-04-28 11:48 ` Oliver Hartkopp
2021-05-06 12:45 Sottas Guillaume (LMB)
2021-05-06 13:38 ` Marc Kleine-Budde
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=f81170bc-b6d2-37c0-a6b0-86fb9570407c@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=Guillaume.Sottas@liebherr.com \
--cc=linux-can@vger.kernel.org \
--cc=mkl@pengutronix.de \
/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 NNTP newsgroup(s).