[PATCH] qcan: isotp: implement N_Ar timeout handling for FC transmission

[PATCH] qcan: isotp: implement N_Ar timeout handling for FC transmission

[yewentian395]

Add N_Ar (ISO 15765-2) timeout logic to detect FC frame transmission
failures on the receiver side. Previously, isotp_send_fc() fired the
FC and immediately started the N_Cr timer (rxtimer) without confirming
that the FC was actually transmitted onto the CAN bus.

Introduce ISOTP_WAIT_FC_TX_CONFIRM state and fc_artimer to implement
a two-phase approach:

  1. N_Ar phase: after can_send(FC), wait for local echo confirmation
  2. N_Cr phase: after echo arrives, start rxtimer to wait for next CF

Key design points:

- fc_artimer and rxtimer are mutually exclusive (never both active)
- Timer handler uses guard condition (no spinlock) to avoid deadlock
  with hrtimer_cancel() callers, following isotp_rx_timer_handler pattern
- CF arrival during WAIT_FC_TX_CONFIRM acts as implicit FC confirmation
  (CF from remote proves FC was transmitted successfully)
- New FF/SF arrival during WAIT_FC_TX_CONFIRM cancels fc_artimer
- ISOTP_FC_OVFLW does not enter N_Ar phase (fire-and-forget)
- can_send() failure triggers immediate rollback

State machine extension (RX path only):

  WAIT_DATA -> isotp_send_fc(CTS) -> WAIT_FC_TX_CONFIRM
  WAIT_FC_TX_CONFIRM -> FC echo / CF arrival -> WAIT_DATA
  WAIT_FC_TX_CONFIRM -> fc_artimer timeout -> IDLE (ECOMM)
  WAIT_FC_TX_CONFIRM -> new FF/SF -> IDLE (reset)

Signed-off-by: yewentian395 <yewentian395@gmail.com>
---
 net/can/isotp.c | 93 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 13 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 1efa377f002e..2bdedfc1d1f8 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -125,12 +125,14 @@ MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
 
 #define ISOTP_FC_TIMEOUT 1	/* 1 sec */
 #define ISOTP_ECHO_TIMEOUT 2	/* 2 secs */
+#define ISOTP_FC_AR_TIMEOUT 1	/* 1 sec - N_Ar timeout for FC tx confirm */
 
 enum {
 	ISOTP_IDLE = 0,
 	ISOTP_WAIT_FIRST_FC,
 	ISOTP_WAIT_FC,
 	ISOTP_WAIT_DATA,
+	ISOTP_WAIT_FC_TX_CONFIRM,	/* RX: waiting for local FC echo */
 	ISOTP_SENDING,
 	ISOTP_SHUTDOWN,
 };
@@ -155,7 +157,7 @@ struct isotp_sock {
 	canid_t rxid;
 	ktime_t tx_gap;
 	ktime_t lastrxcf_tstamp;
-	struct hrtimer rxtimer, txtimer, txfrtimer;
+	struct hrtimer rxtimer, txtimer, txfrtimer, fc_artimer;
 	struct can_isotp_options opt;
 	struct can_isotp_fc_options rxfc, txfc;
 	struct can_isotp_ll_options ll;
@@ -210,6 +212,25 @@ static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer)
 	return HRTIMER_NORESTART;
 }
 
+static enum hrtimer_restart isotp_fc_ar_timer_handler(struct hrtimer *hrtimer)
+{
+	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
+					     fc_artimer);
+	struct sock *sk = &so->sk;
+
+	if (so->rx.state != ISOTP_WAIT_FC_TX_CONFIRM)
+		return HRTIMER_NORESTART;
+
+	so->rx.state = ISOTP_IDLE;
+	so->rx.len = 0;
+
+	sk->sk_err = ECOMM;
+	if (!sock_flag(sk, SOCK_DEAD))
+		sk_error_report(sk);
+
+	return HRTIMER_NORESTART;
+}
+
 static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
 {
 	struct net_device *dev;
@@ -256,22 +277,37 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
 
 	ncf->flags = so->ll.tx_flags;
 
-	can_send_ret = can_send(nskb, 1);
-	if (can_send_ret)
-		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
-			       __func__, ERR_PTR(can_send_ret));
-
-	dev_put(dev);
-
 	/* reset blocksize counter */
 	so->rx.bs = 0;
 
 	/* reset last CF frame rx timestamp for rx stmin enforcement */
 	so->lastrxcf_tstamp = ktime_set(0, 0);
 
