[RESEND v12 0/1] Introducing ETAS ES58X CAN USB interfaces

[RESEND v12 0/1] Introducing ETAS ES58X CAN USB interfaces

[Vincent Mailhol]

*Reason for resend:* the patch does not reach the linux-can mailing
list because it is over the 100000 characters limit. Adding
linux-kernel@vger.kernel.org in CC so that people who are not in CC
can get it from the general mailing list instead.

Here comes the twelfth iteration of the ETAS es58x usb driver. The
major difference from previous version is the total removal of
spinlocks. Aside of that, I also implemented the TDC accordingly to
the patch series which I submitted here two weeks ago.

This patch series is based on linux-can-next/testing. It requires the
latest patches in that branch to compile.

Crossing fingers, hoping that we are now close to a release.

Thank you in advance for your review and for your time!


Yours sincerely,
Vincent

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    |  525 ++++
 drivers/net/can/usb/etas_es58x/es581_4.h    |  208 ++
 drivers/net/can/usb/etas_es58x/es58x_core.c | 2404 +++++++++++++++++++
 drivers/net/can/usb/etas_es58x/es58x_core.h |  690 ++++++
 drivers/net/can/usb/etas_es58x/es58x_fd.c   |  600 +++++
 drivers/net/can/usb/etas_es58x/es58x_fd.h   |  243 ++
 9 files changed, 4684 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
-- 
2.26.2

Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Jimmy Assarsson]

Hi Vincent,

On 2021-03-09 13:09, 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).
...
> 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..31f907a7b75f
> --- /dev/null
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
...
> +/**
> + * es58x_add_skb_idx() - Increment an index of the loopback FIFO.
> + * @priv: ES58X private parameters related to the network device.
> + * @idx: address of the index to be incremented.
> + * @a: the increment. Must be positive and less or equal to
> + *	@priv->can.echo_skb_max.
> + *
> + * Do a modulus addition: set *@idx to (*@idx + @a) %
> + * @priv->can.echo_skb_max.
> + *
> + * Rationale: the modulus operator % takes a decent amount of CPU
> + * cycles (c.f. other division functions such as
> + * include/linux/math64.h:iter_div_u64_rem()).
> + */
> +static __always_inline void es58x_add_skb_idx(struct es58x_priv *priv,
> +					      u16 *idx, u16 a)

Never used?

...
> +/**
> + * es58x_get_product_info() - Get the product information and print them.
> + * @es58x_dev: ES58X device.
> + *
> + * Do a synchronous call to get the product information.
> + *
> + * Return: zero on success, errno when any error occurs.
> + */
> +static int es58x_get_product_info(struct es58x_device *es58x_dev)
> +{
> +	struct usb_device *udev = es58x_dev->udev;
> +	const int es58x_prod_info_idx = 6;
> +	/* Empirical tests show a prod_info length of maximum 83,
> +	 * below should be more than enough.
> +	 */
> +	const size_t prod_info_len = 127;
> +	char *prod_info;
> +	int ret;
> +
> +	prod_info = kmalloc(prod_info_len, GFP_KERNEL);
> +	if (!prod_info)
> +		return -ENOMEM;
> +
> +	ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
> +	if (ret < 0) {
> +		dev_err(es58x_dev->dev,
> +			"%s: Could not read the product info: %pe\n",
> +			__func__, ERR_PTR(ret));

Missing free

> +		return ret;
> +	} else if (ret >= prod_info_len - 1) {
> +		dev_warn(es58x_dev->dev,
> +			 "%s: Buffer is too small, result might be truncated\n",
> +			 __func__);
> +	}
> +	dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
> +	kfree(prod_info);
> +
> +	return 0;
> +}


Regards,
jimmy

Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Vincent MAILHOL]

On Wed. 10 Mar 2021 at 02:27, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
>
> Hi Vincent,
>
> On 2021-03-09 13:09, 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).
> ...
> > 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..31f907a7b75f
> > --- /dev/null
> > +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> ...
> > +/**
> > + * es58x_add_skb_idx() - Increment an index of the loopback FIFO.
> > + * @priv: ES58X private parameters related to the network device.
> > + * @idx: address of the index to be incremented.
> > + * @a: the increment. Must be positive and less or equal to
> > + *   @priv->can.echo_skb_max.
> > + *
> > + * Do a modulus addition: set *@idx to (*@idx + @a) %
> > + * @priv->can.echo_skb_max.
> > + *
> > + * Rationale: the modulus operator % takes a decent amount of CPU
> > + * cycles (c.f. other division functions such as
> > + * include/linux/math64.h:iter_div_u64_rem()).
> > + */
> > +static __always_inline void es58x_add_skb_idx(struct es58x_priv *priv,
> > +                                           u16 *idx, u16 a)
>
> Never used?

Indeed, this is a leftover. Should have been removed in v11 when I
made the device FIFO size a power of two.
I was not warned by the compiler, probably because this is an inline function.

> ...
> > +/**
> > + * es58x_get_product_info() - Get the product information and print them.
> > + * @es58x_dev: ES58X device.
> > + *
> > + * Do a synchronous call to get the product information.
> > + *
> > + * Return: zero on success, errno when any error occurs.
> > + */
> > +static int es58x_get_product_info(struct es58x_device *es58x_dev)
> > +{
> > +     struct usb_device *udev = es58x_dev->udev;
> > +     const int es58x_prod_info_idx = 6;
> > +     /* Empirical tests show a prod_info length of maximum 83,
> > +      * below should be more than enough.
> > +      */
> > +     const size_t prod_info_len = 127;
> > +     char *prod_info;
> > +     int ret;
> > +
> > +     prod_info = kmalloc(prod_info_len, GFP_KERNEL);
> > +     if (!prod_info)
> > +             return -ENOMEM;
> > +
> > +     ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
> > +     if (ret < 0) {
> > +             dev_err(es58x_dev->dev,
> > +                     "%s: Could not read the product info: %pe\n",
> > +                     __func__, ERR_PTR(ret));
>
> Missing free

Absolutely!

> > +             return ret;
> > +     } else if (ret >= prod_info_len - 1) {
> > +             dev_warn(es58x_dev->dev,
> > +                      "%s: Buffer is too small, result might be truncated\n",
> > +                      __func__);
> > +     }
> > +     dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
> > +     kfree(prod_info);
> > +
> > +     return 0;
> > +}

Thanks for the two findings, both will be fixed in v13.


Yours sincerely,
Vincent

Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Vincent MAILHOL]

On Wed. 10 Mar 2021 at 03:11, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
>
> On Wed. 10 Mar 2021 at 02:27, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
> >
> > Hi Vincent,
> >
> > On 2021-03-09 13:09, 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).
> > ...
> > > 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..31f907a7b75f
> > > --- /dev/null
> > > +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> > ...
> > > +/**
> > > + * es58x_add_skb_idx() - Increment an index of the loopback FIFO.
> > > + * @priv: ES58X private parameters related to the network device.
> > > + * @idx: address of the index to be incremented.
> > > + * @a: the increment. Must be positive and less or equal to
> > > + *   @priv->can.echo_skb_max.
> > > + *
> > > + * Do a modulus addition: set *@idx to (*@idx + @a) %
> > > + * @priv->can.echo_skb_max.
> > > + *
> > > + * Rationale: the modulus operator % takes a decent amount of CPU
> > > + * cycles (c.f. other division functions such as
> > > + * include/linux/math64.h:iter_div_u64_rem()).
> > > + */
> > > +static __always_inline void es58x_add_skb_idx(struct es58x_priv *priv,
> > > +                                           u16 *idx, u16 a)
> >
> > Never used?
>
> Indeed, this is a leftover. Should have been removed in v11 when I
> made the device FIFO size a power of two.
> I was not warned by the compiler, probably because this is an inline function.
>
> > ...
> > > +/**
> > > + * es58x_get_product_info() - Get the product information and print them.
> > > + * @es58x_dev: ES58X device.
> > > + *
> > > + * Do a synchronous call to get the product information.
> > > + *
> > > + * Return: zero on success, errno when any error occurs.
> > > + */
> > > +static int es58x_get_product_info(struct es58x_device *es58x_dev)
> > > +{
> > > +     struct usb_device *udev = es58x_dev->udev;
> > > +     const int es58x_prod_info_idx = 6;
> > > +     /* Empirical tests show a prod_info length of maximum 83,
> > > +      * below should be more than enough.
> > > +      */
> > > +     const size_t prod_info_len = 127;
> > > +     char *prod_info;
> > > +     int ret;
> > > +
> > > +     prod_info = kmalloc(prod_info_len, GFP_KERNEL);
> > > +     if (!prod_info)
> > > +             return -ENOMEM;
> > > +
> > > +     ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
> > > +     if (ret < 0) {
> > > +             dev_err(es58x_dev->dev,
> > > +                     "%s: Could not read the product info: %pe\n",
> > > +                     __func__, ERR_PTR(ret));
> >
> > Missing free
>
> Absolutely!
>
> > > +             return ret;
> > > +     } else if (ret >= prod_info_len - 1) {
> > > +             dev_warn(es58x_dev->dev,
> > > +                      "%s: Buffer is too small, result might be truncated\n",
> > > +                      __func__);
> > > +     }
> > > +     dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
> > > +     kfree(prod_info);
> > > +
> > > +     return 0;
> > > +}
>
> Thanks for the two findings, both will be fixed in v13.

