Re: [patch V2 02/13] USB: serial: keyspan_pda: Replace in_interrupt() usage

[Johan Hovold]

On Mon, Oct 19, 2020 at 12:06:31PM +0200, Thomas Gleixner wrote:
> keyspan_pda_write() uses in_interrupt() to check whether it is safe to
> invoke functions which might sleep.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be seperated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> Aside of that it does not cover all contexts which cannot sleep,
> e.g. preempt disabled regions which cannot be reliably detected on all
> kernel configurations.
> 
> With the current printk() implementation console->write() can be invoked
> from almost any context. The upcoming rework of the console core will
> provide thread context for console drivers which require to sleep.
> 
> For now, restrict the room query which can sleep to tty writes which happen
> from preemptible task context.
> 
> The usability for dmesg output is limited anyway because it's almost
> guaranteed to drop the 'LF' which is submitted after the dmesg line because
> the device supports only one transfer on flight. Same for any printk()
> which is coming in before the previous transfer has been done.
> 
> This new restriction does not make it worse than before, but it makes the
> condition correct under all circumstances.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@xxxxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Johan Hovold <johan@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> 
> ---
>  drivers/usb/serial/keyspan_pda.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> --- a/drivers/usb/serial/keyspan_pda.c
> +++ b/drivers/usb/serial/keyspan_pda.c
> @@ -477,10 +477,12 @@ static int keyspan_pda_write(struct tty_
>  
>  	count = (count > port->bulk_out_size) ? port->bulk_out_size : count;
>  
> -	/* Check if we might overrun the Tx buffer.   If so, ask the
> -	   device how much room it really has.  This is done only on
> -	   scheduler time, since usb_control_msg() sleeps. */
> -	if (count > priv->tx_room && !in_interrupt()) {
> +	/*
> +	 * Check if we might overrun the Tx buffer. If so, ask the device
> +	 * how much room it really has. This can only be invoked for tty
> +	 * usage because the console write can't sleep.
> +	 */
> +	if (count > priv->tx_room && tty) {
>  		u8 *room;
>  
>  		room = kmalloc(1, GFP_KERNEL);

There's a ton of issues with this driver, but this is arguably making
things worse. A line discipline may call write() from just about any
context so we cannot rely on tty being non-NULL here (e.g. PPP).

I've prepared a series fixing up the driver's write implementation
that does away with this room check in the write path instead.

Johan