-	/* start rx timeout watchdog */
-	hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
-		      HRTIMER_MODE_REL_SOFT);
+	if (flowstatus == ISOTP_FC_CTS) {
+		/* cancel rxtimer before entering N_Ar phase */
+		hrtimer_cancel(&so->rxtimer);
+
+		/* enter N_Ar confirmation phase */
+		so->rx.state = ISOTP_WAIT_FC_TX_CONFIRM;
+		hrtimer_start(&so->fc_artimer,
+			      ktime_set(ISOTP_FC_AR_TIMEOUT, 0),
+			      HRTIMER_MODE_REL_SOFT);
+	}
+
+	can_send_ret = can_send(nskb, 1);
+	if (can_send_ret) {
+		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
+			       __func__, ERR_PTR(can_send_ret));
+		if (flowstatus == ISOTP_FC_CTS) {
+			hrtimer_cancel(&so->fc_artimer);
+			so->rx.state = ISOTP_IDLE;
+			so->rx.len = 0;
+		}
+		dev_put(dev);
+		return 1;
+	}
+
+	dev_put(dev);
 	return 0;
 }
 
@@ -447,6 +483,9 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen,
 	struct isotp_sock *so = isotp_sk(sk);
 	struct sk_buff *nskb;
 
+	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM)
+		hrtimer_cancel(&so->fc_artimer);
+
 	hrtimer_cancel(&so->rxtimer);
 	so->rx.state = ISOTP_IDLE;
 
@@ -481,6 +520,9 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
 	int off;
 	int ff_pci_sz;
 
+	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM)
+		hrtimer_cancel(&so->fc_artimer);
+
 	hrtimer_cancel(&so->rxtimer);
 	so->rx.state = ISOTP_IDLE;
 
@@ -554,6 +596,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
 	struct sk_buff *nskb;
 	int i;
 
+	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM) {
+		hrtimer_cancel(&so->fc_artimer);
+		so->rx.state = ISOTP_WAIT_DATA;
+		hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
+			      HRTIMER_MODE_REL_SOFT);
+	}
+
 	if (so->rx.state != ISOTP_WAIT_DATA)
 		return 0;
 
@@ -855,9 +904,22 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 	struct sock *sk = (struct sock *)data;
 	struct isotp_sock *so = isotp_sk(sk);
 	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
 
-	/* 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)
+		return;
+
+	/* FC echo handling: confirm FC was transmitted (N_Ar) */
+	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM &&
+	    (cf->data[ae] & 0xF0) == N_PCI_FC) {
+		hrtimer_cancel(&so->fc_artimer);
+		so->rx.state = ISOTP_WAIT_DATA;
+		hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
+			      HRTIMER_MODE_REL_SOFT);
+		return;
+	}
+
+	if (so->cfecho != *(u32 *)cf->data)
 		return;
 
 	/* cancel local echo timeout */
@@ -1225,6 +1287,7 @@ static int isotp_release(struct socket *sock)
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 	hrtimer_cancel(&so->rxtimer);
+	hrtimer_cancel(&so->fc_artimer);
 
 	so->ifindex = 0;
 	so->bound = 0;
@@ -1563,6 +1626,8 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 					  isotp_rcv_echo, sk);
 		}
 
+		hrtimer_cancel(&so->fc_artimer);
+		so->rx.state = ISOTP_IDLE;
 		so->ifindex = 0;
 		so->bound  = 0;
 		release_sock(sk);
@@ -1639,6 +1704,8 @@ static int isotp_init(struct sock *sk)
 	hrtimer_setup(&so->txtimer, isotp_tx_timer_handler, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	hrtimer_setup(&so->txfrtimer, isotp_txfr_timer_handler, CLOCK_MONOTONIC,
 		      HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&so->fc_artimer, isotp_fc_ar_timer_handler, CLOCK_MONOTONIC,
+		      HRTIMER_MODE_REL_SOFT);
 
 	init_waitqueue_head(&so->wait);
 	spin_lock_init(&so->rx_lock);
-- 
2.43.0

Re: [PATCH] qcan: isotp: implement N_Ar timeout handling for FC transmission

[Oliver Hartkopp]

On 24.08.26 14:53, yewentian395 wrote:
> Add N_Ar (ISO 15765-2) timeout logic to detect FC frame transmission
> failures on the receiver side. Previously, isotp_send_fc() fired the
> FC and immediately started the N_Cr timer (rxtimer) without confirming
> that the FC was actually transmitted onto the CAN bus.
> 
> Introduce ISOTP_WAIT_FC_TX_CONFIRM state and fc_artimer to implement
> a two-phase approach:
> 
>    1. N_Ar phase: after can_send(FC), wait for local echo confirmation
>    2. N_Cr phase: after echo arrives, start rxtimer to wait for next CF

The kernel ISO 15765-2 implementation uses a simplified approach for As 
and Ar: You can specify the frame transmission time (N_As/N_Ar) in 
can_isotp_options::frame_txtime which covers the calculated time of the 
CAN frame on the bus (on the "wire"), e.g. for the tx path:

	/* add transmission time for CAN frame N_As */
	so->tx_gap = ktime_add_ns(so->tx_gap, so->frame_txtime);

In fact I don't know any active users of so->frame_txtime since the 
Linux ISO 15765-2 implementation went online in April 2014. It only had 
some value for testing.

Nobody cares about so->frame_txtime and therefore I will not add any 
extra complexity to check for Ar/As timeouts which have no real world 
effect.

> -	/* start rx timeout watchdog */
> -	hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
> -		      HRTIMER_MODE_REL_SOFT);

Btw. while double-checking the code I have seen, that I was missing the 
addition of so->frame_txtime when starting the rxtimer above.

Although this proves again that checking for Ar/As timeouts has no real 
world effect, I will create a patch that adds so->frame_txtime here to 
comply with the documentation in isotp.h:

   __u32 frame_txtime;     /* frame transmission time (N_As/N_Ar)  */
                           /* __u32 value : time in nano secs      */

I'll mention you with a Reported-by tag then.

> +	if (flowstatus == ISOTP_FC_CTS) {
> +		/* cancel rxtimer before entering N_Ar phase */
> +		hrtimer_cancel(&so->rxtimer);
> +
> +		/* enter N_Ar confirmation phase */
> +		so->rx.state = ISOTP_WAIT_FC_TX_CONFIRM;
> +		hrtimer_start(&so->fc_artimer,
> +			      ktime_set(ISOTP_FC_AR_TIMEOUT, 0),
> +			      HRTIMER_MODE_REL_SOFT);
> +	}
> +
> +	can_send_ret = can_send(nskb, 1);
> +	if (can_send_ret) {
> +		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
> +			       __func__, ERR_PTR(can_send_ret));
> +		if (flowstatus == ISOTP_FC_CTS) {
> +			hrtimer_cancel(&so->fc_artimer);
> +			so->rx.state = ISOTP_IDLE;
> +			so->rx.len = 0;
> +		}
> +		dev_put(dev);
> +		return 1;
> +	}
> +
> +	dev_put(dev);
>   	return 0;
>   }
>   
> @@ -447,6 +483,9 @@ static int isotp_rcv_sf(struct sock *sk, struct canfd_frame *cf, int pcilen,
>   	struct isotp_sock *so = isotp_sk(sk);
>   	struct sk_buff *nskb;
>   
> +	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM)
> +		hrtimer_cancel(&so->fc_artimer);
> +
>   	hrtimer_cancel(&so->rxtimer);
>   	so->rx.state = ISOTP_IDLE;
>   
> @@ -481,6 +520,9 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
>   	int off;
>   	int ff_pci_sz;
>   
> +	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM)
> +		hrtimer_cancel(&so->fc_artimer);
> +
>   	hrtimer_cancel(&so->rxtimer);
>   	so->rx.state = ISOTP_IDLE;
>   
> @@ -554,6 +596,13 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
>   	struct sk_buff *nskb;
>   	int i;
>   
> +	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM) {
> +		hrtimer_cancel(&so->fc_artimer);
> +		so->rx.state = ISOTP_WAIT_DATA;
> +		hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
> +			      HRTIMER_MODE_REL_SOFT);
> +	}
> +
>   	if (so->rx.state != ISOTP_WAIT_DATA)
>   		return 0;
>   
> @@ -855,9 +904,22 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>   	struct sock *sk = (struct sock *)data;
>   	struct isotp_sock *so = isotp_sk(sk);
>   	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
>   
> -	/* only handle my own local echo CF/SF skb's (no FF!) */
> -	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)

