Urs Thuermann wrote:
> Patrick McHardy <[EMAIL PROTECTED]> writes:
>
>
>>>+static int loopback; /* loopback testing. Default: 0 (Off) */
>>>+module_param(loopback, int, S_IRUGO);
>>>+MODULE_PARM_DESC(loopback, "Loop back frames (for testing). Default: 0
>>>(Off)");
>>
>>
>>I would still prefer to have this on a per-device level configured
>>through netlink, but since we currently don't support specifying
>>flags for new devices anyways, I won't argue about it anymore
>>(OTOH, if you'd agree I could send a patch to add this feature
>>to the rtnl_link API).
>
>
> Hm, somehow this topic comes up again and again. I think there is
> some misunderstanding about loopback in general and vcan, but I must
> admit that our documentation until recently didn't describe this good
> enough. In fact, I think we also got better understanding from this
> discussion and trying to explain this.
>
> vcan is *not* a special loopback device like lo and it is not needed
> to use PF_CAN. Every CAN device driver should preferably loop back
> frames sent by dev->hard_start_xmit() to netif_rx(). Since this is
> unusual for netdevice drivers, the CAN core can do this itself as a
> fallback for drivers that don't loopback.
I understood that from Oliver's explanations.
> For vcan it makes no difference whether loopback is done in the vcan
> driver or in the CAN core. No user will ever have to use this module
> parameter. Having a driver which can show both driver behaviors is
> however useful for debugging our own code, to check whether the CAN
> core does the right thing in both cases.
>
> vcan is not a loopback device but a null device which simply discards
> all sent frames since there is no hardware to send the frame to. Like
> other CAN drivers it can loop back the frame to the CAN core, but this
> is not different from other CAN drivers.
>
> It can be useful to have several vcan null devices so that different
> apps can talk to each other through different interfaces.
My opinion is simply that stuff like that shouldn't be configured
through module parameters, but as I said, I don't want to get into
this discussion again, its not a big deal if you insist on keeping
it.
> Now I think we should consider removing the loopback code from
> can_send() and demand from each CAN driver that it *has to* implement
> this itself.
Really? I don't know about any other drivers, but it seems to make
sense to me to handle this in the core instead of reimplementing
it in every driver.
>
>
>>>+
>>>+struct vcan_priv {
>>>+ struct net_device *dev;
>>>+ struct list_head list;
>>>+};
>>
>>
>>This is not needed anymore. The rtnl_link_unregister function calls
>>the ->dellink function for each device of this type. Check out the
>>current dummy.c driver.
>
>
> OK.
>
>
>>>+ if (atomic_read(&skb->users) != 1) {
>>>+ struct sk_buff *old_skb = skb;
>>>+
>>>+ skb = skb_clone(old_skb, GFP_ATOMIC);
>>>+ DBG(KERN_INFO "%s: %s: freeing old skbuff %p, "
>>>+ "using new skbuff %p\n",
>>>+ dev->name, __FUNCTION__, old_skb, skb);
>>>+ kfree_skb(old_skb);
>>
>>skb_share_check()?
>
>
> New to me. I read that skb_share_check() decrements the refcount so I
> am not sure it is we want. Will take a look tomorrow.
It kfree_skb's the old skb, just as you do above.
>
>
>>>+ /* receive with packet counting */
>>>+ skb->sk = srcsk;
>>
>>
>>Where is the socket used and what makes sure it still exists?
>
>
> This socket pointer is used when the loopback frame is processed in
> raw_rcv, only to compare it to the receiving socket to determine if
> this frame was sent by the receiving socket itself. The srcsk is only
> compared, not dereferenced.
Thanks for the explanation, that should be fine.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
