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

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

[Vincent Mailhol]

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

** Change log **

Changes in v12 (2021-03-09):
  - Rework the queue stop/wake management so that spinlocks are not
    needed anymore.
  - es58x_start_xmit(): check for valid SKB using
    can_dropped_invalid_skb().
  - Implemented TDC according to latest patches in linux-can-next.
Reference: https://lore.kernel.org/linux-can/20210224002008.4158-1-mailhol.vincent@wanadoo.fr/T/#t

Changes in v11 (2021-01-23):
  - Remove all WARN_ON() calls: these were use during development,
    relevant tests are done not to trigger these.
  - es58x_start_xmit(): added net_ratelimit() condition to prevent
    spamming dmesg.
  - add a new es58x_xmit_more() function and simplify the code of
    es58x_start_xmit().
  - Removed functions {es581_4,es58x_fd}_print_conf() which were only
    there for debug.
  - Additional comment for es58x_fd_param.bitrate_max.
  - Make the device FIFO size a power of two and modify the echo_skb
    indexes logic to prevent the use of spinlocks.

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    |  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: [PATCH v12 0/1] Introducing ETAS ES58X CAN USB interfaces

[Jimmy Assarsson]

On 2021-03-08 17:34, Vincent Mailhol wrote:
> 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

Hi Vincent,

Can you please send me the patch, since it does not reach the mailing list (+100 000 characters).

Regards,
jimmy

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 755 bytes --]

On 3/8/21 5:34 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>

I'm not sure if you're supposed to change dql.min_limit from the driver.

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 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v12 0/1] Introducing ETAS ES58X CAN USB interfaces

[Vincent MAILHOL]

Hi Jimmy,

On Tue. 9 Mar 2021 at 17:30, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
>
> On 2021-03-08 17:34, Vincent Mailhol wrote:
> > 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
>
> Hi Vincent,
>
> Can you please send me the patch, since it does not reach the mailing list (+100 000 characters).

I resent it. It is now available at: https://lkml.org/lkml/2021/3/9/456

Also, I guess that your main interest is the TDC. If so, please
check function es58x_fd_enable_channel() and structure
es58x_tdc_const both in es58x_fd.h


Yours sincerely,
Vincent

Re: [PATCH v12 0/1] Introducing ETAS ES58X CAN USB interfaces

[Jimmy Assarsson]

On 2021-03-09 13:18, Vincent MAILHOL wrote:
> Hi Jimmy,
> 
> On Tue. 9 Mar 2021 at 17:30, Jimmy Assarsson <jimmyassarsson@gmail.com> wrote:
>>
>> On 2021-03-08 17:34, Vincent Mailhol wrote:
>>> 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
>>
>> Hi Vincent,
>>
>> Can you please send me the patch, since it does not reach the mailing list (+100 000 characters).
> 
> I resent it. It is now available at: https://lkml.org/lkml/2021/3/9/456

Great :)

> Also, I guess that your main interest is the TDC. If so, please
> check function es58x_fd_enable_channel() and structure
> es58x_tdc_const both in es58x_fd.h

Correct, thanks!

Regards,
jimmy

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

[Vincent MAILHOL]

Hi Marc,

On Tue. 9 Mar 2021 at 19:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 3/8/21 5:34 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>
>
> I'm not sure if you're supposed to change dql.min_limit from the driver.

One thing for sure, I am the only one to do it.

The reason to do so is because benchmarks show me that values
below this threshold are not good for this device (and I try to
be very permissive on the values).

USB introduces a lot of latency and the small PDU of CAN does not
help. The BQL is here to remediate, however, the algorithms can
take time to adjust, especially if there are small bursts.
Modifying the dql.min_limit was the only solution I found to make
sure that packets can be sent in bulk even during small burst
events.

The BQL was not designed for USB nor was it designed for CAN
which probably explains why I am the first one to ever have
thought of using dql.min_limit like this. Using dql.min_limit is
a hack and I pledge guilty for it. However, because this hack
brings performance improvement, I would like to keep it if you do
not mind.


