Re: confusing comment, explanation of @IFF_RUNNING in if.h

[Oliver Hartkopp]

On 08/27/2018 08:20 AM, Robert P. J. Day wrote:

On Sun, 26 Aug 2018, Stephen Hemminger wrote:


On Sun, 26 Aug 2018 15:20:24 -0400 (EDT)
"Robert P. J. Day"  wrote:


On Sun, 26 Aug 2018, Andrew Lunn wrote:


   i ask since, in my testing, when the interface should have been
up, the attribute file "operstate" for that interface showed
"unknown", and i wondered how worried i should be about that.


Hi Robert

You should probably post the driver for review. A well written
driver should not even need to care about any of this. phylib and
the netdev driver code does all the work. It only gets interesting
when you don't have a PHY, e.g. a stacked device, like bonding, or a
virtual device like tun/tap.


   i wish, but i'm on contract, and proprietary, and NDA and all that.
so i am reduced to crawling through the code, trying to figure out
what is misconfigured that is causing all this grief.

rday



So you expect FOSS developers to help you with proprietary licensed
driver. Good Luck with that.


   sorry, i'm sure this will all be released upon production, just not
while it's in the midst of development.


"released upon production" means usually:
Oh, we put that driver in a tar-ball on a CD that's shipped with the 
product and which will get no further visibility nor (security) maintenance.


Robert, please tell your manager that creating a driver is no rocket 
science and also brings no "costumer differentiation" which needs to be 
covered under NDA.


Posting drivers and bring it into mainline Linux heavily increases the 
quality due to the review process and all the people that are willing to 
help you to get better. At the end your driver gets long-term 
maintenance and other people can benefit from it - as your boss is 
getting benefit from using Linux right now.


When something is "released upon production" it will not be in a quality 
that it could go into the kernel - and no one will have the 
time/money/ambition to spend effort on it then. You have just produced 
one of the numerous dead out-of-tree drivers. That would be just sad.


Best regards,
Oliver

Re: [PATCH 01/18] docs: can.rst: fix a footnote reference

[Oliver Hartkopp]

+ Robert Schwebel (who thankfully did the txt -> rst conversion for can.txt)

On 05/07/2018 11:35 AM, Mauro Carvalho Chehab wrote:
> As stated at:
>   
> http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#footnotes
> 
> A footnote should contain either a number, a reference or
> an auto number, e. g.:
>   [1], [#f1] or [#].
> 
> While using [*] accidentaly works for html, it fails for other
> document outputs. In particular, it causes an error with LaTeX
> output, causing all books after networking to not be built.
> 
> So, replace it by a valid syntax.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>

Acked-by: Oliver Hartkopp <socket...@hartkopp.net>

Thanks Mauro!

Best regards,
Oliver

> ---
>  Documentation/networking/can.rst | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/can.rst 
> b/Documentation/networking/can.rst
> index d23c51abf8c6..2fd0b51a8c52 100644
> --- a/Documentation/networking/can.rst
> +++ b/Documentation/networking/can.rst
> @@ -164,7 +164,7 @@ The Linux network devices (by default) just can handle the
>  transmission and reception of media dependent frames. Due to the
>  arbitration on the CAN bus the transmission of a low prio CAN-ID
>  may be delayed by the reception of a high prio CAN frame. To
> -reflect the correct [*]_ traffic on the node the loopback of the sent
> +reflect the correct [#f1]_ traffic on the node the loopback of the sent
>  data has to be performed right after a successful transmission. If
>  the CAN network interface is not capable of performing the loopback for
>  some reason the SocketCAN core can do this task as a fallback solution.
> @@ -175,7 +175,7 @@ networking behaviour for CAN applications. Due to some 
> requests from
>  the RT-SocketCAN group the loopback optionally may be disabled for each
>  separate socket. See sockopts from the CAN RAW sockets in 
> :ref:`socketcan-raw-sockets`.
>  
> -.. [*] you really like to have this when you're running analyser
> +.. [#f1] you really like to have this when you're running analyser
> tools like 'candump' or 'cansniffer' on the (same) node.
>  
>  
> 

Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

[Oliver Hartkopp]

Hi Kirill, Marc,

I checked the code once more and added some debug output to the other 
parts in CAN notifier code.


In fact the code pointed to by both of you seems to be obsolete as I 
only wanted to be 'really sure' that no leftovers of the CAN filters at 
module unloading.




Yes, this one looks good:
https://marc.info/?l=linux-can=150169589119335=2

Regards,
Kirill



I was obviously too cautious ;-)

All tests I made resulted in the function iterating through all the CAN 
netdevices doing exactly nothing.


I'm fine with removing that stuff - but I'm not sure whether it's worth 
to push that patch to stable 4.12+ or even before 4.12 (without 
namespace support - and removing rcu_barrier() too).


Any opinions?

Best regards,
Oliver

Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

[Oliver Hartkopp]

- DaveM

Hi Kirill,

On 03/05/2018 04:22 PM, Kirill Tkhai wrote:

> Thanks for the explanation, and module unloading should be nice. Just to 
> clarify,
> I worry not about rules, but about netdevices.
> 
>   unshare -n ip link add type vcan
> 
> This command creates net ns, adds vcan there and exits. Then net ns is 
> destroyed.
> Since vcan has rtnl_link_ops, it unregistered by default_device_exit_batch().
> Real can devices are moved to init_net in default_device_exit(), as they don't
> have rtnl_link_ops set.

In fact most of the real CAN drivers have rtnl_link_ops:

https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L1162
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L1225

Just slcan.c which is something like slip.c is missing these feature.

AFAIK real CAN netdevices are created in init_net at system startup, and you
can move them to a netns later.

When we already have rtnl_link_ops in the real CAN drivers - what happens to
them when the namespace is destroyed? Are they still moved to init_net, or do
we need to add some default handler in the current rtnl_link_ops setup?

> So, for_each_netdev_rcu() cycle in can_pernet_exit() should be useless (there 
> are
> no can devices in the list of net's devices). This looks so for me, please say
> what devices are there if my assumption is wrong.

See above?

>>> Since can_pernet_ops is pernet subsys, it's executed after 
>>> default_device_exit()
>>> from default_device_ops pernet device, as devices exit methods are executed 
>>> first
>>> (see net/core/net_namespace.c).
>>
>> Hm - a device exit fires the NETDEV_UNREGISTER notifier which removes the
>> user-generated filters (e.g. in raw_notifier() in net/can/raw.c).
>> Finally the can_dev_rcv_lists structure is free'd in af_can.c.
>>
>> Marc Kleine-Budde recently proposed a patch to create the can_dev_rcv_lists 
>> at
>> netdevice creation time (-> the space is allocated by alloc_netdev() and
>> removed by free_netdev()). This would remove the handling (allocate & free) 
>> of
>> ml_priv by af_can.c. Would this rework fix the described issue?
> 
> Could you please give me a link to the patches? I can't find them in 
> patchwork.

There was a patchset of 14 patches from Marc where some of the refactoring &
renaming already made it into mainline - but the patches to move the
can_dev_rcv_lists data structure into the network device space have not been
pushed:

https://marc.info/?l=linux-can=150169588319315=2
https://marc.info/?l=linux-can=1=201708=2

This patch & documentation describes Marc's proposed idea best:
https://marc.info/?l=linux-can=150169589619340=2

Best regards,
Oliver

Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

[Oliver Hartkopp]

Hi Kirill,

On 03/01/2018 04:53 PM, Kirill Tkhai wrote:

> I'm converting/reviewing pernet_operations either they allow several net 
> namespaces
> to be created/destroyed in parallel or not. Please, see the details in my 
> recent
> patches in net-next.git, if your are interested.

Thanks for your effort to review all these different sites!

> There is a strange place in can_pernet_ops pernet subsys, I found:
> 
> static void can_pernet_exit(struct net *net)
> {
>   ...
>   rcu_read_lock();
>   for_each_netdev_rcu(net, dev) {
>   if (dev->type == ARPHRD_CAN && dev->ml_priv) {
>   struct can_dev_rcv_lists *d = dev->ml_priv; 
> 
>   BUG_ON(d->entries);
>   kfree(d);
>   dev->ml_priv = NULL;
>   }
>   }
>   rcu_read_unlock()
>   ...
> }
> 
> This code clears dev->ml_priv from can devices, and it looks strange.

To give some more background about these 'struct can_dev_rcv_lists':

The receive lists are managed by the AF_CAN framework in linux/net/can for
each CAN network device. When the per-net modules like can-raw, can-bcm or
can-gw are removed (or if there are no more open sockets or the netdevices are
removed) the CAN filters are removed too.

Finally - when can.ko is removed - the filters should be cleared (that's why
the BUG() statement checks the emptiness) and then the empty can_dev_rcv_lists
structure is free'd.

> Since can_pernet_ops is pernet subsys, it's executed after 
> default_device_exit()
> from default_device_ops pernet device, as devices exit methods are executed 
> first
> (see net/core/net_namespace.c).

Hm - a device exit fires the NETDEV_UNREGISTER notifier which removes the
user-generated filters (e.g. in raw_notifier() in net/can/raw.c).
Finally the can_dev_rcv_lists structure is free'd in af_can.c.

Marc Kleine-Budde recently proposed a patch to create the can_dev_rcv_lists at
netdevice creation time (-> the space is allocated by alloc_netdev() and
removed by free_netdev()). This would remove the handling (allocate & free) of
ml_priv by af_can.c. Would this rework fix the described issue?

> There are no NETIF_F_NETNS_LOCAL devices among can devices, though there is
> check of can_link_ops in safe_candev_priv(). I haven't found can devices may
> have net_device::rtnl_link_ops. But the code seems want to allow them.

We use rtnl_link_ops to create and remove virtual CAN interfaces (vcan.c and
vxcan.c) and to alter MTU values and bitrates for real CAN interfaces.

See:

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/can.txt#L1001

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/can.txt#L1041

> Anyway,
> it's wrong in any case:
> 
> 1)If there are can devices, which may be skipped by default_device_exit(),
> can_pernet_exit() must use rtnl_lock() instead of rcu_read_lock(), and
> it must move such devices to init_net. See wifi cfg80211_pernet_exit() for 
> example.
> 
> 2)If there are no such the devices, the code between rcu_read_lock() and 
> rcu_read_unlock()
> is useless, and must be deleted, as it never works and confuses a reader.

The latter would create a memory leak. Maybe the suggested change from Marc
would solve the entire problem then?

Thanks & best regards,
Oliver

Re: WARNING in can_rcv

[Oliver Hartkopp]

On 01/17/2018 10:43 AM, Marc Kleine-Budde wrote:

On 01/17/2018 09:07 AM, Oliver Hartkopp wrote:



On 01/17/2018 08:12 AM, Eric Biggers wrote:

On Wed, Jan 17, 2018 at 07:39:24AM +0100, Oliver Hartkopp wrote:



On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:

On Tue, Jan 16, 2018 at 7:07 PM, Marc Kleine-Budde <m...@pengutronix.de> wrote:

On 01/16/2018 06:58 PM, syzbot wrote:

Hello,

syzkaller hit the following crash on
a8750ddca918032d6349adbf9a4b6555e7db20da
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4386709c0c1284dca...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

device eql entered promiscuous mode
[ cut here ]
PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
net/can/af_can.c:724
Kernel panic - not syncing: panic_on_warn set ...


Invalid packages generate a warning (WARN_ONCE()), and you have
panic_on_warn active. Should we better silently drop these CAN packages?


Hi,

pr_warn_once() will be more appropriate. It prints a single line.



The idea behind this WARN() is to detect really bad things that might have
happen on network driver level:

The CAN subsystem registers with dev_add_pack() for ETH_P_CAN and
ETH_P_CANFD only. These ETH_P_ types are only allowed to be created by CAN
network devices (like vcan, vxcan, and real CAN drivers).

I don't have any strong opinion on using WARN() or pr_warn_once().
Is this detected violation worth using WARN(), as something already must
have gone really wrong to trigger this issue?



WARN() indicates a kernel bug.  If it's instead "userspace did something
stupid", or "someone sent some unexpected network packet", it needs to be
pr_warn_once(), pr_warn_ratelimited(), or removed entirely.


Ok. Thanks for the explanation!
It is "some bogus network driver sent something unexpected" - but that
does not harm the entire system.

pr_warn_once() seems the right way to go then.


Is this an Acked-by for both patches?



Yes :-)

Acked-by: Oliver Hartkopp <socket...@hartkopp.net>

I just did not expect that you wanted to update the patches before 
sending ...


Thanks,
Oliver

Re: WARNING in can_rcv

[Oliver Hartkopp]

On 01/17/2018 08:39 AM, Dmitry Vyukov wrote:

On Wed, Jan 17, 2018 at 8:12 AM, Eric Biggers <ebigge...@gmail.com> wrote:

On Wed, Jan 17, 2018 at 07:39:24AM +0100, Oliver Hartkopp wrote:



On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:

On Tue, Jan 16, 2018 at 7:07 PM, Marc Kleine-Budde <m...@pengutronix.de> wrote:

On 01/16/2018 06:58 PM, syzbot wrote:

Hello,

syzkaller hit the following crash on
a8750ddca918032d6349adbf9a4b6555e7db20da
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4386709c0c1284dca...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

device eql entered promiscuous mode
[ cut here ]
PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
net/can/af_can.c:724
Kernel panic - not syncing: panic_on_warn set ...


Invalid packages generate a warning (WARN_ONCE()), and you have
panic_on_warn active. Should we better silently drop these CAN packages?


Hi,

pr_warn_once() will be more appropriate. It prints a single line.



The idea behind this WARN() is to detect really bad things that might have
happen on network driver level:

The CAN subsystem registers with dev_add_pack() for ETH_P_CAN and
ETH_P_CANFD only. These ETH_P_ types are only allowed to be created by CAN
network devices (like vcan, vxcan, and real CAN drivers).

I don't have any strong opinion on using WARN() or pr_warn_once().
Is this detected violation worth using WARN(), as something already must
have gone really wrong to trigger this issue?



WARN() indicates a kernel bug.  If it's instead "userspace did something
stupid", or "someone sent some unexpected network packet", it needs to be
pr_warn_once(), pr_warn_ratelimited(), or removed entirely.



The packet comes from tun device. We could change tun to filter out
such packages earlier. However, in the context of "syzkaller support
for AF_CAN" discussion, it would actually be useful for fuzzer to be
able emit can packets for testing purposes. 


Yes - definitely! It's a safer process to check the reception side 
instead of maintaining thousands of potential transmitters.



For example, for tcp it
can not just emit random packets, it can build complex user<->network
interactions, for example, open a listening socket, connect to it
"from outside", accept the connection, and then exchange some data
over the active connection. It could do the same for can.


Yes.


Is it possible to allow can packets via tun?


Hm - didn't even think about it.
CAN frames have a fixed data structure (struct can_frame) so the tunnel 
would need to be capable to process SOCK_SEQPACKET (?!?) traffic.


Right now there has been no work to 'tunnel' CAN traffic.


Then we could leave this
WARNING in place.


Yes.


tun/vcan are contained within a net namespace, so
this should not be a security problem, right?


vcan can be created in or moved into a namespace. vxcan can bridge 
namespaces similar to veth. This is all local traffic then.


What kind of security problem would you have in mind there?


Or is there a way to do the same with vcan? If yes, then fuzzer could
use vcan.


Yes. This would be my idea too. Unfortunately I'm very busy @work this 
week - so I would like to dig deeper into your mail some days ago at the 
beginning of next week.



