netdev - Re: [PATCH v10 1/1] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Jan 2021 17:04:38 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
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 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 |



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)