LinuxLists.cc - [PATCH 0/2] can: drop tx skb if the device is in listen only mode

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

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

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

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

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]

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

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