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
