Commit 86633786 authored by Oliver Hartkopp's avatar Oliver Hartkopp Committed by Marc Kleine-Budde

can: isotp: fix tx state handling for echo tx processing

In commit 4b7fe92c ("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: 4b7fe92c ("can: isotp: add local echo tx processing for consecutive frames")
Link: https://lore.kernel.org/linux-can/CAO4mrfe3dG7cMP1V5FLUkw7s+50c9vichigUMQwsxX4M=45QEw@mail.gmail.com/T/#uReported-by: default avatarWei Chen <harperchen1110@gmail.com>
Cc: stable@vger.kernel.org # v6.0
Signed-off-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Link: https://lore.kernel.org/all/20221104142551.16924-1-socketcan@hartkopp.netSigned-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent 8aa59e35
...@@ -111,6 +111,9 @@ MODULE_ALIAS("can-proto-6"); ...@@ -111,6 +111,9 @@ MODULE_ALIAS("can-proto-6");
#define ISOTP_FC_WT 1 /* wait */ #define ISOTP_FC_WT 1 /* wait */
#define ISOTP_FC_OVFLW 2 /* overflow */ #define ISOTP_FC_OVFLW 2 /* overflow */
#define ISOTP_FC_TIMEOUT 1 /* 1 sec */
#define ISOTP_ECHO_TIMEOUT 2 /* 2 secs */
enum { enum {
ISOTP_IDLE = 0, ISOTP_IDLE = 0,
ISOTP_WAIT_FIRST_FC, ISOTP_WAIT_FIRST_FC,
...@@ -258,7 +261,8 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus) ...@@ -258,7 +261,8 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
so->lastrxcf_tstamp = ktime_set(0, 0); so->lastrxcf_tstamp = ktime_set(0, 0);
/* start rx timeout watchdog */ /* start rx timeout watchdog */
hrtimer_start(&so->rxtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT); hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT);
return 0; return 0;
} }
...@@ -344,6 +348,8 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf, ...@@ -344,6 +348,8 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf,
return 0; return 0;
} }
static void isotp_send_cframe(struct isotp_sock *so);
static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
{ {
struct sock *sk = &so->sk; struct sock *sk = &so->sk;
...@@ -398,14 +404,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) ...@@ -398,14 +404,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
case ISOTP_FC_CTS: case ISOTP_FC_CTS:
so->tx.bs = 0; so->tx.bs = 0;
so->tx.state = ISOTP_SENDING; so->tx.state = ISOTP_SENDING;
/* start cyclic timer for sending CF frame */ /* send CF frame and enable echo timeout handling */
hrtimer_start(&so->txtimer, so->tx_gap, hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT); HRTIMER_MODE_REL_SOFT);
isotp_send_cframe(so);
break; break;
case ISOTP_FC_WT: case ISOTP_FC_WT:
/* start timer to wait for next FC frame */ /* start timer to wait for next FC frame */
hrtimer_start(&so->txtimer, ktime_set(1, 0), hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT); HRTIMER_MODE_REL_SOFT);
break; break;
...@@ -600,7 +607,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, ...@@ -600,7 +607,7 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
/* perform blocksize handling, if enabled */ /* perform blocksize handling, if enabled */
if (!so->rxfc.bs || ++so->rx.bs < so->rxfc.bs) { if (!so->rxfc.bs || ++so->rx.bs < so->rxfc.bs) {
/* start rx timeout watchdog */ /* start rx timeout watchdog */
hrtimer_start(&so->rxtimer, ktime_set(1, 0), hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT); HRTIMER_MODE_REL_SOFT);
return 0; return 0;
} }
...@@ -829,7 +836,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) ...@@ -829,7 +836,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
struct isotp_sock *so = isotp_sk(sk); struct isotp_sock *so = isotp_sk(sk);
struct canfd_frame *cf = (struct canfd_frame *)skb->data; struct canfd_frame *cf = (struct canfd_frame *)skb->data;
/* only handle my own local echo skb's */ /* only handle my own local echo CF/SF skb's (no FF!) */
if (skb->sk != sk || so->cfecho != *(u32 *)cf->data) if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
return; return;
...@@ -849,13 +856,16 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data) ...@@ -849,13 +856,16 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
if (so->txfc.bs && so->tx.bs >= so->txfc.bs) { if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {
/* stop and wait for FC with timeout */ /* stop and wait for FC with timeout */
so->tx.state = ISOTP_WAIT_FC; so->tx.state = ISOTP_WAIT_FC;
hrtimer_start(&so->txtimer, ktime_set(1, 0), hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT); HRTIMER_MODE_REL_SOFT);
return; return;
} }
/* no gap between data frames needed => use burst mode */ /* no gap between data frames needed => use burst mode */
if (!so->tx_gap) { if (!so->tx_gap) {
/* enable echo timeout handling */
hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
HRTIMER_MODE_REL_SOFT);
isotp_send_cframe(so); isotp_send_cframe(so);
return; return;
} }
...@@ -879,7 +889,7 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer) ...@@ -879,7 +889,7 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
/* start timeout for unlikely lost echo skb */ /* start timeout for unlikely lost echo skb */
hrtimer_set_expires(&so->txtimer, hrtimer_set_expires(&so->txtimer,
ktime_add(ktime_get(), ktime_add(ktime_get(),
ktime_set(2, 0))); ktime_set(ISOTP_ECHO_TIMEOUT, 0)));
restart = HRTIMER_RESTART; restart = HRTIMER_RESTART;
/* push out the next consecutive frame */ /* push out the next consecutive frame */
...@@ -907,7 +917,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer) ...@@ -907,7 +917,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
break; break;
default: default:
WARN_ON_ONCE(1); WARN_ONCE(1, "can-isotp: tx timer state %08X cfecho %08X\n",
so->tx.state, so->cfecho);
} }
return restart; return restart;
...@@ -923,7 +934,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -923,7 +934,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
struct canfd_frame *cf; struct canfd_frame *cf;
int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0; int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0; int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0;
s64 hrtimer_sec = 0; s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT;
int off; int off;
int err; int err;
...@@ -942,6 +953,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -942,6 +953,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
if (err) if (err)
goto err_out; goto err_out;
so->tx.state = ISOTP_SENDING;
} }
if (!size || size > MAX_MSG_LENGTH) { if (!size || size > MAX_MSG_LENGTH) {
...@@ -986,6 +999,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -986,6 +999,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
cf = (struct canfd_frame *)skb->data; cf = (struct canfd_frame *)skb->data;
skb_put_zero(skb, so->ll.mtu); skb_put_zero(skb, so->ll.mtu);
/* cfecho should have been zero'ed by init / former isotp_rcv_echo() */
if (so->cfecho)
pr_notice_once("can-isotp: uninit cfecho %08X\n", so->cfecho);
/* check for single frame transmission depending on TX_DL */ /* check for single frame transmission depending on TX_DL */
if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) { if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) {
/* The message size generally fits into a SingleFrame - good. /* The message size generally fits into a SingleFrame - good.
...@@ -1011,11 +1028,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -1011,11 +1028,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
else else
cf->data[ae] |= size; cf->data[ae] |= size;
so->tx.state = ISOTP_IDLE; /* set CF echo tag for isotp_rcv_echo() (SF-mode) */
wake_up_interruptible(&so->wait); so->cfecho = *(u32 *)cf->data;
/* don't enable wait queue for a single frame transmission */
wait_tx_done = 0;
} else { } else {
/* send first frame */ /* send first frame */
...@@ -1031,31 +1045,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -1031,31 +1045,23 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
/* disable wait for FCs due to activated block size */ /* disable wait for FCs due to activated block size */
so->txfc.bs = 0; so->txfc.bs = 0;
/* cfecho should have been zero'ed by init */ /* set CF echo tag for isotp_rcv_echo() (CF-mode) */
if (so->cfecho)
pr_notice_once("can-isotp: no fc cfecho %08X\n",
so->cfecho);
/* set consecutive frame echo tag */
so->cfecho = *(u32 *)cf->data; so->cfecho = *(u32 *)cf->data;
/* switch directly to ISOTP_SENDING state */
so->tx.state = ISOTP_SENDING;
/* start timeout for unlikely lost echo skb */
hrtimer_sec = 2;
} else { } else {
/* standard flow control check */ /* standard flow control check */
so->tx.state = ISOTP_WAIT_FIRST_FC; so->tx.state = ISOTP_WAIT_FIRST_FC;
/* start timeout for FC */ /* start timeout for FC */
hrtimer_sec = 1; hrtimer_sec = ISOTP_FC_TIMEOUT;
}
hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0), /* no CF echo tag for isotp_rcv_echo() (FF-mode) */
HRTIMER_MODE_REL_SOFT); so->cfecho = 0;
}
} }
hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0),
HRTIMER_MODE_REL_SOFT);
/* send the first or only CAN frame */ /* send the first or only CAN frame */
cf->flags = so->ll.tx_flags; cf->flags = so->ll.tx_flags;
...@@ -1068,8 +1074,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -1068,8 +1074,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
__func__, ERR_PTR(err)); __func__, ERR_PTR(err));
/* no transmission -> no timeout monitoring */ /* no transmission -> no timeout monitoring */
if (hrtimer_sec) hrtimer_cancel(&so->txtimer);
hrtimer_cancel(&so->txtimer);
/* reset consecutive frame echo tag */ /* reset consecutive frame echo tag */
so->cfecho = 0; so->cfecho = 0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment