On Wed, Feb 09, 2011 at 08:25:50PM +0100, Oliver Hartkopp wrote:
>
> > 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
>
ok, I can live with that :-)
>
>
> > +
> > +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??
thanks!!!
>
> > +/* 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'
ok
>
>
> > +
> > +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) ???
I strip the skb to contain only [dlc] bytes.
before returning here, I restore the skb to it's original size.
See pskb_expand_head() called earlier.
>
>
> > +
> > + /* 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??
The 'always free' policy means: When can_send() fails,
the skb is still freed. This works contra-intuitive for
functions making use of that.
'fix' does mean: skb_get(skb) is called, and when can_send
succeeds, kfree_skb() is called to eliminate the effect of skb_get().
Good remark.
>
>
> > +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.
indeed. Need to create a macro for that.
>
> > +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
is that a good or bad?
>
> Oliver
I appreciate this review.
Kurt
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
