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 [thread overview]
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
next prev parent reply other threads:[~2020-10-14 18:13 UTC|newest]
Thread overview: 27+ 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 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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).