Re: [patch 5/7] CAN: Add virtual CAN netdevice driver

Urs Thuermann wrote:

Oliver Hartkopp [off-list ref] writes:

  

quoted

2. The loopback indication is done by using the unused skb->protocol in
the tx path.
    

I don't think we should (mis-)use skb->protocol as a loopback flag.
  

Yep! After reading the comments from Patrick and Urs i definitely agree
to their concerns.

IMO it would be better to skb->pkt_type.  This is used to indicate
packet type to rcv functions registered by dev_add_pack().  It is set
by netdevice drivers to PACKET_{MULTICAST,BROADCAST,HOST,OTHER} for
received packets.  In the send path it is set to PACKET_OUTGOING on
the copy of the skbuff that is delivered to the sockets registered on
ptype_all (typically packet sockets from tcpdump or other sniffers).
AFAICS, pkt_type is not used otherwise in the send path.

We could set skb->pkt_type = PACKET_LOOPBACK to flag to the CAN
netdevice driver whether to loop back the packet.
  

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