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

[Oliver Hartkopp <socketcan@hartkopp.net>]

Urs Thuermann wrote:
> Oliver Hartkopp <socketcan@hartkopp.net> writes:
>
>   
>> 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