With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
overo gumstix, beagleboard, probably others) we see a high CPU load in a
kworker thread.
Between 2.6.33 and 2.6.34 musb_core.c changed.
IRQ handlers changed with the result that a worker in musb_core.c got
scheduled far more frequently than needed.
I've included a patch below against 3.7, but i think it'll apply against
mainline.
[I apologize for any whitespace mangling. I've also attached the patch.]
I'd like more eyeballs to tell me if this is right. I'd also like to
know who I need to talk to to get this pushed into mainline.
--Adam
# The MUSB IRQ schedules work on every interrupt.
# This is unnecessary, and causes excessive CPU load.
#
# Here we only schedule work if there is something for
# the worker to do.
Index: git/drivers/usb/musb/musb_core.c
===================================================================
--- git.orig/drivers/usb/musb/musb_core.c
+++ git/drivers/usb/musb/musb_core.c
@@ -925,7 +925,9 @@ b_host:
}
#endif
- schedule_work(&musb->irq_work);
+ if (musb->xceiv->state != musb->xceiv_old_state) {
+ schedule_work(&musb->irq_work);
+ }
return handled;
}
Hi Adam,
On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> overo gumstix, beagleboard, probably others) we see a high CPU load in a
> kworker thread.
>
> Between 2.6.33 and 2.6.34 musb_core.c changed.
>
> IRQ handlers changed with the result that a worker in musb_core.c got
> scheduled far more frequently than needed.
>
> I've included a patch below against 3.7, but i think it'll apply against
> mainline.
> [I apologize for any whitespace mangling. I've also attached the patch.]
>
> I'd like more eyeballs to tell me if this is right. I'd also like to
> know who I need to talk to to get this pushed into mainline.
Running the scripts/get_maintainer.pl script on your patch produces
Felipe Balbi <[email protected]> (maintainer:MUSB MULTIPOINT H...)
Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)
[email protected] (open list:MUSB MULTIPOINT H...)
[email protected] (open list)
Felipe Balbi (CC'ed) is the person who you should talk to.
While we're touching the subject of scripts, you should run the
scripts/checkpatch.pl script and fix errors and warnings before submitting
patches. Please see Documentation/SubmittingPatches.
Last (but not least) piece of advice, don't give up if you don't receive
replies to your patches. People are busy and mails fall to cracks from time to
time.
Felipe, apart from the coding style violation and the possibly missing
locking, what's your opinion on this ? Does the patch make sense ?
> # The MUSB IRQ schedules work on every interrupt.
> # This is unnecessary, and causes excessive CPU load.
> #
> # Here we only schedule work if there is something for
> # the worker to do.
>
> Index: git/drivers/usb/musb/musb_core.c
> ===================================================================
> --- git.orig/drivers/usb/musb/musb_core.c
> +++ git/drivers/usb/musb/musb_core.c
> @@ -925,7 +925,9 @@ b_host:
> }
> #endif
>
> - schedule_work(&musb->irq_work);
> + if (musb->xceiv->state != musb->xceiv_old_state) {
> + schedule_work(&musb->irq_work);
> + }
>
> return handled;
> }
--
Regards,
Laurent Pinchart
On Mon, Jul 21, 2014 at 05:28:58PM +0200, Laurent Pinchart wrote:
> Hi Adam,
>
> On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> > With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> > overo gumstix, beagleboard, probably others) we see a high CPU load in a
> > kworker thread.
> >
> > Between 2.6.33 and 2.6.34 musb_core.c changed.
> >
> > IRQ handlers changed with the result that a worker in musb_core.c got
> > scheduled far more frequently than needed.
> >
> > I've included a patch below against 3.7, but i think it'll apply against
> > mainline.
> > [I apologize for any whitespace mangling. I've also attached the patch.]
> >
> > I'd like more eyeballs to tell me if this is right. I'd also like to
> > know who I need to talk to to get this pushed into mainline.
>
> Running the scripts/get_maintainer.pl script on your patch produces
>
> Felipe Balbi <[email protected]> (maintainer:MUSB MULTIPOINT H...)
> Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)
> [email protected] (open list:MUSB MULTIPOINT H...)
> [email protected] (open list)
>
> Felipe Balbi (CC'ed) is the person who you should talk to.
>
> While we're touching the subject of scripts, you should run the
> scripts/checkpatch.pl script and fix errors and warnings before submitting
> patches. Please see Documentation/SubmittingPatches.
>
> Last (but not least) piece of advice, don't give up if you don't receive
> replies to your patches. People are busy and mails fall to cracks from time to
> time.
>
> Felipe, apart from the coding style violation and the possibly missing
> locking, what's your opinion on this ? Does the patch make sense ?
It's a duplication of the check which is already in musb_irq_work():
1742 static void musb_irq_work(struct work_struct *data)
1743 {
1744 struct musb *musb = container_of(data, struct musb, irq_work);
1745
1746 if (musb->xceiv->state != musb->xceiv_old_state) {
1747 musb->xceiv_old_state = musb->xceiv->state;
1748 sysfs_notify(&musb->controller->kobj, NULL, "mode");
1749 }
1750 }
That does look better, but I'd need the check inside musb_irq_work() to
be removed and commit log would have to improve a bit.
ps: there's no missing locking, musb_stage0_irq() is called within
musb_interrupt() which is called within a locked IRQ handler.
--
balbi
Hi Felipe and Adam,
On Monday 21 July 2014 10:40:52 Felipe Balbi wrote:
> On Mon, Jul 21, 2014 at 05:28:58PM +0200, Laurent Pinchart wrote:
> > On Wednesday 29 January 2014 08:44:57 Adam Wozniak wrote:
> > > With a USB 2.0 webcam attached to the OTG port on an OMAP3 (applies to
> > > overo gumstix, beagleboard, probably others) we see a high CPU load in a
> > > kworker thread.
> > >
> > > Between 2.6.33 and 2.6.34 musb_core.c changed.
> > >
> > > IRQ handlers changed with the result that a worker in musb_core.c got
> > > scheduled far more frequently than needed.
> > >
> > > I've included a patch below against 3.7, but i think it'll apply against
> > > mainline.
> > > [I apologize for any whitespace mangling. I've also attached the
> > > patch.]
> > >
> > > I'd like more eyeballs to tell me if this is right. I'd also like to
> > > know who I need to talk to to get this pushed into mainline.
> >
> > Running the scripts/get_maintainer.pl script on your patch produces
> >
> > Felipe Balbi <[email protected]> (maintainer:MUSB MULTIPOINT H...)
> > Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)
> > [email protected] (open list:MUSB MULTIPOINT H...)
> > [email protected] (open list)
> >
> > Felipe Balbi (CC'ed) is the person who you should talk to.
> >
> > While we're touching the subject of scripts, you should run the
> > scripts/checkpatch.pl script and fix errors and warnings before submitting
> > patches. Please see Documentation/SubmittingPatches.
> >
> > Last (but not least) piece of advice, don't give up if you don't receive
> > replies to your patches. People are busy and mails fall to cracks from
> > time to time.
> >
> > Felipe, apart from the coding style violation and the possibly missing
> > locking, what's your opinion on this ? Does the patch make sense ?
>
> It's a duplication of the check which is already in musb_irq_work():
>
> 1742 static void musb_irq_work(struct work_struct *data)
> 1743 {
> 1744 struct musb *musb = container_of(data, struct musb, irq_work);
> 1745
> 1746 if (musb->xceiv->state != musb->xceiv_old_state) {
> 1747 musb->xceiv_old_state = musb->xceiv->state;
> 1748 sysfs_notify(&musb->controller->kobj, NULL, "mode");
> 1749 }
> 1750 }
>
> That does look better, but I'd need the check inside musb_irq_work() to
> be removed and commit log would have to improve a bit.
OK. Adam, could you please modify the patch accordingly and resubmit it ?
> ps: there's no missing locking, musb_stage0_irq() is called within
> musb_interrupt() which is called within a locked IRQ handler.
I hadn't checked that, thank you for the confirmation.
--
Regards,
Laurent Pinchart