[PATCH 0/6] can: add support for ETAS ES58X CAN USB

[PATCH 0/6] can: add support for ETAS ES58X CAN USB

[Vincent Mailhol]

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

-- 
2.26.2

[PATCH 1/6] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context

[Vincent Mailhol]

If a driver calls can_get_echo_skb() during a hardware IRQ (which is
often, but not always, the case), the 'WARN_ON(in_irq)' in
net/core/skbuff.c#skb_release_head_state() might be triggered, under
network congestion circumstances, together with the potential risk of
a NULL pointer dereference.

The root cause of this issue is the call to kfree_skb() instead of
dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().

This patch prevents the skb to be freed within the call to netif_rx()
by incrementing its reference count with skb_get(). The skb is finally
freed by one of the in-irq-context safe functions:
dev_consume_skb_any() or dev_kfree_skb_any().  The "any" version is
used because some drivers might call can_get_echo_skb() in a normal
context.

The reason for this issue to occur is that initially, in the core
network stack, loopback skb were not supposed to be received in
hardware IRQ context. The CAN stack is an exeption.

This bug is exactly what is described in
https://patchwork.ozlabs.org/patch/835236/

While above link proposes a patch that directly modifies
net/core/dev.c, we try to propose here a smoother modification local
to CAN network stack (the assumption behind is that only CAN devices
are affected by this issue).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 68834a2853c9..e291fda395a0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -512,7 +512,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 	if (!skb)
 		return 0;
 
-	netif_rx(skb);
+	skb_get(skb);
+	if (netif_rx(skb) == NET_RX_SUCCESS)
+		dev_consume_skb_any(skb);
+	else
+		dev_kfree_skb_any(skb);
 
 	return len;
 }
-- 
2.26.2

[PATCH 2/6] can: dev: add a helper function to get the correct length of Classical frames

[Vincent Mailhol]

In classical CAN, the length of the data (i.e. CAN payload) is not
always equal to the DLC! If the frame is a Remote Transmission Request
(RTR), data length is always zero regardless of DLC value and else, if
the DLC is greater than 8, the length is 8. Contrary to common belief,
ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
for Classical Frames and specifies that those DLCs shall indicate that
the data field is 8 bytes long.

Above facts are widely unknown and so many developpers uses the "len"
field of "struct canfd_frame" to get the length of classical CAN
frames: this is incorrect!

This patch introduces function get_can_len() which can be used in
remediation. The function takes the SKB as an input in order to be
able to determine if the frame is classical or FD.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/dev.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5e3d45525bd3..72a8a60c0094 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+/*
+ * get_can_len(skb) - get the length of the CAN payload.
+ *
+ * In classical CAN, the length of the data (i.e. CAN payload) is not
+ * always equal to the DLC! If the frame is a Remote Transmission
+ * Request (RTR), data length is always zero regardless of DLC value
+ * and else, if the DLC is greater than 8, the length is 8. Contrary
+ * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
+ * DLCs greater than 8 for Classical Frames and specifies that those
+ * DLCs shall indicate that the data field is 8 bytes long.
+ */
+static inline int get_can_len(struct sk_buff *skb)
+{
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+
+	if (can_is_canfd_skb(skb))
+		return min_t(__u8, cf->len, CANFD_MAX_DLEN);
+	else if (cf->can_id & CAN_RTR_FLAG)
+		return 0;
+	else
+		return min_t(__u8, cf->len, CAN_MAX_DLEN);
+}
+
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 				    unsigned int txqs, unsigned int rxqs);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-- 
2.26.2

[PATCH 3/6] can: dev: __can_get_echo_skb(): fix the return length

[Vincent Mailhol]

The length of Remote Transmission Request (RTR) frames is always 0
bytes. The DLC represents the requested length, not the actual length
of the RTR. But __can_get_echo_skb() returns the DLC value regardless.

Apply get_can_len() function to retrieve the correct length.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e291fda395a0..8c3e11820e03 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -481,14 +481,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
 	}
 
 	if (priv->echo_skb[idx]) {
-		/* Using "struct canfd_frame::len" for the frame
-		 * length is supported on both CAN and CANFD frames.
-		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
-		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
-		u8 len = cf->len;
 
-		*len_ptr = len;
+		*len_ptr = get_can_len(skb);
 		priv->echo_skb[idx] = NULL;
 
 		return skb;
-- 
2.26.2

[PATCH 4/6] can: dev: add a helper function to calculate the duration of one bit

[Vincent Mailhol]

Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
through include/linux/can/dev.h

Add an helper function can_bit_time() which returns the duration (in
time quanta) of one CAN bit.

Rationale for this patch: the sync segment and the bit time are two
concepts which are defined in the CAN ISO standard. Device drivers for
CAN might need those.

Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
additional information.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c   | 13 ++++++-------
 include/linux/can/dev.h | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8c3e11820e03..6070b4ab3bd8 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -60,7 +60,6 @@ EXPORT_SYMBOL_GPL(can_len2dlc);
 
 #ifdef CONFIG_CAN_CALC_BITTIMING
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
-#define CAN_CALC_SYNC_SEG 1
 
 /* Bit-timing calculation derived from:
  *
@@ -86,8 +85,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 	int i;
 
 	for (i = 0; i <= 1; i++) {
-		tseg2 = tseg + CAN_CALC_SYNC_SEG -
-			(sample_point_nominal * (tseg + CAN_CALC_SYNC_SEG)) /
+		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;
@@ -96,8 +95,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 			tseg2 = tseg - tseg1;
 		}
 
-		sample_point = 1000 * (tseg + CAN_CALC_SYNC_SEG - tseg2) /
-			(tseg + CAN_CALC_SYNC_SEG);
+		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 &&
@@ -145,7 +144,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	/* 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_CALC_SYNC_SEG + tseg / 2;
+		tsegall = CAN_SYNC_SEG + tseg / 2;
 
 		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
 		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
@@ -223,7 +222,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 
 	/* real bitrate */
 	bt->bitrate = priv->clock.freq /
-		(bt->brp * (CAN_CALC_SYNC_SEG + tseg1 + tseg2));
+		(bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
 
 	return 0;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 72a8a60c0094..30b327f1a4bd 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -82,6 +82,21 @@ struct can_priv {
 #endif
 };
 
+#define CAN_SYNC_SEG 1
+
+/*
+ * can_bit_time() - Duration of one bit
+ *
+ * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
+ * additional information.
+ *
+ * Return: the number of time quanta in one bit.
+ */
+static inline int can_bit_time(struct can_bittiming *bt)
+{
+	return CAN_SYNC_SEG + bt->prop_seg + bt->phase_seg1 + bt->phase_seg2;
+}
+
 /*
  * 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.
-- 
2.26.2

[PATCH 6/6] USB: cdc-acm: blacklist ETAS ES58X device

[Vincent Mailhol]

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(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..ed9355094e8c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1906,6 +1906,17 @@ static const struct usb_device_id acm_ids[] = {
 	.driver_info = IGNORE_DEVICE,
 	},
 
+	/* Exclude ETAS ES58x */
+	{ USB_DEVICE(0x108c, 0x0159), /* ES581.4 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0168), /* ES582.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0169), /* ES584.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+
 	{ USB_DEVICE(0x1bc7, 0x0021), /* Telit 3G ACM only composition */
 	.driver_info = SEND_ZERO_PACKET,
 	},
-- 
2.26.2

Re: [PATCH 6/6] USB: cdc-acm: blacklist ETAS ES58X device

[Greg Kroah-Hartman]

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

Re: [PATCH 6/6] USB: cdc-acm: blacklist ETAS ES58X device

[Greg Kroah-Hartman]

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

Re: [PATCH 6/6] USB: cdc-acm: blacklist ETAS ES58X device

[Vincent Mailhol]

> > 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.

[PATCH v2 0/6] can: add support for ETAS ES58X CAN USB

[Vincent Mailhol]

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

-- 
2.26.2

[PATCH v2 1/6] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context

[Vincent Mailhol]

If a driver calls can_get_echo_skb() during a hardware IRQ (which is
often, but not always, the case), the 'WARN_ON(in_irq)' in
net/core/skbuff.c#skb_release_head_state() might be triggered, under
network congestion circumstances, together with the potential risk of
a NULL pointer dereference.

The root cause of this issue is the call to kfree_skb() instead of
dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().

This patch prevents the skb to be freed within the call to netif_rx()
by incrementing its reference count with skb_get(). The skb is finally
freed by one of the in-irq-context safe functions:
dev_consume_skb_any() or dev_kfree_skb_any().  The "any" version is
used because some drivers might call can_get_echo_skb() in a normal
context.

The reason for this issue to occur is that initially, in the core
network stack, loopback skb were not supposed to be received in
hardware IRQ context. The CAN stack is an exeption.

This bug was previously reported back in 2017 in [1] but the proposed
patch never got accepted.

While [1] directly modifies net/core/dev.c, we try to propose here a
smoother modification local to CAN network stack (the assumption
behind is that only CAN devices are affected by this issue).

[1] https://patchwork.ozlabs.org/patch/835236/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 68834a2853c9..e291fda395a0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -512,7 +512,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 	if (!skb)
 		return 0;
 
-	netif_rx(skb);
+	skb_get(skb);
+	if (netif_rx(skb) == NET_RX_SUCCESS)
+		dev_consume_skb_any(skb);
+	else
+		dev_kfree_skb_any(skb);
 
 	return len;
 }
-- 
2.26.2

[PATCH v2 2/6] can: dev: add a helper function to get the correct length of Classical frames

[Vincent Mailhol]

In classical CAN, the length of the data (i.e. CAN payload) is not
always equal to the DLC! If the frame is a Remote Transmission Request
(RTR), data length is always zero regardless of DLC value and else, if
the DLC is greater than 8, the length is 8. Contrary to common belief,
ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
for Classical Frames and specifies that those DLCs shall indicate that
the data field is 8 bytes long.

Above facts are widely unknown and so many developpers uses the "len"
field of "struct canfd_frame" to get the length of classical CAN
frames: this is incorrect!

This patch introduces function get_can_len() which can be used in
remediation. The function takes the SKB as an input in order to be
able to determine if the frame is classical or FD.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/dev.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5e3d45525bd3..72a8a60c0094 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+/*
+ * get_can_len(skb) - get the length of the CAN payload.
+ *
+ * In classical CAN, the length of the data (i.e. CAN payload) is not
+ * always equal to the DLC! If the frame is a Remote Transmission
+ * Request (RTR), data length is always zero regardless of DLC value
+ * and else, if the DLC is greater than 8, the length is 8. Contrary
+ * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
+ * DLCs greater than 8 for Classical Frames and specifies that those
+ * DLCs shall indicate that the data field is 8 bytes long.
+ */
+static inline int get_can_len(struct sk_buff *skb)
+{
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+
+	if (can_is_canfd_skb(skb))
+		return min_t(__u8, cf->len, CANFD_MAX_DLEN);
+	else if (cf->can_id & CAN_RTR_FLAG)
+		return 0;
+	else
+		return min_t(__u8, cf->len, CAN_MAX_DLEN);
+}
+
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 				    unsigned int txqs, unsigned int rxqs);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-- 
2.26.2

