Re: [patch 11/12] usb: core: Replace in_interrupt() in comments

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	"Ahmed S. Darwish" <a.darwish@linutronix.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org,
	Thomas Winischhofer <thomas@winischhofer.net>,
	Johan Hovold <johan@kernel.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Valentina Manea <valentina.manea.m@gmail.com>,
	Shuah Khan <shuah@kernel.org>,
	linux-omap@vger.kernel.org, Kukjin Kim <kgene@kernel.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org,
	Felipe Balbi <balbi@kernel.org>,
	Duncan Sands <duncan.sands@free.fr>
Subject: Re: [patch 11/12] usb: core: Replace in_interrupt() in comments
Date: Wed, 14 Oct 2020 18:41:23 +0200	[thread overview]
Message-ID: <20201014164123.hnqqkyrjrjytcxgz@linutronix.de> (raw)
In-Reply-To: <20201014162721.GE712494@rowland.harvard.edu>

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