2022-05-14 02:50:36

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH 0/2] can: drop tx skb if the device is in listen only mode

In listen only mode, tx CAN frames can still reach the driver if
injected via the packet socket. This series add a check toward
CAN_CTRLMODE_LISTENONLY in can_dropped_invalid_skb() to discard such
skb. The first patch does some preparation work and migrates
can_dropped_invalid_skb() from skb.h to dev.h. The second and last
patch is the actual change.

Vincent Mailhol (2):
can: move can_dropped_invalid_skb from skb.h to dev.h
can: dev: drop tx skb if in listen only mode

include/linux/can/dev.h | 35 +++++++++++++++++++++++++++++++++++
include/linux/can/skb.h | 28 ----------------------------
2 files changed, 35 insertions(+), 28 deletions(-)

--
2.35.1



2022-05-14 03:33:30

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v2 0/2] can: drop tx skb if the device is in listen only mode

In listen only mode, tx CAN frames can still reach the driver if
injected via the packet socket. This series add a check toward
CAN_CTRLMODE_LISTENONLY in can_dropped_invalid_skb() to discard such
skb.

The first patch does some preparation work and migrates
can_dropped_invalid_skb() and can_skb_headroom_valid() from skb.h to
skb.c. This preparation is needed because skb.h does not include
linux/can/dev.h (for struct can_priv) and uapi/linux/can/netlink.h
(for the definition of CAN_CTRLMODE_LISTEONLY) which we need for this
change. The function being already big, better to de-inline them and
move them to a .c file.

The second and last patch is the actual change.


* Changelog *

v1 -> v2

* move can_dropped_invalid_skb() to skb.c instead of dev.h

* also move can_skb_headroom_valid() to skb.c

Vincent Mailhol (2):
can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to
skb.c
can: dev: drop tx skb if in listen only mode

drivers/net/can/dev/skb.c | 65 +++++++++++++++++++++++++++++++++++++++
include/linux/can/skb.h | 59 +----------------------------------
2 files changed, 66 insertions(+), 58 deletions(-)

--
2.35.1


2022-05-14 21:55:13

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v3 0/4] can: can_dropped_invalid_skb() and Kbuild changes

In listen only mode, tx CAN frames can still reach the driver if
injected via the packet socket. This series add a check toward
CAN_CTRLMODE_LISTENONLY in can_dropped_invalid_skb() to discard such
skb.

The fourth and last patch is the actual change. This goal cascaded in
the need to create other patches which will be explained in reverse
order.

The third patch migrates can_dropped_invalid_skb() and
can_skb_headroom_valid() from skb.h to skb.c. This preparation is
needed because skb.h does not include linux/can/dev.h (for struct
can_priv) and uapi/linux/can/netlink.h (for the definition of
CAN_CTRLMODE_LISTEONLY) which we need for this change. The function
being already big, better to de-inline them and move them to a .c
file.

The third patch would not work without some adjustment to Kbuild. VCAN
and VXCAN are users of can_dropped_invalid_skb() but do not depend on
CAN_DEV and thus would not see the symbols from skb.o if
CONFIG_CAN_DEV is not selected. c.f. kernel test robot report on the
v2 of this series [1]. The second patch modifies Kbuild to fix it.

slcan does not depend of can_dropped_invalid_skb() which would make it
the only driver with no dependencies on CAN_DEV. Because I wanted an
excuse to move all the driver under CAN_DEV in the second patch, the
first patch applies can_dropped_invalid_skb() to slcan to make it
dependent.

[1] https://lore.kernel.org/linux-can/[email protected]/


* Changelog *

v2 -> v3

* Apply can_dropped_invalid_skb() to slcan.

* Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
modifying Kbuild.

* fix small typos.

v1 -> v2

* move can_dropped_invalid_skb() to skb.c instead of dev.h

* also move can_skb_headroom_valid() to skb.c

Vincent Mailhol (4):
can: slcan: use can_dropped_invalid_skb() instead of manual check
can: Kconfig: change CAN_DEV into a menuconfig
can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to
skb.c
can: dev: drop tx skb if in listen only mode

drivers/net/can/Kconfig | 33 +++++++++++---------
drivers/net/can/dev/skb.c | 65 +++++++++++++++++++++++++++++++++++++++
drivers/net/can/slcan.c | 4 +--
include/linux/can/skb.h | 59 +----------------------------------
4 files changed, 87 insertions(+), 74 deletions(-)

--
2.35.1


2022-05-14 21:57:52

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
grew a lot over the years to a point which it does not make much sense
to have them defined as static inline in header files. Move those two
functions to the .c counterpart of skb.h.

can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
the declaration is removed from the header. Only
can_dropped_invalid_skb() gets its symbol exported.

While doing so, do a small cleanup: add brackets around the else block
in can_dropped_invalid_skb().

Signed-off-by: Vincent Mailhol <mailhol.vi[email protected]>
---
drivers/net/can/dev/skb.c | 58 ++++++++++++++++++++++++++++++++++++++
include/linux/can/skb.h | 59 +--------------------------------------
2 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 61660248c69e..8b1991130de5 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -252,3 +252,61 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
return skb;
}
EXPORT_SYMBOL_GPL(alloc_can_err_skb);
+
+/* Check for outgoing skbs that have not been created by the CAN subsystem */
+static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
+{
+ /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
+ if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
+ return false;
+
+ /* af_packet does not apply CAN skb specific settings */
+ if (skb->ip_summed == CHECKSUM_NONE) {
+ /* init headroom */
+ can_skb_prv(skb)->ifindex = dev->ifindex;
+ can_skb_prv(skb)->skbcnt = 0;
+
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ /* perform proper loopback on capable devices */
+ if (dev->flags & IFF_ECHO)
+ skb->pkt_type = PACKET_LOOPBACK;
+ else
+ skb->pkt_type = PACKET_HOST;
+
+ skb_reset_mac_header(skb);
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+ }
+
+ return true;
+}
+
+/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
+bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
+{
+ const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+ if (skb->protocol == htons(ETH_P_CAN)) {
+ if (unlikely(skb->len != CAN_MTU ||
+ cfd->len > CAN_MAX_DLEN))
+ goto inval_skb;
+ } else if (skb->protocol == htons(ETH_P_CANFD)) {
+ if (unlikely(skb->len != CANFD_MTU ||
+ cfd->len > CANFD_MAX_DLEN))
+ goto inval_skb;
+ } else {
+ goto inval_skb;
+ }
+
+ if (!can_skb_headroom_valid(dev, skb))
+ goto inval_skb;
+
+ return false;
+
+inval_skb:
+ kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return true;
+}
+EXPORT_SYMBOL_GPL(can_dropped_invalid_skb);
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index fdb22b00674a..182749e858b3 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -31,6 +31,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
struct sk_buff *alloc_can_err_skb(struct net_device *dev,
struct can_frame **cf);
+bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);

/*
* The struct can_skb_priv is used to transport additional information along
@@ -96,64 +97,6 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
return nskb;
}

-/* Check for outgoing skbs that have not been created by the CAN subsystem */
-static inline bool can_skb_headroom_valid(struct net_device *dev,
- struct sk_buff *skb)
-{
- /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
- if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
- return false;
-
- /* af_packet does not apply CAN skb specific settings */
- if (skb->ip_summed == CHECKSUM_NONE) {
- /* init headroom */
- can_skb_prv(skb)->ifindex = dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
-
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- /* perform proper loopback on capable devices */
- if (dev->flags & IFF_ECHO)
- skb->pkt_type = PACKET_LOOPBACK;
- else
- skb->pkt_type = PACKET_HOST;
-
- skb_reset_mac_header(skb);
- skb_reset_network_header(skb);
- skb_reset_transport_header(skb);
- }
-
- return true;
-}
-
-/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
-static inline bool can_dropped_invalid_skb(struct net_device *dev,
- struct sk_buff *skb)
-{
- const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
- if (skb->protocol == htons(ETH_P_CAN)) {
- if (unlikely(skb->len != CAN_MTU ||
- cfd->len > CAN_MAX_DLEN))
- goto inval_skb;
- } else if (skb->protocol == htons(ETH_P_CANFD)) {
- if (unlikely(skb->len != CANFD_MTU ||
- cfd->len > CANFD_MAX_DLEN))
- goto inval_skb;
- } else
- goto inval_skb;
-
- if (!can_skb_headroom_valid(dev, skb))
- goto inval_skb;
-
- return false;
-
-inval_skb:
- kfree_skb(skb);
- dev->stats.tx_dropped++;
- return true;
-}
-
static inline bool can_is_canfd_skb(const struct sk_buff *skb)
{
/* the CAN specific type of skb is identified by its data length */
--
2.35.1


2022-05-15 16:00:55

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v3 4/4] can: dev: drop tx skb if in listen only mode

Frames can be directly injected to a can driver via the packet
socket. By doing that, it is possible to reach the
net_device_ops::ndo_start_xmit function even if the driver is
configured in listen only mode.

Add a check in can_dropped_invalid_skb() to discard the skb if
CAN_CTRLMODE_LISTENONLY is set.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/skb.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 8b1991130de5..f7420fc43b99 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -5,6 +5,7 @@
*/

#include <linux/can/dev.h>
+#include <linux/can/netlink.h>

/* Local echo of CAN messages
*
@@ -286,6 +287,7 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
{
const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+ struct can_priv *priv = netdev_priv(dev);

if (skb->protocol == htons(ETH_P_CAN)) {
if (unlikely(skb->len != CAN_MTU ||
@@ -299,8 +301,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
goto inval_skb;
}

- if (!can_skb_headroom_valid(dev, skb))
+ if (!can_skb_headroom_valid(dev, skb)) {
+ goto inval_skb;
+ } else if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+ netdev_info_once(dev,
+ "interface in listen only mode, dropping skb\n");
goto inval_skb;
+ }

return false;

--
2.35.1


2022-05-16 14:24:36

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

Hi Vincent,

On 14.05.22 16:16, Vincent Mailhol wrote:
> The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
> grew a lot over the years to a point which it does not make much sense
> to have them defined as static inline in header files. Move those two
> functions to the .c counterpart of skb.h.
>
> can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
> the declaration is removed from the header. Only
> can_dropped_invalid_skb() gets its symbol exported.

I can see your point but the need for can-dev was always given for
hardware specific stuff like bitrates, TDC, transceivers, whatever.

As there would be more things in slcan (e.g. dev_alloc_skb() could be
unified with alloc_can_skb()) - would it probably make sense to
introduce a new can-skb module that could be used by all CAN
virtual/software interfaces?

Or some other split-up ... any idea?

Best regards,
Oliver

>
> While doing so, do a small cleanup: add brackets around the else block
> in can_dropped_invalid_skb().
>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
> drivers/net/can/dev/skb.c | 58 ++++++++++++++++++++++++++++++++++++++
> include/linux/can/skb.h | 59 +--------------------------------------
> 2 files changed, 59 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 61660248c69e..8b1991130de5 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -252,3 +252,61 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
> return skb;
> }
> EXPORT_SYMBOL_GPL(alloc_can_err_skb);
> +
> +/* Check for outgoing skbs that have not been created by the CAN subsystem */
> +static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
> +{
> + /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
> + if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
> + return false;
> +
> + /* af_packet does not apply CAN skb specific settings */
> + if (skb->ip_summed == CHECKSUM_NONE) {
> + /* init headroom */
> + can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
> +
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> + /* perform proper loopback on capable devices */
> + if (dev->flags & IFF_ECHO)
> + skb->pkt_type = PACKET_LOOPBACK;
> + else
> + skb->pkt_type = PACKET_HOST;
> +
> + skb_reset_mac_header(skb);
> + skb_reset_network_header(skb);
> + skb_reset_transport_header(skb);
> + }
> +
> + return true;
> +}
> +
> +/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
> +bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> +{
> + const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> + if (skb->protocol == htons(ETH_P_CAN)) {
> + if (unlikely(skb->len != CAN_MTU ||
> + cfd->len > CAN_MAX_DLEN))
> + goto inval_skb;
> + } else if (skb->protocol == htons(ETH_P_CANFD)) {
> + if (unlikely(skb->len != CANFD_MTU ||
> + cfd->len > CANFD_MAX_DLEN))
> + goto inval_skb;
> + } else {
> + goto inval_skb;
> + }
> +
> + if (!can_skb_headroom_valid(dev, skb))
> + goto inval_skb;
> +
> + return false;
> +
> +inval_skb:
> + kfree_skb(skb);
> + dev->stats.tx_dropped++;
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(can_dropped_invalid_skb);
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index fdb22b00674a..182749e858b3 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -31,6 +31,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> struct canfd_frame **cfd);
> struct sk_buff *alloc_can_err_skb(struct net_device *dev,
> struct can_frame **cf);
> +bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
>
> /*
> * The struct can_skb_priv is used to transport additional information along
> @@ -96,64 +97,6 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
> return nskb;
> }
>
> -/* Check for outgoing skbs that have not been created by the CAN subsystem */
> -static inline bool can_skb_headroom_valid(struct net_device *dev,
> - struct sk_buff *skb)
> -{
> - /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
> - if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
> - return false;
> -
> - /* af_packet does not apply CAN skb specific settings */
> - if (skb->ip_summed == CHECKSUM_NONE) {
> - /* init headroom */
> - can_skb_prv(skb)->ifindex = dev->ifindex;
> - can_skb_prv(skb)->skbcnt = 0;
> -
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> - /* perform proper loopback on capable devices */
> - if (dev->flags & IFF_ECHO)
> - skb->pkt_type = PACKET_LOOPBACK;
> - else
> - skb->pkt_type = PACKET_HOST;
> -
> - skb_reset_mac_header(skb);
> - skb_reset_network_header(skb);
> - skb_reset_transport_header(skb);
> - }
> -
> - return true;
> -}
> -
> -/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
> -static inline bool can_dropped_invalid_skb(struct net_device *dev,
> - struct sk_buff *skb)
> -{
> - const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> -
> - if (skb->protocol == htons(ETH_P_CAN)) {
> - if (unlikely(skb->len != CAN_MTU ||
> - cfd->len > CAN_MAX_DLEN))
> - goto inval_skb;
> - } else if (skb->protocol == htons(ETH_P_CANFD)) {
> - if (unlikely(skb->len != CANFD_MTU ||
> - cfd->len > CANFD_MAX_DLEN))
> - goto inval_skb;
> - } else
> - goto inval_skb;
> -
> - if (!can_skb_headroom_valid(dev, skb))
> - goto inval_skb;
> -
> - return false;
> -
> -inval_skb:
> - kfree_skb(skb);
> - dev->stats.tx_dropped++;
> - return true;
> -}
> -
> static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> {
> /* the CAN specific type of skb is identified by its data length */

2022-05-17 10:15:03

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

Hi Oliver,

On Mon. 16 May 2022 at 04:17, Oliver Hartkopp <[email protected]> wrote:
> Hi Vincent,
>
> On 14.05.22 16:16, Vincent Mailhol wrote:
> > The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
> > grew a lot over the years to a point which it does not make much sense
> > to have them defined as static inline in header files. Move those two
> > functions to the .c counterpart of skb.h.
> >
> > can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
> > the declaration is removed from the header. Only
> > can_dropped_invalid_skb() gets its symbol exported.
>
> I can see your point but the need for can-dev was always given for
> hardware specific stuff like bitrates, TDC, transceivers, whatever.

I also see your point :)
Actually, I raised the exact same idea in a previous message:
https://lore.kernel.org/linux-can/[email protected]om/

But you were not in CC and it seems that there is a lot of congestion
recently on the mailing list so I wouldn’t be surprised if you tell me
you did not receive it.

> As there would be more things in slcan (e.g. dev_alloc_skb() could be
> unified with alloc_can_skb())

And also the can_{put,get}_echo_skb() I guess.

> would it probably make sense to
> introduce a new can-skb module that could be used by all CAN
> virtual/software interfaces?
>
> Or some other split-up ... any idea?

My concern is: what would be the merrit? If we do not split, the users
of slcan and v(x)can would have to load the can-dev module which will
be slightly bloated for their use, but is this really an issue? I do
not see how this can become a performance bottleneck, so what is the
problem?
I could also argue that most of the devices do not depend on
rx-offload.o. So should we also split this one out of can-dev on the
same basis and add another module dependency?
The benefit (not having to load a bloated module for three drivers)
does not outweigh the added complexity: all hardware modules will have
one additional modprobe dependency on the tiny can-skb module.

But as said above, I am not fully opposed to the split, I am just
strongly divided. If we go for the split, creating a can-skb module is
the natural and only option I see.
If the above argument does not convince you, I will send a v3 with that split.


Yours sincerely,
Vincent Mailhol

2022-05-17 10:58:51

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue. 17 May 2022 at 15:08, Marc Kleine-Budde <[email protected]> wrote:
> On 17.05.2022 10:50:16, Vincent MAILHOL wrote:
> > > would it probably make sense to
> > > introduce a new can-skb module that could be used by all CAN
> > > virtual/software interfaces?
> > >
> > > Or some other split-up ... any idea?
> >
> > My concern is: what would be the merrit? If we do not split, the users
> > of slcan and v(x)can would have to load the can-dev module which will
> > be slightly bloated for their use, but is this really an issue?
>
> If you use modprobe all required modules are loaded automatically.

Yes, this, now I know. In the past, when I started developing
etas_es58x as an out of tree module (which I inserted using insmod),
it took me a little time to figure out the dependencies tree and which
module I actually had to modprobe separately.

> > I do
> > not see how this can become a performance bottleneck, so what is the
> > problem?
> > I could also argue that most of the devices do not depend on
> > rx-offload.o. So should we also split this one out of can-dev on the
> > same basis and add another module dependency?
>
> We can add a non user visible Kconfig symbol for rx-offload and let the
> drivers that need it do a "select" on it. If selected the rx-offload
> would be compiled into to can-dev module.

Yes, and this remark also applies to can-skb I think.

So slcan, v(x)can and can-dev will select can-skb, and some of the
hardware drivers (still have to figure out the list) will select
can-rx-offload (other dependencies will stay as it is today).

I think that splitting the current can-dev into can-skb + can-dev +
can-rx-offload is enough. Please let me know if you see a need for
more.

> > The benefit (not having to load a bloated module for three drivers)
> > does not outweigh the added complexity: all hardware modules will have
> > one additional modprobe dependency on the tiny can-skb module.
> >
> > But as said above, I am not fully opposed to the split, I am just
> > strongly divided. If we go for the split, creating a can-skb module is
> > the natural and only option I see.
> > If the above argument does not convince you, I will send a v3 with that split.

If both you and Oliver prefer the split, then split it would be!

Because this topic is related to Kbuild, there is actually one thing
which bothered me but which I never asked: why are all the CAN devices
under "Networking support" and not "Device Drivers" in menuconfig like
everything else? Would it make sense to move our devices under the
"Device Drivers" section? I can do it while working on the v3.

2022-05-17 13:30:02

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 10:50:16 +0900
Vincent MAILHOL <[email protected]> wrote:

> My concern is: what would be the merrit? If we do not split, the users
> of slcan and v(x)can would have to load the can-dev module which will
> be slightly bloated for their use, but is this really an issue? I do
> not see how this can become a performance bottleneck, so what is the
> problem?
> I could also argue that most of the devices do not depend on
> rx-offload.o. So should we also split this one out of can-dev on the
> same basis and add another module dependency?
> The benefit (not having to load a bloated module for three drivers)
> does not outweigh the added complexity: all hardware modules will have
> one additional modprobe dependency on the tiny can-skb module.
>
> But as said above, I am not fully opposed to the split, I am just
> strongly divided. If we go for the split, creating a can-skb module is
> the natural and only option I see.
> If the above argument does not convince you, I will send a v3 with
> that split.

I originally replied to PATCHv2 in agreement with what Vincent said in
the first part - I agree that simply moving this code into can-dev and
making v(x)can/slcan depend on it is the straightforward thing to do.

Having yet another kernel module for a tiny bit of code adds more
unnecessary complexity IMHO. And from a user perspective, it seems
fairly natural to have can-dev loaded any time some can0, slcan0,
vcan0, etc. pops up.

Finally, embedded applications that are truly short on memory are
likely using a "real" CAN adapter, and hence already have can-dev
loaded anyway.

I think it's okay to just move it to can-dev and call it a day :)


Max

2022-05-17 14:13:47

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 17.05.2022 10:50:16, Vincent MAILHOL wrote:
> > would it probably make sense to
> > introduce a new can-skb module that could be used by all CAN
> > virtual/software interfaces?
> >
> > Or some other split-up ... any idea?
>
> My concern is: what would be the merrit? If we do not split, the users
> of slcan and v(x)can would have to load the can-dev module which will
> be slightly bloated for their use, but is this really an issue?

If you use modprobe all required modules are loaded automatically.

> I do
> not see how this can become a performance bottleneck, so what is the
> problem?
> I could also argue that most of the devices do not depend on
> rx-offload.o. So should we also split this one out of can-dev on the
> same basis and add another module dependency?

We can add a non user visible Kconfig symbol for rx-offload and let the
drivers that need it do a "select" on it. If selected the rx-offload
would be compiled into to can-dev module.

> The benefit (not having to load a bloated module for three drivers)
> does not outweigh the added complexity: all hardware modules will have
> one additional modprobe dependency on the tiny can-skb module.
>
> But as said above, I am not fully opposed to the split, I am just
> strongly divided. If we go for the split, creating a can-skb module is
> the natural and only option I see.
> If the above argument does not convince you, I will send a v3 with that split.

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.71 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-17 22:55:44

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 17:50:03 +0200
Oliver Hartkopp <[email protected]> wrote:

