| Message ID | 20201019101109.753597069@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: Cleanup in_interupt/in_irq/in_atomic() usage | expand |
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@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Johan Hovold <johan@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > > --- > 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
On 2020-10-25 17:56:47 [+0100], Johan Hovold wrote: > 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 wasn't aware of that. I've been looking at the callers each time a `tty' was passed it looked like a preemptible context (due to mutex / GFP_KERNEL) and so on. > > Johan Sebastian
On Mon, Oct 26, 2020 at 01:47:53PM +0100, Sebastian Andrzej Siewior wrote: > On 2020-10-25 17:56:47 [+0100], Johan Hovold wrote: > > 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 wasn't aware of that. I've been looking at the callers each time a > `tty' was passed it looked like a preemptible context (due to mutex / > GFP_KERNEL) and so on. Yeah, the default line discipline only calls in preemptible context (these days), but others do not (e.g. see ppp_async_push()). Johan
--- 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);