Re: [PATCH v3 1/2] Fix J1939 implementation not handling holds correctly

[Hölzl, Alexander]

Hi Oleksij,

Am 10.06.2026 um 11:42 schrieb Oleksij Rempel:
> Hi Alexander,
>
> Sorry I lost the track of this patches.

No worries!

> Can you please take a look here:
> https://sashiko.dev/#/patchset/20260525190948.42461-1-alexander.hoelzl%40gmx.net
>
> You can ignore the warning in net/can/j1939/transport.c
> I guess it is protocol specific issue (potentially can be commented in
> the source code), if you have other opinion, please share :)

The bot is right and after looking through the specs once again
there are requirements mentioned regarding retransmission
requests. In 5.10.3.2 Connection Mode Clear to Send (CTS):
...
When the CTS message is used to request the retransmission of data 
packet(s), it is recommended not to use more than two retransmit 
requests. When this limit is reached, a connection abort with abort 
reason 5 from Table 6 shall be sent.
...

This paragraph to me sounds more like a requirement for the responder to
stop requesting retransmissions.


Second there is also this:
5.12.3 Device Response Time and Timeout Defaults
...
Number of request retries = 2 (3 requests total); this includes the 
situation where the CTS is used to request the retransmission of data 
packet(s). If the retransmit request limit is reached, then the 
connection abort shall be sent with abort reason 5 from Table 6.
...
This sounds a bit more generic and not related specifically to responder 
or originator. I did not see a mention of in any of those requirements
in the compliance spec J1939-82 however...

Do you think I should add a counter for retransmit requests?
If yes should it also apply to holds?

> There are some typos in the tests, can you please address them.
Sure!> On Mon, May 25, 2026 at 09:08:48PM +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.
> >
> > To do so the behavior of j1939_xtp_rx_cts_one was modified to allow the
> > handling of a hold. The previous sanity check was removed as it only
> > guarded against a flood of consecutive CTS, but prevented the hold
> > from working correctly. This patch changes this behavior to allow
> > for consectuive CTS to enable holds. An additional sanity check
> > has been added which prevents requsts of already transferred and
> > acked packets. In this case the kernel will abort immediately
> > instead of going into a timeout.
> >
> > Fix J1939 RTS/CTS session not being able to resume from hold.
> > Replace hardcoded timeout with define.
> > Add CTS hold behavior tests.

...
In addition just want to mention this check I've introduced, which 
prevents requesting packets which the responder has already acknowledged 
in a previous CTS:>> -	/* set packet counters only when not CTS(0) */
> > +	if (session->pkt.tx_acked >= pkt) {
> > +		err = J1939_XTP_ABORT_DUP_SEQ;
> > +		goto out_session_cancel;
> > +	}
> > +
I couldn't find this requirement in J1939-21 but the compliance testing 
spec J1939-82 mentions it in table A7 row 6:

	Verify DUT transmits a TP.Conn_Abort when 'Next packet number
	to be sent' in TP.CM_CTS message:
	- is less than the 'Next packet number to be sent' in 	
	  previous TP.CM_CTS

Should I add this as a comment as well?

Regards,
Alex