Skip to content

Commit 14b89c2

Browse files
rrendecrrendec
committed
can: isotp: fix tx state handling for echo tx processing
JIRA: https://issues.redhat.com/browse/RHEL-80832 commit 8663378 Author: Oliver Hartkopp <socketcan@hartkopp.net> Date: Fri Nov 4 15:25:51 2022 +0100 can: isotp: fix tx state handling for echo tx processing In commit 4b7fe92 ("can: isotp: add local echo tx processing for consecutive frames") the data flow for consecutive frames (CF) has been reworked to improve the reliability of long data transfers. This rework did not touch the transmission and the tx state changes of single frame (SF) transfers which likely led to the WARN in the isotp_tx_timer_handler() catching a wrong tx state. This patch makes use of the improved frame processing for SF frames and sets the ISOTP_SENDING state in isotp_sendmsg() within the cmpxchg() condition handling. A review of the state machine and the timer handling additionally revealed a missing echo timeout handling in the case of the burst mode in isotp_rcv_echo() and removes a potential timer configuration uncertainty in isotp_rcv_fc() when the receiver requests consecutive frames. Fixes: 4b7fe92 ("can: isotp: add local echo tx processing for consecutive frames") Link: https://lore.kernel.org/linux-can/CAO4mrfe3dG7cMP1V5FLUkw7s+50c9vichigUMQwsxX4M=45QEw@mail.gmail.com/T/#u Reported-by: Wei Chen <harperchen1110@gmail.com> Cc: stable@vger.kernel.org # v6.0 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> Link: https://lore.kernel.org/all/20221104142551.16924-1-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Radu Rendec <rrendec@redhat.com>
1 parent 47f61de commit 14b89c2

File tree

1 file changed

+38
-33
lines changed

1 file changed

+38
-33
lines changed

‎net/can/isotp.c‎

Lines changed: 38 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,9 @@ MODULE_ALIAS("can-proto-6");
110110
#define ISOTP_FC_WT 1 /* wait */
111111
#define ISOTP_FC_OVFLW 2 /* overflow */
112112

113+
#define ISOTP_FC_TIMEOUT 1 /* 1 sec */
114+
#define ISOTP_ECHO_TIMEOUT 2 /* 2 secs */
115+
113116
enum {
114117
ISOTP_IDLE = 0,
115118
ISOTP_WAIT_FIRST_FC,
@@ -256,7 +259,8 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
256259
so->lastrxcf_tstamp = ktime_set(0, 0);
257260

258261
/* start rx timeout watchdog */
259-
hrtimer_start(&so->rxtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT);
262+
hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
263+
HRTIMER_MODE_REL_SOFT);
260264
return 0;
261265
}
262266

@@ -342,6 +346,8 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,
342346
return 0;
343347
}
344348

349+
static void isotp_send_cframe(struct isotp_sock *so);
350+
345351
static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
346352
{
347353
struct sock *sk = &so->sk;
@@ -396,14 +402,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
396402
case ISOTP_FC_CTS:
397403
so->tx.bs = 0;
398404
so->tx.state = ISOTP_SENDING;
399-
/* start cyclic timer for sending CF frame */
400-
hrtimer_start(&so->txtimer, so->tx_gap,
405+
/* send CF frame and enable echo timeout handling */
406+
hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
401407
HRTIMER_MODE_REL_SOFT);
408+
isotp_send_cframe(so);
402409
break;
403410

404411
case ISOTP_FC_WT:
405412
/* start timer to wait for next FC frame */
406-
hrtimer_start(&so->txtimer, ktime_set(1, 0),
413+
hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
407414
HRTIMER_MODE_REL_SOFT);
408415
break;
409416

@@ -598,7 +605,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
598605
/* perform blocksize handling, if enabled */
599606
if (!so->rxfc.bs || ++so->rx.bs < so->rxfc.bs) {
600607
/* start rx timeout watchdog */
601-
hrtimer_start(&so->rxtimer, ktime_set(1, 0),
608+
hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
602609
HRTIMER_MODE_REL_SOFT);
603610
return 0;
604611
}
@@ -818,7 +825,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
818825
struct isotp_sock *so = isotp_sk(sk);
819826
struct canfd_frame *cf = (struct canfd_frame *)skb->data;
820827

821-
/* only handle my own local echo skb's */
828+
/* only handle my own local echo CF/SF skb's (no FF!) */
822829
if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
823830
return;
824831

@@ -838,13 +845,16 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
838845
if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {
839846
/* stop and wait for FC with timeout */
840847
so->tx.state = ISOTP_WAIT_FC;
841-
hrtimer_start(&so->txtimer, ktime_set(1, 0),
848+
hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
842849
HRTIMER_MODE_REL_SOFT);
843850
return;
844851
}
845852

846853
/* no gap between data frames needed => use burst mode */
847854
if (!so->tx_gap) {
855+
/* enable echo timeout handling */
856+
hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
857+
HRTIMER_MODE_REL_SOFT);
848858
isotp_send_cframe(so);
849859
return;
850860
}
@@ -868,7 +878,7 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
868878
/* start timeout for unlikely lost echo skb */
869879
hrtimer_set_expires(&so->txtimer,
870880
ktime_add(ktime_get(),
871-
ktime_set(2, 0)));
881+
ktime_set(ISOTP_ECHO_TIMEOUT, 0)));
872882
restart = HRTIMER_RESTART;
873883

874884
/* push out the next consecutive frame */
@@ -896,7 +906,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
896906
break;
897907

898908
default:
899-
WARN_ON_ONCE(1);
909+
WARN_ONCE(1, "can-isotp: tx timer state %08X cfecho %08X\n",
910+
so->tx.state, so->cfecho);
900911
}
901912

902913
return restart;
@@ -912,7 +923,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
912923
struct canfd_frame *cf;
913924
int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
914925
int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0;
915-
s64 hrtimer_sec = 0;
926+
s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT;
916927
int off;
917928
int err;
918929

@@ -931,6 +942,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
931942
err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
932943
if (err)
933944
goto err_out;
945+
946+
so->tx.state = ISOTP_SENDING;
934947
}
935948

936949
if (!size || size > MAX_MSG_LENGTH) {
@@ -975,6 +988,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
975988
cf = (struct canfd_frame *)skb->data;
976989
skb_put_zero(skb, so->ll.mtu);
977990

991+
/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */
992+
if (so->cfecho)
993+
pr_notice_once("can-isotp: uninit cfecho %08X\n", so->cfecho);
994+
978995
/* check for single frame transmission depending on TX_DL */
979996
if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) {
980997
/* The message size generally fits into a SingleFrame - good.
@@ -1000,11 +1017,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
10001017
else
10011018
cf->data[ae] |= size;
10021019

1003-
so->tx.state = ISOTP_IDLE;
1004-
wake_up_interruptible(&so->wait);
1005-
1006-
/* don't enable wait queue for a single frame transmission */
1007-
wait_tx_done = 0;
1020+
/* set CF echo tag for isotp_rcv_echo() (SF-mode) */
1021+
so->cfecho = *(u32 *)cf->data;
10081022
} else {
10091023
/* send first frame */
10101024

@@ -1020,31 +1034,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
10201034
/* disable wait for FCs due to activated block size */
10211035
so->txfc.bs = 0;
10221036

1023-
/* cfecho should have been zero'ed by init */
1024-
if (so->cfecho)
1025-
pr_notice_once("can-isotp: no fc cfecho %08X\n",
1026-
so->cfecho);
1027-
1028-
/* set consecutive frame echo tag */
1037+
/* set CF echo tag for isotp_rcv_echo() (CF-mode) */
10291038
so->cfecho = *(u32 *)cf->data;
1030-
1031-
/* switch directly to ISOTP_SENDING state */
1032-
so->tx.state = ISOTP_SENDING;
1033-
1034-
/* start timeout for unlikely lost echo skb */
1035-
hrtimer_sec = 2;
10361039
} else {
10371040
/* standard flow control check */
10381041
so->tx.state = ISOTP_WAIT_FIRST_FC;
10391042

10401043
/* start timeout for FC */
1041-
hrtimer_sec = 1;
1042-
}
1044+
hrtimer_sec = ISOTP_FC_TIMEOUT;
10431045

1044-
hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0),
1045-
HRTIMER_MODE_REL_SOFT);
1046+
/* no CF echo tag for isotp_rcv_echo() (FF-mode) */
1047+
so->cfecho = 0;
1048+
}
10461049
}
10471050

1051+
hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0),
1052+
HRTIMER_MODE_REL_SOFT);
1053+
10481054
/* send the first or only CAN frame */
10491055
cf->flags = so->ll.tx_flags;
10501056

@@ -1057,8 +1063,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
10571063
__func__, ERR_PTR(err));
10581064

10591065
/* no transmission -> no timeout monitoring */
1060-
if (hrtimer_sec)
1061-
hrtimer_cancel(&so->txtimer);
1066+
hrtimer_cancel(&so->txtimer);
10621067

10631068
/* reset consecutive frame echo tag */
10641069
so->cfecho = 0;

0 commit comments

Comments
 (0)