Re: [PATCH v3 1/2] Fix J1939 implementation not handling holds correctly
From: Oleksij Rempel
Date: Fri Jun 19 2026 - 07:44:10 EST
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 |