> On 17.05.22 17:38, Max Staudt wrote:
> > I'm a bit lost - what would CAN_DEV_SW do?
>
> It should be just *one* enabler of building can-dev-ko
>
> > If it enables can_dropped_invalid_skb(), then the HW drivers would
> > also need to depend on CAN_DEV_SW directly or indirectly, or am I
> > missing something?
>
> And CAN_DEV is another enabler that would build the skb stuff from
> CAN_DEV_SW too (but also the netlink stuff).
>
> That's what I meant with "some Makefile magic" which is then building
> the can-dev.ko with the required features depending on CAN_DEV_SW,
> CAN_DEV, CAN_DEV_RX_OFFLOAD, CAN_CALC_BITTIMING, etc

Ah, I see!
Sounds good :)


Max

2022-05-18 00:21:35

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 17.05.22 12:45, Marc Kleine-Budde wrote:
> On 17.05.2022 16:04:53, Vincent MAILHOL wrote:
>> So slcan, v(x)can and can-dev will select can-skb, and some of the
>> hardware drivers (still have to figure out the list) will select
>> can-rx-offload (other dependencies will stay as it is today).
>
> For rx-offload that's flexcan, ti_hecc and mcp251xfd
>
>> I think that splitting the current can-dev into can-skb + can-dev +
>> can-rx-offload is enough. Please let me know if you see a need for
>> more.

After looking through drivers/net/can/Kconfig I would probably phrase it
like this:

Select CAN devices (hw/sw) -> we compile a can_dev module. E.g. to
handle the skb stuff for vcan's.

Select hardware CAN devices -> we compile the netlink stuff into can_dev
and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled into can_dev too.

In the latter case: The selection of flexcan, ti_hecc and mcp251xfd
automatically selects CAN_RX_OFFLOAD which is then also compiled into
can_dev.

Would that fit in terms of complexity?

Best,
Oliver

2022-05-18 00:57:38

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 17.05.2022 16:04:53, Vincent MAILHOL wrote:
> So slcan, v(x)can and can-dev will select can-skb, and some of the
> hardware drivers (still have to figure out the list) will select
> can-rx-offload (other dependencies will stay as it is today).

For rx-offload that's flexcan, ti_hecc and mcp251xfd

> I think that splitting the current can-dev into can-skb + can-dev +
> can-rx-offload is enough. Please let me know if you see a need for
> more.

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (748.00 B)
signature.asc (499.00 B)
Download all attachments

2022-05-18 02:00:49

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 16:35:05 +0200
Oliver Hartkopp <[email protected]> wrote:

> I think it should be even more simple.
>
> When you enter the current Kconfig page of "CAN Device Drivers" every
> selection of vcan/vxcan/slcan should directly select CAN_DEV_SW.

I'm a bit lost - what would CAN_DEV_SW do?

If it enables can_dropped_invalid_skb(), then the HW drivers would also
need to depend on CAN_DEV_SW directly or indirectly, or am I missing
something?



Max

2022-05-18 03:15:35

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 14:21:53 +0200
Marc Kleine-Budde <[email protected]> wrote:

> On 17.05.2022 14:14:04, Max Staudt wrote:
> > > After looking through drivers/net/can/Kconfig I would probably
> > > phrase it like this:
> > >
> > > Select CAN devices (hw/sw) -> we compile a can_dev module. E.g.
> > > to handle the skb stuff for vcan's.
> > >
> > > Select hardware CAN devices -> we compile the netlink stuff into
> > > can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled
> > > into can_dev too.
> > >
> > > In the latter case: The selection of flexcan, ti_hecc and
> > > mcp251xfd automatically selects CAN_RX_OFFLOAD which is then also
> > > compiled into can_dev.
> > >
> > > Would that fit in terms of complexity?
> >
> > IMHO these should always be compiled into can-dev. Out of tree
> > drivers are fairly common here, and having to determine which kind
> > of can-dev (stripped or not) the user has on their system is a
> > nightmare waiting to happen.
>
> I personally don't care about out-of-tree drivers.

I know that this is the official stance in the kernel.

But out-of-tree drivers do happen on a regular basis, even when
developing with the aim of upstreaming. And if a developer builds a
minimal kernel to host a CAN driver, without building in-tree hardware
CAN drivers, then can-dev will be there but behave differently from
can-dev in a full distro. Leading to heisenbugs and wasting time. The
source of heisenbugs really are the suggested *hidden* Kconfigs.


On another note, is the module accounting overhead in the kernel for
two new modules with relatively little code in each, code that almost
always is loaded when CAN is used, really worth it?


Okay, I think I'm out of 2 cent pieces now :)


Max

2022-05-18 03:37:27

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 13:51:57 +0200
Oliver Hartkopp <[email protected]> wrote:


> After looking through drivers/net/can/Kconfig I would probably phrase
> it like this:
>
> Select CAN devices (hw/sw) -> we compile a can_dev module. E.g. to
> handle the skb stuff for vcan's.
>
> Select hardware CAN devices -> we compile the netlink stuff into
> can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled into
> can_dev too.
>
> In the latter case: The selection of flexcan, ti_hecc and mcp251xfd
> automatically selects CAN_RX_OFFLOAD which is then also compiled into
> can_dev.
>
> Would that fit in terms of complexity?

IMHO these should always be compiled into can-dev. Out of tree drivers
are fairly common here, and having to determine which kind of can-dev
(stripped or not) the user has on their system is a nightmare waiting to
happen.


Max

2022-05-18 03:37:33

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 17.05.22 15:43, Max Staudt wrote:
> On Tue, 17 May 2022 15:35:03 +0200
> Oliver Hartkopp <[email protected]> wrote:
>
>> Oh, I didn't want to introduce two new kernel modules but to have
>> can_dev in different 'feature levels'.
>
> Which I agree is a nice idea, as long as heisenbugs can be avoided :)
>
> (as for the separate modules vs. feature levels of can-dev - sorry, my
> two paragraphs were each referring to a different idea. I mixed them
> into one single email...)
>
>
> Maybe the can-skb and rx-offload parts could be a *visible* sub-option
> of can-dev in Kconfig, which is normally optional, but immediately
> force-selected once a CAN HW driver is selected?

I think it should be even more simple.

When you enter the current Kconfig page of "CAN Device Drivers" every
selection of vcan/vxcan/slcan should directly select CAN_DEV_SW.

The rest could stay the same, e.g. selecting CAN_DEV "Platform CAN
drivers with Netlink support" which then enables CAN_CALC_BITTIMING and
CAN_LEDS to be selectable. Which also makes sure the old .config files
still apply.

And finally the selection of flexcan, ti_hecc and
mcp251xfd automatically selects CAN_DEV_RX_OFFLOAD.

Then only some more Makefile magic has to be done to build can-dev.ko
accordingly.

Best regards,
Oliver



>
>
>> But e.g. the people that are running Linux instances in a cloud only
>> using vcan and vxcan would not need to carry the entire
>> infrastructure of CAN hardware support and rx-offload.
>
> Out of curiosity, do you have an example use case for this vcan cloud
> setup? I can't dream one up...
>
>
>
> Max

2022-05-18 04:04:26

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 17.05.2022 14:14:04, Max Staudt wrote:
> > After looking through drivers/net/can/Kconfig I would probably phrase
> > it like this:
> >
> > Select CAN devices (hw/sw) -> we compile a can_dev module. E.g. to
> > handle the skb stuff for vcan's.
> >
> > Select hardware CAN devices -> we compile the netlink stuff into
> > can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled into
> > can_dev too.
> >
> > In the latter case: The selection of flexcan, ti_hecc and mcp251xfd
> > automatically selects CAN_RX_OFFLOAD which is then also compiled into
> > can_dev.
> >
> > Would that fit in terms of complexity?
>
> IMHO these should always be compiled into can-dev. Out of tree drivers
> are fairly common here, and having to determine which kind of can-dev
> (stripped or not) the user has on their system is a nightmare waiting to
> happen.

I personally don't care about out-of-tree drivers.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.19 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-18 04:34:55

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Tue, 17 May 2022 15:35:03 +0200
Oliver Hartkopp <[email protected]> wrote:

> Oh, I didn't want to introduce two new kernel modules but to have
> can_dev in different 'feature levels'.

Which I agree is a nice idea, as long as heisenbugs can be avoided :)

(as for the separate modules vs. feature levels of can-dev - sorry, my
two paragraphs were each referring to a different idea. I mixed them
into one single email...)


Maybe the can-skb and rx-offload parts could be a *visible* sub-option
of can-dev in Kconfig, which is normally optional, but immediately
force-selected once a CAN HW driver is selected?


> But e.g. the people that are running Linux instances in a cloud only
> using vcan and vxcan would not need to carry the entire
> infrastructure of CAN hardware support and rx-offload.

Out of curiosity, do you have an example use case for this vcan cloud
setup? I can't dream one up...



Max

2022-05-18 04:44:59

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 17.05.22 17:38, Max Staudt wrote:
> On Tue, 17 May 2022 16:35:05 +0200
> Oliver Hartkopp <[email protected]> wrote:
>
>> I think it should be even more simple.
>>
>> When you enter the current Kconfig page of "CAN Device Drivers" every
>> selection of vcan/vxcan/slcan should directly select CAN_DEV_SW.
>
> I'm a bit lost - what would CAN_DEV_SW do?

It should be just *one* enabler of building can-dev-ko

> If it enables can_dropped_invalid_skb(), then the HW drivers would also
> need to depend on CAN_DEV_SW directly or indirectly, or am I missing
> something?

And CAN_DEV is another enabler that would build the skb stuff from
CAN_DEV_SW too (but also the netlink stuff).

That's what I meant with "some Makefile magic" which is then building
the can-dev.ko with the required features depending on CAN_DEV_SW,
CAN_DEV, CAN_DEV_RX_OFFLOAD, CAN_CALC_BITTIMING, etc

Best,
Oliver


2022-05-18 04:45:48

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 17.05.2022 15:43:01, Max Staudt wrote:
> > Oh, I didn't want to introduce two new kernel modules but to have
> > can_dev in different 'feature levels'.
>
> Which I agree is a nice idea, as long as heisenbugs can be avoided :)
>
> (as for the separate modules vs. feature levels of can-dev - sorry, my
> two paragraphs were each referring to a different idea. I mixed them
> into one single email...)
>
> Maybe the can-skb and rx-offload parts could be a *visible* sub-option
> of can-dev in Kconfig, which is normally optional, but immediately
> force-selected once a CAN HW driver is selected?

In the ctucanfd driver we made the base driver "invisible" if
COMPILE_TEST is not selected:

| config CAN_CTUCANFD
| tristate "CTU CAN-FD IP core" if COMPILE_TEST
|
| config CAN_CTUCANFD_PCI
| tristate "CTU CAN-FD IP core PCI/PCIe driver"
| depends on PCI
| select CAN_CTUCANFD

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.20 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-18 04:56:30

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 5/17/22 14:39, Max Staudt wrote:
> On Tue, 17 May 2022 14:21:53 +0200
> Marc Kleine-Budde <[email protected]> wrote:
>
>> On 17.05.2022 14:14:04, Max Staudt wrote:
>>>> After looking through drivers/net/can/Kconfig I would probably
>>>> phrase it like this:
>>>>
>>>> Select CAN devices (hw/sw) -> we compile a can_dev module. E.g.
>>>> to handle the skb stuff for vcan's.
>>>>
>>>> Select hardware CAN devices -> we compile the netlink stuff into
>>>> can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled
>>>> into can_dev too.
>>>>
>>>> In the latter case: The selection of flexcan, ti_hecc and
>>>> mcp251xfd automatically selects CAN_RX_OFFLOAD which is then also
>>>> compiled into can_dev.
>>>>
>>>> Would that fit in terms of complexity?
>>>
>>> IMHO these should always be compiled into can-dev. Out of tree
>>> drivers are fairly common here, and having to determine which kind
>>> of can-dev (stripped or not) the user has on their system is a
>>> nightmare waiting to happen.
>>
>> I personally don't care about out-of-tree drivers.
>
> I know that this is the official stance in the kernel.
>
> But out-of-tree drivers do happen on a regular basis, even when
> developing with the aim of upstreaming. And if a developer builds a
> minimal kernel to host a CAN driver, without building in-tree hardware
> CAN drivers, then can-dev will be there but behave differently from
> can-dev in a full distro. Leading to heisenbugs and wasting time. The
> source of heisenbugs really are the suggested *hidden* Kconfigs.
>
>
> On another note, is the module accounting overhead in the kernel for
> two new modules with relatively little code in each, code that almost
> always is loaded when CAN is used, really worth it?

Oh, I didn't want to introduce two new kernel modules but to have
can_dev in different 'feature levels'.

I would assume a distro kernel to have everything enabled with a full
featured can_dev - which is likely the base for out-of-tree drivers too.

But e.g. the people that are running Linux instances in a cloud only
using vcan and vxcan would not need to carry the entire infrastructure
of CAN hardware support and rx-offload.

Best regards,
Oliver

2022-05-18 12:09:54

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

I didn't think this would trigger such a passionate discussion!

On Tue. 17 mai 2022 at 22:35, Oliver Hartkopp <[email protected]> wrote:
> On 5/17/22 14:39, Max Staudt wrote:
> > On Tue, 17 May 2022 14:21:53 +0200
> > Marc Kleine-Budde <[email protected]> wrote:
> >
> >> On 17.05.2022 14:14:04, Max Staudt wrote:
> >>>> After looking through drivers/net/can/Kconfig I would probably
> >>>> phrase it like this:
> >>>>
> >>>> Select CAN devices (hw/sw) -> we compile a can_dev module. E.g.
> >>>> to handle the skb stuff for vcan's.
> >>>>
> >>>> Select hardware CAN devices -> we compile the netlink stuff into
> >>>> can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled
> >>>> into can_dev too.
> >>>>
> >>>> In the latter case: The selection of flexcan, ti_hecc and
> >>>> mcp251xfd automatically selects CAN_RX_OFFLOAD which is then also
> >>>> compiled into can_dev.
> >>>>
> >>>> Would that fit in terms of complexity?
> >>>
> >>> IMHO these should always be compiled into can-dev. Out of tree
> >>> drivers are fairly common here, and having to determine which kind
> >>> of can-dev (stripped or not) the user has on their system is a
> >>> nightmare waiting to happen.
> >>
> >> I personally don't care about out-of-tree drivers.
> >
> > I know that this is the official stance in the kernel.
> >
> > But out-of-tree drivers do happen on a regular basis, even when
> > developing with the aim of upstreaming. And if a developer builds a
> > minimal kernel to host a CAN driver, without building in-tree hardware
> > CAN drivers, then can-dev will be there but behave differently from
> > can-dev in a full distro. Leading to heisenbugs and wasting time. The
> > source of heisenbugs really are the suggested *hidden* Kconfigs.
> >
> >
> > On another note, is the module accounting overhead in the kernel for
> > two new modules with relatively little code in each, code that almost
> > always is loaded when CAN is used, really worth it?
>
> Oh, I didn't want to introduce two new kernel modules but to have
> can_dev in different 'feature levels'.
>
> I would assume a distro kernel to have everything enabled with a full
> featured can_dev - which is likely the base for out-of-tree drivers too.
>
> But e.g. the people that are running Linux instances in a cloud only
> using vcan and vxcan would not need to carry the entire infrastructure
> of CAN hardware support and rx-offload.

Are there really some people running custom builds of the Linux kernel
in a cloud environment? The benefit of saving a few kilobytes by not
having to carry the entire CAN hardware infrastructure is blown away
by the cost of having to maintain a custom build.

I perfectly follow the idea to split rx-offload. Integrators building
some custom firmware for an embedded device might want to strip out
any unneeded piece. But I am not convinced by this same argument when
applied to v(x)can.
A two level split (with or without rx-offload) is what makes the most
sense to me.

Regardless, having the three level split is not harmful. And because
there seems to be a consensus on that, I am fine to continue in this
direction.

On a different topic, why are all the CAN devices
under "Networking support" and not "Device Drivers" in menuconfig
like everything else? Would it make sense to move our devices
under the "Device Drivers" section?

2022-05-18 12:17:51

by Marc Kleine-Budde

[permalink] [raw]
Subject: Device Drivers: (was: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c)

On 18.05.2022 21:03:37, Vincent MAILHOL wrote:
> On a different topic, why are all the CAN devices
> under "Networking support" and not "Device Drivers" in menuconfig
> like everything else? Would it make sense to move our devices
> under the "Device Drivers" section?

ACK

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (560.00 B)
signature.asc (499.00 B)
Download all attachments

2022-05-18 12:53:02

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: Device Drivers: (was: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c)



On 18.05.22 14:12, Marc Kleine-Budde wrote:
> On 18.05.2022 21:03:37, Vincent MAILHOL wrote:
>> On a different topic, why are all the CAN devices
>> under "Networking support" and not "Device Drivers" in menuconfig
>> like everything else? Would it make sense to move our devices
>> under the "Device Drivers" section?
>
> ACK
>

Bluetooth did it that way too. But I feel the same.
When we clean up the CAN drivers moving the CAN driver selection to
drivers/net/Kconfig would make sense.

ACK

Best regards,
Oliver


2022-05-18 13:15:00

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 18.05.22 14:03, Vincent MAILHOL wrote:
> I didn't think this would trigger such a passionate discussion!

:-D

Maybe your change was the drop that let the bucket run over ;-)

>> But e.g. the people that are running Linux instances in a cloud only
>> using vcan and vxcan would not need to carry the entire infrastructure
>> of CAN hardware support and rx-offload.
>
> Are there really some people running custom builds of the Linux kernel
> in a cloud environment? The benefit of saving a few kilobytes by not
> having to carry the entire CAN hardware infrastructure is blown away
> by the cost of having to maintain a custom build.

When looking to the current Kconfig and Makefile content in
drivers/net/can(/dev) there is also some CONFIG_CAN_LEDS which "depends
on BROKEN" and builds a leds.o from a non existing leds.c ?!?

Oh leds.c is in drivers/net/can/leds.c but not in
drivers/net/can/dev/leds.c where it could build ... ?

So what I would suggest is that we always build a can-dev.ko when a CAN
driver is needed.

Then we have different options that may be built-in:

1. netlink hw config interface
2. bitrate calculation
3. rx-offload
4. leds

E.g. having the netlink interface without bitrate calculation does not
make sense to me too.

> I perfectly follow the idea to split rx-offload. Integrators building
> some custom firmware for an embedded device might want to strip out
> any unneeded piece. But I am not convinced by this same argument when
> applied to v(x)can.

It does. I've seen CAN setups (really more than one or two!) in VMs and
container environments that are fed by Ethernet tunnels - sometimes also
in embedded devices.

> A two level split (with or without rx-offload) is what makes the most
> sense to me.
>
> Regardless, having the three level split is not harmful. And because
> there seems to be a consensus on that, I am fine to continue in this
> direction.

Thanks!

Should we remove the extra option for the bitrate calculation then?

And what about the LEDS support depending on BROKEN?
That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
broken") from Uwe as it seems there were some changes in 2018.

Best regards,
Oliver

2022-05-18 13:32:48

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 18.05.2022 15:10:44, Oliver Hartkopp wrote:
> On 18.05.22 14:03, Vincent MAILHOL wrote:
> > I didn't think this would trigger such a passionate discussion!
>
> :-D
>
> Maybe your change was the drop that let the bucket run over ;-)

It's so trivial that everybody feels the urge to say something. :D

> > > But e.g. the people that are running Linux instances in a cloud only
> > > using vcan and vxcan would not need to carry the entire infrastructure
> > > of CAN hardware support and rx-offload.
> >
> > Are there really some people running custom builds of the Linux kernel
> > in a cloud environment? The benefit of saving a few kilobytes by not
> > having to carry the entire CAN hardware infrastructure is blown away
> > by the cost of having to maintain a custom build.
>
> When looking to the current Kconfig and Makefile content in
> drivers/net/can(/dev) there is also some CONFIG_CAN_LEDS which "depends on
> BROKEN" and builds a leds.o from a non existing leds.c ?!?
>
> Oh leds.c is in drivers/net/can/leds.c but not in drivers/net/can/dev/leds.c
> where it could build ... ?
>
> So what I would suggest is that we always build a can-dev.ko when a CAN
> driver is needed.
>
> Then we have different options that may be built-in:
>
> 1. netlink hw config interface
> 2. bitrate calculation
> 3. rx-offload
> 4. leds
>
> E.g. having the netlink interface without bitrate calculation does not make
> sense to me too.

ACK

> > I perfectly follow the idea to split rx-offload. Integrators building
> > some custom firmware for an embedded device might want to strip out
> > any unneeded piece. But I am not convinced by this same argument when
> > applied to v(x)can.
>
> It does. I've seen CAN setups (really more than one or two!) in VMs and
> container environments that are fed by Ethernet tunnels - sometimes also in
> embedded devices.
>
> > A two level split (with or without rx-offload) is what makes the most
> > sense to me.
> >
> > Regardless, having the three level split is not harmful. And because
> > there seems to be a consensus on that, I am fine to continue in this
> > direction.
>
> Thanks!
>
> Should we remove the extra option for the bitrate calculation then?

+1

> And what about the LEDS support depending on BROKEN?
> That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
> broken") from Uwe as it seems there were some changes in 2018.

There's a proper generic LED trigger now for network devices. So remove
LED triggers, too.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (2.78 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-18 14:14:02

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Wed. 18 May 2022 à 22:32, Marc Kleine-Budde <[email protected]> wrote:
> On 18.05.2022 15:10:44, Oliver Hartkopp wrote:
> > On 18.05.22 14:03, Vincent MAILHOL wrote:
> > > I didn't think this would trigger such a passionate discussion!
> >
> > :-D
> >
> > Maybe your change was the drop that let the bucket run over ;-)
>
> It's so trivial that everybody feels the urge to say something. :D
>
> > > > But e.g. the people that are running Linux instances in a cloud only
> > > > using vcan and vxcan would not need to carry the entire infrastructure
> > > > of CAN hardware support and rx-offload.
> > >
> > > Are there really some people running custom builds of the Linux kernel
> > > in a cloud environment? The benefit of saving a few kilobytes by not
> > > having to carry the entire CAN hardware infrastructure is blown away
> > > by the cost of having to maintain a custom build.
> >
> > When looking to the current Kconfig and Makefile content in
> > drivers/net/can(/dev) there is also some CONFIG_CAN_LEDS which "depends on
> > BROKEN" and builds a leds.o from a non existing leds.c ?!?
> >
> > Oh leds.c is in drivers/net/can/leds.c but not in drivers/net/can/dev/leds.c
> > where it could build ... ?
> >
> > So what I would suggest is that we always build a can-dev.ko when a CAN
> > driver is needed.
> >
> > Then we have different options that may be built-in:
> >
> > 1. netlink hw config interface
> > 2. bitrate calculation
> > 3. rx-offload
> > 4. leds
> >
> > E.g. having the netlink interface without bitrate calculation does not make
> > sense to me too.
>
> ACK
>
> > > I perfectly follow the idea to split rx-offload. Integrators building
> > > some custom firmware for an embedded device might want to strip out
> > > any unneeded piece. But I am not convinced by this same argument when
> > > applied to v(x)can.
> >
> > It does. I've seen CAN setups (really more than one or two!) in VMs and
> > container environments that are fed by Ethernet tunnels - sometimes also in
> > embedded devices.

Are those VM running custom builds of the kernel in which all the CAN
hardware devices have been removed? Also, isn't it hard to keep those
up to date with all the kernel security patches?

> > > A two level split (with or without rx-offload) is what makes the most
> > > sense to me.
> > >
> > > Regardless, having the three level split is not harmful. And because
> > > there seems to be a consensus on that, I am fine to continue in this
> > > direction.
> >
> > Thanks!
> >
> > Should we remove the extra option for the bitrate calculation then?
>
> +1

I can imagine people wanting to ship a product with the bitrate
calculation removed. For example, an infotainment unit designed for
one specific vehicle platform (i.e. baudrate is fixed). In that case,
the integrator might decide to remove bittiming calculation and
hardcode all hand calculated bittiming parameters instead.

So that one, I prefer to keep it. I just didn't mention it in my
previous message because it was already splitted out.

> > And what about the LEDS support depending on BROKEN?
> > That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
> > broken") from Uwe as it seems there were some changes in 2018.
>
> There's a proper generic LED trigger now for network devices. So remove
> LED triggers, too.

Yes, the broken LED topic also popped-up last year:

https://lore.kernel.org/linux-can/[email protected]/

I am OK to add one patch to the series for LED removal. Just some
heads-up: I will take my time, it will definitely not be ready for the
v5.19 merge window. And I also expect that there will be at least one
more round of discussion.

While I am at it, anything else to add to the wish list before I start
to working it?

2022-05-18 14:34:36

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 18.05.22 16:07, Vincent MAILHOL wrote:

>>> It does. I've seen CAN setups (really more than one or two!) in VMs and
>>> container environments that are fed by Ethernet tunnels - sometimes also in
>>> embedded devices.
>
> Are those VM running custom builds of the kernel in which all the CAN
> hardware devices have been removed? Also, isn't it hard to keep those
> up to date with all the kernel security patches?

AFAIK all kind of BSPs create custom configured kernels. And to remove
attack surfaces the idea is to minimize the code size. That's not only
to save some flash space.

>>>> A two level split (with or without rx-offload) is what makes the most
>>>> sense to me.
>>>>
>>>> Regardless, having the three level split is not harmful. And because
>>>> there seems to be a consensus on that, I am fine to continue in this
>>>> direction.
>>>
>>> Thanks!
>>>
>>> Should we remove the extra option for the bitrate calculation then?
>>
>> +1
>
> I can imagine people wanting to ship a product with the bitrate
> calculation removed. For example, an infotainment unit designed for
> one specific vehicle platform (i.e. baudrate is fixed). In that case,
> the integrator might decide to remove bittiming calculation and
> hardcode all hand calculated bittiming parameters instead.
>
> So that one, I prefer to keep it. I just didn't mention it in my
> previous message because it was already splitted out.

Ok. Interesting that we have such different expectations.

>>> And what about the LEDS support depending on BROKEN?
>>> That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
>>> broken") from Uwe as it seems there were some changes in 2018.
>>
>> There's a proper generic LED trigger now for network devices. So remove
>> LED triggers, too.
>
> Yes, the broken LED topic also popped-up last year:
>
> https://lore.kernel.org/linux-can/[email protected]/ > I am OK to add one patch to the series for LED removal.

Hm. We have several drivers that support LED triggers:

$ git grep led.h
at91_can.c:#include <linux/can/led.h>
c_can/c_can_main.c:#include <linux/can/led.h>
ctucanfd/ctucanfd_base.c:#include <linux/can/led.h>
dev/dev.c:#include <linux/can/led.h>
flexcan/flexcan-core.c:#include <linux/can/led.h>
led.c:#include <linux/can/led.h>
m_can/m_can.h:#include <linux/can/led.h>
rcar/rcar_can.c:#include <linux/can/led.h>
rcar/rcar_canfd.c:#include <linux/can/led.h>
sja1000/sja1000.c:#include <linux/can/led.h>
spi/hi311x.c:#include <linux/can/led.h>
spi/mcp251x.c:#include <linux/can/led.h>
sun4i_can.c:#include <linux/can/led.h>
ti_hecc.c:#include <linux/can/led.h>
usb/mcba_usb.c:#include <linux/can/led.h>
usb/usb_8dev.c:#include <linux/can/led.h>
xilinx_can.c:#include <linux/can/led.h>

And I personally like the ability to be able to fire some LEDS (either
as GPIO or probably in a window manager).

I would suggest to remove the Kconfig entry but not all the code inside
the drivers, so that a volunteer can convert the LED support based on
the existing trigger points in the drivers code later.

Our would it make sense to just leave some comment at those places like:

/* LED TX trigger here */

??

> Just some
> heads-up: I will take my time, it will definitely not be ready for the
> v5.19 merge window. And I also expect that there will be at least one
> more round of discussion.
>
> While I am at it, anything else to add to the wish list before I start
> to working it?

Not really. IMO we already share a common picture now. Thanks for
picking this up!

Best regards,
Oliver

2022-05-18 14:39:02

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On 18.05.2022 16:33:58, Oliver Hartkopp wrote:
> > > > And what about the LEDS support depending on BROKEN?
> > > > That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
> > > > broken") from Uwe as it seems there were some changes in 2018.
> > >
> > > There's a proper generic LED trigger now for network devices. So remove
> > > LED triggers, too.
> >
> > Yes, the broken LED topic also popped-up last year:
> >
> > https://lore.kernel.org/linux-can/[email protected]/ > I am OK to add one patch to the series for LED removal.
>
> Hm. We have several drivers that support LED triggers:
>
> $ git grep led.h
> at91_can.c:#include <linux/can/led.h>
> c_can/c_can_main.c:#include <linux/can/led.h>
> ctucanfd/ctucanfd_base.c:#include <linux/can/led.h>
> dev/dev.c:#include <linux/can/led.h>
> flexcan/flexcan-core.c:#include <linux/can/led.h>
> led.c:#include <linux/can/led.h>
> m_can/m_can.h:#include <linux/can/led.h>
> rcar/rcar_can.c:#include <linux/can/led.h>
> rcar/rcar_canfd.c:#include <linux/can/led.h>
> sja1000/sja1000.c:#include <linux/can/led.h>
> spi/hi311x.c:#include <linux/can/led.h>
> spi/mcp251x.c:#include <linux/can/led.h>
> sun4i_can.c:#include <linux/can/led.h>
> ti_hecc.c:#include <linux/can/led.h>
> usb/mcba_usb.c:#include <linux/can/led.h>
> usb/usb_8dev.c:#include <linux/can/led.h>
> xilinx_can.c:#include <linux/can/led.h>
>
> And I personally like the ability to be able to fire some LEDS (either as
> GPIO or probably in a window manager).
>
> I would suggest to remove the Kconfig entry but not all the code inside the
> drivers, so that a volunteer can convert the LED support based on the
> existing trigger points in the drivers code later.

The generic netdev LED trigger code doesn't need any support in the
netdev driver.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (2.09 kB)
signature.asc (499.00 B)
Download all attachments

2022-05-18 14:39:08

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 18.05.22 16:36, Marc Kleine-Budde wrote:
> On 18.05.2022 16:33:58, Oliver Hartkopp wrote:

>> I would suggest to remove the Kconfig entry but not all the code inside the
>> drivers, so that a volunteer can convert the LED support based on the
>> existing trigger points in the drivers code later.
>
> The generic netdev LED trigger code doesn't need any support in the
> netdev driver.

Oh! Yes, then it could be removed. Sorry for not looking that deep into it.

Best,
Oliver

2022-05-18 14:59:19

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c



On 18.05.22 16:38, Oliver Hartkopp wrote:
>
>
> On 18.05.22 16:36, Marc Kleine-Budde wrote:
>> On 18.05.2022 16:33:58, Oliver Hartkopp wrote:
>
>>> I would suggest to remove the Kconfig entry but not all the code
>>> inside the
>>> drivers, so that a volunteer can convert the LED support based on the
>>> existing trigger points in the drivers code later.
>>
>> The generic netdev LED trigger code doesn't need any support in the
>> netdev driver.
>
> Oh! Yes, then it could be removed. Sorry for not looking that deep into it.

I can send a patch for this removal too. That's an easy step which might
get into 5.19 then.

Best regards,
Oliver


2022-05-18 15:42:33

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Wed. 18 May 2022 at 23:59, Oliver Hartkopp <[email protected]> wrote:
> On 18.05.22 16:38, Oliver Hartkopp wrote:
> > On 18.05.22 16:36, Marc Kleine-Budde wrote:
> >> On 18.05.2022 16:33:58, Oliver Hartkopp wrote:
> >
> >>> I would suggest to remove the Kconfig entry but not all the code
> >>> inside the
> >>> drivers, so that a volunteer can convert the LED support based on the
> >>> existing trigger points in the drivers code later.
> >>
> >> The generic netdev LED trigger code doesn't need any support in the
> >> netdev driver.
> >
> > Oh! Yes, then it could be removed. Sorry for not looking that deep into it.
>
> I can send a patch for this removal too. That's an easy step which might
> get into 5.19 then.

OK, go ahead. On my side, I will start to work on the other changes
either next week or next next week, depending on my mood.

2022-05-18 15:52:25

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Thu, 19 May 2022 00:38:51 +0900
Vincent MAILHOL <[email protected]> wrote:

> On Wed. 18 May 2022 at 23:59, Oliver Hartkopp
> <[email protected]> wrote:
> > I can send a patch for this removal too. That's an easy step which
> > might get into 5.19 then.
>
> OK, go ahead. On my side, I will start to work on the other changes
> either next week or next next week, depending on my mood.


Any wishes for the next version of can327/elmcan?

Should I wait until your changes are in?


Max

2022-05-18 16:07:53

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

On Thu. 19 May 2022 at 00:52, Max Staudt <[email protected]> wrote:
> On Thu, 19 May 2022 00:38:51 +0900
> Vincent MAILHOL <[email protected]> wrote:
>
> > On Wed. 18 May 2022 at 23:59, Oliver Hartkopp
> > <[email protected]> wrote:
> > > I can send a patch for this removal too. That's an easy step which
> > > might get into 5.19 then.
> >
> > OK, go ahead. On my side, I will start to work on the other changes
> > either next week or next next week, depending on my mood.
>
> Any wishes for the next version of can327/elmcan?

The only thing I guess would be to remove the check against
CAN_CTRLMODE_LISTENONLY in your xmit() function. The other things, I
already commented :)

> Should I wait until your changes are in?

I do not think you have to wait. There are no real dependencies. You
might just want to add a note after the --- scissors in the patch that
there is a weak dependencies on
https://lore.kernel.org/linux-can/[email protected]/


Yours sincerely,
Vincent Mailhol

2022-06-03 16:27:54

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

Aside of calc_bittiming.o which can be configured with
CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
unconditionally to can-dev.o even if not needed by the user.

This series first goal it to split the can-dev modules so that the
user can decide which features get built in during
compilation. Additionally, the CAN Device Drivers menu is moved from
the "Networking support" category to the "Device Drivers" category
(where all drivers are supposed to be).

Below diagrams illustrate the changes made.
The arrow symbol "x --> y" denotes that "y depends on x".

* menu before this series *

CAN bus subsystem support
symbol: CONFIG_CAN
|
+-> CAN Device Drivers
(no symbol)
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> Platform CAN drivers with Netlink support
symbol: CONFIG_CAN_DEV
|
+-> CAN bit-timing calculation (optional for hardware drivers)
| symbol: CONFIG_CAN_BITTIMING
|
+-> All other CAN devices

* menu after this series *

Network device support
symbol: CONFIG_NETDEVICES
|
+-> CAN Device Drivers
symbol: CONFIG_CAN_DEV
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> CAN device drivers with Netlink support
symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
|
+-> CAN bit-timing calculation (optional for all drivers)
| symbol: CONFIG_CAN_BITTIMING
|
+-> All other CAN devices not relying on RX offload
|
+-> CAN rx offload
symbol: CONFIG_CAN_RX_OFFLOAD
|
+-> CAN devices relying on rx offload
(at time of writing: flexcan, ti_hecc and mcp251xfd)

Patches 1 to 5 of this series do above modification.

The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
can_dropped_invalid_skb() to discard tx skb (such skb can potentially
reach the driver if injected via the packet socket). In more details,
patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
7 is the actual change.

Those last two patches are actually connected to the first five ones:
because slcan and v(x)can requires can_dropped_invalid_skb(), it was
necessary to add those three devices to the scope of can-dev before
moving the function to skb.o.


** N.B. **

This design results from the lengthy discussion in [1].

I did one change from Oliver's suggestions in [2]. The initial idea
was that the first two config symbols should be respectively
CAN_DEV_SW and CAN_DEV instead of CAN_DEV and CAN_NETLINK as proposed
in this series.

* First symbol is changed from CAN_DEV_SW to CAN_DEV. The rationale
is that it is this entry that will trigger the build of can-dev.o
and it makes more sense for me to name the symbol share the same
name as the module. Furthermore, this allows to create a menuentry
with an explicit name that will cover both the virtual and
physical devices (naming the menuentry "CAN Device Software" would
be inconsistent with the fact that physical devices would also be
present in a sub menu). And not using menuentry complexifies the
menu.

* Second symbol is renamed from CAN_DEV to CAN_NETLINK because
CAN_DEV is now taken by the previous menuconfig and netlink is the
predominant feature added at this level. I am opened to other
naming suggestion (CAN_DEV_NETLINK, CAN_DEV_HW...?).

[1] https://lore.kernel.org/linux-can/[email protected]/
[2] https://lore.kernel.org/linux-can/[email protected]/


** Changelog **

v3 -> v4:

* Five additional patches added to split can-dev module and refactor
Kbuild. c.f. below (lengthy) thread:
https://lore.kernel.org/linux-can/[email protected]/


v2 -> v3:

* Apply can_dropped_invalid_skb() to slcan.

* Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
modifying Kbuild.

* fix small typos.

v1 -> v2:

* move can_dropped_invalid_skb() to skb.c instead of dev.h

* also move can_skb_headroom_valid() to skb.c

Vincent Mailhol (7):
can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK
can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
CAN_DEV
can: bittiming: move bittiming calculation functions to
calc_bittiming.c
can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
net: Kconfig: move the CAN device menu to the "Device Drivers" section
can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
to skb.c
can: skb: drop tx skb if in listen only mode

drivers/net/Kconfig | 2 +
drivers/net/can/Kconfig | 66 +++++++--
drivers/net/can/dev/Makefile | 20 ++-
drivers/net/can/dev/bittiming.c | 197 -------------------------
drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
drivers/net/can/dev/dev.c | 9 +-
drivers/net/can/dev/skb.c | 72 +++++++++
drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
include/linux/can/skb.h | 59 +-------
net/can/Kconfig | 5 +-
10 files changed, 351 insertions(+), 282 deletions(-)
create mode 100644 drivers/net/can/dev/calc_bittiming.c

--
2.35.1

2022-06-04 19:32:37

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 1/7] can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK

In the next patches, the scope of the can_dev module will grow to
engloble the software/virtual drivers (slcan, v(x)can). To this
extent, release CAN_DEV by renaming it into CAN_NETLINK. The config
symbol CAN_DEV will be reused to cover this extended scope.

The rationale for the name CAN_NETLINK is that netlink is the
predominant feature added here.

The current description only mentions platform drivers despite the
fact that this symbol is also required by "normal" devices (e.g. USB
or PCI) which do not fall under the platform devices category.

The description is updated accordingly to fix this gap.

Signed-off-by: Vincent Mailhol <[email protected]>
---
Please share if you have any suggestion on the name. I hesitated a lot
between CAN_NETLINK or CAN_DEV_NETLINK (because netlink is the
predominant feature) and CAN_DEV_HW (because this targets the
non-software only drivers, i.e. the hardware ones).
---
drivers/net/can/Kconfig | 18 +++++++++++-------
drivers/net/can/dev/Makefile | 2 +-
2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index b2dcc1e5a388..99f189ad35ad 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -48,15 +48,19 @@ config CAN_SLCAN
can be changed by the 'maxdev=xx' module option. This driver can
also be built as a module. If so, the module will be called slcan.

-config CAN_DEV
- tristate "Platform CAN drivers with Netlink support"
+config CAN_NETLINK
+ tristate "CAN device drivers with Netlink support"
default y
help
- Enables the common framework for platform CAN drivers with Netlink
- support. This is the standard library for CAN drivers.
- If unsure, say Y.
+ Enables the common framework for CAN device drivers. This is the
+ standard library and provides features for the Netlink interface such
+ as bittiming validation, support of CAN error states, device restart
+ and others.
+
+ This is required by all platform and hardware CAN drivers. If you
+ plan to use such devices or if unsure, say Y.

-if CAN_DEV
+if CAN_NETLINK

config CAN_CALC_BITTIMING
bool "CAN bit-timing calculation"
@@ -164,7 +168,7 @@ source "drivers/net/can/softing/Kconfig"
source "drivers/net/can/spi/Kconfig"
source "drivers/net/can/usb/Kconfig"

-endif
+endif #CAN_NETLINK

config CAN_DEBUG_DEVICES
bool "CAN devices debugging messages"
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index af2901db473c..5b4c813c6222 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0

-obj-$(CONFIG_CAN_DEV) += can-dev.o
+obj-$(CONFIG_CAN_NETLINK) += can-dev.o
can-dev-y += bittiming.o
can-dev-y += dev.o
can-dev-y += length.o
--
2.35.1

2022-06-04 19:32:50

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 7/7] can: skb: drop tx skb if in listen only mode

Frames can be directly injected to a can driver via the packet
socket. By doing so, it is possible to reach the
net_device_ops::ndo_start_xmit function even if the driver is
configured in listen only mode.

Add a check in can_dropped_invalid_skb() to discard the skb if
CAN_CTRLMODE_LISTENONLY is set.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/skb.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index dc9da76c0470..8bb62dd864c8 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -5,6 +5,7 @@
*/

#include <linux/can/dev.h>
+#include <linux/can/netlink.h>
#include <linux/module.h>

