On Thu, Feb 10, 2022 at 09:20:47PM +0100, Max Staudt wrote: > 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@xxxxxxxxxxxxxxxxxxx> 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? Will anyone really do that? And if so, what can they do about it? The kernel's job is to work around broken hardware, so we might as well just deal with it :( > > > 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. We really can not do anything about out-of-tree code. > 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. That would work, want to send a patch right now for that? thanks, greg k-h
