From: Patrick McHardy <kaber@trash•net>
To: Urs Thuermann <urs@isnogud•escape.de>
Cc: netdev@vger•kernel.org, David Miller <davem@davemloft•net>,
Thomas Gleixner <tglx@linutronix•de>,
Oliver Hartkopp <oliver@hartkopp•net>,
Oliver Hartkopp <oliver.hartkopp@volkswagen•de>
Subject: Re: [PATCH 5/7] CAN: Add virtual CAN netdevice driver
Date: Wed, 19 Sep 2007 10:41:05 +0200 [thread overview]
Message-ID: <46F0E0A1.6000108@trash.net> (raw)
In-Reply-To: <ygf7imnzk60.fsf@janus.isnogud.escape.de>
Urs Thuermann wrote:
> Patrick McHardy <kaber@trash•net> 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.
next prev parent reply other threads:[~2007-09-19 8:48 UTC|newest]
Thread overview: 119+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-17 10:03 [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #6 Urs Thuermann
2007-09-17 10:03 ` [PATCH 1/7] CAN: Allocate protocol numbers for PF_CAN Urs Thuermann
2007-09-18 13:31 ` Patrick McHardy
2007-09-17 10:03 ` [PATCH 2/7] CAN: Add PF_CAN core module Urs Thuermann
2007-09-17 15:50 ` Paul E. McKenney
2007-09-18 13:31 ` Patrick McHardy
2007-09-18 14:54 ` Urs Thuermann
2007-09-18 15:07 ` Patrick McHardy
2007-09-18 21:20 ` Urs Thuermann
2007-09-19 8:27 ` Patrick McHardy
2007-09-20 8:53 ` Urs Thuermann
2007-09-20 10:33 ` Patrick McHardy
2007-09-20 11:30 ` Urs Thuermann
2007-09-20 11:43 ` Patrick McHardy
2007-09-17 10:03 ` [PATCH 3/7] CAN: Add raw protocol Urs Thuermann
2007-09-18 14:13 ` Patrick McHardy
2007-09-18 21:49 ` Urs Thuermann
2007-09-19 8:34 ` Patrick McHardy
2007-09-17 10:03 ` [PATCH 4/7] CAN: Add broadcast manager (bcm) protocol Urs Thuermann
2007-09-17 10:03 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-09-18 15:02 ` Patrick McHardy
2007-09-18 22:24 ` Urs Thuermann
2007-09-19 6:26 ` Oliver Hartkopp
2007-09-19 8:41 ` Patrick McHardy [this message]
2007-09-17 10:03 ` [PATCH 6/7] CAN: Add maintainer entries Urs Thuermann
2007-09-17 10:03 ` [PATCH 7/7] CAN: Add documentation Urs Thuermann
2007-09-17 17:30 ` Randy Dunlap
2007-09-17 20:22 ` Urs Thuermann
2007-09-17 20:37 ` Thomas Gleixner
2007-09-17 20:49 ` Urs Thuermann
2007-09-17 22:57 ` Randy Dunlap
2007-09-17 23:19 ` Urs Thuermann
2007-09-18 6:51 ` Bill Fink
2007-09-18 7:20 ` Urs Thuermann
-- strict thread matches above, loose matches on Subject: below --
2007-11-16 15:02 [PATCH 0/7] CAN: New PF_CAN protocol family for 2.6.25, update Urs Thuermann
2007-11-16 15:02 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-11-14 12:13 [PATCH 0/7] CAN: New PF_CAN protocol family for 2.6.25 Urs Thuermann
2007-11-14 12:13 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-11-14 12:51 ` Patrick McHardy
2007-11-14 13:36 ` Urs Thuermann
2007-11-14 13:45 ` Patrick McHardy
2007-10-05 10:49 [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #10 Urs Thuermann
2007-10-05 10:49 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-10-02 13:10 [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #9 Urs Thuermann
2007-10-02 13:10 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-10-02 14:20 ` Arnaldo Carvalho de Melo
2007-10-02 15:07 ` Oliver Hartkopp
2007-10-02 16:46 ` Arnaldo Carvalho de Melo
2007-10-02 21:02 ` Oliver Hartkopp
2007-10-02 21:43 ` Arnaldo Carvalho de Melo
2007-10-02 21:50 ` David Miller
2007-10-03 7:06 ` Oliver Hartkopp
2007-10-02 21:52 ` Stephen Hemminger
2007-10-02 22:04 ` David Miller
2007-10-03 17:47 ` Oliver Hartkopp
2007-10-04 11:52 ` Urs Thuermann
2007-09-25 12:20 [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #8 Urs Thuermann
2007-09-25 12:20 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-09-25 13:26 ` YOSHIFUJI Hideaki / 吉藤英明
2007-09-25 14:00 ` Urs Thuermann
2007-09-25 14:47 ` Patrick McHardy
2007-09-25 17:55 ` Oliver Hartkopp
2007-09-25 17:53 ` Patrick McHardy
2007-09-25 19:09 ` Oliver Hartkopp
2007-09-20 18:43 [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #7 Urs Thuermann
2007-09-20 18:43 ` [PATCH 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-09-27 15:54 ` Eric W. Biederman
2007-09-27 16:16 ` Eric W. Biederman
2007-09-27 19:18 ` David Miller
2007-09-28 8:48 ` Oliver Hartkopp
2007-09-28 16:52 ` Eric W. Biederman
2007-09-28 18:33 ` Oliver Hartkopp
2007-08-04 2:06 [patch 0/7] CAN: Add new PF_CAN protocol family, try #5 Urs Thuermann
2007-08-04 2:07 ` [patch 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-06-22 3:44 [patch 0/7] CAN: Add new PF_CAN protocol family, try #3 Urs Thuermann
2007-06-22 3:44 ` [patch 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-06-22 11:02 ` Patrick McHardy
2007-06-22 12:22 ` Urs Thuermann
2007-06-22 12:38 ` Patrick McHardy
2007-06-23 12:05 ` Oliver Hartkopp
2007-06-23 12:52 ` Patrick McHardy
2007-06-23 15:13 ` Oliver Hartkopp
2007-06-23 16:25 ` Patrick McHardy
2007-06-23 16:42 ` Oliver Hartkopp
2007-06-23 17:13 ` Patrick McHardy
2007-07-04 11:37 ` Urs Thuermann
2007-07-04 14:01 ` Patrick McHardy
2007-07-09 11:37 ` Urs Thuermann
2007-07-09 14:18 ` Patrick McHardy
2007-07-09 15:27 ` Oliver Hartkopp
2007-07-11 19:41 ` Oliver Hartkopp
2007-07-11 22:52 ` Patrick McHardy
2007-07-16 6:05 ` Oliver Hartkopp
2007-07-16 8:37 ` David Miller
2007-07-16 13:08 ` Patrick McHardy
2007-07-16 16:27 ` Oliver Hartkopp
2007-07-16 13:07 ` Patrick McHardy
2007-07-16 16:00 ` Oliver Hartkopp
2007-06-23 21:01 ` David Miller
2007-06-23 21:44 ` Oliver Hartkopp
2007-06-23 20:51 ` David Miller
2007-06-23 21:49 ` Oliver Hartkopp
2007-05-30 13:11 [patch 0/7] CAN: Add new PF_CAN protocol family, update Urs Thuermann
2007-05-30 13:11 ` [patch 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
2007-05-30 17:13 ` Patrick McHardy
2007-05-30 18:39 ` Oliver Hartkopp
2007-05-30 19:16 ` Patrick McHardy
2007-05-30 19:48 ` Oliver Hartkopp
2007-05-30 19:58 ` Patrick McHardy
2007-05-31 5:13 ` Oliver Hartkopp
2007-05-31 20:25 ` Oliver Hartkopp
2007-06-01 14:54 ` Patrick McHardy
2007-06-02 9:51 ` Oliver Hartkopp
2007-06-02 19:58 ` Oliver Hartkopp
2007-06-03 19:16 ` Oliver Hartkopp
2007-06-04 11:53 ` Patrick McHardy
2007-06-04 12:44 ` Urs Thuermann
2007-06-06 11:39 ` Patrick McHardy
2007-06-04 12:17 ` Urs Thuermann
2007-06-04 16:32 ` Oliver Hartkopp
2007-06-04 18:26 ` Oliver Hartkopp
2007-06-02 8:09 ` Urs Thuermann
2007-06-03 18:27 ` Patrick McHardy
2007-05-30 17:28 ` Stephen Hemminger
2007-05-30 18:57 ` Oliver Hartkopp
2007-05-30 19:01 ` Stephen Hemminger
2007-05-30 19:25 ` Oliver Hartkopp
2007-05-16 14:51 [patch 0/7] CAN: Add new PF_CAN protocol family Urs Thuermann
2007-05-16 14:51 ` [patch 5/7] CAN: Add virtual CAN netdevice driver Urs Thuermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=46F0E0A1.6000108@trash.net \
--to=kaber@trash$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oliver.hartkopp@volkswagen$(echo .)de \
--cc=oliver@hartkopp$(echo .)net \
--cc=tglx@linutronix$(echo .)de \
--cc=urs@isnogud$(echo .)escape.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox