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

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

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?