Please also note that your patch would not apply on the latest upstream 
isotp code, which does not contain "*(u32 *)cf->data" anymore.

Best regards,
Oliver

> +	if (skb->sk != sk)
> +		return;
> +
> +	/* FC echo handling: confirm FC was transmitted (N_Ar) */
> +	if (so->rx.state == ISOTP_WAIT_FC_TX_CONFIRM &&
> +	    (cf->data[ae] & 0xF0) == N_PCI_FC) {
> +		hrtimer_cancel(&so->fc_artimer);
> +		so->rx.state = ISOTP_WAIT_DATA;
> +		hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
> +			      HRTIMER_MODE_REL_SOFT);
> +		return;
> +	}
> +
> +	if (so->cfecho != *(u32 *)cf->data)
>   		return;
>   
>   	/* cancel local echo timeout */
> @@ -1225,6 +1287,7 @@ static int isotp_release(struct socket *sock)
>   	hrtimer_cancel(&so->txfrtimer);
>   	hrtimer_cancel(&so->txtimer);
>   	hrtimer_cancel(&so->rxtimer);
> +	hrtimer_cancel(&so->fc_artimer);
>   
>   	so->ifindex = 0;
>   	so->bound = 0;
> @@ -1563,6 +1626,8 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
>   					  isotp_rcv_echo, sk);
>   		}
>   
> +		hrtimer_cancel(&so->fc_artimer);
> +		so->rx.state = ISOTP_IDLE;
>   		so->ifindex = 0;
>   		so->bound  = 0;
>   		release_sock(sk);
> @@ -1639,6 +1704,8 @@ static int isotp_init(struct sock *sk)
>   	hrtimer_setup(&so->txtimer, isotp_tx_timer_handler, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
>   	hrtimer_setup(&so->txfrtimer, isotp_txfr_timer_handler, CLOCK_MONOTONIC,
>   		      HRTIMER_MODE_REL_SOFT);
> +	hrtimer_setup(&so->fc_artimer, isotp_fc_ar_timer_handler, CLOCK_MONOTONIC,
> +		      HRTIMER_MODE_REL_SOFT);
>   
>   	init_waitqueue_head(&so->wait);
>   	spin_lock_init(&so->rx_lock);

Re: [PATCH] can: isotp: implement N_Ar timeout handling for FC transmission

[yewentian395]

On 24.08.26 20:03, Oliver Hartkopp wrote:
> Nobody cares about so->frame_txtime and therefore I will not add any
> extra complexity to check for Ar/As timeouts which have no real world
> effect.

Thanks for the detailed explanation. The simplified model makes sense
given that no real-world user depends on strict N_Ar confirmation.

> Btw. while double-checking the code I have seen, that I was missing the
> addition of so->frame_txtime when starting the rxtimer above.
>
> I'll mention you with a Reported-by tag then.

Thank you, appreciated.

> Please also note that your patch would not apply on the latest upstream
> isotp code, which does not contain "*(u32 *)cf->data" anymore.

Noted. I was working against an older tree. Will rebase to latest
upstream for any future submissions.

Best regards,
wentian

Re: [PATCH] can: isotp: implement N_Ar timeout handling for FC transmission

[yewentian395]

Hi Oliver,

Just a gentle ping on the frame_txtime addition to rxtimer you
mentioned. Happy to help with a patch if that saves you time.

Best regards,
wentian

Re: [PATCH] can: isotp: implement N_Ar timeout handling for FC transmission

[Oliver Hartkopp]

Hi Wentian,

On 03.09.26 11:38, yewentian395 wrote:

> Just a gentle ping on the frame_txtime addition to rxtimer you
> mentioned. Happy to help with a patch if that saves you time.

This is my current patch which is sitting on my dev machine:

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 130a0dbec78c..5119afc2f67d 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -125,10 +125,11 @@ MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu 
size (default "
  #define ISOTP_FC_WT 1		/* wait */
  #define ISOTP_FC_OVFLW 2	/* overflow */

  #define ISOTP_FC_TIMEOUT 1	/* 1 sec */
  #define ISOTP_ECHO_TIMEOUT 2	/* 2 secs */
+#define ISOTP_MAX_FRAME_TXTIME_NS (100 * NSEC_PER_MSEC) /* max N_Ar/N_As */

  /* so->tx_result[so->tx_gen % ISOTP_TX_RESULT_SLOTS] holds the packed 
value
   * (err << ISOTP_TX_RESULT_GEN_BITS | gen) for each tx generation 
slot, so it
   * can be handled with a single READ_ONCE()/WRITE_ONCE() access.
   */
@@ -346,11 +347,12 @@ static int isotp_send_fc(struct sock *sk, int ae, 
u8 flowstatus)

  	/* reset last CF frame rx timestamp for rx stmin enforcement */
  	so->lastrxcf_tstamp = ktime_set(0, 0);

  	/* start rx timeout watchdog */
-	hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
+	hrtimer_start(&so->rxtimer,
+		      ktime_set(ISOTP_FC_TIMEOUT, so->frame_txtime),
  		      HRTIMER_MODE_REL_SOFT);
  	return 0;
  }

  static void isotp_rcv_skb(struct sk_buff *skb, struct sock *sk)
@@ -1304,11 +1306,11 @@ static int isotp_sendmsg(struct socket *sock, 
struct msghdr *msg, size_t size)
  		wake_up_interruptible(&so->wait);
  		return -EADDRNOTAVAIL;
  	}
  	/* WAIT_FIRST_FC for standard FF, else stays ISOTP_SENDING */
  	WRITE_ONCE(so->tx.state, new_state);
