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

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

Hi Alexander,

On Fri, Jun 19, 2026 at 01:13:00PM +0200, Hölzl, Alexander wrote:
> 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?

Yes, otherwise it seems to bind system resources for no good reason.
Please also add comments in the code explaining this decision.

> > 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?

Yes please. 

Thank you for your work!

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 |