Out of curiosity, did you find the two issues throughout a code
review or did you use any kind of static analysis tool?

Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

[Jimmy Assarsson]

On 2021-03-09 19:18, Vincent MAILHOL wrote:
> On Wed. 10 Mar 2021 at 03:11, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
>>
>> On Wed. 10 Mar 2021 at 02:27, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
>>>
>>> Hi Vincent,
>>>
>>> On 2021-03-09 13:09, 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).
>>> ...
>>>> 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..31f907a7b75f
>>>> --- /dev/null
>>>> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
>>> ...
>>>> +/**
>>>> + * es58x_add_skb_idx() - Increment an index of the loopback FIFO.
>>>> + * @priv: ES58X private parameters related to the network device.
>>>> + * @idx: address of the index to be incremented.
>>>> + * @a: the increment. Must be positive and less or equal to
>>>> + *   @priv->can.echo_skb_max.
>>>> + *
>>>> + * Do a modulus addition: set *@idx to (*@idx + @a) %
>>>> + * @priv->can.echo_skb_max.
>>>> + *
>>>> + * Rationale: the modulus operator % takes a decent amount of CPU
>>>> + * cycles (c.f. other division functions such as
>>>> + * include/linux/math64.h:iter_div_u64_rem()).
>>>> + */
>>>> +static __always_inline void es58x_add_skb_idx(struct es58x_priv *priv,
>>>> +                                           u16 *idx, u16 a)
>>>
>>> Never used?
>>
>> Indeed, this is a leftover. Should have been removed in v11 when I
>> made the device FIFO size a power of two.
>> I was not warned by the compiler, probably because this is an inline function.
>>
>>> ...
>>>> +/**
>>>> + * es58x_get_product_info() - Get the product information and print them.
>>>> + * @es58x_dev: ES58X device.
>>>> + *
>>>> + * Do a synchronous call to get the product information.
>>>> + *
>>>> + * Return: zero on success, errno when any error occurs.
>>>> + */
>>>> +static int es58x_get_product_info(struct es58x_device *es58x_dev)
>>>> +{
>>>> +     struct usb_device *udev = es58x_dev->udev;
>>>> +     const int es58x_prod_info_idx = 6;
>>>> +     /* Empirical tests show a prod_info length of maximum 83,
>>>> +      * below should be more than enough.
>>>> +      */
>>>> +     const size_t prod_info_len = 127;
>>>> +     char *prod_info;
>>>> +     int ret;
>>>> +
>>>> +     prod_info = kmalloc(prod_info_len, GFP_KERNEL);
>>>> +     if (!prod_info)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
>>>> +     if (ret < 0) {
>>>> +             dev_err(es58x_dev->dev,
>>>> +                     "%s: Could not read the product info: %pe\n",
>>>> +                     __func__, ERR_PTR(ret));
>>>
>>> Missing free
>>
>> Absolutely!
>>
>>>> +             return ret;
>>>> +     } else if (ret >= prod_info_len - 1) {
>>>> +             dev_warn(es58x_dev->dev,
>>>> +                      "%s: Buffer is too small, result might be truncated\n",
>>>> +                      __func__);
>>>> +     }
>>>> +     dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
>>>> +     kfree(prod_info);
>>>> +
>>>> +     return 0;
>>>> +}
>>
>> Thanks for the two findings, both will be fixed in v13.
> 
> Out of curiosity, did you find the two issues throughout a code
> review or did you use any kind of static analysis tool?

Code review.