#define MOD_DESC "CAN device driver interface"
@@ -293,6 +294,7 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
{
const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+ struct can_priv *priv = netdev_priv(dev);

if (skb->protocol == htons(ETH_P_CAN)) {
if (unlikely(skb->len != CAN_MTU ||
@@ -306,8 +308,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
goto inval_skb;
}

- if (!can_skb_headroom_valid(dev, skb))
+ if (!can_skb_headroom_valid(dev, skb)) {
+ goto inval_skb;
+ } else if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+ netdev_info_once(dev,
+ "interface in listen only mode, dropping skb\n");
goto inval_skb;
+ }

return false;

--
2.35.1

2022-06-05 00:52:32

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 5/7] net: Kconfig: move the CAN device menu to the "Device Drivers" section

The devices are meant to be under the "Device Drivers" category of the
menuconfig. The CAN subsystem is currently one of the rare exception
with all of its devices under the "Networking support" category.

The CAN_DEV menuentry gets moved to fix this discrepancy. The CAN menu
is added just before MCTP in order to be consistent with the current
order under the "Networking support" menu.

A dependency on CAN is added to CAN_DEV so that the CAN devices only
show up if the CAN subsystem is enabled.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/Kconfig | 2 ++
drivers/net/can/Kconfig | 1 +
net/can/Kconfig | 5 ++---
3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index b2a4f998c180..5de243899de8 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -499,6 +499,8 @@ config NET_SB1000

source "drivers/net/phy/Kconfig"

+source "drivers/net/can/Kconfig"
+
source "drivers/net/mctp/Kconfig"

source "drivers/net/mdio/Kconfig"
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 1f1d81da1c8c..4ab80507c353 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -3,6 +3,7 @@
menuconfig CAN_DEV
tristate "CAN Device Drivers"
default y
+ depends on CAN
help
Controller Area Network (CAN) is serial communications protocol up to
1Mbit/s for its original release (now known as Classical CAN) and up
diff --git a/net/can/Kconfig b/net/can/Kconfig
index a9ac5ffab286..cb56be8e3862 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -15,7 +15,8 @@ menuconfig CAN
PF_CAN is contained in <Documentation/networking/can.rst>.

If you want CAN support you should say Y here and also to the
- specific driver for your controller(s) below.
+ specific driver for your controller(s) under the Network device
+ support section.

if CAN

@@ -69,6 +70,4 @@ config CAN_ISOTP
If you want to perform automotive vehicle diagnostic services (UDS),
say 'y'.

-source "drivers/net/can/Kconfig"
-
endif
--
2.35.1

2022-06-06 03:37:28

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On Sat. 4 juin 2022 at 20:46, Marc Kleine-Budde <[email protected]> wrote:
> Hello Vincent,
>
> wow! This is a great series which addresses a lot of long outstanding
> issues. Great work!

Thanks.

> As this cover letter brings so much additional information I'll ask
> Jakub and David if they take pull request from me, which itself have
> merges. This cover letter would be part of my merge. If I get the OK,
> can you provide this series as a tag (ideally GPG signed) that I can
> pull?

Fine, but I need a bit of guidance here. To provide a tag, I need to
have my own git repository hosted online, right? Is GitHub OK or
should I create one on https://git.kernel.org/?

> regards,
> Marc
>
> On 03.06.2022 19:28:41, Vincent Mailhol wrote:
> > Aside of calc_bittiming.o which can be configured with
> > CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
> > unconditionally to can-dev.o even if not needed by the user.
> >
> > This series first goal it to split the can-dev modules so that the
> > user can decide which features get built in during
> > compilation. Additionally, the CAN Device Drivers menu is moved from
> > the "Networking support" category to the "Device Drivers" category
> > (where all drivers are supposed to be).
> >
> > Below diagrams illustrate the changes made.
> > The arrow symbol "x --> y" denotes that "y depends on x".
> >
> > * menu before this series *
> >
> > CAN bus subsystem support
> > symbol: CONFIG_CAN
> > |
> > +-> CAN Device Drivers
> > (no symbol)
> > |
> > +-> software/virtual CAN device drivers
> > | (at time of writing: slcan, vcan, vxcan)
> > |
> > +-> Platform CAN drivers with Netlink support
> > symbol: CONFIG_CAN_DEV
> > |
> > +-> CAN bit-timing calculation (optional for hardware drivers)
> > | symbol: CONFIG_CAN_BITTIMING
> > |
> > +-> All other CAN devices
> >
> > * menu after this series *
> >
> > Network device support
> > symbol: CONFIG_NETDEVICES
> > |
> > +-> CAN Device Drivers
> > symbol: CONFIG_CAN_DEV
> > |
> > +-> software/virtual CAN device drivers
> > | (at time of writing: slcan, vcan, vxcan)
> > |
> > +-> CAN device drivers with Netlink support
> > symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> > |
> > +-> CAN bit-timing calculation (optional for all drivers)
> > | symbol: CONFIG_CAN_BITTIMING
> > |
> > +-> All other CAN devices not relying on RX offload
> > |
> > +-> CAN rx offload
> > symbol: CONFIG_CAN_RX_OFFLOAD
> > |
> > +-> CAN devices relying on rx offload
> > (at time of writing: flexcan, ti_hecc and mcp251xfd)
> >
> > Patches 1 to 5 of this series do above modification.
> >
> > The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
> > can_dropped_invalid_skb() to discard tx skb (such skb can potentially
> > reach the driver if injected via the packet socket). In more details,
> > patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
> > 7 is the actual change.
> >
> > Those last two patches are actually connected to the first five ones:
> > because slcan and v(x)can requires can_dropped_invalid_skb(), it was
> > necessary to add those three devices to the scope of can-dev before
> > moving the function to skb.o.
> >
> >
> > ** N.B. **
> >
> > This design results from the lengthy discussion in [1].
> >
> > I did one change from Oliver's suggestions in [2]. The initial idea
> > was that the first two config symbols should be respectively
> > CAN_DEV_SW and CAN_DEV instead of CAN_DEV and CAN_NETLINK as proposed
> > in this series.
> >
> > * First symbol is changed from CAN_DEV_SW to CAN_DEV. The rationale
> > is that it is this entry that will trigger the build of can-dev.o
> > and it makes more sense for me to name the symbol share the same
> > name as the module. Furthermore, this allows to create a menuentry
> > with an explicit name that will cover both the virtual and
> > physical devices (naming the menuentry "CAN Device Software" would
> > be inconsistent with the fact that physical devices would also be
> > present in a sub menu). And not using menuentry complexifies the
> > menu.
> >
> > * Second symbol is renamed from CAN_DEV to CAN_NETLINK because
> > CAN_DEV is now taken by the previous menuconfig and netlink is the
> > predominant feature added at this level. I am opened to other
> > naming suggestion (CAN_DEV_NETLINK, CAN_DEV_HW...?).
> >
> > [1] https://lore.kernel.org/linux-can/[email protected]/
> > [2] https://lore.kernel.org/linux-can/[email protected]/
> >
> >
> > ** Changelog **
> >
> > v3 -> v4:
> >
> > * Five additional patches added to split can-dev module and refactor
> > Kbuild. c.f. below (lengthy) thread:
> > https://lore.kernel.org/linux-can/[email protected]/
> >
> >
> > v2 -> v3:
> >
> > * Apply can_dropped_invalid_skb() to slcan.
> >
> > * Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
> > modifying Kbuild.
> >
> > * fix small typos.
> >
> > v1 -> v2:
> >
> > * move can_dropped_invalid_skb() to skb.c instead of dev.h
> >
> > * also move can_skb_headroom_valid() to skb.c
> >
> > Vincent Mailhol (7):
> > can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK
> > can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
> > CAN_DEV
> > can: bittiming: move bittiming calculation functions to
> > calc_bittiming.c
> > can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
> > net: Kconfig: move the CAN device menu to the "Device Drivers" section
> > can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
> > to skb.c
> > can: skb: drop tx skb if in listen only mode
> >
> > drivers/net/Kconfig | 2 +
> > drivers/net/can/Kconfig | 66 +++++++--
> > drivers/net/can/dev/Makefile | 20 ++-
> > drivers/net/can/dev/bittiming.c | 197 -------------------------
> > drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
> > drivers/net/can/dev/dev.c | 9 +-
> > drivers/net/can/dev/skb.c | 72 +++++++++
> > drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
> > include/linux/can/skb.h | 59 +-------
> > net/can/Kconfig | 5 +-
> > 10 files changed, 351 insertions(+), 282 deletions(-)
> > create mode 100644 drivers/net/can/dev/calc_bittiming.c
> >
> > --
> > 2.35.1
> >
> >
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-06-06 03:52:41

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On Sun. 5 juin 2022 at 19:39, Marc Kleine-Budde <[email protected]> wrote:
> On 05.06.2022 01:32:15, Vincent MAILHOL wrote:
> > On Sun. 5 juin 2022 at 00:18, Marc Kleine-Budde <[email protected]> wrote:
> > > On 04.06.2022 23:59:48, Vincent MAILHOL wrote:
> > > > > > Fine, but I need a bit of guidance here. To provide a tag, I need to
> > > > > > have my own git repository hosted online, right?
> > > > >
> > > > > That is one option.
> > > >
> > > > This suggests that there are other options? What would be those other
> > > > options?
> > >
> > > 2. git.kernel.org (most preferred)
> > > 3. github.com (have to ask Davem/Jakub)
> > >
> > > > > > Is GitHub OK or should I create one on https://git.kernel.org/?
> > > > >
> > > > > Some maintainers don't like github, let's wait what Davem and Jakub say.
> > > > > I think for git.kernel.org you need a GPG key with signatures of 3 users
> > > > > of git.kernel.org.
> > > >
> > > > Personally, I would also prefer getting my own git.kernel.org account.
> > >
> > > See https://korg.docs.kernel.org/accounts.html
> >
> > Thanks for the link. I will have a look at it tomorrow (or the day
> > after tomorrow in the worst case).
> >
> > Meanwhile, I will send the v5 which should address all your comments.
>
> /me just realized that merged are independent of pull requests. I can
> create a local branch and merge it, as Davem and Jakub do it. I've added
> your v5 to can-next/master as a merge and I'll include this in my next
> PR to net-next if Davem and Jakub are OK with merges in my branch.

So my dreams of getting my kernel.org account swag just evaporated
(just kidding :))
I think I will prepare a GPG key just to be ready in the opportunity
to get it signed pop-up one day.

Happy to see that this is reaching an end. Honestly speaking, the
menuconfig cleanup was not my most exciting contribution (euphemism)
but was still a necessity. Glad that this is nearly over after more
than 80 messages in the full thread (including all five versions). If
I recall correctly, this is the longest thread we had in the last two
years. And thanks again to Max, Oliver and you for animating the
debate!


Yours sincerely,
Vincent Mailhol

2022-06-06 03:56:06

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On Sat. 4 June 2022 at 22:55, Marc Kleine-Budde <[email protected]> wrote:
> On 04.06.2022 22:05:09, Vincent MAILHOL wrote:
> > On Sat. 4 juin 2022 at 20:46, Marc Kleine-Budde <[email protected]> wrote:
> > > Hello Vincent,
> > >
> > > wow! This is a great series which addresses a lot of long outstanding
> > > issues. Great work!
> >
> > Thanks.
> >
> > > As this cover letter brings so much additional information I'll ask
> > > Jakub and David if they take pull request from me, which itself have
> > > merges. This cover letter would be part of my merge. If I get the OK,
> > > can you provide this series as a tag (ideally GPG signed) that I can
> > > pull?
> >
> > Fine, but I need a bit of guidance here. To provide a tag, I need to
> > have my own git repository hosted online, right?
>
> That is one option.

This suggests that there are other options? What would be those other options?

> > Is GitHub OK or should I create one on https://git.kernel.org/?
>
> Some maintainers don't like github, let's wait what Davem and Jakub say.
> I think for git.kernel.org you need a GPG key with signatures of 3 users
> of git.kernel.org.

Personally, I would also prefer getting my own git.kernel.org account.
It has infinitely more swag than GitHub. But my religion does not
forbid me from using GitHub :)

Yours sincerely,
Vincent Mailhol

2022-06-06 04:16:32

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 4/7] can: Kconfig: add CONFIG_CAN_RX_OFFLOAD

Only a few drivers rely on the CAN rx offload framework (as of the
writing of this patch, only three: flexcan, ti_hecc and
mcp251xfd). Give the option to the user to deselect this features
during compilation.

The drivers relying on CAN rx offload are in different sub
folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD"
so that the option is automatically enabled whenever one of those
driver is chosen.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/Kconfig | 16 ++++++++++++++++
drivers/net/can/dev/Makefile | 2 ++
drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
3 files changed, 19 insertions(+)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 8f3b97aea638..1f1d81da1c8c 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING

If unsure, say Y.

+config CAN_RX_OFFLOAD
+ bool "CAN RX offload"
+ default y
+ help
+ Framework to offload the controller's RX FIFO during one
+ interrupt. The CAN frames of the FIFO are read and put into a skb
+ queue during that interrupt and transmitted afterwards in a NAPI
+ context.
+
+ The additional features selected by this option will be added to the
+ can-dev module.
+
+ If unsure, say Y.
+
config CAN_AT91
tristate "Atmel AT91 onchip CAN controller"
depends on (ARCH_AT91 || COMPILE_TEST) && HAS_IOMEM
@@ -113,6 +127,7 @@ config CAN_FLEXCAN
tristate "Support for Freescale FLEXCAN based chips"
depends on OF || COLDFIRE || COMPILE_TEST
depends on HAS_IOMEM
+ select CAN_RX_OFFLOAD
help
Say Y here if you want to support for Freescale FlexCAN.

@@ -162,6 +177,7 @@ config CAN_SUN4I
config CAN_TI_HECC
depends on ARM
tristate "TI High End CAN Controller"
+ select CAN_RX_OFFLOAD
help
Driver for TI HECC (High End CAN Controller) module found on many
TI devices. The device specifications are available from http://www.ti.com
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index b8a55b1d90cd..5081d8a3be57 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -11,3 +11,5 @@ can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o

can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
+
+can-dev-$(CONFIG_CAN_RX_OFFLOAD) += rx-offload.o
diff --git a/drivers/net/can/spi/mcp251xfd/Kconfig b/drivers/net/can/spi/mcp251xfd/Kconfig
index dd0fc0a54be1..877e4356010d 100644
--- a/drivers/net/can/spi/mcp251xfd/Kconfig
+++ b/drivers/net/can/spi/mcp251xfd/Kconfig
@@ -2,6 +2,7 @@

config CAN_MCP251XFD
tristate "Microchip MCP251xFD SPI CAN controllers"
+ select CAN_RX_OFFLOAD
select REGMAP
select WANT_DEV_COREDUMP
help
--
2.35.1

2022-06-06 04:25:22

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] can: Kconfig: add CONFIG_CAN_RX_OFFLOAD

On 03.06.2022 19:28:45, Vincent Mailhol wrote:
> Only a few drivers rely on the CAN rx offload framework (as of the
> writing of this patch, only three: flexcan, ti_hecc and
> mcp251xfd). Give the option to the user to deselect this features
> during compilation.
>
> The drivers relying on CAN rx offload are in different sub
> folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD"
> so that the option is automatically enabled whenever one of those
> driver is chosen.
>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
> drivers/net/can/Kconfig | 16 ++++++++++++++++
> drivers/net/can/dev/Makefile | 2 ++
> drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
> 3 files changed, 19 insertions(+)
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 8f3b97aea638..1f1d81da1c8c 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING
>
> If unsure, say Y.
>
> +config CAN_RX_OFFLOAD
> + bool "CAN RX offload"
> + default y
> + help
> + Framework to offload the controller's RX FIFO during one
> + interrupt. The CAN frames of the FIFO are read and put into a skb
> + queue during that interrupt and transmitted afterwards in a NAPI
> + context.
> +
> + The additional features selected by this option will be added to the
> + can-dev module.
> +
> + If unsure, say Y.
> +
> config CAN_AT91
> tristate "Atmel AT91 onchip CAN controller"
> depends on (ARCH_AT91 || COMPILE_TEST) && HAS_IOMEM
> @@ -113,6 +127,7 @@ config CAN_FLEXCAN
> tristate "Support for Freescale FLEXCAN based chips"
> depends on OF || COLDFIRE || COMPILE_TEST
> depends on HAS_IOMEM
> + select CAN_RX_OFFLOAD
> help
> Say Y here if you want to support for Freescale FlexCAN.
>
> @@ -162,6 +177,7 @@ config CAN_SUN4I
> config CAN_TI_HECC
> depends on ARM
> tristate "TI High End CAN Controller"
> + select CAN_RX_OFFLOAD
> help
> Driver for TI HECC (High End CAN Controller) module found on many
> TI devices. The device specifications are available from http://www.ti.com
> diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
> index b8a55b1d90cd..5081d8a3be57 100644
> --- a/drivers/net/can/dev/Makefile
> +++ b/drivers/net/can/dev/Makefile
> @@ -11,3 +11,5 @@ can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
> can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Do you want to remove this?

>
> can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
> +
> +can-dev-$(CONFIG_CAN_RX_OFFLOAD) += rx-offload.o
> diff --git a/drivers/net/can/spi/mcp251xfd/Kconfig b/drivers/net/can/spi/mcp251xfd/Kconfig
> index dd0fc0a54be1..877e4356010d 100644
> --- a/drivers/net/can/spi/mcp251xfd/Kconfig
> +++ b/drivers/net/can/spi/mcp251xfd/Kconfig
> @@ -2,6 +2,7 @@
>
> config CAN_MCP251XFD
> tristate "Microchip MCP251xFD SPI CAN controllers"
> + select CAN_RX_OFFLOAD
> select REGMAP
> select WANT_DEV_COREDUMP
> help

I remember I've given you a list of drivers needing RX offload, I
probably missed the m_can driver. Feel free to squash this patch:

--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -8,7 +8,6 @@ can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o
can-dev-$(CONFIG_CAN_NETLINK) += dev.o
can-dev-$(CONFIG_CAN_NETLINK) += length.o
can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
-can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o

can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o

diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig
index 45ad1b3f0cd0..fc2afab36279 100644
--- a/drivers/net/can/m_can/Kconfig
+++ b/drivers/net/can/m_can/Kconfig
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
menuconfig CAN_M_CAN
tristate "Bosch M_CAN support"
+ select CAN_RX_OFFLOAD
help
Say Y here if you want support for Bosch M_CAN controller framework.
This is common support for devices that embed the Bosch M_CAN IP.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (4.34 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 04:51:45

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On 05.06.2022 01:32:15, Vincent MAILHOL wrote:
> On Sun. 5 juin 2022 at 00:18, Marc Kleine-Budde <[email protected]> wrote:
> > On 04.06.2022 23:59:48, Vincent MAILHOL wrote:
> > > > > Fine, but I need a bit of guidance here. To provide a tag, I need to
> > > > > have my own git repository hosted online, right?
> > > >
> > > > That is one option.
> > >
> > > This suggests that there are other options? What would be those other
> > > options?
> >
> > 2. git.kernel.org (most preferred)
> > 3. github.com (have to ask Davem/Jakub)
> >
> > > > > Is GitHub OK or should I create one on https://git.kernel.org/?
> > > >
> > > > Some maintainers don't like github, let's wait what Davem and Jakub say.
> > > > I think for git.kernel.org you need a GPG key with signatures of 3 users
> > > > of git.kernel.org.
> > >
> > > Personally, I would also prefer getting my own git.kernel.org account.
> >
> > See https://korg.docs.kernel.org/accounts.html
>
> Thanks for the link. I will have a look at it tomorrow (or the day
> after tomorrow in the worst case).
>
> Meanwhile, I will send the v5 which should address all your comments.

/me just realized that merged are independent of pull requests. I can
create a local branch and merge it, as Davem and Jakub do it. I've added
your v5 to can-next/master as a merge and I'll include this in my next
PR to net-next if Davem and Jakub are OK with merges in my branch.

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.69 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 04:56:45

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On 04.06.2022 23:59:48, Vincent MAILHOL wrote:
> > > Fine, but I need a bit of guidance here. To provide a tag, I need to
> > > have my own git repository hosted online, right?
> >
> > That is one option.
>
> This suggests that there are other options? What would be those other
> options?

2. git.kernel.org (most preferred)
3. github.com (have to ask Davem/Jakub)

> > > Is GitHub OK or should I create one on https://git.kernel.org/?
> >
> > Some maintainers don't like github, let's wait what Davem and Jakub say.
> > I think for git.kernel.org you need a GPG key with signatures of 3 users
> > of git.kernel.org.
>
> Personally, I would also prefer getting my own git.kernel.org account.

See https://korg.docs.kernel.org/accounts.html

> It has infinitely more swag than GitHub.

Definitively!

> But my religion does not forbid me from using GitHub :)

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.15 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 04:58:56

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On 05.06.2022 22:57:03, Vincent MAILHOL wrote:
> > /me just realized that merged are independent of pull requests. I can
> > create a local branch and merge it, as Davem and Jakub do it. I've added
> > your v5 to can-next/master as a merge and I'll include this in my next
> > PR to net-next if Davem and Jakub are OK with merges in my branch.
>
> So my dreams of getting my kernel.org account swag just evaporated
> (just kidding :))

No!

> I think I will prepare a GPG key just to be ready in the opportunity
> to get it signed pop-up one day.
>
> Happy to see that this is reaching an end. Honestly speaking, the
> menuconfig cleanup was not my most exciting contribution (euphemism)
> but was still a necessity.

Thanks for the persistence!

> Glad that this is nearly over after more
> than 80 messages in the full thread (including all five versions). If
> I recall correctly, this is the longest thread we had in the last two
> years. And thanks again to Max, Oliver and you for animating the
> debate!

So the longest-thread-badge goes definitely to you!

Thanks again,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.35 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 05:23:30

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On Sun. 5 juin 2022 at 00:18, Marc Kleine-Budde <[email protected]> wrote:
> On 04.06.2022 23:59:48, Vincent MAILHOL wrote:
> > > > Fine, but I need a bit of guidance here. To provide a tag, I need to
> > > > have my own git repository hosted online, right?
> > >
> > > That is one option.
> >
> > This suggests that there are other options? What would be those other
> > options?
>
> 2. git.kernel.org (most preferred)
> 3. github.com (have to ask Davem/Jakub)
>
> > > > Is GitHub OK or should I create one on https://git.kernel.org/?
> > >
> > > Some maintainers don't like github, let's wait what Davem and Jakub say.
> > > I think for git.kernel.org you need a GPG key with signatures of 3 users
> > > of git.kernel.org.
> >
> > Personally, I would also prefer getting my own git.kernel.org account.
>
> See https://korg.docs.kernel.org/accounts.html

Thanks for the link. I will have a look at it tomorrow (or the day
after tomorrow in the worst case).

Meanwhile, I will send the v5 which should address all your comments.


Yours sincerely,
Vincent Mailhol

2022-06-06 05:25:42

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

Hello Vincent,

wow! This is a great series which addresses a lot of long outstanding
issues. Great work!

As this cover letter brings so much additional information I'll ask
Jakub and David if they take pull request from me, which itself have
merges. This cover letter would be part of my merge. If I get the OK,
can you provide this series as a tag (ideally GPG signed) that I can
pull?

regards,
Marc

On 03.06.2022 19:28:41, Vincent Mailhol wrote:
> Aside of calc_bittiming.o which can be configured with
> CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
> unconditionally to can-dev.o even if not needed by the user.
>
> This series first goal it to split the can-dev modules so that the
> user can decide which features get built in during
> compilation. Additionally, the CAN Device Drivers menu is moved from
> the "Networking support" category to the "Device Drivers" category
> (where all drivers are supposed to be).
>
> Below diagrams illustrate the changes made.
> The arrow symbol "x --> y" denotes that "y depends on x".
>
> * menu before this series *
>
> CAN bus subsystem support
> symbol: CONFIG_CAN
> |
> +-> CAN Device Drivers
> (no symbol)
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> Platform CAN drivers with Netlink support
> symbol: CONFIG_CAN_DEV
> |
> +-> CAN bit-timing calculation (optional for hardware drivers)
> | symbol: CONFIG_CAN_BITTIMING
> |
> +-> All other CAN devices
>
> * menu after this series *
>
> Network device support
> symbol: CONFIG_NETDEVICES
> |
> +-> CAN Device Drivers
> symbol: CONFIG_CAN_DEV
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> CAN device drivers with Netlink support
> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> |
> +-> CAN bit-timing calculation (optional for all drivers)
> | symbol: CONFIG_CAN_BITTIMING
> |
> +-> All other CAN devices not relying on RX offload
> |
> +-> CAN rx offload
> symbol: CONFIG_CAN_RX_OFFLOAD
> |
> +-> CAN devices relying on rx offload
> (at time of writing: flexcan, ti_hecc and mcp251xfd)
>
> Patches 1 to 5 of this series do above modification.
>
> The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
> can_dropped_invalid_skb() to discard tx skb (such skb can potentially
> reach the driver if injected via the packet socket). In more details,
> patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
> 7 is the actual change.
>
> Those last two patches are actually connected to the first five ones:
> because slcan and v(x)can requires can_dropped_invalid_skb(), it was
> necessary to add those three devices to the scope of can-dev before
> moving the function to skb.o.
>
>
> ** N.B. **
>
> This design results from the lengthy discussion in [1].
>
> I did one change from Oliver's suggestions in [2]. The initial idea
> was that the first two config symbols should be respectively
> CAN_DEV_SW and CAN_DEV instead of CAN_DEV and CAN_NETLINK as proposed
> in this series.
>
> * First symbol is changed from CAN_DEV_SW to CAN_DEV. The rationale
> is that it is this entry that will trigger the build of can-dev.o
> and it makes more sense for me to name the symbol share the same
> name as the module. Furthermore, this allows to create a menuentry
> with an explicit name that will cover both the virtual and
> physical devices (naming the menuentry "CAN Device Software" would
> be inconsistent with the fact that physical devices would also be
> present in a sub menu). And not using menuentry complexifies the
> menu.
>
> * Second symbol is renamed from CAN_DEV to CAN_NETLINK because
> CAN_DEV is now taken by the previous menuconfig and netlink is the
> predominant feature added at this level. I am opened to other
> naming suggestion (CAN_DEV_NETLINK, CAN_DEV_HW...?).
>
> [1] https://lore.kernel.org/linux-can/[email protected]/
> [2] https://lore.kernel.org/linux-can/[email protected]/
>
>
> ** Changelog **
>
> v3 -> v4:
>
> * Five additional patches added to split can-dev module and refactor
> Kbuild. c.f. below (lengthy) thread:
> https://lore.kernel.org/linux-can/[email protected]/
>
>
> v2 -> v3:
>
> * Apply can_dropped_invalid_skb() to slcan.
>
> * Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
> modifying Kbuild.
>
> * fix small typos.
>
> v1 -> v2:
>
> * move can_dropped_invalid_skb() to skb.c instead of dev.h
>
> * also move can_skb_headroom_valid() to skb.c
>
> Vincent Mailhol (7):
> can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK
> can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
> CAN_DEV
> can: bittiming: move bittiming calculation functions to
> calc_bittiming.c
> can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
> net: Kconfig: move the CAN device menu to the "Device Drivers" section
> can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
> to skb.c
> can: skb: drop tx skb if in listen only mode
>
> drivers/net/Kconfig | 2 +
> drivers/net/can/Kconfig | 66 +++++++--
> drivers/net/can/dev/Makefile | 20 ++-
> drivers/net/can/dev/bittiming.c | 197 -------------------------
> drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
> drivers/net/can/dev/dev.c | 9 +-
> drivers/net/can/dev/skb.c | 72 +++++++++
> drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
> include/linux/can/skb.h | 59 +-------
> net/can/Kconfig | 5 +-
> 10 files changed, 351 insertions(+), 282 deletions(-)
> create mode 100644 drivers/net/can/dev/calc_bittiming.c
>
> --
> 2.35.1
>
>

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (6.45 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 05:38:52

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] can: Kconfig: add CONFIG_CAN_RX_OFFLOAD

On Sat. 4 June 2022 at 20:22, Marc Kleine-Budde <[email protected]> wrote:
> On 03.06.2022 19:28:45, Vincent Mailhol wrote:
> > Only a few drivers rely on the CAN rx offload framework (as of the
> > writing of this patch, only three: flexcan, ti_hecc and
> > mcp251xfd). Give the option to the user to deselect this features
> > during compilation.
> >
> > The drivers relying on CAN rx offload are in different sub
> > folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD"
> > so that the option is automatically enabled whenever one of those
> > driver is chosen.
> >
> > Signed-off-by: Vincent Mailhol <[email protected]>
> > ---
> > drivers/net/can/Kconfig | 16 ++++++++++++++++
> > drivers/net/can/dev/Makefile | 2 ++
> > drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
> > 3 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index 8f3b97aea638..1f1d81da1c8c 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING
> >
> > If unsure, say Y.
> >
> > +config CAN_RX_OFFLOAD
> > + bool "CAN RX offload"
> > + default y
> > + help
> > + Framework to offload the controller's RX FIFO during one
> > + interrupt. The CAN frames of the FIFO are read and put into a skb
> > + queue during that interrupt and transmitted afterwards in a NAPI
> > + context.
> > +
> > + The additional features selected by this option will be added to the
> > + can-dev module.
> > +
> > + If unsure, say Y.
> > +
> > config CAN_AT91
> > tristate "Atmel AT91 onchip CAN controller"
> > depends on (ARCH_AT91 || COMPILE_TEST) && HAS_IOMEM
> > @@ -113,6 +127,7 @@ config CAN_FLEXCAN
> > tristate "Support for Freescale FLEXCAN based chips"
> > depends on OF || COLDFIRE || COMPILE_TEST
> > depends on HAS_IOMEM
> > + select CAN_RX_OFFLOAD
> > help
> > Say Y here if you want to support for Freescale FlexCAN.
> >
> > @@ -162,6 +177,7 @@ config CAN_SUN4I
> > config CAN_TI_HECC
> > depends on ARM
> > tristate "TI High End CAN Controller"
> > + select CAN_RX_OFFLOAD
> > help
> > Driver for TI HECC (High End CAN Controller) module found on many
> > TI devices. The device specifications are available from http://www.ti.com
> > diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
> > index b8a55b1d90cd..5081d8a3be57 100644
> > --- a/drivers/net/can/dev/Makefile
> > +++ b/drivers/net/can/dev/Makefile
> > @@ -11,3 +11,5 @@ can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
> > can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Do you want to remove this?

Absolutely. As you probably guessed, this is just a leftover.

> >
> > can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
> > +
> > +can-dev-$(CONFIG_CAN_RX_OFFLOAD) += rx-offload.o
> > diff --git a/drivers/net/can/spi/mcp251xfd/Kconfig b/drivers/net/can/spi/mcp251xfd/Kconfig
> > index dd0fc0a54be1..877e4356010d 100644
> > --- a/drivers/net/can/spi/mcp251xfd/Kconfig
> > +++ b/drivers/net/can/spi/mcp251xfd/Kconfig
> > @@ -2,6 +2,7 @@
> >
> > config CAN_MCP251XFD
> > tristate "Microchip MCP251xFD SPI CAN controllers"
> > + select CAN_RX_OFFLOAD
> > select REGMAP
> > select WANT_DEV_COREDUMP
> > help
>
> I remember I've given you a list of drivers needing RX offload, I
> probably missed the m_can driver. Feel free to squash this patch:

Added it to v5.

This went through the cracks when testing. Thanks for catching this!

> --- a/drivers/net/can/dev/Makefile
> +++ b/drivers/net/can/dev/Makefile
> @@ -8,7 +8,6 @@ can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o
> can-dev-$(CONFIG_CAN_NETLINK) += dev.o
> can-dev-$(CONFIG_CAN_NETLINK) += length.o
> can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
> -can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
>
> can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
>
> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig
> index 45ad1b3f0cd0..fc2afab36279 100644
> --- a/drivers/net/can/m_can/Kconfig
> +++ b/drivers/net/can/m_can/Kconfig
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> menuconfig CAN_M_CAN
> tristate "Bosch M_CAN support"
> + select CAN_RX_OFFLOAD
> help
> Say Y here if you want support for Bosch M_CAN controller framework.
> This is common support for devices that embed the Bosch M_CAN IP.
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-06-06 05:58:12

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

Aside of calc_bittiming.o which can be configured with
CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
unconditionally to can-dev.o even if not needed by the user.

This series first goal it to split the can-dev modules so that the
user can decide which features get built in during
compilation. Additionally, the CAN Device Drivers menu is moved from
the "Networking support" category to the "Device Drivers" category
(where all drivers are supposed to be).

Below diagrams illustrate the changes made.
The arrow symbol "x --> y" denotes that "y depends on x".

* menu before this series *

CAN bus subsystem support
symbol: CONFIG_CAN
|
+-> CAN Device Drivers
(no symbol)
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> Platform CAN drivers with Netlink support
symbol: CONFIG_CAN_DEV
|
+-> CAN bit-timing calculation (optional for hardware drivers)
| symbol: CONFIG_CAN_BITTIMING
|
+-> All other CAN devices

* menu after this series *

Network device support
symbol: CONFIG_NETDEVICES
|
+-> CAN Device Drivers
symbol: CONFIG_CAN_DEV
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> CAN device drivers with Netlink support
symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
|
+-> CAN bit-timing calculation (optional for all drivers)
| symbol: CONFIG_CAN_BITTIMING
|
+-> All other CAN devices not relying on RX offload
|
+-> CAN rx offload
symbol: CONFIG_CAN_RX_OFFLOAD
|
+-> CAN devices relying on rx offload
(at time of writing: flexcan, m_can, mcp251xfd and ti_hecc)

Patches 1 to 5 of this series do above modification.

The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
can_dropped_invalid_skb() to discard tx skb (such skb can potentially
reach the driver if injected via the packet socket). In more details,
patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
7 is the actual change.

Those last two patches are actually connected to the first five ones:
because slcan and v(x)can requires can_dropped_invalid_skb(), it was
necessary to add those three devices to the scope of can-dev before
moving the function to skb.o.

This design results from the lengthy discussion in [1].

[1] https://lore.kernel.org/linux-can/[email protected]/


** Changelog **

v4 -> v5:

* m_can is also requires RX offload. Add the "select CAN_RX_OFFLOAD"
to its Makefile.

* Reorder the lines of drivers/net/can/dev/Makefile.

* Remove duplicated rx-offload.o target in drivers/net/can/dev/Makefile

* Remove the Nota Bene in the cover letter.


v3 -> v4:

* Five additional patches added to split can-dev module and refactor
Kbuild. c.f. below (lengthy) thread:
https://lore.kernel.org/linux-can/[email protected]/


v2 -> v3:

* Apply can_dropped_invalid_skb() to slcan.

* Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
modifying Kbuild.

* fix small typos.

v1 -> v2:

* move can_dropped_invalid_skb() to skb.c instead of dev.h

* also move can_skb_headroom_valid() to skb.c

Vincent Mailhol (7):
can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK
can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
CAN_DEV
can: bittiming: move bittiming calculation functions to
calc_bittiming.c
can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
net: Kconfig: move the CAN device menu to the "Device Drivers" section
can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
to skb.c
can: skb: drop tx skb if in listen only mode

drivers/net/Kconfig | 2 +
drivers/net/can/Kconfig | 66 +++++++--
drivers/net/can/dev/Makefile | 17 ++-
drivers/net/can/dev/bittiming.c | 197 -------------------------
drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
drivers/net/can/dev/dev.c | 9 +-
drivers/net/can/dev/skb.c | 72 +++++++++
drivers/net/can/m_can/Kconfig | 1 +
drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
include/linux/can/skb.h | 59 +-------
net/can/Kconfig | 5 +-
11 files changed, 349 insertions(+), 282 deletions(-)
create mode 100644 drivers/net/can/dev/calc_bittiming.c

--
2.35.1

2022-06-06 05:58:38

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

On 04.06.2022 22:05:09, Vincent MAILHOL wrote:
> On Sat. 4 juin 2022 at 20:46, Marc Kleine-Budde <[email protected]> wrote:
> > Hello Vincent,
> >
> > wow! This is a great series which addresses a lot of long outstanding
> > issues. Great work!
>
> Thanks.
>
> > As this cover letter brings so much additional information I'll ask
> > Jakub and David if they take pull request from me, which itself have
> > merges. This cover letter would be part of my merge. If I get the OK,
> > can you provide this series as a tag (ideally GPG signed) that I can
> > pull?
>
> Fine, but I need a bit of guidance here. To provide a tag, I need to
> have my own git repository hosted online, right?

That is one option.

> Is GitHub OK or should I create one on https://git.kernel.org/?

Some maintainers don't like github, let's wait what Davem and Jakub say.
I think for git.kernel.org you need a GPG key with signatures of 3 users
of git.kernel.org.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.22 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-06 06:01:44

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 6/7] can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid() to skb.c

The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
grew a lot over the years to a point which it does not make much sense
to have them defined as static inline in header files. Move those two
functions to the .c counterpart of skb.h.

can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
the declaration is removed from the header. Only
can_dropped_invalid_skb() gets its symbol exported.

While doing so, do a small cleanup: add brackets around the else block
in can_dropped_invalid_skb().

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/skb.c | 58 ++++++++++++++++++++++++++++++++++++++
include/linux/can/skb.h | 59 +--------------------------------------
2 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index a4208f125b76..dc9da76c0470 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -259,3 +259,61 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
return skb;
}
EXPORT_SYMBOL_GPL(alloc_can_err_skb);
+
+/* Check for outgoing skbs that have not been created by the CAN subsystem */
+static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
+{
+ /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
+ if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
+ return false;
+
+ /* af_packet does not apply CAN skb specific settings */
+ if (skb->ip_summed == CHECKSUM_NONE) {
+ /* init headroom */
+ can_skb_prv(skb)->ifindex = dev->ifindex;
+ can_skb_prv(skb)->skbcnt = 0;
+
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ /* perform proper loopback on capable devices */
+ if (dev->flags & IFF_ECHO)
+ skb->pkt_type = PACKET_LOOPBACK;
+ else
+ skb->pkt_type = PACKET_HOST;
+
+ skb_reset_mac_header(skb);
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+ }
+
+ return true;
+}
+
+/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
+bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
+{
+ const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+ if (skb->protocol == htons(ETH_P_CAN)) {
+ if (unlikely(skb->len != CAN_MTU ||
+ cfd->len > CAN_MAX_DLEN))
+ goto inval_skb;
+ } else if (skb->protocol == htons(ETH_P_CANFD)) {
+ if (unlikely(skb->len != CANFD_MTU ||
+ cfd->len > CANFD_MAX_DLEN))
+ goto inval_skb;
+ } else {
+ goto inval_skb;
+ }
+
+ if (!can_skb_headroom_valid(dev, skb))
+ goto inval_skb;
+
+ return false;
+
+inval_skb:
+ kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return true;
+}
+EXPORT_SYMBOL_GPL(can_dropped_invalid_skb);
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index fdb22b00674a..182749e858b3 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -31,6 +31,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
struct sk_buff *alloc_can_err_skb(struct net_device *dev,
struct can_frame **cf);
+bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);

/*
* The struct can_skb_priv is used to transport additional information along
@@ -96,64 +97,6 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
return nskb;
}

-/* Check for outgoing skbs that have not been created by the CAN subsystem */
-static inline bool can_skb_headroom_valid(struct net_device *dev,
- struct sk_buff *skb)
-{
- /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
- if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
- return false;
-
- /* af_packet does not apply CAN skb specific settings */
- if (skb->ip_summed == CHECKSUM_NONE) {
- /* init headroom */
- can_skb_prv(skb)->ifindex = dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
-
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- /* perform proper loopback on capable devices */
- if (dev->flags & IFF_ECHO)
- skb->pkt_type = PACKET_LOOPBACK;
- else
- skb->pkt_type = PACKET_HOST;
-
- skb_reset_mac_header(skb);
- skb_reset_network_header(skb);
- skb_reset_transport_header(skb);
- }
-
- return true;
-}
-
-/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
-static inline bool can_dropped_invalid_skb(struct net_device *dev,
- struct sk_buff *skb)
-{
- const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
- if (skb->protocol == htons(ETH_P_CAN)) {
- if (unlikely(skb->len != CAN_MTU ||
- cfd->len > CAN_MAX_DLEN))
- goto inval_skb;
- } else if (skb->protocol == htons(ETH_P_CANFD)) {
- if (unlikely(skb->len != CANFD_MTU ||
- cfd->len > CANFD_MAX_DLEN))
- goto inval_skb;
- } else
- goto inval_skb;
-
- if (!can_skb_headroom_valid(dev, skb))
- goto inval_skb;
-
- return false;
-
-inval_skb:
- kfree_skb(skb);
- dev->stats.tx_dropped++;
- return true;
-}
-
static inline bool can_is_canfd_skb(const struct sk_buff *skb)
{
/* the CAN specific type of skb is identified by its data length */
--
2.35.1

2022-06-06 06:03:20

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 3/7] can: bittiming: move bittiming calculation functions to calc_bittiming.c

The canonical way to select or deselect an object during compilation
is to use this pattern in the relevant Makefile:

bar-$(CONFIG_FOO) := foo.o

bittiming.c instead uses some #ifdef CONFIG_CAN_CALC_BITTIMG.

Create a new file named calc_bittiming.c with all the functions which
are conditionally compiled with CONFIG_CAN_CALC_BITTIMG and modify the
Makefile according to above pattern.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/Kconfig | 4 +
drivers/net/can/dev/Makefile | 2 +
drivers/net/can/dev/bittiming.c | 197 --------------------------
drivers/net/can/dev/calc_bittiming.c | 202 +++++++++++++++++++++++++++
4 files changed, 208 insertions(+), 197 deletions(-)
create mode 100644 drivers/net/can/dev/calc_bittiming.c

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index b1e47f6c5586..8f3b97aea638 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -96,6 +96,10 @@ config CAN_CALC_BITTIMING
source clock frequencies. Disabling saves some space, but then the
bit-timing parameters must be specified directly using the Netlink
arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
+
+ The additional features selected by this option will be added to the
+ can-dev module.
+
If unsure, say Y.

config CAN_AT91
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index 919f87e36eed..b8a55b1d90cd 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -9,3 +9,5 @@ can-dev-$(CONFIG_CAN_NETLINK) += dev.o
can-dev-$(CONFIG_CAN_NETLINK) += length.o
can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
+
+can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index c1e76f0a5064..7ae80763c960 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -4,205 +4,8 @@
* Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
*/

-#include <linux/units.h>
#include <linux/can/dev.h>

