Re: [PATCH v2] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters

[Max Staudt <max@enpas.org>]

Thanks Greg for your quick feedback!

I'll include it and send a new version after waiting for more comments.

A few questions/suggestions below.



On Thu, 10 Feb 2022 18:41:14 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Feb 10, 2022 at 06:13:15PM +0100, Max Staudt wrote:
> > +/* If this is enabled, we'll try to make the best of the situation
> > + * even if we receive unexpected characters on the line.
> > + * No guarantees.
> > + * Handle with care, it's likely your hardware is unreliable!
> > + */
> > +static bool accept_flaky_uart;
> > +module_param_named(accept_flaky_uart, accept_flaky_uart, bool,
> > 0444); +MODULE_PARM_DESC(accept_flaky_uart, "Don't bail at the
> > first invalid character. Behavior undefined.");  
> 
> Module parameters are from the 1990's, please no.  This is a per-code
> setting, when you want it to be a per-device setting, right?  Please
> just drop this.

Agreed.

Ideally, this option would be part of the ldattach moment. But that ABI
for ldattach doesn't exist.

I'll hardcode the driver to be more lenient - i.e. accept_flaky_uart=1.
Whoever relies on it for critical systems likely hasn't read the
disclaimer anyway :)


How about inverting the option, thus having a debug_extra_strict module
option for debugging purposes, so users can more easily see if their
adapter is at fault?


> > diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> > index a58deb3061eb..4d3ad2569641 100644
> > --- a/include/uapi/linux/tty.h
> > +++ b/include/uapi/linux/tty.h
> > @@ -39,5 +39,6 @@
> >  #define N_SPEAKUP	26	/* Speakup communication with
> > synths */ #define N_NULL		27	/* Null ldisc
> > used for error handling */ #define N_MCTP		28
> > /* MCTP-over-serial */ +#define N_ELMCAN	29	/* Serial
> > / USB serial OBD-II Interfaces */  
> 
> We are now full, no more new ones to ever add!  :)
> 
> This looks fine, we can always bump up the number if we want more.

Phew! :)

May I suggest increasing NR_LDISCS right now, while we're at it?

Also, I could set N_ELMCAN to 30 and leave #29 free for out-of-tree
uses. It was quite handy having an unused ldisc number while developing
this driver, and I assume there are other users out there.

#29 is the highest number available for the last eons, so people are
bound to have used it, and (unknowingly) expect it to work even after
forward porting their out-of-tree modules to v5.17's ldisc API. Things
could break in ugly unexpected ways for people who have been using #29
internally and have it hardcoded in scripts using ldattach.

Maybe
  #define N_DEVELOPMENT 29
or something like that?

And then from this point onwards, NR_LDISCS needs only be incremented
once a new ldisc is actually upstreamed.



Thanks,

Max