On 03.02.2011 10:40, Kurt Van Dijck wrote:
> This is the main j1939 protocol stack.
Hello Kurt,
sorry for the delay ...
In the last week i tried to become more familiar with j1939 by reading
multiple documents from the net - and your source code.
First some general remarks inside the text:
> net/can/j1939/j1939-ac.c | 554 ++++++++++++++++
> net/can/j1939/j1939-bus.c | 600 +++++++++++++++++
> net/can/j1939/j1939-filter.c | 230 +++++++
> net/can/j1939/j1939-loop.c | 74 ++
> net/can/j1939/j1939-priv.h | 298 +++++++++
> net/can/j1939/j1939-proc.c | 102 +++
> net/can/j1939/j1939-promisc.c | 90 +++
> net/can/j1939/j1939-rtnl.c | 316 +++++++++
> net/can/j1939/j1939-socket.c | 927 ++++++++++++++++++++++++++
> net/can/j1939/j1939-stack.c | 183 +++++
> net/can/j1939/j1939-tp.c | 1465
> +++++++++++++++++++++++++++++++++++++++++
s/j1939-//g
> net/can/j1939/j1939.c | 438 ++++++++++++
net/can/j1939/main.c
> +
> +static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> +{
> + struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> + struct j1939_sock *jsk = j1939_sk(sock->sk);
> + struct j1939_ecu *ecu = NULL;
> + int ret;
> +
> + if (len < required_size(can_addr.j1939, *addr))
> + return -EINVAL;
> + if (addr->can_family != AF_CAN)
> + return -EINVAL;
> +
> + /* lock s.lock first, to avoid circular lock dependancy */
> + mutex_lock(&s.lock);
> + lock_sock(sock->sk);
> + if (jsk->state & JSK_BOUND) {
> + ret = -EBUSY;
> + if (addr->can_ifindex != jsk->sk.sk_bound_dev_if)
> + goto fail_locked;
> + /*
> + * do not allow to change addres after first bind(),
> + * (it would require updating the j1939_ecu list)
> + * but allow the change SA when using dynaddr,
> + * and allow to change PGN
> + */
> + if (!jsk->addr.src ||
> + (jsk->addr.src != addr->can_addr.j1939.name) ||
> + (jsk->addr.pgn != addr->can_addr.j1939.pgn))
> + goto fail_locked;
> + /* set to be able to send address claims */
> + jsk->addr.sa = addr->can_addr.j1939.addr;
> + /* since this socket is bound already, we can skip a lot */
> + release_sock(sock->sk);
> + return 0;
> + }
mutex_unlock??
> +/* lowest layer */
> +static void j1939_can_recv(struct sk_buff *skb, void *data)
> +{
> + int orig_len;
> + struct j1939_sk_buff_cb *sk_addr;
> + struct can_frame *can;
please name it 'cf' or 'msg'
> +
> +static int j1939_can_send(struct sk_buff *skb)
> +{
> + int ret, dlc;
> + canid_t canid;
> + struct j1939_sk_buff_cb *sk_addr;
> + struct net_device *netdev = NULL;
> + struct can_frame *can;
> +
> + dlc = skb->len;
> + if (dlc > 8)
> + return -EMSGSIZE;
> + ret = pskb_expand_head(skb, SKB_DATA_ALIGN(CAN_HDR),
> + CAN_FTR + (8-dlc), GFP_ATOMIC);
> + if (ret < 0)
> + return ret;
> +
> + can = (void *)skb_push(skb, CAN_HDR);
> + BUG_ON(!can);
> + /* make it a full can frame */
> + skb_put(skb, CAN_FTR + (8 - dlc));
why (8 - dlc) ???
> +
> + /* fix the 'always free' policy of can_send */
> + skb = skb_get(skb);
> + ret = can_send(skb, 1);
> + if (!ret)
> + /* free when can_send succeedded */
> + kfree_skb(skb);
> +failed:
> + if (netdev)
> + dev_put(netdev);
> + return ret;
Does 'fix' mean: Make use of the return value??
> +static inline struct j1939_ecu *find_ecu(int ifindex, name_t name, int sa)
> +{
> + if (name)
> + return j1939_ecu_find_by_name(name, ifindex);
> + else if (sa < 0xfe)
> + return j1939_ecu_find_by_addr(sa, ifindex);
> + return 0;
> +}
There are many occurrences of 0xfe or something similar byte values.
Please use #defines for these j1939 values.
> +static __exit void j1939_module_exit(void)
> +{
> + j1939flt_module_exit();
> + j1939lp_module_exit();
> + j1939_promisc_module_exit();
> + j1939tp_module_exit();
> + j1939ac_module_exit();
> + j1939sk_module_exit();
> + j1939bus_module_exit();
> +
> + j1939_stack_unregister(&j1939_tx_end);
> + unregister_netdevice_notifier(&s.notifier);
> +
> + j1939_stack_module_exit();
> + j1939_proc_module_exit();
> +}
In general all your j1939 implementation adds 5000+ LOC
box:~/net-2.6/net/can/j1939$ cat *.c | wc -l
5000
not counting the .h files.
I'll continue my general remarks in the patch adding the documentation.
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