Yours sincerely,
Vincent

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On 09.03.2021 21:45:58, Vincent MAILHOL wrote:
> > On 3/8/21 5:34 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>
> >
> > I'm not sure if you're supposed to change dql.min_limit from the driver.
> 
> One thing for sure, I am the only one to do it.
> 
> The reason to do so is because benchmarks show me that values
> below this threshold are not good for this device (and I try to
> be very permissive on the values).
> 
> USB introduces a lot of latency and the small PDU of CAN does not
> help. The BQL is here to remediate, however, the algorithms can
> take time to adjust, especially if there are small bursts.
> Modifying the dql.min_limit was the only solution I found to make
> sure that packets can be sent in bulk even during small burst
> events.
> 
> The BQL was not designed for USB nor was it designed for CAN
> which probably explains why I am the first one to ever have
> thought of using dql.min_limit like this.

We can try to sneak it into the kernel or ask on the net-dev list for a
proper interface[1] for setting sensible defaults to the min_limit.

> Using dql.min_limit is a hack and I pledge guilty for it. However,
> because this hack brings performance improvement, I would like to keep
> it if you do not mind.

Your explanation is very good and clear, what about sending new mail
with this problem description to the netdev list? A proper solution
might be something like dql_set_min_limit() with a static inline no-op
of BQL is disabled.

Looking at 114cf5802165 ("bql: Byte queue limits"), you want to include:
- Tom Herbert <therbert@google.com>
- Eric Dumazet <eric.dumazet@gmail.com>

Marc

[1] Having ifdefs to set the value in the driver is clearly a sign of
some misuse :)

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vincent MAILHOL]

Hi Marc,

On Tue. 9 Mar 2021 at 21:57, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 09.03.2021 21:45:58, Vincent MAILHOL wrote:
> > > On 3/8/21 5:34 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>
> > >
> > > I'm not sure if you're supposed to change dql.min_limit from the driver.
> >
> > One thing for sure, I am the only one to do it.
> >
> > The reason to do so is because benchmarks show me that values
> > below this threshold are not good for this device (and I try to
> > be very permissive on the values).
> >
> > USB introduces a lot of latency and the small PDU of CAN does not
> > help. The BQL is here to remediate, however, the algorithms can
> > take time to adjust, especially if there are small bursts.
> > Modifying the dql.min_limit was the only solution I found to make
> > sure that packets can be sent in bulk even during small burst
> > events.
> >
> > The BQL was not designed for USB nor was it designed for CAN
> > which probably explains why I am the first one to ever have
> > thought of using dql.min_limit like this.
>
> We can try to sneak it into the kernel or ask on the net-dev list for a
> proper interface[1] for setting sensible defaults to the min_limit.
>
> > Using dql.min_limit is a hack and I pledge guilty for it. However,
> > because this hack brings performance improvement, I would like to keep
> > it if you do not mind.
>
> Your explanation is very good and clear, what about sending new mail
> with this problem description to the netdev list? A proper solution
> might be something like dql_set_min_limit() with a static inline no-op
> of BQL is disabled.
>
> Looking at 114cf5802165 ("bql: Byte queue limits"), you want to include:
> - Tom Herbert <therbert@google.com>
> - Eric Dumazet <eric.dumazet@gmail.com>
>

Sounds good to me. I will prepare a patch to explain the issue
and try to introduce the dql_set_min_limit() function.

Meanwhile, I would be thankful if you could continue the review :)


Yours sincerely,
Vincent

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On 09.03.2021 22:10:08, Vincent MAILHOL wrote:
> Sounds good to me. I will prepare a patch to explain the issue
> and try to introduce the dql_set_min_limit() function.
> 
> Meanwhile, I would be thankful if you could continue the review :)

Thanks for the mail, looks good.

One note for the patch, though:

> diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
> index 407c2f281b64..32437f168a35 100644
> --- a/include/linux/dynamic_queue_limits.h
> +++ b/include/linux/dynamic_queue_limits.h
> @@ -103,6 +103,9 @@ void dql_reset(struct dql *dql);
>  /* Initialize dql state */
>  void dql_init(struct dql *dql, unsigned int hold_time);
>  
> +/* Set the dql minimum limit */
#ifdef CONFIG_DQL
> +void dql_set_min_limit(struct dql *dql, unsigned int min_limit);
#else
static inline void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
{
}
#endif
> +
>  #endif /* _KERNEL_ */
>  
>  #endif /* _LINUX_DQL_H */
> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> index fde0aa244148..8b6ad1e0a2e3 100644
> --- a/lib/dynamic_queue_limits.c
> +++ b/lib/dynamic_queue_limits.c

This file is only compiled if CONFIG_DQL is set, see lib/Makefile:

| obj-$(CONFIG_DQL) += dynamic_queue_limits.o

> @@ -136,3 +136,11 @@ void dql_init(struct dql *dql, unsigned int hold_time)
>  	dql_reset(dql);
>  }
>  EXPORT_SYMBOL(dql_init);
> +
> +void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> +{
> +#ifdef CONFIG_BQL

remove this ifdef

> +	dql->min_limit = min_limit;
> +#endif
> +}
> +EXPORT_SYMBOL(dql_set_min_limit);

regards,
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vincent MAILHOL]

On Wed. 10 Mar 2021 at 00:35, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 09.03.2021 22:10:08, Vincent MAILHOL wrote:
> > Sounds good to me. I will prepare a patch to explain the issue
> > and try to introduce the dql_set_min_limit() function.
> >
> > Meanwhile, I would be thankful if you could continue the review :)
>
> Thanks for the mail, looks good.
>
> One note for the patch, though:
>
> > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
> > index 407c2f281b64..32437f168a35 100644
> > --- a/include/linux/dynamic_queue_limits.h
> > +++ b/include/linux/dynamic_queue_limits.h
> > @@ -103,6 +103,9 @@ void dql_reset(struct dql *dql);
> >  /* Initialize dql state */
> >  void dql_init(struct dql *dql, unsigned int hold_time);
> >
> > +/* Set the dql minimum limit */
> #ifdef CONFIG_DQL
> > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit);
> #else
> static inline void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> {
> }
> #endif
> > +
> >  #endif /* _KERNEL_ */
> >
> >  #endif /* _LINUX_DQL_H */
> > diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> > index fde0aa244148..8b6ad1e0a2e3 100644
> > --- a/lib/dynamic_queue_limits.c
> > +++ b/lib/dynamic_queue_limits.c
>
> This file is only compiled if CONFIG_DQL is set, see lib/Makefile:
>
> | obj-$(CONFIG_DQL) += dynamic_queue_limits.o

Got it.

> > @@ -136,3 +136,11 @@ void dql_init(struct dql *dql, unsigned int hold_time)
> >       dql_reset(dql);
> >  }
> >  EXPORT_SYMBOL(dql_init);
> > +
> > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> > +{
> > +#ifdef CONFIG_BQL
>
> remove this ifdef
>
> > +     dql->min_limit = min_limit;
> > +#endif
> > +}
> > +EXPORT_SYMBOL(dql_set_min_limit);

Actually, after doing a few more tests, this is a bit more complicated
than anticipated.
The dql member of struct netdev_queue is also guarded by a #ifdef CONFIG_BQL:
https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L629

This means that under the current idea, we would also need to guard
the call to dql_set_min_limit():
#ifdef CONFIG_BQL
    dql_set_min_limit(&netdev_get_tx_queue(netdev, 0)->dql,
              es58x_dev->param->dql_limit_min);
#ifdef CONFIG_BQL

This kills the initial intent of not using the #ifdef CONFIG_BQL to
set the value.

This leads to the need to do:
void netdev_queue_set_dql_min_limit(struct netdev_queue *q, unsigned
int min_limit)
{
#ifdef CONFIG_BQL
    q->dql.min_limit = min_limit;
#endif
}
which would probably be inside /include/linux/netdevice.h.

Does it make sense?


Yours sincerely,
Vincent

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

[Vincent MAILHOL]