[PATCH v2 3/6] can: dev: __can_get_echo_skb(): fix the return length

[Vincent Mailhol]

The length of Remote Transmission Request (RTR) frames is always 0
bytes. The DLC represents the requested length, not the actual length
of the RTR. But __can_get_echo_skb() returns the DLC value regardless.

Apply get_can_len() function to retrieve the correct length.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e291fda395a0..8c3e11820e03 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -481,14 +481,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
 	}
 
 	if (priv->echo_skb[idx]) {
-		/* Using "struct canfd_frame::len" for the frame
-		 * length is supported on both CAN and CANFD frames.
-		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
-		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
-		u8 len = cf->len;
 
-		*len_ptr = len;
+		*len_ptr = get_can_len(skb);
 		priv->echo_skb[idx] = NULL;
 
 		return skb;
-- 
2.26.2

[PATCH v2 4/6] can: dev: add a helper function to calculate the duration of one bit

[Vincent Mailhol]

Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
through include/linux/can/dev.h

Add an helper function can_bit_time() which returns the duration (in
time quanta) of one CAN bit.

Rationale for this patch: the sync segment and the bit time are two
concepts which are defined in the CAN ISO standard. Device drivers for
CAN might need those.

Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
additional information.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c   | 13 ++++++-------
 include/linux/can/dev.h | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8c3e11820e03..6070b4ab3bd8 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -60,7 +60,6 @@ EXPORT_SYMBOL_GPL(can_len2dlc);
 
 #ifdef CONFIG_CAN_CALC_BITTIMING
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
-#define CAN_CALC_SYNC_SEG 1
 
 /* Bit-timing calculation derived from:
  *
@@ -86,8 +85,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 	int i;
 
 	for (i = 0; i <= 1; i++) {
-		tseg2 = tseg + CAN_CALC_SYNC_SEG -
-			(sample_point_nominal * (tseg + CAN_CALC_SYNC_SEG)) /
+		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;
@@ -96,8 +95,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 			tseg2 = tseg - tseg1;
 		}
 
-		sample_point = 1000 * (tseg + CAN_CALC_SYNC_SEG - tseg2) /
-			(tseg + CAN_CALC_SYNC_SEG);
+		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 &&
@@ -145,7 +144,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	/* 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_CALC_SYNC_SEG + tseg / 2;
+		tsegall = CAN_SYNC_SEG + tseg / 2;
 
 		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
 		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
@@ -223,7 +222,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 
 	/* real bitrate */
 	bt->bitrate = priv->clock.freq /
-		(bt->brp * (CAN_CALC_SYNC_SEG + tseg1 + tseg2));
+		(bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
 
 	return 0;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 72a8a60c0094..30b327f1a4bd 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -82,6 +82,21 @@ struct can_priv {
 #endif
 };
 
+#define CAN_SYNC_SEG 1
+
+/*
+ * can_bit_time() - Duration of one bit
+ *
+ * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
+ * additional information.
+ *
+ * Return: the number of time quanta in one bit.
+ */
+static inline int can_bit_time(struct can_bittiming *bt)
+{
+	return CAN_SYNC_SEG + bt->prop_seg + bt->phase_seg1 + bt->phase_seg2;
+}
+
 /*
  * 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.
-- 
2.26.2

[PATCH v2 6/6] usb: cdc-acm: add quirk to blacklist ETAS ES58X devices

[Vincent Mailhol]

The ES58X devices has a CDC ACM interface (used for debug
purpose). During probing, the device is thus recognized as USB Modem
(CDC ACM), preventing the etas-es58x module to load:
  usbcore: registered new interface driver etas_es58x
  usb 1-1.1: new full-speed USB device number 14 using xhci_hcd
  usb 1-1.1: New USB device found, idVendor=108c, idProduct=0159, bcdDevice= 1.00
  usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
  usb 1-1.1: Product: ES581.4
  usb 1-1.1: Manufacturer: ETAS GmbH
  usb 1-1.1: SerialNumber: 2204355
  cdc_acm 1-1.1:1.0: No union descriptor, testing for castrated device
  cdc_acm 1-1.1:1.0: ttyACM0: USB ACM device

Thus, these have been added to the ignore list in
drivers/usb/class/cdc-acm.c

N.B. Future firmware release of the ES58X will remove the CDC-ACM
interface.

`lsusb -v` of the three devices variant (ES581.4, ES582.1 and
ES584.1):

  Bus 001 Device 011: ID 108c:0159 Robert Bosch GmbH ES581.4
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               1.10
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0159
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES581.4
    iSerial                 3 2204355
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0035
      bNumInterfaces          1
      bConfigurationValue     1
      iConfiguration          5 Bus Powered Configuration
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              100mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           3
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      0
        iInterface              4 ACM Control Interface
        CDC Header:
          bcdCDC               1.10
        CDC Call Management:
          bmCapabilities       0x01
            call management
          bDataInterface          0
        CDC ACM:
          bmCapabilities       0x06
            sends break
            line coding and serial state
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0010  1x 16 bytes
          bInterval              10
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x82  EP 2 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x03  EP 3 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval               0
  Device Status:     0x0000
    (Bus Powered)

  Bus 001 Device 012: ID 108c:0168 Robert Bosch GmbH ES582
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0168
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES582
    iSerial                 3 0108933
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0043
      bNumInterfaces          2
      bConfigurationValue     1
      iConfiguration          0
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              500mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           1
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      1 AT-commands (v.25ter)
        iInterface              0
        CDC Header:
          bcdCDC               1.10
        CDC ACM:
          bmCapabilities       0x02
            line coding and serial state
        CDC Union:
          bMasterInterface        0
          bSlaveInterface         1
        CDC Call Management:
          bmCapabilities       0x03
            call management
            use DataInterface
          bDataInterface          1
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x83  EP 3 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval              16
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        1
        bAlternateSetting       0
        bNumEndpoints           2
        bInterfaceClass        10 CDC Data
        bInterfaceSubClass      0
        bInterfaceProtocol      0
        iInterface              0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x02  EP 2 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
  Device Qualifier (for other device speed):
    bLength                10
    bDescriptorType         6
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    bNumConfigurations      1
  Device Status:     0x0000
    (Bus Powered)

  Bus 001 Device 013: ID 108c:0169 Robert Bosch GmbH ES584.1
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0169
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES584.1
    iSerial                 3 0100320
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0043
      bNumInterfaces          2
      bConfigurationValue     1
      iConfiguration          0
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              500mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           1
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      1 AT-commands (v.25ter)
        iInterface              0
        CDC Header:
          bcdCDC               1.10
        CDC ACM:
          bmCapabilities       0x02
            line coding and serial state
        CDC Union:
          bMasterInterface        0
          bSlaveInterface         1
        CDC Call Management:
          bmCapabilities       0x03
            call management
            use DataInterface
          bDataInterface          1
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x83  EP 3 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval              16
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        1
        bAlternateSetting       0
        bNumEndpoints           2
        bInterfaceClass        10 CDC Data
        bInterfaceSubClass      0
        bInterfaceProtocol      0
        iInterface              0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x02  EP 2 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
  Device Qualifier (for other device speed):
    bLength                10
    bDescriptorType         6
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    bNumConfigurations      1
  Device Status:     0x0000
    (Bus Powered)

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/usb/class/cdc-acm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..ed9355094e8c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1906,6 +1906,17 @@ static const struct usb_device_id acm_ids[] = {
 	.driver_info = IGNORE_DEVICE,
 	},
 
+	/* Exclude ETAS ES58x */
+	{ USB_DEVICE(0x108c, 0x0159), /* ES581.4 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0168), /* ES582.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0169), /* ES584.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+
 	{ USB_DEVICE(0x1bc7, 0x0021), /* Telit 3G ACM only composition */
 	.driver_info = SEND_ZERO_PACKET,
 	},
-- 
2.26.2

Re: [PATCH v2 2/6] can: dev: add a helper function to get the correct length of Classical frames

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2868 bytes --]

On 9/30/20 4:45 PM, Vincent Mailhol wrote:
> In classical CAN, the length of the data (i.e. CAN payload) is not
> always equal to the DLC! If the frame is a Remote Transmission Request
> (RTR), data length is always zero regardless of DLC value and else, if
> the DLC is greater than 8, the length is 8. Contrary to common belief,
> ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
> for Classical Frames and specifies that those DLCs shall indicate that
> the data field is 8 bytes long.
> 
> Above facts are widely unknown and so many developpers uses the "len"
> field of "struct canfd_frame" to get the length of classical CAN
> frames: this is incorrect!
> 
> This patch introduces function get_can_len() which can be used in
> remediation. The function takes the SKB as an input in order to be
> able to determine if the frame is classical or FD.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/can/dev.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 5e3d45525bd3..72a8a60c0094 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc);
>  /* map the sanitized data length to an appropriate data length code */
>  u8 can_len2dlc(u8 len);
>  
> +/*
> + * get_can_len(skb) - get the length of the CAN payload.
> + *
> + * In classical CAN, the length of the data (i.e. CAN payload) is not
> + * always equal to the DLC! If the frame is a Remote Transmission
> + * Request (RTR), data length is always zero regardless of DLC value
> + * and else, if the DLC is greater than 8, the length is 8. Contrary
> + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
> + * DLCs greater than 8 for Classical Frames and specifies that those
> + * DLCs shall indicate that the data field is 8 bytes long.
> + */
> +static inline int get_can_len(struct sk_buff *skb)

make this return an u8
make the skb const

> +{
> +	struct canfd_frame *cf = (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

> +}
> +
>  struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
>  				    unsigned int txqs, unsigned int rxqs);
>  #define alloc_candev(sizeof_priv, echo_skb_max) \
> 

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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/6] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Greg Kroah-Hartman]

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

