Date:   Wed, 13 Jan 2021 23:35:49 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     linux-can <linux-can@...r.kernel.org>,
        Arunachalam Santhanam <arunachalam.santhanam@...bosch.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jimmy Assarsson <extja@...ser.com>,
        Masahiro Yamada <masahiroy@...nel.org>,
        "open list : NETWORKING DRIVERS" <netdev@...r.kernel.org>
Subject: Re: [PATCH v10 1/1] can: usb: etas_es58X: add support for ETAS ES58X
 CAN USB interfaces

On Wed. 13 Jan 2021 at 21:15, Vincent MAILHOL
<mailhol.vincent@...adoo.fr> wrote:
>
> Hi Marc,
>
> Thanks for the comments!
>
> On Wed. 13 Jan 2021 à 18:33, Marc Kleine-Budde <mkl@...gutronix.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@...bosch.com>
> > > Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@...bosch.com>
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.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@...adoo.fr>");
> > > +MODULE_AUTHOR("Arunachalam Santhanam <arunachalam.santhanam@...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

Powered by blists - more mailing lists