Re: [PATCH] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype V2

From: Greg KH <gregkh@linuxfoundation.org>
To: Johan Hovold <johan@kernel.org>
Cc: Magnus Damm <damm@opensource.se>,
	linux-serial@vger.kernel.org, robh@kernel.org,
	geert+renesas@glider.be, linux-renesas-soc@vger.kernel.org,
	wsa+renesas@sang-engineering.com, jirislaby@kernel.org
Subject: Re: [PATCH] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype V2
Date: Fri, 31 Dec 2021 13:42:58 +0100	[thread overview]
Message-ID: <Yc760m+/mXsuVo1h@kroah.com> (raw)
In-Reply-To: <Yc7oZ/1tu95Z4wPS@hovoldconsulting.com>

On Fri, Dec 31, 2021 at 12:24:23PM +0100, Johan Hovold wrote:
> On Thu, Dec 30, 2021 at 01:30:18PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Dec 12, 2021 at 10:21:28PM +0900, Magnus Damm wrote:
> > > From: Magnus Damm <damm+renesas@opensource.se>
> > > 
> > > Allow serdev device drivers get notified by hardware errors such as BREAK,
> > > FRAME, PARITY and OVERRUN.
> > > 
> > > With this patch, in the event of an error detected in the UART device driver
> > > the serdev_device_driver will get the newly introduced ->error() callback
> > > invoked if serdev_device_set_error_mask() has previously been used to enable
> > > the type of error. The errors are taken straight from the TTY layer and fed
> > > into the serdev_device_driver after filtering out only enabled errors.
> > > 
> > > Without this patch the hardware errors never reach the serdev_device_driver.
> > > 
> > > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> > > ---
> > > 
> > >  Applies to linux-5.16-rc4
> > > 
> > >  Change since V1:
> > >  - Use __set_bit() instead of set_bit() in ttyport_receive_buf()
> > >  - Switch to assign_bit() in ttyport_set_error_mask()
> > > 
> > >  Thanks to Geert for feedback!
> > > 
> > >  The following prototype patch is using serdev error notifications:
> > >  [PATCH] r8a77995 Draak SCIF0 LED and KEY Serdev prototype V2
> > 
> > Looks good, now applied to my tty tree.
> 
> I really don't think this is ready to be merged. There's been no
> discussion about the design of this interface and importantly there are
> no users (there was an RFC floating around but that one too has issues).
> 
> Some of the problems with this patch include:
> 
>  - performance penalty for all serdev drivers due to unconditional per
>    character processing
>  - flagged characters are still being forwarded to the consumer (e.g.
>    NUL chars inserted on breaks)
>  - it only works with some broken serial drivers which do not honour
>    TTY_DRIVER_REAL_RAW
>  - interface basically limited to the hacky led/input driver mentioned
>    above since it does not match flags with characters
> 
> I suggest reverting for now.

No problem, will go revert it right now, thanks for looking at it.

greg k-h