Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
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 14:13:20 -0400
Message-ID: <20201014181320.GA723420@rowland.harvard.edu> (raw)
In-Reply-To: <20201014164123.hnqqkyrjrjytcxgz@linutronix.de>

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

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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-23 13:38   ` Mathias Nyman
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 [this message]
2020-10-14 14:52 ` [patch 12/12] usb: atm: Replace in_interrupt() usage in comment Thomas Gleixner
2020-10-23  8:01 ` [patch 00/12] UBS: Cleanup in_interupt/in_irq/in_atomic() usage Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201014181320.GA723420@rowland.harvard.edu \
    --to=stern@rowland.harvard.edu \
    --cc=a.darwish@linutronix.de \
    --cc=balbi@kernel.org \
    --cc=bigeasy@linutronix.de \
    --cc=duncan.sands@free.fr \
    --cc=gregkh@linuxfoundation.org \
    --cc=johan@kernel.org \
    --cc=kgene@kernel.org \
    --cc=krzk@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=peterz@infradead.org \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thomas@winischhofer.net \
    --cc=valentina.manea.m@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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