-	hrtimer_start(tx_hrt, ktime_set(hrtimer_sec, 0),
+	hrtimer_start(tx_hrt, ktime_set(hrtimer_sec, so->frame_txtime),
  		      HRTIMER_MODE_REL_SOFT);
  	spin_unlock_bh(&so->rx_lock);

  	/* send the first or only CAN frame */
  	cf->flags = so->ll.tx_flags;
@@ -1711,10 +1713,12 @@ static int isotp_setsockopt_locked(struct socket 
*sock, int level, int optname,

  		/* check for frame_txtime changes (0 => no changes) */
  		if (so->opt.frame_txtime) {
  			if (so->opt.frame_txtime == CAN_ISOTP_FRAME_TXTIME_ZERO)
  				so->frame_txtime = 0;
+			else if (so->opt.frame_txtime > ISOTP_MAX_FRAME_TXTIME_NS)
+				return -EINVAL;
  			else
  				so->frame_txtime = so->opt.frame_txtime;
  		}
  		break;


But this patch will be part of a big chunk of patches to introduce the 
CAN XL support for ISO 15765-2 Ed.5 here:

https://github.com/hartkopp/canxl-isotp

And the latest patch set is here:
https://github.com/hartkopp/xl-isotp/tree/claude/can-isotp-patches-rebase-v3

There are currently stable fixes on the fly that always have impacts on 
the isotp code. An it is really annoying to rebase and rebase and 
rebase. Especially the code split(!) into different .c and .h files 
always has to be checked by hand. Therefore I'll wait for all the 
current stable patches to be landed in mainline and stable and then add 
this patch here to the series of cleanup and enhancement patches.

Best regards,
Oliver