| Message ID | 20210112130538.14912-1-mailhol.vincent@wanadoo.fr |
|---|---|
| Headers | show |
| Series | add support for ETAS ES58X CAN USB interfaces | expand |
On Tue. 12 Jan 2021 at 22:05, Vincent Mailhol <mailhol.vincent@wanadoo.fr> 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> > --- > Something strange is going on with the mailing list. I can not see the second patch (1/1) in the *linux-can* mailing archive (only the cover letter is present): https://lore.kernel.org/linux-can/20210112130538.14912-1-mailhol.vincent@wanadoo.fr/T/# However, the full patch series is present on the *netdev* mailing archives: https://lore.kernel.org/netdev/20210112130538.14912-2-mailhol.vincent@wanadoo.fr/ Are there any restrictions in regard to the patch size on the linux-can mailing list? Or did I did anything wrong which triggered some kind of spam filter? Yours sincerely, Vincent
On 1/12/21 2:05 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> [...] > diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c > new file mode 100644 > index 000000000000..30692d78d8e6 > --- /dev/null > +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c > @@ -0,0 +1,2589 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces. > + * > + * File es58x_core.c: Core logic to manage the network devices and the > + * USB interface. > + * > + * Copyright (C) 2019 Robert Bosch Engineering and Business > + * Solutions. All rights reserved. > + * Copyright (C) 2020 ETAS K.K.. All rights reserved. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/usb.h> > +#include <linux/crc16.h> > +#include <linux/spinlock.h> > +#include <asm/unaligned.h> > + > +#include "es58x_core.h" > + > +#define DRV_VERSION "1.00" > +MODULE_AUTHOR("Mailhol Vincent <mailhol.vincent@wanadoo.fr>"); > +MODULE_AUTHOR("Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>"); > +MODULE_DESCRIPTION("Socket CAN driver for ETAS ES58X USB adapters"); > +MODULE_VERSION(DRV_VERSION); > +MODULE_LICENSE("GPL v2"); > + > +/* Vendor and product id */ > +#define ES58X_MODULE_NAME "etas_es58x" > +#define ES58X_VENDOR_ID 0x108C > +#define ES581_4_PRODUCT_ID 0x0159 > +#define ES582_1_PRODUCT_ID 0x0168 > +#define ES584_1_PRODUCT_ID 0x0169 > + > +/* Table of devices which work with this driver */ > +static const struct usb_device_id es58x_id_table[] = { > + {USB_DEVICE(ES58X_VENDOR_ID, ES581_4_PRODUCT_ID)}, > + {USB_DEVICE(ES58X_VENDOR_ID, ES582_1_PRODUCT_ID)}, > + {USB_DEVICE(ES58X_VENDOR_ID, ES584_1_PRODUCT_ID)}, > + {} /* Terminating entry */ > +}; > + > +MODULE_DEVICE_TABLE(usb, es58x_id_table); > + > +#define es58x_print_hex_dump(buf, len) \ > + print_hex_dump(KERN_DEBUG, \ > + ES58X_MODULE_NAME " " __stringify(buf) ": ", \ > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > + > +#define es58x_print_hex_dump_debug(buf, len) \ > + print_hex_dump_debug(ES58X_MODULE_NAME " " __stringify(buf) ": ",\ > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > + > +/* The last two bytes of an ES58X command is a CRC16. The first two > + * bytes (the start of frame) are skipped and the CRC calculation > + * starts on the third byte. > + */ > +#define ES58X_CRC_CALC_OFFSET 2 > + > +/** > + * es58x_calculate_crc() - Compute the crc16 of a given URB. > + * @urb_cmd: The URB command for which we want to calculate the CRC. > + * @urb_len: Length of @urb_cmd. Must be at least bigger than 4 > + * (ES58X_CRC_CALC_OFFSET + sizeof(crc)) > + * > + * Return: crc16 value. > + */ > +static u16 es58x_calculate_crc(const union es58x_urb_cmd *urb_cmd, u16 urb_len) > +{ > + u16 crc; > + ssize_t len = urb_len - ES58X_CRC_CALC_OFFSET - sizeof(crc); > + > + WARN_ON(len < 0); Is it possible to ensure earlier, that the urbs are of correct length? > + crc = crc16(0, &urb_cmd->raw_cmd[ES58X_CRC_CALC_OFFSET], len); > + return crc; > +} [...] > +/** > + * 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. > + * @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. > + * @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_total_frame_len: sum, in bytes, of the length of each of the > + * CAN messages contained in @tx_urb. To be used as an input of > + * netdev_sent_queue() for BQL. > + * @tx_can_msg_cnt: Number of messages in @tx_urb. > + * @tx_can_msg_is_fd: false: all messages in @tx_urb are Classical > + * CAN, true: all messages in @tx_urb are CAN FD. Rationale: > + * ES58X FD devices do not allow to mix Classical CAN and FD CAN > + * frames in one single bulk transmission. > + * @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. > + * @channel_idx: Channel index, starts at zero. > + */ > +struct es58x_priv { > + struct can_priv can; > + struct es58x_device *es58x_dev; > + struct urb *tx_urb; > + > + spinlock_t echo_skb_spinlock; /* Comments: c.f. supra */ > + u32 current_packet_idx; > + u16 echo_skb_tail_idx; > + u16 echo_skb_head_idx; > + u16 num_echo_skb; Can you explain me how the tx-path works, especially why you need the current_packet_idx. In the mcp251xfd driver, the number of TX buffers is a power of two, that makes things easier. tx_heads % len points to the next buffer to be filled, tx_tail % len points to the next buffer to be completed. tx_head - tx_tail is the fill level of the FIFO. This works without spinlocks. > + > + u16 tx_total_frame_len; > + u8 tx_can_msg_cnt; > + bool tx_can_msg_is_fd; > + > + u8 err_passive_before_rtx_success; > + > + u8 channel_idx; > +}; 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 |
Hi Marc, Thanks for the comments! On Wed. 13 Jan 2021 à 18:33, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 1/12/21 2:05 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> > > [...] > > > diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c > > new file mode 100644 > > index 000000000000..30692d78d8e6 > > --- /dev/null > > +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c > > @@ -0,0 +1,2589 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces. > > + * > > + * File es58x_core.c: Core logic to manage the network devices and the > > + * USB interface. > > + * > > + * Copyright (C) 2019 Robert Bosch Engineering and Business > > + * Solutions. All rights reserved. > > + * Copyright (C) 2020 ETAS K.K.. All rights reserved. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/moduleparam.h> > > +#include <linux/usb.h> > > +#include <linux/crc16.h> > > +#include <linux/spinlock.h> > > +#include <asm/unaligned.h> > > + > > +#include "es58x_core.h" > > + > > +#define DRV_VERSION "1.00" > > +MODULE_AUTHOR("Mailhol Vincent <mailhol.vincent@wanadoo.fr>"); > > +MODULE_AUTHOR("Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>"); > > +MODULE_DESCRIPTION("Socket CAN driver for ETAS ES58X USB adapters"); > > +MODULE_VERSION(DRV_VERSION); > > +MODULE_LICENSE("GPL v2"); > > + > > +/* Vendor and product id */ > > +#define ES58X_MODULE_NAME "etas_es58x" > > +#define ES58X_VENDOR_ID 0x108C > > +#define ES581_4_PRODUCT_ID 0x0159 > > +#define ES582_1_PRODUCT_ID 0x0168 > > +#define ES584_1_PRODUCT_ID 0x0169 > > + > > +/* Table of devices which work with this driver */ > > +static const struct usb_device_id es58x_id_table[] = { > > + {USB_DEVICE(ES58X_VENDOR_ID, ES581_4_PRODUCT_ID)}, > > + {USB_DEVICE(ES58X_VENDOR_ID, ES582_1_PRODUCT_ID)}, > > + {USB_DEVICE(ES58X_VENDOR_ID, ES584_1_PRODUCT_ID)}, > > + {} /* Terminating entry */ > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, es58x_id_table); > > + > > +#define es58x_print_hex_dump(buf, len) \ > > + print_hex_dump(KERN_DEBUG, \ > > + ES58X_MODULE_NAME " " __stringify(buf) ": ", \ > > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > > + > > +#define es58x_print_hex_dump_debug(buf, len) \ > > + print_hex_dump_debug(ES58X_MODULE_NAME " " __stringify(buf) ": ",\ > > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > > + > > +/* The last two bytes of an ES58X command is a CRC16. The first two > > + * bytes (the start of frame) are skipped and the CRC calculation > > + * starts on the third byte. > > + */ > > +#define ES58X_CRC_CALC_OFFSET 2 > > + > > +/** > > + * es58x_calculate_crc() - Compute the crc16 of a given URB. > > + * @urb_cmd: The URB command for which we want to calculate the CRC. > > + * @urb_len: Length of @urb_cmd. Must be at least bigger than 4 > > + * (ES58X_CRC_CALC_OFFSET + sizeof(crc)) > > + * > > + * Return: crc16 value. > > + */ > > +static u16 es58x_calculate_crc(const union es58x_urb_cmd *urb_cmd, u16 urb_len) > > +{ > > + u16 crc; > > + ssize_t len = urb_len - ES58X_CRC_CALC_OFFSET - sizeof(crc); > > + > > + WARN_ON(len < 0); > > Is it possible to ensure earlier, that the urbs are of correct length? Easy answer: it is ensured. On the Tx branch, I create the urbs so I know for sure that the length is correct. On the Rx branch, I have a dedicated function: es58x_check_rx_urb() for this purpose. I will remove that WARN_ON() and the one in es58x_get_crc(). I will also check the other WARN_ON() in my code to see if they can be removed (none on my test throughout the last ten months or so could trigger any of these WARN_ON() so should be fine to remove but I will double check). > > + crc = crc16(0, &urb_cmd->raw_cmd[ES58X_CRC_CALC_OFFSET], len); > > + return crc; > > +} > > [...] > > > +/** > > + * 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. > > + * @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. > > + * @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_total_frame_len: sum, in bytes, of the length of each of the > > + * CAN messages contained in @tx_urb. To be used as an input of > > + * netdev_sent_queue() for BQL. > > + * @tx_can_msg_cnt: Number of messages in @tx_urb. > > + * @tx_can_msg_is_fd: false: all messages in @tx_urb are Classical > > + * CAN, true: all messages in @tx_urb are CAN FD. Rationale: > > + * ES58X FD devices do not allow to mix Classical CAN and FD CAN > > + * frames in one single bulk transmission. > > + * @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. > > + * @channel_idx: Channel index, starts at zero. > > + */ > > +struct es58x_priv { > > + struct can_priv can; > > + struct es58x_device *es58x_dev; > > + struct urb *tx_urb; > > + > > + spinlock_t echo_skb_spinlock; /* Comments: c.f. supra */ > > + u32 current_packet_idx; > > + u16 echo_skb_tail_idx; > > + u16 echo_skb_head_idx; > > + u16 num_echo_skb; > > Can you explain me how the tx-path works, especially why you need the > current_packet_idx. > > In the mcp251xfd driver, the number of TX buffers is a power of two, that makes > things easier. tx_heads % len points to the next buffer to be filled, tx_tail % > len points to the next buffer to be completed. tx_head - tx_tail is the fill > level of the FIFO. This works without spinlocks. For what I understand of your explanations here are the equivalences between the etas_es58x and the mcp251xfd drivers: +--------------------+-------------------+ | etas_es58x | mcp251xfd | +--------------------+-------------------+ | current_packet_idx | tx_head | | echo_skb_tail_idx | tx_tail % len | | echo_skb_head_idx | tx_head % len | | num_echo_skb | tx_head - tx_tail | +--------------------+-------------------+ Especially, the current_packet_idx is sent to the device and returned to the driver upon completion. I wish the TX buffers were a power of two which is unfortunately not the case. The theoretical TX buffer sizes are 330 and 500 for the two devices so I wrote the code to work with those values. The exact size of the TX buffer is actually even more of a mystery because during testing both devices were unstable when using the theoretical values and I had to lower these. There is a comment at the bottom of es581_4.c and es58x_fd.c to reflect those issues. Because I do not have access to the source code of the firmware, I could not identify the root cause. My understanding is that having a queue size being a power of two is required in order not to use spinlocks (else, modulo operations would break when the index wraparound back to zero). I tried to minimize the number of spinlock: only one per bulk send or bulk receive. Yours sincerely, Vincent > > + > > + u16 tx_total_frame_len; > > + u8 tx_can_msg_cnt; > > + bool tx_can_msg_is_fd; > > + > > + u8 err_passive_before_rtx_success; > > + > > + u8 channel_idx; > > +}; > > 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 | >
On Wed. 13 Jan 2021 at 21:15, Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > > Hi Marc, > > Thanks for the comments! > > On Wed. 13 Jan 2021 à 18:33, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > On 1/12/21 2:05 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> > > > > [...] > > > > > diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c > > > new file mode 100644 > > > index 000000000000..30692d78d8e6 > > > --- /dev/null > > > +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c > > > @@ -0,0 +1,2589 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces. > > > + * > > > + * File es58x_core.c: Core logic to manage the network devices and the > > > + * USB interface. > > > + * > > > + * Copyright (C) 2019 Robert Bosch Engineering and Business > > > + * Solutions. All rights reserved. > > > + * Copyright (C) 2020 ETAS K.K.. All rights reserved. > > > + */ > > > + > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/moduleparam.h> > > > +#include <linux/usb.h> > > > +#include <linux/crc16.h> > > > +#include <linux/spinlock.h> > > > +#include <asm/unaligned.h> > > > + > > > +#include "es58x_core.h" > > > + > > > +#define DRV_VERSION "1.00" > > > +MODULE_AUTHOR("Mailhol Vincent <mailhol.vincent@wanadoo.fr>"); > > > +MODULE_AUTHOR("Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>"); > > > +MODULE_DESCRIPTION("Socket CAN driver for ETAS ES58X USB adapters"); > > > +MODULE_VERSION(DRV_VERSION); > > > +MODULE_LICENSE("GPL v2"); > > > + > > > +/* Vendor and product id */ > > > +#define ES58X_MODULE_NAME "etas_es58x" > > > +#define ES58X_VENDOR_ID 0x108C > > > +#define ES581_4_PRODUCT_ID 0x0159 > > > +#define ES582_1_PRODUCT_ID 0x0168 > > > +#define ES584_1_PRODUCT_ID 0x0169 > > > + > > > +/* Table of devices which work with this driver */ > > > +static const struct usb_device_id es58x_id_table[] = { > > > + {USB_DEVICE(ES58X_VENDOR_ID, ES581_4_PRODUCT_ID)}, > > > + {USB_DEVICE(ES58X_VENDOR_ID, ES582_1_PRODUCT_ID)}, > > > + {USB_DEVICE(ES58X_VENDOR_ID, ES584_1_PRODUCT_ID)}, > > > + {} /* Terminating entry */ > > > +}; > > > + > > > +MODULE_DEVICE_TABLE(usb, es58x_id_table); > > > + > > > +#define es58x_print_hex_dump(buf, len) \ > > > + print_hex_dump(KERN_DEBUG, \ > > > + ES58X_MODULE_NAME " " __stringify(buf) ": ", \ > > > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > > > + > > > +#define es58x_print_hex_dump_debug(buf, len) \ > > > + print_hex_dump_debug(ES58X_MODULE_NAME " " __stringify(buf) ": ",\ > > > + DUMP_PREFIX_NONE, 16, 1, buf, len, false) > > > + > > > +/* The last two bytes of an ES58X command is a CRC16. The first two > > > + * bytes (the start of frame) are skipped and the CRC calculation > > > + * starts on the third byte. > > > + */ > > > +#define ES58X_CRC_CALC_OFFSET 2 > > > + > > > +/** > > > + * es58x_calculate_crc() - Compute the crc16 of a given URB. > > > + * @urb_cmd: The URB command for which we want to calculate the CRC. > > > + * @urb_len: Length of @urb_cmd. Must be at least bigger than 4 > > > + * (ES58X_CRC_CALC_OFFSET + sizeof(crc)) > > > + * > > > + * Return: crc16 value. > > > + */ > > > +static u16 es58x_calculate_crc(const union es58x_urb_cmd *urb_cmd, u16 urb_len) > > > +{ > > > + u16 crc; > > > + ssize_t len = urb_len - ES58X_CRC_CALC_OFFSET - sizeof(crc); > > > + > > > + WARN_ON(len < 0); > > > > Is it possible to ensure earlier, that the urbs are of correct length? > > Easy answer: it is ensured. On the Tx branch, I create the urbs so I > know for sure that the length is correct. On the Rx branch, I have a > dedicated function: es58x_check_rx_urb() for this purpose. I > will remove that WARN_ON() and the one in es58x_get_crc(). > > I will also check the other WARN_ON() in my code to see if they > can be removed (none on my test throughout the last ten months or > so could trigger any of these WARN_ON() so should be fine to > remove but I will double check). > > > > + crc = crc16(0, &urb_cmd->raw_cmd[ES58X_CRC_CALC_OFFSET], len); > > > + return crc; > > > +} > > > > [...] > > > > > +/** > > > + * 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. > > > + * @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. > > > + * @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_total_frame_len: sum, in bytes, of the length of each of the > > > + * CAN messages contained in @tx_urb. To be used as an input of > > > + * netdev_sent_queue() for BQL. > > > + * @tx_can_msg_cnt: Number of messages in @tx_urb. > > > + * @tx_can_msg_is_fd: false: all messages in @tx_urb are Classical > > > + * CAN, true: all messages in @tx_urb are CAN FD. Rationale: > > > + * ES58X FD devices do not allow to mix Classical CAN and FD CAN > > > + * frames in one single bulk transmission. > > > + * @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. > > > + * @channel_idx: Channel index, starts at zero. > > > + */ > > > +struct es58x_priv { > > > + struct can_priv can; > > > + struct es58x_device *es58x_dev; > > > + struct urb *tx_urb; > > > + > > > + spinlock_t echo_skb_spinlock; /* Comments: c.f. supra */ > > > + u32 current_packet_idx; > > > + u16 echo_skb_tail_idx; > > > + u16 echo_skb_head_idx; > > > + u16 num_echo_skb; > > > > Can you explain me how the tx-path works, especially why you need the > > current_packet_idx. > > > > In the mcp251xfd driver, the number of TX buffers is a power of two, that makes > > things easier. tx_heads % len points to the next buffer to be filled, tx_tail % > > len points to the next buffer to be completed. tx_head - tx_tail is the fill > > level of the FIFO. This works without spinlocks. > > For what I understand of your explanations here are the equivalences > between the etas_es58x and the mcp251xfd drivers: > > +--------------------+-------------------+ > | etas_es58x | mcp251xfd | > +--------------------+-------------------+ > | current_packet_idx | tx_head | > | echo_skb_tail_idx | tx_tail % len | > | echo_skb_head_idx | tx_head % len | > | num_echo_skb | tx_head - tx_tail | > +--------------------+-------------------+ > > Especially, the current_packet_idx is sent to the device and returned > to the driver upon completion. > > I wish the TX buffers were a power of two which is unfortunately not > the case. The theoretical TX buffer sizes are 330 and 500 for the two > devices so I wrote the code to work with those values. The exact size > of the TX buffer is actually even more of a mystery because during > testing both devices were unstable when using the theoretical values > and I had to lower these. There is a comment at the bottom of > es581_4.c and es58x_fd.c to reflect those issues. Because I do not > have access to the source code of the firmware, I could not identify > the root cause. > > My understanding is that having a queue size being a power of two is > required in order not to use spinlocks (else, modulo operations would > break when the index wraparound back to zero). I tried to minimize the > number of spinlock: only one per bulk send or bulk receive. Or do you mean to round up the skb_echo array length to the next power of two in the driver despite the actual size of the device FIFO? Did not think about that in the past but that should work. I am going to think a bit more of how to improve that. Yours sincerely, Vincent
On 1/13/21 1:15 PM, Vincent MAILHOL wrote: >>> +/** >>> + * es58x_calculate_crc() - Compute the crc16 of a given URB. >>> + * @urb_cmd: The URB command for which we want to calculate the CRC. >>> + * @urb_len: Length of @urb_cmd. Must be at least bigger than 4 >>> + * (ES58X_CRC_CALC_OFFSET + sizeof(crc)) >>> + * >>> + * Return: crc16 value. >>> + */ >>> +static u16 es58x_calculate_crc(const union es58x_urb_cmd *urb_cmd, u16 urb_len) >>> +{ >>> + u16 crc; >>> + ssize_t len = urb_len - ES58X_CRC_CALC_OFFSET - sizeof(crc); >>> + >>> + WARN_ON(len < 0); >> >> Is it possible to ensure earlier, that the urbs are of correct length? > > Easy answer: it is ensured. Okay, then get rid of those checks :) > On the Tx branch, I create the urbs so I > know for sure that the length is correct. On the Rx branch, I have a > dedicated function: es58x_check_rx_urb() for this purpose. I > will remove that WARN_ON() and the one in es58x_get_crc(). > > I will also check the other WARN_ON() in my code to see if they > can be removed (none on my test throughout the last ten months or > so could trigger any of these WARN_ON() so should be fine to > remove but I will double check). [..] >>> +struct es58x_priv { >>> + struct can_priv can; >>> + struct es58x_device *es58x_dev; >>> + struct urb *tx_urb; >>> + >>> + spinlock_t echo_skb_spinlock; /* Comments: c.f. supra */ >>> + u32 current_packet_idx; >>> + u16 echo_skb_tail_idx; >>> + u16 echo_skb_head_idx; >>> + u16 num_echo_skb; >> >> Can you explain me how the tx-path works, especially why you need the >> current_packet_idx. >> >> In the mcp251xfd driver, the number of TX buffers is a power of two, that makes >> things easier. tx_heads % len points to the next buffer to be filled, tx_tail % >> len points to the next buffer to be completed. tx_head - tx_tail is the fill >> level of the FIFO. This works without spinlocks. > > For what I understand of your explanations here are the equivalences > between the etas_es58x and the mcp251xfd drivers: > > +--------------------+-------------------+ > | etas_es58x | mcp251xfd | > +--------------------+-------------------+ > | current_packet_idx | tx_head | > | echo_skb_tail_idx | tx_tail % len | > | echo_skb_head_idx | tx_head % len | > | num_echo_skb | tx_head - tx_tail | > +--------------------+-------------------+ > > Especially, the current_packet_idx is sent to the device and returned > to the driver upon completion. Is current_packet_idx used only for the TX-PATH? > I wish the TX buffers were a power of two which is unfortunately not > the case. The theoretical TX buffer sizes are 330 and 500 for the two > devices so I wrote the code to work with those values. The exact size > of the TX buffer is actually even more of a mystery because during > testing both devices were unstable when using the theoretical values > and I had to lower these. There is a comment at the bottom of > es581_4.c and es58x_fd.c to reflect those issues. What are the performance penalties for using 256 for the fd and 64 ofr the other? > Because I do not > have access to the source code of the firmware, I could not identify > the root cause. ok > My understanding is that having a queue size being a power of two is > required in order not to use spinlocks (else, modulo operations would > break when the index wraparound back to zero). I tried to minimize the > number of spinlock: only one per bulk send or bulk receive. With queue size being power of two the modulo can be written as a mask operation, so it's reasonable fast. So you only need to care about tx_head and tx_tail, and there is only one writer for each variable. With a little dance and barriers when stopping and starting the queue it's race-free without spinlocks. 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 |
On 1/13/21 3:35 PM, Vincent MAILHOL wrote: >> My understanding is that having a queue size being a power of two is >> required in order not to use spinlocks (else, modulo operations would >> break when the index wraparound back to zero). I tried to minimize the >> number of spinlock: only one per bulk send or bulk receive. > > Or do you mean to round up the skb_echo array length to the next power > of two in the driver despite the actual size of the device FIFO? Did > not think about that in the past but that should work. > > I am going to think a bit more of how to improve that. For the echo_skb rounding up might work, but you should round down, so that your can directly use the masked value. 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 |
On Tue. 14 janv. 2021 at 01:04, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 1/13/21 1:15 PM, Vincent MAILHOL wrote: > >>> +/** > >>> + * es58x_calculate_crc() - Compute the crc16 of a given URB. > >>> + * @urb_cmd: The URB command for which we want to calculate the CRC. > >>> + * @urb_len: Length of @urb_cmd. Must be at least bigger than 4 > >>> + * (ES58X_CRC_CALC_OFFSET + sizeof(crc)) > >>> + * > >>> + * Return: crc16 value. > >>> + */ > >>> +static u16 es58x_calculate_crc(const union es58x_urb_cmd *urb_cmd, u16 urb_len) > >>> +{ > >>> + u16 crc; > >>> + ssize_t len = urb_len - ES58X_CRC_CALC_OFFSET - sizeof(crc); > >>> + > >>> + WARN_ON(len < 0); > >> > >> Is it possible to ensure earlier, that the urbs are of correct length? > > > > Easy answer: it is ensured. > > Okay, then get rid of those checks :) > > > On the Tx branch, I create the urbs so I > > know for sure that the length is correct. On the Rx branch, I have a > > dedicated function: es58x_check_rx_urb() for this purpose. I > > will remove that WARN_ON() and the one in es58x_get_crc(). > > > > I will also check the other WARN_ON() in my code to see if they > > can be removed (none on my test throughout the last ten months or > > so could trigger any of these WARN_ON() so should be fine to > > remove but I will double check). Checked, all WARN_ON() will be removed in v11. > >>> +struct es58x_priv { > >>> + struct can_priv can; > >>> + struct es58x_device *es58x_dev; > >>> + struct urb *tx_urb; > >>> + > >>> + spinlock_t echo_skb_spinlock; /* Comments: c.f. supra */ > >>> + u32 current_packet_idx; > >>> + u16 echo_skb_tail_idx; > >>> + u16 echo_skb_head_idx; > >>> + u16 num_echo_skb; > >> > >> Can you explain me how the tx-path works, especially why you need the > >> current_packet_idx. > >> > >> In the mcp251xfd driver, the number of TX buffers is a power of two, that makes > >> things easier. tx_heads % len points to the next buffer to be filled, tx_tail % > >> len points to the next buffer to be completed. tx_head - tx_tail is the fill > >> level of the FIFO. This works without spinlocks. > > > > For what I understand of your explanations here are the equivalences > > between the etas_es58x and the mcp251xfd drivers: > > > > +--------------------+-------------------+ > > | etas_es58x | mcp251xfd | > > +--------------------+-------------------+ > > | current_packet_idx | tx_head | > > | echo_skb_tail_idx | tx_tail % len | > > | echo_skb_head_idx | tx_head % len | > > | num_echo_skb | tx_head - tx_tail | > > +--------------------+-------------------+ > > > > Especially, the current_packet_idx is sent to the device and returned > > to the driver upon completion. > > Is current_packet_idx used only for the TX-PATH? It is used in the RX path of loopback packet. When a packet comes back, its index should be equal to current_packet_idx - num_echo_skb. I use this in es58x_can_get_echo_skb() to check that there are no packet drops. Of course, if the FIFO size is a power of two, the current_packet_idx would become useless. > > I wish the TX buffers were a power of two which is unfortunately not > > the case. The theoretical TX buffer sizes are 330 and 500 for the two > > devices so I wrote the code to work with those values. The exact size > > of the TX buffer is actually even more of a mystery because during > > testing both devices were unstable when using the theoretical values > > and I had to lower these. There is a comment at the bottom of > > es581_4.c and es58x_fd.c to reflect those issues. > > What are the performance penalties for using 256 for the fd and 64 ofr the other? I checked my passed log, actually, I had good results with 256 on the FD. I lowered it to 255 with no strong reasons. For the classical CAN changing from 75 to 64 should still be enough to reach full busload. > > Because I do not > > have access to the source code of the firmware, I could not identify > > the root cause. > > ok > > > My understanding is that having a queue size being a power of two is > > required in order not to use spinlocks (else, modulo operations would > > break when the index wraparound back to zero). I tried to minimize the > > number of spinlock: only one per bulk send or bulk receive. > > With queue size being power of two the modulo can be written as a mask > operation, so it's reasonable fast. So you only need to care about tx_head and > tx_tail, and there is only one writer for each variable. With a little dance and > barriers when stopping and starting the queue it's race-free without spinlocks. Yep, I checked linux/kfifo.h in the past. I think I understand the theory, now I need to practice. I will try to do the change. Will get back to you later when it is done and tested. Yours sincerely, Vincent
Here is the v10 of the patch. Hope that we are now close to a release. Thanks for your comments! Yours sincerely, Vincent --- Changes in v10 (2021-01-12): - Rebased on linux-can-next/testing and modified according to latest BQL patches. Reference: https://lore.kernel.org/linux-can/20210111141930.693847-1-mkl@pengutronix.de/T/#m5f99d4da8e8934a75f9481ecc3137b59f3762413 - Replaced __netdev_sent_queue() by netdev_sent_queue(). Changes in v9 (2021-01-09): - es58x_start_xmit(): do not use skb anymore after the call of can_put_echo_skb(). Rationale: can_put_echo_skb() calls skb_clone() and thus the original skb gets consumed (i.e. use after free issue). - es58x_start_xmit(): Add a "drop_skb" label to free the skb when errors occur. Changes in v8 (2021-01-04): - The driver requires CRC16. Modified Kconfig accordingly. Changes in v7 (2020-11-17): - Fix compilation issue if CONFIG_BQL is not set. Reference: https://lkml.org/lkml/2020/11/15/163 Changes in v6 (2020-11-15): - Rebase the patch on the testing branch of linux-can-next. - Rename the helper functions according latest changes (e.g. can_cc_get_len() -> can_cc_dlc2len()) - Fix comments of enum es58x_physical_layer and enum es58x_sync_edge. Changes in v5 (2020-11-07): - Add support for DLC greater than 8. - All other patches from the previous series were either accepted or dismissed. As such, this is not a series any more but a single patch. Changes in v4 (2020-10-17): - Remove struct es58x_abstracted_can_frame. - Fix formatting (spaces, comment style). - Transform macros into static inline functions when possible. - Fix the ctrlmode_supported flags in es581_4.c and removed misleading comments in enum es58x_samples_per_bit. - Rename enums according to the type. - Remove function es58x_can_put_echo_skb(). Reference: https://lkml.org/lkml/2020/10/10/53 Changes in v3 (2020-10-03): - Remove all the calls to likely() and unlikely(). Reference: https://lkml.org/lkml/2020/9/30/995 Changes in v2 (2020-09-30): - Fixed -W1 warnings (v1 was tested with GCC -WExtra but not with -W1). v1 (2020-09-27): - First release Vincent Mailhol (1): can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces drivers/net/can/usb/Kconfig | 10 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/etas_es58x/Makefile | 3 + drivers/net/can/usb/etas_es58x/es581_4.c | 552 ++++ drivers/net/can/usb/etas_es58x/es581_4.h | 206 ++ drivers/net/can/usb/etas_es58x/es58x_core.c | 2589 +++++++++++++++++++ drivers/net/can/usb/etas_es58x/es58x_core.h | 707 +++++ drivers/net/can/usb/etas_es58x/es58x_fd.c | 662 +++++ drivers/net/can/usb/etas_es58x/es58x_fd.h | 242 ++ 9 files changed, 4972 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