But then we need some fix for this WARNING: either change it
to pr_warn or change tun (I don't have strong preference which one).


From the discussions (also with Eric) I think going with pr_warn is the 
right way for now.


Tnx & best regards,
Oliver

Re: WARNING in can_rcv

[Oliver Hartkopp]

On 01/17/2018 08:12 AM, Eric Biggers wrote:

On Wed, Jan 17, 2018 at 07:39:24AM +0100, Oliver Hartkopp wrote:



On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:

On Tue, Jan 16, 2018 at 7:07 PM, Marc Kleine-Budde <m...@pengutronix.de> wrote:

On 01/16/2018 06:58 PM, syzbot wrote:

Hello,

syzkaller hit the following crash on
a8750ddca918032d6349adbf9a4b6555e7db20da
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4386709c0c1284dca...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

device eql entered promiscuous mode
[ cut here ]
PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
net/can/af_can.c:724
Kernel panic - not syncing: panic_on_warn set ...


Invalid packages generate a warning (WARN_ONCE()), and you have
panic_on_warn active. Should we better silently drop these CAN packages?


Hi,

pr_warn_once() will be more appropriate. It prints a single line.



The idea behind this WARN() is to detect really bad things that might have
happen on network driver level:

The CAN subsystem registers with dev_add_pack() for ETH_P_CAN and
ETH_P_CANFD only. These ETH_P_ types are only allowed to be created by CAN
network devices (like vcan, vxcan, and real CAN drivers).

I don't have any strong opinion on using WARN() or pr_warn_once().
Is this detected violation worth using WARN(), as something already must
have gone really wrong to trigger this issue?



WARN() indicates a kernel bug.  If it's instead "userspace did something
stupid", or "someone sent some unexpected network packet", it needs to be
pr_warn_once(), pr_warn_ratelimited(), or removed entirely.


Ok. Thanks for the explanation!
It is "some bogus network driver sent something unexpected" - but that 
does not harm the entire system.


pr_warn_once() seems the right way to go then.

Thanks,
Oliver

Re: WARNING in can_rcv

[Oliver Hartkopp]

On 01/16/2018 07:11 PM, Dmitry Vyukov wrote:

On Tue, Jan 16, 2018 at 7:07 PM, Marc Kleine-Budde  wrote:

On 01/16/2018 06:58 PM, syzbot wrote:

Hello,

syzkaller hit the following crash on
a8750ddca918032d6349adbf9a4b6555e7db20da
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4386709c0c1284dca...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

device eql entered promiscuous mode
[ cut here ]
PF_CAN: dropped non conform CAN skbuf: dev type 65534, len 42, datalen 0
WARNING: CPU: 0 PID: 3650 at net/can/af_can.c:729 can_rcv+0x1c5/0x200
net/can/af_can.c:724
Kernel panic - not syncing: panic_on_warn set ...


Invalid packages generate a warning (WARN_ONCE()), and you have
panic_on_warn active. Should we better silently drop these CAN packages?


Hi,

pr_warn_once() will be more appropriate. It prints a single line.



The idea behind this WARN() is to detect really bad things that might 
have happen on network driver level:


The CAN subsystem registers with dev_add_pack() for ETH_P_CAN and 
ETH_P_CANFD only. These ETH_P_ types are only allowed to be created by 
CAN network devices (like vcan, vxcan, and real CAN drivers).


I don't have any strong opinion on using WARN() or pr_warn_once().
Is this detected violation worth using WARN(), as something already must 
have gone really wrong to trigger this issue?


Best regards,
Oliver

Re: [PATCH] flex_can: Correct the checking for frame length in flexcan_start_xmit()

[Oliver Hartkopp]

On 01/02/2018 04:44 AM, Luu An Phu wrote:

From: "phu.luuan" <phu.lu...@nxp.com>

The flexcan_start_xmit() function compares the frame length with data register
length to write frame content into data[0] and data[1] register. Data register
length is 4 bytes and frame maximum length is 8 bytes.

Fix the check that compares frame length with 3. Because the register length
is 4.

Signed-off-by: Luu An Phu <phu.lu...@nxp.com>


Reviewed-by: Oliver Hartkopp <socket...@hartkopp.net>

Thanks for this improvement!

Best regards,
Oliver


---
  drivers/net/can/flexcan.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0626dcf..760d2c0 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -526,7 +526,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
data = be32_to_cpup((__be32 *)>data[0]);
flexcan_write(data, >tx_mb->data[0]);
}
-   if (cf->can_dlc > 3) {
+   if (cf->can_dlc > 4) {
data = be32_to_cpup((__be32 *)>data[4]);
flexcan_write(data, >tx_mb->data[1]);
}

Re: [PATCH] flex_can: Fix checking can_dlc

[Oliver Hartkopp]

Answering myself after reading my own comment once more:

In fact the code fix seems to be correct but the commit comment was 
completely wrong which lead to my answer ...


can_dlc can hold values from 0 .. 8.

The first 4 bytes are placed in data[0..3]. When we have more(!) than 4 
bytes in can_dlc, the bytes 5..8 are placed in data[4..7].


The good thing was, that the current check was more conservative than 
your suggestion so that we did not detect any data loss.


Please send a V2 patch with corrected commit message.

Thanks,
Oliver

ps.


From: "phu.luuan" 
+ * Copyright 2017 NXP


A one-liner contribution like this tiny fix usually does not lead to an 
attribution in the copyrights. Your contribution is already perfectly 
recorded by the Signed-off-by tag.

Re: [PATCH] flex_can: Fix checking can_dlc

[Oliver Hartkopp]

This patch looks wrong to me.

On 12/19/2017 09:40 AM, Luu An Phu wrote:

From: "phu.luuan" 

flexcan_start_xmit should write data to register when can_dlc > 4.
Because can_dlc is data length and it has value from 1 to 8.


No. can_dlc can contain values from 0 to 8. Even 0 is a valid DLC.


But CAN data
index has value from 0 to 7. So in case we have data in cf->data[4],
the can_dlc has value is more than 4.

Signed-off-by: Luu An Phu 
---
  drivers/net/can/flexcan.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0626dcf..0e3ff13 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -5,6 +5,7 @@
   * Copyright (c) 2009 Sascha Hauer, Pengutronix
   * Copyright (c) 2010-2017 Pengutronix, Marc Kleine-Budde 

   * Copyright (c) 2014 David Jander, Protonic Holland
+ * Copyright 2017 NXP
   *
   * Based on code originally by Andrey Volkov 
   *
@@ -526,7 +527,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
data = be32_to_cpup((__be32 *)>data[0]);
flexcan_write(data, >tx_mb->data[0]);
}
-   if (cf->can_dlc > 3) {


This is correct as data[0 .. 3] are filled from the code above. And if 
can_dlc is greater than 3 (== 4 .. 8) the following 4 bytes are copied 
into the registers.


So everything is correct with the current code.


+   if (cf->can_dlc > 4) {
data = be32_to_cpup((__be32 *)>data[4]);
flexcan_write(data, >tx_mb->data[1]);
}



Regards,
Oliver

[PATCH] ip: add vxcan/veth to ip-link man page

[Oliver Hartkopp]

veth and vxcan both create a vitual tunnel between a pair of virtual network
devices. This patch adds the content for the now supported vxcan netdevices
and the documentation to create peer devices for vxcan and veth.

Additional remove 'can' that accidently was on the list of link types which
can be created by 'ip link add' as 'can' devices are real network devices.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 man/man8/ip-link.8.in | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index a6a10e57..94ecbece 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -194,6 +194,7 @@ ip-link \- network device configuration
 .BR macvlan  " | "
 .BR macvtap  " | "
 .BR vcan " | "
+.BR vxcan " | "
 .BR veth " | "
 .BR vlan " | "
 .BR vxlan " |"
@@ -252,9 +253,6 @@ Link types:
 .B bond
 - Bonding device
 .sp
-.B can
-- Controller Area Network interface
-.sp
 .B dummy
 - Dummy network interface
 .sp
@@ -276,6 +274,9 @@ Link types:
 .B vcan
 - Virtual Controller Area Network interface
 .sp
+.B vxcan
+- Virtual Controller Area Network tunnel interface
+.sp
 .B veth
 - Virtual ethernet interface
 .sp
@@ -651,6 +652,29 @@ keyword.
 
 .in -8
 
+.TP
+VETH, VXCAN Type Support
+For a link of types
+.I VETH/VXCAN
+the following additional arguments are supported:
+
+.BI "ip link add " DEVICE
+.BR type " { " veth " | " vxcan " }"
+[
+.BR peer
+.BI "name " NAME
+]
+
+.in +8
+.sp
+.BR peer
+.BI "name " NAME
+- specifies the virtual pair device name of the
+.I VETH/VXCAN
+tunnel.
+
+.in -8
+
 .TP
 GRE, IPIP, SIT, ERSPAN Type Support
 For a link of types
-- 
2.15.1

Re: [PATCH iproute2] ip: add vxcan to help text

[Oliver Hartkopp]

On 12/14/2017 03:20 AM, Stephen Hemminger wrote:


Add missing tag 'vxcan' inside the help text which was missing in commit
efe459c76d35f ('ip: link add vxcan support').




Applied. Could you also fix the man page?



Sure!

Will take a look and send a patch.

Best,
Oliver

[PATCH iproute2] ip: add vxcan to help text

[Oliver Hartkopp]

Add missing tag 'vxcan' inside the help text which was missing in commit
efe459c76d35f ('ip: link add vxcan support').

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 ip/ipaddress.c | 2 +-
 ip/iplink.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 8057011e..f150d919 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -74,7 +74,7 @@ static void usage(void)
fprintf(stderr, "CONFFLAG  := [ home | nodad | mngtmpaddr | 
noprefixroute | autojoin ]\n");
fprintf(stderr, "LIFETIME := [ valid_lft LFT ] [ preferred_lft LFT 
]\n");
fprintf(stderr, "LFT := forever | SECONDS\n");
-   fprintf(stderr, "TYPE := { vlan | veth | vcan | dummy | ifb | macvlan | 
macvtap |\n");
+   fprintf(stderr, "TYPE := { vlan | veth | vcan | vxcan | dummy | ifb | 
macvlan | macvtap |\n");
fprintf(stderr, "  bridge | bond | ipoib | ip6tnl | ipip | sit 
| vxlan | lowpan |\n");
fprintf(stderr, "  gre | gretap | erspan | ip6gre | ip6gretap | 
ip6erspan | vti |\n");
fprintf(stderr, "  nlmon | can | bond_slave | ipvlan | geneve | 
bridge_slave |\n");
diff --git a/ip/iplink.c b/ip/iplink.c
index 0a8eb56f..e83f1477 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -109,7 +109,7 @@ void iplink_usage(void)
"\n"
"   ip link help [ TYPE ]\n"
"\n"
-   "TYPE := { vlan | veth | vcan | dummy | ifb | macvlan | 
macvtap |\n"
+   "TYPE := { vlan | veth | vcan | vxcan | dummy | ifb | 
macvlan | macvtap |\n"
"  bridge | bond | team | ipoib | ip6tnl | ipip 
| sit | vxlan |\n"
"  gre | gretap | erspan | ip6gre | ip6gretap | 
ip6erspan |\n"
"  vti | nlmon | team_slave | bond_slave | 
ipvlan | geneve |\n"
-- 
2.15.1

Re: [PATCH] slip: sl_alloc(): remove unused parameter "dev_t line"

[Oliver Hartkopp]

On 12/08/2017 12:22 PM, Marc Kleine-Budde wrote:

Hello Oliver,

I've the corresponding slcan patch already in my queue.


Excellent :-)

Thanks,
Oliver



Marc

On 12/08/2017 12:18 PM, Marc Kleine-Budde wrote:

The first and only parameter of sl_alloc() is unused, so remove it.

Fixes: 5342b77c4123 slip: ("Clean up create and destroy")
Signed-off-by: Marc Kleine-Budde 
---
  drivers/net/slip/slip.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index cc63102ca96e..8940417c30e5 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -731,7 +731,7 @@ static void sl_sync(void)
  
  
  /* Find a free SLIP channel, and link in this `tty' line. */

-static struct slip *sl_alloc(dev_t line)
+static struct slip *sl_alloc(void)
  {
int i;
char name[IFNAMSIZ];
@@ -809,7 +809,7 @@ static int slip_open(struct tty_struct *tty)
  
  	/* OK.  Find a free SLIP channel to use. */

err = -ENFILE;
-   sl = sl_alloc(tty_devnum(tty));
+   sl = sl_alloc();
if (sl == NULL)
goto err_exit;
  

Re: [v2] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 11/01/2017 08:37 PM, SF Markus Elfring wrote:


These addresses were suggested (or recommended?) by the script 
“get_maintainer.pl”.


I know.

(..)


How does this view fit to the information in the section “5) Select
the recipients for your patch” from the document “submitting-patches.rst”?


We discussed and agreed about your patch. It is clear and simple and has 
no side effects to any other driver nor other subsystems.


It was just about getting *this* patch into upstream where the Linux CAN 
ML is the right place to post - that was my suggestion. So there was no 
need to take the *standard* get_maintainer mail pump-gun again for this 
patch in order to save bandwidth, energy and peoples lifetime :-)


Regards,
Oliver

Re: [PATCH v2] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 11/01/2017 03:16 PM, SF Markus Elfring wrote:

From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Wed, 1 Nov 2017 14:56:15 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>


Acked-by: Oliver Hartkopp <socket...@hartkopp.net>

Again: Posting such a patch on linux-...@vger.kernel.org is ENOUGH!

No need to cross post such a patch additionally on

netdev@vger.kernel.org
linux-ker...@vger.kernel.org
kernel-janit...@vger.kernel.org

and to each of the maintainers

m...@pengutronix.de
w...@grandegger.com
socket...@hartkopp.net

We all subscribed the mailing list and listen to it.
That's the intention of a mailing list ...

Cross posting is not appreciated in the community.

Thanks,
Oliver


---

v2:
An approach to make two checks for a failure predicate a bit safer
was rejected on 2017-10-28.
The possibility remains to reconsider such an adjustment later again.
https://lkml.org/lkml/2017/10/28/125
https://lkml.kernel.org/r/<264b3c2b-8354-5769-639c-ac8d2fcbe...@hartkopp.net>

  drivers/net/can/vxcan.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..5d1753cfacea 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct 
net_device *dev,
netif_carrier_off(peer);
  
  	err = rtnl_configure_link(peer, ifmp);

-   if (err < 0) {
-   unregister_netdevice(peer);
-   return err;
-   }
+   if (err < 0)
+   goto unregister_network_device;
  
  	/* register first device */

if (tb[IFLA_IFNAME])
@@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct 
net_device *dev,
snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
  
  	err = register_netdevice(dev);

-   if (err < 0) {
-   unregister_netdevice(peer);
-   return err;
-   }
+   if (err < 0)
+   goto unregister_network_device;
  
  	netif_carrier_off(dev);
  
@@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,

rcu_assign_pointer(priv->peer, dev);
  
  	return 0;

+
+unregister_network_device:
+   unregister_netdevice(peer);
+   return err;
  }
  
  static void vxcan_dellink(struct net_device *dev, struct list_head *head)

Re: can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

Hi Markus,

this discussion went far beyond the original posted patch for vxcan.c

I would suggest you post your idea of the simplified error handling flow 
in vxcan.c just on linux-can ML (which is the right mailing list for CAN 
related stuff).


Thanks,
Oliver

On 10/28/2017 10:13 PM, SF Markus Elfring wrote:

Are you interested in related adjustments for a bigger code base?


No. Definitely not.


You might have noticed that I am proposing similar changes already
for other software modules.   ;-)



If you aim for the the deletion of “ < 0” for all rtnl_configure_link() users
you would need to do this consistently.


How do you see the software situation around a function
like “register_netdevice”?

Regards,
Markus

Re: can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 10/28/2017 08:33 PM, SF Markus Elfring wrote:

So if you would like to change the if-statement:


It will need a small adjustment for the shown transformation.

I was also unsure if the proposal will work in a single update step.



1. Send a patch for vxcan.c to improve the error handling flow


I am going to send a second approach for this update variant.


Ok.




2. Send a separate patch for all rtnl_configure_link() callers to unify the 
result check

Step 2 is optional ... and prepare yourself for more feedback ;-)


I am curious on how software development aspects will evolve around
desired error predicates.
Which scope did you have in mind?


Oh, I don't have any scope in mind.

I just wanted to make clear that I don't want to have a different kind 
of result handling in vxcan.c and veth.c


So if you suggest to simplify the error flow that would be ok for me.

If you want to change the semantic of the result check - this has to 
done consistently at all rtnl_configure_link() caller sites. And not 
only in vxcan.c


That's what I have in mind.

Regards,
Oliver

Re: [PATCH] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 10/28/2017 10:23 AM, SF Markus Elfring wrote:

@@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct 
net_device *dev,
   netif_carrier_off(peer);
     err = rtnl_configure_link(peer, ifmp);
-    if (err < 0) {
-    unregister_netdevice(peer);
-    return err;
-    }
+    if (err)
+    goto unregister_network_device;


You are changing semantic in the if-statement here.


I got an other software development opinion for this implementation detail.

http://elixir.free-electrons.com/linux/v4.14-rc6/source/net/core/rtnetlink.c#L2393
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/core/rtnetlink.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n2513

The success predicate for the function “rtnl_configure_link” is that
the return value is zero. I would prefer to treat other values as
an error code then.


Me not.

In rtnl_configure_link() the check is

if (err < 0)
return err;

And other calling sites as in linux/drivers/net/veth.c are checking for

(err < 0)

too.

All checks done at the calling sites should be consistent.

So if you would like to change the if-statement:

1. Send a patch for vxcan.c to improve the error handling flow
2. Send a separate patch for all rtnl_configure_link() callers to unify 
the result check


Step 2 is optional ... and prepare yourself for more feedback ;-)

Regards,
Oliver

Re: [PATCH] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

Hi Markus,

On 10/27/2017 10:30 PM, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Fri, 27 Oct 2017 22:22:24 +0200

* Add a jump target so that a bit of exception handling can be better
   reused at the end of this function.

* Adjust two condition checks.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
  drivers/net/can/vxcan.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..97f250cbc4ff 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct 
net_device *dev,
netif_carrier_off(peer);
  
  	err = rtnl_configure_link(peer, ifmp);

-   if (err < 0) {
-   unregister_netdevice(peer);
-   return err;
-   }
+   if (err)
+   goto unregister_network_device;


You are changing semantic in the if-statement here.

I would be fine with the patch if you revert that if-statement as I 
would like to stay on the behavior from veth.c in veth_newlink().


Regards,
Oliver

  
  	/* register first device */

if (tb[IFLA_IFNAME])
@@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct 
net_device *dev,
snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
  
  	err = register_netdevice(dev);

-   if (err < 0) {
-   unregister_netdevice(peer);
-   return err;
-   }
+   if (err)
+   goto unregister_network_device;
  
  	netif_carrier_off(dev);
  
@@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,

rcu_assign_pointer(priv->peer, dev);
  
  	return 0;

+
+unregister_network_device:
+   unregister_netdevice(peer);
+   return err;
  }
  
  static void vxcan_dellink(struct net_device *dev, struct list_head *head)

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

On 10/19/2017 09:54 PM, Mario Hüttel wrote:

On 10/19/2017 08:35 PM, Oliver Hartkopp wrote:



We already have this 'dsample-point' implemented in the ip tool:

$ ip link set vcan0 type can help
Usage: ip link set DEVICE type can
 [ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
 [ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
    phase-seg2 PHASE-SEG2 [ sjw SJW ] ]

 [ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |  <<-- here!
 [ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
    dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]

But AFAIK m_can is not using that value in m_can_set_bittiming().


Actually I need some clarification. The sample point of the can core is
between the two time segments.
I always thought that the "sample point" options of the ip tool are used
in the internal
calculation of the two timing segments and is therefore no individual value.


You are right.

See picture at http://www.bittiming.can-wiki.info/

Usually you can give the bitrate and the sample point (which is at 75% 
aka 0.750 by default) and then the kernel-internal bitrate calculating 
algorithm calculates the tq prop-seg phase-seg1 phase-seg2 stuff.


Alternatively you can provide the tq prop-seg phase-seg1 phase-seg2 
stuff on your own which is set to the CAN controller registers then.


For that reason my remark "m_can is not using that value" was wrong as 
m_can just uses the tq prop-seg phase-seg1 phase-seg2 stuff - either 
from the bitrate calculation or provided by the user.


Thanks for the question ;-)

Best,
Oliver

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

Hi Marc,

On 10/19/2017 01:26 PM, Marc Kleine-Budde wrote:

On 10/19/2017 01:14 PM, Oliver Hartkopp wrote:

Since we have a netlink socket interface to configure sample point, I
wonder if that should be extended to configure SSP too (or at least the
offset part of SSP)?


+1 too


The struct can_bittiming in defined in uapi, so we have to keep ABI
compatibility in mind.



Oh, this is fortunately NO problem ;-)

struct can_bittiming {
__u32 bitrate;  /* Bit-rate in bits/second */
__u32 sample_point; /* Sample point in one-tenth of a 
percent */

__u32 tq;   /* Time quanta (TQ) in nanoseconds */
__u32 prop_seg; /* Propagation segment in TQs */
__u32 phase_seg1;   /* Phase buffer segment 1 in TQs */
__u32 phase_seg2;   /* Phase buffer segment 2 in TQs */
__u32 sjw;  /* Synchronisation jump width in TQs */
__u32 brp;  /* Bit-rate prescaler */
};

So we have two of these: One for the arbitration bitrate and one 
sample_point for the data bitrate -> the 'secondary' SP -> SSP


:-)

We already have this 'dsample-point' implemented in the ip tool:

$ ip link set vcan0 type can help
Usage: ip link set DEVICE type can
[ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
[ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
  phase-seg2 PHASE-SEG2 [ sjw SJW ] ]

[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |  <<-- here!
[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]

But AFAIK m_can is not using that value in m_can_set_bittiming().


If good default values are transceiver and board specific, they can go
into the DT. We need a generic (this means driver agnostic) binding for
this. If this table needs to be tweaked for special purpose, then we can
add a netlink interface for this as well. >
Comments?


By now we calculate reasonable default values (e.g. for SP and SJW), you
can override by setting alternative values via netlink configuration.

I would tend to stay on this approach and not hide these things in DTs -
just because of someone wants to initialize his specific interface 'easier'.


If the values are not board specific, then it makes no sense to put them
into the DT.


When they are NOT(?) board specific?

Thinking about non-SoC CAN adapters with PCI and USB pushing the SSP to 
the DT looks wrong to me.


Best,
Oliver

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

On 10/19/2017 11:13 AM, Marc Kleine-Budde wrote:

On 10/19/2017 07:07 AM, Sekhar Nori wrote:




Since we have a netlink socket interface to configure sample point, I
wonder if that should be extended to configure SSP too (or at least the
offset part of SSP)?


+1 too



Sekhar is right that ideally the user should be able to set the SSP at
runtime. However, my issue is that based on my experience CAN users
expect the driver to just work the majority of times. For unique use
cases where the driver calculated values don't work then the user should
be able to override it. This should only be done for a very small
percentage of CAN users. Unless you allow DT to provide a default SSP
many users of MCAN may find that the default SSP doesn't work and must
always use runtime overrides to get anything to work. I don't think that
is a good user experience which is why I don't like the idea.


Fair enough. But not quite sure if CAN users expect CAN-FD to "just
work" without doing any bittiming related setup.


 From my point of view I'd rather buy a board from a HW vendor where
CAN-FD works, rather than a board where I have to tweak the bit-timing
for a simple CAN-FD test setup.

As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
MBit/s -> 50%". Do we need an array of tuples in general?

If good default values are transceiver and board specific, they can go
into the DT. We need a generic (this means driver agnostic) binding for
this. If this table needs to be tweaked for special purpose, then we can
add a netlink interface for this as well. >
Comments?


By now we calculate reasonable default values (e.g. for SP and SJW), you 
can override by setting alternative values via netlink configuration.


I would tend to stay on this approach and not hide these things in DTs - 
just because of someone wants to initialize his specific interface 'easier'.


One tool, one place is IMHO still the best option.

Regards,
Oliver

Re: [PATCH] can: check for null sk before deferencing it via the call to sock_net

[Oliver Hartkopp]

On 10/16/2017 06:37 PM, Josh Boyer wrote:

On Fri, Sep 8, 2017 at 1:46 PM, Oliver Hartkopp <socket...@hartkopp.net> wrote:



On 09/08/2017 05:02 PM, Colin King wrote:


From: Colin Ian King <colin.k...@canonical.com>

The assignment of net via call sock_net will dereference sk. This
is performed before a sanity null check on sk, so there could be
a potential null dereference on the sock_net call if sk is null.
Fix this by assigning net after the sk null check. Also replace
the sk == NULL with the more usual !sk idiom.

Detected by CoverityScan CID#1431862 ("Dereference before null check")

Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM
protocol")
Signed-off-by: Colin Ian King <colin.k...@canonical.com>



Acked-by: Oliver Hartkopp <socket...@hartkopp.net>


I don't see this one queued up in the net or net-next trees.  Did it
fall through the cracks or did it get queued up elsewhere?  Seems like
it's a good candidate to get into 4.14?


It definitely is!

Marc is our responsible guy for CAN related upstreams - but he seems to 
be busy as I already poked him here:


https://marc.info/?l=linux-can=150771819505097=2

If he doesn't send a pull request by beginning of next week, I would ask 
Dave to grab these patches - to get them into 4.14.


Best regards,
Oliver

Re: [PATCH net] net: enable interface alias removal via rtnl

[Oliver Hartkopp]

On 10/06/2017 08:18 PM, David Ahern wrote:

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
the attribute is 1 ("\0"). However, to remove an alias, the attribute
length must be 0 (see dev_set_alias()).


why not add a check in dev_set_alias that if len is 1 and the 1
character is '\0' it means remove the alias?


Yes. That looks indeed better than changing NLA_STRING to NLA_BINARY 
which does not really hit the point.


Nicolas, can you send an updated patch picking up David's suggestion?

Tnx & best regards,
Oliver





Let's define the type to NLA_BINARY, so that the alias can be removed.


that changes the uapi

Re: [PATCH] can: check for null sk before deferencing it via the call to sock_net

[Oliver Hartkopp]

On 09/08/2017 05:02 PM, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The assignment of net via call sock_net will dereference sk. This
is performed before a sanity null check on sk, so there could be
a potential null dereference on the sock_net call if sk is null.
Fix this by assigning net after the sk null check. Also replace
the sk == NULL with the more usual !sk idiom.

Detected by CoverityScan CID#1431862 ("Dereference before null check")

Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM protocol")
Signed-off-by: Colin Ian King <colin.k...@canonical.com>


Acked-by: Oliver Hartkopp <socket...@hartkopp.net>


Thanks Collin!


---
  net/can/bcm.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 47a8748d953a..a3791674b8ce 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1493,13 +1493,14 @@ static int bcm_init(struct sock *sk)
  static int bcm_release(struct socket *sock)
  {
struct sock *sk = sock->sk;
-   struct net *net = sock_net(sk);
+   struct net *net;
struct bcm_sock *bo;
struct bcm_op *op, *next;
  
-	if (sk == NULL)

+   if (!sk)
return 0;
  
+	net = sock_net(sk);

bo = bcm_sk(sk);
  
  	/* remove bcm_ops, timer, rx_unregister(), etc. */

Re: [PATCH 00/27] ip: add -json support to 'ip link show'

[Oliver Hartkopp]

Hi Julien,

On 08/03/2017 05:54 PM, Julien Fortin wrote:

From: Julien Fortin 


what about

link_veth.c
iplink_vcan.c
iplink_vxcan.c

??

Regards,
Oliver



This patch series adds json support to 'ip [-details] link show [dev DEV]'
Each patch describes the json schema it adds and provides some examples.

Julien Fortin (27):
   color: add new COLOR_NONE and disable_color function
   ip: add new command line argument -json (mutually exclusive with
 -color)
   json_writer: add new json handlers (null, float with format, lluint,
 hu)
   ip: ip_print: add new API to print JSON or regular format output
   ip: ipaddress.c: add support for json output
   ip: iplink.c: open/close json object for ip -brief -json link show dev
 DEV
   ip: iplink_bond.c: add json output support
   ip: iplink_bond_slave.c: add json output support (info_slave_data)
   ip: iplink_hsr.c: add json output support
   ip: iplink_bridge.c: add json output support
   ip: iplink_bridge_slave.c: add json output support
   ip: iplink_can.c: add json output support
   ip: iplink_geneve.c: add json output support
   ip: iplink_ipoib.c: add json output support
   ip: iplink_ipvlan.c: add json output support
   ip: iplink_vrf.c: add json output support
   ip: iplink_vxlan.c: add json output support
   ip: iplink_xdp.c: add json output support
   ip: ipmacsec.c: add json output support
   ip: link_gre.c: add json output support
   ip: link_gre6.c: add json output support
   ip: link_ip6tnl.c: add json output support
   ip: link_iptnl.c: add json output support
   ip: link_vti.c: add json output support
   ip: link_vti6.c: add json output support
   ip: link_macvlan.c: add json output support
   ip: iplink_vlan.c: add json output support

  include/color.h  |2 +
  include/json_writer.h|9 +
  include/utils.h  |1 +
  ip/Makefile  |2 +-
  ip/ip.c  |6 +
  ip/ip_common.h   |   56 +++
  ip/ip_print.c|  233 ++
  ip/ipaddress.c   | 1089 --
  ip/iplink.c  |2 +
  ip/iplink_bond.c |  231 +++---
  ip/iplink_bond_slave.c   |   57 ++-
  ip/iplink_bridge.c   |  291 -
  ip/iplink_bridge_slave.c |  185 +---
  ip/iplink_can.c  |  276 +---
  ip/iplink_geneve.c   |   86 +++-
  ip/iplink_hsr.c  |   36 +-
  ip/iplink_ipoib.c|   30 +-
  ip/iplink_ipvlan.c   |8 +-
  ip/iplink_macvlan.c  |   37 +-
  ip/iplink_vlan.c |   62 ++-
  ip/iplink_vrf.c  |   13 +-
  ip/iplink_vxlan.c|  161 ---
  ip/iplink_xdp.c  |   31 +-
  ip/ipmacsec.c|   84 +++-
  ip/link_gre.c|  147 ---
  ip/link_gre6.c   |  142 --
  ip/link_ip6tnl.c |  172 +---
  ip/link_iptnl.c  |  155 ---
  ip/link_vti.c|   23 +-
  ip/link_vti6.c   |   22 +-
  lib/color.c  |9 +-
  lib/json_writer.c|   44 +-
  32 files changed, 2668 insertions(+), 1034 deletions(-)
  create mode 100644 ip/ip_print.c

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

Hi Kurt,

On 07/28/2017 09:41 PM, Kurt Van Dijck wrote:


The transceiver is an analog device that needs to support faster
switching frequency (FETs) including minimizing delay to support CAN-FD
ie higher bitrate. From the transceiver perspective the bits for
"arbitration" and "data" look exactly the same. Since it can't
differentiate between the two (at the physical layer) then the actual
limit isn't specific to which part/type of the CAN message is being
sent. Rather its just a single overall max bitrate limit.


I must disagree here.
The transceiver is an analog device that performs 2 functions:
propagate tx bits to CAN wire, and propagate CAN wire state
(dominant/recesive) to rx bits.

I'll rephrase the above explanation to fit your argument:
During arbitration, both directions are required, and needs to propagate
within 1 bit time. The transceiver doesn't know, it just performs to
best effort.
During data, the round-trip timing requirement of layer2 is relaxed.
The transceiver still doesn't know, it still performs to best effort.
Due to the relaxed round-trip timing requirement, the same transceiver
can suddenly allow higher bitrates. The transceiver didn't change, the
requirement did change.
This is what I meant earlier with "layer2 has been adapted to circumvent
layer1 limitations"

Was I successfull in transcoding my thoughts onto email :-) ?


Yes. But it's not relevant.

I talked to our CAN transceiver & CAN physical layer specialist who was 
involved in the ISO activities too.


We just need ONE value: max-bitrate

The CAN transceiver is qualified to provide this bitrate. That's it.
There's nothing special with the arbitration bitrate - we don't even 
need to outline any CAN FD capability.


The other things like 'loop delay compensation' are done in the CAN 
controller. The better the transceiver get's the bits 'in shape' the 
higher it can be qualified. But from the SoC/Controller/Linux view we 
only need the max-bitrate value to double check with the CAN controllers 
bitrate configuration request (which was Franklins intention).


Best regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

Hi Kurt,

On 07/28/2017 03:02 PM, Kurt Van Dijck wrote:


The word 'max-arbitration-bitrate' makes the difference very clear.


I think you are mixing up ISO layer 1 and ISO layer 2.


In order to provide higher data throughput without putting extra limits
on transceiver & wire, the requirement for the round-trip delay to be
within 1 bittime has been eliminated, but only for the data phase when
arbitration is over.
So layer 2 (CAN FD) has been adapted to circumvent the layer 1
(transceiver + wire) limitations.

In fact, the round-trip delay requirement never actually did matter for
plain CAN during data bits either. CAN FD just makes use of that,
but is therefore incompatible on the wire.

I forgot the precise wording, but this is the principle that Bosch
explained on the CAN conference in Nurnberg several years ago, or at
least this is how I remembered it :-)


I just checked an example for a CAN FD qualified transceiver

http://www.nxp.com/products/automotive-products/energy-power-management/can-transceivers/high-speed-can-transceiver-with-standby-mode:TJA1044

where it states:

The TJA1044T is specified for data rates up to 1 Mbit/s. Pending the 
release of ISO11898-2:2016 including CAN FD and SAE-J2284-4/5, 
additional timing parameters defining loop delay symmetry are specified 
for the TJA1044GT and TJA1044GTK. This implementation enables reliable 
communication in the CAN FD fast phase at data rates up to 5 Mbit/s.


and

TJA1044GT/TJA1044GTK

- Timing guaranteed for data rates up to 5 Mbit/s
- Improved TXD to RXD propagation delay of 210 ns


I haven't followed the developments of transceivers, but with the above
principle in mind, it's obvious that any transceiver allows higher
bitrates during the data segment because the TX-to-RX line delay must
not scale with the bitrate.
In reality, maybe not all transceivers will mention this in their
datasheet.

So whether you call it 'max-arbitration-bitrate' & 'max-data-bitrate'
or 'max-bitrate' & 'max-data-bitrate' does not really matter (I prefer
1st) but you will one day need 2 bitrates.


The question to me is whether it is right option to specify two bitrates 
OR to specify one maximum bitrate and provide a property that a CAN FD 
capable propagation delay is available.


E.g.

max-bitrate
max-data-bitrate

or

max-bitrate
canfd-capable   // CAN FD capable propagation delay available


I assume the optimized propagation delay is 'always on' as the 
transceiver is not able to detect which kind of bits it is processing.
That's why I think providing two bitrates leads to a wrong view on the 
transceiver.


Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/28/2017 06:57 AM, Kurt Van Dijck wrote:


So while _a_ transceiver may be spec'd to 1MBit during arbitration,
CAN FD packets may IMHO exceed that speed during data phase.


When the bitrate is limited to 1Mbit/s you are ONLY allowed to use 
1Mbit/s in the data section too (either with CAN or CAN FD).



That was the whole point of CAN FD: exceed the limits required for
correct arbitration on transceiver & wire.


No. CAN FD is about a different frame format with up to 64 bytes AND the 
possibility to increase the bitrate in the data section of the frame.



So I do not agree on the single bandwidth limitation.


The transceiver provides a single maximum bandwidth. It's an ISO Layer 1 
device.



The word 'max-arbitration-bitrate' makes the difference very clear.


I think you are mixing up ISO layer 1 and ISO layer 2.

Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:





I'm fine with switching to using bitrate instead of speed. Kurk was
originally the one that suggested to use the term arbitration and data
since thats how the spec refers to it. Which I do agree with. But your
right that in the drivers (struct can_priv) we just use bittiming and
data_bittiming (CAN-FD timings). I don't think adding "fd" into the
property name makes sense unless we are calling it something like
"max-canfd-bitrate" which I would agree is the easiest to understand.

So what is the preference if we end up sticking with two properties?
Option 1 or 2?

1)
max-bitrate
max-data-bitrate

2)
max-bitrate
max-canfd-bitrate




1


A CAN transceiver is limited in bandwidth. But you only have one RX and
one TX line between the CAN controller and the CAN transceiver. The
transceiver does not know about CAN FD - it has just a physical(!) layer
with a limited bandwidth. This is ONE limitation.

So I tend to specify only ONE 'max-bitrate' property for the
fixed-transceiver binding.

The fact whether the CAN controller is CAN FD capable or not is provided
by the netlink configuration interface for CAN controllers.


Part of the reasoning to have two properties is to indicate that you
don't support CAN FD while limiting the "arbitration" bit rate.


??

It's a physical layer device which only has a bandwidth limitation.
The transceiver does not know about CAN FD.


With one
property you can not determine this and end up having to make some
assumptions that can quickly end up biting people.


Despite the fact that the transceiver does not know anything about ISO 
layer 2 (CAN/CAN FD) the properties should look like


max-bitrate
canfd-capable

then.

But when the tranceiver is 'canfd-capable' agnostic, why provide a 
property for it?


Maybe I'm wrong but I still can't follow your argumentation ideas.

Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/26/2017 06:41 PM, Andrew Lunn wrote:

On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote:



+
+Optional:
+ max-arbitration-speed: a positive non 0 value that determines the max
+   speed that CAN can run in non CAN-FD mode or during the
+   arbitration phase in CAN-FD mode.


Hi Franklin

Since this and the next property are optional, it is good to document
what happens when they are not in the DT blob. Also document what 0
means.


+
+ max-data-speed:   a positive non 0 value that determines the max data rate
+   that can be used in CAN-FD mode. A value of -1 implies
+   CAN-FD is not supported by the transceiver.


-1 is ugly. I think it would be better to have a missing
max-data-speed property indicate that CAN-FD is not supported.


Thanks Andrew! I had the same feeling about '-1' :-)


And
maybe put 'fd' into the property name.


Good point. In fact the common naming to set bitrates for CAN(FD) 
controllers are 'bitrate' and 'data bitrate'.


'speed' is not really a good word for that.

Finally, @Franklin:

A CAN transceiver is limited in bandwidth. But you only have one RX and 
one TX line between the CAN controller and the CAN transceiver. The 
transceiver does not know about CAN FD - it has just a physical(!) layer 
with a limited bandwidth. This is ONE limitation.


So I tend to specify only ONE 'max-bitrate' property for the 
fixed-transceiver binding.


The fact whether the CAN controller is CAN FD capable or not is provided 
by the netlink configuration interface for CAN controllers.


Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

+ max-data-speed:   a positive non 0 value that determines the max data rate
+   that can be used in CAN-FD mode. A value of -1 implies
+   CAN-FD is not supported by the transceiver.
+
+Examples:


(..)


+   fixed-transceiver {
+   max-data-speed = <(-1)>;


Looks ugly IMHO.

Why didn't you stay on '0' for 'not supported'??

Regards,
Oliver

Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate

[Oliver Hartkopp]

Hi Franklin,

On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:


+#ifdef CONFIG_OF
+void of_transceiver_is_fixed(struct net_device *dev)
+{


(..)


+}
+EXPORT_SYMBOL(of_transceiver_is_fixed);
+#endif


I'm not sure about the naming here.

As this is a CAN transceiver related option it should be named accordingly:

E.g.

can_transceiver_is_fixed
of_can_transceiver_is_fixed
...

Especially as it is defined in include/linux/can/dev.h

Regards,
Oliver

Re: [PATCH net-next 2/2] af_iucv: Move sockaddr length checks to before accessing sa_family in bind and connect handlers

[Oliver Hartkopp]

On 06/23/2017 07:32 PM, Julian Wiedmann wrote:
> From: Mateusz Jurczyk 
> 
> Verify that the caller-provided sockaddr structure is large enough to
> contain the sa_family field, before accessing it in bind() and connect()
> handlers of the AF_IUCV socket. Since neither syscall enforces a minimum
> size of the corresponding memory region, very short sockaddrs (zero or
> one byte long) result in operating on uninitialized memory while
> referencing .sa_family.

Won't it make sense to generally check the minimum length for .sa_family at a
single point before fixing all called sites?

Regards,
Oliver

> 
> Fixes: 52a82e23b9f2 ("af_iucv: Validate socket address length in 
> iucv_sock_bind()")
> Signed-off-by: Mateusz Jurczyk 
> [jwi: removed unneeded null-check for addr]
> Signed-off-by: Julian Wiedmann 
> ---
>  net/iucv/af_iucv.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 05112094d76b..ac033e413bc5 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -715,10 +715,8 @@ static int iucv_sock_bind(struct socket *sock, struct 
> sockaddr *addr,
>   char uid[9];
>  
>   /* Verify the input sockaddr */
> - if (!addr || addr->sa_family != AF_IUCV)
> - return -EINVAL;
> -
> - if (addr_len < sizeof(struct sockaddr_iucv))
> + if (addr_len < sizeof(struct sockaddr_iucv) ||
> + addr->sa_family != AF_IUCV)
>   return -EINVAL;
>  
>   lock_sock(sk);
> @@ -862,7 +860,7 @@ static int iucv_sock_connect(struct socket *sock, struct 
> sockaddr *addr,
>   struct iucv_sock *iucv = iucv_sk(sk);
>   int err;
>  
> - if (addr->sa_family != AF_IUCV || alen < sizeof(struct sockaddr_iucv))
> + if (alen < sizeof(struct sockaddr_iucv) || addr->sa_family != AF_IUCV)
>   return -EINVAL;
>  
>   if (sk->sk_state != IUCV_OPEN && sk->sk_state != IUCV_BOUND)
> 

[PATCH] ip: link add vxcan support

[Oliver Hartkopp]

Since commit a8f820a380a2a06 ('can: add Virtual CAN Tunnel driver (vxcan)')
for Linux 4.12 a virtual CAN tunnel driver analogue to veth is available in
Linux.

This patch adds the ability to create vxcan device pairs.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 include/linux/can/vxcan.h | 12 ++
 ip/Makefile   |  2 +-
 ip/iplink_vxcan.c | 99 +++
 3 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/can/vxcan.h
 create mode 100644 ip/iplink_vxcan.c

diff --git a/include/linux/can/vxcan.h b/include/linux/can/vxcan.h
new file mode 100644
index ..ffb0b715
--- /dev/null
+++ b/include/linux/can/vxcan.h
@@ -0,0 +1,12 @@
+#ifndef _UAPI_CAN_VXCAN_H
+#define _UAPI_CAN_VXCAN_H
+
+enum {
+   VXCAN_INFO_UNSPEC,
+   VXCAN_INFO_PEER,
+
+   __VXCAN_INFO_MAX
+#define VXCAN_INFO_MAX (__VXCAN_INFO_MAX - 1)
+};
+
+#endif
diff --git a/ip/Makefile b/ip/Makefile
index e08c170a..8424b1f6 100644
--- a/ip/Makefile
+++ b/ip/Makefile
@@ -2,7 +2,7 @@ IPOBJ=ip.o ipaddress.o ipaddrlabel.o iproute.o iprule.o 
ipnetns.o \
 rtm_map.o iptunnel.o ip6tunnel.o tunnel.o ipneigh.o ipntable.o iplink.o \
 ipmaddr.o ipmonitor.o ipmroute.o ipprefix.o iptuntap.o iptoken.o \
 ipxfrm.o xfrm_state.o xfrm_policy.o xfrm_monitor.o iplink_dummy.o \
-iplink_ifb.o iplink_nlmon.o iplink_team.o iplink_vcan.o \
+iplink_ifb.o iplink_nlmon.o iplink_team.o iplink_vcan.o iplink_vxcan.o \
 iplink_vlan.o link_veth.o link_gre.o iplink_can.o iplink_xdp.o \
 iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o \
 iplink_vxlan.o tcp_metrics.o iplink_ipoib.o ipnetconf.o link_ip6tnl.o \
diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c
new file mode 100644
index ..680f640f
--- /dev/null
+++ b/ip/iplink_vxcan.c
@@ -0,0 +1,99 @@
+/*
+ * iplink_vxcan.c  vxcan device support (Virtual CAN Tunnel)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Author: Oliver Hartkopp <socket...@hartkopp.net>
+ * Based on:   link_veth.c from Pavel Emelianov <xe...@openvz.org>
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "utils.h"
+#include "ip_common.h"
+
+static void print_usage(FILE *f)
+{
+   printf("Usage: ip link  type vxcan [peer ]\n"
+  "To get  type 'ip link add help'\n");
+}
+
+static void usage(void)
+{
+   print_usage(stderr);
+}
+
+static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
+ struct nlmsghdr *hdr)
+{
+   char *dev = NULL;
+   char *name = NULL;
+   char *link = NULL;
+   char *type = NULL;
+   int index = 0;
+   int err, len;
+   struct rtattr *data;
+   int group;
+   struct ifinfomsg *ifm, *peer_ifm;
+   unsigned int ifi_flags, ifi_change;
+
+   if (strcmp(argv[0], "peer") != 0) {
+   usage();
+   return -1;
+   }
+
+   ifm = NLMSG_DATA(hdr);
+   ifi_flags = ifm->ifi_flags;
+   ifi_change = ifm->ifi_change;
+   ifm->ifi_flags = 0;
+   ifm->ifi_change = 0;
+
+   data = NLMSG_TAIL(hdr);
+   addattr_l(hdr, 1024, VXCAN_INFO_PEER, NULL, 0);
+
+   hdr->nlmsg_len += sizeof(struct ifinfomsg);
+
+   err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)hdr,
+  , , , , , );
+   if (err < 0)
+   return err;
+
+   if (name) {
+   len = strlen(name) + 1;
+   if (len > IFNAMSIZ)
+   invarg("\"name\" too long\n", *argv);
+   addattr_l(hdr, 1024, IFLA_IFNAME, name, len);
+   }
+
+   peer_ifm = RTA_DATA(data);
+   peer_ifm->ifi_index = index;
+   peer_ifm->ifi_flags = ifm->ifi_flags;
+   peer_ifm->ifi_change = ifm->ifi_change;
+   ifm->ifi_flags = ifi_flags;
+   ifm->ifi_change = ifi_change;
+
+   if (group != -1)
+   addattr32(hdr, 1024, IFLA_GROUP, group);
+
+   data->rta_len = (void *)NLMSG_TAIL(hdr) - (void *)data;
+   return argc - 1 - err;
+}
+
+static void vxcan_print_help(struct link_util *lu, int argc, char **argv,
+   FILE *f)
+{
+   print_usage(f);
+}
+
+struct link_util vxcan_link_util = {
+   .id = "vxcan",
+   .parse_opt = vxcan_parse_opt,
+   .print_help = vxcan_print_help,
+};
-- 
2.11.0

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Oliver Hartkopp]

Hi Alexandre,

On 05/16/2017 01:00 AM, Alexandre Belloni wrote:

Hi,

On 15/05/2017 at 20:51:30 -0700, Oliver Hartkopp wrote:

On 05/15/2017 06:50 AM, Marc Kleine-Budde wrote:

Looks good, added to linux-can-next.


Isn't this a fix for linux-can instead?

At least it would make no sense to me to have the upgraded M_CAN driver in
Linux 4.12 without this fix.



The related suspend mode on the sama5d2 is not present in v4.12 so I
think this can wait v4.13.



Agreed!

Thanks for the explanation.

Regards,
Oliver

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Oliver Hartkopp]

On 05/15/2017 06:50 AM, Marc Kleine-Budde wrote:

On 05/12/2017 08:37 AM, Quentin Schulz wrote:

Hi all,

On 05/05/2017 15:50, Quentin Schulz wrote:

To avoid possible ECC/parity checksum errors when reading an
uninitialized buffer, the entire Message RAM is initialized when probing
the driver. This initialization is done in the same function reading the
Device Tree properties.

This patch moves the RAM initialization to a separate function so it can
be called separately from device initialization from Device Tree.

Signed-off-by: Quentin Schulz 


It's been a week since I sent this patch series. Any comments?


Looks good, added to linux-can-next.


Isn't this a fix for linux-can instead?

At least it would make no sense to me to have the upgraded M_CAN driver 
in Linux 4.12 without this fix.


Regards,
Oliver

Re: [PATCH net-next] can: fix build error without CONFIG_PROC_FS

[Oliver Hartkopp]

Hello Arnd,

many thanks for your patch.

Btw

>  static void canbcm_pernet_exit(struct net *net)
>  {
> +#ifdef CONFIG_PROC_FS
>/* remove /proc/net/can-bcm directory */
>if (IS_ENABLED(CONFIG_PROC_FS)) {
>if (net->can.bcmproc_dir)
>remove_proc_entry("can-bcm", net->proc_net);
>}
> +#endif
>  }

"if (IS_ENABLED(CONFIG_PROC_FS))"

becomes obsolete too then ...

So I would suggest to take my patch to fix my fault ;-)

Best regards,
Oliver

On 04/27/2017 04:29 PM, Marc Kleine-Budde wrote:

Hello Arnd,

On 04/27/2017 04:21 PM, Arnd Bergmann wrote:

The procfs dir entry was added inside of an #ifdef, causing a build error
when we try to access it without CONFIG_PROC_FS set:

net/can/bcm.c:1541:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
net/can/bcm.c: In function 'bcm_connect':
net/can/bcm.c:1601:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
net/can/bcm.c: In function 'canbcm_pernet_init':
net/can/bcm.c:1696:11: error: 'struct netns_can' has no member named 
'bcmproc_dir'
net/can/bcm.c: In function 'canbcm_pernet_exit':
net/can/bcm.c:1707:15: error: 'struct netns_can' has no member named 
'bcmproc_dir'

This adds the same #ifdef around all users of the pointer. Alternatively
we could move the pointer outside of the #ifdef.

Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM protocol")
Signed-off-by: Arnd Bergmann 


A fix for this problem is part of the pull request I send to David
earlier today:

https://www.mail-archive.com/netdev@vger.kernel.org/msg165764.html

regards,
Marc

Re: linux-next: Tree for Apr 26 (net/can/bcm.c)

[Oliver Hartkopp]

Hi Randy,

thanks for the report!

Some fallout of my namespace support integration %-)

I posted a patch for it:

http://marc.info/?l=linux-can=149323049630039=2

Many thanks & best regards,
Oliver

On 04/26/2017 04:53 PM, Randy Dunlap wrote:

On 04/26/17 01:03, Stephen Rothwell wrote:

Hi all,

Changes since 20170424:



on x86_64:

when CONFIG_PROC_FS is not enabled:

../net/can/bcm.c:1541:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1601:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1696:11: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1707:15: error: 'struct netns_can' has no member named 
'bcmproc_dir'

2 of those are "protected" by
if (IS_ENABLED(CONFIG_PROC_FS)) {
but that doesn't seem to help/work here.

gcc v4.8.5

[PATCH net-next] can: fix CAN BCM build with CONFIG_PROC_FS disabled

[Oliver Hartkopp]

The introduced namespace support moved the BCM variables for procfs into a
per-net data structure. This leads to a build failure with disabled procfs:

on x86_64:

when CONFIG_PROC_FS is not enabled:

../net/can/bcm.c:1541:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1601:14: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1696:11: error: 'struct netns_can' has no member named 
'bcmproc_dir'
../net/can/bcm.c:1707:15: error: 'struct netns_can' has no member named 
'bcmproc_dir'

http://marc.info/?l=linux-can=149321842526524=2

Reported-by: Randy Dunlap <rdun...@infradead.org>
Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/bcm.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 0e855917b7e1..78409841b74c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -147,6 +147,7 @@ static inline ktime_t bcm_timeval_to_ktime(struct 
bcm_timeval tv)
 /*
  * procfs functions
  */
+#if IS_ENABLED(CONFIG_PROC_FS)
 static char *bcm_proc_getifname(struct net *net, char *result, int ifindex)
 {
struct net_device *dev;
@@ -251,6 +252,7 @@ static const struct file_operations bcm_proc_fops = {
.llseek = seq_lseek,
.release= single_release,
 };
+#endif /* CONFIG_PROC_FS */
 
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
@@ -1537,9 +1539,11 @@ static int bcm_release(struct socket *sock)
bcm_remove_op(op);
}
 
+#if IS_ENABLED(CONFIG_PROC_FS)
/* remove procfs entry */
if (net->can.bcmproc_dir && bo->bcm_proc_read)
remove_proc_entry(bo->procname, net->can.bcmproc_dir);
+#endif /* CONFIG_PROC_FS */
 
/* remove device reference */
if (bo->bound) {
@@ -1598,6 +1602,7 @@ static int bcm_connect(struct socket *sock, struct 
sockaddr *uaddr, int len,
bo->ifindex = 0;
}
 
+#if IS_ENABLED(CONFIG_PROC_FS)
if (net->can.bcmproc_dir) {
/* unique socket address as filename */
sprintf(bo->procname, "%lu", sock_i_ino(sk));
@@ -1609,6 +1614,7 @@ static int bcm_connect(struct socket *sock, struct 
sockaddr *uaddr, int len,
goto fail;
}
}
+#endif /* CONFIG_PROC_FS */
 
bo->bound = 1;
 
@@ -1691,22 +1697,22 @@ static const struct can_proto bcm_can_proto = {
 
 static int canbcm_pernet_init(struct net *net)
 {
+#if IS_ENABLED(CONFIG_PROC_FS)
/* create /proc/net/can-bcm directory */
-   if (IS_ENABLED(CONFIG_PROC_FS)) {
-   net->can.bcmproc_dir =
-   proc_net_mkdir(net, "can-bcm", net->proc_net);
-   }
+   net->can.bcmproc_dir =
+   proc_net_mkdir(net, "can-bcm", net->proc_net);
+#endif /* CONFIG_PROC_FS */
 
return 0;
 }
 
 static void canbcm_pernet_exit(struct net *net)
 {
+#if IS_ENABLED(CONFIG_PROC_FS)
/* remove /proc/net/can-bcm directory */
-   if (IS_ENABLED(CONFIG_PROC_FS)) {
-   if (net->can.bcmproc_dir)
-   remove_proc_entry("can-bcm", net->proc_net);
-   }
+   if (net->can.bcmproc_dir)
+   remove_proc_entry("can-bcm", net->proc_net);
+#endif /* CONFIG_PROC_FS */
 }
 
 static struct pernet_operations canbcm_pernet_ops __read_mostly = {
-- 
2.11.0

Re: [PATCH 0/8] Fix and complete CAN namespace support

[Oliver Hartkopp]

On 04/25/2017 10:45 AM, Marc Kleine-Budde wrote:


FYI:
This series is included in the latest pull request:

git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git
tags/linux-can-next-for-4.12-20170425



Ok, thanks!

Sorry for pushing a bit harder this time. But I wanted to make sure that 
we don't get incomplete stuff into the merge window.


Regards,
Oliver

Re: [PATCH 0/8] Fix and complete CAN namespace support

[Oliver Hartkopp]

On 04/25/2017 08:48 AM, Marc Kleine-Budde wrote:

On 04/25/2017 08:19 AM, Oliver Hartkopp wrote:

Hello Dave,

unfortunately the initial network namespace support by Mario Kicherer
(8e8cda6d737d) slipped into net-next without further review and Marc pushed
the code without my Acked-by. Due to the fact that this code was in net-next
now I spent some nights to fix, clean up, finalize and test the missing pieces
for the namespace support for the CAN subsystem in net/can.

As Marc is currently *VERY* unresponsive on the mailing list due to his 'real'


... and holidays :) But I'm back in the office now.


I created a cleaned patch series to provide more detailed and proper 
patches. And these patches are based on Dave's net-next as you did not 
push your latest changes in mkl/linux-can-next to the public on week ago.


When you manage to get this series integrated today then it would be ok 
for me. Otherwise we should Dave take this series directly.


Thanks,
Oliver

[PATCH 7/8] can: add Virtual CAN Tunnel driver (vxcan)

[Oliver Hartkopp]

Similar to the virtual ethernet driver veth, vxcan implements a
local CAN traffic tunnel between two virtual CAN network devices.
See Kconfig entry for details.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 drivers/net/can/Kconfig|  18 +++
 drivers/net/can/Makefile   |   1 +
 drivers/net/can/vxcan.c| 316 +
 include/uapi/linux/can/vxcan.h |  12 ++
 4 files changed, 347 insertions(+)
 create mode 100644 drivers/net/can/vxcan.c
 create mode 100644 include/uapi/linux/can/vxcan.h

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 22570ea3a8d2..61c2fcf7f880 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -9,6 +9,24 @@ config CAN_VCAN
  This driver can also be built as a module.  If so, the module
  will be called vcan.
 
+config CAN_VXCAN
+   tristate "Virtual CAN Tunnel (vxcan)"
+   ---help---
+ Similar to the virtual ethernet driver veth, vxcan implements a
+ local CAN traffic tunnel between two virtual CAN network devices.
+ When creating a vxcan, two vxcan devices are created as pair.
+ When one end receives the packet it appears on its pair and vice
+ versa. The vxcan can be used for cross namespace communication.
+
+ In opposite to vcan loopback devices the vxcan only forwards CAN
+ frames to its pair and does *not* provide a local echo of sent
+ CAN frames. To disable a potential echo in af_can.c the vxcan driver
+ announces IFF_ECHO in the interface flags. To have a clean start
+ in each namespace the CAN GW hop counter is set to zero.
+
+ This driver can also be built as a module.  If so, the module
+ will be called vxcan.
+
 config CAN_SLCAN
tristate "Serial / USB serial CAN Adaptors (slcan)"
depends on TTY
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 0da4f2f5c7e3..18cc6543b711 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_CAN_VCAN) += vcan.o
+obj-$(CONFIG_CAN_VXCAN)+= vxcan.o
 obj-$(CONFIG_CAN_SLCAN)+= slcan.o
 
 obj-$(CONFIG_CAN_DEV)  += can-dev.o
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
new file mode 100644
index ..7fbb24795681
--- /dev/null
+++ b/drivers/net/can/vxcan.c
@@ -0,0 +1,316 @@
+/*
+ * vxcan.c - Virtual CAN Tunnel for cross namespace communication
+ *
+ * This code is derived from drivers/net/can/vcan.c for the virtual CAN
+ * specific parts and from drivers/net/veth.c to implement the netlink API
+ * for network interface pairs in a common and established way.
+ *
+ * Copyright (c) 2017 Oliver Hartkopp <socket...@hartkopp.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME "vxcan"
+
+MODULE_DESCRIPTION("Virtual CAN Tunnel");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Oliver Hartkopp <socket...@hartkopp.net>");
+MODULE_ALIAS_RTNL_LINK(DRV_NAME);
+
+struct vxcan_priv {
+   struct net_device __rcu *peer;
+};
+
+static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+   struct vxcan_priv *priv = netdev_priv(dev);
+   struct net_device *peer;
+   struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+   struct net_device_stats *peerstats, *srcstats = >stats;
+
+   if (can_dropped_invalid_skb(dev, skb))
+   return NETDEV_TX_OK;
+
+   rcu_read_lock();
+   peer = rcu_dereference(priv->peer);
+   if (unlikely(!peer)) {
+   kfree_skb(skb);
+   dev->stats.tx_dropped++;
+   goto out_unlock;
+   }
+
+   skb = can_create_echo_skb(skb);
+   if (!skb)
+   goto out_unlock;
+
+   /* reset CAN GW hop counter */
+   skb->csum_start = 0;
+   skb->pkt_type   = PACKET_BROADCAST;
+   skb->dev= peer;
+   skb->ip_summed  = CHECKSUM_UNNECESSARY;
+
+   if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
+   srcstats->tx_packets++;
+   srcstats->tx_bytes += cfd->len;
+   peerstats = >stats;
+   pe

[PATCH 3/8] can: remove obsolete definitions

[Oliver Hartkopp]

can_rx_alldev_list is a per-net data structure now. Remove it's definition
here and can_rx_dev_list too.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/af_can.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/net/can/af_can.h b/net/can/af_can.h
index f273c9d9b129..84a35e97c5e0 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -110,9 +110,6 @@ struct s_pstats {
unsigned long rcv_entries_max;
 };
 
-/* receive filters subscribed for 'all' CAN devices */
-extern struct dev_rcv_lists can_rx_alldev_list;
-
 /* function prototypes for the CAN networklayer procfs (proc.c) */
 void can_init_proc(struct net *net);
 void can_remove_proc(struct net *net);
@@ -122,6 +119,5 @@ void can_stat_update(unsigned long data);
 extern struct timer_list can_stattimer;/* timer for statistics update */
 extern struct s_statscan_stats;/* packet statistics */
 extern struct s_pstats   can_pstats;   /* receive list statistics */
-extern struct hlist_head can_rx_dev_list;  /* rx dispatcher structures */
 
 #endif /* AF_CAN_H */
-- 
2.11.0

[PATCH 1/8] can: fix memory leak in initial namespace support

[Oliver Hartkopp]

The can_rx_alldev_list is a per-net data structure now and allocated in
can_pernet_init(). Make sure the memory is free'd in can_pernet_exit() too.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/af_can.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index abf7d854a94d..2c935babe466 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -903,6 +903,8 @@ static void can_pernet_exit(struct net *net)
}
}
rcu_read_unlock();
+
+   kfree(net->can.can_rx_alldev_list);
 }
 
 /*
-- 
2.11.0

[PATCH 5/8] can: network namespace support for CAN_BCM protocol

[Oliver Hartkopp]

The CAN_BCM protocol and its procfs entries were not implemented as per-net
in the initial network namespace support by Mario Kicherer (8e8cda6d737d).
This patch adds the missing per-net functionality for the CAN BCM.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 include/net/netns/can.h |  1 +
 net/can/bcm.c   | 90 +++--
 2 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 574157dbc43a..0f3c31aab8a8 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -23,6 +23,7 @@ struct netns_can {
struct proc_dir_entry *pde_rcvlist_sff;
struct proc_dir_entry *pde_rcvlist_eff;
struct proc_dir_entry *pde_rcvlist_err;
+   struct proc_dir_entry *bcmproc_dir;
 #endif
 
/* receive filters subscribed for 'all' CAN devices */
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 1976629a8463..0e855917b7e1 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1,7 +1,7 @@
 /*
  * bcm.c - Broadcast Manager to filter/send (cyclic) CAN content
  *
- * Copyright (c) 2002-2016 Volkswagen Group Electronic Research
+ * Copyright (c) 2002-2017 Volkswagen Group Electronic Research
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -77,7 +77,7 @@
 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
-#define CAN_BCM_VERSION "20161123"
+#define CAN_BCM_VERSION "20170425"
 
 MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
 MODULE_LICENSE("Dual BSD/GPL");
@@ -118,8 +118,6 @@ struct bcm_op {
struct net_device *rx_reg_dev;
 };
 
-static struct proc_dir_entry *proc_dir;
-
 struct bcm_sock {
struct sock sk;
int bound;
@@ -149,7 +147,7 @@ static inline ktime_t bcm_timeval_to_ktime(struct 
bcm_timeval tv)
 /*
  * procfs functions
  */
