j1939

j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7204 bytes --]

Hello,

I've ported Kurt's latest j1939 patches to linux-4.7. I've squashed all
patches into one. Then a patch to undo unused modifications and a bunch
of patches to make checkpatch happy.

I've pushed the result to

https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939

The remaining warning is about a case statement without fall through
annotations and/or breaks:

> WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> #4017: FILE: net/can/j1939/transport.c:1031:
> +	case tp_cmd_cts:
> 
> WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> #4018: FILE: net/can/j1939/transport.c:1032:
> +	case 0xff: /* did some data */
> 
> WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> #4019: FILE: net/can/j1939/transport.c:1033:
> +	case etp_cmd_dpo:

The code in question is:

> /* transmit function */
> static int j1939tp_txnext(struct session *session)
> {
> 	u8 dat[8];
> 	const u8 *tpdat;
> 	int ret, offset, pkt_done, pkt_end;
> 	unsigned int pkt, len, pdelay;
> 
> 	memset(dat, 0xff, sizeof(dat));
> 	get_session(session); /* do not loose it */
> 
> 	switch (session->last_cmd) {
> 	case 0:
> 		if (!j1939tp_im_transmitter(session->skb))
> 			break;
> 		dat[1] = (session->skb->len >> 0) & 0xff;
> 		dat[2] = (session->skb->len >> 8) & 0xff;
> 		dat[3] = session->pkt.total;
> 		if (session->extd) {
> 			dat[0] = etp_cmd_rts;
> 			dat[1] = (session->skb->len >> 0) & 0xff;
> 			dat[2] = (session->skb->len >> 8) & 0xff;
> 			dat[3] = (session->skb->len >> 16) & 0xff;
> 			dat[4] = (session->skb->len >> 24) & 0xff;
> 		} else if (j1939cb_is_broadcast(session->cb)) {
> 			dat[0] = tp_cmd_bam;
> 			/* fake cts for broadcast */
> 			session->pkt.tx = 0;
> 		} else {
> 			dat[0] = tp_cmd_rts;
> 			dat[4] = dat[3];
> 		}
> 		if (dat[0] == session->last_txcmd)
> 			/* done already */
> 			break;
> 		ret = j1939tp_tx_ctl(session, 0, dat);
> 		if (ret < 0)
> 			goto failed;
> 		session->last_txcmd = dat[0];
> 		/* must lock? */
> 		if (tp_cmd_bam == dat[0])
> 			j1939tp_schedule_txtimer(session, 50);
> 		j1939tp_set_rxtimeout(session, 1250);
> 		break;
> 	case tp_cmd_rts:
> 	case etp_cmd_rts:
> 		if (!j1939tp_im_receiver(session->skb))
> 			break;
>  tx_cts:
> 		ret = 0;
> 		len = session->pkt.total - session->pkt.done;
> 		len = min(max(len, session->pkt.block), block ?: 255);
> 
> 		if (session->extd) {
> 			pkt = session->pkt.done + 1;
> 			dat[0] = etp_cmd_cts;
> 			dat[1] = len;
> 			dat[2] = (pkt >>  0) & 0xff;
> 			dat[3] = (pkt >>  8) & 0xff;
> 			dat[4] = (pkt >> 16) & 0xff;
> 		} else {
> 			dat[0] = tp_cmd_cts;
> 			dat[1] = len;
> 			dat[2] = session->pkt.done + 1;
> 		}
> 		if (dat[0] == session->last_txcmd)
> 			/* done already */
> 			break;
> 		ret = j1939tp_tx_ctl(session, 1, dat);
> 		if (ret < 0)
> 			goto failed;
> 		if (len)
> 			/* only mark cts done when len is set */
> 			session->last_txcmd = dat[0];
> 		j1939tp_set_rxtimeout(session, 1250);
> 		break;
> 	case etp_cmd_cts:
> 		if (j1939tp_im_transmitter(session->skb) && session->extd &&
> 		    (etp_cmd_dpo != session->last_txcmd)) {
> 			/* do dpo */
> 			dat[0] = etp_cmd_dpo;
> 			session->pkt.dpo = session->pkt.done;
> 			pkt = session->pkt.dpo;
> 			dat[1] = session->pkt.last - session->pkt.done;
> 			dat[2] = (pkt >>  0) & 0xff;
> 			dat[3] = (pkt >>  8) & 0xff;
> 			dat[4] = (pkt >> 16) & 0xff;
> 			ret = j1939tp_tx_ctl(session, 0, dat);
> 			if (ret < 0)
> 				goto failed;
> 			session->last_txcmd = dat[0];
> 			j1939tp_set_rxtimeout(session, 1250);
> 			session->pkt.tx = session->pkt.done;
> 		}
> 	case tp_cmd_cts:
> 	case 0xff: /* did some data */
> 	case etp_cmd_dpo:
> 		if ((session->extd || !j1939cb_is_broadcast(session->cb)) &&
> 		    j1939tp_im_receiver(session->skb)) {
> 			if (session->pkt.done >= session->pkt.total) {
> 				if (session->extd) {
> 					dat[0] = etp_cmd_eof;
> 					dat[1] = session->skb->len >> 0;
> 					dat[2] = session->skb->len >> 8;
> 					dat[3] = session->skb->len >> 16;
> 					dat[4] = session->skb->len >> 24;
> 				} else {
> 					dat[0] = tp_cmd_eof;
> 					dat[1] = session->skb->len;
> 					dat[2] = session->skb->len >> 8;
> 					dat[3] = session->pkt.total;
> 				}
> 				if (dat[0] == session->last_txcmd)
> 					/* done already */
> 					break;
> 				ret = j1939tp_tx_ctl(session, 1, dat);
> 				if (ret < 0)
> 					goto failed;
> 				session->last_txcmd = dat[0];
> 				j1939tp_set_rxtimeout(session, 1250);
> 				/* wait for the EOF packet to come in */
> 				break;
> 			} else if (session->pkt.done >= session->pkt.last) {
> 				session->last_txcmd = 0;
> 				goto tx_cts;
> 			}
> 		}
> 	case tp_cmd_bam:
> 		if (!j1939tp_im_transmitter(session->skb))
> 			break;
> 		tpdat = session->skb->data;
> 		ret = 0;
> 		pkt_done = 0;
> 		pkt_end = (!session->extd && j1939cb_is_broadcast(session->cb))
> 			? session->pkt.total : session->pkt.last;
> 
> 		while (session->pkt.tx < pkt_end) {
> 			dat[0] = session->pkt.tx - session->pkt.dpo + 1;
> 			offset = session->pkt.tx * 7;
> 			len = session->skb->len - offset;
> 			if (len > 7)
> 				len = 7;
> 			memcpy(&dat[1], &tpdat[offset], len);
> 			ret = j1939tp_tx_dat(session->skb, session->extd,
> 					     dat, len + 1);
> 			if (ret < 0)
> 				break;
> 			session->last_txcmd = 0xff;
> 			++pkt_done;
> 			++session->pkt.tx;
> 			pdelay = j1939cb_is_broadcast(session->cb) ?  50 :
> 				packet_delay;
> 			if ((session->pkt.tx < session->pkt.total) && pdelay) {
> 				j1939tp_schedule_txtimer(session, pdelay);
> 				break;
> 			}
> 		}
> 		if (pkt_done)
> 			j1939tp_set_rxtimeout(session, 250);
> 		if (ret)
> 			goto failed;
> 		break;
> 	}
> 	put_session(session);
> 	return 0;
>  failed:
> 	put_session(session);
> 	return ret;
> }

Is here someone with insight and can tell if the code is correct this way?

Another thing that IMHO has to be sorted out is the use of module
parameters:

> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");

Better convert them to netlink options.

Next think I'll do is to establish some communication with an external
j1939 component. I'll keep you updated.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Oliver Hartkopp]

On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:

>
> Another thing that IMHO has to be sorted out is the use of module
> parameters:
>
>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");
>
> Better convert them to netlink options.
>

What about putting them into socket depended sockopts with setsockopt()?

Regards,
Oliver


> Next think I'll do is to establish some communication with an external
> j1939 component. I'll keep you updated.
>
> regards,
> Marc
>

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1275 bytes --]

On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:
> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> 
>>
>> Another thing that IMHO has to be sorted out is the use of module
>> parameters:
>>
>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");
>>
>> Better convert them to netlink options.
>>
> 
> What about putting them into socket depended sockopts with setsockopt()?

Is it a per interface or a per socket setting?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Oliver Hartkopp]

On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:
> On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:
>> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
>>
>>>
>>> Another thing that IMHO has to be sorted out is the use of module
>>> parameters:
>>>
>>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
>>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
>>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
>>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
>>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");
>>>
>>> Better convert them to netlink options.
>>>
>>
>> What about putting them into socket depended sockopts with setsockopt()?
>
> Is it a per interface or a per socket setting?
>

At least for iso-tp these kind of settings are transport channel 
specific. Don't know if this can be applied to j1939 too.

This is something Kurt or others can answer better than me.

Regards,
Oliver

Re: j1939

[Austin Schuh]

On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
>
>
> On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:
> > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:
> >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> >>
> >>>
> >>> Another thing that IMHO has to be sorted out is the use of module
> >>> parameters:
> >>>
> >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
> >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
> >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
> >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> >>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");
> >>>
> >>> Better convert them to netlink options.
> >>>
> >>
> >> What about putting them into socket depended sockopts with setsockopt()?
> >
> > Is it a per interface or a per socket setting?
> >
>
> At least for iso-tp these kind of settings are transport channel
> specific. Don't know if this can be applied to j1939 too.
>
> This is something Kurt or others can answer better than me.

These parameters should be defined by the standard.  For the padding,
I'm not aware of any messages (besides the RQST message which should
be 3 bytes long) which are not already 8 bytes.  If you want to
enforce that, I would think it would be better to refuse to accept the
packet rather than pad it out to surprise the user less.  The unused
bits should be set to 1, but that's hard to do at the driver level
when you don't know which bits are used and which are unused in a
request.

SAE J1939-21 defines all the timing requirements and other constraints
pretty well.  Is there a reason to make them configurable?

Austin

Re: j1939

[David Jander]

On Mon, 19 Sep 2016 14:37:15 -0700
Austin Schuh <austin@peloton-tech.com> wrote:

> On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >
> >
> >
> > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:  
> > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:  
> > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > >>  
> > >>>
> > >>> Another thing that IMHO has to be sorted out is the use of module
> > >>> parameters:
> > >>>  
> > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");

This is true only for 99% of all J1939 messages. I need to investigate some
more, but I fear this option can better be removed. I couldn't think of a
situation where I'd ponder setting this parameter to true.

> > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");

Usually you'd want this parameter to be 255, except for some special
situations, and I think this should be a per-socket setting.

> > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");

No idea if this limit makes any sense to the kernel driver... for the protocol
it doesn't. For ISOBus applications this limit is much too low. ISOBus VT
clients for example can easily send packets of several times that amount.
IMHO, this parameter should either be removed or set to infinite per default.

> > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> > >>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");  

These two parameters could be a per interface setting I believe.

> > >>>
> > >>> Better convert them to netlink options.
> > >>>  
> > >>
> > >> What about putting them into socket depended sockopts with setsockopt()?  
> > >
> > > Is it a per interface or a per socket setting?
> > >  
> >
> > At least for iso-tp these kind of settings are transport channel
> > specific. Don't know if this can be applied to j1939 too.
> >
> > This is something Kurt or others can answer better than me.  
> 
> These parameters should be defined by the standard.  For the padding,
> I'm not aware of any messages (besides the RQST message which should
> be 3 bytes long) which are not already 8 bytes.  If you want to
> enforce that, I would think it would be better to refuse to accept the
> packet rather than pad it out to surprise the user less.  The unused
> bits should be set to 1, but that's hard to do at the driver level
> when you don't know which bits are used and which are unused in a
> request.
> 
> SAE J1939-21 defines all the timing requirements and other constraints
> pretty well.  Is there a reason to make them configurable?
> 
> Austin

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[David Jander]

