Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage
@ 2020-10-14 14:52 Thomas Gleixner
  2020-10-14 14:52 ` [patch 01/12] USB: sisusbvga: Make console support depend on BROKEN Thomas Gleixner
                   ` (11 more replies)
  0 siblings, 12 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Thomas Winischhofer, Greg Kroah-Hartman,
	linux-usb, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Johan Hovold, Mathias Nyman, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Felipe Balbi, Duncan Sands

Folks,

in the discussion about preempt count consistency accross kernel configurations:

  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

Linus clearly requested that code in drivers and libraries which changes
behaviour based on execution context should either be split up so that
e.g. task context invocations and BH invocations have different interfaces
or if that's not possible the context information has to be provided by the
caller which knows in which context it is executing.

This includes conditional locking, allocation mode (GFP_*) decisions and
avoidance of code paths which might sleep.

In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
driver code completely.

The usage of such constructs in USB is rather limited. Most of it is in
debug code and (misleading) comments. But of course there are also a few
few bugs including one unfixable.

With the following series applied, USB is clean.

Thanks,

	tglx
---
 atm/usbatm.c             |    2 
 core/buffer.c            |    6 +-
 core/hcd-pci.c           |    6 +-
 core/hcd.c               |   21 ++++----
 core/hub.c               |    3 -
 core/message.c           |   35 +++++++++-----
 core/usb.c               |    4 -
 gadget/udc/core.c        |    2 
 gadget/udc/dummy_hcd.c   |    5 +-
 gadget/udc/pxa27x_udc.c  |   19 ++++---
 host/ehci-fsl.c          |    9 +--
 host/ehci-pmcmsp.c       |   15 +++---
 host/isp1362.h           |    5 +-
 host/ohci-at91.c         |   11 +++-
 host/ohci-omap.c         |    7 +-
 host/ohci-pxa27x.c       |   11 ++--
 host/ohci-s3c2410.c      |   12 ++---
 host/xhci-mem.c          |    2 
 host/xhci.c              |    6 --
 misc/sisusbvga/Kconfig   |    2 
 serial/digi_acceleport.c |    7 +-
 serial/keyspan_pda.c     |  112 ++++++++++++++++++++---------------------------
 usbip/usbip_common.c     |    5 --
 23 files changed, 156 insertions(+), 151 deletions(-)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 01/12] USB: sisusbvga: Make console support depend on BROKEN
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 14:52 ` [patch 02/12] USB: serial: keyspan_pda: Replace in_interrupt() usage Thomas Gleixner
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Thomas Winischhofer, Greg Kroah-Hartman,
	linux-usb, stable, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Johan Hovold, Mathias Nyman, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Felipe Balbi, Duncan Sands

The console part of sisusbvga is broken vs. printk(). It uses in_atomic()
to detect contexts in which it cannot sleep despite the big fat comment in
preempt.h which says: Do not use in_atomic() in driver code.

in_atomic() does not work on kernels with CONFIG_PREEMPT_COUNT=n which
means that spin/rw_lock held regions are not detected by it.

There is no way to make this work by handing context information through to
the driver and this only can be solved once the core printk infrastructure
supports sleepable console drivers.

Make it depend on BROKEN for now.

Fixes: 1bbb4f2035d9 ("[PATCH] USB: sisusb[vga] update")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Thomas Winischhofer <thomas@winischhofer.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: stable@vger.kernel.org
---
 drivers/usb/misc/sisusbvga/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/usb/misc/sisusbvga/Kconfig
+++ b/drivers/usb/misc/sisusbvga/Kconfig
@@ -16,7 +16,7 @@ config USB_SISUSBVGA
 
 config USB_SISUSBVGA_CON
 	bool "Text console and mode switching support" if USB_SISUSBVGA
-	depends on VT
+	depends on VT && BROKEN
 	select FONT_8x16
 	help
 	  Say Y here if you want a VGA text console via the USB dongle or


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 02/12] USB: serial: keyspan_pda: Replace in_interrupt() usage
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
  2020-10-14 14:52 ` [patch 01/12] USB: sisusbvga: Make console support depend on BROKEN Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 14:52 ` [patch 03/12] USB: serial: keyspan_pda: Consolidate room query Thomas Gleixner
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Johan Hovold, Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Mathias Nyman, Valentina Manea, Shuah Khan, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

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);


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
  2020-10-14 14:52 ` [patch 01/12] USB: sisusbvga: Make console support depend on BROKEN Thomas Gleixner
  2020-10-14 14:52 ` [patch 02/12] USB: serial: keyspan_pda: Replace in_interrupt() usage Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 16:14   ` Alan Stern
  2020-10-14 14:52 ` [patch 04/12] USB: serial: digi_acceleport: Remove in_interrupt() usage Thomas Gleixner
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Sebastian Andrzej Siewior, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Felipe Balbi, Duncan Sands

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Having two copies of the same code doesn't make the code more readable and
allocating a buffer of 1 byte for a synchronous operation is a pointless
exercise.

Add a byte buffer to struct keyspan_pda_private which can be used
instead. The buffer is only used in open() and tty->write(). Console writes
are not calling into the query. open() obviously happens before write() and
the writes are serialized by bit 0 of port->write_urbs_free which protects
also the transaction itself.

Move the actual query into a helper function and cleanup the usage sites in
keyspan_pda_write() and keyspan_pda_open().

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 |  102 ++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 59 deletions(-)

--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -47,6 +47,7 @@ struct keyspan_pda_private {
 	struct work_struct			unthrottle_work;
 	struct usb_serial	*serial;
 	struct usb_serial_port	*port;
+	u8			query_buf;
 };
 
 
@@ -436,6 +437,31 @@ static int keyspan_pda_tiocmset(struct t
 	return rc;
 }
 
+/*
+ * Using priv->query_buf is safe here because this is only called for TTY
+ * operations open() and write(). write() comes post open() obviously and
+ * write() itself is serialized via bit 0 of port->write_urbs_free. Console
+ * writes are never calling into this.
+ */
+static int keyspan_pda_query_room(struct usb_serial *serial,
+				  struct keyspan_pda_private *priv)
+{
+	int res;
+
+	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			      6, /* write_room */
+			      USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN,
+			      0, /* value */
+			      0, /* index */
+			      &priv->query_buf,
+			      1,
+			      2000);
+	if (res != 1)
+		return res < 0 ? res : -EIO;
+
+	return (unsigned int)priv->query_buf;
+}
+
 static int keyspan_pda_write(struct tty_struct *tty,
 	struct usb_serial_port *port, const unsigned char *buf, int count)
 {
@@ -483,39 +509,16 @@ static int keyspan_pda_write(struct tty_
 	 * usage because the console write can't sleep.
 	 */
 	if (count > priv->tx_room && tty) {
-		u8 *room;
-
-		room = kmalloc(1, GFP_KERNEL);
-		if (!room) {
-			rc = -ENOMEM;
-			goto exit;
-		}
-
-		rc = usb_control_msg(serial->dev,
-				     usb_rcvctrlpipe(serial->dev, 0),
-				     6, /* write_room */
-				     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-				     | USB_DIR_IN,
-				     0, /* value: 0 means "remaining room" */
-				     0, /* index */
-				     room,
-				     1,
-				     2000);
-		if (rc > 0) {
-			dev_dbg(&port->dev, "roomquery says %d\n", *room);
-			priv->tx_room = *room;
-		}
-		kfree(room);
+		rc = keyspan_pda_query_room(serial, priv);
 		if (rc < 0) {
-			dev_dbg(&port->dev, "roomquery failed\n");
-			goto exit;
-		}
-		if (rc == 0) {
-			dev_dbg(&port->dev, "roomquery returned 0 bytes\n");
-			rc = -EIO; /* device didn't return any data */
+			dev_dbg(&port->dev, "roomquery failed %d\n", rc);
 			goto exit;
 		}
+
+		dev_dbg(&port->dev, "roomquery says %d\n", rc);
+		priv->tx_room = rc;
 	}
+
 	if (count > priv->tx_room) {
 		/* we're about to completely fill the Tx buffer, so
 		   we'll be throttled afterwards. */
@@ -615,45 +618,26 @@ static int keyspan_pda_open(struct tty_s
 					struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
-	u8 *room;
-	int rc = 0;
 	struct keyspan_pda_private *priv;
+	int rc;
 
-	/* find out how much room is in the Tx ring */
-	room = kmalloc(1, GFP_KERNEL);
-	if (!room)
-		return -ENOMEM;
+	priv = usb_get_serial_port_data(port);
 
-	rc = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			     6, /* write_room */
-			     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-			     | USB_DIR_IN,
-			     0, /* value */
-			     0, /* index */
-			     room,
-			     1,
-			     2000);
+	/* find out how much room is in the Tx ring */
+	rc = keyspan_pda_query_room(serial, priv);
 	if (rc < 0) {
-		dev_dbg(&port->dev, "%s - roomquery failed\n", __func__);
-		goto error;
-	}
-	if (rc == 0) {
-		dev_dbg(&port->dev, "%s - roomquery returned 0 bytes\n", __func__);
-		rc = -EIO;
-		goto error;
+		dev_dbg(&port->dev, "roomquery failed %d\n", rc);
+		return rc;
 	}
-	priv = usb_get_serial_port_data(port);
-	priv->tx_room = *room;
-	priv->tx_throttled = *room ? 0 : 1;
+
+	priv->tx_room = rc;
+	priv->tx_throttled = rc ? 0 : 1;
 
 	/*Start reading from the device*/
 	rc = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
-	if (rc) {
+	if (rc)
 		dev_dbg(&port->dev, "%s - usb_submit_urb(read int) failed\n", __func__);
-		goto error;
-	}
-error:
-	kfree(room);
+
 	return rc;
 }
 static void keyspan_pda_close(struct usb_serial_port *port)


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 04/12] USB: serial: digi_acceleport: Remove in_interrupt() usage
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (2 preceding siblings ...)
  2020-10-14 14:52 ` [patch 03/12] USB: serial: keyspan_pda: Consolidate room query Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 14:52 ` [patch 05/12] usb: xhci: Remove in_interrupt() checks Thomas Gleixner
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Johan Hovold, Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Mathias Nyman, Valentina Manea, Shuah Khan, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

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 separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

The debug printk() in digi_write() prints in_interrupt() as context
information. TTY writes happen always in preemptible task context and
console writes can happen from almost any context, so in_interrupt() is not
really helpful.

Aside of that issuing a printk() from a console->write() callback is not a
really brilliant idea for obvious reasons.

Remove the in_interrupt() printout and make the printk() depend on tty.

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/digi_acceleport.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -911,9 +911,10 @@ static int digi_write(struct tty_struct
 	unsigned char *data = port->write_urb->transfer_buffer;
 	unsigned long flags = 0;
 
-	dev_dbg(&port->dev,
-		"digi_write: TOP: port=%d, count=%d, in_interrupt=%ld\n",
-		priv->dp_port_num, count, in_interrupt());
+	if (tty) {
+		dev_dbg(&port->dev, "digi_write: TOP: port=%d, count=%d\n",
+			priv->dp_port_num, count);
+	}
 
 	/* copy user data (which can sleep) before getting spin lock */
 	count = min(count, port->bulk_out_size-2);


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 05/12] usb: xhci: Remove in_interrupt() checks
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (3 preceding siblings ...)
  2020-10-14 14:52 ` [patch 04/12] USB: serial: digi_acceleport: Remove in_interrupt() usage Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 14:52 ` [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage Thomas Gleixner
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	Thomas Winischhofer, Johan Hovold, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Felipe Balbi, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

The usage of in_interrupt() in drivers is phased out for various reasons.

xhci_set_hc_event_deq() has an !in_interrupt() check which is pointless
because the function is only invoked from xhci_mem_init() which is clearly
task context as it does GFP_KERNEL allocations. Remove it.

xhci_urb_enqueue() prints a debug message if an URB is submitted after the
underlying hardware was suspended. But that warning is only issued when
in_interrupt() is true, which makes no sense. Simply return -ESHUTDOWN and
be done with it.

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: Mathias Nyman <mathias.nyman@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
---
 drivers/usb/host/xhci-mem.c |    2 +-
 drivers/usb/host/xhci.c     |    6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2110,7 +2110,7 @@ static void xhci_set_hc_event_deq(struct
 
 	deq = xhci_trb_virt_to_dma(xhci->event_ring->deq_seg,
 			xhci->event_ring->dequeue);
-	if (deq == 0 && !in_interrupt())
+	if (!deq)
 		xhci_warn(xhci, "WARN something wrong with SW event ring "
 				"dequeue ptr.\n");
 	/* Update HC event ring dequeue pointer */
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1473,11 +1473,9 @@ static int xhci_urb_enqueue(struct usb_h
 	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
 	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
 
-	if (!HCD_HW_ACCESSIBLE(hcd)) {
-		if (!in_interrupt())
-			xhci_dbg(xhci, "urb submitted during PCI suspend\n");
+	if (!HCD_HW_ACCESSIBLE(hcd))
 		return -ESHUTDOWN;
-	}
+
 	if (xhci->devs[slot_id]->flags & VDEV_PORT_ERROR) {
 		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
 		return -ENODEV;


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (4 preceding siblings ...)
  2020-10-14 14:52 ` [patch 05/12] usb: xhci: Remove in_interrupt() checks Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 18:49   ` kernel test robot
  2020-10-14 14:52 ` [patch 07/12] usbip: Remove in_interrupt() check Thomas Gleixner
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

