| Message ID | 20200926175810.278529-1-mailhol.vincent@wanadoo.fr |
|---|---|
| Headers | show |
| Series | can: add support for ETAS ES58X CAN USB | expand |
On Sun, Sep 27, 2020 at 02:57:56AM +0900, Vincent Mailhol wrote: > The ES58X devices are incorrectly recognized as USB Modem (CDC ACM), > preventing the etas-es58x module to load. > > Thus, these have been added > to the ignore list in drivers/usb/class/cdc-acm.c > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > drivers/usb/class/cdc-acm.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) Did you mean to send this twice? And where are the 5 other patches in this series? And finally, it's a good idea to include the output of 'lsusb -v' for devices that need quirks so we can figure things out later on, can you fix up your changelog to include that information? thanks, greg k-h
On Sun, Sep 27, 2020 at 07:45:20AM +0200, Greg Kroah-Hartman wrote: > On Sun, Sep 27, 2020 at 02:57:56AM +0900, Vincent Mailhol wrote: > > The ES58X devices are incorrectly recognized as USB Modem (CDC ACM), > > preventing the etas-es58x module to load. > > > > Thus, these have been added > > to the ignore list in drivers/usb/class/cdc-acm.c > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > --- > > drivers/usb/class/cdc-acm.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > Did you mean to send this twice? > > And where are the 5 other patches in this series? > > And finally, it's a good idea to include the output of 'lsusb -v' for > devices that need quirks so we can figure things out later on, can you > fix up your changelog to include that information? Also, why is the device saying it is a cdc-acm compliant device when it is not? Why lie to the operating system like that? thanks, greg k-h
> > Did you mean to send this twice? Sorry for that, I screwed things up a first time when sending the patches: only included the CAN mailing list (linux-can@vger.kernel.org) but ommitted linux-kernel@vger.kernel.org in the cover letter. As a result, it broke the chain reply on lkml.org so I preferred to resend it. > > And where are the 5 other patches in this series? I used the --cc-cmd="scripts/get_maintainer.pl -i" option in git send-email to send the series. The five other patches are not related to USB core but to CAN core, so you were not included in CC by the script. Now, I understand this is confusing, I will take care to CC you on the full series when sending V2. One more time, sorry for that. For your information, the full patch series is available here: https://lkml.org/lkml/2020/9/26/319 > > And finally, it's a good idea to include the output of 'lsusb -v' for > > devices that need quirks so we can figure things out later on, can you > > fix up your changelog to include that information? Noted, will be included in v2 of the patch series. > Also, why is the device saying it is a cdc-acm compliant device when it > is not? Why lie to the operating system like that? This is a leftover debug feature used during development. Future firmware version will have it remove but users with older revision will still face this issue which can be confusing. I will also amend the changelog to better reflect above reason.
The purpose of this patch series is to introduce a new CAN USB
driver to support ETAS USB interfaces (ES58X series).
During development, issues in drivers/net/can/dev.c where discovered,
the fix for those issues are included in this patch series.
We also propose to add two helper functions in include/linux/can/dev.h
which we think can benefit other drivers: get_can_len() and
can_bit_time().
The driver indirectly relies on https://lkml.org/lkml/2020/9/26/251
([PATCH] can: raw: add missing error queue support) for the call to
skb_tx_timestamp() to work but can still compile without it.
*Side notes*: scripts/checkpatch.pl returns 4 'checks' findings in
[PATCH 5/6]. All those findings are of type: "Macro argument reuse 'x'
possible side-effects?". Those arguments reuse are actually made by
calling either __stringify() or sizeof_field() which are both
pre-processor constant. Furthermore, those macro are never called with
arguments sensible to side-effects. So no actual side effect would
occur.
ChangeLog:
v2:
- Fixed -W1 warnings in PATCH 5/6 (v1 was tested with GCC -WExtra
but not with -W1).
- Added lsusb -v information in PATCH 6/6 and rephrased the comment.
- Take care to put everyone in CC of each of the patch of the series
(sorry for the mess in v1...)
Vincent Mailhol (6):
can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ
context
can: dev: add a helper function to get the correct length of Classical
frames
can: dev: __can_get_echo_skb(): fix the return length
can: dev: add a helper function to calculate the duration of one bit
can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces
usb: cdc-acm: add quirk to blacklist ETAS ES58X devices
drivers/net/can/dev.c | 26 +-
drivers/net/can/usb/Kconfig | 9 +
drivers/net/can/usb/Makefile | 1 +
drivers/net/can/usb/etas_es58x/Makefile | 3 +
drivers/net/can/usb/etas_es58x/es581_4.c | 559 ++++
drivers/net/can/usb/etas_es58x/es581_4.h | 237 ++
drivers/net/can/usb/etas_es58x/es58x_core.c | 2725 +++++++++++++++++++
drivers/net/can/usb/etas_es58x/es58x_core.h | 700 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.c | 648 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.h | 243 ++
drivers/usb/class/cdc-acm.c | 11 +
include/linux/can/dev.h | 38 +
12 files changed, 5186 insertions(+), 14 deletions(-)
create mode 100644 drivers/net/can/usb/etas_es58x/Makefile
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.c
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.h
On Wed, Sep 30, 2020 at 11:45:32PM +0900, Vincent Mailhol wrote: > + num_element = > + es58x_msg_num_element(es58x_dev->dev, > + bulk_rx_loopback_msg->rx_loopback_msg, > + msg_len); > + if (unlikely(num_element <= 0)) > + return num_element; Meta-comment on your use of 'unlikely' everywhere. Please drop it, it's only to be used if you can actually measure the difference in a benchmark. You are dealing with USB devices, which are really really slow here. Also, humans make horrible guessers for this type of thing, the compiler and CPU can get this right much more often than we can, and we had the numbers for it (someone measured that 80-90% of our usages of these markings are actually wrong on modern cpus). So just drop them all, it makes the code simpler to read and understand, and the cpu can actually go faster. thanks, greg k-h
> > +static inline int get_can_len(struct sk_buff *skb) > > make this return an u8 > make the skb const > > > +{ > > + struct canfd_frame *cf =3D (struct canfd_frame *)skb->data; > > const > > > + > > + if (can_is_canfd_skb(skb)) > > + return min_t(__u8, cf->len, CANFD_MAX_DLEN); > > u8 > > > + else if (cf->can_id & CAN_RTR_FLAG) > > + return 0; > > + else > > + return min_t(__u8, cf->len, CAN_MAX_DLEN); > > u8 Noted. All those changes will be addressed in v3 series. Thank you. As a side note, macros get_can_dlc() and get_canfd_dlc of the same file (include/linux/can/dev.h) also use __u8 instead of u8. Do you want me to add a patch to change these as below? /* * get_can_dlc(value) - helper macro to cast a given data length code (dlc) - * to __u8 and ensure the dlc value to be max. 8 bytes. + * to u8 and ensure the dlc value to be max. 8 bytes. * * To be used in the CAN netdriver receive path to ensure conformance with * ISO 11898-1 Chapter 8.4.2.3 (DLC field) */ -#define get_can_dlc(i) (min_t(__u8, (i), CAN_MAX_DLC)) -#define get_canfd_dlc(i) (min_t(__u8, (i), CANFD_MAX_DLC)) +#define get_can_dlc(i) (min_t(u8, (i), CAN_MAX_DLC)) +#define get_canfd_dlc(i) (min_t(u8, (i), CANFD_MAX_DLC)) /* Check for outgoing skbs that have not been created by the CAN subsystem */ static inline bool can_skb_headroom_valid(struct net_device *dev,
On 10/1/20 5:45 PM, Vincent Mailhol wrote: >>> +static inline int get_can_len(struct sk_buff *skb) >> >> make this return an u8 >> make the skb const >> >>> +{ >>> + struct canfd_frame *cf =3D (struct canfd_frame *)skb->data; >> >> const >> >>> + >>> + if (can_is_canfd_skb(skb)) >>> + return min_t(__u8, cf->len, CANFD_MAX_DLEN); >> >> u8 >> >>> + else if (cf->can_id & CAN_RTR_FLAG) >>> + return 0; >>> + else >>> + return min_t(__u8, cf->len, CAN_MAX_DLEN); >> >> u8 > > Noted. All those changes will be addressed in v3 series. > Thank you. > > As a side note, macros get_can_dlc() and get_canfd_dlc of the same > file (include/linux/can/dev.h) also use __u8 instead of u8. Do you > want me to add a patch to change these as below? yes. looks good tnx, Marc
> > + num_element = > > + es58x_msg_num_element(es58x_dev->dev, > > + bulk_rx_loopback_msg->rx_loopback_msg, > > + msg_len); > > + if (unlikely(num_element <= 0)) > > + return num_element; > > Meta-comment on your use of 'unlikely' everywhere. Please drop it, it's > only to be used if you can actually measure the difference in a > benchmark. You are dealing with USB devices, which are really really > slow here. Also, humans make horrible guessers for this type of thing, > the compiler and CPU can get this right much more often than we can, and > we had the numbers for it (someone measured that 80-90% of our usages of > these markings are actually wrong on modern cpus). > > So just drop them all, it makes the code simpler to read and understand, > and the cpu can actually go faster. All those branch on which the unlikely() macro were applied were supposed to never been executed under normal circumstances. But I indeed have no benchmark to claim such use. Thank you for the detailed explanation, makes perfect sense. Each use of the likely()/unlikely() macros will be removed in v3 revision.
The purpose of this patch series is to introduce a new CAN USB
driver to support ETAS USB interfaces (ES58X series).
During development, issues in drivers/net/can/dev.c where discovered,
the fix for those issues are included in this patch series.
We also propose to add two helper functions in include/linux/can/dev.h
which we think can benefit other drivers: get_can_len() and
can_bit_time().
The driver indirectly relies on https://lkml.org/lkml/2020/9/26/251
([PATCH] can: raw: add missing error queue support) for the call to
skb_tx_timestamp() to work but can still compile without it.
*Side notes*: scripts/checkpatch.pl returns 4 'checks' findings in
[PATCH 5/6]. All those findings are of type: "Macro argument reuse 'x'
possible side-effects?". Those arguments reuse are actually made by
calling either __stringify() or sizeof_field() which are both
pre-processor constant. Furthermore, those macro are never called with
arguments sensible to side-effects. So no actual side effect would
occur.
Changes in v3:
- Added one additional patch: [PATCH v3 2/7] can: dev: fix type of
get_can_dlc() and get_canfd_dlc() macros.
- Make get_can_len() return u8 and make the skb const in PATCH 3/7.
- Remove all the calls to likely() and unlikely() in PATCH 6/7.
Changes in v2:
- Fixed -W1 warnings in PATCH 6/7 (v1 was tested with GCC -WExtra
but not with -W1).
- Added lsusb -v information in PATCH 7/7 and rephrased the comment.
- Take care to put everyone in CC of each of the patch of the series
(sorry for the mess in v1...)
Vincent Mailhol (7):
can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ
context
can: dev: fix type of get_can_dlc() and get_canfd_dlc() macros
can: dev: add a helper function to get the correct length of Classical
frames
can: dev: __can_get_echo_skb(): fix the return length
can: dev: add a helper function to calculate the duration of one bit
can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces
usb: cdc-acm: add quirk to blacklist ETAS ES58X devices
drivers/net/can/dev.c | 26 +-
drivers/net/can/usb/Kconfig | 9 +
drivers/net/can/usb/Makefile | 1 +
drivers/net/can/usb/etas_es58x/Makefile | 3 +
drivers/net/can/usb/etas_es58x/es581_4.c | 559 ++++
drivers/net/can/usb/etas_es58x/es581_4.h | 237 ++
drivers/net/can/usb/etas_es58x/es58x_core.c | 2725 +++++++++++++++++++
drivers/net/can/usb/etas_es58x/es58x_core.h | 700 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.c | 648 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.h | 243 ++
drivers/usb/class/cdc-acm.c | 11 +
include/linux/can/dev.h | 44 +-
12 files changed, 5189 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/can/usb/etas_es58x/Makefile
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.c
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.h
Resending: In my previous e-mail, I forgot to include the linux-kernel@vger.kernel.org in the cover letter which broke the chain reply... Sorry for the spam. The purpose of this patch series is to introduce a new CAN USB driver to support ETAS USB interfaces (ES58X series). During development, issues in drivers/net/can/dev.c where discovered, the fix for those issues are included in this patch series. We also propose to add two helper functions in include/linux/can/dev.h which we think can benefit other drivers: get_can_len() and can_bit_time(). The driver indirectly relies on https://lkml.org/lkml/2020/9/26/251 ([PATCH] can: raw: add missing error queue support) for the call to skb_tx_timestamp() to work but can still compile without it. *Side notes*: scripts/checkpatch.pl returns 4 'checks' findings in [PATCH 5/6]. All those findings are of type: "Macro argument reuse 'x' possible side-effects?". Those arguments reuse are actually made by calling either __stringify() or sizeof_field() which are both pre-processor constant. Furthermore, those macro are never called with arguments sensible to side-effects. So no actual side effect would occur. Thank you for your comments. Vincent Mailhol (6): can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context can: dev: add a helper function to get the correct length of Classical frames can: dev: __can_get_echo_skb(): fix the return length can: dev: add a helper function to calculate the duration of one bit can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces USB: cdc-acm: blacklist ETAS ES58X device drivers/net/can/dev.c | 26 +- drivers/net/can/usb/Kconfig | 9 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/etas_es58x/Makefile | 4 + drivers/net/can/usb/etas_es58x/es581_4.c | 560 ++++ drivers/net/can/usb/etas_es58x/es581_4.h | 237 ++ drivers/net/can/usb/etas_es58x/es58x_core.c | 2725 +++++++++++++++++++ drivers/net/can/usb/etas_es58x/es58x_core.h | 700 +++++ drivers/net/can/usb/etas_es58x/es58x_fd.c | 650 +++++ drivers/net/can/usb/etas_es58x/es58x_fd.h | 243 ++ drivers/usb/class/cdc-acm.c | 11 + include/linux/can/dev.h | 38 + 12 files changed, 5190 insertions(+), 14 deletions(-) create mode 100644 drivers/net/can/usb/etas_es58x/Makefile create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.c create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.h create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.c create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.h create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.c create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.h