On Mon, 19 Sep 2016 18:54:43 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> Hello,
> 
> I've ported Kurt's latest j1939 patches to linux-4.7. I've squashed all
> patches into one. Then a patch to undo unused modifications and a bunch
> of patches to make checkpatch happy.
> 
> I've pushed the result to
> 
> https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> 
> The remaining warning is about a case statement without fall through
> annotations and/or breaks:
> 
> > WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> > #4017: FILE: net/can/j1939/transport.c:1031:
> > +	case tp_cmd_cts:
> > 
> > WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> > #4018: FILE: net/can/j1939/transport.c:1032:
> > +	case 0xff: /* did some data */
> > 
> > WARNING: Possible switch case/default not preceeded by break or fallthrough comment
> > #4019: FILE: net/can/j1939/transport.c:1033:
> > +	case etp_cmd_dpo:  
> 
> The code in question is:
> 
> > /* transmit function */
> > static int j1939tp_txnext(struct session *session)
> > {
> > 	u8 dat[8];
> > 	const u8 *tpdat;
> > 	int ret, offset, pkt_done, pkt_end;
> > 	unsigned int pkt, len, pdelay;
> > 
> > 	memset(dat, 0xff, sizeof(dat));
> > 	get_session(session); /* do not loose it */
> > 
> > 	switch (session->last_cmd) {
> > 	case 0:
> > 		if (!j1939tp_im_transmitter(session->skb))
> > 			break;
> > 		dat[1] = (session->skb->len >> 0) & 0xff;
> > 		dat[2] = (session->skb->len >> 8) & 0xff;
> > 		dat[3] = session->pkt.total;
> > 		if (session->extd) {
> > 			dat[0] = etp_cmd_rts;
> > 			dat[1] = (session->skb->len >> 0) & 0xff;
> > 			dat[2] = (session->skb->len >> 8) & 0xff;
> > 			dat[3] = (session->skb->len >> 16) & 0xff;
> > 			dat[4] = (session->skb->len >> 24) & 0xff;
> > 		} else if (j1939cb_is_broadcast(session->cb)) {
> > 			dat[0] = tp_cmd_bam;
> > 			/* fake cts for broadcast */
> > 			session->pkt.tx = 0;
> > 		} else {
> > 			dat[0] = tp_cmd_rts;
> > 			dat[4] = dat[3];
> > 		}
> > 		if (dat[0] == session->last_txcmd)
> > 			/* done already */
> > 			break;
> > 		ret = j1939tp_tx_ctl(session, 0, dat);
> > 		if (ret < 0)
> > 			goto failed;
> > 		session->last_txcmd = dat[0];
> > 		/* must lock? */
> > 		if (tp_cmd_bam == dat[0])
> > 			j1939tp_schedule_txtimer(session, 50);
> > 		j1939tp_set_rxtimeout(session, 1250);
> > 		break;
> > 	case tp_cmd_rts:
> > 	case etp_cmd_rts:

This fall-through looks correct.

> > 		if (!j1939tp_im_receiver(session->skb))
> > 			break;
> >  tx_cts:
> > 		ret = 0;
> > 		len = session->pkt.total - session->pkt.done;
> > 		len = min(max(len, session->pkt.block), block ?: 255);
> > 
> > 		if (session->extd) {
> > 			pkt = session->pkt.done + 1;
> > 			dat[0] = etp_cmd_cts;
> > 			dat[1] = len;
> > 			dat[2] = (pkt >>  0) & 0xff;
> > 			dat[3] = (pkt >>  8) & 0xff;
> > 			dat[4] = (pkt >> 16) & 0xff;
> > 		} else {
> > 			dat[0] = tp_cmd_cts;
> > 			dat[1] = len;
> > 			dat[2] = session->pkt.done + 1;
> > 		}
> > 		if (dat[0] == session->last_txcmd)
> > 			/* done already */
> > 			break;
> > 		ret = j1939tp_tx_ctl(session, 1, dat);
> > 		if (ret < 0)
> > 			goto failed;
> > 		if (len)
> > 			/* only mark cts done when len is set */
> > 			session->last_txcmd = dat[0];
> > 		j1939tp_set_rxtimeout(session, 1250);
> > 		break;
> > 	case etp_cmd_cts:
> > 		if (j1939tp_im_transmitter(session->skb) && session->extd &&
> > 		    (etp_cmd_dpo != session->last_txcmd)) {
> > 			/* do dpo */
> > 			dat[0] = etp_cmd_dpo;
> > 			session->pkt.dpo = session->pkt.done;
> > 			pkt = session->pkt.dpo;
> > 			dat[1] = session->pkt.last - session->pkt.done;
> > 			dat[2] = (pkt >>  0) & 0xff;
> > 			dat[3] = (pkt >>  8) & 0xff;
> > 			dat[4] = (pkt >> 16) & 0xff;
> > 			ret = j1939tp_tx_ctl(session, 0, dat);
> > 			if (ret < 0)
> > 				goto failed;
> > 			session->last_txcmd = dat[0];
> > 			j1939tp_set_rxtimeout(session, 1250);
> > 			session->pkt.tx = session->pkt.done;
> > 		}
> > 	case tp_cmd_cts:
> > 	case 0xff: /* did some data */
> > 	case etp_cmd_dpo:

Also in this case, a tp_cmd_cts has almost the same function as etp_cmd_dpo,
so this should be ok. Not yet sure how the 0xff thing is supposed to work...

> > 		if ((session->extd || !j1939cb_is_broadcast(session->cb)) &&
> > 		    j1939tp_im_receiver(session->skb)) {
> > 			if (session->pkt.done >= session->pkt.total) {
> > 				if (session->extd) {
> > 					dat[0] = etp_cmd_eof;
> > 					dat[1] = session->skb->len >> 0;
> > 					dat[2] = session->skb->len >> 8;
> > 					dat[3] = session->skb->len >> 16;
> > 					dat[4] = session->skb->len >> 24;
> > 				} else {
> > 					dat[0] = tp_cmd_eof;
> > 					dat[1] = session->skb->len;
> > 					dat[2] = session->skb->len >> 8;
> > 					dat[3] = session->pkt.total;
> > 				}
> > 				if (dat[0] == session->last_txcmd)
> > 					/* done already */
> > 					break;
> > 				ret = j1939tp_tx_ctl(session, 1, dat);
> > 				if (ret < 0)
> > 					goto failed;
> > 				session->last_txcmd = dat[0];
> > 				j1939tp_set_rxtimeout(session, 1250);
> > 				/* wait for the EOF packet to come in */
> > 				break;
> > 			} else if (session->pkt.done >= session->pkt.last) {
> > 				session->last_txcmd = 0;
> > 				goto tx_cts;
> > 			}
> > 		}

Here we could better have break to be clear I guess. No code paths get to this
point AFAICS, but clarity is always better IMHO.

> > 	case tp_cmd_bam:
> > 		if (!j1939tp_im_transmitter(session->skb))
> > 			break;
> > 		tpdat = session->skb->data;
> > 		ret = 0;
> > 		pkt_done = 0;
> > 		pkt_end = (!session->extd && j1939cb_is_broadcast(session->cb))
> > 			? session->pkt.total : session->pkt.last;
> > 
> > 		while (session->pkt.tx < pkt_end) {
> > 			dat[0] = session->pkt.tx - session->pkt.dpo + 1;
> > 			offset = session->pkt.tx * 7;
> > 			len = session->skb->len - offset;
> > 			if (len > 7)
> > 				len = 7;
> > 			memcpy(&dat[1], &tpdat[offset], len);
> > 			ret = j1939tp_tx_dat(session->skb, session->extd,
> > 					     dat, len + 1);
> > 			if (ret < 0)
> > 				break;
> > 			session->last_txcmd = 0xff;
> > 			++pkt_done;
> > 			++session->pkt.tx;
> > 			pdelay = j1939cb_is_broadcast(session->cb) ?  50 :
> > 				packet_delay;
> > 			if ((session->pkt.tx < session->pkt.total) && pdelay) {
> > 				j1939tp_schedule_txtimer(session, pdelay);
> > 				break;
> > 			}
> > 		}
> > 		if (pkt_done)
> > 			j1939tp_set_rxtimeout(session, 250);
> > 		if (ret)
> > 			goto failed;
> > 		break;

Does coding-style suggest a default:break; here?

> > 	}
> > 	put_session(session);
> > 	return 0;
> >  failed:
> > 	put_session(session);
> > 	return ret;
> > }  
> 
> Is here someone with insight and can tell if the code is correct this way?

Looks correct to me. I have yet to test it though.

> Another thing that IMHO has to be sorted out is the use of module
> parameters:
> 
> > ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
> > ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
> > ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
> > ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> > ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");  
> 
> Better convert them to netlink options.
> 
> Next think I'll do is to establish some communication with an external
> j1939 component. I'll keep you updated.

Thanks!

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7421 bytes --]

On 09/20/2016 09:22 AM, David Jander wrote:
>> Is here someone with insight and can tell if the code is correct this way?
> Looks correct to me. I have yet to test it though.

Thanks for your input, the diff to the previous version is this:

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 56bc5bf36090..5d878b5aaf0a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -979,7 +979,7 @@ static int j1939tp_txnext(struct session *session)
>                 j1939tp_set_rxtimeout(session, 1250);
>                 break;
>         case tp_cmd_rts:
> -       case etp_cmd_rts:
> +       case etp_cmd_rts: /* fallthrough */
>                 if (!j1939tp_im_receiver(session->skb))
>                         break;
>   tx_cts:
> @@ -1028,9 +1028,10 @@ static int j1939tp_txnext(struct session *session)
>                         j1939tp_set_rxtimeout(session, 1250);
>                         session->pkt.tx = session->pkt.done;
>                 }
> -       case tp_cmd_cts:
> -       case 0xff: /* did some data */
> -       case etp_cmd_dpo:
> +               /* fallthrough */
> +       case tp_cmd_cts: /* fallthrough */
> +       case 0xff: /* did some data */                  /* FIXME: let David Jander recheck this */
> +       case etp_cmd_dpo: /* fallthrough */
>                 if ((session->extd || !j1939cb_is_broadcast(session->cb)) &&
>                     j1939tp_im_receiver(session->skb)) {
>                         if (session->pkt.done >= session->pkt.total) {
> @@ -1061,6 +1062,7 @@ static int j1939tp_txnext(struct session *session)
>                                 goto tx_cts;
>                         }
>                 }
> +               break;
>         case tp_cmd_bam:
>                 if (!j1939tp_im_transmitter(session->skb))
>                         break;

For completeness, the whole function:

> /* transmit function */
> static int j1939tp_txnext(struct session *session)
> {
> 	u8 dat[8];
> 	const u8 *tpdat;
> 	int ret, offset, pkt_done, pkt_end;
> 	unsigned int pkt, len, pdelay;
> 
> 	memset(dat, 0xff, sizeof(dat));
> 	get_session(session); /* do not loose it */
> 
> 	switch (session->last_cmd) {
> 	case 0:
> 		if (!j1939tp_im_transmitter(session->skb))
> 			break;
> 		dat[1] = (session->skb->len >> 0) & 0xff;
> 		dat[2] = (session->skb->len >> 8) & 0xff;
> 		dat[3] = session->pkt.total;
> 		if (session->extd) {
> 			dat[0] = etp_cmd_rts;
> 			dat[1] = (session->skb->len >> 0) & 0xff;
> 			dat[2] = (session->skb->len >> 8) & 0xff;
> 			dat[3] = (session->skb->len >> 16) & 0xff;
> 			dat[4] = (session->skb->len >> 24) & 0xff;
> 		} else if (j1939cb_is_broadcast(session->cb)) {
> 			dat[0] = tp_cmd_bam;
> 			/* fake cts for broadcast */
> 			session->pkt.tx = 0;
> 		} else {
> 			dat[0] = tp_cmd_rts;
> 			dat[4] = dat[3];
> 		}
> 		if (dat[0] == session->last_txcmd)
> 			/* done already */
> 			break;
> 		ret = j1939tp_tx_ctl(session, 0, dat);
> 		if (ret < 0)
> 			goto failed;
> 		session->last_txcmd = dat[0];
> 		/* must lock? */
> 		if (tp_cmd_bam == dat[0])
> 			j1939tp_schedule_txtimer(session, 50);
> 		j1939tp_set_rxtimeout(session, 1250);
> 		break;
> 	case tp_cmd_rts:
> 	case etp_cmd_rts: /* fallthrough */
> 		if (!j1939tp_im_receiver(session->skb))
> 			break;
>  tx_cts:
> 		ret = 0;
> 		len = session->pkt.total - session->pkt.done;
> 		len = min(max(len, session->pkt.block), block ?: 255);
> 
> 		if (session->extd) {
> 			pkt = session->pkt.done + 1;
> 			dat[0] = etp_cmd_cts;
> 			dat[1] = len;
> 			dat[2] = (pkt >>  0) & 0xff;
> 			dat[3] = (pkt >>  8) & 0xff;
> 			dat[4] = (pkt >> 16) & 0xff;
> 		} else {
> 			dat[0] = tp_cmd_cts;
> 			dat[1] = len;
> 			dat[2] = session->pkt.done + 1;
> 		}
> 		if (dat[0] == session->last_txcmd)
> 			/* done already */
> 			break;
> 		ret = j1939tp_tx_ctl(session, 1, dat);
> 		if (ret < 0)
> 			goto failed;
> 		if (len)
> 			/* only mark cts done when len is set */
> 			session->last_txcmd = dat[0];
> 		j1939tp_set_rxtimeout(session, 1250);
> 		break;
> 	case etp_cmd_cts:
> 		if (j1939tp_im_transmitter(session->skb) && session->extd &&
> 		    (etp_cmd_dpo != session->last_txcmd)) {
> 			/* do dpo */
> 			dat[0] = etp_cmd_dpo;
> 			session->pkt.dpo = session->pkt.done;
> 			pkt = session->pkt.dpo;
> 			dat[1] = session->pkt.last - session->pkt.done;
> 			dat[2] = (pkt >>  0) & 0xff;
> 			dat[3] = (pkt >>  8) & 0xff;
> 			dat[4] = (pkt >> 16) & 0xff;
> 			ret = j1939tp_tx_ctl(session, 0, dat);
> 			if (ret < 0)
> 				goto failed;
> 			session->last_txcmd = dat[0];
> 			j1939tp_set_rxtimeout(session, 1250);
> 			session->pkt.tx = session->pkt.done;
> 		}
> 		/* fallthrough */
> 	case tp_cmd_cts: /* fallthrough */
> 	case 0xff: /* did some data */			/* FIXME: let David Jander recheck this */
> 	case etp_cmd_dpo: /* fallthrough */
> 		if ((session->extd || !j1939cb_is_broadcast(session->cb)) &&
> 		    j1939tp_im_receiver(session->skb)) {
> 			if (session->pkt.done >= session->pkt.total) {
> 				if (session->extd) {
> 					dat[0] = etp_cmd_eof;
> 					dat[1] = session->skb->len >> 0;
> 					dat[2] = session->skb->len >> 8;
> 					dat[3] = session->skb->len >> 16;
> 					dat[4] = session->skb->len >> 24;
> 				} else {
> 					dat[0] = tp_cmd_eof;
> 					dat[1] = session->skb->len;
> 					dat[2] = session->skb->len >> 8;
> 					dat[3] = session->pkt.total;
> 				}
> 				if (dat[0] == session->last_txcmd)
> 					/* done already */
> 					break;
> 				ret = j1939tp_tx_ctl(session, 1, dat);
> 				if (ret < 0)
> 					goto failed;
> 				session->last_txcmd = dat[0];
> 				j1939tp_set_rxtimeout(session, 1250);
> 				/* wait for the EOF packet to come in */
> 				break;
> 			} else if (session->pkt.done >= session->pkt.last) {
> 				session->last_txcmd = 0;
> 				goto tx_cts;
> 			}
> 		}
> 		break;
> 	case tp_cmd_bam:
> 		if (!j1939tp_im_transmitter(session->skb))
> 			break;
> 		tpdat = session->skb->data;
> 		ret = 0;
> 		pkt_done = 0;
> 		pkt_end = (!session->extd && j1939cb_is_broadcast(session->cb))
> 			? session->pkt.total : session->pkt.last;
> 
> 		while (session->pkt.tx < pkt_end) {
> 			dat[0] = session->pkt.tx - session->pkt.dpo + 1;
> 			offset = session->pkt.tx * 7;
> 			len = session->skb->len - offset;
> 			if (len > 7)
> 				len = 7;
> 			memcpy(&dat[1], &tpdat[offset], len);
> 			ret = j1939tp_tx_dat(session->skb, session->extd,
> 					     dat, len + 1);
> 			if (ret < 0)
> 				break;
> 			session->last_txcmd = 0xff;
> 			++pkt_done;
> 			++session->pkt.tx;
> 			pdelay = j1939cb_is_broadcast(session->cb) ?  50 :
> 				packet_delay;
> 			if ((session->pkt.tx < session->pkt.total) && pdelay) {
> 				j1939tp_schedule_txtimer(session, pdelay);
> 				break;
> 			}
> 		}
> 		if (pkt_done)
> 			j1939tp_set_rxtimeout(session, 250);
> 		if (ret)
> 			goto failed;
> 		break;
> 	}
> 	put_session(session);
> 	return 0;
>  failed:
> 	put_session(session);
> 	return ret;
> }

Thanks,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1520 bytes --]

On 09/19/2016 11:37 PM, Austin Schuh wrote:
>>>> What about putting them into socket depended sockopts with setsockopt()?
>>>
>>> Is it a per interface or a per socket setting?
>>
>> At least for iso-tp these kind of settings are transport channel
>> specific. Don't know if this can be applied to j1939 too.
>>
>> This is something Kurt or others can answer better than me.
> 
> These parameters should be defined by the standard.  For the padding,
> I'm not aware of any messages (besides the RQST message which should
> be 3 bytes long) which are not already 8 bytes.  If you want to
> enforce that, I would think it would be better to refuse to accept the
> packet rather than pad it out to surprise the user less.

So an option is to check if PGN is 0xea00 then length must be 3,
otherwise length must be 8?

> The unused bits should be set to 1, but that's hard to do at the
> driver level when you don't know which bits are used and which are
> unused in a request.

The padding is done in the j1939_send() function. So padding is no
problem here.

> SAE J1939-21 defines all the timing requirements and other constraints
> pretty well.  Is there a reason to make them configurable?

Dunno...Kurt?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

Hey ho,

Please note my @eia.be address is obsolete now.
I should investigate why the kbuild robot send mail to that address.
this (dev.kurt@...) is meant to stay :-)

Ans I was very occupied the due to a move.
I'm glad there's momentum on the J1939 side, a bit sad it happened just
when I was a bit off.

--- Original message ---
> Date:   Mon, 19 Sep 2016 14:37:15 -0700
> From: Austin Schuh <austin@peloton-tech.com>
> To: Oliver Hartkopp <socketcan@hartkopp.net>, Marc Kleine-Budde
>  <mkl@pengutronix.de>, linux-can <linux-can@vger.kernel.org>
> Cc: David Jander <david@protonic.nl>, Kurt Van Dijck <kurt.van.dijck@eia.be>
> Subject: Re: j1939
> 
> On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >
> >
> >
> > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:
> > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:
> > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > >>
> > >>>
> > >>> Another thing that IMHO has to be sorted out is the use of module
> > >>> parameters:
> > >>>
> > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
> > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
> > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
> > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> > >>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");
> > >>>
> > >>> Better convert them to netlink options.
> > >>>
> > >>
> > >> What about putting them into socket depended sockopts with setsockopt()?
> > >
> > > Is it a per interface or a per socket setting?
> > >
> >
> > At least for iso-tp these kind of settings are transport channel
> > specific. Don't know if this can be applied to j1939 too.
> >
> > This is something Kurt or others can answer better than me.
> 
> These parameters should be defined by the standard.  For the padding,
> I'm not aware of any messages (besides the RQST message which should
> be 3 bytes long) which are not already 8 bytes.  If you want to
> enforce that, I would think it would be better to refuse to accept the
> packet rather than pad it out to surprise the user less.  The unused
> bits should be set to 1, but that's hard to do at the driver level
> when you don't know which bits are used and which are unused in a
> request.
> 
> SAE J1939-21 defines all the timing requirements and other constraints
> pretty well.  Is there a reason to make them configurable?

The timing parameters are defined pretty well, but sometimes
light-weight ECU's cannot handle what's defined by the standard.
Therefore, a sort of rate-limiting may be defined.

The 8-byte sutffing also is particular. Do you need to send 7 unused
bytes? The standard says yes, but what happens if you don't.
So I put module parameters there.

Oliver idea is what I was heading to, making a lot of those module
paramters also a socket option, with the module parameters still being
the default. So, the module parameters do not disappear.

I'm still catching up, I agree with David Jander's comments.

Re: j1939

[Kurt Van Dijck]

--- Original message ---
> Date:   Tue, 20 Sep 2016 09:15:25 +0200
> From: David Jander <david@protonic.nl>
> To: Austin Schuh <austin@peloton-tech.com>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>, Marc Kleine-Budde
>  <mkl@pengutronix.de>, linux-can <linux-can@vger.kernel.org>, Kurt Van
>  Dijck <kurt.van.dijck@eia.be>
> Subject: Re: j1939
> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu)
> 
> On Mon, 19 Sep 2016 14:37:15 -0700
> Austin Schuh <austin@peloton-tech.com> wrote:
> 
> > On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > >
> > >
> > >
> > > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:  
> > > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:  
> > > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > > >>  
> > > >>>
> > > >>> Another thing that IMHO has to be sorted out is the use of module
> > > >>> parameters:
> > > >>>  
> > > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");
> 
> This is true only for 99% of all J1939 messages. I need to investigate some
> more, but I fear this option can better be removed. I couldn't think of a
> situation where I'd ponder setting this parameter to true.

There exist engine controller software that adhere to this for the
transport protocol messages. This means, I implemented the transport
protocol to not necessarily use all 8 bytes at the end.

> 
> > > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number of packets to send in burst between flow control (1..255, default 255)");
> 
> Usually you'd want this parameter to be 255, except for some special
> situations, and I think this should be a per-socket setting.

ack. This should also become a per-socket setting.
> 
> > > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum packet size (default 100k)");
> 
> No idea if this limit makes any sense to the kernel driver... for the protocol
> it doesn't. For ISOBus applications this limit is much too low. ISOBus VT
> clients for example can easily send packets of several times that amount.
> IMHO, this parameter should either be removed or set to infinite per default.

The default is questionable :-)
On the other hand, on a low-memory device, it may make no sense to
allocate buffers for large packets that are not intended for the host
anyway. To avoid out-of-memory conditions triggered by multiple j1939
transport sessions, I added this parameter to protect against such
attack.
> 
> > > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet retransmission timeout in msecs, used in case of buffer full. (default 20)");
> > > >>>> ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay, "Delay between packets to avoid buffer overruns (default 0)");  
> 
> These two parameters could be a per interface setting I believe.

as mentioned earlier, the parameters may serve as a system-wide default,
and may be overruled per socket even (it about the TX path).
> 
> > > >>>
> > > >>> Better convert them to netlink options.

I just dropped netlink?
and I'd first import j1939 in the kernel before going to netlink again.
having iproute2 out-of-tree patches is not handy.

Kurt

Re: j1939

[Kurt Van Dijck]

> 
> On 09/19/2016 11:37 PM, Austin Schuh wrote:
> >>>> What about putting them into socket depended sockopts with setsockopt()?
> >>>
> >>> Is it a per interface or a per socket setting?
> >>
> >> At least for iso-tp these kind of settings are transport channel
> >> specific. Don't know if this can be applied to j1939 too.
> >>
> >> This is something Kurt or others can answer better than me.
> > 
> > These parameters should be defined by the standard.  For the padding,
> > I'm not aware of any messages (besides the RQST message which should
> > be 3 bytes long) which are not already 8 bytes.  If you want to
> > enforce that, I would think it would be better to refuse to accept the
> > packet rather than pad it out to surprise the user less.
> 
> So an option is to check if PGN is 0xea00 then length must be 3,
> otherwise length must be 8?
> 
> > The unused bits should be set to 1, but that's hard to do at the
> > driver level when you don't know which bits are used and which are
> > unused in a request.
> 
> The padding is done in the j1939_send() function. So padding is no
> problem here.
> 
> > SAE J1939-21 defines all the timing requirements and other constraints
> > pretty well.  Is there a reason to make them configurable?
> 
> Dunno...Kurt?

As said in a previous email, to support different environments.
The timings are defined in the standard, but in a 'receive' way, i.e.
the standard defines what timing is acceptable.
For the TX path, you may prefer to send ASAP (most of the times) or to
relax the timing so the opposite side can keep up. This preference does
not violate the standard.

> 
> Marc
> 
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 

Re: j1939

[David Jander]

On Tue, 4 Oct 2016 09:37:03 +0200
Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:

> --- Original message ---
> > Date:   Tue, 20 Sep 2016 09:15:25 +0200
> > From: David Jander <david@protonic.nl>
> > To: Austin Schuh <austin@peloton-tech.com>
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>, Marc Kleine-Budde
> >  <mkl@pengutronix.de>, linux-can <linux-can@vger.kernel.org>, Kurt Van
> >  Dijck <kurt.van.dijck@eia.be>
> > Subject: Re: j1939
> > X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu)
> > 
> > On Mon, 19 Sep 2016 14:37:15 -0700
> > Austin Schuh <austin@peloton-tech.com> wrote:
> >   
> > > On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:  
> > > >
> > > >
> > > >
> > > > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:    
> > > > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:    
> > > > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > > > >>    
> > > > >>>
> > > > >>> Another thing that IMHO has to be sorted out is the use of module
> > > > >>> parameters:
> > > > >>>    
> > > > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");  
> > 
> > This is true only for 99% of all J1939 messages. I need to investigate some
> > more, but I fear this option can better be removed. I couldn't think of a
> > situation where I'd ponder setting this parameter to true.  
> 
> There exist engine controller software that adhere to this for the
> transport protocol messages. This means, I implemented the transport
> protocol to not necessarily use all 8 bytes at the end.

Do you mean this only applies to TP messages?
In that case, the option may make sense, although according to the standard,
all TP messages should always be 8-bytes long, so when is it desirable to
have non-standard TP-messages? Is this ECU software you mention not
standards-compliant? Does it bark on standards-compliant TP messages?

> > > > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number
> > > > >>>> of packets to send in burst between flow control (1..255, default
> > > > >>>> 255)");  
> > 
> > Usually you'd want this parameter to be 255, except for some special
> > situations, and I think this should be a per-socket setting.  
> 
> ack. This should also become a per-socket setting.
> >   
> > > > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum
> > > > >>>> packet size (default 100k)");  
> > 
> > No idea if this limit makes any sense to the kernel driver... for the
> > protocol it doesn't. For ISOBus applications this limit is much too low.
> > ISOBus VT clients for example can easily send packets of several times
> > that amount. IMHO, this parameter should either be removed or set to
> > infinite per default.  
> 
> The default is questionable :-)
> On the other hand, on a low-memory device, it may make no sense to
> allocate buffers for large packets that are not intended for the host
> anyway. To avoid out-of-memory conditions triggered by multiple j1939
> transport sessions, I added this parameter to protect against such
> attack.

Ok, I understand your point. I still feel a bit uncomfortable with having a
default limit that, if the user is not aware of it, can cause a lot of
confusion. Things can suddenly and seemingly randomly not work as expected,
and IMHO this is never good. What can we do about this?

> > > > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet
> > > > >>>> retransmission timeout in msecs, used in case of buffer full.
> > > > >>>> (default
> > > > >>>> 20)"); ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay,
> > > > >>>> "Delay between packets to avoid buffer overruns (default 0)");    
> > 
> > These two parameters could be a per interface setting I believe.  
> 
> as mentioned earlier, the parameters may serve as a system-wide default,
> and may be overruled per socket even (it about the TX path).

System-wide would mean per-interface setting (ip link set...?), but overruled
per socket would imply setsockopt... this seems a bit contradictory to me.
Shouldn't it be either one _or_ the other?

> > > > >>>
> > > > >>> Better convert them to netlink options.  
> 
> I just dropped netlink?
> and I'd first import j1939 in the kernel before going to netlink again.
> having iproute2 out-of-tree patches is not handy.

I agree here.

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[Kurt Van Dijck]

> 
> On Tue, 4 Oct 2016 09:37:03 +0200
> Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
> 
> > --- Original message ---
> > > Date:   Tue, 20 Sep 2016 09:15:25 +0200
> > > From: David Jander <david@protonic.nl>
> > > To: Austin Schuh <austin@peloton-tech.com>
> > > Cc: Oliver Hartkopp <socketcan@hartkopp.net>, Marc Kleine-Budde
> > >  <mkl@pengutronix.de>, linux-can <linux-can@vger.kernel.org>, Kurt Van
> > >  Dijck <kurt.van.dijck@eia.be>
> > > Subject: Re: j1939
> > > X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu)
> > > 
> > > On Mon, 19 Sep 2016 14:37:15 -0700
> > > Austin Schuh <austin@peloton-tech.com> wrote:
> > >   
> > > > On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:  
> > > > >
> > > > >
> > > > >
> > > > > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:    
> > > > > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:    
> > > > > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > > > > >>    
> > > > > >>>
> > > > > >>> Another thing that IMHO has to be sorted out is the use of module
> > > > > >>> parameters:
> > > > > >>>    
> > > > > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");  
> > > 
> > > This is true only for 99% of all J1939 messages. I need to investigate some
> > > more, but I fear this option can better be removed. I couldn't think of a
> > > situation where I'd ponder setting this parameter to true.  
> > 
> > There exist engine controller software that adhere to this for the
> > transport protocol messages. This means, I implemented the transport
> > protocol to not necessarily use all 8 bytes at the end.
> 
> Do you mean this only applies to TP messages?
> In that case, the option may make sense, although according to the standard,
> all TP messages should always be 8-bytes long, so when is it desirable to
> have non-standard TP-messages? Is this ECU software you mention not
> standards-compliant? Does it bark on standards-compliant TP messages?

From what I remember (I have no legal access to the specs anymore),
J1939 defines _all_ pgns but 0xea00 to be 8 bytes long.
But often, especially for proprietary messages, not all 8 bytes are
used, and the DLC is cut accordingly.
And less often, this is done for TP too.
I also did that for as long as I can remember.
As long as all defined bytes are there, this should not really be a
problem.

Recently, someone on this list was confronted with an engine ECU that
dropped TP that had the last packet not 8 bytes long.
And I believe I've encountered an issue a long time ago where this
happened on all PGNs.
So I introduced this option, having 3 possibilities:
* no padding
* padding for TP
* padding for all PGN.

> 
> > > > > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number
> > > > > >>>> of packets to send in burst between flow control (1..255, default
> > > > > >>>> 255)");  
> > > 
> > > Usually you'd want this parameter to be 255, except for some special
> > > situations, and I think this should be a per-socket setting.  
> > 
> > ack. This should also become a per-socket setting.
> > >   
> > > > > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum
> > > > > >>>> packet size (default 100k)");  
> > > 
> > > No idea if this limit makes any sense to the kernel driver... for the
> > > protocol it doesn't. For ISOBus applications this limit is much too low.
> > > ISOBus VT clients for example can easily send packets of several times
> > > that amount. IMHO, this parameter should either be removed or set to
> > > infinite per default.  
> > 
> > The default is questionable :-)
> > On the other hand, on a low-memory device, it may make no sense to
> > allocate buffers for large packets that are not intended for the host
> > anyway. To avoid out-of-memory conditions triggered by multiple j1939
> > transport sessions, I added this parameter to protect against such
> > attack.
> 
> Ok, I understand your point. I still feel a bit uncomfortable with having a
> default limit that, if the user is not aware of it, can cause a lot of
> confusion. Things can suddenly and seemingly randomly not work as expected,
> and IMHO this is never good. What can we do about this?

I have no problems modifying the default.
You could even move this problem to Kconfig: make a Kconfig entry
with the (default) max packet size.

Your point is valid, but positioned with isobus in mind.
ISOBus VT clients are the exception that forced isobus to cross the 1785
byte limit.
For a significant amount of people, 1785 is what they expect to be the limit :-)

On the other hand, it is configurable, and it should not cause hard
discussions. If one has something specific in mind, he/she can always
apply it as part of the boot sequence?

> 
> > > > > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time, "Packet
> > > > > >>>> retransmission timeout in msecs, used in case of buffer full.
> > > > > >>>> (default
> > > > > >>>> 20)"); ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay,
> > > > > >>>> "Delay between packets to avoid buffer overruns (default 0)");    
> > > 
> > > These two parameters could be a per interface setting I believe.  
> > 
> > as mentioned earlier, the parameters may serve as a system-wide default,
> > and may be overruled per socket even (it about the TX path).
> 
> System-wide would mean per-interface setting (ip link set...?), but overruled
> per socket would imply setsockopt... this seems a bit contradictory to me.
> Shouldn't it be either one _or_ the other?

I believe it should eventually be a per-socket option.
On the other hand, having it system-wide allows it to be set without
per-application modification.
I don't see a real per-interface need, due to the per-socket goal.

The contradictory nature is eliminated due to module parameters.
/proc/... settings should be evaluated later on, but modules parameters
should not necessarily? Once you set the (e.g. packet retransmission
timeout) on your socket, the system-wide module parameter does not apply
for your socket anymore. Seems reasonable to me.

> 
> > > > > >>>
> > > > > >>> Better convert them to netlink options.  
> > 
> > I just dropped netlink?
> > and I'd first import j1939 in the kernel before going to netlink again.
> > having iproute2 out-of-tree patches is not handy.
> 
> I agree here.
> 
> Best regards,
> 
> -- 
> David Jander
> Protonic Holland.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: j1939

[David Jander]

On Tue, 4 Oct 2016 15:57:01 +0200
Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:

> > 
> > On Tue, 4 Oct 2016 09:37:03 +0200
> > Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> wrote:
> >   
> > > --- Original message ---  
> > > > Date:   Tue, 20 Sep 2016 09:15:25 +0200
> > > > From: David Jander <david@protonic.nl>
> > > > To: Austin Schuh <austin@peloton-tech.com>
> > > > Cc: Oliver Hartkopp <socketcan@hartkopp.net>, Marc Kleine-Budde
> > > >  <mkl@pengutronix.de>, linux-can <linux-can@vger.kernel.org>, Kurt Van
> > > >  Dijck <kurt.van.dijck@eia.be>
> > > > Subject: Re: j1939
> > > > X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu)
> > > > 
> > > > On Mon, 19 Sep 2016 14:37:15 -0700
> > > > Austin Schuh <austin@peloton-tech.com> wrote:
> > > >     
> > > > > On Mon, Sep 19, 2016 at 12:45 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:    
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 09/19/2016 08:52 PM, Marc Kleine-Budde wrote:      
> > > > > > > On 09/19/2016 08:27 PM, Oliver Hartkopp wrote:      
> > > > > > >> On 09/19/2016 06:54 PM, Marc Kleine-Budde wrote:
> > > > > > >>      
> > > > > > >>>
> > > > > > >>> Another thing that IMHO has to be sorted out is the use of module
> > > > > > >>> parameters:
> > > > > > >>>      
> > > > > > >>>> ./main.c:49:MODULE_PARM_DESC(padding, "Pad all packets to 8 bytes, and stuff with 0xff");    
> > > > 
> > > > This is true only for 99% of all J1939 messages. I need to investigate some
> > > > more, but I fear this option can better be removed. I couldn't think of a
> > > > situation where I'd ponder setting this parameter to true.    
> > > 
> > > There exist engine controller software that adhere to this for the
> > > transport protocol messages. This means, I implemented the transport
> > > protocol to not necessarily use all 8 bytes at the end.  
> > 
> > Do you mean this only applies to TP messages?
> > In that case, the option may make sense, although according to the standard,
> > all TP messages should always be 8-bytes long, so when is it desirable to
> > have non-standard TP-messages? Is this ECU software you mention not
> > standards-compliant? Does it bark on standards-compliant TP messages?  
> 
> From what I remember (I have no legal access to the specs anymore),
> J1939 defines _all_ pgns but 0xea00 to be 8 bytes long.

I do have legal access to ISOBus (ISO-11783), which is supposedly a superset
of J1939.

> But often, especially for proprietary messages, not all 8 bytes are
> used, and the DLC is cut accordingly.
> And less often, this is done for TP too.
> I also did that for as long as I can remember.
> As long as all defined bytes are there, this should not really be a
> problem.
> 
> Recently, someone on this list was confronted with an engine ECU that
> dropped TP that had the last packet not 8 bytes long.
> And I believe I've encountered an issue a long time ago where this
> happened on all PGNs.
> So I introduced this option, having 3 possibilities:
> * no padding
> * padding for TP
> * padding for all PGN.

I have yet to check, but I fear that in ISOBus conformance tests on some
places at least this is tested for. I'd say that if the standard says "pad and
fill with 0xff", any ECU should do this and not doing it should be regarded as
a bug... 
I know that PGN 0xEA00 is an exception, and proprietary messages should be
permitted any length IMHO, so padding every frame (even coming from user-space)
seems unnecessary and unacceptable to me, as it clearly violates the standard.
Turning off padding for TP-messages might be a nice-to-have feature, but it
would not be standards-compliant anymore AFAICS.

I guess I don't violate anyone's copyright if I cite a small fragment from
ISO-11783-3 here:

In "Transport Protocol — Data Transfer messages (TP.DT)":
  [...]
  "The last packet of a multi-packet parameter group can require less than
  8B. The extra bytes shall be filled with 0xFF"

If someone can confirm this is the same language as in J1939, I'd say we don't
really need to support non-padded (E)TP-packets. Please note that I am only
referring to (E)TP-packets here!

> > > > > > >>>> ./transport.c:60:MODULE_PARM_DESC(transport_burst_count, "Number
> > > > > > >>>> of packets to send in burst between flow control (1..255, default
> > > > > > >>>> 255)");    
> > > > 
> > > > Usually you'd want this parameter to be 255, except for some special
> > > > situations, and I think this should be a per-socket setting.    
> > > 
> > > ack. This should also become a per-socket setting.  
> > > >     
> > > > > > >>>> ./transport.c:61:MODULE_PARM_DESC(transport_max_size, "Maximum
> > > > > > >>>> packet size (default 100k)");    
> > > > 
> > > > No idea if this limit makes any sense to the kernel driver... for the
> > > > protocol it doesn't. For ISOBus applications this limit is much too low.
> > > > ISOBus VT clients for example can easily send packets of several times
> > > > that amount. IMHO, this parameter should either be removed or set to
> > > > infinite per default.    
> > > 
> > > The default is questionable :-)
> > > On the other hand, on a low-memory device, it may make no sense to
> > > allocate buffers for large packets that are not intended for the host
> > > anyway. To avoid out-of-memory conditions triggered by multiple j1939
> > > transport sessions, I added this parameter to protect against such
> > > attack.  
> > 
> > Ok, I understand your point. I still feel a bit uncomfortable with having a
> > default limit that, if the user is not aware of it, can cause a lot of
> > confusion. Things can suddenly and seemingly randomly not work as expected,
> > and IMHO this is never good. What can we do about this?  
> 
> I have no problems modifying the default.
> You could even move this problem to Kconfig: make a Kconfig entry
> with the (default) max packet size.

Hmm... I could live with that, but I fear that the general consensus is to not
put these kind of "tunables" in KConfig anymore... Can someone confirm this?

> Your point is valid, but positioned with isobus in mind.
> ISOBus VT clients are the exception that forced isobus to cross the 1785
> byte limit.
> For a significant amount of people, 1785 is what they expect to be the
> limit :-)

Yes, that's what Extended-TP sessions are for...
What about firmware-upgrades and such then? Don't those also use ETP?

> On the other hand, it is configurable, and it should not cause hard
> discussions. If one has something specific in mind, he/she can always
> apply it as part of the boot sequence?

You are right in this sense, I am only trying to avoid potential pitfalls for
users that may not be aware this limit exists. I also think this limit should
exist, but I am at a loss about how to implement it in a sensible way.

> > > > > > >>>> ./transport.c:62:MODULE_PARM_DESC(transport_retry_time,
> > > > > > >>>> "Packet retransmission timeout in msecs, used in case of
> > > > > > >>>> buffer full. (default
> > > > > > >>>> 20)"); ./transport.c:63:MODULE_PARM_DESC(transport_packet_delay,
> > > > > > >>>> "Delay between packets to avoid buffer overruns (default
> > > > > > >>>> 0)");      
> > > > 
> > > > These two parameters could be a per interface setting I believe.    
> > > 
> > > as mentioned earlier, the parameters may serve as a system-wide default,
> > > and may be overruled per socket even (it about the TX path).  
> > 
> > System-wide would mean per-interface setting (ip link set...?), but
> > overruled per socket would imply setsockopt... this seems a bit
> > contradictory to me. Shouldn't it be either one _or_ the other?  
> 
> I believe it should eventually be a per-socket option.
> On the other hand, having it system-wide allows it to be set without
> per-application modification.
> I don't see a real per-interface need, due to the per-socket goal.

I can agree to per-socket configuration. As for existing software, I'd say
that we should assume that no prior software does exist until this code hits
mainline, right?
In other words, all prior software should assume the API will change as long
as this code is not in mainline.

> The contradictory nature is eliminated due to module parameters.
> /proc/... settings should be evaluated later on, but modules parameters
> should not necessarily? Once you set the (e.g. packet retransmission
> timeout) on your socket, the system-wide module parameter does not apply
> for your socket anymore. Seems reasonable to me.

Then IMHO, the system-wide setting should not really be a setting, but rather
just a default. It seems confusing to me to have two different sources for a
single parameter value.

> > > > > > >>>
> > > > > > >>> Better convert them to netlink options.    
> > > 
> > > I just dropped netlink?
> > > and I'd first import j1939 in the kernel before going to netlink again.
> > > having iproute2 out-of-tree patches is not handy.  
> > 
> > I agree here.
> > 
> > Best regards,
> > 

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1617 bytes --]

On 10/04/2016 09:28 AM, Kurt Van Dijck wrote:
> Please note my @eia.be address is obsolete now.
> I should investigate why the kbuild robot send mail to that address.
> this (dev.kurt@...) is meant to stay :-)

Maybe it was due to the Author in the git commits. I can (technically)
change the address, don't know about the legal aspects of this, though.

[...]

>> SAE J1939-21 defines all the timing requirements and other constraints
>> pretty well.  Is there a reason to make them configurable?
> 
> The timing parameters are defined pretty well, but sometimes
> light-weight ECU's cannot handle what's defined by the standard.
> Therefore, a sort of rate-limiting may be defined.
> 
> The 8-byte sutffing also is particular. Do you need to send 7 unused
> bytes? The standard says yes, but what happens if you don't.
> So I put module parameters there.
> 
> Oliver idea is what I was heading to, making a lot of those module
> paramters also a socket option, with the module parameters still being
> the default. So, the module parameters do not disappear.

I've a bad feeling about introducing new module parameters to the
kernel. There are ways to configure them properly. Being it a per
interface (netlink) or a per socket option.

> I'm still catching up, I agree with David Jander's comments.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Austin Schuh]

On Tue, Oct 4, 2016 at 7:47 AM David Jander <david@protonic.nl> wrote:
>
> I have yet to check, but I fear that in ISOBus conformance tests on some
> places at least this is tested for. I'd say that if the standard says "pad and
> fill with 0xff", any ECU should do this and not doing it should be regarded as
> a bug...
> I know that PGN 0xEA00 is an exception, and proprietary messages should be
> permitted any length IMHO, so padding every frame (even coming from user-space)
> seems unnecessary and unacceptable to me, as it clearly violates the standard.
> Turning off padding for TP-messages might be a nice-to-have feature, but it
> would not be standards-compliant anymore AFAICS.
>
> I guess I don't violate anyone's copyright if I cite a small fragment from
> ISO-11783-3 here:
>
> In "Transport Protocol — Data Transfer messages (TP.DT)":
>   [...]
>   "The last packet of a multi-packet parameter group can require less than
>   8B. The extra bytes shall be filled with 0xFF"
>
> If someone can confirm this is the same language as in J1939, I'd say we don't
> really need to support non-padded (E)TP-packets. Please note that I am only
> referring to (E)TP-packets here!


From SAE-J1939-21

"Data ranges for parameters used by this Group Function:
Sequence Number:
1 to 255 (1 byte)
Byte:
1
Sequence Number
2-8
Packetized Data (7 bytes). Note the last packet of a multipacket
Parameter Group may
require less than 8 data bytes. The extra bytes should be filled with FF 16 ."

J1939-21 matches what is in ISO-11783-3

J1939-21 section 5.2.7.1 says that messages with less than 8 bytes of
data are legal.  The recommendation is to pad messages out to 8 bytes
for compatibility and future proofing reasons, but it is not required.

Austin

Re: j1939

[Kurt Van Dijck]

> 
> > But often, especially for proprietary messages, not all 8 bytes are
> > used, and the DLC is cut accordingly.
> > And less often, this is done for TP too.
> > I also did that for as long as I can remember.
> > As long as all defined bytes are there, this should not really be a
> > problem.
> > 
> > Recently, someone on this list was confronted with an engine ECU that
> > dropped TP that had the last packet not 8 bytes long.
> > And I believe I've encountered an issue a long time ago where this
> > happened on all PGNs.
> > So I introduced this option, having 3 possibilities:
> > * no padding
> > * padding for TP
> > * padding for all PGN.
> 
> I have yet to check, but I fear that in ISOBus conformance tests on some
> places at least this is tested for. I'd say that if the standard says "pad and
> fill with 0xff", any ECU should do this and not doing it should be regarded as
> a bug... 

I (EIA Electronics) got a certified ISOBus Terminal with this code, back
in 2011, with this code (well, the code back then). I cannot remember
that I needed to pad TP.
The networking layer was the easy part in Potsdam, the layers above were
more difficult :-)

Things may have become more strict in between.
And we should be prepared for the strict setting.
On the other hand, during debugging, not padding helps identifying the
real data, and this has been valueable too for me.

I think we agree on what must be possible, we may just differ on the
defaults and how to set things.

Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

On 10/04/2016 03:57 PM, Kurt Van Dijck wrote:
> Recently, someone on this list was confronted with an engine ECU that
> dropped TP that had the last packet not 8 bytes long.
> And I believe I've encountered an issue a long time ago where this
> happened on all PGNs.
> So I introduced this option, having 3 possibilities:
> * no padding
> * padding for TP
> * padding for all PGN.

How to switch between padding for TP only and padding for all PGN?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2130 bytes --]

On 10/04/2016 07:31 PM, Kurt Van Dijck wrote:
>>
>>> But often, especially for proprietary messages, not all 8 bytes are
>>> used, and the DLC is cut accordingly.
>>> And less often, this is done for TP too.
>>> I also did that for as long as I can remember.
>>> As long as all defined bytes are there, this should not really be a
>>> problem.
>>>
>>> Recently, someone on this list was confronted with an engine ECU that
>>> dropped TP that had the last packet not 8 bytes long.
>>> And I believe I've encountered an issue a long time ago where this
>>> happened on all PGNs.
>>> So I introduced this option, having 3 possibilities:
>>> * no padding
>>> * padding for TP
>>> * padding for all PGN.
>>
>> I have yet to check, but I fear that in ISOBus conformance tests on some
>> places at least this is tested for. I'd say that if the standard says "pad and
>> fill with 0xff", any ECU should do this and not doing it should be regarded as
>> a bug... 
> 
> I (EIA Electronics) got a certified ISOBus Terminal with this code, back
> in 2011, with this code (well, the code back then). I cannot remember
> that I needed to pad TP.
> The networking layer was the easy part in Potsdam, the layers above were
> more difficult :-)
> 
> Things may have become more strict in between.
> And we should be prepared for the strict setting.
> On the other hand, during debugging, not padding helps identifying the
> real data, and this has been valueable too for me.
> 
> I think we agree on what must be possible, we may just differ on the
> defaults and how to set things.

It seems, or rather what I'd like to see is that we want most things to
be per socket option. All module parameters should go away. The default
for transport_max_size might be a system wide value, similar to
/proc/sys/net/core/rmem_max.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Patrick Menschel]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

>> Recently, someone on this list was confronted with an engine ECU that
>> dropped TP that had the last packet not 8 bytes long.
>> And I believe I've encountered an issue a long time ago where this
>> happened on all PGNs.
>> So I introduced this option, having 3 possibilities:
>> * no padding
>> * padding for TP
>> * padding for all PGN.
> How to switch between padding for TP only and padding for all PGN?
>
> Marc
>

I'm working with J1939 regularly and programmed J1939 in Python3 a while
ago.

From my experience, message construction is responsibility of the user
program.

There are numerous SPNs to be implemented in a message while
unimplemented bits are to be padded with ones.
It may be possible to pad all messages with 0xFF but that doesn't solve
the non implemented bits in front of the padding.

I think it's best practice to fulfill the requirements by creating a
j1939 message with 8 Bytes of 0xFF and then replace the bits for a
specific SPN.
This would be in user space when calling the J1939 message constructor.

BTW many j1939 descriptor files types such as the PCAN ".sym" files,
define SPNs as a bit offset and a length (additionally with type, factor
and offset as compu_method), so no need to invent the wheel again.

Regards,
Patrick


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3709 bytes --]

Re: j1939

[Kurt Van Dijck]

> 
> >> Recently, someone on this list was confronted with an engine ECU that
> >> dropped TP that had the last packet not 8 bytes long.
> >> And I believe I've encountered an issue a long time ago where this
> >> happened on all PGNs.
> >> So I introduced this option, having 3 possibilities:
> >> * no padding
> >> * padding for TP
> >> * padding for all PGN.
> > How to switch between padding for TP only and padding for all PGN?

0: no padding
1: TP padding
2: all PGN padding
> >
> > Marc
> >
> 
> I'm working with J1939 regularly and programmed J1939 in Python3 a while
> ago.
> 
> From my experience, message construction is responsibility of the user
> program.
> 
> There are numerous SPNs to be implemented in a message while
> unimplemented bits are to be padded with ones.
> It may be possible to pad all messages with 0xFF but that doesn't solve
> the non implemented bits in front of the padding.
> 
> I think it's best practice to fulfill the requirements by creating a
> j1939 message with 8 Bytes of 0xFF and then replace the bits for a
> specific SPN.
> This would be in user space when calling the J1939 message constructor.

Although this does not apply to TP, it does apply to all other PGN's.
So there is no real need to pad all PGN's.

Kurt

Re: j1939

[Kurt Van Dijck]

> On 10/04/2016 07:31 PM, Kurt Van Dijck wrote:
> >>
> >>> But often, especially for proprietary messages, not all 8 bytes are
> >>> used, and the DLC is cut accordingly.
> >>> And less often, this is done for TP too.
> >>> I also did that for as long as I can remember.
> >>> As long as all defined bytes are there, this should not really be a
> >>> problem.
> >>>
> >>> Recently, someone on this list was confronted with an engine ECU that
> >>> dropped TP that had the last packet not 8 bytes long.
> >>> And I believe I've encountered an issue a long time ago where this
> >>> happened on all PGNs.
> >>> So I introduced this option, having 3 possibilities:
> >>> * no padding
> >>> * padding for TP
> >>> * padding for all PGN.
> >>
> >> I have yet to check, but I fear that in ISOBus conformance tests on some
> >> places at least this is tested for. I'd say that if the standard says "pad and
> >> fill with 0xff", any ECU should do this and not doing it should be regarded as
> >> a bug... 
> > 
> > I (EIA Electronics) got a certified ISOBus Terminal with this code, back
> > in 2011, with this code (well, the code back then). I cannot remember
> > that I needed to pad TP.
> > The networking layer was the easy part in Potsdam, the layers above were
> > more difficult :-)
> > 
> > Things may have become more strict in between.
> > And we should be prepared for the strict setting.
> > On the other hand, during debugging, not padding helps identifying the
> > real data, and this has been valueable too for me.
> > 
> > I think we agree on what must be possible, we may just differ on the
> > defaults and how to set things.
> 
> It seems, or rather what I'd like to see is that we want most things to
> be per socket option. All module parameters should go away. The default
> for transport_max_size might be a system wide value, similar to
> /proc/sys/net/core/rmem_max.

do you propose to add /proc files?
I'm willing to replace module parameters with /proc files, but I don't
see the real advantage yet. And module parameters were quicker :-)

Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

On 10/04/2016 08:38 PM, Kurt Van Dijck wrote:
> 
>>
>>>> Recently, someone on this list was confronted with an engine ECU that
>>>> dropped TP that had the last packet not 8 bytes long.
>>>> And I believe I've encountered an issue a long time ago where this
>>>> happened on all PGNs.
>>>> So I introduced this option, having 3 possibilities:
>>>> * no padding
>>>> * padding for TP
>>>> * padding for all PGN.
>>> How to switch between padding for TP only and padding for all PGN?
> 
> 0: no padding
> 1: TP padding
> 2: all PGN padding

Hmmm, have I taken the latest and greatest code of yours?

"padding" is static and only used once in main.c

https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/tree/net/can/j1939/main.c?h=j1939#n45

https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/tree/net/can/j1939/main.c?h=j1939#n171

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1023 bytes --]

On 10/04/2016 08:40 PM, Kurt Van Dijck wrote:
>>> I think we agree on what must be possible, we may just differ on the
>>> defaults and how to set things.
>>
>> It seems, or rather what I'd like to see is that we want most things to
>> be per socket option. All module parameters should go away. The default
>> for transport_max_size might be a system wide value, similar to
>> /proc/sys/net/core/rmem_max.
> 
> do you propose to add /proc files?

No - However I'm not sure yet, though.

> I'm willing to replace module parameters with /proc files, but I don't
> see the real advantage yet. And module parameters were quicker :-)

When trying to bring new code into the kernel, this is probably not the
best argument :)

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

On 4 October 2016 20:43:01 CEST, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>On 10/04/2016 08:38 PM, Kurt Van Dijck wrote:
>> 
>>>
>>>>> Recently, someone on this list was confronted with an engine ECU
>that
>>>>> dropped TP that had the last packet not 8 bytes long.
>>>>> And I believe I've encountered an issue a long time ago where this
>>>>> happened on all PGNs.
>>>>> So I introduced this option, having 3 possibilities:
>>>>> * no padding
>>>>> * padding for TP
>>>>> * padding for all PGN.
>>>> How to switch between padding for TP only and padding for all PGN?
>> 
>> 0: no padding
>> 1: TP padding
>> 2: all PGN padding
>
>Hmmm, have I taken the latest and greatest code of yours?
>
>"padding" is static and only used once in main.c
>
>https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/tree/net/can/j1939/main.c?h=j1939#n45
>
>https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/tree/net/can/j1939/main.c?h=j1939#n171
>
I see.
I wii check later. Maybe I still needed to extend ... In that case, consider the above a configuration proposal?
Kurt

>Marc

Sent from a small mobile device

Re: j1939

[David Jander]

On Tue, 4 Oct 2016 19:54:30 +0200
Patrick Menschel <menschel.p@posteo.de> wrote:

> >> Recently, someone on this list was confronted with an engine ECU that
> >> dropped TP that had the last packet not 8 bytes long.
> >> And I believe I've encountered an issue a long time ago where this
> >> happened on all PGNs.
> >> So I introduced this option, having 3 possibilities:
> >> * no padding
> >> * padding for TP
> >> * padding for all PGN.  
> > How to switch between padding for TP only and padding for all PGN?
> >
> > Marc
> >  
> 
> I'm working with J1939 regularly and programmed J1939 in Python3 a while
> ago.
> 
> From my experience, message construction is responsibility of the user
> program.

I agree.

> There are numerous SPNs to be implemented in a message while
> unimplemented bits are to be padded with ones.
> It may be possible to pad all messages with 0xFF but that doesn't solve
> the non implemented bits in front of the padding.

I think the kernel should never modify packets from user-space in such a way.
I think we should limit this option to chose between:

 0: (default) Strictly standards compliant (i.e. pad TP messages) and
 1: Not pad TP messages (may break standard).

> I think it's best practice to fulfill the requirements by creating a
> j1939 message with 8 Bytes of 0xFF and then replace the bits for a
> specific SPN.

That may be true for many standard PGN's, but not for all, and certainly not
for proprietary messages. Again, IMHO, the kernel should not mess with
user-space messages.

> This would be in user space when calling the J1939 message constructor.

Precisely.

> BTW many j1939 descriptor files types such as the PCAN ".sym" files,
> define SPNs as a bit offset and a length (additionally with type, factor
> and offset as compu_method), so no need to invent the wheel again.

Don't know exactly what you mean, but this sounds user-space specific to me.

> Regards,
> Patrick
> 

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[David Jander]

On Tue, 4 Oct 2016 19:32:02 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/04/2016 03:57 PM, Kurt Van Dijck wrote:
> > Recently, someone on this list was confronted with an engine ECU that
> > dropped TP that had the last packet not 8 bytes long.
> > And I believe I've encountered an issue a long time ago where this
> > happened on all PGNs.
> > So I introduced this option, having 3 possibilities:
> > * no padding
> > * padding for TP
> > * padding for all PGN.  
> 
> How to switch between padding for TP only and padding for all PGN?

Please don't. I think we should get rid of that last option. It is like trying
to correct HTTP GET request syntax in kernel-space if user-space is too lazy
to get it right. Don't do that.

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939

[Kurt Van Dijck]

> On 10/04/2016 08:40 PM, Kurt Van Dijck wrote:
> >>> I think we agree on what must be possible, we may just differ on the
> >>> defaults and how to set things.
> >>
> >> It seems, or rather what I'd like to see is that we want most things to
> >> be per socket option. All module parameters should go away. The default
> >> for transport_max_size might be a system wide value, similar to
> >> /proc/sys/net/core/rmem_max.
> > 
> > do you propose to add /proc files?

In that case, I didn't understand your point :-(

> 
> No - However I'm not sure yet, though.
> 
> > I'm willing to replace module parameters with /proc files, but I don't
> > see the real advantage yet. And module parameters were quicker :-)
> 
> When trying to bring new code into the kernel, this is probably not the
> best argument :)

Well, in my experience, the infrastructures are built so that
easy-to-read code do the right thing in the right way.
In that regard, I suspect module parameters may be the preferred way?

Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1871 bytes --]

On 10/05/2016 08:02 PM, Kurt Van Dijck wrote:
>> On 10/04/2016 08:40 PM, Kurt Van Dijck wrote:
>>>>> I think we agree on what must be possible, we may just differ on the
>>>>> defaults and how to set things.
>>>>
>>>> It seems, or rather what I'd like to see is that we want most things to
>>>> be per socket option. All module parameters should go away. The default
>>>> for transport_max_size might be a system wide value, similar to
>>>> /proc/sys/net/core/rmem_max.
>>>
>>> do you propose to add /proc files?
> 
>> No - However I'm not sure yet, though.
> In that case, I didn't understand your point :-(

The rmem_max and rmem_default and their wmem friends define both a
system wide default and a maximum for the read and write buffer of
sockets. And there are two sockopts to change the default up to the
maximum value that correspondes to the _max value. Where to put this
value is another question. As it's a system wide setting /proc might be
a sensible place.

>>> I'm willing to replace module parameters with /proc files, but I don't
>>> see the real advantage yet. And module parameters were quicker :-)
>>
>> When trying to bring new code into the kernel, this is probably not the
>> best argument :)
> 
> Well, in my experience, the infrastructures are built so that
> easy-to-read code do the right thing in the right way.
> In that regard, I suspect module parameters may be the preferred way?

Some weeks ago Grek turned down a patch, as it introduced new module
parameters and said quite clearly no new module parameters.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

--- Original message ---
> Date:   Thu, 6 Oct 2016 09:26:00 +0200
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> To: David Jander <david@protonic.nl>, Austin Schuh
>  <austin@peloton-tech.com>, Oliver Hartkopp <socketcan@hartkopp.net>,
>  linux-can <linux-can@vger.kernel.org>
> Subject: Re: j1939
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
>  Icedove/45.2.0
> 
> On 10/05/2016 08:02 PM, Kurt Van Dijck wrote:
> >> On 10/04/2016 08:40 PM, Kurt Van Dijck wrote:
> >>>>> I think we agree on what must be possible, we may just differ on the
> >>>>> defaults and how to set things.
> >>>>
> >>>> It seems, or rather what I'd like to see is that we want most things to
> >>>> be per socket option. All module parameters should go away. The default
> >>>> for transport_max_size might be a system wide value, similar to
> >>>> /proc/sys/net/core/rmem_max.
> >>>
> >>> do you propose to add /proc files?
> > 
> >> No - However I'm not sure yet, though.
> > In that case, I didn't understand your point :-(
> 
> The rmem_max and rmem_default and their wmem friends define both a
> system wide default and a maximum for the read and write buffer of
> sockets. And there are two sockopts to change the default up to the
> maximum value that correspondes to the _max value. Where to put this
> value is another question. As it's a system wide setting /proc might be
> a sensible place.

I see.
Your example happened to live in proc, but that wasn't the point.
I agree with something like that, although I only envisioned xxx_default
and not xxx_max.
> 
> >>> I'm willing to replace module parameters with /proc files, but I don't
> >>> see the real advantage yet. And module parameters were quicker :-)
> >>
> >> When trying to bring new code into the kernel, this is probably not the
> >> best argument :)
> > 
> > Well, in my experience, the infrastructures are built so that
> > easy-to-read code do the right thing in the right way.
> > In that regard, I suspect module parameters may be the preferred way?
> 
> Some weeks ago Grek turned down a patch, as it introduced new module
> parameters and said quite clearly no new module parameters.
> 
> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 

Re: j1939

[Kurt Van Dijck]

Hey Marc,

I was able to take a close look in the code.

> Hello,
> 
> I've ported Kurt's latest j1939 patches to linux-4.7. I've squashed all
> patches into one. Then a patch to undo unused modifications and a bunch
> of patches to make checkpatch happy.
> 
> I've pushed the result to
> 
> https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939
> 
> The remaining warning is about a case statement without fall through
> annotations and/or breaks:
> 
I rebased this code for my own use so that I have access to my
individual commits. You will find this on

git://github.com/kurt-vd/linux, branch j1939d-v4.7

I pushed 2 extra commits:
* 3dfa139: Pad TP data packets only
  This is based on the discussions here. I promised to move the
  padding...
* e2447ef: make TP data transmission work again
  This commit fixes a mistake you've done with the fall-trough
  statements in j1939tp_txnext (this isn't the easiest state machien on
  Earth :-) ).

Kind regards,
Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1216 bytes --]

On 10/06/2016 10:44 AM, Kurt Van Dijck wrote:
>> The remaining warning is about a case statement without fall through
>> annotations and/or breaks:
>>
> I rebased this code for my own use so that I have access to my
> individual commits. You will find this on
> 
> git://github.com/kurt-vd/linux, branch j1939d-v4.7

As long as the diff between you and my branch is zero :)

> I pushed 2 extra commits:
> * 3dfa139: Pad TP data packets only
>   This is based on the discussions here. I promised to move the
>   padding...

Thanks.

> * e2447ef: make TP data transmission work again
>   This commit fixes a mistake you've done with the fall-trough
>   statements in j1939tp_txnext (this isn't the easiest state machien on
>   Earth :-) ).

Thanks.

I have cherry picked both and will push them to v4.7. If everything
works on the v4.7 branch, I'll squash most of it together an move to v4.8.

Thoughts?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

> 
> On 10/06/2016 10:44 AM, Kurt Van Dijck wrote:
> >> The remaining warning is about a case statement without fall through
> >> annotations and/or breaks:
> >>
> > I rebased this code for my own use so that I have access to my
> > individual commits. You will find this on
> > 
> > git://github.com/kurt-vd/linux, branch j1939d-v4.7
> 
> As long as the diff between you and my branch is zero :)

That is the case.

> 
> > I pushed 2 extra commits:
> > * 3dfa139: Pad TP data packets only
> >   This is based on the discussions here. I promised to move the
> >   padding...
> 
> Thanks.
> 
> > * e2447ef: make TP data transmission work again
> >   This commit fixes a mistake you've done with the fall-trough
> >   statements in j1939tp_txnext (this isn't the easiest state machien on
> >   Earth :-) ).
> 
> Thanks.
> 
> I have cherry picked both and will push them to v4.7. If everything
> works on the v4.7 branch, I'll squash most of it together an move to v4.8.
> 
> Thoughts?

1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.
2) Tonight, I happen to meet an "open source license" expert.
   I'll try to discuss with him the email address thing.

Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1210 bytes --]

On 10/06/2016 11:24 AM, Kurt Van Dijck wrote:
>>> * e2447ef: make TP data transmission work again
>>>   This commit fixes a mistake you've done with the fall-trough
>>>   statements in j1939tp_txnext (this isn't the easiest state machien on
>>>   Earth :-) ).
>>
>> Thanks.
>>
>> I have cherry picked both and will push them to v4.7. If everything
>> works on the v4.7 branch, I'll squash most of it together an move to v4.8.
>>
>> Thoughts?
> 
> 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.

Tnx.

> 2) Tonight, I happen to meet an "open source license" expert.
>    I'll try to discuss with him the email address thing.

IANAL: I think the tool pick up the Author of the commit. The (c)
copyright line is not relevant for tools, only humans...and lawyers.
Have a look at the .mailmap feature for author and/or email address
mapping: https://git-scm.com/docs/git-shortlog

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

> On 10/06/2016 11:24 AM, Kurt Van Dijck wrote:
> >>> * e2447ef: make TP data transmission work again
> >>>   This commit fixes a mistake you've done with the fall-trough
> >>>   statements in j1939tp_txnext (this isn't the easiest state machien on
> >>>   Earth :-) ).
> >>
> >> Thanks.
> >>
> >> I have cherry picked both and will push them to v4.7. If everything
> >> works on the v4.7 branch, I'll squash most of it together an move to v4.8.
> >>
> >> Thoughts?
> > 
> > 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.

I'm done.
I fixed the Makefile so it can compile can-j1939 as module again :-)
I changed the defaults to strict j1939 compliant (way-to-large max
	packet size, padding).
And I migrated the module parameters to /proc/sys/net/can-j1939.
I loaded the module, and verified /proc, without any real wire tests.

So you need to cherry-pick another 3 commits.

Kind regards,
Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

On 10/06/2016 12:26 PM, Kurt Van Dijck wrote:
>> On 10/06/2016 11:24 AM, Kurt Van Dijck wrote:
>>>>> * e2447ef: make TP data transmission work again
>>>>>   This commit fixes a mistake you've done with the fall-trough
>>>>>   statements in j1939tp_txnext (this isn't the easiest state machien on
>>>>>   Earth :-) ).
>>>>
>>>> Thanks.
>>>>
>>>> I have cherry picked both and will push them to v4.7. If everything
>>>> works on the v4.7 branch, I'll squash most of it together an move to v4.8.
>>>>
>>>> Thoughts?
>>>
>>> 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.
> 
> I'm done.
> I fixed the Makefile so it can compile can-j1939 as module again :-)
> I changed the defaults to strict j1939 compliant (way-to-large max
> 	packet size, padding).
> And I migrated the module parameters to /proc/sys/net/can-j1939.
> I loaded the module, and verified /proc, without any real wire tests.
> 
> So you need to cherry-pick another 3 commits.

Tnx. Does it compile with disabled PROC support?

Marc


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

> On 10/06/2016 12:26 PM, Kurt Van Dijck wrote:
> >> On 10/06/2016 11:24 AM, Kurt Van Dijck wrote:
> >>>>> * e2447ef: make TP data transmission work again
> >>>>>   This commit fixes a mistake you've done with the fall-trough
> >>>>>   statements in j1939tp_txnext (this isn't the easiest state machien on
> >>>>>   Earth :-) ).
> >>>>
> >>>> Thanks.
> >>>>
> >>>> I have cherry picked both and will push them to v4.7. If everything
> >>>> works on the v4.7 branch, I'll squash most of it together an move to v4.8.
> >>>>
> >>>> Thoughts?
> >>>
> >>> 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.
> > 
> > I'm done.
> > I fixed the Makefile so it can compile can-j1939 as module again :-)
> > I changed the defaults to strict j1939 compliant (way-to-large max
> > 	packet size, padding).
> > And I migrated the module parameters to /proc/sys/net/can-j1939.
> > I loaded the module, and verified /proc, without any real wire tests.
> > 
> > So you need to cherry-pick another 3 commits.
> 
> Tnx. Does it compile with disabled PROC support?

Yes, it just compiled (the module), not sure how it reacts when actually
used like that.
I just think I forgot to add descriptions in Documentation/sysctl.

> 
> Marc
> 
> 
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 

Re: j1939

[Kurt Van Dijck]

> > >>>
> > >>> 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.
> > > 
> > > I'm done.
> > > I fixed the Makefile so it can compile can-j1939 as module again :-)
> > > I changed the defaults to strict j1939 compliant (way-to-large max
> > > 	packet size, padding).
> > > And I migrated the module parameters to /proc/sys/net/can-j1939.
> > > I loaded the module, and verified /proc, without any real wire tests.
> > > 
> > > So you need to cherry-pick another 3 commits.
> > 
> > Tnx. Does it compile with disabled PROC support?
> 
> Yes, it just compiled (the module), not sure how it reacts when actually
> used like that.
> I just think I forgot to add descriptions in Documentation/sysctl.

I added yet another commit, that add documentation of the new sysctl's.

Kurt

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1211 bytes --]

On 10/06/2016 01:32 PM, Kurt Van Dijck wrote:
>>>>>>
>>>>>> 1) I'll try to move module parameters to /proc/sys/net/can-j1939/ items.
>>>>
>>>> I'm done.
>>>> I fixed the Makefile so it can compile can-j1939 as module again :-)
>>>> I changed the defaults to strict j1939 compliant (way-to-large max
>>>> 	packet size, padding).
>>>> And I migrated the module parameters to /proc/sys/net/can-j1939.
>>>> I loaded the module, and verified /proc, without any real wire tests.
>>>>
>>>> So you need to cherry-pick another 3 commits.
>>>
>>> Tnx. Does it compile with disabled PROC support?
>>
>> Yes, it just compiled (the module), not sure how it reacts when actually
>> used like that.
>> I just think I forgot to add descriptions in Documentation/sysctl.
> 
> I added yet another commit, that add documentation of the new sysctl's.

Thanks - Cherry-picked and added two incremental patches on my branch.

Marc.

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

details :-)

commit 5f78bacf9df1df0c54f79f3008b5d74f9b020843
Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
Date:   Thu Oct 6 14:20:16 2016

    can-j1939: update MAINTAINERS
    
    Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

diff --git a/MAINTAINERS b/MAINTAINERS
index 02ab672..32b59fa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2819,9 +2819,8 @@ F:	include/uapi/linux/can/error.h
 F:	include/uapi/linux/can/netlink.h
 
 CAN-J1939 NETWORK LAYER
-M:	Kurt Van Dijck <kurt.van.dijck@eia.be>
-L:	socketcan-core@lists.berlios.de
-L:	netdev@vger.kernel.org
+M:	Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
+L:	linux-can@vger.kernel.org
 S:	Maintained
 F:	Documentation/networking/j1939.txt
 F:	net/can/j1939/

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 398 bytes --]

On 10/06/2016 02:26 PM, Kurt Van Dijck wrote:
> details :-)

Added as a fixup commit to linux-can-next/j1939

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: j1939

[Kurt Van Dijck]

Marc

I tried to replace a spinlock+int by an atomic_t.
Is it something similar that you had in mind.
You were right in your comments that it is replaceable by an atomic_t.
I pushed it to my repo, and appended it to this email.

David, this should be transparent, but I wasn't able to test it.

Kind regards,
Kurt

commit b8bfd1fe84869ee1c5eed3b8e6717da67dc3da60
Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
Date:   Sat Oct 8 21:43:04 2016

    can-j1939: replace spinlock+counter by atomic_t
    
    The skb_pending member of j1939 sockets does not really need an extra spinlock
    
    Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 66d2301..41d59dd 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -57,8 +57,7 @@ struct j1939_sock {
 	 * when transport protocol comes in.
 	 * To allow emitting in order, keep a 'pending' nr. of packets
 	 */
-	int skb_pending;	/* TODO: convert to atomic_t */
-	spinlock_t lock;	/* protects skb_pending */
+	atomic_t skb_pending;
 	wait_queue_head_t waitq;
 };
 
@@ -67,42 +66,39 @@ static inline struct j1939_sock *j1939_sk(const struct sock *sk)
 	return container_of(sk, struct j1939_sock, sk);
 }
 
-/* skb_pending issues */
+/*
+ * j1939_sock_pending_add_first
+ * Succeeds when the first pending SKB is scheduled
+ * Fails when SKB are already pending
+ */
 static inline int j1939_sock_pending_add_first(struct sock *sk)
 {
-	int saved;
 	struct j1939_sock *jsk = j1939_sk(sk);
 
-	spin_lock_bh(&jsk->lock);
-	if (!jsk->skb_pending) {
-		++jsk->skb_pending;
-		saved = 1;
-	} else {
-		saved = 0;
-	}
-	spin_unlock_bh(&jsk->lock);
-	return saved;
+	/*
+	 * atomic_cmpxchg returns the old value
+	 * When it was 0, it is exchanged with 1 and this function
+	 * succeeded. (return 1)
+	 * When it was != 0, it is not exchanged, and this fuction
+	 * fails (returns 0).
+	 */
+	return !atomic_cmpxchg(&jsk->skb_pending, 0, 1);
 }
 
 static inline void j1939_sock_pending_add(struct sock *sk)
 {
 	struct j1939_sock *jsk = j1939_sk(sk);
 
-	spin_lock_bh(&jsk->lock);
-	++jsk->skb_pending;
-	spin_unlock_bh(&jsk->lock);
+	atomic_inc(&jsk->skb_pending);
 }
 
 void j1939_sock_pending_del(struct sock *sk)
 {
 	struct j1939_sock *jsk = j1939_sk(sk);
-	int saved;
 
-	spin_lock_bh(&jsk->lock);
-	--jsk->skb_pending;
-	saved = jsk->skb_pending;
-	spin_unlock_bh(&jsk->lock);
-	if (!saved)
+	/* atomic_dec_return returns the new value */
+	if (!atomic_dec_return(&jsk->skb_pending))
+		/* no pending SKB's */
 		wake_up(&jsk->waitq);
 }
 
@@ -209,13 +205,13 @@ static int j1939sk_init(struct sock *sk)
 	struct j1939_sock *jsk = j1939_sk(sk);
 
 	INIT_LIST_HEAD(&jsk->list);
-	spin_lock_init(&jsk->lock);
 	init_waitqueue_head(&jsk->waitq);
 	jsk->sk.sk_priority = j1939_to_sk_priority(6);
 	jsk->sk.sk_reuse = 1; /* per default */
 	jsk->addr.sa = J1939_NO_ADDR;
 	jsk->addr.da = J1939_NO_ADDR;
 	jsk->addr.pgn = J1939_NO_PGN;
+	atomic_set(&jsk->skb_pending, 0);
 	return 0;
 }
 

Re: j1939

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2062 bytes --]

On 10/08/2016 09:48 PM, Kurt Van Dijck wrote:
> I tried to replace a spinlock+int by an atomic_t.
> Is it something similar that you had in mind.
> You were right in your comments that it is replaceable by an atomic_t.
> I pushed it to my repo, and appended it to this email.
> 
> David, this should be transparent, but I wasn't able to test it.

Added to as a squash commit to linux-can-next/j1939.

> Kind regards,
> Kurt
> 
> commit b8bfd1fe84869ee1c5eed3b8e6717da67dc3da60
> Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> Date:   Sat Oct 8 21:43:04 2016
> 
>     can-j1939: replace spinlock+counter by atomic_t
>     
>     The skb_pending member of j1939 sockets does not really need an extra spinlock
>     
>     Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> 
> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index 66d2301..41d59dd 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -57,8 +57,7 @@ struct j1939_sock {
>  	 * when transport protocol comes in.
>  	 * To allow emitting in order, keep a 'pending' nr. of packets
>  	 */
> -	int skb_pending;	/* TODO: convert to atomic_t */
> -	spinlock_t lock;	/* protects skb_pending */
> +	atomic_t skb_pending;
>  	wait_queue_head_t waitq;
>  };
>  
> @@ -67,42 +66,39 @@ static inline struct j1939_sock *j1939_sk(const struct sock *sk)
>  	return container_of(sk, struct j1939_sock, sk);
>  }
>  
> -/* skb_pending issues */
> +/*
> + * j1939_sock_pending_add_first
> + * Succeeds when the first pending SKB is scheduled
> + * Fails when SKB are already pending
> + */

I've added a fixup commit to convert to net multi line comment style,
which has text directly in the first line of the comment.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: J1939

[Kurt Van Dijck]

Hi David,

On Fri, Nov 15, 2013 at 11:29:42AM -0500, David H. Lynch Jr. wrote:
> Hi Kurt;
> 
>     I mostly do board bringup work as a consultant. So I can handle merging a
> driver into the kernel or using one of the testing kernel trees.
> 
>     Many of my clients use CAN/J1939/NMEA2000/CANOpen, to this point I have
> always had to deal with these in userspace.
>     A standard stack that was in the kernel is very appealing.
> 
>     I have already pulled the 3 git repositories related to J1939.  But my
> quick skim did not  get me far.

Well, I suppose you got linux-can-j1939, can-j1939-utils & iproute2-j1939?
Please mind you want to select a proper branch of linux-can-j1939.

Did you manage to compile them?
The reason I ask this is that most questions I get wrt. j1939 is getting it
merged & compiled.
In the meanwhile, I updated the j1939 page on www.elinux.org with
basic instructions how to build.

> 
>     While I am primarily interested in sending/receiving J1939 messages at the
> socket level, even being able to use cansend to say send a J1939 engine speed
> would be really useful.
> 
>     i can already do that as the J1939 engine speed message fits in an ordinary
> CAN message - but all messages don't.

You're at the right address. The things you describe are exactly what
I implemented.

> 
>     Anyway if it is of value to you to have some one else using and testing
> this and providing feedback, to the extent that I can incorporate it into my
> existing work I would be happy to do so.

The GPL license allows you to reuse the components in your work.
Any real-world comments on the stack should be usefull.
In the past 2 years, some usefull improvements were made this way.

>    
>     When I get a bit more time to look through the code and Docs I will - the
> answers I am looking for are certainly there, I was just hoping to be able to
> get to testing some things quickly.  

I composed a simple kickstart guide to j1939 on linux for new collegues.
It may be of good use for you and others too.
I'm just looking for a place to share the thing.
Can elinux.org use markdown syntax?

Kurt

>      
> 
> 
> On Thu, 2013-11-14 at 21:31 +0100, Kurt Van Dijck wrote:
> 
>     Hi David,
> 
>     On Wed, Nov 13, 2013 at 07:26:28PM -0500, David H. Lynch Jr. wrote:
>     > I recently noticed that J1939 support has been incorporated into the
>     > Linux Kernel CAN stack.
> 
>     Well, the j1939 stack is not yet incorporated in mainline Linux.
>     It's API, which adheres to BSD socket semantics than raw CAN due
>     to the addressing concept, causes some resistance.
> 
>     You are of course welcome to share your opinions about that matter.
> 
>     Once the API is agreed, mainline should be easy.
> 
>     >
>     > I would be happy to help with testing that with some pointers.
> 
>     Great. I have performed only manual tests in the past to get
>     the stack right.
>     Some sort of automated test may be usefull to avoid
>     problems over time with refactoring or changing kernel
>     infrastructures.
> 
>     A sample of the code got certified in an ISOBUS product.
>     At that time, the code proved to be functional, both for
>     the normal use case as for a lot of erroneous situations.
> 
>     >
>     > Any simple examples of sending/receiving specific PGN's ?
> 
>     You can read Documentation/networking/j1939.txt.
>     The can-utils-j1939 package have some tools, but those do not expose
>     the easiest examples.
> 
>     I will add some easier examples that are a bit closer to real-life
>     programs.
> 
>     Kind regards,
>     Kurt
> 
> 

-- 
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
kurt.van.dijck@eia.be
+32-38708534

Re: J1939

[Kurt Van Dijck]

Hi David,

On Wed, Nov 13, 2013 at 07:26:28PM -0500, David H. Lynch Jr. wrote:
> I recently noticed that J1939 support has been incorporated into the
> Linux Kernel CAN stack. 

Well, the j1939 stack is not yet incorporated in mainline Linux.
It's API, which adheres to BSD socket semantics than raw CAN due
to the addressing concept, causes some resistance.

You are of course welcome to share your opinions about that matter.

Once the API is agreed, mainline should be easy.

> 
> I would be happy to help with testing that with some pointers. 

Great. I have performed only manual tests in the past to get
the stack right.
Some sort of automated test may be usefull to avoid
problems over time with refactoring or changing kernel
infrastructures.

A sample of the code got certified in an ISOBUS product.
At that time, the code proved to be functional, both for
the normal use case as for a lot of erroneous situations.

> 
> Any simple examples of sending/receiving specific PGN's ?

You can read Documentation/networking/j1939.txt.
The can-utils-j1939 package have some tools, but those do not expose
the easiest examples.

I will add some easier examples that are a bit closer to real-life
programs.

Kind regards,
Kurt

J1939

[David H. Lynch Jr.]

I recently noticed that J1939 support has been incorporated into the
Linux Kernel CAN stack. 

I would be happy to help with testing that with some pointers. 

Any simple examples of sending/receiving specific PGN's ?