isp1362_show_regs() is a debugging-only function, with no call sites. It
prints the cached value of the HCuPINTENB register if in_interupt() is
true, otherwise it reads the actual register content.

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 separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

Make the conditional based on a function argument.

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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/host/isp1362.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/drivers/usb/host/isp1362.h
+++ b/drivers/usb/host/isp1362.h
@@ -793,7 +793,8 @@ static void isp1362_write_fifo(struct is
 			ISP1362_REG_NO(ISP1362_REG_##r), isp1362_read_reg16(d, r));	\
 }
 
-static void __attribute__((__unused__)) isp1362_show_regs(struct isp1362_hcd *isp1362_hcd)
+static void __attribute__((__unused__))
+isp1362_show_regs(struct isp1362_hcd *isp1362_hc, bool cached_inten)
 {
 	isp1362_show_reg(isp1362_hcd, HCREVISION);
 	isp1362_show_reg(isp1362_hcd, HCCONTROL);
@@ -815,7 +816,7 @@ static void __attribute__((__unused__))
 	isp1362_show_reg(isp1362_hcd, HCXFERCTR);
 	isp1362_show_reg(isp1362_hcd, HCuPINT);
 
-	if (in_interrupt())
+	if (cached_inten)
 		DBG(0, "%-12s[%02x]:     %04x\n", "HCuPINTENB",
 			 ISP1362_REG_NO(ISP1362_REG_HCuPINTENB), isp1362_hcd->irqenb);
 	else


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 07/12] usbip: Remove in_interrupt() check
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (5 preceding siblings ...)
  2020-10-14 14:52 ` [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 15:45   ` Shuah Khan
  2020-10-14 14:52 ` [patch 08/12] usb: hosts: Remove in_interrupt() from comments Thomas Gleixner
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Valentina Manea, Shuah Khan, Greg Kroah-Hartman, linux-usb,
	Thomas Winischhofer, Johan Hovold, Mathias Nyman, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

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 separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

usbip_recv() uses in_interrupt() to conditionally print context information
for debugging messages. The value is zero as the function is only called
from various *_rx_loop() kthread functions. Remove it.

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: Valentina Manea <valentina.manea.m@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/usbip/usbip_common.c |    5 -----
 1 file changed, 5 deletions(-)

--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -324,11 +324,6 @@ int usbip_recv(struct socket *sock, void
 	} while (msg_data_left(&msg));
 
 	if (usbip_dbg_flag_xmit) {
-		if (!in_interrupt())
-			pr_debug("%-10s:", current->comm);
-		else
-			pr_debug("interrupt  :");
-
 		pr_debug("receiving....\n");
 		usbip_dump_buffer(buf, size);
 		pr_debug("received, osize %d ret %d size %zd total %d\n",


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 08/12] usb: hosts: Remove in_interrupt() from comments
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (6 preceding siblings ...)
  2020-10-14 14:52 ` [patch 07/12] usbip: Remove in_interrupt() check Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 15:24   ` Krzysztof Kozlowski
  2020-10-14 16:20   ` Alan Stern
  2020-10-14 14:52 ` [patch 09/12] usb: gadget: pxa27x_udc: Replace in_interrupt() usage in comments Thomas Gleixner
                   ` (3 subsequent siblings)
  11 siblings, 2 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Alan Stern, Greg Kroah-Hartman, linux-usb, linux-omap,
	Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, Felipe Balbi,
	Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

The usage of in_interrupt() in drivers is phased out for various reasons.

Various comments use !in_interrupt() to describe calling context for probe()
and remove() functions. That's wrong because the calling context has to be
preemptible task context, which is not what !in_interrupt() describes.

Cleanup the comments. While at it add the missing kernel doc argument
descriptors and make usb_hcd_msp_remove() static.

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: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org

---
 drivers/usb/host/ehci-fsl.c     |    9 ++++-----
 drivers/usb/host/ehci-pmcmsp.c  |   15 +++++++++------
 drivers/usb/host/ohci-at91.c    |   11 ++++++++---
 drivers/usb/host/ohci-omap.c    |    7 ++++---
 drivers/usb/host/ohci-pxa27x.c  |   11 ++++++-----
 drivers/usb/host/ohci-s3c2410.c |   12 ++++++------
 6 files changed, 37 insertions(+), 28 deletions(-)

--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -39,10 +39,10 @@ static struct hc_driver __read_mostly fs
 /*
  * fsl_ehci_drv_probe - initialize FSL-based HCDs
  * @pdev: USB Host Controller being probed
- * Context: !in_interrupt()
  *
- * Allocates basic resources for this USB host controller.
+ * Context: task context, might sleep
  *
+ * Allocates basic resources for this USB host controller.
  */
 static int fsl_ehci_drv_probe(struct platform_device *pdev)
 {
@@ -684,12 +684,11 @@ static const struct ehci_driver_override
 /**
  * fsl_ehci_drv_remove - shutdown processing for FSL-based HCDs
  * @pdev: USB Host Controller being removed
- * Context: !in_interrupt()
  *
- * Reverses the effect of usb_hcd_fsl_probe().
+ * Context: task context, might sleep
  *
+ * Reverses the effect of usb_hcd_fsl_probe().
  */
-
 static int fsl_ehci_drv_remove(struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata = dev_get_platdata(&pdev->dev);
--- a/drivers/usb/host/ehci-pmcmsp.c
+++ b/drivers/usb/host/ehci-pmcmsp.c
@@ -147,12 +147,14 @@ static int usb_hcd_msp_map_regs(struct m
 
 /**
  * usb_hcd_msp_probe - initialize PMC MSP-based HCDs
- * Context: !in_interrupt()
+ * @driver:	Pointer to hc driver instance
+ * @dev:	USB controller to probe
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
  * through the hotplug entry's driver_data.
- *
  */
 int usb_hcd_msp_probe(const struct hc_driver *driver,
 			  struct platform_device *dev)
@@ -223,8 +225,9 @@ int usb_hcd_msp_probe(const struct hc_dr
 
 /**
  * usb_hcd_msp_remove - shutdown processing for PMC MSP-based HCDs
- * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
+ * @hcd: USB Host Controller being removed
+ *
+ * Context: task context, might sleep
  *
  * Reverses the effect of usb_hcd_msp_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
@@ -233,7 +236,7 @@ int usb_hcd_msp_probe(const struct hc_dr
  * may be called without controller electrically present
  * may be called with controller, bus, and devices active
  */
-void usb_hcd_msp_remove(struct usb_hcd *hcd, struct platform_device *dev)
+static void usb_hcd_msp_remove(struct usb_hcd *hcd)
 {
 	usb_remove_hcd(hcd);
 	iounmap(hcd->regs);
@@ -306,7 +309,7 @@ static int ehci_hcd_msp_drv_remove(struc
 {
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 
-	usb_hcd_msp_remove(hcd, pdev);
+	usb_hcd_msp_remove(hcd);
 
 	/* free TWI GPIO USB_HOST_DEV pin */
 	gpio_free(MSP_PIN_USB0_HOST_DEV);
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -155,7 +155,10 @@ static struct regmap *at91_dt_syscon_sfr
 
 /*
  * usb_hcd_at91_probe - initialize AT91-based HCDs
- * Context: !in_interrupt()
+ * @driver:	Pointer to hc driver instance
+ * @pdev:	USB controller to probe
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
@@ -246,12 +249,14 @@ static int usb_hcd_at91_probe(const stru
 
 /*
  * usb_hcd_at91_remove - shutdown processing for AT91-based HCDs
- * Context: !in_interrupt()
+ * @hcd:	USB controller to remove
+ * @pdev:	Platform device required for cleanup
+ *
+ * Context: task context, might sleep
  *
  * Reverses the effect of usb_hcd_at91_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
  * context, "rmmod" or something similar.
- *
  */
 static void usb_hcd_at91_remove(struct usb_hcd *hcd,
 				struct platform_device *pdev)
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -285,7 +285,9 @@ static int ohci_omap_reset(struct usb_hc
 
 /**
  * ohci_hcd_omap_probe - initialize OMAP-based HCDs
- * Context: !in_interrupt()
+ * @pdev:	USB controller to probe
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
@@ -399,8 +401,7 @@ static int ohci_hcd_omap_probe(struct pl
 
 /**
  * ohci_hcd_omap_remove - shutdown processing for OMAP-based HCDs
- * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
+ * @pdev: USB Host Controller being removed
  *
  * Reverses the effect of ohci_hcd_omap_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
--- a/drivers/usb/host/ohci-pxa27x.c
+++ b/drivers/usb/host/ohci-pxa27x.c
@@ -410,12 +410,13 @@ static int ohci_pxa_of_init(struct platf
 
 /**
  * ohci_hcd_pxa27x_probe - initialize pxa27x-based HCDs
- * Context: !in_interrupt()
+ * @pdev:	USB Host controller to probe
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
  * through the hotplug entry's driver_data.
- *
  */
 static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
 {
@@ -509,13 +510,13 @@ static int ohci_hcd_pxa27x_probe(struct
 
 /**
  * ohci_hcd_pxa27x_remove - shutdown processing for pxa27x-based HCDs
- * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
+ * @pdev: USB Host Controller being removed
+ *
+ * Context: task context, might sleep
  *
  * Reverses the effect of ohci_hcd_pxa27x_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
  * context, normally "rmmod", "apmd", or something similar.
- *
  */
 static int ohci_hcd_pxa27x_remove(struct platform_device *pdev)
 {
--- a/drivers/usb/host/ohci-s3c2410.c
+++ b/drivers/usb/host/ohci-s3c2410.c
@@ -324,14 +324,13 @@ static void s3c2410_hcd_oc(struct s3c241
 /*
  * ohci_hcd_s3c2410_remove - shutdown processing for HCD
  * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * Reverses the effect of ohci_hcd_3c2410_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
  * context, normally "rmmod", "apmd", or something similar.
- *
-*/
-
+ */
 static int
 ohci_hcd_s3c2410_remove(struct platform_device *dev)
 {
@@ -345,12 +344,13 @@ ohci_hcd_s3c2410_remove(struct platform_
 
 /*
  * ohci_hcd_s3c2410_probe - initialize S3C2410-based HCDs
- * Context: !in_interrupt()
+ * @dev: USB Host Controller to be probed
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
  * through the hotplug entry's driver_data.
- *
  */
 static int ohci_hcd_s3c2410_probe(struct platform_device *dev)
 {


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 09/12] usb: gadget: pxa27x_udc: Replace in_interrupt() usage in comments
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (7 preceding siblings ...)
  2020-10-14 14:52 ` [patch 08/12] usb: hosts: Remove in_interrupt() from comments Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 14:52 ` [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments Thomas Gleixner
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Felipe Balbi, Greg Kroah-Hartman, linux-arm-kernel, linux-usb,
	Thomas Winischhofer, Johan Hovold, Mathias Nyman,
	Valentina Manea, Shuah Khan, Alan Stern, linux-omap, Kukjin Kim,
	Krzysztof Kozlowski, linux-samsung-soc, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

The usage of in_interrupt() in drivers is phased out for various reasons.

Documenting calling contexts of functions with 'in_interrupt()' or
'!in_interrupt()' is imprecise: For a function which might sleep the
condition is preemptible task context, which is not what '!in_interrupt()'
describes.

Replace the context docummentation with plain text and make them match
reality.

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: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/gadget/udc/pxa27x_udc.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -304,7 +304,7 @@ static struct pxa_ep *find_pxa_ep(struct
  * update_pxa_ep_matches - update pxa_ep cached values in all udc_usb_ep
  * @udc: pxa udc
  *
- * Context: in_interrupt()
+ * Context: interrupt handler
  *
  * Updates all pxa_ep fields in udc_usb_ep structures, if this field was
  * previously set up (and is not NULL). The update is necessary is a
@@ -859,7 +859,7 @@ static int write_packet(struct pxa_ep *e
  * @ep: pxa physical endpoint
  * @req: usb request
  *
- * Context: callable when in_interrupt()
+ * Context: interrupt handler
  *
  * Unload as many packets as possible from the fifo we use for usb OUT
  * transfers and put them into the request. Caller should have made sure
@@ -997,7 +997,7 @@ static int read_ep0_fifo(struct pxa_ep *
  * @ep: control endpoint
  * @req: request
  *
- * Context: callable when in_interrupt()
+ * Context: interrupt handler
  *
  * Sends a request (or a part of the request) to the control endpoint (ep0 in).
  * If the request doesn't fit, the remaining part will be sent from irq.
@@ -1036,8 +1036,8 @@ static int write_ep0_fifo(struct pxa_ep
  * @_req: usb request
  * @gfp_flags: flags
  *
- * Context: normally called when !in_interrupt, but callable when in_interrupt()
- * in the special case of ep0 setup :
+ * Context: thread context or from the interrupt handler in the
+ * special case of ep0 setup :
  *   (irq->handle_ep0_ctrl_req->gadget_setup->pxa_ep_queue)
  *
  * Returns 0 if succedeed, error otherwise
@@ -1512,7 +1512,8 @@ static int should_disable_udc(struct pxa
  * pxa_udc_pullup - Offer manual D+ pullup control
  * @_gadget: usb gadget using the control
  * @is_active: 0 if disconnect, else connect D+ pullup resistor
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * Returns 0 if OK, -EOPNOTSUPP if udc driver doesn't handle D+ pullup
  */
@@ -1560,7 +1561,7 @@ static int pxa_udc_vbus_session(struct u
  * @_gadget: usb gadget
  * @mA: current drawn
  *
- * Context: !in_interrupt()
+ * Context: task context, might sleep
  *
  * Called after a configuration was chosen by a USB host, to inform how much
  * current can be drawn by the device from VBus line.
@@ -1886,7 +1887,7 @@ static void handle_ep0_ctrl_req(struct p
  * @fifo_irq: 1 if triggered by fifo service type irq
  * @opc_irq: 1 if triggered by output packet complete type irq
  *
- * Context : when in_interrupt() or with ep->lock held
+ * Context : interrupt handler
  *
  * Tries to transfer all pending request data into the endpoint and/or
  * transfer all pending data in the endpoint into usb requests.
@@ -2011,7 +2012,7 @@ static void handle_ep0(struct pxa_udc *u
  * Tries to transfer all pending request data into the endpoint and/or
  * transfer all pending data in the endpoint into usb requests.
  *
- * Is always called when in_interrupt() and with ep->lock released.
+ * Is always called from the interrupt handler. ep->lock must not be held.
  */
 static void handle_ep(struct pxa_ep *ep)
 {


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (8 preceding siblings ...)
  2020-10-14 14:52 ` [patch 09/12] usb: gadget: pxa27x_udc: Replace in_interrupt() usage in comments Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 16:22   ` Alan Stern
  2020-10-14 14:52 ` [patch 11/12] usb: core: Replace in_interrupt() in comments Thomas Gleixner
  2020-10-14 14:52 ` [patch 12/12] usb: atm: Replace in_interrupt() usage in comment Thomas Gleixner
  11 siblings, 1 reply; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Felipe Balbi, Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Johan Hovold, Mathias Nyman, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

The usage of in_irq()/in_interrupt() in drivers is phased out for various
reasons.

The context description for usb_gadget_giveback_request() is misleading as
in_interupt() means: hard interrupt or soft interrupt or bottom half
disabled regions. But it's also invoked from task context when endpoints
are torn down. Remove it as it's more confusing than helpful.

Replace also the in_irq() comment with plain text.

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: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/gadget/udc/core.c      |    2 --
 drivers/usb/gadget/udc/dummy_hcd.c |    5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)

--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -894,8 +894,6 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_reque
  * @ep: the endpoint to be used with with the request
  * @req: the request being given back
  *
- * Context: in_interrupt()
- *
  * This is called by device controller drivers in order to return the
  * completed request back to the gadget layer.
  */
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -1754,8 +1754,9 @@ static int handle_control_request(struct
 	return ret_val;
 }
 
-/* drive both sides of the transfers; looks like irq handlers to
- * both drivers except the callbacks aren't in_irq().
+/* drive both sides of the transfers; looks like irq handlers to both
+ * drivers except that the callbacks are invoked from soft interrupt
+ * context.
  */
 static void dummy_timer(struct timer_list *t)
 {


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 11/12] usb: core: Replace in_interrupt() in comments
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (9 preceding siblings ...)
  2020-10-14 14:52 ` [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  2020-10-14 16:27   ` Alan Stern
  2020-10-14 14:52 ` [patch 12/12] usb: atm: Replace in_interrupt() usage in comment Thomas Gleixner
  11 siblings, 1 reply; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

From: Ahmed S. Darwish <a.darwish@linutronix.de>

The usage of in_interrupt() in drivers is phased out for various reasons.

Various comments use !in_interrupt() to describe calling context for
functions which might sleep. That's wrong because the calling context has
to be preemptible task context, which is not what !in_interrupt()
describes.

Replace !in_interrupt() with more accurate plain text descriptions.

The comment for usb_hcd_poll_rh_status() is misleading as this function is
called from all kinds of contexts including preemptible task
context. Remove it as there is obviously no restriction.

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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/core/buffer.c  |    6 ++++--
 drivers/usb/core/hcd-pci.c |    6 ++++--
 drivers/usb/core/hcd.c     |   21 +++++++++++----------
 drivers/usb/core/hub.c     |    3 ++-
 drivers/usb/core/message.c |   35 ++++++++++++++++++++++-------------
 drivers/usb/core/usb.c     |    4 ++--
 6 files changed, 45 insertions(+), 30 deletions(-)

--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -51,7 +51,8 @@ void __init usb_init_pool_max(void)
 /**
  * hcd_buffer_create - initialize buffer pools
  * @hcd: the bus whose buffer pools are to be initialized
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * Call this as part of initializing a host controller that uses the dma
  * memory allocators.  It initializes some pools of dma-coherent memory that
@@ -88,7 +89,8 @@ int hcd_buffer_create(struct usb_hcd *hc
 /**
  * hcd_buffer_destroy - deallocate buffer pools
  * @hcd: the bus whose buffer pools are to be destroyed
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * This frees the buffer pools created by hcd_buffer_create().
  */
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -160,7 +160,8 @@ static void ehci_wait_for_companions(str
  * @dev: USB Host Controller being probed
  * @id: pci hotplug id connecting controller to HCD framework
  * @driver: USB HC driver handle
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * Allocates basic PCI resources for this USB host controller, and
  * then invokes the start() method for the HCD associated with it
@@ -304,7 +305,8 @@ EXPORT_SYMBOL_GPL(usb_hcd_pci_probe);
 /**
  * usb_hcd_pci_remove - shutdown processing for PCI-based HCDs
  * @dev: USB Host Controller being removed
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep
  *
  * Reverses the effect of usb_hcd_pci_probe(), first invoking
  * the HCD's stop() method.  It is always called from a thread
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h
  * Root Hub interrupt transfers are polled using a timer if the
  * driver requests it; otherwise the driver is responsible for
  * calling usb_hcd_poll_rh_status() when an event occurs.
- *
- * Completions are called in_interrupt(), but they may or may not
- * be in_irq().
  */
 void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
 {
@@ -904,7 +901,8 @@ static void usb_bus_init (struct usb_bus
 /**
  * usb_register_bus - registers the USB host controller with the usb core
  * @bus: pointer to the bus to register
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Assigns a bus number, and links the controller into usbcore data
  * structures so that it can be seen by scanning the bus list.
@@ -939,7 +937,8 @@ static int usb_register_bus(struct usb_b
 /**
  * usb_deregister_bus - deregisters the USB host controller
  * @bus: pointer to the bus to deregister
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Recycles the bus number, and unlinks the controller from usbcore data
  * structures so that it won't be seen by scanning the bus list.
@@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned
  * @hcd: host controller returning the URB
  * @urb: urb being returned to the USB device driver.
  * @status: completion status code for the URB.
- * Context: in_interrupt()
  *
  * This hands the URB from HCD to its USB device driver, using its
  * completion function.  The HCD has freed all per-urb resources
@@ -2268,7 +2266,7 @@ EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hu
  * usb_bus_start_enum - start immediate enumeration (for OTG)
  * @bus: the bus (must use hcd framework)
  * @port_num: 1-based number of port; usually bus->otg_port
- * Context: in_interrupt()
+ * Context: atomic
  *
  * Starts enumeration, with an immediate reset followed later by
  * hub_wq identifying and possibly configuring the device.
@@ -2474,7 +2472,8 @@ EXPORT_SYMBOL_GPL(__usb_create_hcd);
  * @bus_name: value to store in hcd->self.bus_name
  * @primary_hcd: a pointer to the usb_hcd structure that is sharing the
  *              PCI device.  Only allocate certain resources for the primary HCD
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Allocate a struct usb_hcd, with extra space at the end for the
  * HC driver's private data.  Initialize the generic members of the
@@ -2496,7 +2495,8 @@ EXPORT_SYMBOL_GPL(usb_create_shared_hcd)
  * @driver: HC driver that will use this hcd
  * @dev: device for this HC, stored in hcd->self.controller
  * @bus_name: value to store in hcd->self.bus_name
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Allocate a struct usb_hcd, with extra space at the end for the
  * HC driver's private data.  Initialize the generic members of the
@@ -2830,7 +2830,8 @@ EXPORT_SYMBOL_GPL(usb_add_hcd);
 /**
  * usb_remove_hcd - shutdown processing for generic HCDs
  * @hcd: the usb_hcd structure to remove
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Disconnects the root hub, then reverses the effects of usb_add_hcd(),
  * invoking the HCD's stop() method.
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2171,7 +2171,8 @@ static void hub_disconnect_children(stru
 /**
  * usb_disconnect - disconnect a device (usbcore-internal)
  * @pdev: pointer to device being disconnected
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep
  *
  * Something got disconnected. Get rid of it and all of its children.
  *
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -119,7 +119,7 @@ static int usb_internal_control_msg(stru
  * @timeout: time in msecs to wait for the message to complete before timing
  *	out (if 0 the wait is forever)
  *
- * Context: !in_interrupt ()
+ * Context: task context, might sleep.
  *
  * This function sends a simple control message to a specified endpoint and
  * waits for the message to complete, or timeout.
@@ -173,7 +173,7 @@ EXPORT_SYMBOL_GPL(usb_control_msg);
  * @timeout: time in msecs to wait for the message to complete before
  *	timing out (if 0 the wait is forever)
  *
- * Context: !in_interrupt ()
+ * Context: task context, might sleep.
  *
  * This function sends a simple interrupt message to a specified endpoint and
  * waits for the message to complete, or timeout.
@@ -206,7 +206,7 @@ EXPORT_SYMBOL_GPL(usb_interrupt_msg);
  * @timeout: time in msecs to wait for the message to complete before
  *	timing out (if 0 the wait is forever)
  *
- * Context: !in_interrupt ()
+ * Context: task context, might sleep.
  *
  * This function sends a simple bulk message to a specified endpoint
  * and waits for the message to complete, or timeout.
@@ -473,7 +473,8 @@ EXPORT_SYMBOL_GPL(usb_sg_init);
  * usb_sg_wait - synchronously execute scatter/gather request
  * @io: request block handle, as initialized with usb_sg_init().
  * 	some fields become accessible when this call returns.
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * This function blocks until the specified I/O operation completes.  It
  * leverages the grouping of the related I/O requests to get good transfer
@@ -627,7 +628,8 @@ EXPORT_SYMBOL_GPL(usb_sg_cancel);
  * @index: the number of the descriptor
  * @buf: where to put the descriptor
  * @size: how big is "buf"?
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * Gets a USB descriptor.  Convenience functions exist to simplify
  * getting some types of descriptors.  Use
@@ -675,7 +677,8 @@ EXPORT_SYMBOL_GPL(usb_get_descriptor);
  * @index: the number of the descriptor
  * @buf: where to put the string
  * @size: how big is "buf"?
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * Retrieves a string, encoded using UTF-16LE (Unicode, 16 bits per character,
  * in little-endian byte order).
@@ -810,7 +813,8 @@ static int usb_get_langid(struct usb_dev
  * @index: the number of the descriptor
  * @buf: where to put the string
  * @size: how big is "buf"?
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * This converts the UTF-16LE encoded strings returned by devices, from
  * usb_get_string_descriptor(), to null-terminated UTF-8 encoded ones
@@ -899,7 +903,8 @@ char *usb_cache_string(struct usb_device
  * usb_get_device_descriptor - (re)reads the device descriptor (usbcore)
  * @dev: the device whose device descriptor is being updated
  * @size: how much of the descriptor to read
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * Updates the copy of the device descriptor stored in the device structure,
  * which dedicates space for this purpose.
@@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb
 /*
  * usb_set_isoch_delay - informs the device of the packet transmit delay
  * @dev: the device whose delay is to be informed
- * Context: !in_interrupt()
+ * Context: can sleep
  *
  * Since this is an optional request, we don't bother if it fails.
  */
@@ -962,7 +967,8 @@ int usb_set_isoch_delay(struct usb_devic
  * @type: USB_STATUS_TYPE_*; for standard or PTM status types
  * @target: zero (for device), else interface or endpoint number
  * @data: pointer to two bytes of bitmap data
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * Returns device, interface, or endpoint status.  Normally only of
  * interest to see if the device is self powered, or has enabled the
@@ -1039,7 +1045,8 @@ EXPORT_SYMBOL_GPL(usb_get_status);
  * usb_clear_halt - tells device to clear endpoint halt/stall condition
  * @dev: device whose endpoint is halted
  * @pipe: endpoint "pipe" being cleared
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * This is used to clear halt conditions for bulk and interrupt endpoints,
  * as reported by URB completion status.  Endpoints that are halted are
@@ -1343,7 +1350,8 @@ void usb_enable_interface(struct usb_dev
  * @dev: the device whose interface is being updated
  * @interface: the interface being updated
  * @alternate: the setting being chosen.
- * Context: !in_interrupt ()
+ *
+ * Context: task context, might sleep.
  *
  * This is used to enable data transfers on interfaces that may not
  * be enabled by default.  Not all devices support such configurability.
@@ -1762,7 +1770,8 @@ static void __usb_queue_reset_device(str
  * usb_set_configuration - Makes a particular device setting be current
  * @dev: the device whose configuration is being updated
  * @configuration: the configuration being chosen.
- * Context: !in_interrupt(), caller owns the device lock
+ *
+ * Context: task context, might sleep. Caller holds device lock.
  *
  * This is used to enable non-default device modes.  Not all devices
  * use this kind of configurability; many devices only have one
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -28,7 +28,6 @@
 #include <linux/string.h>
 #include <linux/bitops.h>
 #include <linux/slab.h>
-#include <linux/interrupt.h>  /* for in_interrupt() */
 #include <linux/kmod.h>
 #include <linux/init.h>
 #include <linux/spinlock.h>
@@ -561,7 +560,8 @@ static bool usb_dev_authorized(struct us
  * @parent: hub to which device is connected; null to allocate a root hub
  * @bus: bus used to access the device
  * @port1: one-based index of port; ignored for root hubs
- * Context: !in_interrupt()
+ *
+ * Context: task context, might sleep.
  *
  * Only hub drivers (including virtual root hub drivers for host
  * controllers) should ever call this.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [patch 12/12] usb: atm: Replace in_interrupt() usage in comment
  2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
                   ` (10 preceding siblings ...)
  2020-10-14 14:52 ` [patch 11/12] usb: core: Replace in_interrupt() in comments Thomas Gleixner
@ 2020-10-14 14:52 ` Thomas Gleixner
  11 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 14:52 UTC (permalink / raw)
  To: LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Duncan Sands, Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Johan Hovold, Mathias Nyman, Valentina Manea, Shuah Khan,
	Alan Stern, linux-omap, Kukjin Kim, Krzysztof Kozlowski,
	linux-arm-kernel, linux-samsung-soc, Felipe Balbi

in_interrupt() is a pretty vague context description as it means: hard
interrupt, soft interrupt or bottom half disabled regions.

Replace the vague comment with a proper reasoning why spin_lock_irqsave()
needs to be used.

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: Duncan Sands <duncan.sands@free.fr>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

---
 drivers/usb/atm/usbatm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -249,7 +249,7 @@ static void usbatm_complete(struct urb *
 	/* vdbg("%s: urb 0x%p, status %d, actual_length %d",
 	     __func__, urb, status, urb->actual_length); */
 
-	/* usually in_interrupt(), but not always */
+	/* Can be invoked from task context, protect against interrupts */
 	spin_lock_irqsave(&channel->lock, flags);
 
 	/* must add to the back when receiving; doesn't matter when sending */


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 08/12] usb: hosts: Remove in_interrupt() from comments
  2020-10-14 14:52 ` [patch 08/12] usb: hosts: Remove in_interrupt() from comments Thomas Gleixner
@ 2020-10-14 15:24   ` Krzysztof Kozlowski
  2020-10-14 16:20   ` Alan Stern
  1 sibling, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-14 15:24 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Peter Zijlstra, Ahmed S. Darwish,
	Sebastian Andrzej Siewior, Alan Stern, Greg Kroah-Hartman,
	linux-usb, linux-omap, Kukjin Kim, linux-arm-kernel,
	linux-samsung-soc, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, Felipe Balbi,
	Duncan Sands

On Wed, Oct 14, 2020 at 04:52:23PM +0200, Thomas Gleixner wrote:
> From: Ahmed S. Darwish <a.darwish@linutronix.de>
> 
> The usage of in_interrupt() in drivers is phased out for various reasons.
> 
> Various comments use !in_interrupt() to describe calling context for probe()
> and remove() functions. That's wrong because the calling context has to be
> preemptible task context, which is not what !in_interrupt() describes.
> 
> Cleanup the comments. While at it add the missing kernel doc argument
> descriptors and make usb_hcd_msp_remove() static.
> 
> 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: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> 
> ---
>  drivers/usb/host/ehci-fsl.c     |    9 ++++-----
>  drivers/usb/host/ehci-pmcmsp.c  |   15 +++++++++------
>  drivers/usb/host/ohci-at91.c    |   11 ++++++++---
>  drivers/usb/host/ohci-omap.c    |    7 ++++---
>  drivers/usb/host/ohci-pxa27x.c  |   11 ++++++-----
>  drivers/usb/host/ohci-s3c2410.c |   12 ++++++------

For the s3c2410:
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 07/12] usbip: Remove in_interrupt() check
  2020-10-14 14:52 ` [patch 07/12] usbip: Remove in_interrupt() check Thomas Gleixner
@ 2020-10-14 15:45   ` Shuah Khan
  0 siblings, 0 replies; 26+ messages in thread
From: Shuah Khan @ 2020-10-14 15:45 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: Peter Zijlstra, Ahmed S. Darwish, Sebastian Andrzej Siewior,
	Valentina Manea, Shuah Khan, Greg Kroah-Hartman, linux-usb,
	Thomas Winischhofer, Johan Hovold, Mathias Nyman, Alan Stern,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands, Shuah Khan

On 10/14/20 8:52 AM, Thomas Gleixner wrote:
> From: Ahmed S. Darwish <a.darwish@linutronix.de>
> 
> 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 separated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> usbip_recv() uses in_interrupt() to conditionally print context information
> for debugging messages. The value is zero as the function is only called
> from various *_rx_loop() kthread functions. Remove it.
> 
> 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: Valentina Manea <valentina.manea.m@gmail.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> 
> ---
>   drivers/usb/usbip/usbip_common.c |    5 -----
>   1 file changed, 5 deletions(-)
> 
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -324,11 +324,6 @@ int usbip_recv(struct socket *sock, void
>   	} while (msg_data_left(&msg));
>   
>   	if (usbip_dbg_flag_xmit) {
> -		if (!in_interrupt())
> -			pr_debug("%-10s:", current->comm);
> -		else
> -			pr_debug("interrupt  :");
> -
>   		pr_debug("receiving....\n");
>   		usbip_dump_buffer(buf, size);
>   		pr_debug("received, osize %d ret %d size %zd total %d\n",
> 
> 

Looks good to me.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 14:52 ` [patch 03/12] USB: serial: keyspan_pda: Consolidate room query Thomas Gleixner
@ 2020-10-14 16:14   ` Alan Stern
  2020-10-14 16:17     ` Thomas Gleixner
  2020-10-14 16:27     ` Sebastian Andrzej Siewior
  0 siblings, 2 replies; 26+ messages in thread
From: Alan Stern @ 2020-10-14 16:14 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Peter Zijlstra, Sebastian Andrzej Siewior, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On Wed, Oct 14, 2020 at 04:52:18PM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Having two copies of the same code doesn't make the code more readable and
> allocating a buffer of 1 byte for a synchronous operation is a pointless
> exercise.

Not so.  In fact, it is required, because a portion of a structure 
cannot be mapped for DMA unless it is aligned at a cache line boundary.

> Add a byte buffer to struct keyspan_pda_private which can be used
> instead. The buffer is only used in open() and tty->write().

This won't work.

>  Console writes
> are not calling into the query. open() obviously happens before write() and
> the writes are serialized by bit 0 of port->write_urbs_free which protects
> also the transaction itself.
> 
> Move the actual query into a helper function and cleanup the usage sites in
> keyspan_pda_write() and keyspan_pda_open().
> 
> 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 |  102 ++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 59 deletions(-)
> 
> --- a/drivers/usb/serial/keyspan_pda.c
> +++ b/drivers/usb/serial/keyspan_pda.c
> @@ -47,6 +47,7 @@ struct keyspan_pda_private {
>  	struct work_struct			unthrottle_work;
>  	struct usb_serial	*serial;
>  	struct usb_serial_port	*port;
> +	u8			query_buf;
>  };
>  
>  
> @@ -436,6 +437,31 @@ static int keyspan_pda_tiocmset(struct t
>  	return rc;
>  }
>  
> +/*
> + * Using priv->query_buf is safe here because this is only called for TTY
> + * operations open() and write(). write() comes post open() obviously and
> + * write() itself is serialized via bit 0 of port->write_urbs_free. Console
> + * writes are never calling into this.
> + */
> +static int keyspan_pda_query_room(struct usb_serial *serial,
> +				  struct keyspan_pda_private *priv)
> +{
> +	int res;
> +
> +	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +			      6, /* write_room */
> +			      USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN,
> +			      0, /* value */
> +			      0, /* index */
> +			      &priv->query_buf,
> +			      1,
> +			      2000);

Instead, consider using the new usb_control_msg_recv() API.  But it 
might be better to allocate the buffer once and for all.

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 16:14   ` Alan Stern
@ 2020-10-14 16:17     ` Thomas Gleixner
  2020-10-14 16:27     ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2020-10-14 16:17 UTC (permalink / raw)
  To: Alan Stern
  Cc: LKML, Peter Zijlstra, Sebastian Andrzej Siewior, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On Wed, Oct 14 2020 at 12:14, Alan Stern wrote:
> On Wed, Oct 14, 2020 at 04:52:18PM +0200, Thomas Gleixner wrote:
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> 
>> Having two copies of the same code doesn't make the code more readable and
>> allocating a buffer of 1 byte for a synchronous operation is a pointless
>> exercise.
>
> Not so.  In fact, it is required, because a portion of a structure 
> cannot be mapped for DMA unless it is aligned at a cache line boundary.
>
>> Add a byte buffer to struct keyspan_pda_private which can be used
>> instead. The buffer is only used in open() and tty->write().
>
> This won't work.

Ok.

>> +	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
>> +			      6, /* write_room */
>> +			      USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN,
>> +			      0, /* value */
>> +			      0, /* index */
>> +			      &priv->query_buf,
>> +			      1,
>> +			      2000);
>
> Instead, consider using the new usb_control_msg_recv() API.  But it 
> might be better to allocate the buffer once and for all.

Let me have a look.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 08/12] usb: hosts: Remove in_interrupt() from comments
  2020-10-14 14:52 ` [patch 08/12] usb: hosts: Remove in_interrupt() from comments Thomas Gleixner
  2020-10-14 15:24   ` Krzysztof Kozlowski
@ 2020-10-14 16:20   ` Alan Stern
  1 sibling, 0 replies; 26+ messages in thread
From: Alan Stern @ 2020-10-14 16:20 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Peter Zijlstra, Ahmed S. Darwish,
	Sebastian Andrzej Siewior, Greg Kroah-Hartman, linux-usb,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, Felipe Balbi,
	Duncan Sands

On Wed, Oct 14, 2020 at 04:52:23PM +0200, Thomas Gleixner wrote:
> From: Ahmed S. Darwish <a.darwish@linutronix.de>
> 
> The usage of in_interrupt() in drivers is phased out for various reasons.
> 
> Various comments use !in_interrupt() to describe calling context for probe()
> and remove() functions. That's wrong because the calling context has to be
> preemptible task context, which is not what !in_interrupt() describes.
> 
> Cleanup the comments. While at it add the missing kernel doc argument
> descriptors and make usb_hcd_msp_remove() static.
> 
> 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: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-omap@vger.kernel.org
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> 
> ---

> --- a/drivers/usb/host/ehci-pmcmsp.c
> +++ b/drivers/usb/host/ehci-pmcmsp.c

> @@ -233,7 +236,7 @@ int usb_hcd_msp_probe(const struct hc_dr
>   * may be called without controller electrically present
>   * may be called with controller, bus, and devices active
>   */
> -void usb_hcd_msp_remove(struct usb_hcd *hcd, struct platform_device *dev)
> +static void usb_hcd_msp_remove(struct usb_hcd *hcd)

Please don't intermix changes to comments with other more substantive 
changes.

> --- a/drivers/usb/host/ohci-omap.c
> +++ b/drivers/usb/host/ohci-omap.c

> @@ -399,8 +401,7 @@ static int ohci_hcd_omap_probe(struct pl
>  
>  /**
>   * ohci_hcd_omap_remove - shutdown processing for OMAP-based HCDs
> - * @dev: USB Host Controller being removed
> - * Context: !in_interrupt()
> + * @pdev: USB Host Controller being removed
>   *
>   * Reverses the effect of ohci_hcd_omap_probe(), first invoking
>   * the HCD's stop() method.  It is always called from a thread

You forgot to add the Context comment.

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments
  2020-10-14 14:52 ` [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments Thomas Gleixner
@ 2020-10-14 16:22   ` Alan Stern
  0 siblings, 0 replies; 26+ messages in thread
From: Alan Stern @ 2020-10-14 16:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Peter Zijlstra, Ahmed S. Darwish,
	Sebastian Andrzej Siewior, Felipe Balbi, Greg Kroah-Hartman,
	linux-usb, Thomas Winischhofer, Johan Hovold, Mathias Nyman,
	Valentina Manea, Shuah Khan, linux-omap, Kukjin Kim,
	Krzysztof Kozlowski, linux-arm-kernel, linux-samsung-soc,
	Duncan Sands

On Wed, Oct 14, 2020 at 04:52:25PM +0200, Thomas Gleixner wrote:
> From: Ahmed S. Darwish <a.darwish@linutronix.de>
> 
> The usage of in_irq()/in_interrupt() in drivers is phased out for various
> reasons.
> 
> The context description for usb_gadget_giveback_request() is misleading as
> in_interupt() means: hard interrupt or soft interrupt or bottom half
> disabled regions. But it's also invoked from task context when endpoints
> are torn down. Remove it as it's more confusing than helpful.
> 
> Replace also the in_irq() comment with plain text.
> 
> 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: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> 
> ---

> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -1754,8 +1754,9 @@ static int handle_control_request(struct
>  	return ret_val;
>  }
>  
> -/* drive both sides of the transfers; looks like irq handlers to
> - * both drivers except the callbacks aren't in_irq().
> +/* drive both sides of the transfers; looks like irq handlers to both
> + * drivers except that the callbacks are invoked from soft interrupt
> + * context.
>   */

You might as well fix the formatting of the multiline comment while 
you're changing its content.

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 16:14   ` Alan Stern
  2020-10-14 16:17     ` Thomas Gleixner
@ 2020-10-14 16:27     ` Sebastian Andrzej Siewior
  2020-10-14 16:34       ` Alan Stern
  1 sibling, 1 reply; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-10-14 16:27 UTC (permalink / raw)
  To: Alan Stern
  Cc: Thomas Gleixner, LKML, Peter Zijlstra, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On 2020-10-14 12:14:33 [-0400], Alan Stern wrote:
> Instead, consider using the new usb_control_msg_recv() API.  But it 
> might be better to allocate the buffer once and for all.

This will still allocate and free buffer on each invocation. What about
moving the query_buf to the begin of the struct / align it?

> Alan Stern

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 11/12] usb: core: Replace in_interrupt() in comments
  2020-10-14 14:52 ` [patch 11/12] usb: core: Replace in_interrupt() in comments Thomas Gleixner
@ 2020-10-14 16:27   ` Alan Stern
  2020-10-14 16:41     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 26+ messages in thread
From: Alan Stern @ 2020-10-14 16:27 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: LKML, Peter Zijlstra, Ahmed S. Darwish,
	Sebastian Andrzej Siewior, Greg Kroah-Hartman, linux-usb,
	Thomas Winischhofer, Johan Hovold, Mathias Nyman,
	Valentina Manea, Shuah Khan, linux-omap, Kukjin Kim,
	Krzysztof Kozlowski, linux-arm-kernel, linux-samsung-soc,
	Felipe Balbi, Duncan Sands

On Wed, Oct 14, 2020 at 04:52:26PM +0200, Thomas Gleixner wrote:
> From: Ahmed S. Darwish <a.darwish@linutronix.de>
> 
> The usage of in_interrupt() in drivers is phased out for various reasons.
> 
> Various comments use !in_interrupt() to describe calling context for
> functions which might sleep. That's wrong because the calling context has
> to be preemptible task context, which is not what !in_interrupt()
> describes.
> 
> Replace !in_interrupt() with more accurate plain text descriptions.
> 
> The comment for usb_hcd_poll_rh_status() is misleading as this function is
> called from all kinds of contexts including preemptible task
> context. Remove it as there is obviously no restriction.
> 
> 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> 
> ---

> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h
>   * Root Hub interrupt transfers are polled using a timer if the
>   * driver requests it; otherwise the driver is responsible for
>   * calling usb_hcd_poll_rh_status() when an event occurs.
> - *
> - * Completions are called in_interrupt(), but they may or may not
> - * be in_irq().

This comment should not be removed; instead it should be changed to say 
that completion handlers are called with interrupts disabled.

> @@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned
>   * @hcd: host controller returning the URB
>   * @urb: urb being returned to the USB device driver.
>   * @status: completion status code for the URB.
> - * Context: in_interrupt()

The comment should be changed to say that the routine runs in a BH 
handler (or however you want to express it).

> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c

> @@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb
>  /*
>   * usb_set_isoch_delay - informs the device of the packet transmit delay
>   * @dev: the device whose delay is to be informed
> - * Context: !in_interrupt()
> + * Context: can sleep

Why is this comment different from all the others?

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 16:27     ` Sebastian Andrzej Siewior
@ 2020-10-14 16:34       ` Alan Stern
  2020-10-14 16:44         ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 26+ messages in thread
From: Alan Stern @ 2020-10-14 16:34 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Thomas Gleixner, LKML, Peter Zijlstra, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On Wed, Oct 14, 2020 at 06:27:14PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-10-14 12:14:33 [-0400], Alan Stern wrote:
> > Instead, consider using the new usb_control_msg_recv() API.  But it 
> > might be better to allocate the buffer once and for all.
> 
> This will still allocate and free buffer on each invocation. What about

Yes.  That's why I suggesting doing a single buffer allocation at the 
start and using it for each I/O transfer.  (But I'm not familiar with 
this code, and I don't know if there might be multiple transfers going 
on concurrently.)

> moving the query_buf to the begin of the struct / align it?

No, thank won't work either.  The key to the issue is that while some 
memory is mapped for DMA, the CPU must not touch it or anything else in 
the same cache line.  If a field is a member of a data structure, the 
CPU might very well access a neighboring member while this one is 
mapped, thereby messing up the cache line.

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 11/12] usb: core: Replace in_interrupt() in comments
  2020-10-14 16:27   ` Alan Stern
@ 2020-10-14 16:41     ` Sebastian Andrzej Siewior
  2020-10-14 18:13       ` Alan Stern
  0 siblings, 1 reply; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-10-14 16:41 UTC (permalink / raw)
  To: Alan Stern
  Cc: Thomas Gleixner, LKML, Peter Zijlstra, Ahmed S. Darwish,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, linux-omap,
	Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On 2020-10-14 12:27:21 [-0400], Alan Stern wrote:
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h
> >   * Root Hub interrupt transfers are polled using a timer if the
> >   * driver requests it; otherwise the driver is responsible for
> >   * calling usb_hcd_poll_rh_status() when an event occurs.
> > - *
> > - * Completions are called in_interrupt(), but they may or may not
> > - * be in_irq().
> 
> This comment should not be removed; instead it should be changed to say 
> that completion handlers are called with interrupts disabled.

The timer callback:
  rh_timer_func() -> usb_hcd_poll_rh_status()  

invokes the function with enabled interrupts.

> > @@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned
> >   * @hcd: host controller returning the URB
> >   * @urb: urb being returned to the USB device driver.
> >   * @status: completion status code for the URB.
> > - * Context: in_interrupt()
> 
> The comment should be changed to say that the routine runs in a BH 
> handler (or however you want to express it).

Do you mean usb_hcd_giveback_urb() runs in BH context or that the
completion callback of the URB runs in BH context?
The completion callback of the URB may run in BH or IRQ context
depending on HCD.

> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> 
> > @@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb
> >  /*
> >   * usb_set_isoch_delay - informs the device of the packet transmit delay
> >   * @dev: the device whose delay is to be informed
> > - * Context: !in_interrupt()
> > + * Context: can sleep
> 
> Why is this comment different from all the others?

It says !in_interrupt() which is also true for preempt-disabled regions.
But the caller must not have preemption disabled. "can sleep" is more
obvious as what it needs.

> Alan Stern

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 03/12] USB: serial: keyspan_pda: Consolidate room query
  2020-10-14 16:34       ` Alan Stern
@ 2020-10-14 16:44         ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-10-14 16:44 UTC (permalink / raw)
  To: Alan Stern
  Cc: Thomas Gleixner, LKML, Peter Zijlstra, Johan Hovold,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer,
	Ahmed S. Darwish, Mathias Nyman, Valentina Manea, Shuah Khan,
	linux-omap, Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On 2020-10-14 12:34:25 [-0400], Alan Stern wrote:
> On Wed, Oct 14, 2020 at 06:27:14PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2020-10-14 12:14:33 [-0400], Alan Stern wrote:
> > > Instead, consider using the new usb_control_msg_recv() API.  But it 
> > > might be better to allocate the buffer once and for all.
> > 
> > This will still allocate and free buffer on each invocation. What about
> 
> Yes.  That's why I suggesting doing a single buffer allocation at the 
> start and using it for each I/O transfer.  (But I'm not familiar with 
> this code, and I don't know if there might be multiple transfers going 
> on concurrently.)

There are no concurrent transfer. There is a bit used as a lock. The
first one does the transfer, the other wait.

> > moving the query_buf to the begin of the struct / align it?
> 
> No, thank won't work either.  The key to the issue is that while some 
> memory is mapped for DMA, the CPU must not touch it or anything else in 
> the same cache line.  If a field is a member of a data structure, the 
> CPU might very well access a neighboring member while this one is 
> mapped, thereby messing up the cache line.

that is unfortunately true. Let me do the single buffer.

> Alan Stern

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 11/12] usb: core: Replace in_interrupt() in comments
  2020-10-14 16:41     ` Sebastian Andrzej Siewior
@ 2020-10-14 18:13       ` Alan Stern
  0 siblings, 0 replies; 26+ messages in thread
From: Alan Stern @ 2020-10-14 18:13 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Thomas Gleixner, LKML, Peter Zijlstra, Ahmed S. Darwish,
	Greg Kroah-Hartman, linux-usb, Thomas Winischhofer, Johan Hovold,
	Mathias Nyman, Valentina Manea, Shuah Khan, linux-omap,
	Kukjin Kim, Krzysztof Kozlowski, linux-arm-kernel,
	linux-samsung-soc, Felipe Balbi, Duncan Sands

On Wed, Oct 14, 2020 at 06:41:23PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-10-14 12:27:21 [-0400], Alan Stern wrote:
> > > --- a/drivers/usb/core/hcd.c
> > > +++ b/drivers/usb/core/hcd.c
> > > @@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h
> > >   * Root Hub interrupt transfers are polled using a timer if the
> > >   * driver requests it; otherwise the driver is responsible for
> > >   * calling usb_hcd_poll_rh_status() when an event occurs.
> > > - *
> > > - * Completions are called in_interrupt(), but they may or may not
> > > - * be in_irq().
> > 
> > This comment should not be removed; instead it should be changed to say 
> > that completion handlers are called with interrupts disabled.
> 
> The timer callback:
>   rh_timer_func() -> usb_hcd_poll_rh_status()  
> 
> invokes the function with enabled interrupts.

Well, it doesn't change the interrupt settings.  It might call 
usb_hcd_poll_rh_status() with interrupts enabled or disabled, depending 
on how it was called originally.

But that wasn't what I meant.  usb_hcd_poll_rh_status() calls 
usb_hcd_giveback_urb() with interrupts disabled always, and that routine 
may call __usb_hcd_giveback_urb(), which calls

	urb->complete(urb);

In this case the completion handler would be invoked with interrupts 
disabled.  Alternatively, __usb_hcd_giveback_urb() may be invoked from a 
BH handler, in which case the completion handler will run in softirq 
context with interrupts enabled.

So I guess it would be best to say that completion handlers may be 
called with interrupts enabled or disabled.  Or you might want to put 
such a comment in __usb_hcd_giveback_urb().

> > > @@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned
> > >   * @hcd: host controller returning the URB
> > >   * @urb: urb being returned to the USB device driver.
> > >   * @status: completion status code for the URB.
> > > - * Context: in_interrupt()
> > 
> > The comment should be changed to say that the routine runs in a BH 
> > handler (or however you want to express it).
> 
> Do you mean usb_hcd_giveback_urb() runs in BH context or that the
> completion callback of the URB runs in BH context?

Actually I meant that usb_hcd_giveback_urb_bh() runs in BH context.  
Sorry, I got confused about the location of this hunk.

To be explicit: The comment for usb_hcd_giveback_urb() should say that 
the function expects to be called with interrupts disabled (whether the 
context is task, atomic, BH, interrupt, etc. doesn't matter).

> The completion callback of the URB may run in BH or IRQ context
> depending on HCD.
> 
> > > --- a/drivers/usb/core/message.c
> > > +++ b/drivers/usb/core/message.c
> > 
> > > @@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb
> > >  /*
> > >   * usb_set_isoch_delay - informs the device of the packet transmit delay
> > >   * @dev: the device whose delay is to be informed
> > > - * Context: !in_interrupt()
> > > + * Context: can sleep
> > 
> > Why is this comment different from all the others?
> 
> It says !in_interrupt() which is also true for preempt-disabled regions.
> But the caller must not have preemption disabled. "can sleep" is more
> obvious as what it needs.

But all the other comments in this patch say:

 * Context: task context, might sleep.

Why doesn't this comment say the same thing?

Alan Stern

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage
  2020-10-14 14:52 ` [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage Thomas Gleixner
@ 2020-10-14 18:49   ` kernel test robot
  0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2020-10-14 18:49 UTC (permalink / raw)
  To: Thomas Gleixner, LKML
  Cc: kbuild-all, Peter Zijlstra, Ahmed S. Darwish,
	Sebastian Andrzej Siewior, Greg Kroah-Hartman, linux-usb,
	Thomas Winischhofer, Johan Hovold, Mathias Nyman,
	Valentina Manea


[-- Attachment #1: Type: text/plain, Size: 9561 bytes --]

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb-serial/usb-next balbi-usb/testing/next v5.9 next-20201013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Gleixner/UBS-Cleanup-in_interupt-in_irq-in_atomic-usage/20201014-232156
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ee4d0a1f1ad7068ce381fd87cac015b76b77de60
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Thomas-Gleixner/UBS-Cleanup-in_interupt-in_irq-in_atomic-usage/20201014-232156
        git checkout ee4d0a1f1ad7068ce381fd87cac015b76b77de60
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:405,
                    from include/linux/kernel.h:15,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/usb/host/isp1362-hcd.c:63:
   drivers/usb/host/isp1362.h: In function 'isp1362_show_regs':
>> drivers/usb/host/isp1362.h:799:19: error: 'isp1362_hcd' undeclared (first use in this function)
     799 |  isp1362_show_reg(isp1362_hcd, HCREVISION);
         |                   ^~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     157 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:420:2: note: in expansion of macro 'dynamic_pr_debug'
     420 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   drivers/usb/host/isp1362.h:543:4: note: in expansion of macro 'pr_debug'
     543 |    pr_debug(fmt); \
         |    ^~~~~~~~
   drivers/usb/host/isp1362.h:789:3: note: in expansion of macro 'DBG'
     789 |   DBG(0, "%-12s[%02x]: %08x\n", #r,     \
         |   ^~~
   drivers/usb/host/isp1362.h:790:37: note: in expansion of macro 'isp1362_read_reg32'
     790 |    ISP1362_REG_NO(ISP1362_REG_##r), isp1362_read_reg32(d, r)); \
         |                                     ^~~~~~~~~~~~~~~~~~
   drivers/usb/host/isp1362.h:799:2: note: in expansion of macro 'isp1362_show_reg'
     799 |  isp1362_show_reg(isp1362_hcd, HCREVISION);
         |  ^~~~~~~~~~~~~~~~
   drivers/usb/host/isp1362.h:799:19: note: each undeclared identifier is reported only once for each function it appears in
     799 |  isp1362_show_reg(isp1362_hcd, HCREVISION);
         |                   ^~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     157 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:420:2: note: in expansion of macro 'dynamic_pr_debug'
     420 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   drivers/usb/host/isp1362.h:543:4: note: in expansion of macro 'pr_debug'
     543 |    pr_debug(fmt); \
         |    ^~~~~~~~
   drivers/usb/host/isp1362.h:789:3: note: in expansion of macro 'DBG'
     789 |   DBG(0, "%-12s[%02x]: %08x\n", #r,     \
         |   ^~~
   drivers/usb/host/isp1362.h:790:37: note: in expansion of macro 'isp1362_read_reg32'
     790 |    ISP1362_REG_NO(ISP1362_REG_##r), isp1362_read_reg32(d, r)); \
         |                                     ^~~~~~~~~~~~~~~~~~
   drivers/usb/host/isp1362.h:799:2: note: in expansion of macro 'isp1362_show_reg'
     799 |  isp1362_show_reg(isp1362_hcd, HCREVISION);
         |  ^~~~~~~~~~~~~~~~

vim +/isp1362_hcd +799 drivers/usb/host/isp1362.h

a9d43091c5be1e7 Lothar Wassmann 2009-07-16  795  
ee4d0a1f1ad7068 Thomas Gleixner 2020-10-14  796  static void __attribute__((__unused__))
ee4d0a1f1ad7068 Thomas Gleixner 2020-10-14  797  isp1362_show_regs(struct isp1362_hcd *isp1362_hc, bool cached_inten)
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  798  {
a9d43091c5be1e7 Lothar Wassmann 2009-07-16 @799  	isp1362_show_reg(isp1362_hcd, HCREVISION);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  800  	isp1362_show_reg(isp1362_hcd, HCCONTROL);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  801  	isp1362_show_reg(isp1362_hcd, HCCMDSTAT);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  802  	isp1362_show_reg(isp1362_hcd, HCINTSTAT);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  803  	isp1362_show_reg(isp1362_hcd, HCINTENB);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  804  	isp1362_show_reg(isp1362_hcd, HCFMINTVL);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  805  	isp1362_show_reg(isp1362_hcd, HCFMREM);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  806  	isp1362_show_reg(isp1362_hcd, HCFMNUM);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  807  	isp1362_show_reg(isp1362_hcd, HCLSTHRESH);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  808  	isp1362_show_reg(isp1362_hcd, HCRHDESCA);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  809  	isp1362_show_reg(isp1362_hcd, HCRHDESCB);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  810  	isp1362_show_reg(isp1362_hcd, HCRHSTATUS);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  811  	isp1362_show_reg(isp1362_hcd, HCRHPORT1);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  812  	isp1362_show_reg(isp1362_hcd, HCRHPORT2);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  813  
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  814  	isp1362_show_reg(isp1362_hcd, HCHWCFG);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  815  	isp1362_show_reg(isp1362_hcd, HCDMACFG);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  816  	isp1362_show_reg(isp1362_hcd, HCXFERCTR);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  817  	isp1362_show_reg(isp1362_hcd, HCuPINT);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  818  
ee4d0a1f1ad7068 Thomas Gleixner 2020-10-14  819  	if (cached_inten)
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  820  		DBG(0, "%-12s[%02x]:     %04x\n", "HCuPINTENB",
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  821  			 ISP1362_REG_NO(ISP1362_REG_HCuPINTENB), isp1362_hcd->irqenb);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  822  	else
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  823  		isp1362_show_reg(isp1362_hcd, HCuPINTENB);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  824  	isp1362_show_reg(isp1362_hcd, HCCHIPID);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  825  	isp1362_show_reg(isp1362_hcd, HCSCRATCH);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  826  	isp1362_show_reg(isp1362_hcd, HCBUFSTAT);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  827  	isp1362_show_reg(isp1362_hcd, HCDIRADDR);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  828  	/* Access would advance fifo
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  829  	 * isp1362_show_reg(isp1362_hcd, HCDIRDATA);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  830  	 */
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  831  	isp1362_show_reg(isp1362_hcd, HCISTLBUFSZ);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  832  	isp1362_show_reg(isp1362_hcd, HCISTLRATE);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  833  	isp1362_show_reg(isp1362_hcd, HCINTLBUFSZ);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  834  	isp1362_show_reg(isp1362_hcd, HCINTLBLKSZ);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  835  	isp1362_show_reg(isp1362_hcd, HCINTLDONE);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  836  	isp1362_show_reg(isp1362_hcd, HCINTLSKIP);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  837  	isp1362_show_reg(isp1362_hcd, HCINTLLAST);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  838  	isp1362_show_reg(isp1362_hcd, HCINTLCURR);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  839  	isp1362_show_reg(isp1362_hcd, HCATLBUFSZ);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  840  	isp1362_show_reg(isp1362_hcd, HCATLBLKSZ);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  841  	/* only valid after ATL_DONE interrupt
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  842  	 * isp1362_show_reg(isp1362_hcd, HCATLDONE);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  843  	 */
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  844  	isp1362_show_reg(isp1362_hcd, HCATLSKIP);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  845  	isp1362_show_reg(isp1362_hcd, HCATLLAST);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  846  	isp1362_show_reg(isp1362_hcd, HCATLCURR);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  847  	isp1362_show_reg(isp1362_hcd, HCATLDTC);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  848  	isp1362_show_reg(isp1362_hcd, HCATLDTCTO);
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  849  }
a9d43091c5be1e7 Lothar Wassmann 2009-07-16  850  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65103 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 14:52 [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Thomas Gleixner
2020-10-14 14:52 ` [patch 01/12] USB: sisusbvga: Make console support depend on BROKEN Thomas Gleixner
2020-10-14 14:52 ` [patch 02/12] USB: serial: keyspan_pda: Replace in_interrupt() usage Thomas Gleixner
2020-10-14 14:52 ` [patch 03/12] USB: serial: keyspan_pda: Consolidate room query Thomas Gleixner
2020-10-14 16:14   ` Alan Stern
2020-10-14 16:17     ` Thomas Gleixner
2020-10-14 16:27     ` Sebastian Andrzej Siewior
2020-10-14 16:34       ` Alan Stern
2020-10-14 16:44         ` Sebastian Andrzej Siewior
2020-10-14 14:52 ` [patch 04/12] USB: serial: digi_acceleport: Remove in_interrupt() usage Thomas Gleixner
2020-10-14 14:52 ` [patch 05/12] usb: xhci: Remove in_interrupt() checks Thomas Gleixner
2020-10-14 14:52 ` [patch 06/12] usb: host: isp1362: Replace in_interrupt() usage Thomas Gleixner
2020-10-14 18:49   ` kernel test robot
2020-10-14 14:52 ` [patch 07/12] usbip: Remove in_interrupt() check Thomas Gleixner
2020-10-14 15:45   ` Shuah Khan
2020-10-14 14:52 ` [patch 08/12] usb: hosts: Remove in_interrupt() from comments Thomas Gleixner
2020-10-14 15:24   ` Krzysztof Kozlowski
2020-10-14 16:20   ` Alan Stern
2020-10-14 14:52 ` [patch 09/12] usb: gadget: pxa27x_udc: Replace in_interrupt() usage in comments Thomas Gleixner
2020-10-14 14:52 ` [patch 10/12] usb: gadget: udc: Remove in_interrupt()/in_irq() from comments Thomas Gleixner
2020-10-14 16:22   ` Alan Stern
2020-10-14 14:52 ` [patch 11/12] usb: core: Replace in_interrupt() in comments Thomas Gleixner
2020-10-14 16:27   ` Alan Stern
2020-10-14 16:41     ` Sebastian Andrzej Siewior
2020-10-14 18:13       ` Alan Stern
2020-10-14 14:52 ` [patch 12/12] usb: atm: Replace in_interrupt() usage in comment Thomas Gleixner

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git