Re: [PATCH] Fix J1939 implementation not handling holds correctly

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Tue, May 05, 2026 at 12:18:33AM +0200, Alexander Hölzl wrote:
> The J1939 protocol allows the receiver of directed segemented messages
> to hold the data transfer. The kernel implementation did not handle hold
> messages correctly was not able to resume from a hold.
> Additionally the incorrect timeout for a held open connection was
> changed to 1050ms.
> Some simple tests for the general protocol behavior have been added.
> 
> This is just an initial draft I'm just looking for some feedback
> if the general setup is acceptable.
> 
> Also I still have some points I'm not sure about.
> 1. There is no check for the reception of a CTS during an ongoing data
>    transfer. According to the standard it is illegal, but I dont think
>    the implementation cares about it. J1939_XTP_ABORT_GENERIC is never
>    referenced and the previous check I removed only really prevented
>    consecutive CTS which are legal if they are holds. Should I add a
>    check for this? This seems to be a slightly different problem so I
>    did not address it.

Current I do not have opinion about this. As long as it is in a separate
patch with enough explanation and tests - I'm OK.

> 2. Should userspace be notified about holds? I don't think the standard
>    gives a maximum 'hold open time' so in theory it could be held
>    indefinetly. This might be a situation a user could be intrested in
>    even though it is rather theoretical.

Yes, it will be good to have a user space feedback. But, it would block
resources. Will we get difference scenarios where we still have an open
session which is blocking other transfers?

> Please tell me what you think.
> 
> Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
> ---
>  net/can/j1939/transport.c                     |  50 ++--
>  tools/testing/selftests/net/can/.gitignore    |   1 +
>  tools/testing/selftests/net/can/Makefile      |   8 +-
>  tools/testing/selftests/net/can/config        |   1 +
>  .../testing/selftests/net/can/test_cts_hold.c | 269 ++++++++++++++++++
>  .../selftests/net/can/test_cts_hold.sh        |  45 +++
>  6 files changed, 355 insertions(+), 19 deletions(-)
>  create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c
>  create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da..e53a9fbe82a9 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1442,9 +1442,27 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>  
>  	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>  
> -	if (session->last_cmd == dat[0]) {
> -		err = J1939_XTP_ABORT_DUP_SEQ;
> -		goto out_session_cancel;
> +	session->last_cmd = dat[0];
> +
> +	/* TODO
> +	 *according to the standard it is illegal to send a CTS while data transfer is ongoing.
> +	 * If a CTS is sent while data transfer is ongoing an abort with code 4 should be sent.
> +	 * The previous check I removed was simply (session->last_cmd == dat[0]).
> +	 * This did not check for a invalid CTS after some data frames had already been
> +	 * sent but before all of them were sent.
> +	 * So CTS(0), CTS(5) was illegal. CTS(5), DAT, CTS(5) was legal,
> +	 * even though only one data frame has been sent instead of 5.
> +	 * Should a check for this also be added?
> +	 */
> +	if (!dat[1]) {

We need a helper function or some defines for different CTS variants.

> +		/* CTS(0) */
> +		if (session->transmission) {
> +			/* TODO notify error queue about hold messages? */

Yes. Can be done in a separate patch.

> +			j1939_session_txtimer_cancel(session);
> +		}
> +		j1939_tp_set_rxtimeout(session, 1050);

I guess we need defines for timouts too.

> +		netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
> +		return;
>  	}
>  
....

> diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
> index 764a53fc837f..96ef18ae986d 100644
> --- a/tools/testing/selftests/net/can/.gitignore
> +++ b/tools/testing/selftests/net/can/.gitignore
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  test_raw_filter
> +test_cts_hold
> \ No newline at end of file
> diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
> index 5b82e60a03e7..182346682bce 100644
> --- a/tools/testing/selftests/net/can/Makefile
> +++ b/tools/testing/selftests/net/can/Makefile
> @@ -4,8 +4,12 @@ top_srcdir = ../../../../..
>  
>  CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
>  
> -TEST_PROGS := test_raw_filter.sh
> +TEST_PROGS := \
> +	test_raw_filter.sh \
> +	test_cts_hold.sh \
>  
> -TEST_GEN_FILES := test_raw_filter
> +TEST_GEN_FILES := \
> +	test_raw_filter \
> +	test_cts_hold \
>  
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/net/can/config b/tools/testing/selftests/net/can/config
> index 188f79796670..cb538ed93ae4 100644
> --- a/tools/testing/selftests/net/can/config
> +++ b/tools/testing/selftests/net/can/config
> @@ -1,3 +1,4 @@
>  CONFIG_CAN=m
>  CONFIG_CAN_DEV=m
>  CONFIG_CAN_VCAN=m
> +CONFIG_CAN_J1939=m
> \ No newline at end of file
> diff --git a/tools/testing/selftests/net/can/test_cts_hold.c b/tools/testing/selftests/net/can/test_cts_hold.c
> new file mode 100644
> index 000000000000..a36efb099522
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_cts_hold.c

Sashiko has here some comments for this file.
https://sashiko.dev/#/patchset/20260504221833.53629-1-alexander.hoelzl%40gmx.net

> +/* Test holding RTS/CTS transport on first frame and resuming immediatley */
> +TEST_F(can_env, test_hold_resume_immediate)
> +/* Test send hold in transport session and resuming */
> +TEST_F(can_env, test_hold_resume)
> +/* Test timeout after not resuming hold */
> +TEST_F(can_env, test_hold_timeout)
 
Nice!

It would be good to add usual transfer test as baseline to make sure
stack works as expected. I guess a simple 2 frame TP should be enough.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |