j1939 - mainline problems

Hi Kurt,

i still try to get behind the implementation.

Simplicity is something else ...

Some things that definitely will not go into mainline:

7 can-j1939 SYSCTL

    7.1 /proc/sys/net/can-j1939/transport_max_payload_in_bytes
    7.2 /proc/sys/net/can-j1939/transport_cts_nr_of_frames
    7.3 /proc/sys/net/can-j1939/transport_tx_retry_ms

sysfs for this kind of configuration is not getting into mainline.

Provide proper defaults and a sockopt interface, if people really want to change their private defaults.

6 can-j1939 procfs interface

    6.1 /proc/net/can-j1939/ecu
    6.2 /proc/net/can-j1939/filter
    6.3 /proc/net/can-j1939/sock
    6.4 /proc/net/can-j1939/transport

What does transport mean?
Does this entry appear when a PDU is sent and disappear afterwards?

-> seq_printf(sqf, "iface\tsrc\tdst\tpgn\tdone/total\n");

Supposed debugging output has to be removed anyway.

Things that look fishy:

#1 (Documentation)

Per default j1939 is not active. Specifying can_ifindex != 0 in bind(2) or connect(2) needs an active j1939 on that interface. You must have done

  $ ip link set canX j1939 on
  on that interface.

Eh?

Never seen something like "ip link set eth0 tcp on" before setting the ip address ??

Why doesn't it 'just work' ?

#2 (Documentation)

Add the address to the system:

  $ ip addr add j1939 0x20 dev can0

Bind:

    struct sockaddr_can addr;

    memset(&addr, 0, sizeof(addr));
    addr.can_ifindex = ifindex("can0"); // ifindex is a substitute.
    addr.can_addr.j1939.name = J1939_NO_NAME;
    addr.can_addr.j1939.addr = 0x20;
    addr.can_addr.j1939.pgn = J1939_NO_PGN;

    bind(sk, (void *)&addr, sizeof(addr));

Why does bind() not make the "ip addr add j1939 0x20 dev can0" automatically?

#3 (j1939/main.c)

in static void j1939_can_recv(struct sk_buff *skb, void *data)

        msg = (void *)skb->data;
        orig_len = skb->len;
        skb_pull(skb, CAN_HDR);
        /* fix length, set to dlc, with 8 maximum */
        skb_trim(skb, min_t(uint8_t, msg->can_dlc, 8));

You can not fiddle with skb offsets unless you've cloned it.

#4 (segments / ecus / bus / sk / ...)

Even when trying to follow any concepts in the source code, it's hard to follow any idea when these structures and definitions are not described.

E.g. why don't you add a j1939_priv pointer to struct dev_rcv_lists when CONFIG_CAN_J1939 is enabled?

Looks like j1939_segment_find() is a frequently used walkthrough function ...

#5 (j1939/main.c) filters

#define J1939_CAN_ID CAN_EFF_FLAG
#define J1939_CAN_MASK (CAN_EFF_FLAG | CAN_RTR_FLAG)

        ret = can_rx_register(netdev, J1939_CAN_ID, J1939_CAN_MASK,
                        j1939_can_recv, jseg, "j1939");

You read EVERY EFF frame. Filters should be created based on the concrete requirement of the userspace (socket).

#6 (amount of code)

ls *.c | grep -v mod | xargs wc -l
227 address-claim.c
523 bus.c

    76 filter.c

458 main.c
104 proc.c
43 promisc.c
306 rtnl.c
984 socket.c
1449 transport.c
4170 total

Sometimes i feel a much more simple approach is needed.

You provide a full featured 4 tons SUV IMO no one understands.
- thousands of knobs to configure
- and when looking under the hood you get lost from all the cables

I would like to have a bicycle and probably a quad with AC some days later.

E.g. with a struct j1939 message having an address field (similar to the CAN-ID) with some data behind it.

That's simple and similar to other implementations.

I might be wrong with this requirement of simplicity. But i might be right too.

Let's wait for beta-testers feedback if your concept is easy to use and adopt for j1939 application programmers ...

Best regards,
Oliver

Do you mean the code or the j1939 protocol :-) ?

s/sysfs/sysctl/ ?

Where to put tunables else than sysctl?
I tread /proc/sys/net/can-j1939/transport_tx_retry_ms
on the same level as /proc/sys/net/ipv4/tcp_keepalive_intvl and such.

These are not socket specific!
I could turn them into link specifics and use

	$ ip link set can0 j1939 transport-max-payload-in-bytes xxx
What about that?
it would put more effort in iproute2, and not decrease code size in kernel either.

Nope,
transport lists the pending transport sessions. This is not debug, I have a specific use case for this:

Imagine a bigger transport protocol session, of about 500KB.
This takes (a lot of) time. A socket (like a gui application)
has no way of knowing that an incoming session with data is pending. Since 500KB takes a lot of time, a GUI may choose to read /proc/..../transport and look if sessions for its socket are pending.

ecu, filter & sock are some kind of 'debug'.

see:
net/can/bcm.c L160
net/can/proc.c

I'm willing to remove real 'debug' or 'developer statistics', but 'transport' stays somehow somewhere. Alternatives are too expensive.

I'm glad you compare with ethernet. In fact, j1939 compares to UDP a bit.

Because ethernet headers have a specific field to indicate 'ip' (or ipv6 or ...), the software can autodetect to which protocol handler should receive the packet. CAN just has not such field.
It would be wrong to assume every 29bit frame is j1939 when the module is loaded on the system.

I would have preferred that, but it's not feasable.

assigning an address to an iface during bind()?

	$ man 2 bind
bind assigns a local address to a socket.
$ ip addr ... assign a local address to an interface.

I'll fix.

ok.

euhm, can you explain a bit more?

Your optimizing already?

Note that single J1939 PGN's may map onto multiple CANid's, _and_ single CANid's may result into different J1939 PGN's.

Btw,

	$ ip link set IFACE j1939 on
is explicit.

The major benefit of this code is it's API to userspace. I'm sure the lines of code can change.
So first issue is to focus on the API.

In contrast to CAN, J1939 features a Source & destination addressing concept, and variable length packets (up to several Megabytes using ISOBUS extensions). BSD sockets primitives sendto & recvfrom happen to map 100% to this.

Putting all address info as data just because RAW CAN does this is bad idea.

s/other implementation/straight CAN protocols/

50% chance?

Kurt

This would allow to lookup a j1939_priv struct from a netdev directly. That is a great idea!
I had not thought about that yet. Partly because I did start out-of-tree :-)

I'll just go in that direction.
Btw, It would be a struct j1939_segment * currently. I consider

	s/j1939_segment/j1939_priv/g 

if that makes things more clear.

Your idea of dev_rcv_lists solves that.

Kind regards,
Kurt

'priv' looks consistent from the af_can point of view, as it is a j1939 private structure attached to the generic CAN per-netdevice structure.

That's why i was pointing at this possibility :-)

Regards,
Oliver