-static char *bcm_proc_getifname(char *result, int ifindex)
+static char *bcm_proc_getifname(struct net *net, char *result, int ifindex)
 {
struct net_device *dev;
 
@@ -157,7 +155,7 @@ static char *bcm_proc_getifname(char *result, int ifindex)
return "any";
 
rcu_read_lock();
-   dev = dev_get_by_index_rcu(_net, ifindex);
+   dev = dev_get_by_index_rcu(net, ifindex);
if (dev)
strcpy(result, dev->name);
else
@@ -170,7 +168,8 @@ static char *bcm_proc_getifname(char *result, int ifindex)
 static int bcm_proc_show(struct seq_file *m, void *v)
 {
char ifname[IFNAMSIZ];
-   struct sock *sk = (struct sock *)m->private;
+   struct net *net = m->private;
+   struct sock *sk = (struct sock *)PDE_DATA(m->file->f_inode);
struct bcm_sock *bo = bcm_sk(sk);
struct bcm_op *op;
 
@@ -178,7 +177,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
seq_printf(m, " / sk %pK", sk);
seq_printf(m, " / bo %pK", bo);
seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
-   seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex));
+   seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, 
bo->ifindex));
seq_printf(m, " <<<\n");
 
list_for_each_entry(op, >rx_ops, list) {
@@ -190,7 +189,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
continue;
 
seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
-  bcm_proc_getifname(ifname, op->ifindex));
+  bcm_proc_getifname(net, ifname, op->ifindex));
 
if (op->flags & CAN_FD_FRAME)
seq_printf(m, "(%u)", op->nframes);
@@ -219,7 +218,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
list_for_each_entry(op, >tx_ops, list) {
 
seq_printf(m, "tx_op: %03X %s ", op->can_id,
-  bcm_proc_getifname(ifname, op->ifindex));
+  bcm_proc_getifname(net, ifname, op->ifindex));
 