Re: [PATCH v2 2/6] can: dev: add a helper function

[Vincent Mailhol]

> > +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,

Re: [PATCH v2 2/6] can: dev: add a helper function

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1078 bytes --]

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

-- 
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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/6] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Vincent Mailhol]

> > +	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.

[PATCH v3 0/7] can: add support for ETAS ES58X CAN USB

[Vincent Mailhol]

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

-- 
2.26.2

[PATCH v3 1/7] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context

[Vincent Mailhol]

If a driver calls can_get_echo_skb() during a hardware IRQ (which is
often, but not always, the case), the 'WARN_ON(in_irq)' in
net/core/skbuff.c#skb_release_head_state() might be triggered, under
network congestion circumstances, together with the potential risk of
a NULL pointer dereference.

The root cause of this issue is the call to kfree_skb() instead of
dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().

This patch prevents the skb to be freed within the call to netif_rx()
by incrementing its reference count with skb_get(). The skb is finally
freed by one of the in-irq-context safe functions:
dev_consume_skb_any() or dev_kfree_skb_any().  The "any" version is
used because some drivers might call can_get_echo_skb() in a normal
context.

The reason for this issue to occur is that initially, in the core
network stack, loopback skb were not supposed to be received in
hardware IRQ context. The CAN stack is an exeption.

This bug was previously reported back in 2017 in [1] but the proposed
patch never got accepted.

While [1] directly modifies net/core/dev.c, we try to propose here a
smoother modification local to CAN network stack (the assumption
behind is that only CAN devices are affected by this issue).

[1] https://patchwork.ozlabs.org/patch/835236/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3: None

Changes in v2:
 - Minor changes of link format in the changelog.
---
 drivers/net/can/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 68834a2853c9..e291fda395a0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -512,7 +512,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 	if (!skb)
 		return 0;
 
-	netif_rx(skb);
+	skb_get(skb);
+	if (netif_rx(skb) == NET_RX_SUCCESS)
+		dev_consume_skb_any(skb);
+	else
+		dev_kfree_skb_any(skb);
 
 	return len;
 }
-- 
2.26.2

[PATCH v3 2/7] can: dev: fix type of get_can_dlc() and get_canfd_dlc() macros

[Vincent Mailhol]

The macros get_can_dlc() and get_canfd_dlc() are not visible in
userland. As such, type u8 should be preferred over type __u8.

Reference: https://lkml.org/lkml/2020/10/1/708
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/dev.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5e3d45525bd3..132b4133f9d0 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -84,13 +84,13 @@ struct can_priv {
 
 /*
  * 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,
-- 
2.26.2

[PATCH v3 3/7] can: dev: add a helper function to get the correct length of Classical frames

[Vincent Mailhol]

In classical CAN, the length of the data (i.e. CAN payload) is not
always equal to the DLC! If the frame is a Remote Transmission Request
(RTR), data length is always zero regardless of DLC value and else, if
the DLC is greater than 8, the length is 8. Contrary to common belief,
ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
for Classical Frames and specifies that those DLCs shall indicate that
the data field is 8 bytes long.

Above facts are widely unknown and so many developpers uses the "len"
field of "struct canfd_frame" to get the length of classical CAN
frames: this is incorrect!

This patch introduces function get_can_len() which can be used in
remediation. The function takes the SKB as an input in order to be
able to determine if the frame is classical or FD.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3:
  - Make get_can_len() return u8.
  - Make the skb const.

Changes in v2: None
---
 include/linux/can/dev.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 132b4133f9d0..791c452d98e1 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+/*
+ * get_can_len(skb) - get the length of the CAN payload.
+ *
+ * In classical CAN, the length of the data (i.e. CAN payload) is not
+ * always equal to the DLC! If the frame is a Remote Transmission
+ * Request (RTR), data length is always zero regardless of DLC value
+ * and else, if the DLC is greater than 8, the length is 8. Contrary
+ * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
+ * DLCs greater than 8 for Classical Frames and specifies that those
+ * DLCs shall indicate that the data field is 8 bytes long.
+ */
+static inline u8 get_can_len(const struct sk_buff *skb)
+{
+	const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
+
+	if (can_is_canfd_skb(skb))
+		return min_t(u8, cf->len, CANFD_MAX_DLEN);
+	else if (cf->can_id & CAN_RTR_FLAG)
+		return 0;
+	else
+		return min_t(u8, cf->len, CAN_MAX_DLEN);
+}
+
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 				    unsigned int txqs, unsigned int rxqs);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-- 
2.26.2

[PATCH v3 4/7] can: dev: __can_get_echo_skb(): fix the return length

[Vincent Mailhol]

The length of Remote Transmission Request (RTR) frames is always 0
bytes. The DLC represents the requested length, not the actual length
of the RTR. But __can_get_echo_skb() returns the DLC value regardless.

Apply get_can_len() function to retrieve the correct length.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3: None

Changes in v2: None
---
 drivers/net/can/dev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e291fda395a0..8c3e11820e03 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -481,14 +481,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
 	}
 
 	if (priv->echo_skb[idx]) {
-		/* Using "struct canfd_frame::len" for the frame
-		 * length is supported on both CAN and CANFD frames.
-		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
-		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
-		u8 len = cf->len;
 
-		*len_ptr = len;
+		*len_ptr = get_can_len(skb);
 		priv->echo_skb[idx] = NULL;
 
 		return skb;
-- 
2.26.2

[PATCH v3 5/7] can: dev: add a helper function to calculate the duration of one bit

[Vincent Mailhol]

Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
through include/linux/can/dev.h

Add an helper function can_bit_time() which returns the duration (in
time quanta) of one CAN bit.

Rationale for this patch: the sync segment and the bit time are two
concepts which are defined in the CAN ISO standard. Device drivers for
CAN might need those.

Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
additional information.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3: None

Changes in v2: None
---
 drivers/net/can/dev.c   | 13 ++++++-------
 include/linux/can/dev.h | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8c3e11820e03..6070b4ab3bd8 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -60,7 +60,6 @@ EXPORT_SYMBOL_GPL(can_len2dlc);
 
 #ifdef CONFIG_CAN_CALC_BITTIMING
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
-#define CAN_CALC_SYNC_SEG 1
 
 /* Bit-timing calculation derived from:
  *
@@ -86,8 +85,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 	int i;
 
 	for (i = 0; i <= 1; i++) {
-		tseg2 = tseg + CAN_CALC_SYNC_SEG -
-			(sample_point_nominal * (tseg + CAN_CALC_SYNC_SEG)) /
+		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;
@@ -96,8 +95,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 			tseg2 = tseg - tseg1;
 		}
 
-		sample_point = 1000 * (tseg + CAN_CALC_SYNC_SEG - tseg2) /
-			(tseg + CAN_CALC_SYNC_SEG);
+		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 &&
@@ -145,7 +144,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	/* 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_CALC_SYNC_SEG + tseg / 2;
+		tsegall = CAN_SYNC_SEG + tseg / 2;
 
 		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
 		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
@@ -223,7 +222,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 
 	/* real bitrate */
 	bt->bitrate = priv->clock.freq /
-		(bt->brp * (CAN_CALC_SYNC_SEG + tseg1 + tseg2));
+		(bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
 
 	return 0;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 791c452d98e1..77c3ea49b8fb 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -82,6 +82,21 @@ struct can_priv {
 #endif
 };
 
+#define CAN_SYNC_SEG 1
+
+/*
+ * can_bit_time() - Duration of one bit
+ *
+ * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
+ * additional information.
+ *
+ * Return: the number of time quanta in one bit.
+ */
+static inline int can_bit_time(struct can_bittiming *bt)
+{
+	return CAN_SYNC_SEG + bt->prop_seg + bt->phase_seg1 + bt->phase_seg2;
+}
+
 /*
  * 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.
-- 
2.26.2

[PATCH v3 7/7] usb: cdc-acm: add quirk to blacklist ETAS ES58X devices

[Vincent Mailhol]

The ES58X devices has a CDC ACM interface (used for debug
purpose). During probing, the device is thus recognized as USB Modem
(CDC ACM), preventing the etas-es58x module to load:
  usbcore: registered new interface driver etas_es58x
  usb 1-1.1: new full-speed USB device number 14 using xhci_hcd
  usb 1-1.1: New USB device found, idVendor=108c, idProduct=0159, bcdDevice= 1.00
  usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
  usb 1-1.1: Product: ES581.4
  usb 1-1.1: Manufacturer: ETAS GmbH
  usb 1-1.1: SerialNumber: 2204355
  cdc_acm 1-1.1:1.0: No union descriptor, testing for castrated device
  cdc_acm 1-1.1:1.0: ttyACM0: USB ACM device

Thus, these have been added to the ignore list in
drivers/usb/class/cdc-acm.c

N.B. Future firmware release of the ES58X will remove the CDC-ACM
interface.

`lsusb -v` of the three devices variant (ES581.4, ES582.1 and
ES584.1):

  Bus 001 Device 011: ID 108c:0159 Robert Bosch GmbH ES581.4
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               1.10
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0159
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES581.4
    iSerial                 3 2204355
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0035
      bNumInterfaces          1
      bConfigurationValue     1
      iConfiguration          5 Bus Powered Configuration
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              100mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           3
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      0
        iInterface              4 ACM Control Interface
        CDC Header:
          bcdCDC               1.10
        CDC Call Management:
          bmCapabilities       0x01
            call management
          bDataInterface          0
        CDC ACM:
          bmCapabilities       0x06
            sends break
            line coding and serial state
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0010  1x 16 bytes
          bInterval              10
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x82  EP 2 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x03  EP 3 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval               0
  Device Status:     0x0000
    (Bus Powered)

  Bus 001 Device 012: ID 108c:0168 Robert Bosch GmbH ES582
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0168
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES582
    iSerial                 3 0108933
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0043
      bNumInterfaces          2
      bConfigurationValue     1
      iConfiguration          0
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              500mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           1
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      1 AT-commands (v.25ter)
        iInterface              0
        CDC Header:
          bcdCDC               1.10
        CDC ACM:
          bmCapabilities       0x02
            line coding and serial state
        CDC Union:
          bMasterInterface        0
          bSlaveInterface         1
        CDC Call Management:
          bmCapabilities       0x03
            call management
            use DataInterface
          bDataInterface          1
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x83  EP 3 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval              16
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        1
        bAlternateSetting       0
        bNumEndpoints           2
        bInterfaceClass        10 CDC Data
        bInterfaceSubClass      0
        bInterfaceProtocol      0
        iInterface              0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x02  EP 2 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
  Device Qualifier (for other device speed):
    bLength                10
    bDescriptorType         6
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    bNumConfigurations      1
  Device Status:     0x0000
    (Bus Powered)

  Bus 001 Device 013: ID 108c:0169 Robert Bosch GmbH ES584.1
  Device Descriptor:
    bLength                18
    bDescriptorType         1
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    idVendor           0x108c Robert Bosch GmbH
    idProduct          0x0169
    bcdDevice            1.00
    iManufacturer           1 ETAS GmbH
    iProduct                2 ES584.1
    iSerial                 3 0100320
    bNumConfigurations      1
    Configuration Descriptor:
      bLength                 9
      bDescriptorType         2
      wTotalLength       0x0043
      bNumInterfaces          2
      bConfigurationValue     1
      iConfiguration          0
      bmAttributes         0x80
        (Bus Powered)
      MaxPower              500mA
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        0
        bAlternateSetting       0
        bNumEndpoints           1
        bInterfaceClass         2 Communications
        bInterfaceSubClass      2 Abstract (modem)
        bInterfaceProtocol      1 AT-commands (v.25ter)
        iInterface              0
        CDC Header:
          bcdCDC               1.10
        CDC ACM:
          bmCapabilities       0x02
            line coding and serial state
        CDC Union:
          bMasterInterface        0
          bSlaveInterface         1
        CDC Call Management:
          bmCapabilities       0x03
            call management
            use DataInterface
          bDataInterface          1
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x83  EP 3 IN
          bmAttributes            3
            Transfer Type            Interrupt
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0040  1x 64 bytes
          bInterval              16
      Interface Descriptor:
        bLength                 9
        bDescriptorType         4
        bInterfaceNumber        1
        bAlternateSetting       0
        bNumEndpoints           2
        bInterfaceClass        10 CDC Data
        bInterfaceSubClass      0
        bInterfaceProtocol      0
        iInterface              0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x81  EP 1 IN
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
        Endpoint Descriptor:
          bLength                 7
          bDescriptorType         5
          bEndpointAddress     0x02  EP 2 OUT
          bmAttributes            2
            Transfer Type            Bulk
            Synch Type               None
            Usage Type               Data
          wMaxPacketSize     0x0200  1x 512 bytes
          bInterval               0
  Device Qualifier (for other device speed):
    bLength                10
    bDescriptorType         6
    bcdUSB               2.00
    bDeviceClass            2 Communications
    bDeviceSubClass         0
    bDeviceProtocol         0
    bMaxPacketSize0        64
    bNumConfigurations      1
  Device Status:     0x0000
    (Bus Powered)

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3: None

Changes in v2:
  - Added dmesg and lsusb -v information and rephrased the comment.
---
 drivers/usb/class/cdc-acm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..ed9355094e8c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1906,6 +1906,17 @@ static const struct usb_device_id acm_ids[] = {
 	.driver_info = IGNORE_DEVICE,
 	},
 
+	/* Exclude ETAS ES58x */
+	{ USB_DEVICE(0x108c, 0x0159), /* ES581.4 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0168), /* ES582.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+	{ USB_DEVICE(0x108c, 0x0169), /* ES584.1 */
+	.driver_info = IGNORE_DEVICE,
+	},
+
 	{ USB_DEVICE(0x1bc7, 0x0021), /* Telit 3G ACM only composition */
 	.driver_info = SEND_ZERO_PACKET,
 	},
-- 
2.26.2

Re: [PATCH v3 5/7] can: dev: add a helper function to calculate the duration of one bit

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4067 bytes --]

On 10/2/20 5:41 PM, Vincent Mailhol wrote:
> Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
> through include/linux/can/dev.h
> 
> Add an helper function can_bit_time() which returns the duration (in
> time quanta) of one CAN bit.
> 
> Rationale for this patch: the sync segment and the bit time are two
> concepts which are defined in the CAN ISO standard. Device drivers for
> CAN might need those.
> 
> Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
> additional information.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> 
> Changes in v3: None
> 
> Changes in v2: None
> ---
>  drivers/net/can/dev.c   | 13 ++++++-------
>  include/linux/can/dev.h | 15 +++++++++++++++
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 8c3e11820e03..6070b4ab3bd8 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -60,7 +60,6 @@ EXPORT_SYMBOL_GPL(can_len2dlc);
>  
>  #ifdef CONFIG_CAN_CALC_BITTIMING
>  #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
> -#define CAN_CALC_SYNC_SEG 1
>  
>  /* Bit-timing calculation derived from:
>   *
> @@ -86,8 +85,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>  	int i;
>  
>  	for (i = 0; i <= 1; i++) {
> -		tseg2 = tseg + CAN_CALC_SYNC_SEG -
> -			(sample_point_nominal * (tseg + CAN_CALC_SYNC_SEG)) /
> +		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;
> @@ -96,8 +95,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>  			tseg2 = tseg - tseg1;
>  		}
>  
> -		sample_point = 1000 * (tseg + CAN_CALC_SYNC_SEG - tseg2) /
> -			(tseg + CAN_CALC_SYNC_SEG);
> +		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 &&
> @@ -145,7 +144,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
>  	/* 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_CALC_SYNC_SEG + tseg / 2;
> +		tsegall = CAN_SYNC_SEG + tseg / 2;
>  
>  		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
>  		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
> @@ -223,7 +222,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
>  
>  	/* real bitrate */
>  	bt->bitrate = priv->clock.freq /
> -		(bt->brp * (CAN_CALC_SYNC_SEG + tseg1 + tseg2));
> +		(bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
>  
>  	return 0;
>  }
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 791c452d98e1..77c3ea49b8fb 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -82,6 +82,21 @@ struct can_priv {
>  #endif
>  };
>  
> +#define CAN_SYNC_SEG 1
> +
> +/*
> + * can_bit_time() - Duration of one bit
> + *
> + * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
> + * additional information.
> + *
> + * Return: the number of time quanta in one bit.
> + */
> +static inline int can_bit_time(struct can_bittiming *bt)

make it return an unsigned int
make the bt pointer const

> +{
> +	return CAN_SYNC_SEG + bt->prop_seg + bt->phase_seg1 + bt->phase_seg2;
> +}
> +
>  /*
>   * 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.
> 

tnx,
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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 5/7] can: dev: add a helper function to calculate the duration of one bit

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1576 bytes --]

On 10/4/20 1:06 PM, Marc Kleine-Budde wrote:
> On 10/2/20 5:41 PM, Vincent Mailhol wrote:
>> Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
>> through include/linux/can/dev.h
>>
>> Add an helper function can_bit_time() which returns the duration (in
>> time quanta) of one CAN bit.
>>
>> Rationale for this patch: the sync segment and the bit time are two
>> concepts which are defined in the CAN ISO standard. Device drivers for
>> CAN might need those.
>>
>> Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
>> additional information.
>>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

[...]

>> index 791c452d98e1..77c3ea49b8fb 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -82,6 +82,21 @@ struct can_priv {
>>  #endif
>>  };
>>  
>> +#define CAN_SYNC_SEG 1
>> +
>> +/*
>> + * can_bit_time() - Duration of one bit
>> + *
>> + * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
>> + * additional information.
>> + *
>> + * Return: the number of time quanta in one bit.
>> + */
>> +static inline int can_bit_time(struct can_bittiming *bt)
> 
> make it return an unsigned int
> make the bt pointer const

I'll change this while applying the patch.

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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 6/7] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 31342 bytes --]

On 10/2/20 5:41 PM, Vincent Mailhol wrote:
> This driver supports the ES581.4, ES582.1 and ES584.1 interfaces from
> ETAS GmbH (https://www.etas.com/en/products/es58x.php).
> 
> Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> 
> Changes in v3:
>   - Remove all the calls to likely() and unlikely().
> 
> Changes in v2:
>   - Fixed -W1 warnings (v1 was tested with GCC -WExtra but not with -W1).
> ---
>  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 ++
>  9 files changed, 5125 insertions(+)
>  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

[...]

Just one header file for now :)

> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
> new file mode 100644
> index 000000000000..359ddc44a3ad
> --- /dev/null
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
> @@ -0,0 +1,700 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces.
> + *
> + * File es58x_core.h: All common definitions and declarations.
> + *
> + * Copyright (C) 2019 Robert Bosch Engineering and Business
> + * Solutions. All rights reserved.
> + * Copyright (C) 2020 ETAS K.K.. All rights reserved.
> + */
> +
> +#ifndef __ES58X_COMMON_H__
> +#define __ES58X_COMMON_H__
> +
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +#include <linux/netdevice.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +
> +#include "es581_4.h"
> +#include "es58x_fd.h"
> +
> +/* Size of a CAN Standard Frame (rounded up and ignoring bitsuffing). */
> +#define ES58X_SFF_BYTES(data_len) (round_up(47, 8) / 8 + (data_len))

DIV_ROUNDUP

> +/* Size of a CAN Extended Frame (rounded up and ignoring bitsuffing). */
> +#define ES58X_EFF_BYTES(data_len) (round_up(67, 8) / 8 + (data_len))

dame here
> +/* Maximum size of a CAN frame (rounded up and ignoring bitsuffing). */
> +#define ES58X_CAN_FRAME_BYTES_MAX ES58X_EFF_BYTES(CAN_MAX_DLEN)

please add a new file between the define and the doc of the next one

> +/* Maximum size of a CAN-FD frame (rough estimation because
> + * ES58X_SFF_BYTES() and ES58X_EFF_BYTES() macros are using the
> + * constant values for CAN not CAN-FD).
> + */
> +#define ES58X_CANFD_FRAME_BYTES_MAX ES58X_EFF_BYTES(CANFD_MAX_DLEN)
> +
> +/* Driver constants */
> +#define ES58X_RX_URBS_MAX         5	// Empirical value
> +#define ES58X_TX_URBS_MAX         8	// Empirical value

please use one space only

> +
> +#define ES58X_MAX(param)				\
> +	(ES581_4_##param > ES58X_FD_##param ?		\
> +		ES581_4_##param : ES58X_FD_##param)
> +#define ES58X_TX_BULK_MAX ES58X_MAX(TX_BULK_MAX)
> +#define ES58X_RX_BULK_MAX ES58X_MAX(RX_BULK_MAX)
> +#define ES58X_RX_LOOPBACK_BULK_MAX ES58X_MAX(RX_LOOPBACK_BULK_MAX)
> +#define ES58X_NUM_CAN_CH_MAX ES58X_MAX(NUM_CAN_CH)
> +
> +/* Use this when channel index is irrelevant (e.g. device
> + * timestamp).
> + */
> +#define ES58X_CHANNEL_IDX_NA 0xFF
> +#define ES58X_EMPTY_MSG NULL
> +
> +/* Threshold on consecutive CAN_STATE_ERROR_PASSIVE. If we receive
> + * ES58X_CONSECUTIVE_ERR_PASSIVE_MAX times the event
> + * ES58X_ERR_CRTL_PASSIVE in a row without any successful Rx or Tx,
> + * we force the device to switch to CAN_STATE_BUS_OFF state.
> + */
> +#define ES58X_CONSECUTIVE_ERR_PASSIVE_MAX 254

Does the device recover from bus off automatically or why is this needed?

> +
> +enum es58x_self_reception_mode {
> +	ES58X_SELF_RECEPTION_OFF = 0,
> +	ES58X_SELF_RECEPTION_ON = 1
> +};

nitpick: can you name all enums (here and below) according to the type?

e.g. ES58x_SELF_RECEPTION_MODE_OFF

> +
> +enum es58x_physical_media {
> +	ES58X_MEDIA_HIGH_SPEED = 1,
> +	ES58X_MEDIA_FAULT_TOLERANT = 2

You mean with FAULT_TOLERANT you mean ISO 11898-3? According to [1] they should
be named low speed.

[1]
https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/

> +};
> +
> +enum es58x_samples_per_bit {
> +	ES58X_ONE_SAMPLE_PER_BIT = 1,
> +
> +	/* Some CAN controllers do not support three samples per
> +	 * bit. In this case the default value of one sample per bit
> +	 * is used, even if the configuration is set to
> +	 * ES58X_THREE_SAMPLES_PER_BIT.
> +	 */

Can you autodetect the controller and avoid announcing tripple sample mode to
the driver framework?

> +	ES58X_THREE_SAMPLES_PER_BIT = 2
> +};
> +
> +enum es58x_sync_edge {
> +	/* ISO CAN specification defines the use of a single edge
> +	 * synchronization. The synchronization should be done on
> +	 * recessive to dominant level change.
> +	 */
> +	ES58X_SINGLE_SYNC_EDGE = 1,
> +
> +	/* In addition to the ISO CAN specification, a double
> +	 * synchronization is also supported: recessive to dominant
> +	 * level change and dominant to recessive level change.
> +	 */
> +	ES58X_DUAL_SYNC_EDGE = 2

We don't have a setting in the CAN framework for this....

> +};
> +
> +/**
> + * enum es58x_flag_type - CAN flags for RX/TX messages.
> + * @ES58X_FLAG_EFF: Extended Frame Format (EFF).
> + * @ES58X_FLAG_RTR: Remote Transmission Request (RTR).
> + * @ES58X_FLAG_SELFRECEPTION: The message is a Self reception frame
> + *	(not used yet in this implementation).
> + * @ES58X_FLAG_FD_BRS: Bit rate switch (BRS): second bitrate for
> + *	payload data.
> + * @ES58X_FLAG_FD_MSG_TRUNCATED: FD message was truncated and padded
> + *	(not used).
> + * @ES58X_FLAG_FD_ESI: Error State Indicator (ESI): tell if the
> + *	transmitting node is in error passive mode.
> + * @ES58X_FLAG_FD_DATA: CAN FD frame.
> + */
> +enum es58x_flag_type {
> +	ES58X_FLAG_EFF = BIT(0),
> +	ES58X_FLAG_RTR = BIT(1),
> +	ES58X_FLAG_SELFRECEPTION = BIT(2),
> +	ES58X_FLAG_FD_BRS = BIT(3),
> +	ES58X_FLAG_FD_MSG_TRUNCATED = BIT(4),
> +	ES58X_FLAG_FD_ESI = BIT(5),
> +	ES58X_FLAG_FD_DATA = BIT(6)
> +};
> +
> +/**
> + * enum es58x_error - CAN error detection.
> + * @ES58X_ERR_OK: No errors.
> + * @ES58X_ERR_PROT_STUFF: Bit stuffing error: more than 5 consecutive
> + *	equal bits.
> + * @ES58X_ERR_PROT_FORM: Frame format error.
> + * @ES58X_ERR_ACK: Received no ACK on transmission.
> + * @ES58X_ERR_PROT_BIT: Single bit error.
> + * @ES58X_ERR_PROT_CRC: Incorrect 15, 17 or 21 bits CRC.
> + * @ES58X_ERR_PROT_BIT1: Unable to send recessive bit: tried to send
> + *	recessive bit 1 but monitored dominant bit 0.
> + * @ES58X_ERR_PROT_BIT0: Unable to send dominant bit: tried to send
> + *	dominant bit 0 but monitored recessive bit 1.
> + * @ES58X_ERR_PROT_OVERLOAD: Bus overload.
> + * @ES58X_ERR_PROT_UNSPEC: Unspecified.
> + *
> + * Please refer to ISO 11898-1:2015, section 10.11 "Error detection"
> + * and section 10.13 "Overload signaling" for additional details.
> + */
> +enum es58x_error {
> +	ES58X_ERR_OK = 0,
> +	ES58X_ERR_PROT_STUFF = BIT(0),
> +	ES58X_ERR_PROT_FORM = BIT(1),
> +	ES58X_ERR_ACK = BIT(2),
> +	ES58X_ERR_PROT_BIT = BIT(3),
> +	ES58X_ERR_PROT_CRC = BIT(4),
> +	ES58X_ERR_PROT_BIT1 = BIT(5),
> +	ES58X_ERR_PROT_BIT0 = BIT(6),
> +	ES58X_ERR_PROT_OVERLOAD = BIT(7),
> +	ES58X_ERR_PROT_UNSPEC = BIT(31)
> +};
> +
> +/**
> + * enum es58x_event - CAN error codes returned by the device.
> + * @ES58X_EVENT_OK: No errors.
> + * @ES58X_ERR_CRTL_ACTIVE: Active state: both TR and RX error count is
> + *	less than 128.
> + * @ES58X_ERR_CRTL_PASSIVE: Passive state: either TX or RX error count
> + *	is greater than 127.
> + * @ES58X_ERR_CRTL_WARNING: Warning state: either TX or RX error count
> + *	is greater than 96.
> + * @ES58X_ERR_BUSOFF: Bus off.
> + * @ES58X_ERR_SINGLE_WIRE: Lost connection on either CAN high or CAN low.
> + *
> + * Please refer to ISO 11898-1:2015, section 12.1.4 "Rules of fault
> + * confinement" for additional details.
> + */
> +enum es58x_event {
> +	ES58X_EVENT_OK = 0,
> +	ES58X_ERR_CRTL_ACTIVE = BIT(0),
> +	ES58X_ERR_CRTL_PASSIVE = BIT(1),
> +	ES58X_ERR_CRTL_WARNING = BIT(2),
> +	ES58X_ERR_BUSOFF = BIT(3),
> +	ES58X_ERR_SINGLE_WIRE = BIT(4)
> +};
> +
> +/**
> + * enum es58x_dev_ret_code_u8 - Device error codes, 8 bit format.
> + *
> + * Specific to ES581.4.
> + */
> +enum es58x_dev_ret_code_u8 {
> +	ES58X_RET_U8_OK = 0x00,
> +	ES58X_RET_U8_ERR_UNSPECIFIED_FAILURE = 0x80,
> +	ES58X_RET_U8_ERR_NO_MEM = 0x81,
> +	ES58X_RET_U8_ERR_BAD_CRC = 0x99
> +};
> +
> +/**
> + * enum es58x_dev_ret_code_u32 - Device error codes, 32 bit format.
> + */
> +enum es58x_cmd_ret_code_u32 {
> +	ES58X_RET_U32_OK = 0x00000000UL,
> +	ES58X_RET_U32_ERR_UNSPECIFIED_FAILURE = 0x80000000UL,
> +	ES58X_RET_U32_ERR_NO_MEM = 0x80004001UL,
> +	ES58X_RET_U32_WARN_PARAM_ADJUSTED = 0x40004000UL,
> +	ES58X_RET_U32_WARN_TX_MAYBE_REORDER = 0x40004001UL,
> +	ES58X_RET_U32_ERR_TIMEOUT = 0x80000008UL,
> +	ES58X_RET_U32_ERR_FIFO_FULL = 0x80003002UL,
> +	ES58X_RET_U32_ERR_BAD_CONFIG = 0x80004000UL,
> +	ES58X_RET_U32_ERR_NO_RESOURCE = 0x80004002UL
> +};
> +
> +enum es58x_cmd_ret_type {
> +	ES58X_RET_TYPE_SET_BITTIMING,
> +	ES58X_RET_TYPE_ENABLE_CHANNEL,
> +	ES58X_RET_TYPE_DISABLE_CHANNEL,
> +	ES58X_RET_TYPE_TX_MSG,
> +	ES58X_RET_TYPE_RESET_RX,
> +	ES58X_RET_TYPE_RESET_TX,
> +	ES58X_RET_TYPE_DEVICE_ERR_FRAME,
> +	ES58X_CMD_RET_TYPE_NUM_ENTRIES
> +};
> +
> +/**
> + * struct es58x_abstracted_can_frame - Common structure to hold can
> + *	frame information.

why do you have an itermediate can frame format? We have the struct can_frame
and the skb for this.

> + * @timestamp: Hardware time stamp (only relevant in rx branches).
> + * @data: CAN payload.
> + * @can_id: CAN ID.
> + * @is_can_fd: false: non-FD CAN, true: CAN-FD.
> + * @flags: Please refer to enum es58x_flag_type.
> + * @dlc: Data Length Code (raw value). When using standard (non-FD)
> + *	CAN, ES58X devices allow to send DLC bigger than 8
> + *	(i.e. values 9 to 15 reserved for CAN-FD) as specified in ISO
> + *	11898-1:2015 section 8.4.2.4 "DLC field". In such case, the
> + *	@dlc field will contain whatever value was obtained when
> + *	sending or receiving but the @len field will contain the
> + *	sanitized length of the @data field (i.e. not more than
> + *	CAN_MAX_DLEN for standard CAN). To be able to send/receive
> + *	such "out of range" DLC values, you would need to modify the
> + *	first occurrence of the if conditions "cfd->len >
> + *	CAN_MAX_DLEN" into "cfd->len > CANFD_MAX_DLC" in functions
> + *	net/can/af_can.c:can_send() and net/can/af_can.c:can_rcv().
> + * @len: Length of @data field (sanitized in es58x_core.c).
> + *
> + * ES581.4 and the ES58X FD family uses different tx_can_msg
> + * structures (same fields but in different order). This abstracted
> + * structure allows to calculate the parameters once for all. The
> + * specific functions of each device model can then use the
> + * pre-computed information from this abstracted can frame.
> + */
> +struct es58x_abstracted_can_frame {
> +	u64 timestamp;
> +	const u8 *data;
> +	canid_t can_id;
> +	bool is_can_fd;
> +	u8 flags;
> +	u8 dlc;
> +	u8 len;
> +};
> +
> +union es58x_urb_cmd {
> +	u8 raw_cmd[0];

I have to polish my C, what's an empty array in the beginning of a struct?

> +	struct es581_4_urb_cmd es581_4_urb_cmd;
> +	struct es58x_fd_urb_cmd es58x_fd_urb_cmd;
> +	struct {		// Common header parts of all variants

no // comments please

> +		__le16 sof;
> +		u8 cmd_type;
> +		u8 cmd_id;
> +	} __packed;
> +};
> +
> +/**
> + * struct es58x_priv - All information specific to a can channel.
> + * @can: struct can_priv must be the first member (Socket CAN relies
> + *	on the fact that function netdev_priv() returns a pointer to
> + *	a struct can_priv).
> + * @es58x_dev: pointer to the corresponding ES58X device.
> + * @echo_skb_spinlock: Spinlock to protect the access to the echo skb
> + *	FIFO.
> + * @current_packet_idx: Keeps track of the packet indexes.
> + * @echo_skb_tail_idx: beginning of the echo skb FIFO, i.e. index of
> + *	the first element.
> + * @echo_skb_head_idx: end of the echo skb FIFO plus one, i.e. first
> + *	free index.
> + * @num_echo_skb: actual number of elements in the FIFO. Thus, the end
> + *	of the FIFO is echo_skb_head = (echo_skb_tail_idx +
> + *	num_echo_skb) % can.echo_skb_max.
> + * @tx_urb: Used as a buffer to concatenate the TX messages and to do
> + *	a bulk send. Please refer to es58x_start_xmit() for more
> + *	details.
> + * @tx_can_msg_is_fd: false: all messages in @tx_urb are non-FD CAN,
> + *	true: all messages in @tx_urb are CAN-FD. Rationale: ES58X FD
> + *	devices do not allow to mix standard and FD CAN in one single
> + *	bulk transmission.
> + * @tx_can_msg_cnt: Number of messages in @tx_urb.
> + * @err_passive_before_rtx_success: The ES58X device might enter in a
> + *	state in which it keeps alternating between error passive
> + *	and active state. This counter keeps track of the number of
> + *	error passive and if it gets bigger than
> + *	ES58X_CONSECUTIVE_ERR_PASSIVE_MAX, es58x_rx_err_msg() will
> + *	force the status to bus-off.

Is this a bug or a feature?

> + * @channel_idx: Channel index, starts at zero.
> + */
> +struct es58x_priv {
> +	struct can_priv can;
> +	struct es58x_device *es58x_dev;
> +
> +	spinlock_t echo_skb_spinlock;	// Comments: c.f. supra

please no // comments

> +	u32 current_packet_idx;
> +	u16 echo_skb_tail_idx;
> +	u16 echo_skb_head_idx;
> +	u16 num_echo_skb;
> +
> +	struct urb *tx_urb;
> +	bool tx_can_msg_is_fd;
> +	u8 tx_can_msg_cnt;
> +
> +	u8 err_passive_before_rtx_success;
> +
> +	u8 channel_idx;
> +};
> +
> +/**
> + * struct es58x_parameters - Constant parameters of a given hardware
> + *	variant.
> + * @can_bittiming_const: Nominal bittimming parameters (used for
> + *	non-FD CAN and arbitration field of CAN-FD).
> + * @data_bittiming_const: Data bittiming parameters (used for CAN-FD
> + *	payload)
> + * @bitrate_max: Maximum bitrate supported by the device.
> + * @clock: CAN clock parameters.
> + * @ctrlmode_supported: List of supported modes. Please refer to
> + *	can/netlink.h file for additional details.
> + * @tx_start_of_frame: Magic number at the beginning of each TX URB
> + *	command.
> + * @rx_start_of_frame: Magic number at the beginning of each RX URB
> + *	command.
> + * @tx_urb_cmd_max_len: Maximum length of a TX URB command.
> + * @rx_urb_cmd_max_len: Maximum length of a RX URB command.
> + * @echo_skb_max: Maximum number of echo SKB. This value must not
> + *	exceed the maximum size of the device internal TX FIFO
> + *	length. This parameter is used to control the network queue
> + *	wake/stop logic.
> + * @dql_limit_min: Dynamic Queue Limits (DQL) absolute minimum limit
> + *	of bytes allowed to be queued on this network device transmit
> + *	queue. Used by the Byte Queue Limits (BQL) to determine how
> + *	frequently the xmit_more flag will be set to true in
> + *	es58x_start_xmit(). Set this value higher to optimize for
> + *	throughput but be aware that it might have a negative impact
> + *	on the latency! This value can also be set dynamically. Please
> + *	refer to Documentation/ABI/testing/sysfs-class-net-queues for
> + *	more details.
> + * @tx_bulk_max: Maximum number of Tx messages that can be sent in one
> + *	single URB packet.
> + * @urb_cmd_header_len: Length of the URB command header.
> + * @rx_urb_max: Number of RX URB to be allocated during device probe.
> + * @tx_urb_max: Number of TX URB to be allocated during device probe.
> + * @channel_idx_offset: Some of the ES58x starts channel numbering
> + *	from 0 (ES58X FD), others from 1 (ES581.4).
> + */
> +struct es58x_parameters {
> +	const struct can_bittiming_const *bittiming_const;
> +	const struct can_bittiming_const *data_bittiming_const;
> +	u32 bitrate_max;
> +	struct can_clock clock;
> +	u32 ctrlmode_supported;
> +	u16 tx_start_of_frame;
> +	u16 rx_start_of_frame;
> +	u16 tx_urb_cmd_max_len;
> +	u16 rx_urb_cmd_max_len;
> +	u16 echo_skb_max;
> +	u16 dql_limit_min;
> +	u8 tx_bulk_max;
> +	u8 urb_cmd_header_len;
> +	u8 rx_urb_max;
> +	u8 tx_urb_max;
> +	u8 channel_idx_offset;
> +};
> +
> +/**
> + * struct es58x_operators - Function pointers used to encode/decode
> + *	the TX/RX messages.
> + * @get_msg_len: Get field msg_len of the urb_cmd. The offset of
> + *	msg_len inside urb_cmd depends of the device model.
> + * @handle_urb_cmd: Handle URB command received from the device and
> + *	manage the return code from device.
> + * @fill_urb_header: Fill the header of urb_cmd.
> + * @tx_can_msg: Encode a TX CAN message and add it to the bulk buffer
> + *	cmd_buf of es58x_dev.
> + * @set_bittiming: Encode the bittiming information and send it.
> + * @enable_channel: Start the CAN channel with index channel_idx.
> + * @disable_channel: Stop the CAN channel with index channel_idx.
> + * @reset_rx: Reset the RX queue of the ES58X device.
> + * @reset_tx: Reset the TX queue of the ES58X device.
> + * @get_timestamp: Request a timestamp from the ES58X device.
> + */
> +struct es58x_operators {
> +	u16 (*get_msg_len)(const union es58x_urb_cmd *urb_cmd);
> +	int (*handle_urb_cmd)(struct es58x_device *es58x_dev,
> +			      const union es58x_urb_cmd *urb_cmd);
> +	void (*fill_urb_header)(union es58x_urb_cmd *urb_cmd, u8 cmd_type,
> +				u8 cmd_id, u8 channel_idx, u16 cmd_len);
> +	int (*tx_can_msg)(struct es58x_device *es58x_dev, int channel_idx,
> +			  u32 packet_idx,
> +			   struct es58x_abstracted_can_frame *es58x_frame,
> +			   union es58x_urb_cmd *urb_cmd, u32 *urb_cmd_len);
> +	int (*set_bittiming)(struct es58x_device *es58x_dev, int channel_idx);
> +	int (*enable_channel)(struct es58x_device *es58x_dev,
> +			      int channel_idx);
> +	int (*disable_channel)(struct es58x_device *es58x_dev,
> +			       int channel_idx);
> +	int (*reset_rx)(struct es58x_device *es58x_dev, int channel_idx);
> +	int (*reset_tx)(struct es58x_device *es58x_dev, int channel_idx);
> +	int (*get_timestamp)(struct es58x_device *es58x_dev);
> +};
> +
> +/**
> + * struct es58x_device - All information specific to an ES58X device.
> + * @dev: Device information.
> + * @udev: USB device information.
> + * @netdev: Array of our CAN channels.
> + * @param: The constant parameters.
> + * @ops: Operators.
> + * @rx_pipe: USB reception pipe.
> + * @tx_pipe: USB transmission pipe.
> + * @rx_urbs: Anchor for received URBs.
> + * @tx_urbs_busy: Anchor for TX URBs which were send to the device.
> + * @tx_urbs_idle: Anchor for TX USB which are idle. This driver
> + *	allocates the memory for the URBs during the probe. When a TX
> + *	URB is needed, it can be taken from this anchor. The network
> + *	queue wake/stop logic should prevent this URB from getting
> + *	empty. Please refer to es58x_get_tx_urb() for more details.
> + * @tx_urbs_idle_cnt: number of urbs in @tx_urbs_idle.
> + * @ktime_req_ns: kernel timestamp when es58x_set_realtime_diff_ns()
> + *	was called.
> + * @realtime_diff_ns: difference in nanoseconds between the clocks of
> + *	the ES58X device and the kernel.
> + * @timestamps: a temporary buffer to store the time stamps before
> + *	feeding them to es58x_can_get_echo_skb(). Can only be used
> + *	in rx branches.
> + * @can_frames: a temporary buffer to store the can frames before
> + *	feeding them to es58x_rx_can_msg(). Can only be used in rx
> + *	branches.
> + * @rx_max_packet_size: Maximum length of bulk-in URB.
> + * @num_can_ch: Number of CAN channel (i.e. number of elements of @netdev).
> + * @rx_cmd_buf_len: Length of @rx_cmd_buf.
> + * @rx_cmd_buf: The device might split the URB commands in an
> + *	arbitrary amount of pieces. This buffer is used to concatenate
> + *	all those pieces. Can only be used in rx branches. This field
> + *	has to be the last one of the structure because it is has a
> + *	flexible size (c.f. ES58X_SIZEOF_ES58X_DEVICE() macro).
> + */
> +struct es58x_device {
> +	struct device *dev;
> +	struct usb_device *udev;
> +	struct net_device *netdev[ES58X_NUM_CAN_CH_MAX];
> +
> +	const struct es58x_parameters *param;
> +	const struct es58x_operators *ops;
> +
> +	int rx_pipe;
> +	int tx_pipe;
> +
> +	struct usb_anchor rx_urbs;
> +	struct usb_anchor tx_urbs_busy;
> +	struct usb_anchor tx_urbs_idle;
> +	atomic_t tx_urbs_idle_cnt;
> +
> +	u64 ktime_req_ns;
> +	s64 realtime_diff_ns;
> +
> +	union {
> +		u64 timestamps[ES58X_RX_LOOPBACK_BULK_MAX];
> +		struct es58x_abstracted_can_frame can_frames[ES58X_RX_BULK_MAX];
> +	};
> +
> +	u16 rx_max_packet_size;
> +	u8 num_can_ch;
> +
> +	u16 rx_cmd_buf_len;
> +	union es58x_urb_cmd rx_cmd_buf;
> +};
> +
> +/**
> + * ES58X_SIZEOF_ES58X_DEVICE() - Calculate the maximum length of
> + *	struct es58x_device.
> + * @es58x_dev_param: The constant parameters of the device.
> + *
> + * The length of struct es58x_device depends on the length of its last
> + * field: rx_cmd_buf. This macro allows to optimize size for memory
> + * allocation.
> + *
> + * Return: length of struct es58x_device.
> + */
> +#define ES58X_SIZEOF_ES58X_DEVICE(es58x_dev_param)			\
> +	(offsetof(struct es58x_device, rx_cmd_buf) +			\
> +		(es58x_dev_param)->rx_urb_cmd_max_len)

can this be made a static inline?

> +
> +/* Megabit per second (multiply x per one million) */
> +#define ES58X_MBPS(x) (1000000UL * (x))
> +/* Megahertz (multiply x per one million) */
> +#define ES58X_MHZ(x)  (1000000UL * (x))
> +
> +/**
> + * es58x_check_msg_len() - Check the size of a received message.
> + * @dev: Device, used to print error messages.
> + * @msg: Received message, must not be a pointer.
> + * @actual_len: Length of the message as advertised in the command header.
> + *
> + * Must be a macro in order to retrieve the actual size using
> + * sizeof(). Can be use with any of the messages which have a fixed
> + * length. Check for an exact match of the size.

You can provide an outer macro that does the sizeof() and then calls the a
normal (static inline) function to do the actual work. Applied to the next 3 macros.

> + *
> + * Return: zero on success, -EMSGSIZE if @actual_len differs from the
> + * expected length.
> + */
> +#define es58x_check_msg_len(dev, msg, actual_len)			\
> +({									\
> +	size_t __expected_len = sizeof(msg);				\
> +	size_t __actual_len = (actual_len);				\
> +	int __res = 0;							\
> +	if (__expected_len != __actual_len) {				\
> +		dev_err(dev,						\
> +			"%s: Length of %s is %zu but received command is %zu.\n",		\
> +			__func__, __stringify(msg),			\
> +			__expected_len, __actual_len);			\
> +		__res = -EMSGSIZE;					\
> +	}								\
> +	__res;								\
> +})
> +
> +/**
> + * es58x_check_msg_max_len() - Check the maximum size of a received message.
> + * @dev: Device, used to print error messages.
> + * @msg: Received message, must not be a pointer.
> + * @actual_len: Length of the message as advertised in the command header.
> + *
> + * Must be a macro in order to retrieve the actual size using
> + * sizeof(). To be used with the messages of variable sizes. Only
> + * check that the message is not bigger than the maximum expected
> + * size.
> + *
> + * Return: zero on success, -EOVERFLOW if @actual_len is greater than
> + * the expected length.
> + */
> +#define es58x_check_msg_max_len(dev, msg, actual_len)			\
> +({									\
> +	size_t __actual_len = (actual_len);				\
> +	size_t __expected_len = sizeof(msg);				\
> +	int __res = 0;							\
> +	if (__actual_len > __expected_len) {				\
> +		dev_err(dev,						\
> +			"%s: Maximum length for %s is %zu but received command is %zu.\n",	\
> +			__func__, __stringify(msg),			\
> +			__expected_len, __actual_len);			\
> +		__res = -EOVERFLOW;					\
> +	}								\
> +	__res;								\
> +})
> +
> +/**
> + * es58x_msg_num_element() - Check size and give the number of
> + *	elements in a message of array type.
> + * @dev: Device, used to print error messages.
> + * @msg: Received message, must be an array.
> + * @actual_len: Length of the message as advertised in the command
> + *	header.
> + *
> + * Must be a macro in order to retrieve the actual size using
> + * sizeof(). To be used on message of array type. Array's element has
> + * to be of fixed size (else use es58x_check_msg_max_len()). Check
> + * that the total length is an exact multiple of the length of a
> + * single element.
> + *
> + * Return: number of elements in the array on success, -EOVERFLOW if
> + * @actual_len is greater than the expected length, -EMSGSIZE if
> + * @actual_len is not a multiple of a single element.
> + */
> +#define es58x_msg_num_element(dev, msg, actual_len)			\
> +({									\
> +	const struct device *__dev = (dev);				\
> +	size_t __actual_len = (actual_len);				\
> +	size_t __elem_len = sizeof((msg)[0]) + __must_be_array(msg);	\
> +	size_t __actual_num_elem = __actual_len / __elem_len;		\
> +	size_t __expected_num_elem = sizeof(msg) / __elem_len;		\
> +	int __res = __actual_num_elem;					\
> +	if (__actual_num_elem == 0) {					\
> +		dev_err(__dev,						\
> +			"%s: Minimum length for %s is %zu but received command is %zu.\n",	\
> +			__func__, __stringify(msg),			\
> +			__elem_len, __actual_len);			\
> +		__res = -EMSGSIZE;					\
> +	} else if ((__actual_len % __elem_len) != 0) {			\
> +		dev_err(__dev,						\
> +			"%s: Received command length: %zu is not a multiple of %s[0]: %zu\n",	\
> +			__func__, __actual_len,				\
> +			__stringify(msg), __elem_len);			\
> +		__res = -EMSGSIZE;					\
> +	} else if (__actual_num_elem >	__expected_num_elem) {		\
> +		dev_err(__dev,						\
> +			"%s: Array %s is supposed to have %zu elements each of size %zu...\n",	\
> +			__func__, __stringify(msg),			\
> +			__expected_num_elem, __elem_len);		\
> +		dev_err(__dev,						\
> +			"... But received command has %zu elements (total length %zu).\n",	\
> +			__actual_num_elem, __actual_len);		\
> +		__res = -EOVERFLOW;					\
> +	}								\
> +	__res;								\
> +})
> +
> +/**
> + * es58x_priv() - Get the priv member and cast it to struct es58x_priv.
> + * @netdev: CAN network device.
> + *
> + * Return: ES58X device.
> + */
> +static inline struct es58x_priv *es58x_priv(struct net_device *netdev)
> +{
> +	return (struct es58x_priv *)netdev_priv(netdev);
> +}
> +
> +/**
> + * ES58X_SIZEOF_URB_CMD() - Calculate the maximum length of an urb
> + *	command for a given message field name.
> + * @es58x_urb_cmd_type: type (either "struct es581_4_urb_cmd" or
> + *	"struct es58x_fd_urb_cmd").
> + * @msg_field: name of the message field.
> + *
> + * Return: length of the urb command.
> + */
> +#define ES58X_SIZEOF_URB_CMD(es58x_urb_cmd_type, msg_field)		\
> +	(offsetof(es58x_urb_cmd_type, raw_msg)				\
> +		+ sizeof_field(es58x_urb_cmd_type, msg_field)		\
> +		+ sizeof_field(es58x_urb_cmd_type,			\
> +			reserved_for_crc16_do_not_use))

static inline?

> +
> +/**
> + * es58x_get_urb_cmd_len() - Calculate the actual length of an urb
> + *	command for a given message length.
> + * @es58x_dev: ES58X device.
> + * @msg_len: Length of the message.
> + *
> + * Add the header and CRC lengths to the message length.
> + *
> + * Return: length of the urb command.
> + */
> +static inline size_t es58x_get_urb_cmd_len(struct es58x_device *es58x_dev,
> +					   u16 msg_len)
> +{
> +	/* URB Length = URB header len + Message len + sizeof crc16 */
> +	return es58x_dev->param->urb_cmd_header_len + msg_len + sizeof(u16);
> +}
> +
> +/**
> + * es58x_get_netdev() - Get the network device.
> + * @es58x_dev: ES58X device.
> + * @channel_no: The channel number as advertised in the urb command.
> + * @netdev: CAN network device.
> + *
> + * ES581.4 starts the numbering on channels from 1, ES58X FD family
> + * starts it from 0. This method does the sanity check.
> + *
> + * Return: zero on success, -ECHRNG if the received channel number is
> + * out of range and -ENODEV if the network device is not yet
> + * configured.
> + */
> +static inline int es58x_get_netdev(struct es58x_device *es58x_dev,
> +				   int channel_no, struct net_device **netdev)
> +{
> +	int channel_idx = channel_no - es58x_dev->param->channel_idx_offset;
> +
> +	*netdev = NULL;
> +	if (channel_idx < 0 || channel_idx >= es58x_dev->num_can_ch)
> +		return -ECHRNG;
> +
> +	*netdev = es58x_dev->netdev[channel_idx];
> +	if (!netdev || !netif_device_present(*netdev))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +int es58x_can_get_echo_skb(struct net_device *netdev, u32 packet_idx,
> +			   u64 *tstamps, unsigned int pkts);
> +int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries,
> +		     enum es58x_cmd_ret_code_u32 rx_cmd_ret_u32);
> +int es58x_rx_can_msg(struct net_device *netdev,
> +		     struct es58x_abstracted_can_frame *es58x_cf,
> +		     unsigned int pkts);
> +int es58x_rx_err_msg(struct net_device *netdev, enum es58x_error error,
> +		     enum es58x_event event, u64 timestamp);
> +int es58x_rx_timestamp(struct es58x_device *es58x_dev, u64 timestamp);
> +int es58x_rx_dev_ret_u8(struct device *dev,
> +			enum es58x_cmd_ret_type cmd_ret_type,
> +			enum es58x_dev_ret_code_u8 rx_dev_ret_u8);
> +int es58x_rx_cmd_ret_u32(struct net_device *netdev,
> +			 enum es58x_cmd_ret_type cmd_ret_type,
> +			 enum es58x_cmd_ret_code_u32 rx_cmd_ret_u32);
> +int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
> +		   const void *msg, u16 cmd_len, int channel_idx);
> +
> +extern const struct es58x_parameters es581_4_param;
> +extern const struct es58x_operators es581_4_ops;
> +
> +extern const struct es58x_parameters es58x_fd_param;
> +extern const struct es58x_operators es58x_fd_ops;
> +
> +#endif				//__ES58X_COMMON_H__

just one space please

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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 7/7] usb: cdc-acm: add quirk to blacklist ETAS ES58X devices

[Greg Kroah-Hartman]

On Sat, Oct 03, 2020 at 12:41:51AM +0900, Vincent Mailhol wrote:
> The ES58X devices has a CDC ACM interface (used for debug
> purpose). During probing, the device is thus recognized as USB Modem
> (CDC ACM), preventing the etas-es58x module to load:
>   usbcore: registered new interface driver etas_es58x
>   usb 1-1.1: new full-speed USB device number 14 using xhci_hcd
>   usb 1-1.1: New USB device found, idVendor=108c, idProduct=0159, bcdDevice= 1.00
>   usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>   usb 1-1.1: Product: ES581.4
>   usb 1-1.1: Manufacturer: ETAS GmbH
>   usb 1-1.1: SerialNumber: 2204355
>   cdc_acm 1-1.1:1.0: No union descriptor, testing for castrated device
>   cdc_acm 1-1.1:1.0: ttyACM0: USB ACM device
> 
> Thus, these have been added to the ignore list in
> drivers/usb/class/cdc-acm.c
> 
> N.B. Future firmware release of the ES58X will remove the CDC-ACM
> interface.

I'll queue this up now, as it's needed no matter what the status of the
other patches in this series.

thanks,

greg k-h

Re: [PATCH v3 6/7] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Vincent Mailhol]

> Just one header file for now :)

Thanks for the review, very constructive comments :)

I acknowledge all the trivial fixes (space, new line, // comments,
naming...), those will be fixed in v4 (will also review other files
for similar mistakes). In this reply, I will only focus on the points
which need explanations.

v4 will come a bit later.


> > +/* Threshold on consecutive CAN_STATE_ERROR_PASSIVE. If we receive
> > + * ES58X_CONSECUTIVE_ERR_PASSIVE_MAX times the event
> > + * ES58X_ERR_CRTL_PASSIVE in a row without any successful Rx or Tx,
> > + * we force the device to switch to CAN_STATE_BUS_OFF state.
> > + */
> > +#define ES58X_CONSECUTIVE_ERR_PASSIVE_MAX 254
> 
> Does the device recover from bus off automatically or why is this needed?
> 

Will be answered below together with your other question on
@err_passive_before_rtx_success of struct es58x_priv.

> > +
> > +enum es58x_physical_media {
> > +	ES58X_MEDIA_HIGH_SPEED = 1,
> > +	ES58X_MEDIA_FAULT_TOLERANT = 2
> 
> You mean with FAULT_TOLERANT you mean ISO 11898-3? According to [1] they should
> be named low speed.

Two comments:
 1/ Yes, this is "low speed". I did not know about the ISO 11898-3,
    thanks for the hint.
 2/ After double checking, this option is not supported by the devices
    in scope of this driver (other devices of the ESxxx portfolio
    might support it).
This option will be removed in v4.

> > +};
> > +
> > +enum es58x_samples_per_bit {
> > +	ES58X_ONE_SAMPLE_PER_BIT = 1,
> > +
> > +	/* Some CAN controllers do not support three samples per
> > +	 * bit. In this case the default value of one sample per bit
> > +	 * is used, even if the configuration is set to
> > +	 * ES58X_THREE_SAMPLES_PER_BIT.
> > +	 */
> 
> Can you autodetect the controller and avoid announcing tripple sample mode to
> the driver framework?

Will be addressed in v4. Your remarks made me realized that some of
the controller modes might not have been announced correctly. Will
double check the other CAN_CTRLMODE_* as well.

> > +	ES58X_THREE_SAMPLES_PER_BIT = 2
> > +};
> > +
> > +enum es58x_sync_edge {
> > +	/* ISO CAN specification defines the use of a single edge
> > +	 * synchronization. The synchronization should be done on
> > +	 * recessive to dominant level change.
> > +	 */
> > +	ES58X_SINGLE_SYNC_EDGE = 1,
> > +
> > +	/* In addition to the ISO CAN specification, a double
> > +	 * synchronization is also supported: recessive to dominant
> > +	 * level change and dominant to recessive level change.
> > +	 */
> > +	ES58X_DUAL_SYNC_EDGE = 2
> 
> >We don't have a setting in the CAN framework for this....

The idea here was just to let know people that the option exists so
that if someone needs the feature one day, he or she can hack the
driver for his or her own use.

Is it OK to keep it (maybe with a comment such as "not implemented in
this driver") or should it be simply removed?

There are other similar references in other files. Will change these
accordingly to your answer on above question.

> > +/**
> > + * struct es58x_abstracted_can_frame - Common structure to hold can
> > + *	frame information.
> 
> why do you have an itermediate can frame format? We have the struct can_frame
> and the skb for this.

The goal of this structure was to factorize code when calculating the
CAN flags. I will try to rethink this part in v4.

> > +union es58x_urb_cmd {
> > +	u8 raw_cmd[0];
> 
> I have to polish my C, what's an empty array in the beginning of a struct?

This is not a struct but a union (it would indeed make no sense at the
beginning of a struct).

Because it is in a union, the order of the fields does not make a
difference (if you prefer this to be at the end, I can fix it).

This field is used to cast the union to an u8 array. Because the
length is unknown it is declared as empty.

For reference, I could at least find a few other references of union
starting with an empty array in the kernel. One example here:
https://elixir.bootlin.com/linux/latest/source/include/linux/bpf.h#L821

> 
> > +/**
> > + * struct es58x_priv - All information specific to a can channel.
> > + * @can: struct can_priv must be the first member (Socket CAN relies
> > + *	on the fact that function netdev_priv() returns a pointer to
> > + *	a struct can_priv).
> > + * @es58x_dev: pointer to the corresponding ES58X device.
> > + * @echo_skb_spinlock: Spinlock to protect the access to the echo skb
> > + *	FIFO.
> > + * @current_packet_idx: Keeps track of the packet indexes.
> > + * @echo_skb_tail_idx: beginning of the echo skb FIFO, i.e. index of
> > + *	the first element.
> > + * @echo_skb_head_idx: end of the echo skb FIFO plus one, i.e. first
> > + *	free index.
> > + * @num_echo_skb: actual number of elements in the FIFO. Thus, the end
> > + *	of the FIFO is echo_skb_head = (echo_skb_tail_idx +
> > + *	num_echo_skb) % can.echo_skb_max.
> > + * @tx_urb: Used as a buffer to concatenate the TX messages and to do
> > + *	a bulk send. Please refer to es58x_start_xmit() for more
> > + *	details.
> > + * @tx_can_msg_is_fd: false: all messages in @tx_urb are non-FD CAN,
> > + *	true: all messages in @tx_urb are CAN-FD. Rationale: ES58X FD
> > + *	devices do not allow to mix standard and FD CAN in one single
> > + *	bulk transmission.
> > + * @tx_can_msg_cnt: Number of messages in @tx_urb.
> > + * @err_passive_before_rtx_success: The ES58X device might enter in a
> > + *	state in which it keeps alternating between error passive
> > + *	and active state. This counter keeps track of the number of
> > + *	error passive and if it gets bigger than
> > + *	ES58X_CONSECUTIVE_ERR_PASSIVE_MAX, es58x_rx_err_msg() will
> > + *	force the status to bus-off.
> 
> Is this a bug or a feature?

This is a bug of the device.

Rationale: According to ISO 11898-1, paragraph 12.1.4.2 "Error
counting", the two only possible scenarios to decrements the error
counter are "After the successful transmission of a frame" (paragraph
g) and "After the successful reception of a frame" (paragraph h).

Here, the device switch from error passive state to error active state
without any successful Tx or Rx of a frame. This means that the error
counter does not behave as stipulated in the ISO.

When the issue occurs, the only solution would be to set down the
network. Forcing the bus off allows at least the user to recover (with
restart or restart-ms).

For information, this issue was only witnessed when the device is
trying to send frames on a bus which is already at 100% load.

Example to reproduce: have can0 and can1 on the same bus and do:
cangen -g0 can0
cangen -g0 can1

Note: development team of the device's firmware was informed of this
issue and will consider how to fix it.

> > +#define ES58X_SIZEOF_ES58X_DEVICE(es58x_dev_param)			\
> > +	(offsetof(struct es58x_device, rx_cmd_buf) +			\
> > +		(es58x_dev_param)->rx_urb_cmd_max_len)
> 
> can this be made a static inline?

Yes. Will be fixed in v4.

> > + * Must be a macro in order to retrieve the actual size using
> > + * sizeof(). Can be use with any of the messages which have a fixed
> > + * length. Check for an exact match of the size.
> 
> You can provide an outer macro that does the sizeof() and then calls the a
> normal (static inline) function to do the actual work. Applied to the next 3 macros.

OK. Will be fixed in v4.

> > +#define ES58X_SIZEOF_URB_CMD(es58x_urb_cmd_type, msg_field)		\
> > +	(offsetof(es58x_urb_cmd_type, raw_msg)				\
> > +		+ sizeof_field(es58x_urb_cmd_type, msg_field)		\
> > +		+ sizeof_field(es58x_urb_cmd_type,			\
> > +			reserved_for_crc16_do_not_use))
> 
> static inline?

Sorry but this one can not be converted into a static inline: the
first argument is a type (that will become the first argument of
offsetof() and sizeof_field()).


One more time, thank you for your time and your review!

Yours sincerely,
Vincent Mailhol