* [RESEND v12 0/1] Introducing ETAS ES58X CAN USB interfaces @ 2021-03-09 12:09 Vincent Mailhol [not found] ` <20210309120946.1640-2-mailhol.vincent@wanadoo.fr> 0 siblings, 1 reply; 5+ messages in thread From: Vincent Mailhol @ 2021-03-09 12:09 UTC (permalink / raw) To: Marc Kleine-Budde, linux-can Cc: Arunachalam Santhanam, linux-kernel, Jimmy Assarsson, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20210309120946.1640-2-mailhol.vincent@wanadoo.fr>]
* Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces [not found] ` <20210309120946.1640-2-mailhol.vincent@wanadoo.fr> @ 2021-03-09 17:27 ` Jimmy Assarsson 2021-03-09 18:11 ` Vincent MAILHOL 0 siblings, 1 reply; 5+ messages in thread From: Jimmy Assarsson @ 2021-03-09 17:27 UTC (permalink / raw) To: Vincent Mailhol Cc: Arunachalam Santhanam, linux-kernel, Marc Kleine-Budde, linux-can 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces 2021-03-09 17:27 ` [RESEND v12] can: usb: etas_es58X: add support for " Jimmy Assarsson @ 2021-03-09 18:11 ` Vincent MAILHOL 2021-03-09 18:18 ` Vincent MAILHOL 0 siblings, 1 reply; 5+ messages in thread From: Vincent MAILHOL @ 2021-03-09 18:11 UTC (permalink / raw) To: Jimmy Assarsson Cc: Arunachalam Santhanam, open list, Marc Kleine-Budde, linux-can 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces 2021-03-09 18:11 ` Vincent MAILHOL @ 2021-03-09 18:18 ` Vincent MAILHOL 2021-03-09 18:22 ` Jimmy Assarsson 0 siblings, 1 reply; 5+ messages in thread From: Vincent MAILHOL @ 2021-03-09 18:18 UTC (permalink / raw) To: Jimmy Assarsson Cc: Arunachalam Santhanam, open list, Marc Kleine-Budde, linux-can 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND v12] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces 2021-03-09 18:18 ` Vincent MAILHOL @ 2021-03-09 18:22 ` Jimmy Assarsson 0 siblings, 0 replies; 5+ messages in thread From: Jimmy Assarsson @ 2021-03-09 18:22 UTC (permalink / raw) To: Vincent MAILHOL Cc: Arunachalam Santhanam, open list, Marc Kleine-Budde, linux-can 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-09 18:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-09 12:09 [RESEND v12 0/1] Introducing ETAS ES58X CAN USB interfaces Vincent Mailhol [not found] ` <20210309120946.1640-2-mailhol.vincent@wanadoo.fr> 2021-03-09 17:27 ` [RESEND v12] can: usb: etas_es58X: add support for " Jimmy Assarsson 2021-03-09 18:11 ` Vincent MAILHOL 2021-03-09 18:18 ` Vincent MAILHOL 2021-03-09 18:22 ` Jimmy Assarsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).