if (op->flags & CAN_FD_FRAME)
seq_printf(m, "(%u) ", op->nframes);
@@ -242,7 +241,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 
 static int bcm_proc_open(struct inode *inode, struct file *file)
 {
-   return single_open(file, bcm_proc_show, PDE_DATA(inode));
+   return single_open_net(inode, file, bcm_proc_show);
 }
 
 static const struct file_operations bcm_proc_fops = {
@@ -267,7 +266,7 @@ static void bcm_can_tx(struct bcm_op *op)
if (!op->ifindex)
return;
 
-   dev = dev_get_by_index(_net, op->ifindex);
+   dev = dev_get_by_index(sock_net(op->sk), op->ifindex

[PATCH 4/8] can: complete initial namespace support

[Oliver Hartkopp]

The statistics and its proc output was not implemented as per-net in the
initial network namespace support by Mario Kicherer (8e8cda6d737d).
This patch adds the missing per-net statistics for the CAN subsystem.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 include/linux/can/core.h |   4 +-
 include/net/netns/can.h  |   5 ++
 net/can/af_can.c |  71 +---
 net/can/af_can.h |   5 --
 net/can/proc.c   | 141 +--
 5 files changed, 121 insertions(+), 105 deletions(-)

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 319a0da827b8..c9a17bb1221c 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -5,7 +5,7 @@
  *
  * Authors: Oliver Hartkopp <oliver.hartk...@volkswagen.de>
  *  Urs Thuermann   <urs.thuerm...@volkswagen.de>
- * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * Copyright (c) 2002-2017 Volkswagen Group Electronic Research
  * All rights reserved.
  *
  */
@@ -17,7 +17,7 @@
 #include 
 #include 
 
-#define CAN_VERSION "20120528"
+#define CAN_VERSION "20170425"
 
 /* increment this number each time you change some user-space interface */
 #define CAN_ABI_VERSION "9"
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index e8beba772f1a..574157dbc43a 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -8,6 +8,8 @@
 #include 
 
 struct dev_rcv_lists;
+struct s_stats;
+struct s_pstats;
 
 struct netns_can {
 #if IS_ENABLED(CONFIG_PROC_FS)
@@ -26,6 +28,9 @@ struct netns_can {
/* receive filters subscribed for 'all' CAN devices */
struct dev_rcv_lists *can_rx_alldev_list;
spinlock_t can_rcvlists_lock;
+   struct timer_list can_stattimer;/* timer for statistics update */
+   struct s_stats *can_stats;  /* packet statistics */
+   struct s_pstats *can_pstats;/* receive list statistics */
 };
 
 #endif /* __NETNS_CAN_H__ */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 421b60fc42c3..b6406fe33c76 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -2,7 +2,7 @@
  * af_can.c - Protocol family CAN core module
  *(used by different CAN protocol modules)
  *
- * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * Copyright (c) 2002-2017 Volkswagen Group Electronic Research
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -81,10 +81,6 @@ static struct kmem_cache *rcv_cache __read_mostly;
 static const struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
 static DEFINE_MUTEX(proto_tab_lock);
 
-struct timer_list can_stattimer;   /* timer for statistics update */
-struct s_statscan_stats;   /* packet statistics */
-struct s_pstats   can_pstats;  /* receive list statistics */
-
 static atomic_t skbcounter = ATOMIC_INIT(0);
 
 /*
@@ -221,6 +217,7 @@ int can_send(struct sk_buff *skb, int loop)
 {
struct sk_buff *newskb = NULL;
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+   struct s_stats *can_stats = dev_net(skb->dev)->can.can_stats;
int err = -EINVAL;
 
if (skb->len == CAN_MTU) {
@@ -309,8 +306,8 @@ int can_send(struct sk_buff *skb, int loop)
netif_rx_ni(newskb);
 
/* update statistics */
-   can_stats.tx_frames++;
-   can_stats.tx_frames_delta++;
+   can_stats->tx_frames++;
+   can_stats->tx_frames_delta++;
 
return 0;
 
@@ -468,6 +465,7 @@ int can_rx_register(struct net *net, struct net_device 
*dev, canid_t can_id,
struct receiver *r;
struct hlist_head *rl;
struct dev_rcv_lists *d;
+   struct s_pstats *can_pstats = net->can.can_pstats;
int err = 0;
 
/* insert new receiver  (dev,canid,mask) -> (func,data) */
@@ -499,9 +497,9 @@ int can_rx_register(struct net *net, struct net_device 
*dev, canid_t can_id,
hlist_add_head_rcu(>list, rl);
d->entries++;
 
-   can_pstats.rcv_entries++;
-   if (can_pstats.rcv_entries_max < can_pstats.rcv_entries)
-   can_pstats.rcv_entries_max = can_pstats.rcv_entries;
+   can_pstats->rcv_entries++;
+   if (can_pstats->rcv_entries_max < can_pstats->rcv_entries)
+   can_pstats->rcv_entries_max = can_pstats->rcv_entries;
} else {
kmem_cache_free(rcv_cache, r);
err = -ENODEV;
@@ -543,6 +541,7 @@ void can_rx_unregister(struct net *net, struct net_device 
*dev, canid_t can_id,
 {
struct receiver *r = NULL;
struct hlist_head *rl;
+   struct s_pstats *can_pstats = net->can.can_pstats;
struct dev_rcv_lists *d;
 
if (dev && dev->type != ARPHRD_CAN)
@@ -589,8 +588,8 @@ void can_rx_unre

[PATCH 8/8] can: enable module auto loading for virtual CAN interfaces

[Oliver Hartkopp]

Autoload the vcan module when a vcan instance is to be created by
'ip link add type vcan'

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 drivers/net/can/vcan.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 674f367087c5..facca33d53e9 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -1,7 +1,7 @@
 /*
  * vcan.c - Virtual CAN interface
  *
- * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * Copyright (c) 2002-2017 Volkswagen Group Electronic Research
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -50,9 +50,12 @@
 #include 
 #include 
 
+#define DRV_NAME "vcan"
+
 MODULE_DESCRIPTION("virtual CAN interface");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Urs Thuermann <urs.thuerm...@volkswagen.de>");
+MODULE_ALIAS_RTNL_LINK(DRV_NAME);
 
 
 /*
@@ -164,7 +167,7 @@ static void vcan_setup(struct net_device *dev)
 }
 
 static struct rtnl_link_ops vcan_link_ops __read_mostly = {
-   .kind   = "vcan",
+   .kind   = DRV_NAME,
.setup  = vcan_setup,
 };
 
-- 
2.11.0

[PATCH 2/8] can: remove obsolete pernet_operations definitions

[Oliver Hartkopp]

The namespace support for the CAN subsystem does not need any additional
memory. So when ".size = 0" there's no extra memory allocated by the system.
And therefore ".id" is obsolete too.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/af_can.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 2c935babe466..421b60fc42c3 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -75,8 +75,6 @@ static int stats_timer __read_mostly = 1;
 module_param(stats_timer, int, S_IRUGO);
 MODULE_PARM_DESC(stats_timer, "enable timer for statistics (default:on)");
 
-static int can_net_id;
-
 static struct kmem_cache *rcv_cache __read_mostly;
 
 /* table of registered CAN protocols */
@@ -935,8 +933,6 @@ static struct notifier_block can_netdev_notifier 
__read_mostly = {
 static struct pernet_operations can_pernet_ops __read_mostly = {
.init = can_pernet_init,
.exit = can_pernet_exit,
-   .id = _net_id,
-   .size = 0,
 };
 
 static __init int can_init(void)
-- 
2.11.0

[PATCH 0/8] Fix and complete CAN namespace support

[Oliver Hartkopp]

Hello Dave,

unfortunately the initial network namespace support by Mario Kicherer
(8e8cda6d737d) slipped into net-next without further review and Marc pushed
the code without my Acked-by. Due to the fact that this code was in net-next
now I spent some nights to fix, clean up, finalize and test the missing pieces
for the namespace support for the CAN subsystem in net/can.

As Marc is currently *VERY* unresponsive on the mailing list due to his 'real'
job I send this patch set directly to you to make sure it gets through the
current merge window. We are already in -rc8 and I would like to avoid to push
an incomplete functionality - that has to be fixed - to Linus.

This patch set is based on the latest net-next.

Thanks,
Oliver

Oliver Hartkopp (8):
  can: fix memory leak in initial namespace support
  can: remove obsolete pernet_operations definitions
  can: remove obsolete definitions
  can: complete initial namespace support
  can: network namespace support for CAN_BCM protocol
  can: network namespace support for CAN gateway
  can: add Virtual CAN Tunnel driver (vxcan)
  can: enable module auto loading for virtual CAN interfaces

 drivers/net/can/Kconfig|  18 +++
 drivers/net/can/Makefile   |   1 +
 drivers/net/can/vcan.c |   7 +-
 drivers/net/can/vxcan.c| 316 +
 include/linux/can/core.h   |   4 +-
 include/net/netns/can.h|   9 ++
 include/uapi/linux/can/vxcan.h |  12 ++
 net/can/af_can.c   |  77 +-
 net/can/af_can.h   |   9 --
 net/can/bcm.c  |  90 +++-
 net/can/gw.c   |  72 ++
 net/can/proc.c | 141 +-
 12 files changed, 580 insertions(+), 176 deletions(-)
 create mode 100644 drivers/net/can/vxcan.c
 create mode 100644 include/uapi/linux/can/vxcan.h

-- 
2.11.0

[PATCH 6/8] can: network namespace support for CAN gateway

[Oliver Hartkopp]

The CAN gateway was not implemented as per-net in the initial network
namespace support by Mario Kicherer (8e8cda6d737d).
This patch enables the CAN gateway to be used in different namespaces.

Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 include/net/netns/can.h |  3 +++
 net/can/gw.c| 72 ++---
 2 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 0f3c31aab8a8..b106e6ae2e5b 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -32,6 +32,9 @@ struct netns_can {
struct timer_list can_stattimer;/* timer for statistics update */
struct s_stats *can_stats;  /* packet statistics */
struct s_pstats *can_pstats;/* receive list statistics */
+
+   /* CAN GW per-net gateway jobs */
+   struct hlist_head cgw_list;
 };
 
 #endif /* __NETNS_CAN_H__ */
diff --git a/net/can/gw.c b/net/can/gw.c
index ad5bf5d508d3..29748d844c3f 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -1,7 +1,7 @@
 /*
  * gw.c - CAN frame Gateway/Router/Bridge with netlink interface
  *
- * Copyright (c) 2011 Volkswagen Group Electronic Research
+ * Copyright (c) 2017 Volkswagen Group Electronic Research
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -59,7 +59,7 @@
 #include 
 #include 
 
-#define CAN_GW_VERSION "20130117"
+#define CAN_GW_VERSION "20170425"
 #define CAN_GW_NAME "can-gw"
 
 MODULE_DESCRIPTION("PF_CAN netlink gateway");
@@ -79,9 +79,7 @@ MODULE_PARM_DESC(max_hops,
 __stringify(CGW_MAX_HOPS) " hops, "
 "default: " __stringify(CGW_DEFAULT_HOPS) ")");
 
-static HLIST_HEAD(cgw_list);
 static struct notifier_block notifier;
-
 static struct kmem_cache *cgw_cache __read_mostly;
 
 /* structure that contains the (on-the-fly) CAN frame modifications */
@@ -438,16 +436,16 @@ static void can_can_gw_rcv(struct sk_buff *skb, void 
*data)
gwj->handled_frames++;
 }
 
-static inline int cgw_register_filter(struct cgw_job *gwj)
+static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
 {
-   return can_rx_register(_net, gwj->src.dev, gwj->ccgw.filter.can_id,
+   return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
   gwj->ccgw.filter.can_mask, can_can_gw_rcv,
   gwj, "gw", NULL);
 }
 
-static inline void cgw_unregister_filter(struct cgw_job *gwj)
+static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
 {
-   can_rx_unregister(_net, gwj->src.dev, gwj->ccgw.filter.can_id,
+   can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
  gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
 }
 
@@ -455,9 +453,8 @@ static int cgw_notifier(struct notifier_block *nb,
unsigned long msg, void *ptr)
 {
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+   struct net *net = dev_net(dev);
 
-   if (!net_eq(dev_net(dev), _net))
-   return NOTIFY_DONE;
if (dev->type != ARPHRD_CAN)
return NOTIFY_DONE;
 
@@ -468,11 +465,11 @@ static int cgw_notifier(struct notifier_block *nb,
 
ASSERT_RTNL();
 
-   hlist_for_each_entry_safe(gwj, nx, _list, list) {
+   hlist_for_each_entry_safe(gwj, nx, >can.cgw_list, list) {
 
if (gwj->src.dev == dev || gwj->dst.dev == dev) {
hlist_del(>list);
-   cgw_unregister_filter(gwj);
+   cgw_unregister_filter(net, gwj);
kmem_cache_free(cgw_cache, gwj);
}
}
@@ -592,12 +589,13 @@ static int cgw_put_job(struct sk_buff *skb, struct 
cgw_job *gwj, int type,
 /* Dump information about all CAN gateway jobs, in response to RTM_GETROUTE */
 static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 {
+   struct net *net = sock_net(skb->sk);
struct cgw_job *gwj = NULL;
int idx = 0;
int s_idx = cb->args[0];
 
rcu_read_lock();
-   hlist_for_each_entry_rcu(gwj, _list, list) {
+   hlist_for_each_entry_rcu(gwj, >can.cgw_list, list) {
if (idx < s_idx)
goto cont;
 
@@ -812,6 +810,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct 
cf_mod *mod,
 static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
  struct netlink_ext_ack *extack)
 {
+   struct net *net = sock_net(skb->sk);
struct rtcanmsg *r;
struct cgw_job *gwj;
struct cf_mod mod;
@@ -842,7 +841,7 @@ static int cgw_create_job(s

Re: [PATCH net-next v2] can: initial support for network namespaces

[Oliver Hartkopp]

Hello Mario,

On 04/10/2017 09:51 AM, Mario Kicherer wrote:


You didn't include the statistics here:

+   struct s_stats *can_stats;  /* packet statistics */
+   struct s_pstats *can_pstats;/* receive list statistics */

which need to be per-net too, right?


I was not sure how this information is used - e.g., maybe just as debug
information that the system is working. Hence, I just left them as they
were.


Ok. Completed that now ;-)


You do a kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL) in
can_pernet_init().

Doesn't it need a kfree(net->can.can_rx_alldev_list) then??


Yes, I missed this. Thanks!


I consistently did not remove my new structs too ;-)
Will send a v2 of my 'completion' patch for a review.


Unfortunately, I don't have access to my CAN hardware right now, hence
I cannot test a corresponding patch at the moment. :/


Using virtual CANs should be ok for testing.

Best regards,
Oliver

Re: [PATCH net-next v2] can: initial support for network namespaces

[Oliver Hartkopp]

LED(CONFIG_PROC_FS))
can_init_proc(net);

diff --git a/net/can/af_can.h b/net/can/af_can.h
index f273c9d9b129..10d46bd2e9ea 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -110,9 +110,6 @@ struct s_pstats {
unsigned long rcv_entries_max;
 };

-/* receive filters subscribed for 'all' CAN devices */
-extern struct dev_rcv_lists can_rx_alldev_list;
-
 /* function prototypes for the CAN networklayer procfs (proc.c) */
 void can_init_proc(struct net *net);
 void can_remove_proc(struct net *net);
@@ -120,8 +117,5 @@ void can_stat_update(unsigned long data);

 /* structures and variables from af_can.c needed in proc.c for reading */
 extern struct timer_list can_stattimer;/* timer for statistics 
update */

-extern struct s_statscan_stats;/* packet statistics */
-extern struct s_pstats   can_pstats;   /* receive list statistics */
-extern struct hlist_head can_rx_dev_list;  /* rx dispatcher structures */

 #endif /* AF_CAN_H */


On 04/08/2017 07:24 PM, Oliver Hartkopp wrote:

Hello Mario,

unfortunately Marc pushed this patch before I finally was able to review
it ... :-(

Some questions:

On 02/21/2017 12:19 PM, Mario Kicherer wrote:

This patch adds initial support for network namespaces. The changes only
enable support in the CAN raw, proc and af_can code. GW and BCM still
have their checks that ensure that they are used only from the main
namespace.

The patch boils down to moving the global structures, i.e. the global
filter list and their /proc stats, into a per-namespace structure and
passing
around the corresponding "struct net" in a lot of different places.


(..)


diff --git a/include/net/netns/can.h b/include/net/netns/can.h
new file mode 100644
index 000..e8beba7
--- /dev/null
+++ b/include/net/netns/can.h
@@ -0,0 +1,31 @@
+/*
+ * can in net namespaces
+ */
+
+#ifndef __NETNS_CAN_H__
+#define __NETNS_CAN_H__
+
+#include 
+
+struct dev_rcv_lists;
+
+struct netns_can {
+#if IS_ENABLED(CONFIG_PROC_FS)
+struct proc_dir_entry *proc_dir;
+struct proc_dir_entry *pde_version;
+struct proc_dir_entry *pde_stats;
+struct proc_dir_entry *pde_reset_stats;
+struct proc_dir_entry *pde_rcvlist_all;
+struct proc_dir_entry *pde_rcvlist_fil;
+struct proc_dir_entry *pde_rcvlist_inv;
+struct proc_dir_entry *pde_rcvlist_sff;
+struct proc_dir_entry *pde_rcvlist_eff;
+struct proc_dir_entry *pde_rcvlist_err;
+#endif
+
+/* receive filters subscribed for 'all' CAN devices */
+struct dev_rcv_lists *can_rx_alldev_list;
+spinlock_t can_rcvlists_lock;


You didn't include the statistics here:

+   struct s_stats *can_stats;  /* packet statistics */
+   struct s_pstats *can_pstats;/* receive list statistics */

which need to be per-net too, right?

(..)

@@ -877,6 +871,41 @@ static int can_notifier(struct notifier_block
*nb, unsigned long msg,
 return NOTIFY_DONE;
 }

+int can_pernet_init(struct net *net)
+{
+net->can.can_rcvlists_lock =
+__SPIN_LOCK_UNLOCKED(net->can.can_rcvlists_lock);
+net->can.can_rx_alldev_list =
+kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
+memset(net->can.can_rx_alldev_list, 0, sizeof(struct
dev_rcv_lists));
+
+if (IS_ENABLED(CONFIG_PROC_FS))
+can_init_proc(net);
+
+return 0;
+}
+
+void can_pernet_exit(struct net *net)
+{
+struct net_device *dev;
+
+if (IS_ENABLED(CONFIG_PROC_FS))
+can_remove_proc(net);
+
+/* remove created dev_rcv_lists from still registered CAN devices */
+rcu_read_lock();
+for_each_netdev_rcu(net, dev) {
+if (dev->type == ARPHRD_CAN && dev->ml_priv) {
+struct dev_rcv_lists *d = dev->ml_priv;
+
+BUG_ON(d->entries);
+kfree(d);
+dev->ml_priv = NULL;
+}
+}
+rcu_read_unlock();


You do a kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL) in
can_pernet_init().

Doesn't it need a kfree(net->can.can_rx_alldev_list) then??

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index e8beba772f1a..a8866ae1788f 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -8,6 +8,8 @@
 #include 
 
 struct dev_rcv_lists;
+struct s_stats;
+struct s_pstats;
 
 struct netns_can {
 #if IS_ENABLED(CONFIG_PROC_FS)
@@ -26,6 +28,8 @@ struct netns_can {
 	/* receive filters subscribed for 'all' CAN devices */
 	struct dev_rcv_lists *can_rx_alldev_list;
 	spinlock_t can_rcvlists_lock;
+	struct s_stats *can_stats;	/* packet statistics */
+	struct s_pstats *can_pstats;	/* receive list statistics */
 };
 
 #endif /* __NETNS_CAN_H__ */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index abf7d854a94d..db35d6a5ac26 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c

Re: [PATCH net-next v2] can: initial support for network namespaces

[Oliver Hartkopp]

Hello Mario,

unfortunately Marc pushed this patch before I finally was able to review 
it ... :-(


Some questions:

On 02/21/2017 12:19 PM, Mario Kicherer wrote:

This patch adds initial support for network namespaces. The changes only
enable support in the CAN raw, proc and af_can code. GW and BCM still
have their checks that ensure that they are used only from the main
namespace.

The patch boils down to moving the global structures, i.e. the global
filter list and their /proc stats, into a per-namespace structure and passing
around the corresponding "struct net" in a lot of different places.


(..)


diff --git a/include/net/netns/can.h b/include/net/netns/can.h
new file mode 100644
index 000..e8beba7
--- /dev/null
+++ b/include/net/netns/can.h
@@ -0,0 +1,31 @@
+/*
+ * can in net namespaces
+ */
+
+#ifndef __NETNS_CAN_H__
+#define __NETNS_CAN_H__
+
+#include 
+
+struct dev_rcv_lists;
+
+struct netns_can {
+#if IS_ENABLED(CONFIG_PROC_FS)
+   struct proc_dir_entry *proc_dir;
+   struct proc_dir_entry *pde_version;
+   struct proc_dir_entry *pde_stats;
+   struct proc_dir_entry *pde_reset_stats;
+   struct proc_dir_entry *pde_rcvlist_all;
+   struct proc_dir_entry *pde_rcvlist_fil;
+   struct proc_dir_entry *pde_rcvlist_inv;
+   struct proc_dir_entry *pde_rcvlist_sff;
+   struct proc_dir_entry *pde_rcvlist_eff;
+   struct proc_dir_entry *pde_rcvlist_err;
+#endif
+
+   /* receive filters subscribed for 'all' CAN devices */
+   struct dev_rcv_lists *can_rx_alldev_list;
+   spinlock_t can_rcvlists_lock;


You didn't include the statistics here:

+   struct s_stats *can_stats;  /* packet statistics */
+   struct s_pstats *can_pstats;/* receive list statistics */

which need to be per-net too, right?

(..)

@@ -877,6 +871,41 @@ static int can_notifier(struct notifier_block *nb, 
unsigned long msg,
return NOTIFY_DONE;
 }

+int can_pernet_init(struct net *net)
+{
+   net->can.can_rcvlists_lock =
+   __SPIN_LOCK_UNLOCKED(net->can.can_rcvlists_lock);
+   net->can.can_rx_alldev_list =
+   kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
+   memset(net->can.can_rx_alldev_list, 0, sizeof(struct dev_rcv_lists));
+
+   if (IS_ENABLED(CONFIG_PROC_FS))
+   can_init_proc(net);
+
+   return 0;
+}
+
+void can_pernet_exit(struct net *net)
+{
+   struct net_device *dev;
+
+   if (IS_ENABLED(CONFIG_PROC_FS))
+   can_remove_proc(net);
+
+   /* remove created dev_rcv_lists from still registered CAN devices */
+   rcu_read_lock();
+   for_each_netdev_rcu(net, dev) {
+   if (dev->type == ARPHRD_CAN && dev->ml_priv) {
+   struct dev_rcv_lists *d = dev->ml_priv;
+
+   BUG_ON(d->entries);
+   kfree(d);
+   dev->ml_priv = NULL;
+   }
+   }
+   rcu_read_unlock();


You do a kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL) in 
can_pernet_init().


Doesn't it need a kfree(net->can.can_rx_alldev_list) then??

Regards,
Oliver

Re: [PATCH] Enable tx timestamping on loopback and dummy

[Oliver Hartkopp]

Hi Ezequiel,

On 03/11/2017 03:42 PM, Ezequiel Lara Gomez wrote:

Also, cleanup some warnings from timestamping code.


in fact you're doing three different things here:

1. introduce tx timestamping
2. silently change an include:  -> 
3. fix some whitespace and empty line issues

You'd better provide one patch for 1 & 2 and explain why 2 is needed.

Regards,
Oliver



This enables testing of SO_TIMESTAMPING options by targetting localhost
addresses.

Tested on qemu using txtimestamping.c from the kernel selftests.

Signed-off-by: Ezequiel Lara Gomez 
---
 drivers/net/dummy.c|  1 +
 drivers/net/loopback.c | 14 +++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 2c80611..32fdc00 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -125,6 +125,7 @@ static netdev_tx_t dummy_xmit(struct sk_buff *skb, struct 
net_device *dev)
dstats->tx_bytes += skb->len;
u64_stats_update_end(>syncp);

+   skb_tx_timestamp(skb);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
 }
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index b23b719..8bcf479 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -13,7 +13,7 @@
  *
  * Alan Cox:   Fixed oddments for NET3.014
  * Alan Cox:   Rejig for NET3.029 snap #3
- * Alan Cox:   Fixed NET3.029 bugs and sped up
+ * Alan Cox:   Fixed NET3.029 bugs and sped up
  * Larry McVoy :   Tiny tweak to double performance
  * Alan Cox:   Backed out LMV's tweak - the linux mm
  * can't take it...
@@ -41,7 +41,7 @@
 #include 

 #include 
-#include 
+#include 

 #include 
 #include 
@@ -74,6 +74,7 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
struct pcpu_lstats *lb_stats;
int len;

+   skb_tx_timestamp(skb);
skb_orphan(skb);

/* Before queueing this packet to netif_rx(),
@@ -149,8 +150,8 @@ static void loopback_dev_free(struct net_device *dev)
 }

 static const struct net_device_ops loopback_ops = {
-   .ndo_init  = loopback_dev_init,
-   .ndo_start_xmit= loopback_xmit,
+   .ndo_init= loopback_dev_init,
+   .ndo_start_xmit  = loopback_xmit,
.ndo_get_stats64 = loopback_get_stats64,
.ndo_set_mac_address = eth_mac_addr,
 };
@@ -170,7 +171,7 @@ static void loopback_setup(struct net_device *dev)
dev->priv_flags  |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
netif_keep_dst(dev);
dev->hw_features = NETIF_F_GSO_SOFTWARE;
-   dev->features= NETIF_F_SG | NETIF_F_FRAGLIST
+   dev->features= NETIF_F_SG | NETIF_F_FRAGLIST
| NETIF_F_GSO_SOFTWARE
| NETIF_F_HW_CSUM
| NETIF_F_RXCSUM
@@ -206,7 +207,6 @@ static __net_init int loopback_net_init(struct net *net)
net->loopback_dev = dev;
return 0;

-
 out_free_netdev:
free_netdev(dev);
 out:
@@ -217,5 +217,5 @@ static __net_init int loopback_net_init(struct net *net)

 /* Registered in net/core/dev.c */
 struct pernet_operations __net_initdata loopback_net_ops = {
-   .init = loopback_net_init,
+   .init = loopback_net_init,
 };

Re: [PATCH v2] can: m_can: enable transmission of FD frame on latest version

[Oliver Hartkopp]

@Wenyou Yang: Can you please test the two patches posted here:

[PATCH 1/2] can: m_can: handle bitrate setup on IP core >= 3.1.x
http://marc.info/?l=linux-can=148883529927720=2

[PATCH 2/2] can: m_can: handle frame transmission on IP core >= 3.1.x
http://marc.info/?l=linux-can=148883529927718=2

Tnx & regards,
Oliver

Re: [PATCH v2] can: m_can: enable transmission of FD frame on latest version

[Oliver Hartkopp]

Hi Marc,

On 03/06/2017 11:53 AM, Marc Kleine-Budde wrote:

On 03/06/2017 03:21 AM, Wenyou Yang wrote:

Enables the transmission of CAN FD frames on M_CAN IP core >= v3.1.x
and with the bit rate switching.

Tested on M_CAN IP 3.1.0 (CREL = 0x31040730) of SAMA5D2 SoC.


Does this patch work still with the old version of the silicon?


The bits that were added in the TX FIFO element are 'reserved' in the 
old silicon - so it should not harm.


This code enables

 if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
-   cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
+   cccr |= (CCCR_CME_CANFD_BRS | CCCR_CME_CANFD) << CCCR_CME_SHIFT;

the CAN FD support in the new silicon.

This register is set for the old silicon EVERY time a CAN frame is sent.
So this change should not harm the old silicon either.

In fact I was told that the v3.0.x IP core is rather seldom in the wild.
Although I don't have a v3.0.x to test it should work from the 
documentation side of view.


Reviewed-by: Oliver Hartkopp <socket...@hartkopp.net>

If we would like to make it really better, the code in 
m_can_start_xmit() should only fiddle with the M_CAN_CCCR register when 
working with the v3.0.x silicon.


In fact I would suggest to use the

if (m_can_read_core_rev(priv) < M_CAN_COREREL_3_1_0)

method from

http://marc.info/?l=linux-can=148716783119090=2

to split the code in m_can_start_xmit() accordingly.

@Wenyou Yang: Can you please send a v3 which splits the tx function?

Regards,
Oliver

Re: [PATCH] can: m_can: support transmit frame in CAN FD format

[Oliver Hartkopp]

The patch has some issues:

On 03/03/2017 06:33 AM, Wenyou Yang wrote:

Add support to transmit the frame in the CAN FD format and with
the bit rate switching.


This is a misleading comment.

"can: m_can: support transmit frame in CAN FD format"
is misleading too. You were able to send CAN FD frames before.

This patch enables the transmission of CAN FD frames on M_CAN IP cores 
>= v3.1.x, right?


So this should be mentioned properly.


Tested on SAMA5D2 Xplained board.


Tested on which M_CAN IP core revision would be useful here.


Signed-off-by: Wenyou Yang 
---
The testing is based on
[RESEND PATCH 1/1] can: m_can: fix bitrate setup on latest silicon
http://lkml.iu.edu/hypermail/linux/kernel/1702.1/05347.html

 drivers/net/can/m_can/m_can.c | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 195f15edb32e..9ef9b337d25b 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -266,8 +266,12 @@ enum m_can_mram_cfg {

 /* Tx Buffer Element */
 /* R0 */
+#define TX_BUF_ESI BIT(31)
 #define TX_BUF_XTD BIT(30)
 #define TX_BUF_RTR BIT(29)
+#define TX_BUF_EFC BIT(23)
+#define TX_BUF_EDL BIT(21)


This bit is named FDF (FD Format bit).

It has to be:

#define TX_BUF_FDF  BIT(21)


+#define TX_BUF_BRS BIT(20)

 /* address offset and element number for each FIFO/Buffer in the Message RAM */
 struct mram_cfg {
@@ -884,7 +888,7 @@ static void m_can_chip_config(struct net_device *dev)
}

if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
-   cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
+   cccr |= (CCCR_CME_CANFD_BRS | CCCR_CME_CANFD) << CCCR_CME_SHIFT;

m_can_write(priv, M_CAN_CCCR, cccr);
m_can_write(priv, M_CAN_TEST, test);
@@ -1047,6 +1051,7 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
struct canfd_frame *cf = (struct canfd_frame *)skb->data;
u32 id, cccr;
int i;
+   u32 dlc;

if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -1065,7 +1070,6 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

/* message ram configuration */
m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
-   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);

for (i = 0; i < cf->len; i += 4)
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
@@ -1073,20 +1077,29 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

can_put_echo_skb(skb, dev, 0);

+   dlc = can_len2dlc(cf->len) << 16;
+
if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
cccr = m_can_read(priv, M_CAN_CCCR);
cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
if (can_is_canfd_skb(skb)) {
-   if (cf->flags & CANFD_BRS)
+   dlc |= TX_BUF_EDL;


dito FDF


+   if (cf->flags & CANFD_ESI)
+   dlc |= TX_BUF_ESI;
+   if (cf->flags & CANFD_BRS) {
+   dlc |= TX_BUF_BRS;
cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
-   else
+   } else {
cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
+   }
} else {
cccr |= CCCR_CMR_CAN << CCCR_CMR_SHIFT;
}
m_can_write(priv, M_CAN_CCCR, cccr);
}

+   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, dlc);
+
/* enable first TX buffer to start transfer  */
m_can_write(priv, M_CAN_TXBTIE, 0x1);
m_can_write(priv, M_CAN_TXBAR, 0x1);



Beside the documentation and the wrong bit naming the patch looks ok.
Please resend.

Regards,
Oliver

Re: [RESEND PATCH 1/1] can: m_can: fix bitrate setup on latest silicon

[Oliver Hartkopp]

Hi all,

On 02/15/2017 03:08 PM, Quentin Schulz wrote:

From: Florian Vallee 

According to the m_can user manual changelog the BTP register layout was
updated with core revision 3.1.0

This change is not backward-compatible and using the current driver along
with a recent IP results in an incorrect bitrate on the wire.


the BTP register layout is only one of eleven changes between 3.0.1 and 
3.1.0:


1. Register FBTP renamed to DBTP and restructured
2. Register TEST restructured
3. Register CCCR restructured
4. Register BTP renamed to NBTP and restructured <<< HERE
5. Register PSR updated
6. Register TDCR added
7. Register IR updated
8. Register IE updated
9. Register ILS updated
10. Rx Buffer and FIFO Element updated
11. Tx Buffer Element updated

Especially the change #11 allows the switch between CAN and CAN FD 
frames on a per-frame setting without switching the entire controller 
mode via CCCR register in IP core 3.0.1:


https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/net/can/m_can/m_can.c?h=linux-4.9.y#n1075

Fortunately I was contacted by Bosch this Friday as they want to 
contribute a driver upgrade to support IP cores up to 3.2.x.
Additionally their plan is to use Device Tree information to determine 
the IP core revision - or at least to cross check its information with 
the register information used in your suggestion.


For that reason I would suggest to wait for the driver updates from 
Bosch instead of adding an incomplete fix for only one change of eleven.


Btw. it would be very cool if you could provide some testing for the 
upcoming changes from Bosch when they are posted.


Best regards,
Oliver



Tested with a SAMA5D2 SoC (CREL = 0x31040730)

Signed-off-by: Florian Vallee 
Tested-by: Quentin Schulz 
---
 drivers/net/can/m_can/m_can.c | 38 +++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 195f15e..246584e 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -105,6 +105,10 @@ enum m_can_mram_cfg {
MRAM_CFG_NUM,
 };

+/* Core Release Register (CREL) */
+#define CRR_REL_MASK   0xfff
+#define CRR_REL_SHIFT  20
+
 /* Fast Bit Timing & Prescaler Register (FBTP) */
 #define FBTR_FBRP_MASK 0x1f
 #define FBTR_FBRP_SHIFT16
@@ -136,7 +140,7 @@ enum m_can_mram_cfg {
 #define CCCR_INIT  BIT(0)
 #define CCCR_CANFD 0x10

-/* Bit Timing & Prescaler Register (BTP) */
+/* Bit Timing & Prescaler Register (BTP) (M_CAN IP < 3.1.0) */
 #define BTR_BRP_MASK   0x3ff
 #define BTR_BRP_SHIFT  16
 #define BTR_TSEG1_SHIFT8
@@ -146,6 +150,16 @@ enum m_can_mram_cfg {
 #define BTR_SJW_SHIFT  0
 #define BTR_SJW_MASK   0xf

+/* Nominal Bit Timing & Prescaler Register (NBTP) (M_CAN IP >= 3.1.0) */
+#define NBTR_SJW_SHIFT 25
+#define NBTR_SJW_MASK  (0x7f << NBTR_SJW_SHIFT)
+#define NBTR_BRP_SHIFT 16
+#define NBTR_BRP_MASK  (0x3ff << NBTR_BRP_SHIFT)
+#define NBTR_TSEG1_SHIFT   8
+#define NBTR_TSEG1_MASK(0xff << NBTR_TSEG1_SHIFT)
+#define NBTR_TSEG2_SHIFT   0
+#define NBTR_TSEG2_MASK(0x7f << NBTR_TSEG2_SHIFT)
+
 /* Error Counter Register(ECR) */
 #define ECR_RP BIT(15)
 #define ECR_REC_SHIFT  8
@@ -200,6 +214,9 @@ enum m_can_mram_cfg {
 IR_RF1L | IR_RF0L)
 #define IR_ERR_ALL (IR_ERR_STATE | IR_ERR_BUS)

+/* Core Version */
+#define M_CAN_COREREL_3_1_00x310
+
 /* Interrupt Line Select (ILS) */
 #define ILS_ALL_INT0   0x0
 #define ILS_ALL_INT1   0x
@@ -357,6 +374,13 @@ static inline void m_can_disable_all_interrupts(const 
struct m_can_priv *priv)
m_can_write(priv, M_CAN_ILE, 0x0);
 }

+static inline int m_can_read_core_rev(const struct m_can_priv *priv)
+{
+   u32 reg = m_can_read(priv, M_CAN_CREL);
+
+   return ((reg >> CRR_REL_SHIFT) & CRR_REL_MASK);
+}
+
 static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 {
struct net_device_stats *stats = >stats;
@@ -811,8 +835,16 @@ static int m_can_set_bittiming(struct net_device *dev)
sjw = bt->sjw - 1;
tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
tseg2 = bt->phase_seg2 - 1;
-   reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
-   (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
+
+   if (m_can_read_core_rev(priv) < M_CAN_COREREL_3_1_0)
+   reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
+   (tseg1 << BTR_TSEG1_SHIFT) |
+   (tseg2 << BTR_TSEG2_SHIFT);
+   else
+   reg_btp = (brp << NBTR_BRP_SHIFT) | (sjw << NBTR_SJW_SHIFT) |
+   (tseg1 << 

Re: [PATCH net-next] can: initial support for network namespaces

[Oliver Hartkopp]

Hello Mario,

thanks for your patch.

On 02/16/2017 12:39 PM, Mario Kicherer wrote:

This patch adds initial support for network namespaces. The changes only
enable support in the CAN raw, proc and af_can code. GW and BCM still
have their checks that ensure that they are used only from the main
namespace.

The patch boils down to moving the global structures, i.e. the global
filter list and their /proc stats, into a per-namespace structure and passing
around the corresponding "struct net" in a lot of different places.

Signed-off-by: Mario Kicherer 


Although it does not apply on the latest kernel due to this patch:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f1712c73714088a7252d276a57126d56c7d37e64

Please rebase your patch to the latest kernel tree and post it again.

Additionally you should post CAN related stuff to the linux-can ML (see 
CC) and the CAN maintainers found in the MAINTAINERS file. In this case 
cross posting to netdev ML is fine as namespaces are very netdev related.


Do you have an idea how to go with namespace aware virtual CAN 
interfaces to link the namespaces?


Regards,
Oliver

Re: [PATCH v3] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

On 02/10/2017 04:16 PM, David Miller wrote:

From: Oliver Hartkopp <socket...@hartkopp.net>
Date: Fri, 10 Feb 2017 09:28:57 +0100


can you please check whether this upstream commit

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f1712c73714088a7252d276a57126d56c7d37e64

really was queued up for -stable?


You never need to ask me this question, it is presented always, here:


http://patchwork.ozlabs.org/bundle/davem/stable/?submitter==*==

And it is indeed there.



Actually I was only aware of

http://patchwork.ozlabs.org/project/netdev/list/?state=*

which I checked before asking (of course). But there's no chance to 
figure out whether a patch is queued for -stable.


I'll put your stable queue URL into my bookmarks.

Thanks,
Oliver

Re: [PATCH v3] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

Hello Dave, Greg,

On 01/30/2017 12:34 AM, David Miller wrote:

From: Eric Dumazet 
Date: Fri, 27 Jan 2017 08:11:44 -0800


From: Eric Dumazet 

Zhang Yanmin reported crashes [1] and provided a patch adding a
synchronize_rcu() call in can_rx_unregister()

The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

Recent kernels could use sock_set_flag(sk, SOCK_RCU_FREE), but let's
ease stable backports with the following fix instead.

 ...

Reported-by: Zhang Yanmin 
Signed-off-by: Eric Dumazet 


Applied and queued up for -stable, thanks Eric.



can you please check whether this upstream commit

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f1712c73714088a7252d276a57126d56c7d37e64

really was queued up for -stable?

This commit

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a06393ed03167771246c4c43192d9c264bc48412

was posted later and already got into the 4.4 and 4.9 stable trees.

Best regards,
Oliver

Re: [PATCH v3] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

On 01/27/2017 05:11 PM, Eric Dumazet wrote:

From: Eric Dumazet <eduma...@google.com>

Zhang Yanmin reported crashes [1] and provided a patch adding a
synchronize_rcu() call in can_rx_unregister()

The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

Recent kernels could use sock_set_flag(sk, SOCK_RCU_FREE), but let's
ease stable backports with the following fix instead.

[1]
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [] selinux_socket_sock_rcv_skb+0x65/0x2a0

Call Trace:
 
 [] security_sock_rcv_skb+0x4c/0x60
 [] sk_filter+0x41/0x210
 [] sock_queue_rcv_skb+0x53/0x3a0
 [] raw_rcv+0x2a3/0x3c0
 [] can_rcv_filter+0x12b/0x370
 [] can_receive+0xd9/0x120
 [] can_rcv+0xab/0x100
 [] __netif_receive_skb_core+0xd8c/0x11f0
 [] __netif_receive_skb+0x24/0xb0
 [] process_backlog+0x127/0x280
 [] net_rx_action+0x33b/0x4f0
 [] __do_softirq+0x184/0x440
 [] do_softirq_own_stack+0x1c/0x30
 
 [] do_softirq.part.18+0x3b/0x40
 [] do_softirq+0x1d/0x20
 [] netif_rx_ni+0xe5/0x110
 [] slcan_receive_buf+0x507/0x520
 [] flush_to_ldisc+0x21c/0x230
 [] process_one_work+0x24f/0x670
 [] worker_thread+0x9d/0x6f0
 [] ? rescuer_thread+0x480/0x480
 [] kthread+0x12c/0x150
 [] ret_from_fork+0x3f/0x70

Reported-by: Zhang Yanmin <yanmin.zh...@intel.com>
Signed-off-by: Eric Dumazet <eduma...@google.com>


Acked-by: Oliver Hartkopp <socket...@hartkopp.net>

Thanks Eric!

BR
Oliver


---
 include/linux/can/core.h |7 +++
 net/can/af_can.c |   14 +++---
 net/can/af_can.h |3 ++-
 net/can/bcm.c|4 ++--
 net/can/gw.c |2 +-
 net/can/raw.c|4 ++--
 6 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 
a0875001b13c84ad70a9b2909654e9ffb6824c58..df08a41d5be5f26cfa4cdc74935f5eae7fa51385
 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -45,10 +45,9 @@ struct can_proto {
 extern int  can_proto_register(const struct can_proto *cp);
 extern void can_proto_unregister(const struct can_proto *cp);

-extern int  can_rx_register(struct net_device *dev, canid_t can_id,
-   canid_t mask,
-   void (*func)(struct sk_buff *, void *),
-   void *data, char *ident);
+int can_rx_register(struct net_device *dev, canid_t can_id, canid_t mask,
+   void (*func)(struct sk_buff *, void *),
+   void *data, char *ident, struct sock *sk);

 extern void can_rx_unregister(struct net_device *dev, canid_t can_id,
  canid_t mask,
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 
1108079d934f8383a599d7997b08100fca0465e9..d2b0638284b9a71aaba9cc433822329baf82a34e
 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -445,6 +445,7 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, 
canid_t *mask,
  * @func: callback function on filter match
  * @data: returned parameter for callback function
  * @ident: string for calling module identification
+ * @sk: socket pointer (might be NULL)
  *
  * Description:
  *  Invokes the callback function with the received sk_buff and the given
@@ -468,7 +469,7 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, 
canid_t *mask,
  */
 int can_rx_register(struct net_device *dev, canid_t can_id, canid_t mask,
void (*func)(struct sk_buff *, void *), void *data,
-   char *ident)
+   char *ident, struct sock *sk)
 {
struct receiver *r;
struct hlist_head *rl;
@@ -496,6 +497,7 @@ int can_rx_register(struct net_device *dev, canid_t can_id, 
canid_t mask,
r->func= func;
r->data= data;
r->ident   = ident;
+   r->sk  = sk;

hlist_add_head_rcu(>list, rl);
d->entries++;
@@ -520,8 +522,11 @@ EXPORT_SYMBOL(can_rx_register);
 static void can_rx_delete_receiver(struct rcu_head *rp)
 {
struct receiver *r = container_of(rp, struct receiver, rcu);
-
+   struct sock *sk = r->sk;
+   
kmem_cache_free(rcv_cache, r);
+   if (sk)
+   sock_put(sk);
 }

 /**
@@ -596,8 +601,11 @@ void can_rx_unregister(struct net_device *dev, canid_t 
can_id, canid_t mask,
spin_unlock(_rcvlists_lock);

/* schedule the receiver item for deletion */
-   if (r)
+   if (r) {
+   if (r->sk)
+   sock_hold(r->sk);
call_rcu(>rcu, can_rx_delete_receiver);
+   }
 }
 EXPORT_SYMBOL(can_rx_unregister);

diff --git a/net/can/af_can.h b/net/can/af_can.h
index 
fca0fe9fc45a497cdf3da82d5414e846e7cc61b7..b86f5129e8385fe84ef671bb914e8e05c2977ca0
 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -50,13 +50,14

Re: NAPI on USB network drivers

[Oliver Hartkopp]

On 01/25/2017 09:57 PM, Alexander Duyck wrote:

On Wed, Jan 25, 2017 at 5:33 AM, Oliver Hartkopp <socket...@hartkopp.net> wrote:

You could probably get around the o-o-o problem by enabling RPS for
the interface.  I have found that it works for me to do that in order
to resolve o-o-o frames generated by VMs on virtual interfaces that
can't use NAPI.


Sounds promising!

In fact I tried to fix this o-o-o stuff here:

http://marc.info/?l=linux-can=143637774606287=2

Do you have an example how to do it right?

Best regards,
Oliver

Re: NAPI on USB network drivers

[Oliver Hartkopp]

On 01/25/2017 10:39 AM, Hayes Wang wrote:

Oliver Neukum [mailto:oneu...@suse.com]

Sent: Wednesday, January 25, 2017 5:35 PM

[...]

looking at r8152 I noticed that it uses NAPI. I never considered
this for the generic USB networking code as you cannot disable
interrupts for USB. Is it still worth it? What are the benefits?


You could use napi_gro_receive() and it influences the performance.


Another positive effect with NAPI is that you won't face out-of-order 
ethernet frames as you get with non-NAPI drivers, e.g. ax88179_178a


http://marc.info/?l=linux-can=148049063812807=2

We have the issue with CAN drivers where all USB drivers and >90% of the 
I/O mapped drivers do not use NAPI.


I wonder whether it makes sense to add NAPI to a driver which only has 
ONE RX buffer ... but when searching for a solution for o-o-o frames I 
was always pointed to NAPI.


Regards,
Oliver

Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

Hello Eric,

On 01/14/2017 04:43 AM, Liu Shuo wrote:

On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:

On 01/12/2017 02:01 PM, Eric Dumazet wrote:



The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

On recent kernels, following patch could help :



Thanks Eric!

@Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your
setup?

Sorry for late reply. I was OOO yesterday.
With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
socket flag" in the latest kernel. With backporting this one plus Eric's
following patch, it fixs my failure.


what would be the best approach to fix this issue - even in stable kernels?

E.g. would this change be ok for a stable as a quick fix?

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1108079d934f..6b974c2b66ef 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -112,6 +112,7 @@ EXPORT_SYMBOL(can_ioctl);

 static void can_sock_destruct(struct sock *sk)
 {
+   synchronize_rcu();
skb_queue_purge(>sk_receive_queue);
 }

And once this arrived in the mainline tree your suggested patch could be 
applied?


In any case we should not forget to give Reported-by credits to Liu.

Best regards,
Oliver

Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

On 01/12/2017 02:01 PM, Eric Dumazet wrote:

On Thu, 2017-01-12 at 09:22 +0100, Oliver Hartkopp wrote:



But my main concern is:

The reason why can_rx_delete_receiver() was introduced was the need to
remove a huge number of receivers with can_rx_unregister().

When you call synchronize_rcu() after each receiver removal this would
potentially lead to a big performance issue when e.g. closing CAN_RAW
sockets with a high number of receivers.

So the idea was to remove/unlink the receiver hlist_del_rcu(>list)
and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that
all elements are cleaned up by rcu at a later point.

Is it possible that the problems emerge due to hlist_del_rcu(>list)
and you accidently fix it with your introduced synchronize_rcu()?


I agree this patch does not fix the root cause.

The main problem seems that the sockets themselves are not RCU
protected.

If CAN uses RCU for delivery, then sockets should be freed only after
one RCU grace period.

On recent kernels, following patch could help :



Thanks Eric!

@Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your 
setup?


Best regards,
Oliver

Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb

[Oliver Hartkopp]

On 01/12/2017 07:33 AM, Liu ShuoX wrote:

From: Zhang Yanmin 

The patch is for fix the below kernel panic:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [] selinux_socket_sock_rcv_skb+0x65/0x2a0

Call Trace:
 
 [] security_sock_rcv_skb+0x4c/0x60
 [] sk_filter+0x41/0x210
 [] sock_queue_rcv_skb+0x53/0x3a0
 [] raw_rcv+0x2a3/0x3c0
 [] can_rcv_filter+0x12b/0x370
 [] can_receive+0xd9/0x120
 [] can_rcv+0xab/0x100
 [] __netif_receive_skb_core+0xd8c/0x11f0
 [] __netif_receive_skb+0x24/0xb0
 [] process_backlog+0x127/0x280
 [] net_rx_action+0x33b/0x4f0
 [] __do_softirq+0x184/0x440
 [] do_softirq_own_stack+0x1c/0x30
 
 [] do_softirq.part.18+0x3b/0x40
 [] do_softirq+0x1d/0x20
 [] netif_rx_ni+0xe5/0x110
 [] slcan_receive_buf+0x507/0x520
 [] flush_to_ldisc+0x21c/0x230
 [] process_one_work+0x24f/0x670
 [] worker_thread+0x9d/0x6f0
 [] ? rescuer_thread+0x480/0x480
 [] kthread+0x12c/0x150
 [] ret_from_fork+0x3f/0x70

The sk dereferenced in panic has been released. After the rcu_call in
can_rx_unregister, receiver was protected by RCU but inner data was
not, then later sk will be freed while other CPU is still using it.
We need wait here to make sure sk referenced via receiver was safe.

=> security_sk_free
=> sk_destruct
=> __sk_free
=> sk_free
=> raw_release
=> sock_release
=> sock_close
=> __fput
=> fput
=> task_work_run
=> exit_to_usermode_loop
=> syscall_return_slowpath
=> int_ret_from_sys_call

Signed-off-by: Zhang Yanmin 
Signed-off-by: He, Bo 
Signed-off-by: Liu Shuo A 
---
 net/can/af_can.c | 14 --
 net/can/af_can.h |  1 -
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1108079..fcbe971 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -517,10 +517,8 @@ int can_rx_register(struct net_device *dev, canid_t 
can_id, canid_t mask,
 /*
  * can_rx_delete_receiver - rcu callback for single receiver entry removal
  */
-static void can_rx_delete_receiver(struct rcu_head *rp)
+static void can_rx_delete_receiver(struct receiver *r)
 {
-   struct receiver *r = container_of(rp, struct receiver, rcu);
-
kmem_cache_free(rcv_cache, r);
 }

@@ -595,9 +593,13 @@ void can_rx_unregister(struct net_device *dev, canid_t 
can_id, canid_t mask,
  out:
spin_unlock(_rcvlists_lock);

-   /* schedule the receiver item for deletion */
-   if (r)
-   call_rcu(>rcu, can_rx_delete_receiver);
+   /* synchronize_rcu to wait until a grace period has elapsed, to make
+* sure all receiver's sk dereferenced by others.
+*/
+   if (r) {
+   synchronize_rcu();
+   can_rx_delete_receiver(r);


Nitpick: When can_rx_delete_receiver() just contains 
kmem_cache_free(rcv_cache, r), then the function definition should be 
removed.


But my main concern is:

The reason why can_rx_delete_receiver() was introduced was the need to 
remove a huge number of receivers with can_rx_unregister().


When you call synchronize_rcu() after each receiver removal this would 
potentially lead to a big performance issue when e.g. closing CAN_RAW 
sockets with a high number of receivers.


So the idea was to remove/unlink the receiver hlist_del_rcu(>list) 
and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that 
all elements are cleaned up by rcu at a later point.


Is it possible that the problems emerge due to hlist_del_rcu(>list) 
and you accidently fix it with your introduced synchronize_rcu()?


Regards,
Oliver



+   }
 }
 EXPORT_SYMBOL(can_rx_unregister);

diff --git a/net/can/af_can.h b/net/can/af_can.h
index fca0fe9..a0cbf83 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -50,7 +50,6 @@

 struct receiver {
struct hlist_node list;
-   struct rcu_head rcu;
canid_t can_id;
canid_t mask;
unsigned long matches;

Re: net/can: warning in raw_setsockopt/__alloc_pages_slowpath

[Oliver Hartkopp]

On 12/02/2016 04:42 PM, Marc Kleine-Budde wrote:

On 12/02/2016 04:11 PM, Oliver Hartkopp wrote:



On 12/02/2016 02:24 PM, Marc Kleine-Budde wrote:

On 12/02/2016 01:43 PM, Andrey Konovalov wrote:




 [] raw_setsockopt+0x1be/0x9f0 net/can/raw.c:506


We should add a check for a sensible optlen


static int raw_setsockopt(struct socket *sock, int level, int optname,
  char __user *optval, unsigned int optlen)
{
struct sock *sk = sock->sk;
struct raw_sock *ro = raw_sk(sk);
struct can_filter *filter = NULL;  /* dyn. alloc'ed filters */
struct can_filter sfilter; /* single filter */
struct net_device *dev = NULL;
can_err_mask_t err_mask = 0;
int count = 0;
int err = 0;

if (level != SOL_CAN_RAW)
return -EINVAL;

switch (optname) {

case CAN_RAW_FILTER:
if (optlen % sizeof(struct can_filter) != 0)
return -EINVAL;


here...

if (optlen > 64 * sizeof(struct can_filter))
return -EINVAL;



Agreed.

But what is sensible here?
64 filters is way to small IMO.

When thinking about picking a bunch of single CAN IDs I would tend to
something like 512 filters.


Ok - 64 was just an arbitrary chosen value for demonstration purposes :)



:-)

Would you like to send a patch?

Regards,
Oliver

Re: net/can: warning in raw_setsockopt/__alloc_pages_slowpath

[Oliver Hartkopp]

On 12/02/2016 02:24 PM, Marc Kleine-Budde wrote:

On 12/02/2016 01:43 PM, Andrey Konovalov wrote:




 [] raw_setsockopt+0x1be/0x9f0 net/can/raw.c:506


We should add a check for a sensible optlen


static int raw_setsockopt(struct socket *sock, int level, int optname,
  char __user *optval, unsigned int optlen)
{
struct sock *sk = sock->sk;
struct raw_sock *ro = raw_sk(sk);
struct can_filter *filter = NULL;  /* dyn. alloc'ed filters */
struct can_filter sfilter; /* single filter */
struct net_device *dev = NULL;
can_err_mask_t err_mask = 0;
int count = 0;
int err = 0;

if (level != SOL_CAN_RAW)
return -EINVAL;

switch (optname) {

case CAN_RAW_FILTER:
if (optlen % sizeof(struct can_filter) != 0)
return -EINVAL;


here...

if (optlen > 64 * sizeof(struct can_filter))
return -EINVAL;



Agreed.

But what is sensible here?
64 filters is way to small IMO.

When thinking about picking a bunch of single CAN IDs I would tend to 
something like 512 filters.


Regards,
Oliver



count = optlen / sizeof(struct can_filter);

if (count > 1) {
/* filter does not fit into dfilter => alloc space */
filter = memdup_user(optval, optlen);
if (IS_ERR(filter))
return PTR_ERR(filter);
} else if (count == 1) {
if (copy_from_user(, optval, sizeof(sfilter)))
return -EFAULT;
}

lock_sock(sk);


Marc

[PATCH] can: bcm: fix support for CAN FD frames

[Oliver Hartkopp]

Since commit 6f3b911d5f29b98 ("can: bcm: add support for CAN FD frames") the
CAN broadcast manager supports CAN and CAN FD data frames.

As these data frames are embedded in struct can[fd]_frames which have a
different length the access to the provided array of CAN frames became
dependend of op->cfsiz. By using a struct canfd_frame pointer for the array of
CAN frames the new offset calculation based on op->cfsiz was accidently applied
to CAN FD frame element lengths.

This fix makes the pointer to the arrays of the different CAN frame types a
void pointer so that the offset calculation in bytes accesses the correct CAN
frame elements.

Reference: http://marc.info/?l=linux-netdev=147980658909653

Reported-by: Andrey Konovalov <andreyk...@google.com>
Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/bcm.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 8af9d25..436a753 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -77,7 +77,7 @@
 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
-#define CAN_BCM_VERSION "20160617"
+#define CAN_BCM_VERSION "20161123"
 
 MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
 MODULE_LICENSE("Dual BSD/GPL");
@@ -109,8 +109,9 @@ struct bcm_op {
u32 count;
u32 nframes;
u32 currframe;
-   struct canfd_frame *frames;
-   struct canfd_frame *last_frames;
+   /* void pointers to arrays of struct can[fd]_frame */
+   void *frames;
+   void *last_frames;
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
@@ -681,7 +682,7 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
 
if (op->flags & RX_FILTER_ID) {
/* the easiest case */
-   bcm_rx_update_and_send(op, >last_frames[0], rxframe);
+   bcm_rx_update_and_send(op, op->last_frames, rxframe);
goto rx_starttimer;
}
 
@@ -1068,7 +1069,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, 
struct msghdr *msg,
 
if (msg_head->nframes) {
/* update CAN frames content */
-   err = memcpy_from_msg((u8 *)op->frames, msg,
+   err = memcpy_from_msg(op->frames, msg,
  msg_head->nframes * op->cfsiz);
if (err < 0)
return err;
@@ -1118,7 +1119,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, 
struct msghdr *msg,
}
 
if (msg_head->nframes) {
-   err = memcpy_from_msg((u8 *)op->frames, msg,
+   err = memcpy_from_msg(op->frames, msg,
  msg_head->nframes * op->cfsiz);
if (err < 0) {
if (op->frames != >sframe)
@@ -1163,6 +1164,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, 
struct msghdr *msg,
/* check flags */
 
if (op->flags & RX_RTR_FRAME) {
+   struct canfd_frame *frame0 = op->frames;
 
/* no timers in RTR-mode */
hrtimer_cancel(>thrtimer);
@@ -1174,8 +1176,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, 
struct msghdr *msg,
 * prevent a full-load-loopback-test ... ;-]
 */
if ((op->flags & TX_CP_CAN_ID) ||
-   (op->frames[0].can_id == op->can_id))
-   op->frames[0].can_id = op->can_id & ~CAN_RTR_FLAG;
+   (frame0->can_id == op->can_id))
+   frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
 
} else {
if (op->flags & SETTIMER) {
-- 
2.10.2

Re: net/can: use-after-free in bcm_rx_thr_flush

[Oliver Hartkopp]

On 11/22/2016 06:37 PM, Andrey Konovalov wrote:

On Tue, Nov 22, 2016 at 6:29 PM, Oliver Hartkopp <socket...@hartkopp.net> wrote:

Hi Andrey,

thanks for the report.

Although I can't see the issue in the code ...



Oh, I can see it now m(

Will send a patch today.

Many thanks,
Oliver

Re: net/can: use-after-free in bcm_rx_thr_flush

[Oliver Hartkopp]

Hi Andrey,

thanks for the report.

Although I can't see the issue in the code ...

On 11/22/2016 10:22 AM, Andrey Konovalov wrote:


==
BUG: KASAN: use-after-free in bcm_rx_thr_flush+0x284/0x2b0
Read of size 1 at addr 88006c1faae5 by task a.out/3874

page:ea0001b07e80 count:1 mapcount:0 mapping:  (null) index:0x0
flags: 0x180(slab)
page dumped because: kasan: bad access detected


(..)



The buggy address belongs to the object at 88006c1faae0
 which belongs to the cache kmalloc-32 of size 32


???


The buggy address 88006c1faae5 is located 5 bytes inside
 of 32-byte region [88006c1faae0, 88006c1fab00)


(..)


Memory state around the buggy address:
 88006c1fa980: fc fc fb fb fb fb fc fc fb fb fb fb fc fc fb fb
 88006c1faa00: fb fb fc fc fb fb fb fb fc fc fb fb fb fb fc fc

88006c1faa80: fb fb fb fb fc fc fb fb fb fb fc fc fb fb fb fb

   ^
 88006c1fab00: fc fc fb fb fb fb fc fc 00 00 00 00 fc fc 00 00
 88006c1fab80: 00 00 fc fc fb fb fb fb fc fc fb fb fb fb fc fc
==


(should be some zero initialized memory here)

The relevant code of bcm_rx_do_flush() can be found here:

http://lxr.free-electrons.com/source/net/can/bcm.c#L589

static inline int bcm_rx_do_flush(struct bcm_op *op, int update,
  unsigned int index)
{
struct canfd_frame *lcf = op->last_frames + op->cfsiz * index;

if ((op->last_frames) && (lcf->flags & RX_THR)) {  <<<- !!!
if (update)
bcm_rx_changed(op, lcf);
return 1;
}
return 0;
}


lcf->flags points into an array of struct canfd_frame at offset 5 which 
is allocated here:


http://lxr.free-electrons.com/source/net/can/bcm.c#L1105

/* create and init array for received CAN frames */
op->last_frames = kzalloc(msg_head->nframes * op->cfsiz,
  GFP_KERNEL);

So why does KASAN complain about accessing some kind of 32 byte cache 
when it should point into a zero initialized allocated space?


I will write some other test cases with a similar setting of options to 
check if I can trigger the instability too.


Tnx & regards,
Oliver

[PATCH] can: fix warning in bcm_connect/proc_register

[Oliver Hartkopp]

Andrey Konovalov reported an issue with proc_register in bcm.c.
As suggested by Cong Wang this patch adds a lock_sock() protection and
a check for unsuccessful proc_create_data() in bcm_connect().

Reference: http://marc.info/?l=linux-netdev=147732648731237

Reported-by: Andrey Konovalov <andreyk...@google.com>
Suggested-by: Cong Wang <xiyou.wangc...@gmail.com>
Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
---
 net/can/bcm.c | 32 +++-
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 8e999ff..8af9d25 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1549,24 +1549,31 @@ static int bcm_connect(struct socket *sock, struct 
sockaddr *uaddr, int len,
struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
struct sock *sk = sock->sk;
struct bcm_sock *bo = bcm_sk(sk);
+   int ret = 0;
 
if (len < sizeof(*addr))
return -EINVAL;
 
-   if (bo->bound)
-   return -EISCONN;
+   lock_sock(sk);
+
+   if (bo->bound) {
+   ret = -EISCONN;
+   goto fail;
+   }
 
/* bind a device to this socket */
if (addr->can_ifindex) {
struct net_device *dev;
 
dev = dev_get_by_index(_net, addr->can_ifindex);
-   if (!dev)
-   return -ENODEV;
-
+   if (!dev) {
+   ret = -ENODEV;
+   goto fail;
+   }
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
-   return -ENODEV;
+   ret = -ENODEV;
+   goto fail;
}
 
bo->ifindex = dev->ifindex;
@@ -1577,17 +1584,24 @@ static int bcm_connect(struct socket *sock, struct 
sockaddr *uaddr, int len,
bo->ifindex = 0;
}
 
-   bo->bound = 1;
-
if (proc_dir) {
/* unique socket address as filename */
sprintf(bo->procname, "%lu", sock_i_ino(sk));
bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
 proc_dir,
 _proc_fops, sk);
+   if (!bo->bcm_proc_read) {
+   ret = -ENOMEM;
+   goto fail;
+   }
}
 
-   return 0;
+   bo->bound = 1;
+
+fail:
+   release_sock(sk);
+
+   return ret;
 }
 
 static int bcm_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
-- 
2.9.3

Re: net/can: warning in bcm_connect/proc_register

[Oliver Hartkopp]

Hello Andrey, hello Cong,

thanks for catching this issue.

I added lock_sock() and a check for a failing proc_create_data() below.

Can you please check if it solved the issue?
I tested the patched version with the stress tool as advised by Andrey 
and did not see any problems in dmesg anymore.

If ok I can provide a proper patch.

Many thanks,
Oliver


diff --git a/net/can/bcm.c b/net/can/bcm.c
index 8e999ff..8af9d25 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1549,24 +1549,31 @@ static int bcm_connect(struct socket *sock, 
struct sockaddr *uaddr, int len,

struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
struct sock *sk = sock->sk;
struct bcm_sock *bo = bcm_sk(sk);
+   int ret = 0;

if (len < sizeof(*addr))
return -EINVAL;

-   if (bo->bound)
-   return -EISCONN;
+   lock_sock(sk);
+
+   if (bo->bound) {
+   ret = -EISCONN;
+   goto fail;
+   }

/* bind a device to this socket */
if (addr->can_ifindex) {
struct net_device *dev;

dev = dev_get_by_index(_net, addr->can_ifindex);
-   if (!dev)
-   return -ENODEV;
-
+   if (!dev) {
+   ret = -ENODEV;
+   goto fail;
+   }
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
-   return -ENODEV;
+   ret = -ENODEV;
+   goto fail;
}

bo->ifindex = dev->ifindex;
@@ -1577,17 +1584,24 @@ static int bcm_connect(struct socket *sock, 
struct sockaddr *uaddr, int len,

bo->ifindex = 0;
}

-   bo->bound = 1;
-
if (proc_dir) {
/* unique socket address as filename */
sprintf(bo->procname, "%lu", sock_i_ino(sk));
bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
 proc_dir,
 _proc_fops, sk);
+   if (!bo->bcm_proc_read) {
+   ret = -ENOMEM;
+   goto fail;
+   }
}

-   return 0;
+   bo->bound = 1;
+
+fail:
+   release_sock(sk);
+
+   return ret;
 }

 static int bcm_recvmsg(struct socket *sock, struct msghdr *msg, size_t 
size,



On 10/24/2016 07:31 PM, Andrey Konovalov wrote:

Hi Cong,

I'm able to reproduce it by running
https://gist.github.com/xairy/33f2eb6bf807b004e643bae36c3d02d7 in a
tight parallel loop with stress
(https://godoc.org/golang.org/x/tools/cmd/stress):
$ gcc -lpthread tmp.c
$ ./stress ./a.out

The C program was generated from the following syzkaller prog:
mmap(&(0x7f00/0x991000)=nil, (0x991000), 0x3, 0x32,
0x, 0x0)
socket(0x1d, 0x80002, 0x2)
r0 = socket(0x1d, 0x80002, 0x2)
connect$nfc_llcp(r0, &(0x7f00c000)={0x27, 0x1, 0x0, 0x5,
0x1, 0x1,
"341b3a01b257849ca1d7d1ff9f999d8127b185f88d1d775d59c88a3aa6a8ddacdf2bdc324ea6578a21b85114610186c3817c34b05eaffd2c3f54f57fa81ba0",
0x1ff}, 0x60)
connect$nfc_llcp(r0, &(0x7f991000-0x60)={0x27, 0x1, 0x1,
0x5, 0xfffd, 0x0,
"341b3a01b257849ca1d7d1ff9f999d8127b185f88d1d775dbec88a3aa6a8ddacdf2bdc324ea6578a21b85114610186c3817c34b05eaffd2c3f54f57fa81ba0",
0x1ff}, 0x60)

Unfortunately I wasn't able to create a simpler reproducer.

Thanks!

On Mon, Oct 24, 2016 at 6:58 PM, Cong Wang  wrote:

On Mon, Oct 24, 2016 at 9:21 AM, Andrey Konovalov  wrote:

Hi,

I've got the following error report while running the syzkaller fuzzer:

WARNING: CPU: 0 PID: 32451 at fs/proc/generic.c:345 proc_register+0x25e/0x300
proc_dir_entry 'can-bcm/249757' already registered
Kernel panic - not syncing: panic_on_warn set ...


Looks like we have two problems here:

1) A check for bo->bcm_proc_read != NULL seems missing
2) We need to lock the sock in bcm_connect().

I will work on a patch. Meanwhile, it would help a lot if you could provide
a reproducer.

Thanks!

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

[Oliver Hartkopp]

On 08/11/2016 10:58 AM, Andreas Werner wrote:

On Thu, Aug 11, 2016 at 10:45:00AM +0200, Oliver Hartkopp wrote:




When you still have the possibility to change the IP core I would suggest to
create some kind of 16/32 bit value which you can pass to the CAN controller
along with the CAN frame to be sent.

And when this frame comes back due to the loopback you can use this non-zero
16/32 bit value to match into a list of tx skb pointers for IFF_ECHO.

E.g. when this 16/32 bit value is zero this CAN frame obviously was received
from another CAN node.

Just an idea.



I am not sure if we have a way to change the IP but i will try to talk with
my IC designer. He will be available next week.

Your idea sounds good. I will check a few more driver to get more information
how they did the implementation.



I just looked into your patch at 
http://marc.info/?l=linux-can=146952497113100=2


The

struct men_z192_cf_buf {
u32 can_id;
u32 data[2];
u32 length;
};

has a u32 for the length which is masked by

#define MEN_Z192_CFBUF_LEN  GENMASK(3, 0)

in men_z192_read_frame() and is just copied in men_z192_xmit()

writel(cf->can_dlc, _buf->length);

as only 4 bits are used in the u32 length you probably already can use 
the upper 16 bits for the discussed IFF_ECHO purpose.


Don't know how your IP core handles this u32 length when you enable the 
loopback - maybe the upper 16 bits are still there in the receive path 
and you can implement this idea directly :-)


Regards,
Oliver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

[Oliver Hartkopp]

On 08/11/2016 09:14 AM, Andreas Werner wrote:

On Wed, Aug 10, 2016 at 10:28:45PM +0200, Oliver Hartkopp wrote:



Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have IFF_ECHO
set - but implement it in a different way without using the provided
machanism from dev.c .



Ok I am with you.


Great :-)


A local loopback inside the CAN controller which is generated after
successful transmit is an excellent implementation with excellent
timestamps. The only problem for you is to detect the looped CAN frames and
match them to the skb pointer of the outgoing frame to 'receive' the correct
echo skb.



At the moment, i think there is no way to detect those looped frames.
I will talk to our IC designer and discuss this issue with him. Maybe we
have the possibility to get a local loopback inside the CAN controller.
This seems to be the best way to do it.


When you still have the possibility to change the IP core I would 
suggest to create some kind of 16/32 bit value which you can pass to the 
CAN controller along with the CAN frame to be sent.


And when this frame comes back due to the loopback you can use this 
non-zero 16/32 bit value to match into a list of tx skb pointers for 
IFF_ECHO.


E.g. when this 16/32 bit value is zero this CAN frame obviously was 
received from another CAN node.


Just an idea.

Regards,
Oliver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

[Oliver Hartkopp]

Hi Andreas,

On 08/09/2016 08:10 AM, Andreas Werner wrote:

On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote:



You specify here one echo_skb but it's not used anywhere. Local loopback
seems not to be implemented.



Agree with you, will set it to "0".


No, the local loopback is mandetory!



Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c
it is not mandatory.


It is.

Even those drivers that show up to use zero echo skbs in alloc_candev() 
implement the echo functionality correct.


Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have 
IFF_ECHO set - but implement it in a different way without using the 
provided machanism from dev.c .



In the Documentation/networking/can.txt
there is also a "should" and a fallback mechnism if the driver
does not support the local loopback.


But this fallback mechanism is bad - really bad!

E.g. the slcan.c driver sends a stream of CAN frames without knowing 
whether the frames ever hit the wire. The slcan driver is more less for 
hobby users. The CAN frame echo with IFF_ECHO gives a correct 
representation of the traffic on the wire - including the correct 
timestamps.


You really want to know whether a CAN frame was sent correctly on the 
bus instead of getting some shortcut info from inside the network layer.

.


Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.



Sure...


I'm currently ok with this fallback mechanism.


/me not.


Anyway I am not sure that the driver can handle the echo skb correctly.
If i understand it correctly, the can_get_echo_skb() is normally called
on a "TX done IRQ" to get the skb and loop it back.


ack.


I do not have such a "TX done IRQ" and have not implemented implemented
and added the local looback.


I'm not really sure how grcan.c and janz-ican3.c implemented the echo 
functionality but they must have faced a similar situation.


A local loopback inside the CAN controller which is generated after 
successful transmit is an excellent implementation with excellent 
timestamps. The only problem for you is to detect the looped CAN frames 
and match them to the skb pointer of the outgoing frame to 'receive' the 
correct echo skb.


When you send CAN frames to an unconnected CAN bus it can't be sent out 
due to the missing acknowledge from other nodes. So when you send frames 
and you get echo frames due to the fallback mode your cool CAN 
controller degrades to slcan level.


Regards,
Oliver

ps. Do you have any URL where one can get the MEN 16Z192 spec?

Re: [PATCH 2/3] can: fix oops caused by wrong rtnl dellink usage

[Oliver Hartkopp]

On 06/28/2016 09:36 AM, Holger Schurig wrote:

static void can_dellink(struct net_device *dev, struct list_head *head);

and

static void can_dellink(struct net_device *dev, struct list_head *head)
{
return;
}


Wouldn't the canonical form be this:

static void can_dellink(struct net_device *dev, struct list_head *head)
{
}


- the curly braces make sure this isn't a forward definition
- but no useless return either


But then again, this "return" is only cosmetical.


Yes it is just coding style.

> No compiler will

generate any code from it.


ACK.

If you check

~/linux$ git grep \{\ return\;

there are many occurrences of empty void functions having a 'return' 
inside the curly braces.


I think

static void can_dellink( ... ){}

would have made it too.

Now can_dellink() just locks similar to can_newlink() some lines above.

Regards,
Oliver

Re: [PATCH 2/3] can: fix oops caused by wrong rtnl dellink usage

[Oliver Hartkopp]

On 06/23/2016 03:09 PM, Sergei Shtylyov wrote:


+static void can_dellink(struct net_device *dev, struct list_head
*head)
+{
+return;


   Why?



http://marc.info/?l=linux-can=146651600421205=2

The same reason as for commit 993e6f2fd.


   I was asking just about the useless *return* statement...



Ah!

I did some investigation before whether using 'return' in empty void 
functions or not.


static void can_dellink(struct net_device *dev, struct list_head *head);

and

static void can_dellink(struct net_device *dev, struct list_head *head)
{
return;
}

do the same job, right?

But the first one looks like a forward declaration and you would try to 
find the 'implementing' function then.


Of course you can write less code and both implementations are correct - 
but this representation makes it pretty clear that here's nothing to do :-)


Regards,
Oliver

Re: [PATCH 2/3] can: fix oops caused by wrong rtnl dellink usage

[Oliver Hartkopp]

On 06/23/2016 02:55 PM, Sergei Shtylyov wrote:

Hello.

On 6/23/2016 12:22 PM, Marc Kleine-Budde wrote:


From: Oliver Hartkopp <socket...@hartkopp.net>

For 'real' hardware CAN devices the netlink interface is used to set CAN
specific communication parameters. Real CAN hardware can not be
created nor
removed with the ip tool ...

This patch adds a private dellink function for the CAN device driver
interface
that does just nothing.

It's a follow up to commit 993e6f2fd ("can: fix oops caused by wrong rtnl
newlink usage") but for dellink.

Reported-by: ajneu <ajn...@gmail.com>
Signed-off-by: Oliver Hartkopp <socket...@hartkopp.net>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <m...@pengutronix.de>
---
 drivers/net/can/dev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 348dd5001fa4..ad535a854e5c 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -1011,6 +1011,11 @@ static int can_newlink(struct net *src_net,
struct net_device *dev,
 return -EOPNOTSUPP;
 }

+static void can_dellink(struct net_device *dev, struct list_head *head)
+{
+return;


   Why?



http://marc.info/?l=linux-can=146651600421205=2

The same reason as for commit 993e6f2fd.

Regards,
Oliver


+}
+
 static struct rtnl_link_ops can_link_ops __read_mostly = {
 .kind= "can",
 .maxtype= IFLA_CAN_MAX,

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v5 1/2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

On 06/03/2016 07:03 PM, Ulrich Hecht wrote:


Thanks; I missed that every register is described twice.

Nevertheless, names often vary more or less subtly between your patch
and the specs, making it very hard to review. Some have letters added,
some have letters removed, and some are just plain confusing. For
instance, RCANFD_DCFG_* apparently does not describe, as one might
think, RSCFDnCFDCmDCFG, but RSCFDnCFDCmFDCFG. These names are, of
course, completely ridiculous, but inventing a new set makes things
even worse, IMO.


???

You suggest to use 'completely ridiculous' definitions in favor to 
definitions that have a proper name space RCANFD_ ?


When there is a more readable way that maintains proper readable code 
there's no reason to adopt crappy definitions just because some chip 
designer has no clue how to design proper register names.


When there's some mapping from RSCFDnCFDCmFDCFG to RCANFD_DCFG_* this 
could be mentioned in the comments.


But I'm totally against these blurry upper/lower case letter stuff for 
register definitions.


Regards,
Oliver




At least for the bits, I'd stick with the names given in the
datasheet. They usually make a modicum of sense, and it makes it way
easier to search for them. It would also help if the bits were sorted
consistently.

CU
Uli

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

Hello Ramesh,

please send out a new v3 patchset to trigger the process again :-)

Best regards,
Oliver

On 04/13/2016 08:25 AM, Ramesh Shanmugasundaram wrote:

HI Marc,

Gentle reminder!
Are you happy with the open comment's disposition? I can send a next version of 
patch if we have a closure on current set of comments.

Thanks,
Ramesh


-Original Message-
From: Ramesh Shanmugasundaram
Sent: 01 April 2016 13:49
To: Ramesh Shanmugasundaram ;
w...@grandegger.com; robh...@kernel.org; pawel.m...@arm.com;
mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk; ga...@codeaurora.org;
cor...@lwn.net
Cc: linux-renesas-...@vger.kernel.org; devicet...@vger.kernel.org; linux-
c...@vger.kernel.org; netdev@vger.kernel.org; linux-...@vger.kernel.org;
geert+rene...@glider.be; Chris Paterson 
Subject: RE: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

Hi Marc,

Thanks for your time & review comments.


-Original Message-

(...)

+#define RCANFD_NAPI_WEIGHT 8   /* Rx poll quota */
+
+#define RCANFD_NUM_CHANNELS2
+#define RCANFD_CHANNELS_MASK   0x3 /* Two channels max */


(BIT(RCANFD_NUM_CHANNELS) - 1


OK




+
+/* Rx FIFO is a global resource of the controller. There are 8 such

FIFOs

+ * available. Each channel gets a dedicated Rx FIFO (i.e.) the
+ channel

(...)

+#define RCANFD_CMFIFO_CFDLC(x) (((x) & 0xf) << 28)
+#define RCANFD_CMFIFO_CFPTR(x) (((x) & 0xfff) << 16)
+#define RCANFD_CMFIFO_CFTS(x)  (((x) & 0xff) << 0)
+
+/* Global Test Config register */
+#define RCANFD_GTSTCFG_C0CBCE  BIT(0)
+#define RCANFD_GTSTCFG_C1CBCE  BIT(1)
+
+#define RCANFD_GTSTCTR_ICBCTME BIT(0)
+
+/* AFL Rx rules registers */
+#define RCANFD_AFLCFG_SETRNC0(x)   (((x) & 0xff) << 24)
+#define RCANFD_AFLCFG_SETRNC1(x)   (((x) & 0xff) << 16)


What about something like:

#define RCANFD_AFLCFG_SETRNC(n, x)  (((x) & 0xff) << (24 - n * 8))

This will save some if()s in the code


Nice :-). Will update.




+#define RCANFD_AFLCFG_GETRNC0(x)   (((x) >> 24) & 0xff)
+#define RCANFD_AFLCFG_GETRNC1(x)   (((x) >> 16) & 0xff)
+
+#define RCANFD_AFL_PAGENUM(entry)  ((entry) / 16)

(...)

+#define rcar_canfd_read(priv, offset)  \
+   readl(priv->base + (offset))
+#define rcar_canfd_write(priv, offset, val)\
+   writel(val, priv->base + (offset))
+#define rcar_canfd_set_bit(priv, reg, val) \
+   rcar_canfd_update(val, val, priv->base + (reg))
+#define rcar_canfd_clear_bit(priv, reg, val)   \
+   rcar_canfd_update(val, 0, priv->base + (reg))
+#define rcar_canfd_update_bit(priv, reg, mask, val)\
+   rcar_canfd_update(mask, val, priv->base + (reg))


please use static inline functions instead of defines.


OK.




+
+static void rcar_canfd_get_data(struct canfd_frame *cf,
+   struct rcar_canfd_channel *priv, u32 off)


Please use "struct rcar_canfd_channel *priv" as first argument, struct
canfd_frame *cf as second. Remove off, as the offset is already
defined by the channel.


I'll re-order priv, cf as you mentioned. I'll leave "off" arg as it is
because it is based on FIFO number of channel + mode (CAN only or CANFD
only). Otherwise, I will have to add another check inside this function
for mode.


+{
+   u32 i, lwords;
+
+   lwords = cf->len / sizeof(u32);
+   if (cf->len % sizeof(u32))
+   lwords++;


Use DIV_ROUND_UP() instead of open coding it.


Agreed. Thanks.




+   for (i = 0; i < lwords; i++)
+   *((u32 *)cf->data + i) =
+   rcar_canfd_read(priv, off + (i * sizeof(u32))); }
+
+static void rcar_canfd_put_data(struct canfd_frame *cf,
+   struct rcar_canfd_channel *priv, u32 off)


same here


Yes (same as _get_data)




+{
+   u32 i, j, lwords, leftover;
+   u32 data = 0;
+
+   lwords = cf->len / sizeof(u32);
+   leftover = cf->len % sizeof(u32);
+   for (i = 0; i < lwords; i++)
+   rcar_canfd_write(priv, off + (i * sizeof(u32)),
+*((u32 *)cf->data + i));


Here you don't convert the endianess...


Yes


+
+   if (leftover) {
+   u8 *p = (u8 *)((u32 *)cf->data + i);
+
+   for (j = 0; j < leftover; j++)
+   data |= p[j] << (j * 8);


...here you do an implicit endianess conversion. "data" is little
endian, while p[j] is big endian.


Not sure I got the question correctly.
Controller expectation of data bytes in 32bit register is bits[7:0] =
byte0, bits[15:8] = byte1 and so on - little endian.
For e.g. if cf->data points to byte stream H'112233445566 (cf->data[0] =
0x11), first rcar_canfd_write will write 0x44332211 value to register. Yes
the host cpu is assumed little endian. In leftover case, data will be
0x6655 - again little endian.
I think I 

Re: [PATCH] can: m_can: fix bitrate setup on latest silicon

[Oliver Hartkopp]

On 04/26/2016 03:46 PM, Florian Vallee wrote:

According to the m_can user manual changelog the BTP register layout was
updated with core revision 3.1.0


Hello Florian,

nice to see a real v3.1.0 user emerging on the mailing list :-)

I wonder whether this small change covers the updates made between
v3.0.1 and v3.1.0.

IIRC

v3.0.1:
NON_ISO operation / general CAN/CANFD and BRS switch in register CCCR
v3.1.0:
ISO operation / per frame CAN/CANFD switch (FDF/BRS bit in TX/RX buffer)
v3.2.x:
Ability to switch ISO/NON_ISO operation (NISO bit on register CCCR)
updated range of NBTP.NTSEG2

The current v3.0.1 driver is fixed to tell to be a NON_ISO controller:

https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/diff/drivers/net/can/m_can/m_can.c?h=testing

The v3.1.0 is a fixed ISO controller and additional it does not make 
sense anymore to configure the CCCR register each time, before sending a 
frame:


https://git.kernel.org/cgit/linux/kernel/git/mkl/linux-can-next.git/tree/drivers/net/can/m_can/m_can.c?h=testing=885cc17abad6c3064f266099a6ded2d357012380#n1075

Just as the new TX/RX buffer layout contains a FDF and BRS bits for this 
reason.



+static inline int m_can_read_core_rev(const struct m_can_priv *priv)
+{
+   u32 reg = m_can_read(priv, M_CAN_CREL);
+
+   return ((reg >> CRR_REL_SHIFT) & CRR_REL_MASK);
+}
+
  static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
  {
struct net_device_stats *stats = >stats;
@@ -814,8 +838,16 @@ static int m_can_set_bittiming(struct net_device *dev)
sjw = bt->sjw - 1;
tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
tseg2 = bt->phase_seg2 - 1;
-   reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
-   (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
+
+   if (m_can_read_core_rev(priv) < M_CAN_COREREL_3_1_0)


Your patch looks very good so far. I would appreciate if you could 
update the other register changes too as I don't have a hardware to 
test. I can provide the ISO/NON_ISO config for the netlink interface 
updates then :-)


Best regards,
Oliver

Re: Poorer networking performance in later kernels?

[Oliver Hartkopp]

On 04/19/2016 04:54 PM, Butler, Peter wrote:



I think the issue is resolved.  I had to recompile my 4.4.0 kernel with a few 
options pertaining to the Intel NIC which somehow (?) got left out or otherwise 
clobbered when I ported my 3.4.2 .config to the 4.4.0 kernel source tree.  With 
those changes now in I see essentially identical performance with the two 
kernels.  Sorry for any confusion and/or waste of time here.  My bad.



Can you please send the relevant changes in the config that caused the 
discussed issue?


Just in the case other people do a similar kernel upgrade from 3.x to a 
recent kernel and the current defaults lead to this non-optimal result.


Thanks,
Oliver

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

Hi Ramesh,

On 03/11/2016 08:14 AM, Ramesh Shanmugasundaram wrote:


As we are fixing this issue in CAN dev.c, I'll remove this check in ndo_open and 
set CAN_CTRLMODE_FD flag in ctrlmode & remove the flag in ctrlmode_supported in 
the next v3 version of the patch.


I posted a V2 version of that fix some minutes ago.
It also adds some documentation and a new variable ctrlmode_static which 
makes it clear how to specify static/fixed control modes.


Please use the new can_set_static_ctrlmode() helper as suggested in the 
changes for m_can.c.


A feedback is welcome whether this new fix fits your expectations.


Are there any further comments on v2 patch please?


Besides the stuff I wrote above: No :-)

Best regards,
Oliver

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

On 03/08/2016 01:48 PM, Ramesh Shanmugasundaram wrote:

>> In fact you provided a CAN driver which is "CAN-FD-only".
> 
> Yes. That's the status of current submission.
> 
>> We did not had that before but there's a solution for this kind of setup.
>>
>> There is a similar case with CAN_CTRLMODE_FD_NON_ISO we had on the M_CAN
>> driver which only provides non-ISO configuration for the supported IP core
>> and _no_ option to _change_ this value:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id
>> =6cfda7fbebe8a4fd33ea5722fa0212f98f643c35
>>
>> If you would do it similar in rcar_canfd.c with
>>
>> /* CAN_CTRLMODE_FD is fixed with R-Car CAN FD */
>> priv->can.ctrlmode = CAN_CTRLMODE_FD;
>>
>> and remove CAN_CTRLMODE_FD from the priv->can.ctrlmode_supported
>> assignment then it should do the entire configuration process correctly
>> for you.
>> Including the proper tests for the two bitrates.
>> (see open_candev() in linux/drivers/net/can/dev.c)
>>
>> Right?
> 
> I did try this option earlier but there are two problems with this method.
> 
> 1) Below configuration is not possible
> 
> ip link set can0 up type can bitrate 100 dbitrate 100 fd on
> 
> "fd on" -> This is not allowed because CAN_CTRLMODE_FD bit is not set in 
> ctrlmode_supported.
> 
> 2) If I ignore "fd on", my interface MTU stays as CAN_MTU only. If I have to 
> change the MTU alone to CANFD_MTU using another netlink message, it again 
> checks ctrlmode_supported where it would fail. I have the option of providing 
> my own change_mtu function & ignore this check but two configuration messages 
> are required for my driver alone :-(.
> 
> Both these anomalies are addressed with the current check I have. 

Oh - you are right with complaining about this inconsistency.

Can you check my RFC patch for Linux stable I just sent on the mailing list?
http://marc.info/?l=linux-can=145745724917976=2

Many thanks,
Oliver

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

Hi Ramesh,

On 08.03.2016 09:57, Ramesh Shanmugasundaram wrote:



As you mentioned further down, when a user does this

"ip link set can0 up type can bitrate 100"

the intention is to put the controller in CAN 2.0 mode. Even if we use a status 
flag or copy the data bitrate equal to the nominal bitrate, what would it achieve? 
It still cannot be a CAN 2.0 node - it is a CAN FD node configured with same 
nominal & data bitrate.

This is why I have this check in ndo_open, so that the user is aware it is a 
CAN FD node always and avoid misconfiguration like above with EOPNOTSUPP.



ok - got it now.

In fact you provided a CAN driver which is "CAN-FD-only".
We did not had that before but there's a solution for this kind of setup.

There is a similar case with CAN_CTRLMODE_FD_NON_ISO we had on the M_CAN 
driver which only provides non-ISO configuration for the supported IP core and 
_no_ option to _change_ this value:


https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cfda7fbebe8a4fd33ea5722fa0212f98f643c35

If you would do it similar in rcar_canfd.c with

/* CAN_CTRLMODE_FD is fixed with R-Car CAN FD */
priv->can.ctrlmode = CAN_CTRLMODE_FD;

and remove CAN_CTRLMODE_FD from the priv->can.ctrlmode_supported assignment 
then it should do the entire configuration process correctly for you.

Including the proper tests for the two bitrates.
(see open_candev() in linux/drivers/net/can/dev.c)

Right?

Best regards,
Oliver

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

On 03/07/2016 09:32 AM, Ramesh Shanmugasundaram wrote:

> + /* Ensure channel starts in FD mode */
> + if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
> + netdev_err(ndev, "enable can fd mode for channel %d\n", ch);
> + goto fail_mode;
> + }

 What's the reason behind this check?

 A CAN FD capable CAN controller can be either configured to run as
 CAN 2.0 (Classic CAN) or as CAN FD controller.

 So why are to throwing an error here and produce an initialization
 failure?
>>>
>>> When this controller is configured in FD mode and used only with CAN
>>> 2.0 nodes, it still expects a DTSEG (data bitrate) configuration same
>>> as NTSEG (nominal bitrate). This check, specifically in ndo_open,
>>> ensures both are configured and will work fine with CAN 2.0 nodes
>>> (e.g.)
>>>
>>> "ip link set can0 up type can bitrate 100 dbitrate 100 fd on"
>>>
>>> If I don't have this check, a configuration like this
>>>
>>> "ip link set can0 up type can bitrate 100"
>>>
>>> will bring up the controller without DTSEG configured.

What about spending some status flag or setting the data bitrate equal to the
nominal bitrate unless a data bitrate is provided?

>>
>> That should bring up the controller in CAN 2.0 mode.
> 
> Yes, that's the user's intention but the manual states DTSEG still need to be 
> configured. In the above configuration, it will not be.
> Besides, this will not be a "pure" CAN 2.0 node (i.e.) if a frame with length 
> > 8 bytes is received the controller will "ACK" because in FD mode it can 
> receive up to 64 bytes frame.

Oh. We probably mix something up here (CAN frame formats & bitrates).

A CAN2.0 frame and a CAN FD frame have very different representations on the
wire! So if you see a FDF (former EDL) bit this is a CAN FD frame, which
requires two bitrates (nominal/data bitrate) where the data bitrate has to be
greater or equal the nominal bitrate.

The fact that the data bitrate is equal the nominal/arbitration bitrate has
nothing to do with CAN2.0 then. Regarding your answer this is not even "a pure
CAN2.0" node - it still looks like a CAN FD node with equal data/nominal 
bitrates.

The fact that a CAN FD frame has a size of 8 bytes doesn't make it a CAN2.0
frame :-)

> 
> The controller does support a "pure" classical CAN mode with a different set 
> of register map itself.

Is this a can_rcar controller register mapping then?

> Do you think a pure CAN 2.0 mode support would be beneficial? I can submit 
> this in coming days on top of current submission.
> 
> The current submission status is:
>  - Controller operates in CAN FD mode only.
>  - If needed to interoperate with CAN 2.0 nodes, data bitrate still need to 
> be configured and it will work perfectly. However, it is not a "pure" CAN 2.0 
> node as mentioned above.

When you have a CAN FD /capable/ controller the idea is:

"ip link set can0 up type can bitrate 100"

The controller is in CAN2.0 mode:

1. It can send and receive CAN2.0 frames @1MBit/s.
2. The MTU is set to 16 (sizeof(struct can_frame)) ; CAN_CTRLMODE_FD is unset.
3. The CAN controller is not CAN FD tolerant (will produce error frames)

"ip link set can0 up type can bitrate 100 dbitrate 100 fd on"

1. It can send and receive CAN2.0 frames @1MBit/s.
2. It can send and receive CAN FD frames @1MBit/s (arbitration bitrate).
3. The MTU is set to 72 (sizeof(struct canfd_frame)) ; CAN_CTRLMODE_FD is set.

For CAN FD frames the data bitrate can be increased like:
"ip link set can0 up type can bitrate 100 dbitrate 400 fd on"

So when CAN_CTRLMODE_FD is unset the controller should act like a "pure
CAN2.0" node. When people configure a CAN FD controller with "fd on" and use
CAN2.0 frames all the time this is ok either - but the controller is able to
process CAN FD frames with the correct bitrate too.

Regards,
Oliver

Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:

> Changes since v1:
>   * Removed testmodes & debugfs code (suggested by Oliver H)
>   * Fixed tx path race issue by introducing lock (suggested by Marc K)
>   * Removed __maybe_unused attribute of rcar_canfd_of_table
> 


>  obj-$(CONFIG_CAN_M_CAN)  += m_can/
>  obj-$(CONFIG_CAN_RCAR)   += rcar_can.o
> +obj-$(CONFIG_CANFD_RCAR) += rcar_canfd.o

Just for the sake of an ordered directory structure:

Would it make sense to create a rcar directory where rcar_can.c and
rcar_canfd.c are placed?

Additionally (besides of one accident with the obsolete PCH_CAN) all CAN
drivers start with CONFIG_CAN_...

Following that scheme the config option should be named

CONFIG_CAN_RCAR_CANFD

as we had in CONFIG_CAN_IFI_CANFD.


>  obj-$(CONFIG_CAN_SJA1000)+= sja1000/
>  obj-$(CONFIG_CAN_SUN4I)  += sun4i_can.o
>  obj-$(CONFIG_CAN_TI_HECC)+= ti_hecc.o
> diff --git a/drivers/net/can/rcar_canfd.c b/drivers/net/can/rcar_canfd.c

(..)

> +/* Channel priv data */
> +struct rcar_canfd_channel {
> + struct can_priv can;/* Must be the first member */
> + struct net_device *ndev;
> + struct rcar_canfd_global *gpriv;/* Controller reference */
> + void __iomem *base; /* Register base address */
> +#ifdef CONFIG_DEBUG_FS
> + struct dentry *dev_root;
> +#endif /* CONFIG_DEBUG_FS */

Some missed stuff from debugfs removal?

> + struct napi_struct napi;
> + u8  tx_len[RCANFD_FIFO_DEPTH];  /* For net stats */
> + u32 tx_head;/* Incremented on xmit */
> + u32 tx_tail;/* Incremented on xmit done */
> + u32 channel;/* Channel number */
> + spinlock_t tx_lock; /* To protect tx path */
> +};

(..)

> +static int rcar_canfd_start(struct net_device *ndev)
> +{
> + struct rcar_canfd_channel *priv = netdev_priv(ndev);
> + int err = -EOPNOTSUPP;
> + u32 sts, ch = priv->channel;
> + u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> + /* Ensure channel starts in FD mode */
> + if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
> + netdev_err(ndev, "enable can fd mode for channel %d\n", ch);
> + goto fail_mode;
> + }

What's the reason behind this check?

A CAN FD capable CAN controller can be either configured to run as CAN 2.0
(Classic CAN) or as CAN FD controller.

So why are to throwing an error here and produce an initialization failure?

Regards,
Oliver

Re: [PATCH V2 2/4] net: can: ifi: Fix TX DLC configuration

[Oliver Hartkopp]

Hi Marek,

sorry for picking this patch up again.

After looking around in the original source I have one more comment:

On 03/02/2016 11:42 AM, Marek Vasut wrote:

> - if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
> - if (can_is_canfd_skb(skb)) {
> - txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
> - if (cf->flags & CANFD_BRS)
> - txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
> - }
> + txdlc |= can_len2dlc(cf->len);

At the top of the function you defined

u32 txst, txid;
u32 txdlc = 0;

and here you use 'txdlc |= ...'.

That's weird style IMO.

Please change it to

u32 txst, txid, txdlc;

and

txdlc = can_len2dlc(cf->len);


Thanks,
Oliver

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Oliver Hartkopp]

On 03/01/2016 10:27 PM, Marek Vasut wrote:
> On 03/01/2016 07:11 PM, Oliver Hartkopp wrote:
> 
> Hi!
> 
>> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>>> The TX DLC, the transmission length information, was not written
>>> into the transmit configuration register. When using the CAN core
>>> with different CAN controller, the receiving CAN controller will
>>> receive only the ID part of the CAN frame, but no data at all.
>>>
>>> This patch adds the TX DLC into the register to fix this issue.
>>>
>>> Signed-off-by: Marek Vasut <ma...@denx.de>
>>> Cc: Marc Kleine-Budde <m...@pengutronix.de>
>>> Cc: Mark Rutland <mark.rutl...@arm.com>
>>> Cc: Oliver Hartkopp <socket...@hartkopp.net>
>>> Cc: Wolfgang Grandegger <w...@grandegger.com>
>>> ---
>>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c 
>>> b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> index 72f5205..82a33bd 100644
>>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct 
>>> sk_buff *skb,
>>>  
>>> if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>>> if (can_is_canfd_skb(skb)) {
>>> +   txdlc |= can_len2dlc(cf->len);
>>> txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>> if (cf->flags & CANFD_BRS)
>>> txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>> +   } else {
>>> +   txdlc |= cf->len;
>>> }
>>> +   } else {
>>> +   txdlc |= cf->len;
>>> }
>>
>> Please use
>>
>>  txdlc |= can_len2dlc(cf->len);
>>
>> by default (it works for CAN and CAN FD).
>>
>> So that it looks more like:
>>
>>  txdlc |= can_len2dlc(cf->len);
> 
> Roger.
> 
>>  if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
>>  txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>  if (cf->flags & CANFD_BRS)
>>  txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>  }
>>
>> Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
>> This configuration bit is just for the protocol on the wire and is no
>> distinction for CAN / CAN FD.
> 
> So CAN_CTRLMODE_FD is always set if the system operates in CAN/FD mode.

Not the 'system' but this specific CAN netdevice.

> And in addition to that, if the system operates in CAN/FD BOSCH mode,
> the CAN_CTRLMODE_FD_NON_ISO is set. Do I understand it correctly ?

Yep! ('the CAN netdev')

Regards,
Oliver

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Oliver Hartkopp]

Hi Marek,

On 03/01/2016 10:23 PM, Marek Vasut wrote:
> On 03/01/2016 06:49 PM, Oliver Hartkopp wrote:


>>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK0x3ff
>>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK0x7ff
>>>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK0x1fff
>>
>> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
>> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.
>>
>> You won't have trapped into this problem then :-)
> 
> These are register bitfield definitions, so should I really ?
> 
> My OCD kicks in and tells me it'd be odd and inconsistent with the rest
> of the bitfields, but if you prefer it that way, I'll just send an
> updated patch.
> 

Your bit mask is masking the CAN ID out of a given variable.
That's what CAN_SFF_MASK and CAN_EFF_MASK is made for.

So at least it should be:

#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK CAN_SFF_MASK
#define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK CAN_EFF_MASK

Btw. These defines are _never_ referenced in ifi_canfd.c so they should be
removed anyway.

Regards,
Oliver

Re: [PATCH] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp]

On 03/01/2016 10:34 AM, Ramesh Shanmugasundaram wrote:
> This patch adds support for the CAN FD controller found in Renesas R-Car
> SoCs. The controller operates in CAN FD mode by default. Two test modes
> are available and can be enabled by the "rcar_canfd.testmode" module
> parameter. Refer to Documentation/kernel-parameters.txt.

(..)

> +#ifdef CONFIG_DEBUG_FS
> +#include 
> +
> +static int rcar_canfd_showregs(struct seq_file *s, void *data)
> +{
> + struct rcar_canfd_channel *priv = s->private;
> + u32 ch = priv->channel;
> + u32 rf = ch + RCANFD_RFFIFO_IDX;
> + u32 cf = RCANFD_CFFIFO_IDX;
> + u32 val, offset, i;
> +
> + if (testmode)
> + seq_printf(s, "RCANFD : testmode = %d\n", testmode);
> + seq_printf(s, "RCANFD register dump: channel %u\n", ch);
> + seq_printf(s, "%-40s: %x\n", "g: cfg",
> +rcar_canfd_read(priv, RCANFD_GCFG));

Why do you think you would need this kind of register dumps in debugfs?

This could be interesting for development purposes - but in production drivers
this is pretty unusual and needless.

I would sugest to remove either the testmode (already suggested by Marc) and
this register dump stuff.

Regards,
Oliver

Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

[Oliver Hartkopp]

On 02/29/2016 08:59 PM, Marek Vasut wrote:
> The TX DLC, the transmission length information, was not written
> into the transmit configuration register. When using the CAN core
> with different CAN controller, the receiving CAN controller will
> receive only the ID part of the CAN frame, but no data at all.
> 
> This patch adds the TX DLC into the register to fix this issue.
> 
> Signed-off-by: Marek Vasut <ma...@denx.de>
> Cc: Marc Kleine-Budde <m...@pengutronix.de>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Cc: Oliver Hartkopp <socket...@hartkopp.net>
> Cc: Wolfgang Grandegger <w...@grandegger.com>
> ---
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c 
> b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 72f5205..82a33bd 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff 
> *skb,
>  
>   if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>   if (can_is_canfd_skb(skb)) {
> + txdlc |= can_len2dlc(cf->len);
>   txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>   if (cf->flags & CANFD_BRS)
>   txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
> + } else {
> + txdlc |= cf->len;
>   }
> + } else {
> + txdlc |= cf->len;
>   }

Please use

txdlc |= can_len2dlc(cf->len);

by default (it works for CAN and CAN FD).

So that it looks more like:

txdlc |= can_len2dlc(cf->len);
if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
if (cf->flags & CANFD_BRS)
txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
}

Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
This configuration bit is just for the protocol on the wire and is no
distinction for CAN / CAN FD.

Best regards,
Oliver

>  
>   if (cf->can_id & CAN_RTR_FLAG)
> 

Re: [PATCH 3/4] net: can: ifi: Fix RX and TX ID mask

[Oliver Hartkopp]

Hi Marek,

On 02/29/2016 08:59 PM, Marek Vasut wrote:
> The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes
> the incorrect mask, which caused the CAN IDs to miss the MSBit both
> on receive and transmit.
> 
> Signed-off-by: Marek Vasut <ma...@denx.de>
> Cc: Marc Kleine-Budde <m...@pengutronix.de>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Cc: Oliver Hartkopp <socket...@hartkopp.net>
> Cc: Wolfgang Grandegger <w...@grandegger.com>
> ---
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c 
> b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 82a33bd..6704098 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -135,7 +135,7 @@
>  
>  #define IFI_CANFD_RXFIFO_ID  0x6c
>  #define IFI_CANFD_RXFIFO_ID_ID_OFFSET0
> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK  0x3ff
> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK  0x7ff
>  #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK  0x1fff

You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of
defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions.

You won't have trapped into this problem then :-)

>  #define IFI_CANFD_RXFIFO_ID_IDE  BIT(29)
>  
> @@ -156,7 +156,7 @@
>  
>  #define IFI_CANFD_TXFIFO_ID  0xbc
>  #define IFI_CANFD_TXFIFO_ID_ID_OFFSET0
> -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK  0x3ff
> +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK  0x7ff
>  #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK  0x1fff

dito.


Regards,
Oliver

>  #define IFI_CANFD_TXFIFO_ID_IDE  BIT(29)
>  
> 

Re: [RFC][PATCH] net: arinc429: Add ARINC-429 stack

[Oliver Hartkopp]

Hi Marek,

On 03.11.2015 22:41, Marek Vasut wrote:

On Tuesday, November 03, 2015 at 10:24:23 PM, Oliver Hartkopp wrote:




Comparing to typical ethernet frames with 1500 bytes the 16 bytes for CAN
frames or 72 bytes for CAN FD frames are already too small in relation to
the socket buffer overhead.

If you want to improve the memory efficiency for arinc290 you should
probably consider to implement a character device based driver instead of
creating a new network protocol family.


See discussion:

http://comments.gmane.org/gmane.linux.kernel/1512019

Which is why I picked the socket variant.



Oh. This provides a completely new perspective.

So far we just discussed about the 32 bit messages to be handled and filtered 
by the kernel.


This could have been done by

1. A chardev interface driver

2. A netdev interface driver (drivers/net/arinc429/...) and using PF_PACKET 
together with Berkley packet filtering (BPF)


3. A netdev interface driver (drivers/net/arinc429/...) and using PF_CAN which 
would lead to a arinc data structure which needs to be struct can_frame 
compatible.


But if you plan to implement the transport protocols (FTP/MAC) inside the 
Linux kernel that have been discussed in the URL above this would be a real 
difference to CAN related protocols. Only from that perspective a new protocol 
family would make sense.


(..)

 From what I've read so far there's also the sending of cyclic messages and
label filtering outside the HW - or why did you copy/paste the can_id/label
filter mechanism from af_can.c ?


I think you might be mixing two people together here, I sent the patch and
Andrey and Aleksander are working for some other interested company.


Sorry. Will try to bash only *you* in the future :-))


The label filtering makes sense if you want to separate what you receive on
which socket in userland, which allows an application to receive only relevant
traffic.


ok.



Hardware-accelerated filtering is another thing and at this point, we should
not mix these two things. Does CAN framework have any such support for hardware
assisted can_id filtering btw ?



No. We discussed about it. But the HW filter capabilities of CAN controllers 
have a wide range of functionality. Most filters make only sense in ECUs that 
pick specific CAN IDs for their functionality. It made no real sense to have 
an administrative configuration interface that limits the view to the CAN bus 
for the entire system. An we never had performance issues with the filtering 
in the softirq context in af_can.c



I'd  prefer to have ARINC framework simple as it could be and separate
from  CAN,  as  these  buses are not similar, besides desire to re-use
SocketCAN interface/API to expose ARINC429 bus.





Maybe you don't need that fancy stuff that comes with PF_CAN. Did you ever
thought about implementing a chardev driver for the ARINC429 hardware?
There are out-of-tree CAN drivers (e.g. can4linux or PEAK System Linux
driver) that handle the transfer of data structures (CAN frames) from/to
kernel space via character device. See an example at
https://en.wikipedia.org/wiki/Can4linux


I guess I can answer that -- yes, I did, see above.



Ok - it seems to stick on the future plan whether it makes sense to implement 
the FTP/MAC transport protocols inside the kernel or not.


What's your thought about that?

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html