Re: [patch 5/7] CAN: Add virtual CAN netdevice driver
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: 2007-06-04 18:27:09
Oliver Hartkopp wrote:
I think, it goes into the right direction to use skb->pkt_type. The flag should really be somewhere inside the skb as all back references into the sk would become sticky in the implementation. But regarding the use of skb->pkt_type i would suggest to take a closer look on the definitions in include/linux/if_packet.h and how the pkt_type is to be used inside the kernel. In my opinion we should use ... * TX-Path: PACKET_OTHERHOST: send the CAN frame without loopback PACKET_BROADCAST : send the CAN frame with loopback (aka local broadcast) See an example of this approach in drivers/net/arcnet/rfc1051.c : http://www.linux-m32r.org/lxr/http/source/drivers/net/arcnet/rfc1051.c?a=i386#L99 * RX-Path: PACKET_HOST : just an incoming CAN frame for this host Any comments? ACKs? Best regards, Oliver
The updated changes would look like this. Of course it has been tested and is working fine :-) Regards, Oliver Index: net/can/af_can.c ===================================================================
--- net/can/af_can.c (revision 325)
+++ net/can/af_can.c (working copy)@@ -257,7 +257,6 @@ */ int can_send(struct sk_buff *skb, int loop) { - struct sock **tx_sk = (struct sock **)skb->cb; int err; if (skb->dev->type != ARPHRD_CAN) {
@@ -265,30 +264,43 @@ return -EPERM; } + skb->protocol = htons(ETH_P_CAN); + if (loop) { /* local loopback of sent CAN frames (default) */ - /* indication for the CAN driver: do loopback */ - *tx_sk = skb->sk; + /* + * This packet is not only sent on the CAN bus but + * also broadcasted to local subscribers on this host. + */ + skb->pkt_type = PACKET_BROADCAST; /* - * The reference to the originating sock may be also required + * The reference to the originating sock may be required * by the receiving socket to indicate (and ignore) his own - * sent data. Example: can_raw sockopt CAN_RAW_RECV_OWN_MSGS + * sent data. Example: can_raw sockopt CAN_RAW_RECV_OWN_MSGS. + * Therefore we have to ensure that skb->sk remains the + * reference to the originating sock by restoring skb->sk + * after each skb_clone() or skb_orphan() usage. + * skb->sk is usually unused and unset in the rx path. */ /* interface not capabable to do the loopback itself? */ if (!(skb->dev->flags & IFF_LOOPBACK)) { + struct sock *srcsk = skb->sk; struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC); /* perform the local loopback here */ - newskb->protocol = htons(ETH_P_CAN); newskb->ip_summed = CHECKSUM_UNNECESSARY; + newskb->sk = srcsk; netif_rx(newskb); } } else { - /* indication for the CAN driver: no loopback required */ - *tx_sk = NULL; + /* + * Indication for the CAN driver: No loopback required! + * This packet is only transmitted to the CAN bus. + */ + skb->pkt_type = PACKET_OTHERHOST; } if (!(skb->dev->flags & IFF_UP))
@@ -581,10 +593,12 @@ static inline void deliver(struct sk_buff *skb, struct receiver *r) { + struct sock *srcsk = skb->sk; struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC); DBG("skbuff %p cloned to %p\n", skb, clone); if (clone) { + clone->sk = srcsk; r->func(clone, r->data); r->matches++; }
Index: net/can/raw.c ===================================================================
--- net/can/raw.c (revision 325)
+++ net/can/raw.c (working copy)@@ -154,7 +154,7 @@ if (!ro->recv_own_msgs) { /* check the received tx sock reference */ - if (*(struct sock **)skb->cb == sk) { + if (skb->sk == sk) { DBG("trashed own tx msg\n"); kfree_skb(skb); return;
Index: drivers/net/can/vcan.c ===================================================================
--- drivers/net/can/vcan.c (revision 325)
+++ drivers/net/can/vcan.c (working copy)@@ -133,6 +133,7 @@ skb->protocol = htons(ETH_P_CAN); skb->dev = dev; skb->ip_summed = CHECKSUM_UNNECESSARY; + skb->pkt_type = PACKET_HOST; DBG("received skbuff on interface %d\n", dev->ifindex);
@@ -149,8 +150,8 @@ stats->tx_packets++; stats->tx_bytes += skb->len; - /* tx socket reference pointer: Loopback required if not NULL */ - loop = *(struct sock **)skb->cb != NULL; + /* indication for CAN netdevice drivers that loopback is required */ + loop = (skb->pkt_type == PACKET_BROADCAST); if (!loopback) { /* no loopback handling available inside this driver */
@@ -170,6 +171,8 @@ /* perform standard loopback handling for CAN network interfaces */ if (loop) { + struct sock *srcsk = skb->sk; + if (atomic_read(&skb->users) != 1) { struct sk_buff *old_skb = skb;
@@ -183,6 +186,8 @@ } else skb_orphan(skb); + skb->sk = srcsk; + /* receive with packet counting */ vcan_rx(skb, dev); } else {