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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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);