-#ifdef CONFIG_CAN_CALC_BITTIMING
-#define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
-
-/* Bit-timing calculation derived from:
- *
- * Code based on LinCAN sources and H8S2638 project
- * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
- * Copyright 2005 Stanislav Marek
- * email: [email protected]
- *
- * Calculates proper bit-timing parameters for a specified bit-rate
- * and sample-point, which can then be used to set the bit-timing
- * registers of the CAN controller. You can find more information
- * in the header file linux/can/netlink.h.
- */
-static int
-can_update_sample_point(const struct can_bittiming_const *btc,
- const unsigned int sample_point_nominal, const unsigned int tseg,
- unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
- unsigned int *sample_point_error_ptr)
-{
- unsigned int sample_point_error, best_sample_point_error = UINT_MAX;
- unsigned int sample_point, best_sample_point = 0;
- unsigned int tseg1, tseg2;
- int i;
-
- for (i = 0; i <= 1; i++) {
- tseg2 = tseg + CAN_SYNC_SEG -
- (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
- 1000 - i;
- tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
- tseg1 = tseg - tseg2;
- if (tseg1 > btc->tseg1_max) {
- tseg1 = btc->tseg1_max;
- tseg2 = tseg - tseg1;
- }
-
- sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
- (tseg + CAN_SYNC_SEG);
- sample_point_error = abs(sample_point_nominal - sample_point);
-
- if (sample_point <= sample_point_nominal &&
- sample_point_error < best_sample_point_error) {
- best_sample_point = sample_point;
- best_sample_point_error = sample_point_error;
- *tseg1_ptr = tseg1;
- *tseg2_ptr = tseg2;
- }
- }
-
- if (sample_point_error_ptr)
- *sample_point_error_ptr = best_sample_point_error;
-
- return best_sample_point;
-}
-
-int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
- const struct can_bittiming_const *btc)
-{
- struct can_priv *priv = netdev_priv(dev);
- unsigned int bitrate; /* current bitrate */
- unsigned int bitrate_error; /* difference between current and nominal value */
- unsigned int best_bitrate_error = UINT_MAX;
- unsigned int sample_point_error; /* difference between current and nominal value */
- unsigned int best_sample_point_error = UINT_MAX;
- unsigned int sample_point_nominal; /* nominal sample point */
- unsigned int best_tseg = 0; /* current best value for tseg */
- unsigned int best_brp = 0; /* current best value for brp */
- unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
- u64 v64;
-
- /* Use CiA recommended sample points */
- if (bt->sample_point) {
- sample_point_nominal = bt->sample_point;
- } else {
- if (bt->bitrate > 800 * KILO /* BPS */)
- sample_point_nominal = 750;
- else if (bt->bitrate > 500 * KILO /* BPS */)
- sample_point_nominal = 800;
- else
- sample_point_nominal = 875;
- }
-
- /* tseg even = round down, odd = round up */
- for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
- tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
- tsegall = CAN_SYNC_SEG + tseg / 2;
-
- /* Compute all possible tseg choices (tseg=tseg1+tseg2) */
- brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
-
- /* choose brp step which is possible in system */
- brp = (brp / btc->brp_inc) * btc->brp_inc;
- if (brp < btc->brp_min || brp > btc->brp_max)
- continue;
-
- bitrate = priv->clock.freq / (brp * tsegall);
- bitrate_error = abs(bt->bitrate - bitrate);
-
- /* tseg brp biterror */
- if (bitrate_error > best_bitrate_error)
- continue;
-
- /* reset sample point error if we have a better bitrate */
- if (bitrate_error < best_bitrate_error)
- best_sample_point_error = UINT_MAX;
-
- can_update_sample_point(btc, sample_point_nominal, tseg / 2,
- &tseg1, &tseg2, &sample_point_error);
- if (sample_point_error >= best_sample_point_error)
- continue;
-
- best_sample_point_error = sample_point_error;
- best_bitrate_error = bitrate_error;
- best_tseg = tseg / 2;
- best_brp = brp;
-
- if (bitrate_error == 0 && sample_point_error == 0)
- break;
- }
-
- if (best_bitrate_error) {
- /* Error in one-tenth of a percent */
- v64 = (u64)best_bitrate_error * 1000;
- do_div(v64, bt->bitrate);
- bitrate_error = (u32)v64;
- if (bitrate_error > CAN_CALC_MAX_ERROR) {
- netdev_err(dev,
- "bitrate error %d.%d%% too high\n",
- bitrate_error / 10, bitrate_error % 10);
- return -EDOM;
- }
- netdev_warn(dev, "bitrate error %d.%d%%\n",
- bitrate_error / 10, bitrate_error % 10);
- }
-
- /* real sample point */
- bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
- best_tseg, &tseg1, &tseg2,
- NULL);
-
- v64 = (u64)best_brp * 1000 * 1000 * 1000;
- do_div(v64, priv->clock.freq);
- bt->tq = (u32)v64;
- bt->prop_seg = tseg1 / 2;
- bt->phase_seg1 = tseg1 - bt->prop_seg;
- bt->phase_seg2 = tseg2;
-
- /* check for sjw user settings */
- if (!bt->sjw || !btc->sjw_max) {
- bt->sjw = 1;
- } else {
- /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
- if (bt->sjw > btc->sjw_max)
- bt->sjw = btc->sjw_max;
- /* bt->sjw must not be higher than tseg2 */
- if (tseg2 < bt->sjw)
- bt->sjw = tseg2;
- }
-
- bt->brp = best_brp;
-
- /* real bitrate */
- bt->bitrate = priv->clock.freq /
- (bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
-
- return 0;
-}
-
-void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
- const struct can_bittiming *dbt,
- u32 *ctrlmode, u32 ctrlmode_supported)
-
-{
- if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
- return;
-
- *ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
-
- /* As specified in ISO 11898-1 section 11.3.3 "Transmitter
- * delay compensation" (TDC) is only applicable if data BRP is
- * one or two.
- */
- if (dbt->brp == 1 || dbt->brp == 2) {
- /* Sample point in clock periods */
- u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
- dbt->phase_seg1) * dbt->brp;
-
- if (sample_point_in_tc < tdc_const->tdco_min)
- return;
- tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
- *ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
- }
-}
-#endif /* CONFIG_CAN_CALC_BITTIMING */
-
/* Checks the validity of the specified bit-timing parameters prop_seg,
* phase_seg1, phase_seg2 and sjw and tries to determine the bitrate
* prescaler value brp. You can find more information in the header
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
new file mode 100644
index 000000000000..d3caa040614d
--- /dev/null
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2005 Marc Kleine-Budde, Pengutronix
+ * Copyright (C) 2006 Andrey Volkov, Varma Electronics
+ * Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
+ */
+
+#include <linux/units.h>
+#include <linux/can/dev.h>
+
+#define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
+
+/* Bit-timing calculation derived from:
+ *
+ * Code based on LinCAN sources and H8S2638 project
+ * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
+ * Copyright 2005 Stanislav Marek
+ * email: [email protected]
+ *
+ * Calculates proper bit-timing parameters for a specified bit-rate
+ * and sample-point, which can then be used to set the bit-timing
+ * registers of the CAN controller. You can find more information
+ * in the header file linux/can/netlink.h.
+ */
+static int
+can_update_sample_point(const struct can_bittiming_const *btc,
+ const unsigned int sample_point_nominal, const unsigned int tseg,
+ unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
+ unsigned int *sample_point_error_ptr)
+{
+ unsigned int sample_point_error, best_sample_point_error = UINT_MAX;
+ unsigned int sample_point, best_sample_point = 0;
+ unsigned int tseg1, tseg2;
+ int i;
+
+ for (i = 0; i <= 1; i++) {
+ tseg2 = tseg + CAN_SYNC_SEG -
+ (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
+ 1000 - i;
+ tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
+ tseg1 = tseg - tseg2;
+ if (tseg1 > btc->tseg1_max) {
+ tseg1 = btc->tseg1_max;
+ tseg2 = tseg - tseg1;
+ }
+
+ sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
+ (tseg + CAN_SYNC_SEG);
+ sample_point_error = abs(sample_point_nominal - sample_point);
+
+ if (sample_point <= sample_point_nominal &&
+ sample_point_error < best_sample_point_error) {
+ best_sample_point = sample_point;
+ best_sample_point_error = sample_point_error;
+ *tseg1_ptr = tseg1;
+ *tseg2_ptr = tseg2;
+ }
+ }
+
+ if (sample_point_error_ptr)
+ *sample_point_error_ptr = best_sample_point_error;
+
+ return best_sample_point;
+}
+
+int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
+{
+ struct can_priv *priv = netdev_priv(dev);
+ unsigned int bitrate; /* current bitrate */
+ unsigned int bitrate_error; /* difference between current and nominal value */
+ unsigned int best_bitrate_error = UINT_MAX;
+ unsigned int sample_point_error; /* difference between current and nominal value */
+ unsigned int best_sample_point_error = UINT_MAX;
+ unsigned int sample_point_nominal; /* nominal sample point */
+ unsigned int best_tseg = 0; /* current best value for tseg */
+ unsigned int best_brp = 0; /* current best value for brp */
+ unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
+ u64 v64;
+
+ /* Use CiA recommended sample points */
+ if (bt->sample_point) {
+ sample_point_nominal = bt->sample_point;
+ } else {
+ if (bt->bitrate > 800 * KILO /* BPS */)
+ sample_point_nominal = 750;
+ else if (bt->bitrate > 500 * KILO /* BPS */)
+ sample_point_nominal = 800;
+ else
+ sample_point_nominal = 875;
+ }
+
+ /* tseg even = round down, odd = round up */
+ for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
+ tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
+ tsegall = CAN_SYNC_SEG + tseg / 2;
+
+ /* Compute all possible tseg choices (tseg=tseg1+tseg2) */
+ brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
+
+ /* choose brp step which is possible in system */
+ brp = (brp / btc->brp_inc) * btc->brp_inc;
+ if (brp < btc->brp_min || brp > btc->brp_max)
+ continue;
+
+ bitrate = priv->clock.freq / (brp * tsegall);
+ bitrate_error = abs(bt->bitrate - bitrate);
+
+ /* tseg brp biterror */
+ if (bitrate_error > best_bitrate_error)
+ continue;
+
+ /* reset sample point error if we have a better bitrate */
+ if (bitrate_error < best_bitrate_error)
+ best_sample_point_error = UINT_MAX;
+
+ can_update_sample_point(btc, sample_point_nominal, tseg / 2,
+ &tseg1, &tseg2, &sample_point_error);
+ if (sample_point_error >= best_sample_point_error)
+ continue;
+
+ best_sample_point_error = sample_point_error;
+ best_bitrate_error = bitrate_error;
+ best_tseg = tseg / 2;
+ best_brp = brp;
+
+ if (bitrate_error == 0 && sample_point_error == 0)
+ break;
+ }
+
+ if (best_bitrate_error) {
+ /* Error in one-tenth of a percent */
+ v64 = (u64)best_bitrate_error * 1000;
+ do_div(v64, bt->bitrate);
+ bitrate_error = (u32)v64;
+ if (bitrate_error > CAN_CALC_MAX_ERROR) {
+ netdev_err(dev,
+ "bitrate error %d.%d%% too high\n",
+ bitrate_error / 10, bitrate_error % 10);
+ return -EDOM;
+ }
+ netdev_warn(dev, "bitrate error %d.%d%%\n",
+ bitrate_error / 10, bitrate_error % 10);
+ }
+
+ /* real sample point */
+ bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
+ best_tseg, &tseg1, &tseg2,
+ NULL);
+
+ v64 = (u64)best_brp * 1000 * 1000 * 1000;
+ do_div(v64, priv->clock.freq);
+ bt->tq = (u32)v64;
+ bt->prop_seg = tseg1 / 2;
+ bt->phase_seg1 = tseg1 - bt->prop_seg;
+ bt->phase_seg2 = tseg2;
+
+ /* check for sjw user settings */
+ if (!bt->sjw || !btc->sjw_max) {
+ bt->sjw = 1;
+ } else {
+ /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
+ if (bt->sjw > btc->sjw_max)
+ bt->sjw = btc->sjw_max;
+ /* bt->sjw must not be higher than tseg2 */
+ if (tseg2 < bt->sjw)
+ bt->sjw = tseg2;
+ }
+
+ bt->brp = best_brp;
+
+ /* real bitrate */
+ bt->bitrate = priv->clock.freq /
+ (bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
+
+ return 0;
+}
+
+void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
+ const struct can_bittiming *dbt,
+ u32 *ctrlmode, u32 ctrlmode_supported)
+
+{
+ if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
+ return;
+
+ *ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+
+ /* As specified in ISO 11898-1 section 11.3.3 "Transmitter
+ * delay compensation" (TDC) is only applicable if data BRP is
+ * one or two.
+ */
+ if (dbt->brp == 1 || dbt->brp == 2) {
+ /* Sample point in clock periods */
+ u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
+ dbt->phase_seg1) * dbt->brp;
+
+ if (sample_point_in_tc < tdc_const->tdco_min)
+ return;
+ tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
+ *ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
+ }
+}
--
2.35.1

2022-06-06 06:03:44

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v5 7/7] can: skb: drop tx skb if in listen only mode

Frames can be directly injected to a can driver via the packet
socket. By doing so, it is possible to reach the
net_device_ops::ndo_start_xmit function even if the driver is
configured in listen only mode.

Add a check in can_dropped_invalid_skb() to discard the skb if
CAN_CTRLMODE_LISTENONLY is set.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/skb.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index dc9da76c0470..8bb62dd864c8 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -5,6 +5,7 @@
*/

#include <linux/can/dev.h>
+#include <linux/can/netlink.h>
#include <linux/module.h>

#define MOD_DESC "CAN device driver interface"
@@ -293,6 +294,7 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
{
const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+ struct can_priv *priv = netdev_priv(dev);

if (skb->protocol == htons(ETH_P_CAN)) {
if (unlikely(skb->len != CAN_MTU ||
@@ -306,8 +308,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
goto inval_skb;
}

- if (!can_skb_headroom_valid(dev, skb))
+ if (!can_skb_headroom_valid(dev, skb)) {
+ goto inval_skb;
+ } else if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+ netdev_info_once(dev,
+ "interface in listen only mode, dropping skb\n");
goto inval_skb;
+ }

return false;

--
2.35.1

2022-06-06 06:09:12

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v4 2/7] can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using CAN_DEV

In the next patches, the software/virtual drivers (slcan, v(x)can)
will depend on drivers/net/can/dev/skb.o.

This patch changes the scope of the can-dev module to include the
above mentioned drivers.

To do so, we reuse the menu "CAN Device Drivers" and turn it into a
configmenu using the config symbol CAN_DEV (which we released in
previous patch). Also, add a description to this new CAN_DEV
menuconfig.

The symbol CAN_DEV now only triggers the build of skb.o. For this
reasons, all the macros from linux/module.h are deported from
drivers/net/can/dev/dev.c to drivers/net/can/dev/skb.c.

Finally, drivers/net/can/dev/Makefile is adjusted accordingly.

Suggested-by: Oliver Hartkopp <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/Kconfig | 29 ++++++++++++++++++++++++++---
drivers/net/can/dev/Makefile | 16 +++++++++-------
drivers/net/can/dev/dev.c | 9 +--------
drivers/net/can/dev/skb.c | 7 +++++++
4 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 99f189ad35ad..b1e47f6c5586 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -1,5 +1,25 @@
# SPDX-License-Identifier: GPL-2.0-only
-menu "CAN Device Drivers"
+
+menuconfig CAN_DEV
+ tristate "CAN Device Drivers"
+ default y
+ help
+ Controller Area Network (CAN) is serial communications protocol up to
+ 1Mbit/s for its original release (now known as Classical CAN) and up
+ to 8Mbit/s for the more recent CAN with Flexible Data-Rate
+ (CAN-FD). The CAN bus was originally mainly for automotive, but is now
+ widely used in marine (NMEA2000), industrial, and medical
+ applications. More information on the CAN network protocol family
+ PF_CAN is contained in <Documentation/networking/can.rst>.
+
+ This section contains all the CAN(-FD) device drivers including the
+ virtual ones. If you own such devices or plan to use the virtual CAN
+ interface to develop applications, say Y here.
+
+ To compile as a module, choose M here: the module will be called
+ can-dev.
+
+if CAN_DEV

config CAN_VCAN
tristate "Virtual Local CAN Interface (vcan)"
@@ -49,7 +69,7 @@ config CAN_SLCAN
also be built as a module. If so, the module will be called slcan.

config CAN_NETLINK
- tristate "CAN device drivers with Netlink support"
+ bool "CAN device drivers with Netlink support"
default y
help
Enables the common framework for CAN device drivers. This is the
@@ -57,6 +77,9 @@ config CAN_NETLINK
as bittiming validation, support of CAN error states, device restart
and others.

+ The additional features selected by this option will be added to the
+ can-dev module.
+
This is required by all platform and hardware CAN drivers. If you
plan to use such devices or if unsure, say Y.

@@ -178,4 +201,4 @@ config CAN_DEBUG_DEVICES
a problem with CAN support and want to see more of what is going
on.

-endmenu
+endif #CAN_DEV
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index 5b4c813c6222..919f87e36eed 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -1,9 +1,11 @@
# SPDX-License-Identifier: GPL-2.0

-obj-$(CONFIG_CAN_NETLINK) += can-dev.o
-can-dev-y += bittiming.o
-can-dev-y += dev.o
-can-dev-y += length.o
-can-dev-y += netlink.o
-can-dev-y += rx-offload.o
-can-dev-y += skb.o
+obj-$(CONFIG_CAN_DEV) += can-dev.o
+
+can-dev-$(CONFIG_CAN_DEV) += skb.o
+
+can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o
+can-dev-$(CONFIG_CAN_NETLINK) += dev.o
+can-dev-$(CONFIG_CAN_NETLINK) += length.o
+can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
+can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 96c9d9db00cf..523eaacfe29e 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -4,7 +4,6 @@
* Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
*/

-#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/netdevice.h>
@@ -17,12 +16,6 @@
#include <linux/gpio/consumer.h>
#include <linux/of.h>

-#define MOD_DESC "CAN device driver interface"
-
-MODULE_DESCRIPTION(MOD_DESC);
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
-
static void can_update_state_error_stats(struct net_device *dev,
enum can_state new_state)
{
@@ -513,7 +506,7 @@ static __init int can_dev_init(void)

err = can_netlink_register();
if (!err)
- pr_info(MOD_DESC "\n");
+ pr_info("CAN device driver interface\n");

return err;
}
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 61660248c69e..a4208f125b76 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -5,6 +5,13 @@
*/

#include <linux/can/dev.h>
+#include <linux/module.h>
+
+#define MOD_DESC "CAN device driver interface"
+
+MODULE_DESCRIPTION(MOD_DESC);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");

/* Local echo of CAN messages
*
--
2.35.1

2022-06-07 01:46:51

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

Hi Vincent,

great work!

On 04.06.22 18:29, Vincent Mailhol wrote:

> * menu after this series *
>
> Network device support
> symbol: CONFIG_NETDEVICES
> |
> +-> CAN Device Drivers
> symbol: CONFIG_CAN_DEV
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> CAN device drivers with Netlink support
> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> |
> +-> CAN bit-timing calculation (optional for all drivers)
> | symbol: CONFIG_CAN_BITTIMING
> |
> +-> All other CAN devices not relying on RX offload
> |
> +-> CAN rx offload
> symbol: CONFIG_CAN_RX_OFFLOAD

Is this still true in patch series 5?

If I understood it correctly CONFIG_CAN_BITTIMING and
CONFIG_CAN_RX_OFFLOAD can be enabled by the user and
(alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd
and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.

Right?

> |
> +-> CAN devices relying on rx offload
> (at time of writing: flexcan, m_can, mcp251xfd and ti_hecc)

Best regards,
Oliver

2022-06-07 08:53:47

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

On 07.06.2022 11:49:30, Vincent MAILHOL wrote:
[...]
> So I think that the diagram is correct. Maybe rephrasing the cover
> letter as below would address your concerns?

BTW: I got the OK from Jakub to send PR with merges.

If you think the cover letter needs rephrasing, send a new series and
I'm going to force push that over can-next/master. After that let's
consider can-next/master as fast-forward only.

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (708.00 B)
signature.asc (499.00 B)
Download all attachments

2022-06-07 09:20:48

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

On Tue. 7 Jun. 2022 at 04:43, Oliver Hartkopp <[email protected]> wrote:
>
> Hi Vincent,
>
> great work!

Thanks!

> On 04.06.22 18:29, Vincent Mailhol wrote:
>
> > * menu after this series *
> >
> > Network device support
> > symbol: CONFIG_NETDEVICES
> > |
> > +-> CAN Device Drivers
> > symbol: CONFIG_CAN_DEV
> > |
> > +-> software/virtual CAN device drivers
> > | (at time of writing: slcan, vcan, vxcan)
> > |
> > +-> CAN device drivers with Netlink support
> > symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> > |
> > +-> CAN bit-timing calculation (optional for all drivers)
> > | symbol: CONFIG_CAN_BITTIMING
^^^^^^^^^^^^^^^^^^^^
Typo: the symbol is CONFIG_CAN_*CALC*_BITTIMING. I made that typo
twice in the cover letter (once in each diagram). The patches and
their comments remain correct.

> > |
> > +-> All other CAN devices not relying on RX offload
> > |
> > +-> CAN rx offload
> > symbol: CONFIG_CAN_RX_OFFLOAD
>
> Is this still true in patch series 5?
>
> If I understood it correctly CONFIG_CAN_BITTIMING and
> CONFIG_CAN_RX_OFFLOAD can be enabled by the user and
> (alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd
> and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.
>
> Right?

Yes, this is correct. Maybe what troubles you is the meaning of the
"x --> y" arrow in the graph. I said it denotes that "y depends on x".
Here "depends on" has a loose meaning. It translates to either:
* Feature Y is encapsulated in Kbuild by some "if X/endif" and won't
show up unless X is selected.
* Feature Y has a "selects X" tag and will forcibly enable X if selected.

CONFIG_CAN_*CALC*_BITTIMING is on the left side of an arrow starting
from CONFIG_CAN_NETLINK so it "depends" on CONFIG_CAN_NETLINK. On the
other hand, CONFIG_CAN_*CALC*_BITTIMING does not have any arrow
starting from it so indeed, it can be enabled by the user
independently of the other features as long as CONFIG_CAN_NETLINK is
selected.
CONFIG_CAN_RX_OFFLOAD is also on the left side of an arrow starting
from CONFIG_CAN_NETLINK. Furthermore, there is an arrow starting from
CONFIG_CAN_RX_OFFLOAD going to the "rx offload drivers". So those
drivers need CONFIG_CAN_RX_OFFLOAD (which is implemented using the
"selects CONFIG_CAN_RX_OFFLOAD"). However, CONFIG_CAN_RX_OFFLOAD can
be selected independently of the "rx offload drivers" as long as its
CONFIG_CAN_NETLINK dependency is met.

So I think that the diagram is correct. Maybe rephrasing the cover
letter as below would address your concerns?

| Below diagrams illustrate the changes made. The arrow symbol "X --> Y"
| denotes that "Y needs X". Most of the time, it is implemented using "if X"
| and "endif" predicates in X’s Kbuild to encapsulate Y so that Y
| does not show up unless X is selected. The exception is rx
| offload which is implemented by adding a "selects
| CONFIG_CAN_RX_OFFLOAD" tag in Y’s Kbuild.

> > |
> > +-> CAN devices relying on rx offload
> > (at time of writing: flexcan, m_can, mcp251xfd and ti_hecc)

Yours sincerely,
Vincent Mailhol

2022-06-07 11:11:57

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

On Tue. 7 Jun. 2022 at 16:13, Marc Kleine-Budde <[email protected]> wrote:
> On 07.06.2022 11:49:30, Vincent MAILHOL wrote:
> [...]
> > So I think that the diagram is correct. Maybe rephrasing the cover
> > letter as below would address your concerns?
>
> BTW: I got the OK from Jakub to send PR with merges.
>
> If you think the cover letter needs rephrasing, send a new series and
> I'm going to force push that over can-next/master. After that let's
> consider can-next/master as fast-forward only.

I will first wait for Oliver’s feedback. Once we are aligned, I can do
the v6 and I really hope that would be the last one.

2022-06-08 06:21:40

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

Hi Vincent,

On 07.06.22 04:49, Vincent MAILHOL wrote:
> On Tue. 7 Jun. 2022 at 04:43, Oliver Hartkopp <[email protected]> wrote:
>>

>>> |
>>> +-> All other CAN devices not relying on RX offload
>>> |
>>> +-> CAN rx offload
>>> symbol: CONFIG_CAN_RX_OFFLOAD
>>
>> Is this still true in patch series 5?
>>
>> If I understood it correctly CONFIG_CAN_BITTIMING and
>> CONFIG_CAN_RX_OFFLOAD can be enabled by the user and
>> (alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd
>> and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.
>>
>> Right?
>
> Yes, this is correct. Maybe what troubles you is the meaning of the
> "x --> y" arrow in the graph. I said it denotes that "y depends on x".
> Here "depends on" has a loose meaning. It translates to either:
> * Feature Y is encapsulated in Kbuild by some "if X/endif" and won't
> show up unless X is selected.
> * Feature Y has a "selects X" tag and will forcibly enable X if selected.
>
> CONFIG_CAN_*CALC*_BITTIMING is on the left side of an arrow starting
> from CONFIG_CAN_NETLINK so it "depends" on CONFIG_CAN_NETLINK. On the
> other hand, CONFIG_CAN_*CALC*_BITTIMING does not have any arrow
> starting from it so indeed, it can be enabled by the user
> independently of the other features as long as CONFIG_CAN_NETLINK is
> selected.

Ok.

> CONFIG_CAN_RX_OFFLOAD is also on the left side of an arrow starting
> from CONFIG_CAN_NETLINK. Furthermore, there is an arrow starting from
> CONFIG_CAN_RX_OFFLOAD going to the "rx offload drivers". So those
> drivers need CONFIG_CAN_RX_OFFLOAD (which is implemented using the
> "selects CONFIG_CAN_RX_OFFLOAD"). However, CONFIG_CAN_RX_OFFLOAD can
> be selected independently of the "rx offload drivers" as long as its
> CONFIG_CAN_NETLINK dependency is met.
>
> So I think that the diagram is correct. Maybe rephrasing the cover
> letter as below would address your concerns?
I applied your series and played with the options and it works like
charm - and as expected.

But the point remains that from your figure I would still assume that
the M_CAN driver would only show up when CONFIG_CAN_RX_OFFLOAD was
selected by the user.

But the current (good) implementation shows *all* drivers and selects
CONFIG_CAN_RX_OFFLOAD when e.g. M_CAN is selected.

So what about:

symbol: CONFIG_NETDEVICES
|
+-> CAN Device Drivers
symbol: CONFIG_CAN_DEV
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> hardware CAN device drivers with Netlink support
symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
|
+-> CAN bit-timing calculation (optional for all drivers)
| symbol: CONFIG_CAN_BITTIMING
|
+-> CAN rx offload (optional but selected by some drivers)
| symbol: CONFIG_CAN_RX_OFFLOAD
|
+-> CAN devices drivers
(some may select CONFIG_CAN_RX_OFFLOAD)

(I also added 'hardware' to CAN device drivers with Netlink support) to
have a distinction to 'software/virtual' CAN device drivers)

At least this would help me to understand the new configuration setup.

Best regards,
Oliver

2022-06-08 06:22:11

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
> So what about:
>
> symbol: CONFIG_NETDEVICES
> |
> +-> CAN Device Drivers
> symbol: CONFIG_CAN_DEV
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> hardware CAN device drivers with Netlink support
> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> |
> +-> CAN bit-timing calculation (optional for all drivers)
> | symbol: CONFIG_CAN_BITTIMING
> |
> +-> CAN rx offload (optional but selected by some drivers)
> | symbol: CONFIG_CAN_RX_OFFLOAD
> |
> +-> CAN devices drivers
> (some may select CONFIG_CAN_RX_OFFLOAD)
>
> (I also added 'hardware' to CAN device drivers with Netlink support) to have
> a distinction to 'software/virtual' CAN device drivers)

The line between hardware and software/virtual devices ist blurry, the
new can327 driver uses netlink and the slcan is currently being
converted....

> At least this would help me to understand the new configuration setup.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (1.42 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-08 06:23:20

by Vincent Mailhol

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild

On Wed. 8 Jun 2022 at 05:51, Oliver Hartkopp <[email protected]> wrote:
> On 07.06.22 22:27, Marc Kleine-Budde wrote:
> > On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
> >> So what about:
> >>
> >> symbol: CONFIG_NETDEVICES
> >> |
> >> +-> CAN Device Drivers
> >> symbol: CONFIG_CAN_DEV
> >> |
> >> +-> software/virtual CAN device drivers
> >> | (at time of writing: slcan, vcan, vxcan)
> >> |
> >> +-> hardware CAN device drivers with Netlink support
> >> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> >> |
> >> +-> CAN bit-timing calculation (optional for all drivers)
> >> | symbol: CONFIG_CAN_BITTIMING
> >> |
> >> +-> CAN rx offload (optional but selected by some drivers)
> >> | symbol: CONFIG_CAN_RX_OFFLOAD
> >> |
> >> +-> CAN devices drivers
> >> (some may select CONFIG_CAN_RX_OFFLOAD)

OK, this does not follow the definition I set for the "x --> y" arrow,
but it is easy to read. I am OK with your suggestion. I will also
remove the definition of the "x --> y" arrow because your diagram is
self explanatory.

> >> (I also added 'hardware' to CAN device drivers with Netlink support) to have
> >> a distinction to 'software/virtual' CAN device drivers)

This line you modified is the verbatim copy of the title in
menuconfig. So you are suggesting adding "hardware" to the menuconfig
as well? It did not have this word in the title before this series.
I was hesitating on this. If we name the symbol CAN_NETLINK, then I do
not see the need to also add "hardware" in the title. If you look at
the help menu, you will see: "This is required by all platform and
hardware CAN drivers." Mentioning it in the help menu is enough for
me.

And because of the blur line between slcan (c.f. Marc's comment
below), I am not convinced to add this.

> > The line between hardware and software/virtual devices ist blurry, the
> > new can327 driver uses netlink and the slcan is currently being
> > converted....
>
> Right, which could mean that slcan and can327 should be located in the
> 'usual' CAN device driver section and not in the sw/virtual device section.

ACK, but as discussed with Marc, I will just focus on the series
itself and ignore (for the moment) that slcan will probably be moved
within CAN_NETLINK scope in the future.
https://lore.kernel.org/linux-can/[email protected]/

> The slcan and can327 need some kind of hardware - while vcan and vxcan
> don't.


Yours sincerely,
Vincent Mailhol

2022-06-08 06:37:17

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild



On 07.06.22 22:27, Marc Kleine-Budde wrote:
> On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
>> So what about:
>>
>> symbol: CONFIG_NETDEVICES
>> |
>> +-> CAN Device Drivers
>> symbol: CONFIG_CAN_DEV
>> |
>> +-> software/virtual CAN device drivers
>> | (at time of writing: slcan, vcan, vxcan)
>> |
>> +-> hardware CAN device drivers with Netlink support
>> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
>> |
>> +-> CAN bit-timing calculation (optional for all drivers)
>> | symbol: CONFIG_CAN_BITTIMING
>> |
>> +-> CAN rx offload (optional but selected by some drivers)
>> | symbol: CONFIG_CAN_RX_OFFLOAD
>> |
>> +-> CAN devices drivers
>> (some may select CONFIG_CAN_RX_OFFLOAD)
>>
>> (I also added 'hardware' to CAN device drivers with Netlink support) to have
>> a distinction to 'software/virtual' CAN device drivers)
>
> The line between hardware and software/virtual devices ist blurry, the
> new can327 driver uses netlink and the slcan is currently being
> converted....

Right, which could mean that slcan and can327 should be located in the
'usual' CAN device driver section and not in the sw/virtual device section.

The slcan and can327 need some kind of hardware - while vcan and vxcan
don't.

Best regards,
Oliver

2022-06-08 20:38:25

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v5 0/7] can: refactoring of can-dev module and of Kbuild



On 08.06.22 01:59, Vincent MAILHOL wrote:
> On Wed. 8 Jun 2022 at 05:51, Oliver Hartkopp <[email protected]> wrote:

>>>> (I also added 'hardware' to CAN device drivers with Netlink support) to have
>>>> a distinction to 'software/virtual' CAN device drivers)
>
> This line you modified is the verbatim copy of the title in
> menuconfig. So you are suggesting adding "hardware" to the menuconfig
> as well? It did not have this word in the title before this series.
> I was hesitating on this. If we name the symbol CAN_NETLINK, then I do
> not see the need to also add "hardware" in the title. If you look at
> the help menu, you will see: "This is required by all platform and
> hardware CAN drivers." Mentioning it in the help menu is enough for
> me.
>
> And because of the blur line between slcan (c.f. Marc's comment
> below), I am not convinced to add this.

Yes, discussing about this change I'm not convinced either ;-)

>>> The line between hardware and software/virtual devices ist blurry, the
>>> new can327 driver uses netlink and the slcan is currently being
>>> converted....
>>
>> Right, which could mean that slcan and can327 should be located in the
>> 'usual' CAN device driver section and not in the sw/virtual device section.
>
> ACK, but as discussed with Marc, I will just focus on the series
> itself and ignore (for the moment) that slcan will probably be moved
> within CAN_NETLINK scope in the future.
> https://lore.kernel.org/linux-can/[email protected]/

Ok.

Sorry for the noise!

Best regards,
Oliver

2022-06-10 14:46:57

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild

Aside of calc_bittiming.o which can be configured with
CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
unconditionally to can-dev.o even if not needed by the user.

This series first goal it to split the can-dev modules so that the
only the needed features get built in during compilation.
Additionally, the CAN Device Drivers menu is moved from the
"Networking support" category to the "Device Drivers" category (where
all drivers are supposed to be).


* menu before this series *

CAN bus subsystem support
symbol: CONFIG_CAN
|
+-> CAN Device Drivers
(no symbol)
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> Platform CAN drivers with Netlink support
symbol: CONFIG_CAN_DEV
|
+-> CAN bit-timing calculation (optional for hardware drivers)
| symbol: CONFIG_CAN_CALC_BITTIMING
|
+-> All other CAN devices drivers

* menu after this series *

Network device support
symbol: CONFIG_NETDEVICES
|
+-> CAN Device Drivers
symbol: CONFIG_CAN_DEV
|
+-> software/virtual CAN device drivers
| (at time of writing: slcan, vcan, vxcan)
|
+-> CAN device drivers with Netlink support
symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
|
+-> CAN bit-timing calculation (optional for all drivers)
| symbol: CONFIG_CAN_CALC_BITTIMING
|
+-> All other CAN devices drivers
(some may select CONFIG_CAN_RX_OFFLOAD)
|
+-> CAN rx offload (automatically selected by some drivers)
(hidden symbol: CONFIG_CAN_RX_OFFLOAD)

Patches 1 to 5 of this series do above modification.

The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
can_dropped_invalid_skb() to discard tx skb (such skb can potentially
reach the driver if injected via the packet socket). In more details,
patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
7 is the actual change.

Those last two patches are actually connected to the first five ones:
because slcan and v(x)can requires can_dropped_invalid_skb(), it was
necessary to add those three devices to the scope of can-dev before
moving the function to skb.o.

This design results from the lengthy discussion in [1].

[1] https://lore.kernel.org/linux-can/[email protected]/


** Changelog **

v5 -> v6:

* fix typo in patch #1's title: Kbuild -> Kconfig.

* make CONFIG_RX_CAN an hidden config symbol and modify the diagram
in the cover letter accordingly.

@Oliver, with CONFIG_CAN_RX_OFFLOAD now being an hidden config,
that option fully depends on the drivers. So contrary to your
suggestion, I put CONFIG_CAN_RX_OFFLOAD below the device drivers
in the diagram.

* fix typo in cover letter: CONFIG_CAN_BITTIMING -> CONFIG_CAN_CALC_BITTIMING.

v4 -> v5:

* m_can is also requires RX offload. Add the "select CAN_RX_OFFLOAD"
to its Makefile.

* Reorder the lines of drivers/net/can/dev/Makefile.

* Remove duplicated rx-offload.o target in drivers/net/can/dev/Makefile

* Remove the Nota Bene in the cover letter.


v3 -> v4:

* Five additional patches added to split can-dev module and refactor
Kbuild. c.f. below (lengthy) thread:
https://lore.kernel.org/linux-can/[email protected]/


v2 -> v3:

* Apply can_dropped_invalid_skb() to slcan.

* Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
modifying Kbuild.

* fix small typos.

v1 -> v2:

* move can_dropped_invalid_skb() to skb.c instead of dev.h

* also move can_skb_headroom_valid() to skb.c

Vincent Mailhol (7):
can: Kconfig: rename config symbol CAN_DEV into CAN_NETLINK
can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
CAN_DEV
can: bittiming: move bittiming calculation functions to
calc_bittiming.c
can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
net: Kconfig: move the CAN device menu to the "Device Drivers" section
can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
to skb.c
can: skb: drop tx skb if in listen only mode

drivers/net/Kconfig | 2 +
drivers/net/can/Kconfig | 55 +++++--
drivers/net/can/dev/Makefile | 17 ++-
drivers/net/can/dev/bittiming.c | 197 -------------------------
drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
drivers/net/can/dev/dev.c | 9 +-
drivers/net/can/dev/skb.c | 72 +++++++++
drivers/net/can/m_can/Kconfig | 1 +
drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
include/linux/can/skb.h | 59 +-------
net/can/Kconfig | 5 +-
11 files changed, 338 insertions(+), 282 deletions(-)
create mode 100644 drivers/net/can/dev/calc_bittiming.c

--
2.35.1

2022-06-10 14:48:59

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v6 3/7] can: bittiming: move bittiming calculation functions to calc_bittiming.c

The canonical way to select or deselect an object during compilation
is to use this pattern in the relevant Makefile:

bar-$(CONFIG_FOO) := foo.o

bittiming.c instead uses some #ifdef CONFIG_CAN_CALC_BITTIMG.

Create a new file named calc_bittiming.c with all the functions which
are conditionally compiled with CONFIG_CAN_CALC_BITTIMG and modify the
Makefile according to above pattern.

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/Kconfig | 4 +
drivers/net/can/dev/Makefile | 1 +
drivers/net/can/dev/bittiming.c | 197 --------------------------
drivers/net/can/dev/calc_bittiming.c | 202 +++++++++++++++++++++++++++
4 files changed, 207 insertions(+), 197 deletions(-)
create mode 100644 drivers/net/can/dev/calc_bittiming.c

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 3c692af16676..87470feae6b1 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -96,6 +96,10 @@ config CAN_CALC_BITTIMING
source clock frequencies. Disabling saves some space, but then the
bit-timing parameters must be specified directly using the Netlink
arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
+
+ The additional features selected by this option will be added to the
+ can-dev module.
+
If unsure, say Y.

config CAN_AT91
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index 1baaf7020f7c..791e6b297ea3 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o

can-dev-y += skb.o

+can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o
can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o
can-dev-$(CONFIG_CAN_NETLINK) += dev.o
can-dev-$(CONFIG_CAN_NETLINK) += length.o
diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index c1e76f0a5064..7ae80763c960 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -4,205 +4,8 @@
* Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
*/

-#include <linux/units.h>
#include <linux/can/dev.h>

-#ifdef CONFIG_CAN_CALC_BITTIMING
-#define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
-
-/* Bit-timing calculation derived from:
- *
- * Code based on LinCAN sources and H8S2638 project
- * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
- * Copyright 2005 Stanislav Marek
- * email: [email protected]
- *
- * Calculates proper bit-timing parameters for a specified bit-rate
- * and sample-point, which can then be used to set the bit-timing
- * registers of the CAN controller. You can find more information
- * in the header file linux/can/netlink.h.
- */
-static int
-can_update_sample_point(const struct can_bittiming_const *btc,
- const unsigned int sample_point_nominal, const unsigned int tseg,
- unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
- unsigned int *sample_point_error_ptr)
-{
- unsigned int sample_point_error, best_sample_point_error = UINT_MAX;
- unsigned int sample_point, best_sample_point = 0;
- unsigned int tseg1, tseg2;
- int i;
-
- for (i = 0; i <= 1; i++) {
- tseg2 = tseg + CAN_SYNC_SEG -
- (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
- 1000 - i;
- tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
- tseg1 = tseg - tseg2;
- if (tseg1 > btc->tseg1_max) {
- tseg1 = btc->tseg1_max;
- tseg2 = tseg - tseg1;
- }
-
- sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
- (tseg + CAN_SYNC_SEG);
- sample_point_error = abs(sample_point_nominal - sample_point);
-
- if (sample_point <= sample_point_nominal &&
- sample_point_error < best_sample_point_error) {
- best_sample_point = sample_point;
- best_sample_point_error = sample_point_error;
- *tseg1_ptr = tseg1;
- *tseg2_ptr = tseg2;
- }
- }
-
- if (sample_point_error_ptr)
- *sample_point_error_ptr = best_sample_point_error;
-
- return best_sample_point;
-}
-
-int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
- const struct can_bittiming_const *btc)
-{
- struct can_priv *priv = netdev_priv(dev);
- unsigned int bitrate; /* current bitrate */
- unsigned int bitrate_error; /* difference between current and nominal value */
- unsigned int best_bitrate_error = UINT_MAX;
- unsigned int sample_point_error; /* difference between current and nominal value */
- unsigned int best_sample_point_error = UINT_MAX;
- unsigned int sample_point_nominal; /* nominal sample point */
- unsigned int best_tseg = 0; /* current best value for tseg */
- unsigned int best_brp = 0; /* current best value for brp */
- unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
- u64 v64;
-
- /* Use CiA recommended sample points */
- if (bt->sample_point) {
- sample_point_nominal = bt->sample_point;
- } else {
- if (bt->bitrate > 800 * KILO /* BPS */)
- sample_point_nominal = 750;
- else if (bt->bitrate > 500 * KILO /* BPS */)
- sample_point_nominal = 800;
- else
- sample_point_nominal = 875;
- }
-
- /* tseg even = round down, odd = round up */
- for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
- tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
- tsegall = CAN_SYNC_SEG + tseg / 2;
-
- /* Compute all possible tseg choices (tseg=tseg1+tseg2) */
- brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
-
- /* choose brp step which is possible in system */
- brp = (brp / btc->brp_inc) * btc->brp_inc;
- if (brp < btc->brp_min || brp > btc->brp_max)
- continue;
-
- bitrate = priv->clock.freq / (brp * tsegall);
- bitrate_error = abs(bt->bitrate - bitrate);
-
- /* tseg brp biterror */
- if (bitrate_error > best_bitrate_error)
- continue;
-
- /* reset sample point error if we have a better bitrate */
- if (bitrate_error < best_bitrate_error)
- best_sample_point_error = UINT_MAX;
-
- can_update_sample_point(btc, sample_point_nominal, tseg / 2,
- &tseg1, &tseg2, &sample_point_error);
- if (sample_point_error >= best_sample_point_error)
- continue;
-
- best_sample_point_error = sample_point_error;
- best_bitrate_error = bitrate_error;
- best_tseg = tseg / 2;
- best_brp = brp;
-
- if (bitrate_error == 0 && sample_point_error == 0)
- break;
- }
-
- if (best_bitrate_error) {
- /* Error in one-tenth of a percent */
- v64 = (u64)best_bitrate_error * 1000;
- do_div(v64, bt->bitrate);
- bitrate_error = (u32)v64;
- if (bitrate_error > CAN_CALC_MAX_ERROR) {
- netdev_err(dev,
- "bitrate error %d.%d%% too high\n",
- bitrate_error / 10, bitrate_error % 10);
- return -EDOM;
- }
- netdev_warn(dev, "bitrate error %d.%d%%\n",
- bitrate_error / 10, bitrate_error % 10);
- }
-
- /* real sample point */
- bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
- best_tseg, &tseg1, &tseg2,
- NULL);
-
- v64 = (u64)best_brp * 1000 * 1000 * 1000;
- do_div(v64, priv->clock.freq);
- bt->tq = (u32)v64;
- bt->prop_seg = tseg1 / 2;
- bt->phase_seg1 = tseg1 - bt->prop_seg;
- bt->phase_seg2 = tseg2;
-
- /* check for sjw user settings */
- if (!bt->sjw || !btc->sjw_max) {
- bt->sjw = 1;
- } else {
- /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
- if (bt->sjw > btc->sjw_max)
- bt->sjw = btc->sjw_max;
- /* bt->sjw must not be higher than tseg2 */
- if (tseg2 < bt->sjw)
- bt->sjw = tseg2;
- }
-
- bt->brp = best_brp;
-
- /* real bitrate */
- bt->bitrate = priv->clock.freq /
- (bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
-
- return 0;
-}
-
-void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
- const struct can_bittiming *dbt,
- u32 *ctrlmode, u32 ctrlmode_supported)
-
-{
- if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
- return;
-
- *ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
-
- /* As specified in ISO 11898-1 section 11.3.3 "Transmitter
- * delay compensation" (TDC) is only applicable if data BRP is
- * one or two.
- */
- if (dbt->brp == 1 || dbt->brp == 2) {
- /* Sample point in clock periods */
- u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
- dbt->phase_seg1) * dbt->brp;
-
- if (sample_point_in_tc < tdc_const->tdco_min)
- return;
- tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
- *ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
- }
-}
-#endif /* CONFIG_CAN_CALC_BITTIMING */
-
/* Checks the validity of the specified bit-timing parameters prop_seg,
* phase_seg1, phase_seg2 and sjw and tries to determine the bitrate
* prescaler value brp. You can find more information in the header
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
new file mode 100644
index 000000000000..d3caa040614d
--- /dev/null
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2005 Marc Kleine-Budde, Pengutronix
+ * Copyright (C) 2006 Andrey Volkov, Varma Electronics
+ * Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
+ */
+
+#include <linux/units.h>
+#include <linux/can/dev.h>
+
+#define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
+
+/* Bit-timing calculation derived from:
+ *
+ * Code based on LinCAN sources and H8S2638 project
+ * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
+ * Copyright 2005 Stanislav Marek
+ * email: [email protected]
+ *
+ * Calculates proper bit-timing parameters for a specified bit-rate
+ * and sample-point, which can then be used to set the bit-timing
+ * registers of the CAN controller. You can find more information
+ * in the header file linux/can/netlink.h.
+ */
+static int
+can_update_sample_point(const struct can_bittiming_const *btc,
+ const unsigned int sample_point_nominal, const unsigned int tseg,
+ unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
+ unsigned int *sample_point_error_ptr)
+{
+ unsigned int sample_point_error, best_sample_point_error = UINT_MAX;
+ unsigned int sample_point, best_sample_point = 0;
+ unsigned int tseg1, tseg2;
+ int i;
+
+ for (i = 0; i <= 1; i++) {
+ tseg2 = tseg + CAN_SYNC_SEG -
+ (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
+ 1000 - i;
+ tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
+ tseg1 = tseg - tseg2;
+ if (tseg1 > btc->tseg1_max) {
+ tseg1 = btc->tseg1_max;
+ tseg2 = tseg - tseg1;
+ }
+
+ sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
+ (tseg + CAN_SYNC_SEG);
+ sample_point_error = abs(sample_point_nominal - sample_point);
+
+ if (sample_point <= sample_point_nominal &&
+ sample_point_error < best_sample_point_error) {
+ best_sample_point = sample_point;
+ best_sample_point_error = sample_point_error;
+ *tseg1_ptr = tseg1;
+ *tseg2_ptr = tseg2;
+ }
+ }
+
+ if (sample_point_error_ptr)
+ *sample_point_error_ptr = best_sample_point_error;
+
+ return best_sample_point;
+}
+
+int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
+{
+ struct can_priv *priv = netdev_priv(dev);
+ unsigned int bitrate; /* current bitrate */
+ unsigned int bitrate_error; /* difference between current and nominal value */
+ unsigned int best_bitrate_error = UINT_MAX;
+ unsigned int sample_point_error; /* difference between current and nominal value */
+ unsigned int best_sample_point_error = UINT_MAX;
+ unsigned int sample_point_nominal; /* nominal sample point */
+ unsigned int best_tseg = 0; /* current best value for tseg */
+ unsigned int best_brp = 0; /* current best value for brp */
+ unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
+ u64 v64;
+
+ /* Use CiA recommended sample points */
+ if (bt->sample_point) {
+ sample_point_nominal = bt->sample_point;
+ } else {
+ if (bt->bitrate > 800 * KILO /* BPS */)
+ sample_point_nominal = 750;
+ else if (bt->bitrate > 500 * KILO /* BPS */)
+ sample_point_nominal = 800;
+ else
+ sample_point_nominal = 875;
+ }
+
+ /* tseg even = round down, odd = round up */
+ for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
+ tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
+ tsegall = CAN_SYNC_SEG + tseg / 2;
+
+ /* Compute all possible tseg choices (tseg=tseg1+tseg2) */
+ brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
+
+ /* choose brp step which is possible in system */
+ brp = (brp / btc->brp_inc) * btc->brp_inc;
+ if (brp < btc->brp_min || brp > btc->brp_max)
+ continue;
+
+ bitrate = priv->clock.freq / (brp * tsegall);
+ bitrate_error = abs(bt->bitrate - bitrate);
+
+ /* tseg brp biterror */
+ if (bitrate_error > best_bitrate_error)
+ continue;
+
+ /* reset sample point error if we have a better bitrate */
+ if (bitrate_error < best_bitrate_error)
+ best_sample_point_error = UINT_MAX;
+
+ can_update_sample_point(btc, sample_point_nominal, tseg / 2,
+ &tseg1, &tseg2, &sample_point_error);
+ if (sample_point_error >= best_sample_point_error)
+ continue;
+
+ best_sample_point_error = sample_point_error;
+ best_bitrate_error = bitrate_error;
+ best_tseg = tseg / 2;
+ best_brp = brp;
+
+ if (bitrate_error == 0 && sample_point_error == 0)
+ break;
+ }
+
+ if (best_bitrate_error) {
+ /* Error in one-tenth of a percent */
+ v64 = (u64)best_bitrate_error * 1000;
+ do_div(v64, bt->bitrate);
+ bitrate_error = (u32)v64;
+ if (bitrate_error > CAN_CALC_MAX_ERROR) {
+ netdev_err(dev,
+ "bitrate error %d.%d%% too high\n",
+ bitrate_error / 10, bitrate_error % 10);
+ return -EDOM;
+ }
+ netdev_warn(dev, "bitrate error %d.%d%%\n",
+ bitrate_error / 10, bitrate_error % 10);
+ }
+
+ /* real sample point */
+ bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
+ best_tseg, &tseg1, &tseg2,
+ NULL);
+
+ v64 = (u64)best_brp * 1000 * 1000 * 1000;
+ do_div(v64, priv->clock.freq);
+ bt->tq = (u32)v64;
+ bt->prop_seg = tseg1 / 2;
+ bt->phase_seg1 = tseg1 - bt->prop_seg;
+ bt->phase_seg2 = tseg2;
+
+ /* check for sjw user settings */
+ if (!bt->sjw || !btc->sjw_max) {
+ bt->sjw = 1;
+ } else {
+ /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
+ if (bt->sjw > btc->sjw_max)
+ bt->sjw = btc->sjw_max;
+ /* bt->sjw must not be higher than tseg2 */
+ if (tseg2 < bt->sjw)
+ bt->sjw = tseg2;
+ }
+
+ bt->brp = best_brp;
+
+ /* real bitrate */
+ bt->bitrate = priv->clock.freq /
+ (bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
+
+ return 0;
+}
+
+void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
+ const struct can_bittiming *dbt,
+ u32 *ctrlmode, u32 ctrlmode_supported)
+
+{
+ if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
+ return;
+
+ *ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+
+ /* As specified in ISO 11898-1 section 11.3.3 "Transmitter
+ * delay compensation" (TDC) is only applicable if data BRP is
+ * one or two.
+ */
+ if (dbt->brp == 1 || dbt->brp == 2) {
+ /* Sample point in clock periods */
+ u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
+ dbt->phase_seg1) * dbt->brp;
+
+ if (sample_point_in_tc < tdc_const->tdco_min)
+ return;
+ tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
+ *ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
+ }
+}
--
2.35.1

