| Message ID | 20220514141650.1109542-4-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: can_dropped_invalid_skb() and Kbuild changes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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 <mailhol.vincent@wanadoo.fr> > --- > 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 */
Hi Oliver, On Mon. 16 May 2022 at 04:17, Oliver Hartkopp <socketcan@hartkopp.net> 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/CAMZ6RqLU-Wg0Cau3cM=QsU-t-7Lyzmo1nJ_VAA4Mbw3u0jnNtw@mail.gmail.com/ 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
On Tue, 17 May 2022 10:50:16 +0900 Vincent MAILHOL <mailhol.vincent@wanadoo.fr> 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
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
On Tue. 17 May 2022 at 15:08, Marc Kleine-Budde <mkl@pengutronix.de> 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.
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
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
On Tue, 17 May 2022 13:51:57 +0200 Oliver Hartkopp <socketcan@hartkopp.net> 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
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
On Tue, 17 May 2022 14:21:53 +0200 Marc Kleine-Budde <mkl@pengutronix.de> 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
On 5/17/22 14:39, Max Staudt wrote: > On Tue, 17 May 2022 14:21:53 +0200 > Marc Kleine-Budde <mkl@pengutronix.de> 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
On Tue, 17 May 2022 15:35:03 +0200 Oliver Hartkopp <socketcan@hartkopp.net> 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
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
On 17.05.22 15:43, Max Staudt wrote: > On Tue, 17 May 2022 15:35:03 +0200 > Oliver Hartkopp <socketcan@hartkopp.net> 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
On Tue, 17 May 2022 16:35:05 +0200 Oliver Hartkopp <socketcan@hartkopp.net> 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
On 17.05.22 17:38, Max Staudt wrote: > On Tue, 17 May 2022 16:35:05 +0200 > Oliver Hartkopp <socketcan@hartkopp.net> 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
On Tue, 17 May 2022 17:50:03 +0200 Oliver Hartkopp <socketcan@hartkopp.net> 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
I didn't think this would trigger such a passionate discussion! On Tue. 17 mai 2022 at 22:35, Oliver Hartkopp <socketcan@hartkopp.net> wrote: > On 5/17/22 14:39, Max Staudt wrote: > > On Tue, 17 May 2022 14:21:53 +0200 > > Marc Kleine-Budde <mkl@pengutronix.de> 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?
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
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
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
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
On Wed. 18 May 2022 à 22:32, Marc Kleine-Budde <mkl@pengutronix.de> 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/20210906143057.zrpor5fkh67uqwi2@pengutronix.de/ 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?
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/20210906143057.zrpor5fkh67uqwi2@pengutronix.de/ > 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
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/20210906143057.zrpor5fkh67uqwi2@pengutronix.de/ > 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
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
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
On Wed. 18 May 2022 at 23:59, Oliver Hartkopp <socketcan@hartkopp.net> 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.
On Thu, 19 May 2022 00:38:51 +0900 Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > On Wed. 18 May 2022 at 23:59, Oliver Hartkopp > <socketcan@hartkopp.net> 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
On Thu. 19 May 2022 at 00:52, Max Staudt <max@enpas.org> wrote: > On Thu, 19 May 2022 00:38:51 +0900 > Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > > > On Wed. 18 May 2022 at 23:59, Oliver Hartkopp > > <socketcan@hartkopp.net> 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/20220514141650.1109542-5-mailhol.vincent@wanadoo.fr/ Yours sincerely, Vincent Mailhol
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 */
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.vincent@wanadoo.fr> --- drivers/net/can/dev/skb.c | 58 ++++++++++++++++++++++++++++++++++++++ include/linux/can/skb.h | 59 +-------------------------------------- 2 files changed, 59 insertions(+), 58 deletions(-)