Le mer. 10 mars 2021 à 02:54, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> a écrit :
>
> On Wed. 10 Mar 2021 at 00:35, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > On 09.03.2021 22:10:08, Vincent MAILHOL wrote:
> > > Sounds good to me. I will prepare a patch to explain the issue
> > > and try to introduce the dql_set_min_limit() function.
> > >
> > > Meanwhile, I would be thankful if you could continue the review :)
> >
> > Thanks for the mail, looks good.
> >
> > One note for the patch, though:
> >
> > > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
> > > index 407c2f281b64..32437f168a35 100644
> > > --- a/include/linux/dynamic_queue_limits.h
> > > +++ b/include/linux/dynamic_queue_limits.h
> > > @@ -103,6 +103,9 @@ void dql_reset(struct dql *dql);
> > >  /* Initialize dql state */
> > >  void dql_init(struct dql *dql, unsigned int hold_time);
> > >
> > > +/* Set the dql minimum limit */
> > #ifdef CONFIG_DQL
> > > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit);
> > #else
> > static inline void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> > {
> > }
> > #endif
> > > +
> > >  #endif /* _KERNEL_ */
> > >
> > >  #endif /* _LINUX_DQL_H */
> > > diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> > > index fde0aa244148..8b6ad1e0a2e3 100644
> > > --- a/lib/dynamic_queue_limits.c
> > > +++ b/lib/dynamic_queue_limits.c
> >
> > This file is only compiled if CONFIG_DQL is set, see lib/Makefile:
> >
> > | obj-$(CONFIG_DQL) += dynamic_queue_limits.o
>
> Got it.
>
> > > @@ -136,3 +136,11 @@ void dql_init(struct dql *dql, unsigned int hold_time)
> > >       dql_reset(dql);
> > >  }
> > >  EXPORT_SYMBOL(dql_init);
> > > +
> > > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> > > +{
> > > +#ifdef CONFIG_BQL
> >
> > remove this ifdef
> >
> > > +     dql->min_limit = min_limit;
> > > +#endif
> > > +}
> > > +EXPORT_SYMBOL(dql_set_min_limit);
>
> Actually, after doing a few more tests, this is a bit more complicated
> than anticipated.
> The dql member of struct netdev_queue is also guarded by a #ifdef CONFIG_BQL:
> https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L629
>
> This means that under the current idea, we would also need to guard
> the call to dql_set_min_limit():
> #ifdef CONFIG_BQL
>     dql_set_min_limit(&netdev_get_tx_queue(netdev, 0)->dql,
>               es58x_dev->param->dql_limit_min);
> #ifdef CONFIG_BQL
>
> This kills the initial intent of not using the #ifdef CONFIG_BQL to
> set the value.
>
> This leads to the need to do:
> void netdev_queue_set_dql_min_limit(struct netdev_queue *q, unsigned int min_limit)

Of course, I meant:
static inline void netdev_queue_set_dql_min_limit(struct netdev_queue
*q, unsigned int min_limit)

> {
> #ifdef CONFIG_BQL
>     q->dql.min_limit = min_limit;
> #endif
> }
> which would probably be inside /include/linux/netdevice.h.
>
> Does it make sense?

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

On 10.03.2021 03:26:12, Vincent MAILHOL wrote:
> > > > @@ -136,3 +136,11 @@ void dql_init(struct dql *dql, unsigned int hold_time)
> > > >       dql_reset(dql);
> > > >  }
> > > >  EXPORT_SYMBOL(dql_init);
> > > > +
> > > > +void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
> > > > +{
> > > > +#ifdef CONFIG_BQL
> > >
> > > remove this ifdef
> > >
> > > > +     dql->min_limit = min_limit;
> > > > +#endif
> > > > +}
> > > > +EXPORT_SYMBOL(dql_set_min_limit);

Just for completeness. If it would be a real function, the kernel way
would be:

#ifdef CONFIG_BQL
void dql_set_min_limit(struct dql *dql, unsigned int min_limit)
{
     dql->min_limit = min_limit;
}

EXPORT_SYMBOL(dql_set_min_limit);
#endif

...and have a static inline no-op in the header file.

> >
> > Actually, after doing a few more tests, this is a bit more complicated
> > than anticipated.
> > The dql member of struct netdev_queue is also guarded by a #ifdef CONFIG_BQL:
> > https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L629
> >
> > This means that under the current idea, we would also need to guard
> > the call to dql_set_min_limit():
> > #ifdef CONFIG_BQL
> >     dql_set_min_limit(&netdev_get_tx_queue(netdev, 0)->dql,
> >               es58x_dev->param->dql_limit_min);
> > #ifdef CONFIG_BQL
> >
> > This kills the initial intent of not using the #ifdef CONFIG_BQL to
> > set the value.
> >
> > This leads to the need to do:
> > void netdev_queue_set_dql_min_limit(struct netdev_queue *q, unsigned int min_limit)
> 
> Of course, I meant:
> static inline void netdev_queue_set_dql_min_limit(struct netdev_queue
> *q, unsigned int min_limit)
> 
> > {
> > #ifdef CONFIG_BQL
> >     q->dql.min_limit = min_limit;
> > #endif
> > }
> > which would probably be inside /include/linux/netdevice.h.
> >
> > Does it make sense?

Yes, or the other way in illustrated above, if someone doesn't like
static inlines with ifdefs inside the function.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]