2022-06-10 14:52:07

by Vincent Mailhol

[permalink] [raw]
Subject: [PATCH v6 2/7] can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using CAN_DEV

In the next patches, the software/virtual drivers (slcan, v(x)can)
will depend on drivers/net/can/dev/skb.o.

This patch changes the scope of the can-dev module to include the
above mentioned drivers.

To do so, we reuse the menu "CAN Device Drivers" and turn it into a
configmenu using the config symbol CAN_DEV (which we released in
previous patch). Also, add a description to this new CAN_DEV
menuconfig.

The symbol CAN_DEV now only triggers the build of skb.o. For this
reasons, all the macros from linux/module.h are deported from
drivers/net/can/dev/dev.c to drivers/net/can/dev/skb.c.

Finally, drivers/net/can/dev/Makefile is adjusted accordingly.

Suggested-by: Oliver Hartkopp <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/Kconfig | 29 ++++++++++++++++++++++++++---
drivers/net/can/dev/Makefile | 16 +++++++++-------
drivers/net/can/dev/dev.c | 9 +--------
drivers/net/can/dev/skb.c | 7 +++++++
4 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 99f189ad35ad..3c692af16676 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -1,5 +1,25 @@
# SPDX-License-Identifier: GPL-2.0-only
-menu "CAN Device Drivers"
+
+menuconfig CAN_DEV
+ tristate "CAN Device Drivers"
+ default y
+ help
+ Controller Area Network (CAN) is serial communications protocol up to
+ 1Mbit/s for its original release (now known as Classical CAN) and up
+ to 8Mbit/s for the more recent CAN with Flexible Data-Rate
+ (CAN-FD). The CAN bus was originally mainly for automotive, but is now
+ widely used in marine (NMEA2000), industrial, and medical
+ applications. More information on the CAN network protocol family
+ PF_CAN is contained in <Documentation/networking/can.rst>.
+
+ This section contains all the CAN(-FD) device drivers including the
+ virtual ones. If you own such devices or plan to use the virtual CAN
+ interfaces to develop applications, say Y here.
+
+ To compile as a module, choose M here: the module will be called
+ can-dev.
+
+if CAN_DEV

config CAN_VCAN
tristate "Virtual Local CAN Interface (vcan)"
@@ -49,7 +69,7 @@ config CAN_SLCAN
also be built as a module. If so, the module will be called slcan.

config CAN_NETLINK
- tristate "CAN device drivers with Netlink support"
+ bool "CAN device drivers with Netlink support"
default y
help
Enables the common framework for CAN device drivers. This is the
@@ -57,6 +77,9 @@ config CAN_NETLINK
as bittiming validation, support of CAN error states, device restart
and others.

+ The additional features selected by this option will be added to the
+ can-dev module.
+
This is required by all platform and hardware CAN drivers. If you
plan to use such devices or if unsure, say Y.

@@ -178,4 +201,4 @@ config CAN_DEBUG_DEVICES
a problem with CAN support and want to see more of what is going
on.

-endmenu
+endif #CAN_DEV
diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile
index 5b4c813c6222..1baaf7020f7c 100644
--- a/drivers/net/can/dev/Makefile
+++ b/drivers/net/can/dev/Makefile
@@ -1,9 +1,11 @@
# SPDX-License-Identifier: GPL-2.0

-obj-$(CONFIG_CAN_NETLINK) += can-dev.o
-can-dev-y += bittiming.o
-can-dev-y += dev.o
-can-dev-y += length.o
-can-dev-y += netlink.o
-can-dev-y += rx-offload.o
-can-dev-y += skb.o
+obj-$(CONFIG_CAN_DEV) += can-dev.o
+
+can-dev-y += skb.o
+
+can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o
+can-dev-$(CONFIG_CAN_NETLINK) += dev.o
+can-dev-$(CONFIG_CAN_NETLINK) += length.o
+can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
+can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 96c9d9db00cf..523eaacfe29e 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -4,7 +4,6 @@
* Copyright (C) 2008-2009 Wolfgang Grandegger <[email protected]>
*/

-#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/netdevice.h>
@@ -17,12 +16,6 @@
#include <linux/gpio/consumer.h>
#include <linux/of.h>

-#define MOD_DESC "CAN device driver interface"
-
-MODULE_DESCRIPTION(MOD_DESC);
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");
-
static void can_update_state_error_stats(struct net_device *dev,
enum can_state new_state)
{
@@ -513,7 +506,7 @@ static __init int can_dev_init(void)

err = can_netlink_register();
if (!err)
- pr_info(MOD_DESC "\n");
+ pr_info("CAN device driver interface\n");

return err;
}
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 61660248c69e..a4208f125b76 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -5,6 +5,13 @@
*/

#include <linux/can/dev.h>
+#include <linux/module.h>
+
+#define MOD_DESC "CAN device driver interface"
+
+MODULE_DESCRIPTION(MOD_DESC);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Wolfgang Grandegger <[email protected]>");

/* Local echo of CAN messages
*
--
2.35.1

2022-06-10 22:51:39

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild



On 10.06.22 16:30, Vincent Mailhol wrote:
> Aside of calc_bittiming.o which can be configured with
> CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
> unconditionally to can-dev.o even if not needed by the user.
>
> This series first goal it to split the can-dev modules so that the
> only the needed features get built in during compilation.
> Additionally, the CAN Device Drivers menu is moved from the
> "Networking support" category to the "Device Drivers" category (where
> all drivers are supposed to be).
>
>
> * menu before this series *
>
> CAN bus subsystem support
> symbol: CONFIG_CAN
> |
> +-> CAN Device Drivers
> (no symbol)
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> Platform CAN drivers with Netlink support
> symbol: CONFIG_CAN_DEV
> |
> +-> CAN bit-timing calculation (optional for hardware drivers)
> | symbol: CONFIG_CAN_CALC_BITTIMING
> |
> +-> All other CAN devices drivers
>
> * menu after this series *
>
> Network device support
> symbol: CONFIG_NETDEVICES
> |
> +-> CAN Device Drivers
> symbol: CONFIG_CAN_DEV
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> CAN device drivers with Netlink support
> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> |
> +-> CAN bit-timing calculation (optional for all drivers)
> | symbol: CONFIG_CAN_CALC_BITTIMING
> |
> +-> All other CAN devices drivers
> (some may select CONFIG_CAN_RX_OFFLOAD)
> |
> +-> CAN rx offload (automatically selected by some drivers)
> (hidden symbol: CONFIG_CAN_RX_OFFLOAD)
>
> Patches 1 to 5 of this series do above modification.
>
> The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
> can_dropped_invalid_skb() to discard tx skb (such skb can potentially
> reach the driver if injected via the packet socket). In more details,
> patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
> 7 is the actual change.
>
> Those last two patches are actually connected to the first five ones:
> because slcan and v(x)can requires can_dropped_invalid_skb(), it was
> necessary to add those three devices to the scope of can-dev before
> moving the function to skb.o.
>
> This design results from the lengthy discussion in [1].
>
> [1] https://lore.kernel.org/linux-can/[email protected]/
>
>
> ** Changelog **
>
> v5 -> v6:
>
> * fix typo in patch #1's title: Kbuild -> Kconfig.
>
> * make CONFIG_RX_CAN an hidden config symbol and modify the diagram
> in the cover letter accordingly.
>
> @Oliver, with CONFIG_CAN_RX_OFFLOAD now being an hidden config,
> that option fully depends on the drivers. So contrary to your
> suggestion, I put CONFIG_CAN_RX_OFFLOAD below the device drivers
> in the diagram.

Yes, fine for me.

I did some Kconfig configuration tests and built the drivers to see
vcan.ko depending on can-dev.ko at module loading the first time ;-)

Nice work.

So for these bits I can provide a

Tested-by: Oliver Hartkopp <[email protected]>

Thanks,
Oliver

>
> * fix typo in cover letter: CONFIG_CAN_BITTIMING -> CONFIG_CAN_CALC_BITTIMING.
>
> v4 -> v5:
>
> * m_can is also requires RX offload. Add the "select CAN_RX_OFFLOAD"
> to its Makefile.
>
> * Reorder the lines of drivers/net/can/dev/Makefile.
>
> * Remove duplicated rx-offload.o target in drivers/net/can/dev/Makefile
>
> * Remove the Nota Bene in the cover letter.
>
>
> v3 -> v4:
>
> * Five additional patches added to split can-dev module and refactor
> Kbuild. c.f. below (lengthy) thread:
> https://lore.kernel.org/linux-can/[email protected]/
>
>
> v2 -> v3:
>
> * Apply can_dropped_invalid_skb() to slcan.
>
> * Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
> modifying Kbuild.
>
> * fix small typos.
>
> v1 -> v2:
>
> * move can_dropped_invalid_skb() to skb.c instead of dev.h
>
> * also move can_skb_headroom_valid() to skb.c
>
> Vincent Mailhol (7):
> can: Kconfig: rename config symbol CAN_DEV into CAN_NETLINK
> can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
> CAN_DEV
> can: bittiming: move bittiming calculation functions to
> calc_bittiming.c
> can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
> net: Kconfig: move the CAN device menu to the "Device Drivers" section
> can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
> to skb.c
> can: skb: drop tx skb if in listen only mode
>
> drivers/net/Kconfig | 2 +
> drivers/net/can/Kconfig | 55 +++++--
> drivers/net/can/dev/Makefile | 17 ++-
> drivers/net/can/dev/bittiming.c | 197 -------------------------
> drivers/net/can/dev/calc_bittiming.c | 202 ++++++++++++++++++++++++++
> drivers/net/can/dev/dev.c | 9 +-
> drivers/net/can/dev/skb.c | 72 +++++++++
> drivers/net/can/m_can/Kconfig | 1 +
> drivers/net/can/spi/mcp251xfd/Kconfig | 1 +
> include/linux/can/skb.h | 59 +-------
> net/can/Kconfig | 5 +-
> 11 files changed, 338 insertions(+), 282 deletions(-)
> create mode 100644 drivers/net/can/dev/calc_bittiming.c
>

2022-06-10 23:06:30

by Max Staudt

[permalink] [raw]
Subject: Re: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild

Looks good to me. Thanks Vincent for this clean-up, and thanks to
everyone involved for the discussion, reviews, and testing!

Acked-by: Max Staudt <[email protected]>

2022-06-11 15:55:48

by Marc Kleine-Budde

[permalink] [raw]
Subject: Re: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild

On 10.06.2022 23:30:02, Vincent Mailhol wrote:
> Aside of calc_bittiming.o which can be configured with
> CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
> unconditionally to can-dev.o even if not needed by the user.
>
> This series first goal it to split the can-dev modules so that the
> only the needed features get built in during compilation.
> Additionally, the CAN Device Drivers menu is moved from the
> "Networking support" category to the "Device Drivers" category (where
> all drivers are supposed to be).
>
>
> * menu before this series *
>
> CAN bus subsystem support
> symbol: CONFIG_CAN
> |
> +-> CAN Device Drivers
> (no symbol)
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> Platform CAN drivers with Netlink support
> symbol: CONFIG_CAN_DEV
> |
> +-> CAN bit-timing calculation (optional for hardware drivers)
> | symbol: CONFIG_CAN_CALC_BITTIMING
> |
> +-> All other CAN devices drivers
>
> * menu after this series *
>
> Network device support
> symbol: CONFIG_NETDEVICES
> |
> +-> CAN Device Drivers
> symbol: CONFIG_CAN_DEV
> |
> +-> software/virtual CAN device drivers
> | (at time of writing: slcan, vcan, vxcan)
> |
> +-> CAN device drivers with Netlink support
> symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> |
> +-> CAN bit-timing calculation (optional for all drivers)
> | symbol: CONFIG_CAN_CALC_BITTIMING
> |
> +-> All other CAN devices drivers
> (some may select CONFIG_CAN_RX_OFFLOAD)
> |
> +-> CAN rx offload (automatically selected by some drivers)
> (hidden symbol: CONFIG_CAN_RX_OFFLOAD)
>
> Patches 1 to 5 of this series do above modification.
>
> The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
> can_dropped_invalid_skb() to discard tx skb (such skb can potentially
> reach the driver if injected via the packet socket). In more details,
> patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
> 7 is the actual change.
>
> Those last two patches are actually connected to the first five ones:
> because slcan and v(x)can requires can_dropped_invalid_skb(), it was
> necessary to add those three devices to the scope of can-dev before
> moving the function to skb.o.
>
> This design results from the lengthy discussion in [1].
>
> [1] https://lore.kernel.org/linux-can/[email protected]/
>
>
> ** Changelog **
>
> v5 -> v6:
>
> * fix typo in patch #1's title: Kbuild -> Kconfig.
>
> * make CONFIG_RX_CAN an hidden config symbol and modify the diagram
> in the cover letter accordingly.
>
> @Oliver, with CONFIG_CAN_RX_OFFLOAD now being an hidden config,
> that option fully depends on the drivers. So contrary to your
> suggestion, I put CONFIG_CAN_RX_OFFLOAD below the device drivers
> in the diagram.
>
> * fix typo in cover letter: CONFIG_CAN_BITTIMING -> CONFIG_CAN_CALC_BITTIMING.
>
> v4 -> v5:
>
> * m_can is also requires RX offload. Add the "select CAN_RX_OFFLOAD"
> to its Makefile.
>
> * Reorder the lines of drivers/net/can/dev/Makefile.
>
> * Remove duplicated rx-offload.o target in drivers/net/can/dev/Makefile
>
> * Remove the Nota Bene in the cover letter.
>
>
> v3 -> v4:
>
> * Five additional patches added to split can-dev module and refactor
> Kbuild. c.f. below (lengthy) thread:
> https://lore.kernel.org/linux-can/[email protected]/
>
>
> v2 -> v3:
>
> * Apply can_dropped_invalid_skb() to slcan.
>
> * Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
> modifying Kbuild.
>
> * fix small typos.
>
> v1 -> v2:
>
> * move can_dropped_invalid_skb() to skb.c instead of dev.h
>
> * also move can_skb_headroom_valid() to skb.c

Applied to can-next/master....as a merge with the above message!
Congrats on this series and the first ever merge to the linux-can
branch!

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |


Attachments:
(No filename) (4.50 kB)
signature.asc (499.00 B)
Download all attachments