Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree
Hi, On Wed, Jul 10, 2013 at 10:42:27AM -0700, Julius Werner wrote: > Hi Felipe, > > This is intended to pull down a reset signal line, not to switch power > to the device. I could implement that with the regulator framework > too, but I think that would just be confusing and harder to understand > without providing any benefit. It's really just a plain old GPIO. alright, in that case how about using drivers/reset/ ? -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: macro replacement Signed-off-by: Daniel Ciocirlan
On Fri, Jul 12, 2013 at 09:48:08AM +0300, Daniel Ciocirlan wrote: > --- > drivers/usb/gadget/composite.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) you need to fix the patch by adding a commit log and moving your SoB to the commit log, rather than the subject. -- balbi signature.asc Description: Digital signature
[PATCH] usb: macro replacement Signed-off-by: Daniel Ciocirlan
--- drivers/usb/gadget/composite.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 55f4df6..9d25912 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -1138,7 +1138,7 @@ struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev, uc = copy_gadget_strings(sp, n_gstrings, n_strings); if (IS_ERR(uc)) - return ERR_PTR(PTR_ERR(uc)); + return ERR_CAST(uc); n_gs = get_containers_gs(uc); ret = usb_string_ids_tab(cdev, n_gs[0]->strings); -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question on skel_read func of usb_skeleton.c
On Fri, Jul 12, 2013 at 05:51:04AM +, WangChen wrote: > I'm writing a driver for OSRFX2 learning board to test bulkloop > read/write. What's wrong with the existing usb-serial driver that can easily do this? > I think non-blocking write is good for such device due to > the FX2 chipset has dual-buffer for bulk endpoints. I am just > wondering if I also need to support blocking write IO due to I used to > think from driver perspective, for example for the read/write > operations, we need to support both blocking and nonblocking by > default. As you control the userspace interaction to the device, you can decide on this, right? Also, why even write a kernel driver at all for this, why not just use libusb or usbfs directly from userspace? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: question on skel_read func of usb_skeleton.c
I'm writing a driver for OSRFX2 learning board to test bulkloop read/write. I think non-blocking write is good for such device due to the FX2 chipset has dual-buffer for bulk endpoints. I am just wondering if I also need to support blocking write IO due to I used to think from driver perspective, for example for the read/write operations, we need to support both blocking and nonblocking by default. So you mean on this question, driver writer can determine if support one or both, then app programmer may have to consult with driver writer to know about this before writing their application? Looks like this is not a tech question ... > Date: Thu, 11 Jul 2013 21:56:39 -0700 > From: gre...@linuxfoundation.org > To: unicorn_w...@outlook.com > CC: oneu...@suse.de; linux-usb@vger.kernel.org > Subject: Re: question on skel_read func of usb_skeleton.c > > On Fri, Jul 12, 2013 at 04:49:54AM +, WangChen wrote: >> Hi, Oliver, >> Regarding skel_write, I see your current desgin only refuse its >> execution when> WRITES_IN_FLIGHT are on the fly, but this is not >> blocking IO due to write() will not wait before callback returns, >> right? Do you think it's unnecessary to support blocking IO on write >> or any special reason else? > > Take a step back. > > What type of USB driver do you wish to write? For what type of device? > That will answer usually all of these types of questions as the > userspace api for the device will dictate this type of thing. > > thanks, > > greg k-h
[PATCH] usb: renesas_usbhs: tidyup original usbhsx_for_each_xxx macro
Current usbhsx_for_each_xxx macro will read out-of-array's
memory after last loop operation.
It was not good C language operation, and the binary which was
compiled by (at least) gcc 4.8.1 is broken
This patch tidyup these issues
Reported-by: Yusuke Goda
Reviewed-by: Takashi Yoshii
Signed-off-by: Kuninori Morimoto
---
drivers/usb/renesas_usbhs/mod_gadget.c |6 +++---
drivers/usb/renesas_usbhs/mod_host.c |6 +++---
drivers/usb/renesas_usbhs/pipe.h |6 +++---
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
b/drivers/usb/renesas_usbhs/mod_gadget.c
index ed4949f..28a07de 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -77,9 +77,9 @@ struct usbhsg_recip_handle {
struct usbhsg_gpriv, mod)
#define __usbhsg_for_each_uep(start, pos, g, i)\
- for (i = start, pos = (g)->uep + i; \
-i < (g)->uep_size; \
-i++, pos = (g)->uep + i)
+ for ((i) = start; \
+((i) < (g)->uep_size) && ((pos) = (g)->uep + (i)); \
+(i)++)
#define usbhsg_for_each_uep(pos, gpriv, i) \
__usbhsg_for_each_uep(1, pos, gpriv, i)
diff --git a/drivers/usb/renesas_usbhs/mod_host.c
b/drivers/usb/renesas_usbhs/mod_host.c
index b868154..e40f565 100644
--- a/drivers/usb/renesas_usbhs/mod_host.c
+++ b/drivers/usb/renesas_usbhs/mod_host.c
@@ -111,9 +111,9 @@ static const char usbhsh_hcd_name[] = "renesas_usbhs host";
container_of(usbhs_mod_get(priv, USBHS_HOST), struct usbhsh_hpriv, mod)
#define __usbhsh_for_each_udev(start, pos, h, i) \
- for (i = start, pos = (h)->udev + i;\
-i < USBHSH_DEVICE_MAX; \
-i++, pos = (h)->udev + i)
+ for ((i) = start; \
+((i) < USBHSH_DEVICE_MAX) && ((pos) = (h)->udev + (i));\
+(i)++)
#define usbhsh_for_each_udev(pos, hpriv, i)\
__usbhsh_for_each_udev(1, pos, hpriv, i)
diff --git a/drivers/usb/renesas_usbhs/pipe.h b/drivers/usb/renesas_usbhs/pipe.h
index b476fde..3e53498 100644
--- a/drivers/usb/renesas_usbhs/pipe.h
+++ b/drivers/usb/renesas_usbhs/pipe.h
@@ -54,9 +54,9 @@ struct usbhs_pipe_info {
* pipe list
*/
#define __usbhs_for_each_pipe(start, pos, info, i) \
- for (i = start, pos = (info)->pipe + i; \
-i < (info)->size; \
-i++, pos = (info)->pipe + i)
+ for ((i) = start; \
+((i) < (info)->size) && ((pos) = (info)->pipe + (i)); \
+(i)++)
#define usbhs_for_each_pipe(pos, priv, i) \
__usbhs_for_each_pipe(1, pos, &((priv)->pipe_info), i)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question on skel_read func of usb_skeleton.c
On Fri, Jul 12, 2013 at 04:49:54AM +, WangChen wrote: > Hi, Oliver, > Regarding skel_write, I see your current desgin only refuse its > execution when> WRITES_IN_FLIGHT are on the fly, but this is not > blocking IO due to write() will not wait before callback returns, > right? Do you think it's unnecessary to support blocking IO on write > or any special reason else? Take a step back. What type of USB driver do you wish to write? For what type of device? That will answer usually all of these types of questions as the userspace api for the device will dictate this type of thing. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: question on skel_read func of usb_skeleton.c
Hi, Oliver, Regarding skel_write, I see your current desgin only refuse its execution when> WRITES_IN_FLIGHT are on the fly, but this is not blocking IO due to write() will not wait before callback returns, right? Do you think it's unnecessary to support blocking IO on write or any special reason else? > From: unicorn_w...@outlook.com > To: oneu...@suse.de > CC: linux-usb@vger.kernel.org > Subject: RE: question on skel_read func of usb_skeleton.c > Date: Thu, 11 Jul 2013 09:07:22 + > > Thank you, Oliver. I'm getting to understand your code :) > >> From: oneu...@suse.de >> To: unicorn_w...@outlook.com >> CC: linux-usb@vger.kernel.org >> Subject: Re: question on skel_read func of usb_skeleton.c >> Date: Tue, 9 Jul 2013 12:01:19 +0200 >> >> On Tuesday 09 July 2013 09:32:35 WangChen wrote: >>> Plus, regarding the reset case, urb-status will be non-zero, won't it? >> >> Only at the first call to read() >> >> Regards >> Oliver >>
Re: Help adding trace events to xHCI
Sarah Sharp writes: > My initial list of specific trace points was something like: > > 1. xHCI host initialization and shutdown > > 2. xHCI memory allocation (dynamic ring resizing, structure alloc, etc) > > 3. A few individual xHCI host controller command tracepoints: >* status only for all completed commands >* Address Device command status and output >* Configure Endpoint and Evaluate Context output >* individual trace points for other xHCI commands > > 4. Tracepoints for all USB transfer types: >* Control TX output (only for non-successful transfers) >* Bulk TX >* Interrupt TX >* Isoc TX > > 5. URB cancellation > > And probably more. Basically, I want to be able to control what gets > printed, based on where I think the xHCI bug might be. Does that sound > reasonable? Instead of individual trace points for command I would recommend to consider just pushing the whole command buffer to the trace point and parse the command in trace-cmd plugin in user space. Kernel code would be simpler that way. -- Kalle Valo -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help adding trace events to xHCI
Johannes Berg writes: >> (Mentors and wireless folks, we're struggling a bit with adding trace >> events to the xHCI USB host controller driver. I'm trying to look at >> the ath6kl driver trace events as an example. We could use some help >> and/or advice.) > > Those were in turn modelled on mac80211, cfg80211 and/or iwlwifi, I > think; might be worth also looking there. In general though, it's all > pretty similar. Correct, I used your code as example. > The message tracing was really more of an after-thought in iwlwifi (and > ath6kl as well I guess) because we already had a lot of debug messages > and capturing it all together can be useful. > > However, tracing all the debug messages is actually fairly expensive, I > think in part because of the string formatting and in part because of > the always-max allocations in the ringbuffer. The reason why added message tracing to ath6kl was that I wanted to combine wpasupplicant, cfg80211 and ath6kl debug messages all into file with synchronised timestamps. In this case I wasn't actually concerned about performance at all. -- Kalle Valo -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
Hi Peter,
> On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote:
> > Hi Peter,
> >
> > > This patchset adds tested otg id switch function and
> > > vbus connect and disconnect detection for chipidea driver.
> > > And fix kinds of bugs found at chipidea drivers after enabling
> > > id and vbus detection.
> > >
> > > This patch is fully tested at imx6 sabresd platform.
> > > My chipidea repo: https://github.com/hzpeterchen/linux-usb.git
> > >
> > > Changes for v12:
> > > - Rebased greg's usb-next tree (3.10.0-rc7+)
> > > - Split more small patches for single function and fix.
> >
> > I tested the patchset. Here are the results:
> >
> > - VBUS switching
> >
> > I'm no longer getting any ID interrupts at all when I apply the patch
> > below. The board stays in HOST mode all the time. If I configure it as
> > peripheral, it works as peripheral. Note with [1], I was able to switch
> > from Peripheral->Host , not the other way around.
>
> Thanks for your testing. But first, can you have me check
> if your ID wakeup is enabled?
ID wakeup? How do I check?
> I can have a test at mx28evk.
> Is it current upstream kernel can boot mx28evk run?
Yes, I use next-20130711 and it works fine.
> I have a RevC board, I would like to know if
> any patches needed.
Apply just this patchset and you should be all set.
> > --- a/arch/arm/boot/dts/imx28-m28evk.dts
> > +++ b/arch/arm/boot/dts/imx28-m28evk.dts
> > @@ -240,6 +240,8 @@
> >
> > ahb@8008 {
> >
> > usb0: usb@8008 {
> >
> > + dr_mode = "otg";
> > + phy_mode = "utmi";
> >
> > vbus-supply = <®_usb0_vbus>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&usbphy0_pins_a>;
> >
> > ---
> >
> > - MX23 UDC issue
> >
> > I found a workaround. Now running 'dmesg' via telnet through USB CDC link
> > no longer hangs the USB driver, but works as expected. I applied this
> > small patch that enables the streaming mode. Works on MX23EVK. It's
> > surprising this issue doesn't manifest on MX28, maybe MX28 contains a
> > new revision of the controller. I remember there was some discussion
> > about the streaming mode on MXS some time ago.
>
> It seems not reasonable
I talked to Fabio and he said he met similar issue on MX6, where disabling the
streaming mode fixed the problem. Could it possibly be so that the
non-streaming
mode is broken on mx23?
btw. I also tested this on STMP3780-based device, works the same way as MX23
does, streaming mode has to be enabled.
> the same question for mx23evk,
> it can run well with current kernel? I have no board on hand,
> let me see if I can find one.
Yes, MX23EVK rev. B2 works just fine with next-20130711 . You need to add the
DT
USB sections for MX23EVK, but they can be copied from imx28.dtsi +
imx28-evk.dts
.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 06/13] usb: chipidea: add otg_cap attribute for otg capable
Dear Peter Chen,
> On Thu, Jul 11, 2013 at 05:36:10PM +0200, Marek Vasut wrote:
> > Dear Peter Chen,
> >
> > > Since we need otgsc to know vbus's status at some chipidea
> > > controllers even it is peripheral-only mode. Besides, some
> > > SoCs (eg, AR9331 SoC) don't have otgsc register even
> > > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS.
> > > We inroduce otg_cap attribute to indicate if the controller
> > > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC
> > > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if
> > > there is exception, the platform can override it by device tree or
> > > platform data.
> > >
> > > Signed-off-by: Peter Chen
> > > ---
> > >
> > > drivers/usb/chipidea/core.c | 35
> > > --- include/linux/usb/chipidea.h |
> > > 13 +
> > > 2 files changed, 41 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > index 93961ff..e8ceb04 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc
> > > *ci)
> > >
> > > ci_hdrc_host_destroy(ci);
> > >
> > > }
> > >
> > > +static void ci_get_otg_capable(struct ci_hdrc *ci)
> > > +{
> > > + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED)
> >
> > IS_NONEXISTENT , no?
>
> You mean the English for this string? ok, I can change.
Yes, IS_NOT_EXISTED doesn't make much sense. Sorry, don't mean to nitpick.
> It means if the "otg_cap" is not existed at platform data,
> then, the "otg_cap" can be determined by DCCPARAMS
Then maybe just call it OTG_CAP_ATTR_NOT_SET or something .
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Linux USB file storage gadget with new UDC
Hi, > However, the USB-2 spec says (section 9.2.6.4) that devices should be > able to carry out requests with no Data stage (such as Set-Config) > within 50 ms. Does your gadget really take longer than that to handle > the exception? > > To find out, collect a usbmon trace showing what happens when your new > driver is plugged into a Linux host. > >> I have set the NAK and >> stall the endpoint 0 after receiving Set-Config request, however, > > That doesn't make sense. Stalling the endpoint means sending a STALL > packet. You can't send both a STALL and a NAK. > >> Get-Config request is still sent out by USBCV host immediately. > > There should be at least a 50-ms delay, unless the UDC driver is doing > something wrong. The latest usbmon trace is attached. From the trace, the timing is within 50ms from Set-Config request to the next request. > Does your gadget really take longer than that to handle the exception? Yes, i think there is a delay before gadget calls the handle_exception() routine. So the problem is before handle_exception() of Set-Config request is called, the next request is sent out already by the host. So if the next request is Get-Config, it will not return the latest config value. Thanks, victor usbmon_july12.log Description: Binary data
Re: [PATCH v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote:
> Hi Peter,
>
> > This patchset adds tested otg id switch function and
> > vbus connect and disconnect detection for chipidea driver.
> > And fix kinds of bugs found at chipidea drivers after enabling
> > id and vbus detection.
> >
> > This patch is fully tested at imx6 sabresd platform.
> > My chipidea repo: https://github.com/hzpeterchen/linux-usb.git
> >
> > Changes for v12:
> > - Rebased greg's usb-next tree (3.10.0-rc7+)
> > - Split more small patches for single function and fix.
>
> I tested the patchset. Here are the results:
>
> - VBUS switching
>
> I'm no longer getting any ID interrupts at all when I apply the patch below.
> The
> board stays in HOST mode all the time. If I configure it as peripheral, it
> works
> as peripheral. Note with [1], I was able to switch from Peripheral->Host ,
> not
> the other way around.
Thanks for your testing. But first, can you have me check
if your ID wakeup is enabled?
I can have a test at mx28evk.
Is it current upstream kernel can boot mx28evk run?
I have a RevC board, I would like to know if
any patches needed.
>
> --- a/arch/arm/boot/dts/imx28-m28evk.dts
> +++ b/arch/arm/boot/dts/imx28-m28evk.dts
> @@ -240,6 +240,8 @@
>
> ahb@8008 {
> usb0: usb@8008 {
> + dr_mode = "otg";
> + phy_mode = "utmi";
> vbus-supply = <®_usb0_vbus>;
> pinctrl-names = "default";
> pinctrl-0 = <&usbphy0_pins_a>;
>
> ---
>
> - MX23 UDC issue
>
> I found a workaround. Now running 'dmesg' via telnet through USB CDC link no
> longer hangs the USB driver, but works as expected. I applied this small
> patch
> that enables the streaming mode. Works on MX23EVK. It's surprising this issue
> doesn't manifest on MX28, maybe MX28 contains a new revision of the
> controller.
> I remember there was some discussion about the streaming mode on MXS some
> time
> ago.
It seems not reasonable, the same question for mx23evk,
it can run well with current kernel? I have no board on hand,
let me see if I can find one.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 06/13] usb: chipidea: add otg_cap attribute for otg capable
On Thu, Jul 11, 2013 at 05:36:10PM +0200, Marek Vasut wrote:
> Dear Peter Chen,
>
> > Since we need otgsc to know vbus's status at some chipidea
> > controllers even it is peripheral-only mode. Besides, some
> > SoCs (eg, AR9331 SoC) don't have otgsc register even
> > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS.
> > We inroduce otg_cap attribute to indicate if the controller
> > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC
> > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there
> > is exception, the platform can override it by device tree or platform data.
> >
> > Signed-off-by: Peter Chen
> > ---
> > drivers/usb/chipidea/core.c | 35 ---
> > include/linux/usb/chipidea.h | 13 +
> > 2 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 93961ff..e8ceb04 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc *ci)
> > ci_hdrc_host_destroy(ci);
> > }
> >
> > +static void ci_get_otg_capable(struct ci_hdrc *ci)
> > +{
> > + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED)
>
> IS_NONEXISTENT , no?
>
You mean the English for this string? ok, I can change.
It means if the "otg_cap" is not existed at platform data,
then, the "otg_cap" can be determined by DCCPARAMS
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Jul 11 (usb/chipidea)
On Fri, Jul 12, 2013 at 2:47 AM, Randy Dunlap wrote: > On 07/10/13 21:17, Stephen Rothwell wrote: >> Hi all, >> >> Changes since 20130710: >> > > > on x86_64: > > # CONFIG_USB_GADGET is not set > > ERROR: "usb_gadget_map_request" [drivers/usb/chipidea/ci_hdrc.ko] undefined! > ERROR: "usb_del_gadget_udc" [drivers/usb/chipidea/ci_hdrc.ko] undefined! > ERROR: "usb_gadget_unmap_request" [drivers/usb/chipidea/ci_hdrc.ko] undefined! > ERROR: "usb_add_gadget_udc" [drivers/usb/chipidea/ci_hdrc.ko] undefined! > > > > Full randconfig file is attached. > > Should be fixed with below fix, the problem is the same with host. http://marc.info/?l=linux-usb&m=137331577204167&w=2 -- BR, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/50] USB: usblp: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 11:33 PM, Pete Zaitcev wrote: > On Thu, 11 Jul 2013 17:05:26 +0800 > Ming Lei wrote: > >> Complete() will be run with interrupt enabled, so change to >> spin_lock_irqsave(). >> >> Cc: Pete Zaitcev >> Signed-off-by: Ming Lei > > Signed-off-by: Pete Zaitcev Thank you. > Good luck with that, Ming. I think the spin_lock_irqsave thing should've > been done back in days of uhci-hcd. But we always had some super annoying > edge cases when we considered it previously. I see Alan Stern signed off > the HCD changes at the beginning of July, so you're on the right track. > > BTW, perfect formatting in usblp.c, too bad for Sergei's heartburn ^_^ > > -- Pete > > P.S. Is it just me, or you forgot to fix usb-skeleton.c in your 50 > patches? It is my script which ignored the top directory of drivers/usb, :-) and I will add it in the next round, thanks for pointing it out. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] xhci: add traces for debug messages in xhci_address_device()
On 07/12/2013 03:54 AM, Greg KH wrote:
Minor coding style nits below:
On Fri, Jul 12, 2013 at 03:36:27AM +0300, Xenia Ragiadakou wrote:
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -21,6 +21,7 @@
*/
#include "xhci.h"
+#include "xhci-trace.h"
#define XHCI_INIT_VALUE 0x0
@@ -580,3 +581,16 @@ void xhci_dbg_ctx(struct xhci_hcd *xhci,
xhci_dbg_slot_ctx(xhci, ctx);
xhci_dbg_ep_ctx(xhci, ctx, last_ep);
}
+
+void xhci_dbg_address(struct xhci_hcd *xhci, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+ xhci_dbg(xhci, "%pV", &vaf);
+trace_xhci_dbg_address(&vaf);
+va_end(args);
You forgot to run your patch through checkpatch.pl :)
Nope, I forgot to do 'add' again after i corrected it :)
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.c
@@ -0,0 +1,21 @@
+/*
+ * xHCI host controller driver
+ *
+ * Author: Ksenia Ragiadakou
Email address here please. Actually, it should have your copyright, not
just "Author:".
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
We don't want to track the office of the FSF for the next 40+ years, so
both of these two paragraphs can be removed (we all know what the GPL
is.)
+ */
+
+#define CREATE_TRACE_POINTS
+#include "xhci-trace.h"
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
new file mode 100644
index 000..a3e9ca3
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.h
@@ -0,0 +1,55 @@
+/*
+ * xHCI host controller driver
+ *
+ * Author: Ksenia Ragiadakou
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
Same comments here as above.
greg k-h
I hope it is fine now, after the changes.
Thx for the quick comments!
regards,
ksenia
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2] xhci: add traces for debug messages in xhci_address_device()
This patch declares an event class for trace events that
trace messages with variadic arguments, called xhci_log_msg,
and defines a trace event for tracing the debug messages in
xhci_address_device() function, called xhci_dbg_address.
In order to implement this type of trace events, a wrapper function,
called xhci_dbg_address(), was created that records the format string
and variadic arguments into a va_format structure which is passed as
argument to the tracepoints, called trace_xhci_dbg_address().
All the xhci_dbg() calls in xhci_address_device() are replaced
with calls to xhci_dbg_address(). The functionality of xhci_dbg()
log messages was not removed though, but it is placed inside
xhci_dbg_address().
This trace event aims to give the ability to the user or the
developper to isolate and trace the debug messages generated
when a Address Device Command is issued to xHC.
Also, the unnecessary cast (unsigned long long) is removed
from le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id])
to reduce line length below 80 characters.
Signed-off-by: Xenia Ragiadakou
---
Differences from version 1:
Fix checkpatch.pl errors
Fix Copyright and mail information
Informationally:
By adding the tracepoints, the text segment size of xhci-hcd.o
was increased by 0.6%.
One way to enable the above trace event is:
$ echo 1 > tracing/events/xhci-hcd/xhci_dbg_address/enable
Similar output to this of the above trace can be achieved, if dynamic
debugging is enabled, by:
$ echo -n 'module xhci_hcd func xhci_debug_address +pf' > dynamic_debug/control
$ dmesg | grep "xhci_dbg_address"
drivers/usb/host/Makefile | 4
drivers/usb/host/xhci-dbg.c | 14 +
drivers/usb/host/xhci-trace.c | 15 +
drivers/usb/host/xhci-trace.h | 49 +++
drivers/usb/host/xhci.c | 33 +++--
drivers/usb/host/xhci.h | 1 +
6 files changed, 100 insertions(+), 16 deletions(-)
create mode 100644 drivers/usb/host/xhci-trace.c
create mode 100644 drivers/usb/host/xhci-trace.h
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index bea7112..ca0a119 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -4,6 +4,9 @@
ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
+# tell define_trace.h where to find the xhci trace header
+CFLAGS_xhci-trace.o := -I$(src)
+
isp1760-y := isp1760-hcd.o isp1760-if.o
fhci-y := fhci-hcd.o fhci-hub.o fhci-q.o
@@ -13,6 +16,7 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o
xhci-hcd-y := xhci.o xhci-mem.o
xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
+xhci-hcd-y += xhci-trace.o
xhci-hcd-$(CONFIG_PCI) += xhci-pci.o
ifneq ($(CONFIG_USB_XHCI_PLATFORM), )
diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5d5e58f..22fc6d7 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -21,6 +21,7 @@
*/
#include "xhci.h"
+#include "xhci-trace.h"
#define XHCI_INIT_VALUE 0x0
@@ -580,3 +581,16 @@ void xhci_dbg_ctx(struct xhci_hcd *xhci,
xhci_dbg_slot_ctx(xhci, ctx);
xhci_dbg_ep_ctx(xhci, ctx, last_ep);
}
+
+void xhci_dbg_address(struct xhci_hcd *xhci, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+ xhci_dbg(xhci, "%pV", &vaf);
+ trace_xhci_dbg_address(&vaf);
+ va_end(args);
+}
diff --git a/drivers/usb/host/xhci-trace.c b/drivers/usb/host/xhci-trace.c
new file mode 100644
index 000..7cf30c8
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.c
@@ -0,0 +1,15 @@
+/*
+ * xHCI host controller driver
+ *
+ * Copyright (C) 2013 Xenia Ragiadakou
+ *
+ * Author: Xenia Ragiadakou
+ * Email : burzalod...@gmail.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define CREATE_TRACE_POINTS
+#include "xhci-trace.h"
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
new file mode 100644
index 000..432aa67
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.h
@@ -0,0 +1,49 @@
+/*
+ * xHCI host controller driver
+ *
+ * Copyright (C) 2013 Xenia Ragiadakou
+ *
+ * Author: Xenia Ragiadakou
+ * Email : burzalod...@gmail.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM xhci-hcd
+
+#if !defined(__XHCI_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define __XHCI_TRACE_H
+
+#include
+
+#define XHCI_MSG_MAX 500
+
+DECLARE_EVENT_CLASS(xhci_log_msg,
+ TP_PROTO(struct va_format *vaf),
+ TP_ARGS(vaf),
+ TP_STRUCT__entry(__dynamic_array(char, msg, XHCI_MSG_MAX)),
+ TP_fast_assign(
+ vsnprintf(__get_str(msg), XH
Re: [RFC] xhci: add traces for debug messages in xhci_address_device()
Minor coding style nits below:
On Fri, Jul 12, 2013 at 03:36:27AM +0300, Xenia Ragiadakou wrote:
> --- a/drivers/usb/host/xhci-dbg.c
> +++ b/drivers/usb/host/xhci-dbg.c
> @@ -21,6 +21,7 @@
> */
>
> #include "xhci.h"
> +#include "xhci-trace.h"
>
> #define XHCI_INIT_VALUE 0x0
>
> @@ -580,3 +581,16 @@ void xhci_dbg_ctx(struct xhci_hcd *xhci,
> xhci_dbg_slot_ctx(xhci, ctx);
> xhci_dbg_ep_ctx(xhci, ctx, last_ep);
> }
> +
> +void xhci_dbg_address(struct xhci_hcd *xhci, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> + xhci_dbg(xhci, "%pV", &vaf);
> +trace_xhci_dbg_address(&vaf);
> +va_end(args);
You forgot to run your patch through checkpatch.pl :)
> --- /dev/null
> +++ b/drivers/usb/host/xhci-trace.c
> @@ -0,0 +1,21 @@
> +/*
> + * xHCI host controller driver
> + *
> + * Author: Ksenia Ragiadakou
Email address here please. Actually, it should have your copyright, not
just "Author:".
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
We don't want to track the office of the FSF for the next 40+ years, so
both of these two paragraphs can be removed (we all know what the GPL
is.)
> + */
> +
> +#define CREATE_TRACE_POINTS
> +#include "xhci-trace.h"
> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
> new file mode 100644
> index 000..a3e9ca3
> --- /dev/null
> +++ b/drivers/usb/host/xhci-trace.h
> @@ -0,0 +1,55 @@
> +/*
> + * xHCI host controller driver
> + *
> + * Author: Ksenia Ragiadakou
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
Same comments here as above.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree
On Friday, July 12, 2013 6:46 AM, Julius Werner wrote: > > Hi Jingoo, > > Yeah, I followed that discussion back then, but it seems to have > stalled a little (at least the HSIC patches haven't been picked up in > any kernel.org repo yet to my knowledge). > > The problem is that I think these approaches cannot work reliably. I > agree that it would be nice to control the HSIC device from its own > driver, and have spent quite some time playing around with the > usb/misc/usb3503.c driver to try to make this work... but there's a > timing dependency here that you just can't model correctly with > independent drivers. > > If the HSIC device is already active during boot (e.g. because it was > used by firmware), there's always a chance that the USB stack will > come up before the driver that resets it does. The device will be > enumerated as normal, and when the other driver later pulls the reset > signal the USB stack will not notice because there is no real > disconnect detection on HSIC. Only when you eventually try to send > another transfer to the device will you start to get timeouts, and the > newly reset device will not be able to reenumerate because the host > never asks it to. > > I really don't see how you could solve this without putting some kind > of synchronization mechanism in the USB drivers. So this leaves > ehci-s5p and phy-samsung-usb2 as the only possible places, and I chose > the latter since all the host-side HSIC initialization is also there > already. I think if you think of it as "reset whatever is on the other > side of this PHY", it's okay to put it as an optional feature into the > PHY driver. CC'ed Tomasz Figa, Dongjin Kim, Yulgon Kim Hi Tomasz, Dongjin, Julius Werner wants to put 'SMSC 3503 hub reset on Arndale board' to 'phy-samsung-usb*.c' files, because there is a timing dependency above mentioned. The following is the original patch sent by Julius Werner two day ago. (http://www.spinics.net/lists/linux-samsung-soc/msg20250.html) Previously, Olof mentioned that 'drivers/platform/arm/' would be used. (http://patches.linaro.org/16856/) Also, another way was mentioned by Fabio Estevam, using 'drivers/reset/gpio-reset.c' which is not merged yet. (http://permalink.gmane.org/gmane.linux.drivers.devicetree/36830) I think that 'phy-samsung-usb*.c' files are not a good place. However, Julius Werner's comment looks reasonable enough. If you have a comment, please feel free to share it. :) Thank you. Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] xhci: add traces for debug messages in xhci_address_device()
This patch declares an event class for trace events that
trace messages with variadic arguments, called xhci_log_msg,
and defines a trace event for tracing the debug messages in
xhci_address_device() function, called xhci_dbg_address.
In order to implement this type of trace events, a wrapper function,
called xhci_dbg_address(), was created that records the format string
and variadic arguments into a va_format structure which is passed as
argument to the tracepoints, called trace_xhci_dbg_address().
All the xhci_dbg() calls in xhci_address_device() are replaced
with calls to xhci_dbg_address(). The functionality of xhci_dbg()
log messages was not removed though, but it is placed inside
xhci_dbg_address().
This trace event aims to give the ability to the user or the
developper to isolate and trace the debug messages generated
when a Address Device Command is issued to xHC.
Also, the unnecessary cast (unsigned long long) is removed
from le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id])
to reduce line length below 80 characters.
Signed-off-by: Xenia Ragiadakou
---
Informationally:
By adding the tracepoints, the text segment size of xhci-hcd.o
was increased by 0.6%.
One way to enable the above trace event is:
$ echo 1 > tracing/events/xhci-hcd/xhci_dbg_address/enable
Similar output to this of the above trace can be achieved, if dynamic
debugging is enabled, by:
$ echo -n 'module xhci_hcd func xhci_debug_address +pf' > dynamic_debug/control
$ dmesg | grep "xhci_dbg_address"
drivers/usb/host/Makefile | 4
drivers/usb/host/xhci-dbg.c | 14 +++
drivers/usb/host/xhci-trace.c | 21 +
drivers/usb/host/xhci-trace.h | 55 +++
drivers/usb/host/xhci.c | 33 +-
drivers/usb/host/xhci.h | 1 +
6 files changed, 112 insertions(+), 16 deletions(-)
create mode 100644 drivers/usb/host/xhci-trace.c
create mode 100644 drivers/usb/host/xhci-trace.h
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index bea7112..ca0a119 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -4,6 +4,9 @@
ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
+# tell define_trace.h where to find the xhci trace header
+CFLAGS_xhci-trace.o := -I$(src)
+
isp1760-y := isp1760-hcd.o isp1760-if.o
fhci-y := fhci-hcd.o fhci-hub.o fhci-q.o
@@ -13,6 +16,7 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o
xhci-hcd-y := xhci.o xhci-mem.o
xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
+xhci-hcd-y += xhci-trace.o
xhci-hcd-$(CONFIG_PCI) += xhci-pci.o
ifneq ($(CONFIG_USB_XHCI_PLATFORM), )
diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5d5e58f..66c8358 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -21,6 +21,7 @@
*/
#include "xhci.h"
+#include "xhci-trace.h"
#define XHCI_INIT_VALUE 0x0
@@ -580,3 +581,16 @@ void xhci_dbg_ctx(struct xhci_hcd *xhci,
xhci_dbg_slot_ctx(xhci, ctx);
xhci_dbg_ep_ctx(xhci, ctx, last_ep);
}
+
+void xhci_dbg_address(struct xhci_hcd *xhci, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va = &args;
+ xhci_dbg(xhci, "%pV", &vaf);
+trace_xhci_dbg_address(&vaf);
+va_end(args);
+}
diff --git a/drivers/usb/host/xhci-trace.c b/drivers/usb/host/xhci-trace.c
new file mode 100644
index 000..6190df9
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.c
@@ -0,0 +1,21 @@
+/*
+ * xHCI host controller driver
+ *
+ * Author: Ksenia Ragiadakou
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#define CREATE_TRACE_POINTS
+#include "xhci-trace.h"
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
new file mode 100644
index 000..a3e9ca3
--- /dev/null
+++ b/drivers/usb/host/xhci-trace.h
@@ -0,0 +1,55 @@
+/*
+ * xHCI host controller driver
+ *
+ * Author: Ksenia Ragiadakou
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Pu
Re: [PATCH net-next] net/usb: add relative mii functions for r815x
From: Hayes Wang Date: Mon, 8 Jul 2013 10:41:21 +0800 > Base on cdc_ether, add the mii functions for RTL8152 and RTL8153. > The RTL8152 and RTL8153 support ECM mode which use the driver of > cdc_ether. Add the mii functions. Then, the basic PHY access is > possible. > > Signed-off-by: Hayes Wang This looks like a pretty straightforward addition of support for new chips, so applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree
Hi Jingoo, Yeah, I followed that discussion back then, but it seems to have stalled a little (at least the HSIC patches haven't been picked up in any kernel.org repo yet to my knowledge). The problem is that I think these approaches cannot work reliably. I agree that it would be nice to control the HSIC device from its own driver, and have spent quite some time playing around with the usb/misc/usb3503.c driver to try to make this work... but there's a timing dependency here that you just can't model correctly with independent drivers. If the HSIC device is already active during boot (e.g. because it was used by firmware), there's always a chance that the USB stack will come up before the driver that resets it does. The device will be enumerated as normal, and when the other driver later pulls the reset signal the USB stack will not notice because there is no real disconnect detection on HSIC. Only when you eventually try to send another transfer to the device will you start to get timeouts, and the newly reset device will not be able to reenumerate because the host never asks it to. I really don't see how you could solve this without putting some kind of synchronization mechanism in the USB drivers. So this leaves ehci-s5p and phy-samsung-usb2 as the only possible places, and I chose the latter since all the host-side HSIC initialization is also there already. I think if you think of it as "reset whatever is on the other side of this PHY", it's okay to put it as an optional feature into the PHY driver. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 5:02 PM, Alan Stern wrote: > You could try doing the opposite test: Leave the driver otherwise > intact, but have it not deliver the video buffers to the userspace > client. Interesting. Only one error in 72 frames of video. Suggests the problem is somewhere that is anywhere other than where I've been focusing all my attention. :-( Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Hardware bug in Intel USB-2 hub?
On Thu, 11 Jul 2013, Sarah Sharp wrote: > > I don't know. The real cause of the problem is that the USB-2 spec > > never took this race into account. (AFAIK neither did the USB-3 spec; > > didn't you face this problem at one point?) It contains detailed state > > diagrams and explanations for how a hub should work, and any hub that > > follows those instructions exactly will experience the problems I > > described. > > I was dealing with a VIA USB 3.0 hub that didn't advertise remote wakeup > at all, and thus probably wasn't actually designed to be suspended. I > had hacked around its lack of remote wakeup to make the USB core suspend > the USB 3.0 portion during runtime, and it kept sending wakeup requests > whenever it was suspended. Basically, I don't think this issue was > related to your issue. Does the USB-3 spec say what should happen when a remote wakeup signal on a downstream port in U3 races with a U0->U3 transition on the hub's upstream port? I tried to figure it out, but section 10.8 doesn't seem to consider that the upstream port might go into U3 while a downstream port is driving a remote wakeup signal. Figure 10-13 is ambiguous too. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, 11 Jul 2013, Devin Heitmueller wrote: > Funny - I had the same thought a few hours ago about moving the actual > URB processing to a tasklet. However I don't think it will help. > Here's why: > > Expanding on the test where I cut out everything in the URB handler > except for the submit, I tweaked the code so that it still does the > accounting so that video buffers get delivered but it skips the > memcpy() call (which is probably 99% of the work). This results in > the URB handler itself having essentially zero cost but empty buffers > get delivered to userland so the rest of the system behaves as it > normally would. In this case I was still seeing corruption and > skipped microframes via the Beagle, suggesting that the reason the > corruption went away when I commented out the body of the completion > handler wasn't because the cost of executing the completion handler > went to zero, but rather because the rest of the system load had gone > down as well because no video buffers were being delivered to the rest > of the system. You could try doing the opposite test: Leave the driver otherwise intact, but have it not deliver the video buffers to the userspace client. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: global suspend and remote wakeup don't mix
On Thu, 11 Jul 2013, Toralf Förster wrote: > On 07/11/2013 08:58 PM, Alan Stern wrote: > > This should be applied to the 3.10 stable kernel. > > > > Signed-off-by: Alan Stern > > Reported-and-tested-by: Toralf Förster > > CC: > > tested this patch against 3.10 - works fine too - thx. > > (this patch is a slightly modified variant of what you sent a week ago, > right ?) Yes, it is slightly modified. I decided it would be safest to include the 10-ms delay whenever a port was selectively suspended. That way an aborted system suspend would not cause any ports to be resumed too soon after being suspended. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Audio I/O parameters
James Stone wrote: > On Mon, Jul 8, 2013 at 2:12 PM, James Stone wrote: >>> ALSA: poll time out, polled for 2176094 usecs >>> JackAudioDriver::ProcessAsync: read error, stopping... >>> >>> This is a definite reduction in performance compared to earlier kernels. In theory, "poll time out" indicates that the stream does not appear to be running. But I doubt this is a driver problem. >> Some further info - on 3.5.0-28, I can start jackd in playback only >> with 8 frames/period, and capture only at 16 frames/period. > > Any thoughts on further investigating this bug with the 3.8.0 kernel > with the Focusrite Scarlett 2i4? I'll see if I can reproduce this with my own async-feedback device. Regards, Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help adding trace events to xHCI
On Thu, 2013-07-11 at 19:08 +0200, Johannes Berg wrote: > > > lets say that we want the tracepoint function to have the prototype: > > > > > > void trace_cmd_address_device(const char *fmt, ...). > > I'm not sure this is possible. I think we (wireless) do this with > > void trace_cmd_address_device(struct va_format *vaf) > > instead only. > > Note that there's no easy way to dynamically allocate the right amount > of space in the ringbuffer, or at least I haven't found one. We > therefore have a static size, which is somewhat inefficient. Can you add a helper function? If these trace events can't nest (one in interrupt context and another in normal context with interrupts enabled), then you can just allocate a per-cpu buffer and store the string in that, and then move the string into the buffer. vsnprintf(this_cpu_ptr(trace_buf), MAX_BUF_LEN, fmt, va); __assign_str(str, trace_buf); You could even use the reg, unreg, methods for TRACE_EVENT_FN() that will allocate the buffers when the tracepoint is created. This will mean that you don't waste memory when not tracing. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 3:02 PM, Johannes Stezenbach wrote: > I'm not familiar with musb but a quick glance at the code > shows it uses software packet scheduling. If you do too > much processing in the URB completion callback (which > is called from irq context), then the next bus transaction > is delayed by that time. See musb_advance_schedule() vs. > musb_giveback() and also the big comment at the start > of musb_core.c. I guess if you want to make em28xx work > with musb you need to move all processing to a tasklet > or similar. I think the issue you are seeing on musb has > nothing to do with the issue on Intel EHCI. Funny - I had the same thought a few hours ago about moving the actual URB processing to a tasklet. However I don't think it will help. Here's why: Expanding on the test where I cut out everything in the URB handler except for the submit, I tweaked the code so that it still does the accounting so that video buffers get delivered but it skips the memcpy() call (which is probably 99% of the work). This results in the URB handler itself having essentially zero cost but empty buffers get delivered to userland so the rest of the system behaves as it normally would. In this case I was still seeing corruption and skipped microframes via the Beagle, suggesting that the reason the corruption went away when I commented out the body of the completion handler wasn't because the cost of executing the completion handler went to zero, but rather because the rest of the system load had gone down as well because no video buffers were being delivered to the rest of the system. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: global suspend and remote wakeup don't mix
On 07/11/2013 08:58 PM, Alan Stern wrote: > This should be applied to the 3.10 stable kernel. > > Signed-off-by: Alan Stern > Reported-and-tested-by: Toralf Förster > CC: tested this patch against 3.10 - works fine too - thx. (this patch is a slightly modified variant of what you sent a week ago, right ?) -- MfG/Sincerely Toralf Förster pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Hardware bug in Intel USB-2 hub?
On Wed, Jul 03, 2013 at 10:07:13AM -0400, Alan Stern wrote: > On Tue, 2 Jul 2013, Sarah Sharp wrote: > > > > On the other hand, I know from personal experience that some companies > > > are worried about this race and insist on avoiding it. Should I add a > > > Kconfig option to force global suspend always? > > > > If we add a Kconfig option, I worry about a) confusing users and b) > > someone turning it on that has an Intel rate matching hub. I don't > > think the Kconfig option is a good one. How does a user know how to set > > the Kconfig option if they have both a rate matching hub and one of > > these hubs with a race condition? > > > > In the end, I suspect distros would leave the Kconfig option off, to > > avoid breaking Intel systems. How would anyone know to recompile their > > kernel and turn this option on? Can we detect these hubs with the race > > conditions and warn users to do so? Or dynamically turn on global > > suspend if we detect the hub and there's no Intel rate matching hub or > > Gensys hub attached? > > The Kconfig option would be intended for embedded systems only, and it > would say so in the help text. Distributions and normal users would be > expected to leave it disabled. Ok, I would be fine with a Kconfig option, if it was explicitly marked as being for embedded systems only. > There is no way to detect whether a hub suffers from the race condition > without special testing equipment. And I don't know whether any hubs > other than Intel and Genesys Logic have the wakeup bug, but it wouldn't > be surprising to find some that do. Makers of embedded systems are > presumably in a position to test their hardware and know exactly how it > performs. > > > It all starts to sound rather complicated. Perhaps people with hubs > > with the race condition will just have to live with it. How common are > > these hubs? > > I don't know. The real cause of the problem is that the USB-2 spec > never took this race into account. (AFAIK neither did the USB-3 spec; > didn't you face this problem at one point?) It contains detailed state > diagrams and explanations for how a hub should work, and any hub that > follows those instructions exactly will experience the problems I > described. I was dealing with a VIA USB 3.0 hub that didn't advertise remote wakeup at all, and thus probably wasn't actually designed to be suspended. I had hacked around its lack of remote wakeup to make the USB core suspend the USB 3.0 portion during runtime, and it kept sending wakeup requests whenever it was suspended. Basically, I don't think this issue was related to your issue. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 02:12:08PM -0400, Alan Stern wrote: > On Thu, 11 Jul 2013, Devin Heitmueller wrote: > > > Another strange thing: If I change the completion handler on the 8147 > > to comment out the actual processing of the URB (such that all the > > completion handler does is resubmit the URB), the problem disappears. > > The isoc stream as captured by the Beagle shows no video corruption, > > and no 250us gaps. If I were to speculate, I think this might be > > either because I've reduced the overall load on the system (not only > > is the URB handler not actually doing anything, but the userland code > > which process the video is no longer receiving frames), or > > alternatively the URB completion handler takes such a long time to > > execute that it actually prevents the hardware from sending out > > microframes (due to raw CPU load, the fact it's happening in interrupt > > context, or because of some shared lock). > > The musb driver makes much greater demands on the CPU than the EHCI > driver, relative to the different CPU speeds. Reducing the total load > on the ti8147 thus could significantly improve the performance. I'm not familiar with musb but a quick glance at the code shows it uses software packet scheduling. If you do too much processing in the URB completion callback (which is called from irq context), then the next bus transaction is delayed by that time. See musb_advance_schedule() vs. musb_giveback() and also the big comment at the start of musb_core.c. I guess if you want to make em28xx work with musb you need to move all processing to a tasklet or similar. I think the issue you are seeing on musb has nothing to do with the issue on Intel EHCI. Johannes -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help adding trace events to xHCI
On Thu, Jul 11, 2013 at 07:08:53PM +0200, Johannes Berg wrote:
> [also adding Steven, he's the tracing expert after all :-)]
>
> Hi Xenia, Sarah, all,
>
> > (Mentors and wireless folks, we're struggling a bit with adding trace
> > events to the xHCI USB host controller driver. I'm trying to look at
> > the ath6kl driver trace events as an example. We could use some help
> > and/or advice.)
>
> Those were in turn modelled on mac80211, cfg80211 and/or iwlwifi, I
> think; might be worth also looking there. In general though, it's all
> pretty similar.
>
> > > lets say that we want the tracepoint function to have the prototype:
> > >
> > > void trace_cmd_address_device(const char *fmt, ...).
>
> I'm not sure this is possible. I think we (wireless) do this with
>
> void trace_cmd_address_device(struct va_format *vaf)
>
> instead only.
>
> Note that there's no easy way to dynamically allocate the right amount
> of space in the ringbuffer, or at least I haven't found one. We
> therefore have a static size, which is somewhat inefficient.
Yes, that does look inefficient. Would it make sense to add a couple
different trace event classes for different sized buffers, and call
those trace classes conditionally, based on the size of the formatted
string? Or would that be too much of a mess.
Either way, it's only inefficient when trace events are turned on. We
don't expect xHCI debugging to be enabled by users very often. I do
expect that there will be a lot of debugging when it gets turned on.
Can userspace increase the size of the ring buffer? How much space do
we have by default?
> > The ath driver defines a new trace event class, ath6kl_log_event.
> > Various types of tracepoints, like trace_ath6kl_log_warn, use that event
> > class. Wrappers like ath6kl_warn() call those trace points, passing it
> > a struct va_format on the stack:
> >
> > int ath6kl_warn(const char *fmt, ...)
> > {
> > struct va_format vaf = {
> > .fmt = fmt,
> > };
> > va_list args;
> > int ret;
> >
> > va_start(args, fmt);
> > vaf.va = &args;
> > ret = ath6kl_printk(KERN_WARNING, "%pV", &vaf);
>
> Note also on older kernels you used to have to do va_copy() here because
> "%pV" didn't do it by itself. Guess you don't care though, but I
> sometimes have to worry about backporting :-)
The trace events are unlikely to get backported, so I'm not concerned
about that.
> > int xhci_debug_address(const char *fmt, ...)
>
> This confuses me somewhat -- why is it called "xhci_debug_address()"
> when it takes arbitrary parameters? Where's the "address" part?
It's debugging the xHCI Address command output. Depending on the status
of the command, we print different statement in the xHCI code. E.g. we
print when the command times out, or finishes with an error status
because there was a USB transfer error on the bus.
We can work on better names later. :)
> > DEFINE_EVENT(xhci_log_event, xhci_dbg_address,
> > TP_PROTO(struct va_format *vaf),
> > TP_ARGS(vaf)
> > );
> >
> > DECLARE_EVENT_CLASS(xhci_log_event,
> > TP_PROTO(struct va_format *vaf),
> > TP_ARGS(vaf),
> > TP_STRUCT__entry(
> > __dynamic_array(char, msg, ATH6KL_MSG_MAX)
>
> Should probably not be ATH6KL_MSG_MAX :-)
Yes.
> And this is what I talked about before -- it always allocates the max in
> the ringbuffer even for really short messages.
Noted.
> > And then code in xhci_address_device() in drivers/usb/host/xhci.c can do
> > things like:
> >
> > xhci_debug_address(xhci, "Bad Slot ID %d\n", udev->slot_id);
>
> Otherwise this looks about right (you have an xhci argument you didn't
> declare above, but this is obviously pseudo-code only)
>
> > And we can define similar trace event classes for the various xHCI
> > commands or ring debugging, so that we can enable or disable trace
> > points individually for different parts of the xHCI driver.
>
> I think it'd be worth (also) doing more specific tracepoints instead
> though.
>
> I don't really know what xhci does, but I suppose it has register
> read/write, maybe packet (urb?) submissions etc. so something like the
> iwlwifi_io system in drivers/net/wireless/iwlwifi/iwl-devtrace.h might
> (also) be (more) useful. In iwlwifi I have tracing for
> * IO accesses & interrupt notifications/reasons
> * commands and TX packets submitted to the device
> * notifications/RX packets received from the device
> * previously existing debug messages
My initial list of specific trace points was something like:
1. xHCI host initialization and shutdown
2. xHCI memory allocation (dynamic ring resizing, structure alloc, etc)
3. A few individual xHCI host controller command tracepoints:
* status only for all completed commands
* Address Device command status and output
* Configure Endpoint and Evaluate Context output
* individual trace points for other xHCI commands
4. Tracepoin
[PATCH] USB: global suspend and remote wakeup don't mix
The hub driver was recently changed to use "global" suspend for system
suspend transitions on non-SuperSpeed buses. This means that we don't
suspend devices individually by setting the suspend feature on the
upstream hub port; instead devices all go into suspend automatically
when the root hub stops transmitting packets. The idea was to save
time and to avoid certain kinds of wakeup races.
Now it turns out that many hubs are buggy; they don't relay wakeup
requests from a downstream port to their upstream port if the
downstream port's suspend feature is not set (depending on the speed
of the downstream port, whether or not the hub is enabled for remote
wakeup, and possibly other factors).
We can't have hubs dropping wakeup requests. Therefore this patch
goes partway back to the old policy: It sets the suspend feature for a
port if the device attached to that port or any of its descendants is
enabled for wakeup. People will still be able to benefit from the
time savings if they don't care about wakeup and leave it disabled on
all their devices.
In order to accomplish this, the patch adds a new field to the usb_hub
structure: wakeup_enabled_descendants is a count of how many devices
below a suspended hub are enabled for remote wakeup. A corresponding
new subroutine determines the number of wakeup-enabled devices at or
below an arbitrary suspended USB device.
This should be applied to the 3.10 stable kernel.
Signed-off-by: Alan Stern
Reported-and-tested-by: Toralf Förster
CC:
---
[as1692]
drivers/usb/core/hub.c | 39 +++
drivers/usb/core/hub.h |3 +++
2 files changed, 34 insertions(+), 8 deletions(-)
Index: usb-3.10/drivers/usb/core/hub.h
===
--- usb-3.10.orig/drivers/usb/core/hub.h
+++ usb-3.10/drivers/usb/core/hub.h
@@ -59,6 +59,9 @@ struct usb_hub {
struct usb_tt tt; /* Transaction Translator */
unsignedmA_per_port;/* current for each child */
+#ifdef CONFIG_PM
+ unsignedwakeup_enabled_descendants;
+#endif
unsignedlimited_power:1;
unsignedquiescing:1;
Index: usb-3.10/drivers/usb/core/hub.c
===
--- usb-3.10.orig/drivers/usb/core/hub.c
+++ usb-3.10/drivers/usb/core/hub.c
@@ -2848,6 +2848,15 @@ static int usb_disable_function_remotewa
USB_CTRL_SET_TIMEOUT);
}
+/* Count of wakeup-enabled devices at or below udev */
+static unsigned wakeup_enabled_descendants(struct usb_device *udev)
+{
+ struct usb_hub *hub = usb_hub_to_struct_hub(udev);
+
+ return udev->do_remote_wakeup +
+ (hub ? hub->wakeup_enabled_descendants : 0);
+}
+
/*
* usb_port_suspend - suspend a usb device's upstream port
* @udev: device that's no longer in active use, not a root hub
@@ -2888,8 +2897,8 @@ static int usb_disable_function_remotewa
* Linux (2.6) currently has NO mechanisms to initiate that: no khubd
* timer, no SRP, no requests through sysfs.
*
- * If Runtime PM isn't enabled or used, non-SuperSpeed devices really get
- * suspended only when their bus goes into global suspend (i.e., the root
+ * If Runtime PM isn't enabled or used, non-SuperSpeed devices may not get
+ * suspended until their bus goes into global suspend (i.e., the root
* hub is suspended). Nevertheless, we change @udev->state to
* USB_STATE_SUSPENDED as this is the device's "logical" state. The actual
* upstream port setting is stored in @udev->port_is_suspended.
@@ -2960,15 +2969,21 @@ int usb_port_suspend(struct usb_device *
/* see 7.1.7.6 */
if (hub_is_superspeed(hub->hdev))
status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
- else if (PMSG_IS_AUTO(msg))
- status = set_port_feature(hub->hdev, port1,
- USB_PORT_FEAT_SUSPEND);
+
/*
* For system suspend, we do not need to enable the suspend feature
* on individual USB-2 ports. The devices will automatically go
* into suspend a few ms after the root hub stops sending packets.
* The USB 2.0 spec calls this "global suspend".
+*
+* However, many USB hubs have a bug: They don't relay wakeup requests
+* from a downstream port if the port's suspend feature isn't on.
+* Therefore we will turn on the suspend feature if udev or any of its
+* descendants is enabled for remote wakeup.
*/
+ else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0)
+ status = set_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_SUSPEND);
else {
really_suspend = false;
status = 0;
@@ -3003,15 +3018,16 @@ int usb_port_suspend(struct usb_device *
[PATCH] USB: remove redundant "#if"
This patch removes a redundant nested "#ifdef CONFIG_PM" from the hub
driver. It also adds a label to the "#endif" line corresponding to
the outer "#ifdef CONFIG_PM".
Signed-off-by: Alan Stern
---
[as1691]
drivers/usb/core/hub.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
Index: usb-3.10/drivers/usb/core/hub.c
===
--- usb-3.10.orig/drivers/usb/core/hub.c
+++ usb-3.10/drivers/usb/core/hub.c
@@ -2829,7 +2829,6 @@ void usb_enable_ltm(struct usb_device *u
}
EXPORT_SYMBOL_GPL(usb_enable_ltm);
-#ifdef CONFIG_PM
/*
* usb_disable_function_remotewakeup - disable usb3.0
* device's function remote wakeup
@@ -3265,8 +3264,6 @@ int usb_port_resume(struct usb_device *u
return status;
}
-#endif /* CONFIG_PM */
-
#ifdef CONFIG_PM_RUNTIME
/* caller has locked udev */
@@ -3834,7 +3831,8 @@ EXPORT_SYMBOL_GPL(usb_disable_ltm);
void usb_enable_ltm(struct usb_device *udev) { }
EXPORT_SYMBOL_GPL(usb_enable_ltm);
-#endif
+
+#endif /* CONFIG_PM */
/* USB 2.0 spec, 7.1.7.3 / fig 7-29:
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, 11 Jul 2013, Devin Heitmueller wrote: > It looks like I may have created some confusion by attempting to > correlate the usbmon trace against the Beagle data. I've got two > environments I'm debugging on: an x86 box with the stock Intel EHCI > HCD, and a Davinci TI8147 ARM system which has the musb HCD. Because > I was seeing what visually looks like identical behavior on both > platforms, I had been actively going back and forth between them > during debugging. In this case, the Beagle trace was collected > against the Davinci board but the usbmon traces were collected against > the x86 system. Aha. Well, that's a relief -- now we don't have to worry about how the computer received data that the device didn't send. > Looking closer at the Beagle trace on the x86 system -- while there > aren't 250us gaps prior to the corruption, there *is* a delay between > the SOF and the IN packet on the order of 30-50us. This is present in > all instances where there is corruption, and in all other cases the IN > packet occurs within 1us of the SOF. In theory, delays of this sort are not supposed to matter. USB makes no guarantees about _when_ in the microframe an isochronous transfer will occur. It could be at the very start of one microframe and the very end of the next one. Devices are supposed to be able to cope with this sort of variation. > I suspect we've got the same basic issue on both platforms where there > is a delay between SOF and the IN packet, but the x86 system is so > much faster than the ti8147 platform that there's still time to grab > the microframe within the frame interval (albeit delayed), while on > the 8147 platform the HCD skips the microframe entirely (the HCD is > expected to skip a microframe if it concludes that it would not be > finished before the next SOF). The musb hardware is much crankier and more difficult to work with than EHCI. It's not at all surprising that small delays could get magnified. > Because of the difference in behavior between x86 and ti1847, I > figured it would be a worthwhile exercise to go back to the 8147 > platform and apply the ISO_ASAP fix that Alan mentioned yesterday, as > well as a fix for an incorrect spin_lock() call which I had tried on > the x86 box but hadn't yet applied to my 8147 tree (which out of dumb > luck, Ming Lei happened to submit a patch for this morning as well). > > http://www.mail-archive.com/linux-usb@vger.kernel.org/msg24178.html Using spin_lock() is not incorrect with the current kernel. But it will be wrong in the future. The ISO_ASAP thing won't make any difference in your setting. It matters only when the pipeline length is quite low (on the order of 1 ms), but yours is 40 ms. You may just as well leave it turned on all the time. > The ISO_ASAP change had no visible effect on the URB handling. > However the change from spin_lock() to spin_lock_irqsave() had a > unexpectedly *huge* effect. Now essentially every frame is corrupted > and it would appear that the system can no longer keep up with the > stream. It's not clear to me why this would cause any change in > behavior. Probably also worth noting that with that change, I was not > getting *multiple* microframes being skipped (e.g. 250, 500, 750us > gaps), at a much higher frequency than before. Very bizarre. > Another strange thing: If I change the completion handler on the 8147 > to comment out the actual processing of the URB (such that all the > completion handler does is resubmit the URB), the problem disappears. > The isoc stream as captured by the Beagle shows no video corruption, > and no 250us gaps. If I were to speculate, I think this might be > either because I've reduced the overall load on the system (not only > is the URB handler not actually doing anything, but the userland code > which process the video is no longer receiving frames), or > alternatively the URB completion handler takes such a long time to > execute that it actually prevents the hardware from sending out > microframes (due to raw CPU load, the fact it's happening in interrupt > context, or because of some shared lock). The musb driver makes much greater demands on the CPU than the EHCI driver, relative to the different CPU speeds. Reducing the total load on the ti8147 thus could significantly improve the performance. > No clear conclusions based on all of this, other than that I really > need to independently debug on the x86 versus the TI8147 platform, as > they are not in fact behaving in the same manner (despite both of them > showing essentially the same symptoms in terms of the appearance of > the video). The question becomes: Why would the packets be delayed with EHCI? One possible reason is that there is other activity on the USB bus. But it would have to be isochronous too, and in any case I imagine you don't have any other ongoing activity while running the tests. The only other explanation I can think of is memory/PCI bus contention.
Re: [PATCH v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
Hi Peter,
> This patchset adds tested otg id switch function and
> vbus connect and disconnect detection for chipidea driver.
> And fix kinds of bugs found at chipidea drivers after enabling
> id and vbus detection.
>
> This patch is fully tested at imx6 sabresd platform.
> My chipidea repo: https://github.com/hzpeterchen/linux-usb.git
>
> Changes for v12:
> - Rebased greg's usb-next tree (3.10.0-rc7+)
> - Split more small patches for single function and fix.
I tested the patchset. Here are the results:
- VBUS switching
I'm no longer getting any ID interrupts at all when I apply the patch below.
The
board stays in HOST mode all the time. If I configure it as peripheral, it
works
as peripheral. Note with [1], I was able to switch from Peripheral->Host , not
the other way around.
--- a/arch/arm/boot/dts/imx28-m28evk.dts
+++ b/arch/arm/boot/dts/imx28-m28evk.dts
@@ -240,6 +240,8 @@
ahb@8008 {
usb0: usb@8008 {
+ dr_mode = "otg";
+ phy_mode = "utmi";
vbus-supply = <®_usb0_vbus>;
pinctrl-names = "default";
pinctrl-0 = <&usbphy0_pins_a>;
---
- MX23 UDC issue
I found a workaround. Now running 'dmesg' via telnet through USB CDC link no
longer hangs the USB driver, but works as expected. I applied this small patch
that enables the streaming mode. Works on MX23EVK. It's surprising this issue
doesn't manifest on MX28, maybe MX28 contains a new revision of the controller.
I remember there was some discussion about the streaming mode on MXS some time
ago.
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index d06355e..bba8e25 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -92,8 +92,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
.name = "ci_hdrc_imx",
.capoffset = DEF_CAPOFFSET,
.flags = CI_HDRC_REQUIRE_TRANSCEIVER |
- CI_HDRC_PULLUP_ON_VBUS |
- CI_HDRC_DISABLE_STREAMING,
+ CI_HDRC_PULLUP_ON_VBUS,
};
struct resource *res;
int ret;
[1]
http://git.pengutronix.de/?p=mgr/linux.git;a=shortlog;h=refs/heads/v3.10/topic/usb-
peterchen
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 11:28 AM, Alan Stern wrote: > On Thu, 11 Jul 2013, Johannes Stezenbach wrote: > >> I took a peek at the usbmon log, and there is one thing I don't get. >> >> $ grep "C Zi:1:005:2" em28xx_usbmon.log | less -S >> >> There are several cases where the isoc descriptor actual length is >> short (< 2892, e.g. 0, 552 or 1928), yet the actual_length for the >> whole URB at the end of the line is still 64*2892 -- is that like it should >> be? > > Yes. When one packet is short, the following packets don't get moved > up in the buffer -- their offsets are fixed in advance. The unused > space at the end of the short packet remains, and it contributes to the > total actual_length value for the entire URB. > >> Also, is it expected that the em28xx doesn't have data to send for >> some microframes (for analog video capture e.g. during horizontal >> or vertical blank interval)? I guess in the analyzer that should >> show as an IN token with no reply instead of DATA[012] replies. > > Not necessarily. It might send random data, or a short (maybe even > 0-length) reply. > > In fact, the usbmon trace does show some short packets (as you > mentioned above) and even some 0-length packets. For example, see > timestamp 3976305153. > >> Can you confirm that in the error case there is no IN token at all? > > That's what the analyzer log showed. Of course, it's possible that the > analyzer itself was faulty, but I don't think so because Devin said the > missing packets matched exactly with the corrupted data in the > following microframes. > >> And another dumb question: Is there another USB video grabber >> with different hardware which also uses isoc and works? >> E.g. the cx231xx? Or do you think it might have the same missing >> microframe issue but copes better with it due to larger >> buffer space on the device? > > The uvcvideo driver works. But as you point out, the circumstances may > be significantly different. > > Alan Stern > Sorry for the delayed response in contributing to this thread. I've been testing and gathering data all morning. So a few new things: It looks like I may have created some confusion by attempting to correlate the usbmon trace against the Beagle data. I've got two environments I'm debugging on: an x86 box with the stock Intel EHCI HCD, and a Davinci TI8147 ARM system which has the musb HCD. Because I was seeing what visually looks like identical behavior on both platforms, I had been actively going back and forth between them during debugging. In this case, the Beagle trace was collected against the Davinci board but the usbmon traces were collected against the x86 system. I finally ran the Beagle against the x86 system, and found that in fact I'm not getting the intermittent 250us gaps between microframes when corruption occurs (which BTW is entirely consistent with the usbmon trace). Hence we've been trying to figure out why there was a missing microframe in the Beagle trace but not in the usbmon trace, when in fact that was a red-herring and there's no lost microframe to begin with. Looking closer at the Beagle trace on the x86 system -- while there aren't 250us gaps prior to the corruption, there *is* a delay between the SOF and the IN packet on the order of 30-50us. This is present in all instances where there is corruption, and in all other cases the IN packet occurs within 1us of the SOF. I suspect we've got the same basic issue on both platforms where there is a delay between SOF and the IN packet, but the x86 system is so much faster than the ti8147 platform that there's still time to grab the microframe within the frame interval (albeit delayed), while on the 8147 platform the HCD skips the microframe entirely (the HCD is expected to skip a microframe if it concludes that it would not be finished before the next SOF). Because of the difference in behavior between x86 and ti1847, I figured it would be a worthwhile exercise to go back to the 8147 platform and apply the ISO_ASAP fix that Alan mentioned yesterday, as well as a fix for an incorrect spin_lock() call which I had tried on the x86 box but hadn't yet applied to my 8147 tree (which out of dumb luck, Ming Lei happened to submit a patch for this morning as well). http://www.mail-archive.com/linux-usb@vger.kernel.org/msg24178.html The ISO_ASAP change had no visible effect on the URB handling. However the change from spin_lock() to spin_lock_irqsave() had a unexpectedly *huge* effect. Now essentially every frame is corrupted and it would appear that the system can no longer keep up with the stream. It's not clear to me why this would cause any change in behavior. Probably also worth noting that with that change, I was not getting *multiple* microframes being skipped (e.g. 250, 500, 750us gaps), at a much higher frequency than before. Another strange thing: If I change the completion handler on the 8147 to comment out the actual processing of the URB (such that all the completion han
Re: [RFC] ux500 dma & short transfers on MUSB
On 07/11/2013 06:58 PM, Greg KH wrote: >> following scenario: >> you attach an UART-TO-USB adapter to your musb port running ux500-dma >> code. The USB UARt driver queues 1x RX URB with the size of 256 bytes >> (example) and the max packet size is 64. >> >> The other side sends only one byte because it is mean. > > That's actually quite common, why is it "mean"? Hehe. Because it is the unexpected one. common depends on the use case. Mass storage doesn't do this. Not sure if network notices the difference, maybe ncm. >> Now, the way I understand it is, you tell musb that the complete >> transfer of 256 bytes has ended instead one byte that really >> happened. Is my assumption wrong? > > What do you mean by "tell musb"? Of course the transfer has completed, > that's all the device sent to the host controller, so it has to complete > the transfer and send that on up to the driver that requested the urb. > > I don't understand the question/problem you are asking here, care to be > more descriptive? Okay. musb offloads the actual transfer to the DMA engine it is using. Once it does so, it relies on whatever comes back from dma engine regarding transfer complete, transferred size etc. In case of ux500-dma (as far as I can tell) musb forwards the RX request to the DMA engine, which will receive one byte instead of the requested 256bytes. Since the DMA engine did not inform musb about the correct transfer size, musb will complete that URB with 256 bytes. If you take a look on ux500_dma_callback() you will see the line: ux500_channel->channel.actual_len = ux500_channel->cur_len; ->actual_len is what musb thinks got transferred. ->cur_len is what musb asked to transfer. I don't see where the case of a shorter transfer is considered. Again I have no HW I was just browsing. > thanks, > > greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 4:24 AM, Johannes Stezenbach wrote: > There are several cases where the isoc descriptor actual length is > short (< 2892, e.g. 0, 552 or 1928), yet the actual_length for the > whole URB at the end of the line is still 64*2892 -- is that like it should > be? Yup, as Alan stated, this is expected behavior. > Also, is it expected that the em28xx doesn't have data to send for > some microframes (for analog video capture e.g. during horizontal > or vertical blank interval)? I guess in the analyzer that should > show as an IN token with no reply instead of DATA[012] replies. > Can you confirm that in the error case there is no IN token at all? > > And another dumb question: Is there another USB video grabber > with different hardware which also uses isoc and works? > E.g. the cx231xx? Or do you think it might have the same missing > microframe issue but copes better with it due to larger > buffer space on the device? I've been actively testing also with an au0828 device (HVR-950q), which has a virtually identical URB completion handler (not shocking, since I copied it from the em28xx driver when I wrote it in 2009). The HVR-950q doesn't exhibit any of the problems described in this thread though (presumably because it has a larger on-chip FIFO). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help adding trace events to xHCI
[also adding Steven, he's the tracing expert after all :-)]
Hi Xenia, Sarah, all,
> (Mentors and wireless folks, we're struggling a bit with adding trace
> events to the xHCI USB host controller driver. I'm trying to look at
> the ath6kl driver trace events as an example. We could use some help
> and/or advice.)
Those were in turn modelled on mac80211, cfg80211 and/or iwlwifi, I
think; might be worth also looking there. In general though, it's all
pretty similar.
> > lets say that we want the tracepoint function to have the prototype:
> >
> > void trace_cmd_address_device(const char *fmt, ...).
I'm not sure this is possible. I think we (wireless) do this with
void trace_cmd_address_device(struct va_format *vaf)
instead only.
Note that there's no easy way to dynamically allocate the right amount
of space in the ringbuffer, or at least I haven't found one. We
therefore have a static size, which is somewhat inefficient.
> The ath driver defines a new trace event class, ath6kl_log_event.
> Various types of tracepoints, like trace_ath6kl_log_warn, use that event
> class. Wrappers like ath6kl_warn() call those trace points, passing it
> a struct va_format on the stack:
>
> int ath6kl_warn(const char *fmt, ...)
> {
> struct va_format vaf = {
> .fmt = fmt,
> };
> va_list args;
> int ret;
>
> va_start(args, fmt);
> vaf.va = &args;
> ret = ath6kl_printk(KERN_WARNING, "%pV", &vaf);
Note also on older kernels you used to have to do va_copy() here because
"%pV" didn't do it by itself. Guess you don't care though, but I
sometimes have to worry about backporting :-)
> int xhci_debug_address(const char *fmt, ...)
This confuses me somewhat -- why is it called "xhci_debug_address()"
when it takes arbitrary parameters? Where's the "address" part?
> DEFINE_EVENT(xhci_log_event, xhci_dbg_address,
> TP_PROTO(struct va_format *vaf),
> TP_ARGS(vaf)
> );
>
> DECLARE_EVENT_CLASS(xhci_log_event,
> TP_PROTO(struct va_format *vaf),
> TP_ARGS(vaf),
> TP_STRUCT__entry(
> __dynamic_array(char, msg, ATH6KL_MSG_MAX)
Should probably not be ATH6KL_MSG_MAX :-)
And this is what I talked about before -- it always allocates the max in
the ringbuffer even for really short messages.
> And then code in xhci_address_device() in drivers/usb/host/xhci.c can do
> things like:
>
> xhci_debug_address(xhci, "Bad Slot ID %d\n", udev->slot_id);
Otherwise this looks about right (you have an xhci argument you didn't
declare above, but this is obviously pseudo-code only)
> And we can define similar trace event classes for the various xHCI
> commands or ring debugging, so that we can enable or disable trace
> points individually for different parts of the xHCI driver.
I think it'd be worth (also) doing more specific tracepoints instead
though.
I don't really know what xhci does, but I suppose it has register
read/write, maybe packet (urb?) submissions etc. so something like the
iwlwifi_io system in drivers/net/wireless/iwlwifi/iwl-devtrace.h might
(also) be (more) useful. In iwlwifi I have tracing for
* IO accesses & interrupt notifications/reasons
* commands and TX packets submitted to the device
* notifications/RX packets received from the device
* previously existing debug messages
The message tracing was really more of an after-thought in iwlwifi (and
ath6kl as well I guess) because we already had a lot of debug messages
and capturing it all together can be useful.
However, tracing all the debug messages is actually fairly expensive, I
think in part because of the string formatting and in part because of
the always-max allocations in the ringbuffer.
> > That would have worked if the tracepoint was just :
> >
> > void trace_cmd_address_device(const char *fmt, ...)
> > {
> > if (trace event cmd_address_device is enabled) do {
> > (void(*)(const char *fmt, ...)) callback ((const
> > char *)&fmt);
> > }
> > }
I'm not really sure what the whole "callback()" part is about?
Are you trying to use the "tracepoint is enabled" to do something
unrelated to the tracing? I'm guessing that's _possible_, but I wouldn't
recommend it.
> I'm actually wondering if the call to ath6kl_printk is somehow necessary
> in order to be able to pass arguments on the stack.
I don't think it is, but formatting the messages *only* for tracing
seems a bit odd?
Hth,
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help adding trace events to xHCI
On 07/11/2013 06:20 PM, Sarah Sharp wrote:
On Mon, Jul 08, 2013 at 09:17:59PM +0300, Xenia Ragiadakou wrote:
But when there are other function calls before the callback call, I don't
no why but i cannot track anymore the position of the args following the
fmt argumenent in the stack with the pointer to fmt.
I'm actually wondering if the call to ath6kl_printk is somehow necessary
in order to be able to pass arguments on the stack. Perhaps you should
try defining a similar function for xHCI and see if that helps?
The ath6klk_printk() is not related to the trace function. It is a
separate code path to get the message in the kernel log. I have seen
these constructs before (and implemented it in brcmfmac) but it seems
not very efficient when tracing as the printk can affect run-time behaviour.
int ath6kl_printk(const char *level, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
int rtn;
va_start(args, fmt);
vaf.fmt = fmt;
vaf.va = &args;
rtn = printk("%sath6kl: %pV", level, &vaf);
va_end(args);
return rtn;
}
EXPORT_SYMBOL(ath6kl_printk);
Anyway, something dirty like that will not enter the kernel but i will try
to do stack debugging on an example program to see why that happens.
I would suggest just copy-pasting parts of the ath6kl trace code into
the xHCI driver, and changing one of the xhci_dbg() calls to use that
code, and see if it works. If it doesn't work, send out an RFC patch
(using the Cc list I've used), and we'll try to figure out what's going
wrong.
The biggest challenge in adding tracepoints is identifying what you want
to trace. While tracing debug messages can be convenient the real
strength is in tracing code artifacts like for USB the thing that comes
to my mind first is defining tracepoint for the urb and there are
probably other internals that are informational.
Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage with hcd_urb_list_lock
On Thu, 11 Jul 2013, Sarah Sharp wrote:
> On Sat, Jul 06, 2013 at 04:48:47PM -0400, Alan Stern wrote:
> > On Sat, 6 Jul 2013, Maarten Lankhorst wrote:
> >
> > > I didn't even know I still had lockdep on.
> > > The following lockdep splat happened when I plugged in a usb bluetooth
> > > dongle, using
> > > the pre-rc1 3.11 kernel at HEAD b2c311075db
> > >
> > > =
> > > [ INFO: inconsistent lock state ]
> > > 3.10.0+ #106 Not tainted
> > > -
> > > inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> > > irq/42-xhci_hcd/97 [HC0[0]:SC0[2]:HE1:SE0] takes:
> > > (hcd_urb_list_lock){?.}, at: []
> > > usb_hcd_unlink_urb_from_ep+0x28/0x4e
> >
> > > stack backtrace:
> > > CPU: 1 PID: 97 Comm: irq/42-xhci_hcd Not tainted 3.10.0+ #106
> > > Hardware name: Acer Aspire M3985/Aspire M3985, BIOS P01-A1 03/12/2012
> > > 8210c150 88040834da48 81691af4 0007
> > > 8804082e20b0 88040834daa8 8168cb10 0002
> > > 88040001 8804 8100f4f7 88040834dac4
> > > Call Trace:
> > > [] dump_stack+0x4f/0x84
> > > [] print_usage_bug+0x1f5/0x206
> > > [] ? save_stack_trace+0x2f/0x50
> > > [] mark_lock+0x276/0x2cf
> > > [] ? check_usage_forwards+0x12f/0x12f
> > > [] __lock_acquire+0x5c0/0x1c2e
> > > [] ? mark_held_locks+0x6d/0x117
> > > [] ? __slab_free+0x1c7/0x2ed
> > > [] ? trace_hardirqs_on_caller+0xac/0x1bb
> > > [] ? trace_hardirqs_on+0xd/0xf
> > > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > > [] lock_acquire+0x87/0x139
> > > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > > [] _raw_spin_lock+0x3b/0x4a
> > > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > > [] usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > > [] xhci_irq+0x5ac/0x143d
> > > [] ? _raw_spin_unlock_irq+0x3b/0x5d
> > > [] ? finish_task_switch+0x7c/0x101
> > > [] ? finish_task_switch+0x3f/0x101
> > > [] ? __schedule+0x42a/0x885
> > > [] ? irq_thread_fn+0x48/0x48
> > > [] xhci_msi_irq+0x11/0x15
> >
> > It looks like xhci_msi_irq() needs to call local_irq_save() and
> > local_irq_restore().
>
> Alan, does this only need to be called when MSI is enabled?
> xhci_msi_irq() basically calls xhci_irq, which is called if MSI is
> disabled and legacy PCI interrupts are used.
>
> I'm trying to figure out whether to put the calls to local_irq_save()
> and local_irq_restore() in xhci_msi_irq() or xhci_irq().
Ming Lei pointed out that my comment was wrong. The reason for the
lockdep violation was because Maarten had set up xhci-hcd to use
threaded interrupts. You can see this in the current process name in
the lockdep warning: "irq/42-xhci_hcd" is the name of an
interrupt-handler thread.
Without threaded interrupts, it seems likely that no changes are
needed. Certainly no changes are needed in xhci_irq(), because usbcore
disables local interrupts explicitly before invoking legacy interrupt
handlers (see usb_hcd_irq() in hcd.c). In the absense of threaded
interrupts, I assume xhci_msi_irq() will be invoked with interrupts
disabled -- if not then it will need to disable them before calling
xhci_irq().
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] ux500 dma & short transfers on MUSB
On Thu, Jul 11, 2013 at 11:08:59AM +0200, Sebastian Andrzej Siewior wrote: > Hello, > > following scenario: > you attach an UART-TO-USB adapter to your musb port running ux500-dma > code. The USB UARt driver queues 1x RX URB with the size of 256 bytes > (example) and the max packet size is 64. > > The other side sends only one byte because it is mean. That's actually quite common, why is it "mean"? > Now, the way I understand it is, you tell musb that the complete > transfer of 256 bytes has ended instead one byte that really > happened. Is my assumption wrong? What do you mean by "tell musb"? Of course the transfer has completed, that's all the device sent to the host controller, so it has to complete the transfer and send that on up to the driver that requested the urb. I don't understand the question/problem you are asking here, care to be more descriptive? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage with hcd_urb_list_lock
On Sat, Jul 06, 2013 at 04:48:47PM -0400, Alan Stern wrote:
> On Sat, 6 Jul 2013, Maarten Lankhorst wrote:
>
> > I didn't even know I still had lockdep on.
> > The following lockdep splat happened when I plugged in a usb bluetooth
> > dongle, using
> > the pre-rc1 3.11 kernel at HEAD b2c311075db
> >
> > =
> > [ INFO: inconsistent lock state ]
> > 3.10.0+ #106 Not tainted
> > -
> > inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> > irq/42-xhci_hcd/97 [HC0[0]:SC0[2]:HE1:SE0] takes:
> > (hcd_urb_list_lock){?.}, at: []
> > usb_hcd_unlink_urb_from_ep+0x28/0x4e
>
> > stack backtrace:
> > CPU: 1 PID: 97 Comm: irq/42-xhci_hcd Not tainted 3.10.0+ #106
> > Hardware name: Acer Aspire M3985/Aspire M3985, BIOS P01-A1 03/12/2012
> > 8210c150 88040834da48 81691af4 0007
> > 8804082e20b0 88040834daa8 8168cb10 0002
> > 88040001 8804 8100f4f7 88040834dac4
> > Call Trace:
> > [] dump_stack+0x4f/0x84
> > [] print_usage_bug+0x1f5/0x206
> > [] ? save_stack_trace+0x2f/0x50
> > [] mark_lock+0x276/0x2cf
> > [] ? check_usage_forwards+0x12f/0x12f
> > [] __lock_acquire+0x5c0/0x1c2e
> > [] ? mark_held_locks+0x6d/0x117
> > [] ? __slab_free+0x1c7/0x2ed
> > [] ? trace_hardirqs_on_caller+0xac/0x1bb
> > [] ? trace_hardirqs_on+0xd/0xf
> > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > [] lock_acquire+0x87/0x139
> > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > [] _raw_spin_lock+0x3b/0x4a
> > [] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > [] usb_hcd_unlink_urb_from_ep+0x28/0x4e
> > [] xhci_irq+0x5ac/0x143d
> > [] ? _raw_spin_unlock_irq+0x3b/0x5d
> > [] ? finish_task_switch+0x7c/0x101
> > [] ? finish_task_switch+0x3f/0x101
> > [] ? __schedule+0x42a/0x885
> > [] ? irq_thread_fn+0x48/0x48
> > [] xhci_msi_irq+0x11/0x15
>
> It looks like xhci_msi_irq() needs to call local_irq_save() and
> local_irq_restore().
Alan, does this only need to be called when MSI is enabled?
xhci_msi_irq() basically calls xhci_irq, which is called if MSI is
disabled and legacy PCI interrupts are used.
I'm trying to figure out whether to put the calls to local_irq_save()
and local_irq_restore() in xhci_msi_irq() or xhci_irq().
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: dwc2: fix value used in dwc2_set_all_params
On Thu, Jul 11, 2013 at 02:24:10PM +0200, Julien Delacou wrote: > From: Julien Delacou > > This fix uses 'value' parameter as it should be instead > of hardcoded -1. Woops! > Signed-off-by: Julien Delacou > Acked-by: Paul Zimmerman Reviewed-by: Matthijs Kooijman > --- > drivers/staging/dwc2/hcd.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c > index 2ed54b1..ca38aaa 100644 > --- a/drivers/staging/dwc2/hcd.c > +++ b/drivers/staging/dwc2/hcd.c > @@ -2690,7 +2690,7 @@ void dwc2_set_all_params(struct dwc2_core_params > *params, int value) > int i; > > for (i = 0; i < size; i++) > - p[i] = -1; > + p[i] = value; > } > EXPORT_SYMBOL_GPL(dwc2_set_all_params); > > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Help adding trace events to xHCI
On Mon, Jul 08, 2013 at 09:17:59PM +0300, Xenia Ragiadakou wrote:
> Hi Sarah,
Hi Xenia,
Comments below.
(Mentors and wireless folks, we're struggling a bit with adding trace
events to the xHCI USB host controller driver. I'm trying to look at
the ath6kl driver trace events as an example. We could use some help
and/or advice.)
> lets say that we want the tracepoint function to have the prototype:
>
> void trace_cmd_address_device(const char *fmt, ...).
>
> That internally will be implemented as:
>
> void trace_cmd_address_device(const char *fmt, ...)
> {
> if (trace event cmd_address_device is enabled) do {
> [...some other code will run here]
> (void(*)(const char *fmt, ...)) callback (fmt, '
> ' <-- we need to pass args here);
> }
> }
>
> callback will be the function that we actually intend to run and
> which we define when we define the trace_event. This function
> we want to create a string from fmt and the args and copy it
> into the ring buffer.
>
> The only way to pass the args into this function is by doing
> the following:
>
> Note: remember that the callback and tracepoint functions must
> have the same prototype.
>
> void trace_cmd_address_device(void *ptr, const char *fmt, ...)
> {
> if (trace event cmd_address_device is enabled) do {
> [...some other code will run here]
> va_list args;
> va_start(args, fmt);
> (void(*)(void *ptr, const char *fmt, ...))
> callback (args, fmt);
> va_end(args);
> }
> }
Right, that looks pretty similar to what's defined in
drivers/net/wireless/ath/ath6kl/trace.h. (Note that I'm working on the
very latest Linux kernel, so you may need to `git fetch` Greg KH's USB
tree, and rebase against the usb-next branch.)
> But, that cannot be done because the current tracing framework does not
> give a means to add code outside the callback.
I'm confused about why you need to add more code. Here's how I
understand the code works, so please correct me if I misinterpreted the
code.
The ath driver defines a new trace event class, ath6kl_log_event.
Various types of tracepoints, like trace_ath6kl_log_warn, use that event
class. Wrappers like ath6kl_warn() call those trace points, passing it
a struct va_format on the stack:
int ath6kl_warn(const char *fmt, ...)
{
struct va_format vaf = {
.fmt = fmt,
};
va_list args;
int ret;
va_start(args, fmt);
vaf.va = &args;
ret = ath6kl_printk(KERN_WARNING, "%pV", &vaf);
trace_ath6kl_log_warn(&vaf);
va_end(args);
return ret;
}
EXPORT_SYMBOL(ath6kl_warn);
trace_ath6kl_log_warn() uses the log event class:
DEFINE_EVENT(ath6kl_log_event, ath6kl_log_warn,
TP_PROTO(struct va_format *vaf),
TP_ARGS(vaf)
);
DECLARE_EVENT_CLASS(ath6kl_log_event,
TP_PROTO(struct va_format *vaf),
TP_ARGS(vaf),
TP_STRUCT__entry(
__dynamic_array(char, msg, ATH6KL_MSG_MAX)
),
TP_fast_assign(
WARN_ON_ONCE(vsnprintf(__get_dynamic_array(msg),
ATH6KL_MSG_MAX,
vaf->fmt,
*vaf->va) >= ATH6KL_MSG_MAX);
),
TP_printk("%s", __get_str(msg))
);
And then the code in files like drivers/net/wireless/ath6kl/cfg80211.c
can simply make what look like printk calls to ath6kl_warn:
ath6kl_warn("clear wmi ctrl data failed: %d\n", left);
I think that we can apply a similar technique to define trace events for
the xhci debugging that's printed when we're issuing a Set Address
command. Something like:
int xhci_debug_address(const char *fmt, ...)
{
struct va_format vaf = {
.fmt = fmt,
};
va_list args;
int ret;
va_start(args, fmt);
vaf.va = &args;
ret = xhci_printk(KERN_WARNING, "%pV", &vaf);
trace_xhci_dbg_address(&vaf);
va_end(args);
return ret;
}
EXPORT_SYMBOL(xhci_debug_address);
DEFINE_EVENT(xhci_log_event, xhci_dbg_address,
TP_PROTO(struct va_format *vaf),
TP_ARGS(vaf)
);
DECLARE_EVENT_CLASS(xhci_log_event,
TP_PROTO(struct va_format *vaf),
TP_ARGS(vaf),
TP_STRUCT__entry(
__dynamic_array(char, msg, ATH6KL_MSG_MAX)
),
TP_fast_assign(
WARN_ON_ONCE(vsnprintf(__get_dynamic_array(msg),
ATH6KL_MSG_MAX,
vaf->fmt,
*vaf->va) >= ATH6KL_MSG_MAX);
),
TP_printk("%s", __get_str(msg))
);
And then code in xhci_address_device() in drivers/usb/host/xhci.c can do
things like:
xhci_debug_address(xhci, "Bad Slot ID %d\n", udev->slot_id);
And we can de
Re: Video corruption varies by system load
On Thu, Jul 11, 2013 at 11:28:38AM -0400, Alan Stern wrote: > On Thu, 11 Jul 2013, Johannes Stezenbach wrote: > > > Can you confirm that in the error case there is no IN token at all? > > That's what the analyzer log showed. Of course, it's possible that the > analyzer itself was faulty, but I don't think so because Devin said the > missing packets matched exactly with the corrupted data in the > following microframes. I think there is the possibility the analyzer view is configured to not show details of unanswered IN tokens. But I don't know TotalPhase, I only used LeCroy at a customer's site and it has lots of filters and options for the trace view. Since it makes a big difference for the bug hunt I'm asking for double confirmation. Johannes -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] [RFC] usb: gadget: USB_GADGET should depend on HAS_DMA
On Thu, 11 Jul 2013, Geert Uytterhoeven wrote:
> If NO_DMA=y:
>
> drivers/built-in.o: In function `dma_set_coherent_mask':
> include/linux/dma-mapping.h:93: undefined reference to `dma_supported'
> include/linux/dma-mapping.h:93: undefined reference to `dma_supported'
> drivers/built-in.o: In function `usb_gadget_unmap_request':
> drivers/usb/gadget/udc-core.c:91: undefined reference to `dma_unmap_sg'
> drivers/usb/gadget/udc-core.c:96: undefined reference to `dma_unmap_single'
> drivers/built-in.o: In function `usb_gadget_map_request':
> drivers/usb/gadget/udc-core.c:62: undefined reference to `dma_map_sg'
> drivers/usb/gadget/udc-core.c:71: undefined reference to `dma_map_single'
> drivers/usb/gadget/udc-core.c:74: undefined reference to `dma_mapping_error'
>
> Signed-off-by: Geert Uytterhoeven
> ---
> This one is very debatable: probably the parts using the DMA API should
> be factored out, instead of disabling the whole USB gadget subsystem.
Indeed. How does this look instead?
Alan Stern
Index: usb-3.10/drivers/usb/gadget/udc-core.c
===
--- usb-3.10.orig/drivers/usb/gadget/udc-core.c
+++ usb-3.10/drivers/usb/gadget/udc-core.c
@@ -50,6 +50,8 @@ static DEFINE_MUTEX(udc_lock);
/* - */
+#ifdef CONFIG_HAS_DMA
+
int usb_gadget_map_request(struct usb_gadget *gadget,
struct usb_request *req, int is_in)
{
@@ -99,6 +101,8 @@ void usb_gadget_unmap_request(struct usb
}
EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
+#endif /* CONFIG_HAS_DMA */
+
/* - */
void usb_gadget_set_state(struct usb_gadget *gadget,
@@ -194,9 +198,11 @@ int usb_add_gadget_udc_release(struct de
dev_set_name(&gadget->dev, "gadget");
gadget->dev.parent = parent;
+#ifdef CONFIG_HAS_DMA
dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
gadget->dev.dma_parms = parent->dma_parms;
gadget->dev.dma_mask = parent->dma_mask;
+#endif
if (release)
gadget->dev.release = release;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/50] USB: usblp: spin_lock in complete() cleanup
On Thu, 11 Jul 2013 17:05:26 +0800 Ming Lei wrote: > Complete() will be run with interrupt enabled, so change to > spin_lock_irqsave(). > > Cc: Pete Zaitcev > Signed-off-by: Ming Lei Signed-off-by: Pete Zaitcev Good luck with that, Ming. I think the spin_lock_irqsave thing should've been done back in days of uhci-hcd. But we always had some super annoying edge cases when we considered it previously. I see Alan Stern signed off the HCD changes at the beginning of July, so you're on the right track. BTW, perfect formatting in usblp.c, too bad for Sergei's heartburn ^_^ -- Pete P.S. Is it just me, or you forgot to fix usb-skeleton.c in your 50 patches? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 06/13] usb: chipidea: add otg_cap attribute for otg capable
Dear Peter Chen,
> Since we need otgsc to know vbus's status at some chipidea
> controllers even it is peripheral-only mode. Besides, some
> SoCs (eg, AR9331 SoC) don't have otgsc register even
> the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS.
> We inroduce otg_cap attribute to indicate if the controller
> is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC
> and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there
> is exception, the platform can override it by device tree or platform data.
>
> Signed-off-by: Peter Chen
> ---
> drivers/usb/chipidea/core.c | 35 ---
> include/linux/usb/chipidea.h | 13 +
> 2 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 93961ff..e8ceb04 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc *ci)
> ci_hdrc_host_destroy(ci);
> }
>
> +static void ci_get_otg_capable(struct ci_hdrc *ci)
> +{
> + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED)
IS_NONEXISTENT , no?
[..]
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Video corruption varies by system load
On Thu, 11 Jul 2013, Johannes Stezenbach wrote: > I took a peek at the usbmon log, and there is one thing I don't get. > > $ grep "C Zi:1:005:2" em28xx_usbmon.log | less -S > > There are several cases where the isoc descriptor actual length is > short (< 2892, e.g. 0, 552 or 1928), yet the actual_length for the > whole URB at the end of the line is still 64*2892 -- is that like it should > be? Yes. When one packet is short, the following packets don't get moved up in the buffer -- their offsets are fixed in advance. The unused space at the end of the short packet remains, and it contributes to the total actual_length value for the entire URB. > Also, is it expected that the em28xx doesn't have data to send for > some microframes (for analog video capture e.g. during horizontal > or vertical blank interval)? I guess in the analyzer that should > show as an IN token with no reply instead of DATA[012] replies. Not necessarily. It might send random data, or a short (maybe even 0-length) reply. In fact, the usbmon trace does show some short packets (as you mentioned above) and even some 0-length packets. For example, see timestamp 3976305153. > Can you confirm that in the error case there is no IN token at all? That's what the analyzer log showed. Of course, it's possible that the analyzer itself was faulty, but I don't think so because Devin said the missing packets matched exactly with the corrupted data in the following microframes. > And another dumb question: Is there another USB video grabber > with different hardware which also uses isoc and works? > E.g. the cx231xx? Or do you think it might have the same missing > microframe issue but copes better with it due to larger > buffer space on the device? The uvcvideo driver works. But as you point out, the circumstances may be significantly different. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
On Thu, 11 Jul 2013, Roger Quadros wrote: > > The other two problems are both related to the interaction between > > system PM and runtime PM. Suppose the controller is already runtime > > suspended when the system goes to sleep. Because it is runtime > > suspended, it is enabled for wakeup. But device_may_wakeup() could > > return false; if this happens then you have to do a runtime-resume in > > omap_ehci_suspend() before calling ehci_suspend(), so that the > > controller can be suspended again with wakeups disabled. (Or you could > > choose an alternative method for accomplishing the same result, such as > > disabling the wakeup signal from the pad without going through a whole > > EHCI resume/suspend cycle.) Conversely, if device_may_wakeup() returns > > true then you shouldn't do anything at all, because the controller is > > already suspended with the correct wakeup setting. > > I think this case is taken care of by the Runtime PM core at least for the > OMAP > platform according to the documentation > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647 No; that section refers only to races, not to wakeup settings. > At the end of this mail is the log during system suspend/resume > > You can notice the following sequence > > -ehci runtime suspends > -system suspend triggered > -ehci runtime resumes > -ehci suspends (uses new wakeup settings) > -system wakeup triggered > -ehci resumes > -ehci runtime suspends This is because the root hub was runtime suspended with the wrong wakeup setting. The USB core, which is careful about these things, resumed and re-suspended it with the proper wakeup setting. In the process, the controller had to be runtime resumed as well. Try doing the test over again, but this time with the root hub enabled for wakeup and the controller disabled. (I know this is a bizarre combination, but try it anyway.) Also, after the system wakes up, see whether the root hub and controller get runtime suspended. > > For the third problem, suppose the controller was runtime suspended > > when the system went to sleep. When the system wakes up, the > > controller will become active, so you have to inform the runtime PM > > core about its change of state. Basically, if omap_ehci_resume() sees > > that ehci_resume() returned 0 then it must do: > > > > pm_runtime_disable(dev); > > pm_runtime_set_active(dev); > > pm_runtime_enable(dev); > > > > All of these issues are discussed (among lots of other material) in > > Documentation/power/runtime_pm.txt. > > Is this still applicable? Documentation claims > >"During system resume it calls pm_runtime_enable() and > pm_runtime_put_sync() > for every device right after executing the subsystem-level .resume_early() > callback and right after executing the subsystem-level .resume() callback > for it, respectively." Yes, this is applicable, but it is irrelevant to the problem I described. You still have to tell the runtime PM core that the device is now active. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Thu, 11 Jul 2013, Geert Uytterhoeven wrote:
> On Thu, Jul 11, 2013 at 3:01 AM, Alan Stern wrote:
> > On Thu, 11 Jul 2013, Arnd Bergmann wrote:
> >
> >> On Wednesday 10 July 2013, Alan Stern wrote:
> >> > This isn't right. There are USB host controllers that use PIO, not
> >> > DMA. The HAS_DMA dependency should go with the controller driver, not
> >> > the USB core.
> >> >
> >> > On the other hand, the USB core does call various routines like
> >> > dma_unmap_single. It ought to be possible to compile these calls even
> >> > when DMA isn't enabled. That is, they should be defined as do-nothing
> >> > stubs.
> >>
> >> The asm-generic/dma-mapping-broken.h file intentionally causes link
> >> errors, but that could be changed.
> >>
> >> The better approach in my mind would be to replace code like
> >>
> >>
> >> if (hcd->self.uses_dma)
> >>
> >> with
> >>
> >> if (IS_ENABLED(CONFIG_HAS_DMA) && hcd->self.uses_dma) {
> >>
> >> which will reliably cause that reference to be omitted from object code,
> >> but not stop giving link errors for drivers that actually require
> >> DMA.
> >
> > How will it give link errors for drivers that require DMA?
>
> It won't. Unless the host driver itself calls into the DMA API, too
> (are there any that don't?).
To my knowledge, all the host controller drivers which use DMA _do_
call functions in the DMA API. So they would still get link errors,
even though the USB core wouldn't.
Therefore adding the appropriate HAS_DMA dependencies should be
straightforward: Try to build all the drivers and see which ones fail
to link.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Linux USB file storage gadget with new UDC
On Thu, 11 Jul 2013, Victor Yeo wrote: > Thanks. The USBCV test has tight timing requirement. Once Set-Config > request is sent out, USBCV sends out Get-Config request to get the > config value immediately. At that time, gadget driver has not yet done > the handle_exception. So Get-Config request returns old config value, > and USBCV declares the test failed. Please see the log below. > > Is there any way to speed up the handle_exception No. > or to ask the USBCV > host to not send out Get-Config immediately? I am not at all familiar with USBCV, but I doubt it. However, the USB-2 spec says (section 9.2.6.4) that devices should be able to carry out requests with no Data stage (such as Set-Config) within 50 ms. Does your gadget really take longer than that to handle the exception? To find out, collect a usbmon trace showing what happens when your new driver is plugged into a Linux host. > I have set the NAK and > stall the endpoint 0 after receiving Set-Config request, however, That doesn't make sense. Stalling the endpoint means sending a STALL packet. You can't send both a STALL and a NAK. > Get-Config request is still sent out by USBCV host immediately. There should be at least a 50-ms delay, unless the UDC driver is doing something wrong. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Audio I/O parameters
On Thu, 11 Jul 2013, James Stone wrote: > Hi Clemens, > > On Mon, Jul 8, 2013 at 2:12 PM, James Stone wrote: > > >> Acquire audio card Audio0 > >> creating alsa driver ... hw:USB,0|-|64|2|44100|0|0|nomon|swmeter|-|16bit > >> Using ALSA driver USB-Audio running on card 0 - Focusrite Scarlett 2i4 > >> USB at usb-:00:12.2-3, high speed > >> configuring for 44100Hz, period = 64 frames (1.5 ms), buffer = 2 periods > >> ALSA: final selected sample format for playback: 32bit integer > >> little-endian > >> ALSA: use 2 periods for playback > >> ALSA: poll time out, polled for 2176094 usecs > >> JackAudioDriver::ProcessAsync: read error, stopping... > >> > >> This is a definite reduction in performance compared to earlier kernels. > >> > > > > Some further info - on 3.5.0-28, I can start jackd in playback only > > with 8 frames/period, and capture only at 16 frames/period. > > > > Any thoughts on further investigating this bug with the 3.8.0 kernel > with the Focusrite Scarlett 2i4? I'm happy to continue with any > further testing if it would be helpful.. Clemens, if it's not already clear, I have reached the limit of my knowledge at this point. We are both hoping that you can help solve this new bug. James, I can offer one suggestion: Try running JACK under strace, with playback-only at 64 frames/period. The strace output may indicate why JACK thinks an error occurred when in fact the USB transfers worked perfectly. > One thing is that another person affected by the same bug reports that > it may be hardware-specific: See: > > https://bugs.launchpad.net/ubuntu/+source/linux-lowlatency/+bug/1185563 > > Jori Neimi reports: "My laptop can handle jackd with a latency of 32 > samples on my Focusrite Scarlett 2i2 and 3.8.0-25 lowlatency kernel. > On my desktop jackd won't even start with a latency of less than 512 > samples using the same kernel and same USB audio device. No help from > the proposed 3.8.0-26, so I'll continue using 3.5.x kernels on my > desktop." > > Could this mean it is specific to some type of USB hardware on the > motherboard?? Yes, with the existing drivers hardware differences can affect the result. But with Clemens's new patch applied, the hardware shouldn't matter. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 10:34 PM, Takashi Iwai wrote:
> At Thu, 11 Jul 2013 22:13:35 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai wrote:
>> > At Thu, 11 Jul 2013 17:08:30 +0400,
>> > Sergei Shtylyov wrote:
>> >>
>> >> On 11-07-2013 13:06, Ming Lei wrote:
>> >>
>> >> > Complete() will be run with interrupt enabled, so change to
>> >> > spin_lock_irqsave().
>> >>
>> >> Changelog doesn't match the patch.
>> >
>> > Yep, but moreover...
>> >
>> >> > Cc: Jaroslav Kysela
>> >> > Cc: Takashi Iwai
>> >> > Cc: alsa-de...@alsa-project.org
>> >> > Signed-off-by: Ming Lei
>> >> > ---
>> >> > sound/usb/usx2y/usbusx2yaudio.c |4
>> >> > 1 file changed, 4 insertions(+)
>> >>
>> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c
>> >> > b/sound/usb/usx2y/usbusx2yaudio.c
>> >> > index 4967fe9..e2ee893 100644
>> >> > --- a/sound/usb/usx2y/usbusx2yaudio.c
>> >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
>> >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev
>> >> > *usX2Y)
>> >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
>> >> > if (subs) {
>> >> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
>> >> > + unsigned long flags;
>> >> > +
>> >> > + local_irq_save(flags);
>> >> > snd_pcm_stop(subs->pcm_substream,
>> >> > SNDRV_PCM_STATE_XRUN);
>> >> > + local_irq_restore(flags);
>> >> > }
>> >
>> > ... actually this snd_pcm_stop() call should have been covered by
>> > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
>> > together with the change, i.e. wrapping with
>> > snd_pcm_stream_lock_irqsave().
>>
>> Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
>> out similar patch later, :-)
>>
>> >
>> > I'll prepare the patch for 3.11 independently from your patch series,
>> > so please drop this one.
>>
>> OK, thanks for dealing with that.
>>
>> >
>> >
>> > BTW, the word "cleanup" in the subject is inappropriate. This is
>> > rather a fix together with the core change.
>>
>> It is a cleanup since the patchset only addresses lock problem which
>> is caused by the tasklet conversion.
>
> Well, the conversion to irqsave() is needed for the future drop of
> irq_save() in the caller, right? Then this isn't a cleanup but a
> preparation for movement ahead.
Sounds more accurate, and I will change the title in next round, :-)
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
At Thu, 11 Jul 2013 22:13:35 +0800,
Ming Lei wrote:
>
> On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai wrote:
> > At Thu, 11 Jul 2013 17:08:30 +0400,
> > Sergei Shtylyov wrote:
> >>
> >> On 11-07-2013 13:06, Ming Lei wrote:
> >>
> >> > Complete() will be run with interrupt enabled, so change to
> >> > spin_lock_irqsave().
> >>
> >> Changelog doesn't match the patch.
> >
> > Yep, but moreover...
> >
> >> > Cc: Jaroslav Kysela
> >> > Cc: Takashi Iwai
> >> > Cc: alsa-de...@alsa-project.org
> >> > Signed-off-by: Ming Lei
> >> > ---
> >> > sound/usb/usx2y/usbusx2yaudio.c |4
> >> > 1 file changed, 4 insertions(+)
> >>
> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c
> >> > b/sound/usb/usx2y/usbusx2yaudio.c
> >> > index 4967fe9..e2ee893 100644
> >> > --- a/sound/usb/usx2y/usbusx2yaudio.c
> >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
> >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev
> >> > *usX2Y)
> >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
> >> > if (subs) {
> >> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
> >> > + unsigned long flags;
> >> > +
> >> > + local_irq_save(flags);
> >> > snd_pcm_stop(subs->pcm_substream,
> >> > SNDRV_PCM_STATE_XRUN);
> >> > + local_irq_restore(flags);
> >> > }
> >
> > ... actually this snd_pcm_stop() call should have been covered by
> > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
> > together with the change, i.e. wrapping with
> > snd_pcm_stream_lock_irqsave().
>
> Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
> out similar patch later, :-)
>
> >
> > I'll prepare the patch for 3.11 independently from your patch series,
> > so please drop this one.
>
> OK, thanks for dealing with that.
>
> >
> >
> > BTW, the word "cleanup" in the subject is inappropriate. This is
> > rather a fix together with the core change.
>
> It is a cleanup since the patchset only addresses lock problem which
> is caused by the tasklet conversion.
Well, the conversion to irqsave() is needed for the future drop of
irq_save() in the caller, right? Then this isn't a cleanup but a
preparation for movement ahead.
> > And, I wonder whether we can take a safer approach. When the caller
> > condition changed, we often introduced either a different ops
> > (e.g. ioctl case) or a flag for the new condition, at least during the
> > transition period.
>
> Interrupt is't enabled until all current drivers are cleaned up, so it is
> really
> safe, please see patch [2].
OK.
> > Last but not least, is a conversion to tasklet really preferred?
> > tasklet is rather an obsoleted infrastructure nowadays, and people
> > don't recommend to use it any longer, AFAIK.
>
> We discussed the problem in the below link previously[1], Steven
> and Thomas suggested to use threaded irq handler, but which
> may degrade USB mass storage performance, so we have to
> take tasklet now until we rewrite transport part of USB mass storage
> driver.
>
> Also the conversion[2] has avoided the tasklet spin lock problem
> already.
OK, good to know.
thanks,
Takashi
> [1], http://marc.info/?t=13707911921&r=1&w=2
> [2], http://marc.info/?l=linux-usb&m=137286326726326&w=2
>
> Thanks,
> --
> Ming Lei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai wrote:
> At Thu, 11 Jul 2013 17:08:30 +0400,
> Sergei Shtylyov wrote:
>>
>> On 11-07-2013 13:06, Ming Lei wrote:
>>
>> > Complete() will be run with interrupt enabled, so change to
>> > spin_lock_irqsave().
>>
>> Changelog doesn't match the patch.
>
> Yep, but moreover...
>
>> > Cc: Jaroslav Kysela
>> > Cc: Takashi Iwai
>> > Cc: alsa-de...@alsa-project.org
>> > Signed-off-by: Ming Lei
>> > ---
>> > sound/usb/usx2y/usbusx2yaudio.c |4
>> > 1 file changed, 4 insertions(+)
>>
>> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c
>> > b/sound/usb/usx2y/usbusx2yaudio.c
>> > index 4967fe9..e2ee893 100644
>> > --- a/sound/usb/usx2y/usbusx2yaudio.c
>> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
>> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
>> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
>> > if (subs) {
>> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
>> > + unsigned long flags;
>> > +
>> > + local_irq_save(flags);
>> > snd_pcm_stop(subs->pcm_substream,
>> > SNDRV_PCM_STATE_XRUN);
>> > + local_irq_restore(flags);
>> > }
>
> ... actually this snd_pcm_stop() call should have been covered by
> snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
> together with the change, i.e. wrapping with
> snd_pcm_stream_lock_irqsave().
Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
out similar patch later, :-)
>
> I'll prepare the patch for 3.11 independently from your patch series,
> so please drop this one.
OK, thanks for dealing with that.
>
>
> BTW, the word "cleanup" in the subject is inappropriate. This is
> rather a fix together with the core change.
It is a cleanup since the patchset only addresses lock problem which
is caused by the tasklet conversion.
>
> And, I wonder whether we can take a safer approach. When the caller
> condition changed, we often introduced either a different ops
> (e.g. ioctl case) or a flag for the new condition, at least during the
> transition period.
Interrupt is't enabled until all current drivers are cleaned up, so it is really
safe, please see patch [2].
>
> Last but not least, is a conversion to tasklet really preferred?
> tasklet is rather an obsoleted infrastructure nowadays, and people
> don't recommend to use it any longer, AFAIK.
We discussed the problem in the below link previously[1], Steven
and Thomas suggested to use threaded irq handler, but which
may degrade USB mass storage performance, so we have to
take tasklet now until we rewrite transport part of USB mass storage
driver.
Also the conversion[2] has avoided the tasklet spin lock problem
already.
[1], http://marc.info/?t=13707911921&r=1&w=2
[2], http://marc.info/?l=linux-usb&m=137286326726326&w=2
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup
On 11.07.2013 11:06, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
>
> Cc: Daniel Mack
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Cc: alsa-de...@alsa-project.org
> Signed-off-by: Ming Lei
Sound right to me, thanks.
Acked-by: Daniel Mack
> ---
> sound/usb/caiaq/audio.c |5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 7103b09..e5675ab 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
> offset += len;
>
> if (len > 0) {
> - spin_lock(&cdev->spinlock);
> + unsigned long flags;
> + spin_lock_irqsave(&cdev->spinlock, flags);
> fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
> read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
> - spin_unlock(&cdev->spinlock);
> + spin_unlock_irqrestore(&cdev->spinlock, flags);
> check_for_elapsed_periods(cdev, cdev->sub_playback);
> check_for_elapsed_periods(cdev, cdev->sub_capture);
> send_it = 1;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/50] USB: serial: sierra: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 9:02 PM, Sergei Shtylyov wrote: > Hello. > > > On 11-07-2013 13:05, Ming Lei wrote: > >> Complete() will be run with interrupt enabled, so change to >> spin_lock_irqsave(). > > >> Cc: Johan Hovold >> Signed-off-by: Ming Lei >> --- >> drivers/usb/serial/sierra.c |9 + >> 1 file changed, 5 insertions(+), 4 deletions(-) > > >> diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c >> index de958c5..e79b6ad 100644 >> --- a/drivers/usb/serial/sierra.c >> +++ b/drivers/usb/serial/sierra.c >> @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb) >> struct sierra_port_private *portdata = >> usb_get_serial_port_data(port); >> struct sierra_intf_private *intfdata; >> int status = urb->status; >> + unsigned long flags; >> >> intfdata = port->serial->private; >> >> @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb) >> dev_dbg(&port->dev, "%s - nonzero write bulk status " >> "received: %d\n", __func__, status); >> >> - spin_lock(&portdata->lock); >> + spin_lock_irqsave(&portdata->lock, flags); >> --portdata->outstanding_urbs; >> - spin_unlock(&portdata->lock); >> - spin_lock(&intfdata->susp_lock); >> + spin_unlock_irqrestore(&portdata->lock, flags); >> + spin_lock_irqsave(&intfdata->susp_lock, flags); > > > You are allowing an interrupt enabled window where previously it wasn't > possible. Why notleave these 2 lines as is? It isn't a big deal because no lock is held when interrupt is enabled. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] usb: gadget: USB_FOTG210_UDC should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `fotg210_start_dma': drivers/usb/gadget/fotg210-udc.c:354: undefined reference to `dma_map_single' drivers/usb/gadget/fotg210-udc.c:357: undefined reference to `dma_mapping_error' drivers/usb/gadget/fotg210-udc.c:362: undefined reference to `dma_sync_single_for_cpu' drivers/usb/gadget/fotg210-udc.c:376: undefined reference to `dma_unmap_single' Signed-off-by: Geert Uytterhoeven --- drivers/usb/gadget/Kconfig |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 95375d1..8743742 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -193,6 +193,7 @@ config USB_FUSB300 Faraday usb device controller FUSB300 driver config USB_FOTG210_UDC + depends on HAS_DMA tristate "Faraday FOTG210 USB Peripheral Controller" help Faraday USB2.0 OTG controller which can be configured as -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] usb: chipidea: USB_CHIPIDEA_UDC should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `dma_set_coherent_mask': include/linux/dma-mapping.h:93: undefined reference to `dma_supported' drivers/built-in.o: In function `destroy_eps': drivers/usb/chipidea/udc.c:1598: undefined reference to `dma_pool_free' drivers/built-in.o: In function `free_pending_td': drivers/usb/chipidea/udc.c:540: undefined reference to `dma_pool_free' drivers/built-in.o: In function `add_td_to_list': drivers/usb/chipidea/udc.c:383: undefined reference to `dma_pool_alloc' drivers/built-in.o: In function `_ep_nuke': drivers/usb/chipidea/udc.c:639: undefined reference to `dma_pool_free' drivers/built-in.o: In function `udc_stop': drivers/usb/chipidea/udc.c:1855: undefined reference to `dma_pool_destroy' drivers/built-in.o: In function `init_eps': drivers/usb/chipidea/udc.c:1564: undefined reference to `dma_pool_alloc' drivers/built-in.o: In function `udc_start': drivers/usb/chipidea/udc.c:1766: undefined reference to `dma_pool_create' drivers/usb/chipidea/udc.c:1835: undefined reference to `dma_pool_destroy' drivers/usb/chipidea/udc.c:1837: undefined reference to `dma_pool_destroy' drivers/built-in.o: In function `ep_free_request': drivers/usb/chipidea/udc.c:1268: undefined reference to `dma_pool_free' Signed-off-by: Geert Uytterhoeven --- drivers/usb/chipidea/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig index eb2aa2e..189dbc6 100644 --- a/drivers/usb/chipidea/Kconfig +++ b/drivers/usb/chipidea/Kconfig @@ -12,7 +12,7 @@ if USB_CHIPIDEA config USB_CHIPIDEA_UDC bool "ChipIdea device controller" - depends on USB_GADGET=y || USB_CHIPIDEA=m + depends on (USB_GADGET=y || USB_CHIPIDEA=m) && HAS_DMA help Say Y here to enable device controller functionality of the ChipIdea driver. -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] usb: dwc3: USB_DWC3 should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `dwc3_free_one_event_buffer': drivers/usb/dwc3/core.c:132: undefined reference to `dma_free_coherent' drivers/built-in.o: In function `dwc3_alloc_one_event_buffer': drivers/usb/dwc3/core.c:154: undefined reference to `dma_alloc_coherent' drivers/built-in.o: In function `dma_set_coherent_mask': include/linux/dma-mapping.h:93: undefined reference to `dma_supported' drivers/built-in.o: In function `dwc3_free_trb_pool': drivers/usb/dwc3/gadget.c:407: undefined reference to `dma_free_coherent' drivers/built-in.o: In function `dwc3_gadget_exit': drivers/usb/dwc3/gadget.c:2693: undefined reference to `dma_free_coherent' drivers/built-in.o: In function `dwc3_alloc_trb_pool': drivers/usb/dwc3/gadget.c:391: undefined reference to `dma_alloc_coherent' drivers/built-in.o: In function `dwc3_gadget_init': drivers/usb/dwc3/gadget.c:2598: undefined reference to `dma_alloc_coherent' drivers/usb/dwc3/gadget.c:2667: undefined reference to `dma_free_coherent' drivers/usb/dwc3/gadget.c:2674: undefined reference to `dma_free_coherent' drivers/usb/dwc3/gadget.c:2678: undefined reference to `dma_free_coherent' Signed-off-by: Geert Uytterhoeven --- drivers/usb/dwc3/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 757aa18..2378958 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -1,6 +1,6 @@ config USB_DWC3 tristate "DesignWare USB3 DRD Core Support" - depends on (USB || USB_GADGET) && GENERIC_HARDIRQS + depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD help Say Y or M here if your system has a Dual Role SuperSpeed -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] [RFC] usb: gadget: USB_GADGET should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `dma_set_coherent_mask': include/linux/dma-mapping.h:93: undefined reference to `dma_supported' include/linux/dma-mapping.h:93: undefined reference to `dma_supported' drivers/built-in.o: In function `usb_gadget_unmap_request': drivers/usb/gadget/udc-core.c:91: undefined reference to `dma_unmap_sg' drivers/usb/gadget/udc-core.c:96: undefined reference to `dma_unmap_single' drivers/built-in.o: In function `usb_gadget_map_request': drivers/usb/gadget/udc-core.c:62: undefined reference to `dma_map_sg' drivers/usb/gadget/udc-core.c:71: undefined reference to `dma_map_single' drivers/usb/gadget/udc-core.c:74: undefined reference to `dma_mapping_error' Signed-off-by: Geert Uytterhoeven --- This one is very debatable: probably the parts using the DMA API should be factored out, instead of disabling the whole USB gadget subsystem. drivers/usb/gadget/Kconfig |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 3f8df56..a88fc47 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -15,6 +15,7 @@ menuconfig USB_GADGET tristate "USB Gadget Support" + depends on HAS_DMA select NLS help USB is a master/slave protocol, organized with one master -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] usb: gadget: USB_MV_U3D should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `mv_u3d_done': drivers/usb/gadget/mv_u3d_core.c:206: undefined reference to `dma_pool_free' drivers/usb/gadget/mv_u3d_core.c:209: undefined reference to `dma_unmap_single' drivers/built-in.o: In function `mv_u3d_build_trb_one': drivers/usb/gadget/mv_u3d_core.c:311: undefined reference to `dma_pool_alloc' drivers/built-in.o: In function `mv_u3d_req_to_trb': drivers/usb/gadget/mv_u3d_core.c:480: undefined reference to `dma_map_single' drivers/built-in.o: In function `mv_u3d_remove': drivers/usb/gadget/mv_u3d_core.c:1770: undefined reference to `dma_pool_destroy' drivers/usb/gadget/mv_u3d_core.c:1773: undefined reference to `dma_free_coherent' drivers/built-in.o: In function `mv_u3d_probe': drivers/usb/gadget/mv_u3d_core.c:1880: undefined reference to `dma_alloc_coherent' drivers/usb/gadget/mv_u3d_core.c:1890: undefined reference to `dma_pool_create' drivers/usb/gadget/mv_u3d_core.c:1984: undefined reference to `dma_pool_destroy' drivers/usb/gadget/mv_u3d_core.c:1986: undefined reference to `dma_free_coherent' Signed-off-by: Geert Uytterhoeven --- drivers/usb/gadget/Kconfig |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 8743742..3f8df56 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -336,6 +336,7 @@ config USB_MV_UDC full speed USB peripheral. config USB_MV_U3D + depends on HAS_DMA tristate "MARVELL PXA2128 USB 3.0 controller" help MARVELL PXA2128 Processor series include a super speed USB3.0 device -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] usb: gadget: USB_MV_UDC should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `done': drivers/usb/gadget/mv_udc_core.c:239: undefined reference to `dma_pool_free' drivers/built-in.o: In function `build_dtd': drivers/usb/gadget/mv_udc_core.c:371: undefined reference to `dma_pool_alloc' drivers/built-in.o: In function `udc_prime_status': drivers/usb/gadget/mv_udc_core.c:1465: undefined reference to `dma_map_single' drivers/built-in.o: In function `mv_udc_remove': drivers/usb/gadget/mv_udc_core.c:2087: undefined reference to `dma_pool_destroy' drivers/usb/gadget/mv_udc_core.c:2090: undefined reference to `dma_free_coherent' drivers/built-in.o: In function `mv_udc_probe': drivers/usb/gadget/mv_udc_core.c:2190: undefined reference to `dma_alloc_coherent' drivers/usb/gadget/mv_udc_core.c:2201: undefined reference to `dma_pool_create' drivers/usb/gadget/mv_udc_core.c:2315: undefined reference to `dma_pool_destroy' drivers/usb/gadget/mv_udc_core.c:2317: undefined reference to `dma_free_coherent' Signed-off-by: Geert Uytterhoeven --- drivers/usb/gadget/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 62f6802..95375d1 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -328,7 +328,7 @@ config USB_S3C_HSUDC config USB_MV_UDC tristate "Marvell USB2.0 Device Controller" - depends on GENERIC_HARDIRQS + depends on GENERIC_HARDIRQS && HAS_DMA help Marvell Socs (including PXA and MMP series) include a high speed USB2.0 OTG controller, which can be configured as high speed or -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
At Thu, 11 Jul 2013 17:08:30 +0400,
Sergei Shtylyov wrote:
>
> On 11-07-2013 13:06, Ming Lei wrote:
>
> > Complete() will be run with interrupt enabled, so change to
> > spin_lock_irqsave().
>
> Changelog doesn't match the patch.
Yep, but moreover...
> > Cc: Jaroslav Kysela
> > Cc: Takashi Iwai
> > Cc: alsa-de...@alsa-project.org
> > Signed-off-by: Ming Lei
> > ---
> > sound/usb/usx2y/usbusx2yaudio.c |4
> > 1 file changed, 4 insertions(+)
>
> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c
> > b/sound/usb/usx2y/usbusx2yaudio.c
> > index 4967fe9..e2ee893 100644
> > --- a/sound/usb/usx2y/usbusx2yaudio.c
> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
> > if (subs) {
> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > snd_pcm_stop(subs->pcm_substream,
> > SNDRV_PCM_STATE_XRUN);
> > + local_irq_restore(flags);
> > }
... actually this snd_pcm_stop() call should have been covered by
snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
together with the change, i.e. wrapping with
snd_pcm_stream_lock_irqsave().
I'll prepare the patch for 3.11 independently from your patch series,
so please drop this one.
BTW, the word "cleanup" in the subject is inappropriate. This is
rather a fix together with the core change.
And, I wonder whether we can take a safer approach. When the caller
condition changed, we often introduced either a different ops
(e.g. ioctl case) or a flag for the new condition, at least during the
transition period.
Last but not least, is a conversion to tasklet really preferred?
tasklet is rather an obsoleted infrastructure nowadays, and people
don't recommend to use it any longer, AFAIK.
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Hello. On 11-07-2013 16:36, Oliver Neukum wrote: I don't think this patch passes checkpatch.pl. This series is a mechanical replacement in dozens of drivers. That mechanicity shows too much in some patches. We cannot demand nice formatting. If you want to do something productive, check the locking in the driver. I'm not paid for it and don't have time to do it for free. Regards Oliver WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 42/50] media: usb: tlg2300: spin_lock in complete() cleanup
On 11-07-2013 13:06, Ming Lei wrote: Subject doesn't match the patch. Complete() will be run with interrupt enabled, so disable local interrupt before holding a global lock which is held without irqsave. Cc: Mauro Carvalho Chehab Cc: linux-me...@vger.kernel.org Signed-off-by: Ming Lei --- drivers/media/usb/tlg2300/pd-alsa.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/media/usb/tlg2300/pd-alsa.c b/drivers/media/usb/tlg2300/pd-alsa.c index 3f3e141..cbccc96 100644 --- a/drivers/media/usb/tlg2300/pd-alsa.c +++ b/drivers/media/usb/tlg2300/pd-alsa.c [...] @@ -156,6 +157,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed) memcpy(runtime->dma_area + oldptr * stride, cp, len * stride); /* update the statas */ + local_irq_save(flags); snd_pcm_stream_lock(pa->capture_pcm_substream); pa->rcv_position += len; if (pa->rcv_position >= runtime->buffer_size) @@ -167,6 +169,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed) *period_elapsed = 1; } snd_pcm_stream_unlock(pa->capture_pcm_substream); + local_irq_restore(flags); } WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup
On 11-07-2013 13:06, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Daniel Mack
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei
---
sound/usb/caiaq/audio.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 7103b09..e5675ab 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
offset += len;
if (len > 0) {
- spin_lock(&cdev->spinlock);
+ unsigned long flags;
Emoty line wouldn't hurt here, after declaration.
+ spin_lock_irqsave(&cdev->spinlock, flags);
fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
- spin_unlock(&cdev->spinlock);
+ spin_unlock_irqrestore(&cdev->spinlock, flags);
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 46/50] Sound: usb: ua101: spin_lock in complete() cleanup
On 11-07-2013 13:06, Ming Lei wrote:
Here the subject doesn't match the patch.
Complete() will be run with interrupt enabled, so disable local
interrupt before holding a global lock which is held without irqsave.
Cc: Clemens Ladisch
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei
---
sound/usb/misc/ua101.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
index 8b5d2c5..52a60c6 100644
--- a/sound/usb/misc/ua101.c
+++ b/sound/usb/misc/ua101.c
@@ -613,14 +613,24 @@ static int start_usb_playback(struct ua101 *ua)
static void abort_alsa_capture(struct ua101 *ua)
{
- if (test_bit(ALSA_CAPTURE_RUNNING, &ua->states))
+ if (test_bit(ALSA_CAPTURE_RUNNING, &ua->states)) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(ua->capture.substream, SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
+ }
}
static void abort_alsa_playback(struct ua101 *ua)
{
- if (test_bit(ALSA_PLAYBACK_RUNNING, &ua->states))
+ if (test_bit(ALSA_PLAYBACK_RUNNING, &ua->states)) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(ua->playback.substream, SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
+ }
}
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
On 11-07-2013 13:06, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Changelog doesn't match the patch.
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei
---
sound/usb/usx2y/usbusx2yaudio.c |4
1 file changed, 4 insertions(+)
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 4967fe9..e2ee893 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
struct snd_usX2Y_substream *subs = usX2Y->subs[s];
if (subs) {
if (atomic_read(&subs->state) >= state_PRERUNNING) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(subs->pcm_substream,
SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
}
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/50] USB: serial: sierra: spin_lock in complete() cleanup
Hello. On 11-07-2013 13:05, Ming Lei wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Johan Hovold Signed-off-by: Ming Lei --- drivers/usb/serial/sierra.c |9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c index de958c5..e79b6ad 100644 --- a/drivers/usb/serial/sierra.c +++ b/drivers/usb/serial/sierra.c @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb) struct sierra_port_private *portdata = usb_get_serial_port_data(port); struct sierra_intf_private *intfdata; int status = urb->status; + unsigned long flags; intfdata = port->serial->private; @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb) dev_dbg(&port->dev, "%s - nonzero write bulk status " "received: %d\n", __func__, status); - spin_lock(&portdata->lock); + spin_lock_irqsave(&portdata->lock, flags); --portdata->outstanding_urbs; - spin_unlock(&portdata->lock); - spin_lock(&intfdata->susp_lock); + spin_unlock_irqrestore(&portdata->lock, flags); + spin_lock_irqsave(&intfdata->susp_lock, flags); You are allowing an interrupt enabled window where previously it wasn't possible. Why notleave these 2 lines as is? --intfdata->in_flight; - spin_unlock(&intfdata->susp_lock); + spin_unlock_irqrestore(&intfdata->susp_lock, flags); usb_serial_port_softint(port); } WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 36/50] media: usb: em28xx: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 5:05 AM, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
>
> Cc: Mauro Carvalho Chehab
> Cc: linux-me...@vger.kernel.org
> Signed-off-by: Ming Lei
> ---
> drivers/media/usb/em28xx/em28xx-core.c |5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c
> b/drivers/media/usb/em28xx/em28xx-core.c
> index fc157af..0d698f9 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -941,6 +941,7 @@ static void em28xx_irq_callback(struct urb *urb)
> {
> struct em28xx *dev = urb->context;
> int i;
> + unsigned long flags;
>
> switch (urb->status) {
> case 0: /* success */
> @@ -956,9 +957,9 @@ static void em28xx_irq_callback(struct urb *urb)
> }
>
> /* Copy data from URB */
> - spin_lock(&dev->slock);
> + spin_lock_irqsave(&dev->slock, flags);
> dev->usb_ctl.urb_data_copy(dev, urb);
> - spin_unlock(&dev->slock);
> + spin_unlock_irqrestore(&dev->slock, flags);
>
> /* Reset urb buffers */
> for (i = 0; i < urb->number_of_packets; i++) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
I actually stumbled across this a couple of weeks ago, and have had an
identical patch running in a local dev tree.
Reviewed-by: Devin Heitmueller
Tested-by: Devin Heitmueller
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 8:18 PM, Sergei Shtylyov
wrote:
> Hello.
>
>
> On 11-07-2013 13:05, Ming Lei wrote:
>
>> Complete() will be run with interrupt enabled, so change to
>> spin_lock_irqsave().
>
>
>> Cc: Juergen Stuber
>> Signed-off-by: Ming Lei
>> ---
>> drivers/usb/misc/legousbtower.c |5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>
>> diff --git a/drivers/usb/misc/legousbtower.c
>> b/drivers/usb/misc/legousbtower.c
>> index 8089479..4044989 100644
>> --- a/drivers/usb/misc/legousbtower.c
>> +++ b/drivers/usb/misc/legousbtower.c
>> @@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> struct lego_usb_tower *dev = urb->context;
>> int status = urb->status;
>> int retval;
>> + unsigned long flags;
>>
>> dbg(4, "%s: enter, status %d", __func__, status);
>>
>> @@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> }
>>
>> if (urb->actual_length > 0) {
>> - spin_lock (&dev->read_buffer_lock);
>> + spin_lock_irqsave (&dev->read_buffer_lock, flags);
>> if (dev->read_buffer_length + urb->actual_length <
>> read_buffer_size) {
>> memcpy (dev->read_buffer +
>> dev->read_buffer_length,
>> dev->interrupt_in_buffer,
>> @@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> } else {
>> printk(KERN_WARNING "%s: read_buffer overflow, %d
>> bytes dropped", __func__, urb->actual_length);
>> }
>> - spin_unlock (&dev->read_buffer_lock);
>> + spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
>> }
>
>
>I don't think this patch passes checkpatch.pl.
No errors reported from checkpatch.pl, only warnings which isn't introduced
by this patch.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
On Thursday 11 July 2013 16:18:17 Sergei Shtylyov wrote: > I don't think this patch passes checkpatch.pl. This series is a mechanical replacement in dozens of drivers. We cannot demand nice formatting. If you want to do something productive, check the locking in the driver. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] staging: dwc2: fix value used in dwc2_set_all_params
From: Julien Delacou This fix uses 'value' parameter as it should be instead of hardcoded -1. Signed-off-by: Julien Delacou Acked-by: Paul Zimmerman --- drivers/staging/dwc2/hcd.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c index 2ed54b1..ca38aaa 100644 --- a/drivers/staging/dwc2/hcd.c +++ b/drivers/staging/dwc2/hcd.c @@ -2690,7 +2690,7 @@ void dwc2_set_all_params(struct dwc2_core_params *params, int value) int i; for (i = 0; i < size; i++) - p[i] = -1; + p[i] = value; } EXPORT_SYMBOL_GPL(dwc2_set_all_params); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Hello.
On 11-07-2013 13:05, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber
Signed-off-by: Ming Lei
---
drivers/usb/misc/legousbtower.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 8089479..4044989 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb->context;
int status = urb->status;
int retval;
+ unsigned long flags;
dbg(4, "%s: enter, status %d", __func__, status);
@@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb->actual_length > 0) {
- spin_lock (&dev->read_buffer_lock);
+ spin_lock_irqsave (&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length <
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
} else {
printk(KERN_WARNING "%s: read_buffer overflow, %d bytes
dropped", __func__, urb->actual_length);
}
- spin_unlock (&dev->read_buffer_lock);
+ spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
}
I don't think this patch passes checkpatch.pl.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/50] USB: misc: uss720: spin_lock in complete() cleanup
Hello.
On 11-07-2013 13:05, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei
---
drivers/usb/misc/uss720.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
index e129cf6..f7d15e8 100644
--- a/drivers/usb/misc/uss720.c
+++ b/drivers/usb/misc/uss720.c
@@ -121,6 +121,7 @@ static void async_complete(struct urb *urb)
dev_err(&urb->dev->dev, "async_complete: urb error %d\n",
status);
} else if (rq->dr.bRequest == 3) {
+ unsigned long flags;
Empty line wouldn't hurt here, after declaration.
memcpy(priv->reg, rq->reg, sizeof(priv->reg));
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Announcing libusbx-1.0.16 final
Hi All, I'm very happy to announce the official libusbx-1.0.16 release! Highlights of changes since 1.0.15: --- * As Nathan Hjelm already announced in his "libusb and libusbx merging" mail, Nathan has taken over libusb maintenance and this release is a combined effort between the libusb and libusbx projects! * Hotplug support for Darwin and Linux * Superspeed endpoint companion and BOS descriptor support * We finally have a libusb_strerror API You can download the 1.0.16 tarbal here: http://downloads.sourceforge.net/libusbx/libusbx-1.0.16.tar.bz2 Please give it a thorough testing, and report any issues you find. For those interested the full ChangeLog is below. Regards, Hans 2013-07-11: v1.0.16 * Add hotplug support for Darwin and Linux (#9) * Add superspeed endpoint companion descriptor support (#15) * Add BOS descriptor support (#15) * Make descriptor parsing code more robust * New libusb_get_port_numbers API, this is libusb_get_port_path without the unnecessary context parameter, libusb_get_port_path is now deprecated * New libusb_strerror API (#14) * New libusb_set_auto_detach_kernel_driver API (#17) * Improve topology API docs (#95) * Logging now use a single write call per log-message, avoiding log-message "interlacing" when using multiple threads. * Android: use Android logging when building on Android (#101) * Darwin: make libusb_reset reenumerate device on descriptors change (#89) * Darwin: add support for the LIBUSB_TRANSFER_ADD_ZERO_PACKET flag (#91) * Darwin: add a device cache (#112, #114) * Examples: Add sam3u_benchmark isochronous example by Harald Welte (#109) * Many other bug fixes and improvements The (#xx) numbers are libusbx issue numbers, see ie: https://github.com/libusbx/libusbx/issues/9 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 10/13] usb: chipidea: add wait vbus lower than OTGSC_BSV before role starts
On 07/11/2013 11:25 AM, Peter Chen wrote: > On Thu, Jul 11, 2013 at 09:24:56AM +0200, Marc Kleine-Budde wrote: >> On 07/11/2013 08:27 AM, Peter Chen wrote: >>> When the gadget role starts, we need to make sure the vbus is lower >>> than OTGSC_BSV, or there will be an vbus interrupt since we use >>> B_SESSION_VALID as vbus interrupt to indicate connect and disconnect. >>> When the host role starts, it may not be useful to wait vbus to lower >>> than OTGSC_BSV, but it can indicate some hardware problems like the >>> vbus is still higher than OTGSC_BSV after we disconnect to host some >>> time later (500 jiffies currently), which is obvious not correct. >> >> Jiffies ist not constant. I think you have to specify the timeout in >> fractions of HZ. The system ticks with HZ jiffies per second. So a >> timeout of 500ms is "HZ / 2". > > Thanks, I will use msecs_to_jiffies. even better. > >>> +/** >>> + * hw_wait_reg: wait the register value >>> + * >>> + * Sometimes, it needs to wait register value before going on. >>> + * Eg, when switch to device mode, the vbus value should be lower >>> + * than OTGSC_BSV before connects to host. >>> + * >>> + * @ci: the controller >>> + * @reg: register index >>> + * @mask: mast bit >>> + * @value: the bit value to wait >>> + * @timeout: timeout to indicate an error >> >> >> Better: timeout in jiffies >> > > As I will use msecs_to_jiffies, @timeout: timeout in millisecond. Good idea - you might want to use timeout_ms. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
[PATCH 03/50] USB: usblp: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Pete Zaitcev
Signed-off-by: Ming Lei
---
drivers/usb/class/usblp.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index d4c47d5..04163d8 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -297,6 +297,7 @@ static void usblp_bulk_read(struct urb *urb)
{
struct usblp *usblp = urb->context;
int status = urb->status;
+ unsigned long flags;
if (usblp->present && usblp->used) {
if (status)
@@ -304,14 +305,14 @@ static void usblp_bulk_read(struct urb *urb)
"nonzero read bulk status received: %d\n",
usblp->minor, status);
}
- spin_lock(&usblp->lock);
+ spin_lock_irqsave(&usblp->lock, flags);
if (status < 0)
usblp->rstatus = status;
else
usblp->rstatus = urb->actual_length;
usblp->rcomplete = 1;
wake_up(&usblp->rwait);
- spin_unlock(&usblp->lock);
+ spin_unlock_irqrestore(&usblp->lock, flags);
usb_free_urb(urb);
}
@@ -320,6 +321,7 @@ static void usblp_bulk_write(struct urb *urb)
{
struct usblp *usblp = urb->context;
int status = urb->status;
+ unsigned long flags;
if (usblp->present && usblp->used) {
if (status)
@@ -327,7 +329,7 @@ static void usblp_bulk_write(struct urb *urb)
"nonzero write bulk status received: %d\n",
usblp->minor, status);
}
- spin_lock(&usblp->lock);
+ spin_lock_irqsave(&usblp->lock, flags);
if (status < 0)
usblp->wstatus = status;
else
@@ -335,7 +337,7 @@ static void usblp_bulk_write(struct urb *urb)
usblp->no_paper = 0;
usblp->wcomplete = 1;
wake_up(&usblp->wwait);
- spin_unlock(&usblp->lock);
+ spin_unlock_irqrestore(&usblp->lock, flags);
usb_free_urb(urb);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 10/13] usb: chipidea: add wait vbus lower than OTGSC_BSV before role starts
On Thu, Jul 11, 2013 at 09:24:56AM +0200, Marc Kleine-Budde wrote: > On 07/11/2013 08:27 AM, Peter Chen wrote: > > When the gadget role starts, we need to make sure the vbus is lower > > than OTGSC_BSV, or there will be an vbus interrupt since we use > > B_SESSION_VALID as vbus interrupt to indicate connect and disconnect. > > When the host role starts, it may not be useful to wait vbus to lower > > than OTGSC_BSV, but it can indicate some hardware problems like the > > vbus is still higher than OTGSC_BSV after we disconnect to host some > > time later (500 jiffies currently), which is obvious not correct. > > Jiffies ist not constant. I think you have to specify the timeout in > fractions of HZ. The system ticks with HZ jiffies per second. So a > timeout of 500ms is "HZ / 2". Thanks, I will use msecs_to_jiffies. > > +/** > > + * hw_wait_reg: wait the register value > > + * > > + * Sometimes, it needs to wait register value before going on. > > + * Eg, when switch to device mode, the vbus value should be lower > > + * than OTGSC_BSV before connects to host. > > + * > > + * @ci: the controller > > + * @reg: register index > > + * @mask: mast bit > > + * @value: the bit value to wait > > + * @timeout: timeout to indicate an error > > > Better: timeout in jiffies > As I will use msecs_to_jiffies, @timeout: timeout in millisecond. Thanks. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/50] USB: cdc-wdm: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Oliver Neukum
Signed-off-by: Ming Lei
---
drivers/usb/class/cdc-wdm.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 8a230f0..5f78d18 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -143,10 +143,12 @@ found:
static void wdm_out_callback(struct urb *urb)
{
struct wdm_device *desc;
+ unsigned long flags;
+
desc = urb->context;
- spin_lock(&desc->iuspin);
+ spin_lock_irqsave(&desc->iuspin, flags);
desc->werr = urb->status;
- spin_unlock(&desc->iuspin);
+ spin_unlock_irqrestore(&desc->iuspin, flags);
kfree(desc->outbuf);
desc->outbuf = NULL;
clear_bit(WDM_IN_USE, &desc->flags);
@@ -158,8 +160,9 @@ static void wdm_in_callback(struct urb *urb)
struct wdm_device *desc = urb->context;
int status = urb->status;
int length = urb->actual_length;
+ unsigned long flags;
- spin_lock(&desc->iuspin);
+ spin_lock_irqsave(&desc->iuspin, flags);
clear_bit(WDM_RESPONDING, &desc->flags);
if (status) {
@@ -203,7 +206,7 @@ skip_error:
wake_up(&desc->wait);
set_bit(WDM_READ, &desc->flags);
- spin_unlock(&desc->iuspin);
+ spin_unlock_irqrestore(&desc->iuspin, flags);
}
static void wdm_int_callback(struct urb *urb)
@@ -212,6 +215,7 @@ static void wdm_int_callback(struct urb *urb)
int status = urb->status;
struct wdm_device *desc;
struct usb_cdc_notification *dr;
+ unsigned long flags;
desc = urb->context;
dr = (struct usb_cdc_notification *)desc->sbuf;
@@ -260,7 +264,7 @@ static void wdm_int_callback(struct urb *urb)
goto exit;
}
- spin_lock(&desc->iuspin);
+ spin_lock_irqsave(&desc->iuspin, flags);
clear_bit(WDM_READ, &desc->flags);
set_bit(WDM_RESPONDING, &desc->flags);
if (!test_bit(WDM_DISCONNECTING, &desc->flags)
@@ -269,7 +273,7 @@ static void wdm_int_callback(struct urb *urb)
dev_dbg(&desc->intf->dev, "%s: usb_submit_urb %d",
__func__, rv);
}
- spin_unlock(&desc->iuspin);
+ spin_unlock_irqrestore(&desc->iuspin, flags);
if (rv < 0) {
clear_bit(WDM_RESPONDING, &desc->flags);
if (rv == -EPERM)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 32/50] wireless: ath: carl9170: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Christian Lamparter
Cc: "John W. Linville"
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/net/wireless/ath/carl9170/rx.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/rx.c
b/drivers/net/wireless/ath/carl9170/rx.c
index 4684dd9..61f62a6 100644
--- a/drivers/net/wireless/ath/carl9170/rx.c
+++ b/drivers/net/wireless/ath/carl9170/rx.c
@@ -129,6 +129,7 @@ static int carl9170_check_sequence(struct ar9170 *ar,
unsigned int seq)
static void carl9170_cmd_callback(struct ar9170 *ar, u32 len, void *buffer)
{
+ unsigned long flags;
/*
* Some commands may have a variable response length
* and we cannot predict the correct length in advance.
@@ -148,7 +149,7 @@ static void carl9170_cmd_callback(struct ar9170 *ar, u32
len, void *buffer)
carl9170_restart(ar, CARL9170_RR_INVALID_RSP);
}
- spin_lock(&ar->cmd_lock);
+ spin_lock_irqsave(&ar->cmd_lock, flags);
if (ar->readbuf) {
if (len >= 4)
memcpy(ar->readbuf, buffer + 4, len - 4);
@@ -156,7 +157,7 @@ static void carl9170_cmd_callback(struct ar9170 *ar, u32
len, void *buffer)
ar->readbuf = NULL;
}
complete(&ar->cmd_wait);
- spin_unlock(&ar->cmd_lock);
+ spin_unlock_irqrestore(&ar->cmd_lock, flags);
}
void carl9170_handle_command_response(struct ar9170 *ar, void *buf, u32 len)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/50] USB: iowarrior: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei
---
drivers/usb/misc/iowarrior.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index d36f34e..010ed6d 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -162,6 +162,7 @@ static void iowarrior_callback(struct urb *urb)
int offset;
int status = urb->status;
int retval;
+ unsigned long flags;
switch (status) {
case 0:
@@ -175,7 +176,7 @@ static void iowarrior_callback(struct urb *urb)
goto exit;
}
- spin_lock(&dev->intr_idx_lock);
+ spin_lock_irqsave(&dev->intr_idx_lock, flags);
intr_idx = atomic_read(&dev->intr_idx);
/* aux_idx become previous intr_idx */
aux_idx = (intr_idx == 0) ? (MAX_INTERRUPT_BUFFER - 1) : (intr_idx - 1);
@@ -211,7 +212,7 @@ static void iowarrior_callback(struct urb *urb)
*(dev->read_queue + offset + (dev->report_size)) = dev->serial_number++;
atomic_set(&dev->intr_idx, aux_idx);
- spin_unlock(&dev->intr_idx_lock);
+ spin_unlock_irqrestore(&dev->intr_idx_lock, flags);
/* tell the blocking read about the new data */
wake_up_interruptible(&dev->read_wait);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 39/50] media: usb: tm6000: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/media/usb/tm6000/tm6000-video.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/tm6000/tm6000-video.c
b/drivers/media/usb/tm6000/tm6000-video.c
index cc1aa14..8bb440f 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -434,6 +434,7 @@ static void tm6000_irq_callback(struct urb *urb)
struct tm6000_dmaqueue *dma_q = urb->context;
struct tm6000_core *dev = container_of(dma_q, struct tm6000_core, vidq);
int i;
+ unsigned long flags;
switch (urb->status) {
case 0:
@@ -450,9 +451,9 @@ static void tm6000_irq_callback(struct urb *urb)
break;
}
- spin_lock(&dev->slock);
+ spin_lock_irqsave(&dev->slock, flags);
tm6000_isoc_copy(urb);
- spin_unlock(&dev->slock);
+ spin_unlock_irqrestore(&dev->slock, flags);
/* Reset urb buffers */
for (i = 0; i < urb->number_of_packets; i++) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 49/50] staging: ced1401: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: de...@driverdev.osuosl.org
Signed-off-by: Ming Lei
---
drivers/staging/ced1401/usb1401.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/ced1401/usb1401.c
b/drivers/staging/ced1401/usb1401.c
index 97c55f9..70d2f43 100644
--- a/drivers/staging/ced1401/usb1401.c
+++ b/drivers/staging/ced1401/usb1401.c
@@ -265,6 +265,7 @@ static void ced_writechar_callback(struct urb *pUrb)
{
DEVICE_EXTENSION *pdx = pUrb->context;
int nGot = pUrb->actual_length; /* what we transferred */
+ unsigned long flags;
if (pUrb->status) { /* sync/async unlink faults aren't errors */
if (!
@@ -275,24 +276,24 @@ static void ced_writechar_callback(struct urb *pUrb)
__func__, pUrb->status);
}
- spin_lock(&pdx->err_lock);
+ spin_lock_irqsave(&pdx->err_lock, flags);
pdx->errors = pUrb->status;
- spin_unlock(&pdx->err_lock);
+ spin_unlock_irqrestore(&pdx->err_lock, flags);
nGot = 0; /* and tidy up again if so */
- spin_lock(&pdx->charOutLock); /* already at irq level */
+ spin_lock_irqsave(&pdx->charOutLock, flags);/* already at
irq level */
pdx->dwOutBuffGet = 0; /* Reset the output buffer */
pdx->dwOutBuffPut = 0;
pdx->dwNumOutput = 0; /* Clear the char count */
pdx->bPipeError[0] = 1; /* Flag an error for later */
pdx->bSendCharsPending = false; /* Allow other threads again */
- spin_unlock(&pdx->charOutLock); /* already at irq level */
+ spin_unlock_irqrestore(&pdx->charOutLock, flags); /*
already at irq level */
dev_dbg(&pdx->interface->dev,
"%s - char out done, 0 chars sent", __func__);
} else {
dev_dbg(&pdx->interface->dev,
"%s - char out done, %d chars sent", __func__, nGot);
- spin_lock(&pdx->charOutLock); /* already at irq level */
+ spin_lock_irqsave(&pdx->charOutLock, flags);/* already at
irq level */
pdx->dwNumOutput -= nGot; /* Now adjust the char send
buffer */
pdx->dwOutBuffGet += nGot; /* to match what we did */
if (pdx->dwOutBuffGet >= OUTBUF_SZ) /* Can't do this any
earlier as data could be overwritten */
@@ -305,7 +306,7 @@ static void ced_writechar_callback(struct urb *pUrb)
unsigned int dwCount = pdx->dwNumOutput;/*
maximum to send */
if ((pdx->dwOutBuffGet + dwCount) > OUTBUF_SZ) /*
does it cross buffer end? */
dwCount = OUTBUF_SZ - pdx->dwOutBuffGet;
- spin_unlock(&pdx->charOutLock); /* we are done with
stuff that changes */
+ spin_unlock_irqrestore(&pdx->charOutLock, flags);
/* we are done with stuff that changes */
memcpy(pdx->pCoherCharOut, pDat, dwCount); /*
copy output data to the buffer */
usb_fill_bulk_urb(pdx->pUrbCharOut, pdx->udev,
usb_sndbulkpipe(pdx->udev,
@@ -318,7 +319,7 @@ static void ced_writechar_callback(struct urb *pUrb)
iReturn = usb_submit_urb(pdx->pUrbCharOut, GFP_ATOMIC);
dev_dbg(&pdx->interface->dev, "%s n=%d>%s<", __func__,
dwCount, pDat);
- spin_lock(&pdx->charOutLock); /* grab lock for
errors */
+ spin_lock_irqsave(&pdx->charOutLock, flags);/*
grab lock for errors */
if (iReturn) {
pdx->bPipeError[nPipe] = 1; /* Flag an
error to be handled later */
pdx->bSendCharsPending = false; /* Allow other
threads again */
@@ -329,7 +330,7 @@ static void ced_writechar_callback(struct urb *pUrb)
}
} else
pdx->bSendCharsPending = false; /* Allow other threads
again */
- spin_unlock(&pdx->charOutLock); /* already at irq level */
+ spin_unlock_irqrestore(&pdx->charOutLock, flags); /*
already at irq level */
}
}
@@ -505,8 +506,9 @@ static void staged_callback(struct urb *pUrb)
unsigned int nGot = pUrb->actual_length;/* what we transferred
*/
bool bCancel = false;
bool bRestartCharInput; /* used at the end */
+ unsigned long flags;
- spin_lock(&pdx->stagedLock);/* stop ReadWriteMem() action while
this routine is running */
+ spin_lock_irqsave(&pdx->stagedLock, flag
[PATCH 43/50] sound: usb: midi: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: Clemens Ladisch
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei
---
sound/usb/midi.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index b901f46..86af276 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -279,15 +279,16 @@ static void snd_usbmidi_out_urb_complete(struct urb* urb)
struct out_urb_context *context = urb->context;
struct snd_usb_midi_out_endpoint* ep = context->ep;
unsigned int urb_index;
+ unsigned long flags;
- spin_lock(&ep->buffer_lock);
+ spin_lock_irqsave(&ep->buffer_lock, flags);
urb_index = context - ep->urbs;
ep->active_urbs &= ~(1 << urb_index);
if (unlikely(ep->drain_urbs)) {
ep->drain_urbs &= ~(1 << urb_index);
wake_up(&ep->drain_wait);
}
- spin_unlock(&ep->buffer_lock);
+ spin_unlock_irqrestore(&ep->buffer_lock, flags);
if (urb->status < 0) {
int err = snd_usbmidi_urb_error(urb->status);
if (err < 0) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] ux500 dma & short transfers on MUSB
Hello, following scenario: you attach an UART-TO-USB adapter to your musb port running ux500-dma code. The USB UARt driver queues 1x RX URB with the size of 256 bytes (example) and the max packet size is 64. The other side sends only one byte because it is mean. Now, the way I understand it is, you tell musb that the complete transfer of 256 bytes has ended instead one byte that really happened. Is my assumption wrong? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 29/50] USBNET: rtl8150: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: net...@vger.kernel.org Signed-off-by: Ming Lei --- drivers/net/usb/rtl8150.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c index 6cbdac6..199e0fb 100644 --- a/drivers/net/usb/rtl8150.c +++ b/drivers/net/usb/rtl8150.c @@ -372,6 +372,7 @@ static void read_bulk_callback(struct urb *urb) u16 rx_stat; int status = urb->status; int result; + unsigned long flags; dev = urb->context; if (!dev) @@ -413,9 +414,9 @@ static void read_bulk_callback(struct urb *urb) netdev->stats.rx_packets++; netdev->stats.rx_bytes += pkt_len; - spin_lock(&dev->rx_pool_lock); + spin_lock_irqsave(&dev->rx_pool_lock, flags); skb = pull_skb(dev); - spin_unlock(&dev->rx_pool_lock); + spin_unlock_irqrestore(&dev->rx_pool_lock, flags); if (!skb) goto resched; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 36/50] media: usb: em28xx: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/media/usb/em28xx/em28xx-core.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-core.c
b/drivers/media/usb/em28xx/em28xx-core.c
index fc157af..0d698f9 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -941,6 +941,7 @@ static void em28xx_irq_callback(struct urb *urb)
{
struct em28xx *dev = urb->context;
int i;
+ unsigned long flags;
switch (urb->status) {
case 0: /* success */
@@ -956,9 +957,9 @@ static void em28xx_irq_callback(struct urb *urb)
}
/* Copy data from URB */
- spin_lock(&dev->slock);
+ spin_lock_irqsave(&dev->slock, flags);
dev->usb_ctl.urb_data_copy(dev, urb);
- spin_unlock(&dev->slock);
+ spin_unlock_irqrestore(&dev->slock, flags);
/* Reset urb buffers */
for (i = 0; i < urb->number_of_packets; i++) {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 31/50] wireless: zd1211rw: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Daniel Drake
Cc: Ulrich Kunitz
Cc: "John W. Linville"
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/net/wireless/zd1211rw/zd_usb.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/zd1211rw/zd_usb.c
b/drivers/net/wireless/zd1211rw/zd_usb.c
index 7ef0b4a..8169ee0 100644
--- a/drivers/net/wireless/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zd1211rw/zd_usb.c
@@ -372,14 +372,15 @@ static inline void handle_regs_int_override(struct urb
*urb)
{
struct zd_usb *usb = urb->context;
struct zd_usb_interrupt *intr = &usb->intr;
+ unsigned long flags;
- spin_lock(&intr->lock);
+ spin_lock_irqsave(&intr->lock, flags);
if (atomic_read(&intr->read_regs_enabled)) {
atomic_set(&intr->read_regs_enabled, 0);
intr->read_regs_int_overridden = 1;
complete(&intr->read_regs.completion);
}
- spin_unlock(&intr->lock);
+ spin_unlock_irqrestore(&intr->lock, flags);
}
static inline void handle_regs_int(struct urb *urb)
@@ -388,9 +389,10 @@ static inline void handle_regs_int(struct urb *urb)
struct zd_usb_interrupt *intr = &usb->intr;
int len;
u16 int_num;
+ unsigned long flags;
ZD_ASSERT(in_interrupt());
- spin_lock(&intr->lock);
+ spin_lock_irqsave(&intr->lock, flags);
int_num = le16_to_cpu(*(__le16 *)(urb->transfer_buffer+2));
if (int_num == CR_INTERRUPT) {
@@ -426,7 +428,7 @@ static inline void handle_regs_int(struct urb *urb)
}
out:
- spin_unlock(&intr->lock);
+ spin_unlock_irqrestore(&intr->lock, flags);
/* CR_INTERRUPT might override read_reg too. */
if (int_num == CR_INTERRUPT && atomic_read(&intr->read_regs_enabled))
@@ -666,6 +668,7 @@ static void rx_urb_complete(struct urb *urb)
struct zd_usb_rx *rx;
const u8 *buffer;
unsigned int length;
+ unsigned long flags;
switch (urb->status) {
case 0:
@@ -694,14 +697,14 @@ static void rx_urb_complete(struct urb *urb)
/* If there is an old first fragment, we don't care. */
dev_dbg_f(urb_dev(urb), "*** first fragment ***\n");
ZD_ASSERT(length <= ARRAY_SIZE(rx->fragment));
- spin_lock(&rx->lock);
+ spin_lock_irqsave(&rx->lock, flags);
memcpy(rx->fragment, buffer, length);
rx->fragment_length = length;
- spin_unlock(&rx->lock);
+ spin_unlock_irqrestore(&rx->lock, flags);
goto resubmit;
}
- spin_lock(&rx->lock);
+ spin_lock_irqsave(&rx->lock, flags);
if (rx->fragment_length > 0) {
/* We are on a second fragment, we believe */
ZD_ASSERT(length + rx->fragment_length <=
@@ -711,9 +714,9 @@ static void rx_urb_complete(struct urb *urb)
handle_rx_packet(usb, rx->fragment,
rx->fragment_length + length);
rx->fragment_length = 0;
- spin_unlock(&rx->lock);
+ spin_unlock_irqrestore(&rx->lock, flags);
} else {
- spin_unlock(&rx->lock);
+ spin_unlock_irqrestore(&rx->lock, flags);
handle_rx_packet(usb, buffer, length);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 37/50] media: usb: sn9x102: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/media/usb/sn9c102/sn9c102_core.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/sn9c102/sn9c102_core.c
b/drivers/media/usb/sn9c102/sn9c102_core.c
index 2cb44de..33dc595 100644
--- a/drivers/media/usb/sn9c102/sn9c102_core.c
+++ b/drivers/media/usb/sn9c102/sn9c102_core.c
@@ -784,12 +784,14 @@ end_of_frame:
cam->sensor.pix_format.pixelformat ==
V4L2_PIX_FMT_JPEG) && eof)) {
u32 b;
+ unsigned long flags;
b = (*f)->buf.bytesused;
(*f)->state = F_DONE;
(*f)->buf.sequence= ++cam->frame_count;
- spin_lock(&cam->queue_lock);
+ spin_lock_irqsave(&cam->queue_lock,
+ flags);
list_move_tail(&(*f)->frame,
&cam->outqueue);
if (!list_empty(&cam->inqueue))
@@ -799,7 +801,8 @@ end_of_frame:
frame );
else
(*f) = NULL;
- spin_unlock(&cam->queue_lock);
+ spin_unlock_irqrestore(&cam->queue_lock,
+ flags);
memcpy(cam->sysfs.frame_header,
cam->sof.header, soflen);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 47/50] staging: btmtk_usb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: de...@driverdev.osuosl.org
Signed-off-by: Ming Lei
---
drivers/staging/btmtk_usb/btmtk_usb.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c
b/drivers/staging/btmtk_usb/btmtk_usb.c
index 0e783e8..ea10d4f 100644
--- a/drivers/staging/btmtk_usb/btmtk_usb.c
+++ b/drivers/staging/btmtk_usb/btmtk_usb.c
@@ -1218,6 +1218,7 @@ static void btmtk_usb_tx_complete(struct urb *urb)
struct sk_buff *skb = urb->context;
struct hci_dev *hdev = (struct hci_dev *)skb->dev;
struct btmtk_usb_data *data = hci_get_drvdata(hdev);
+ unsigned long flags;
BT_DBG("%s: %s urb %p status %d count %d\n", __func__, hdev->name,
urb, urb->status, urb->actual_length);
@@ -1231,9 +1232,9 @@ static void btmtk_usb_tx_complete(struct urb *urb)
hdev->stat.err_tx++;
done:
- spin_lock(&data->txlock);
+ spin_lock_irqsave(&data->txlock, flags);
data->tx_in_flight--;
- spin_unlock(&data->txlock);
+ spin_unlock_irqrestore(&data->txlock, flags);
kfree(urb->setup_packet);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 22/50] BT: btusb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Marcel Holtmann
Cc: Gustavo Padovan
Cc: Johan Hedberg
Cc: linux-blueto...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/bluetooth/btusb.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ea63958..018b8b0 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -573,6 +573,7 @@ static void btusb_tx_complete(struct urb *urb)
struct sk_buff *skb = urb->context;
struct hci_dev *hdev = (struct hci_dev *) skb->dev;
struct btusb_data *data = hci_get_drvdata(hdev);
+ unsigned long flags;
BT_DBG("%s urb %p status %d count %d", hdev->name,
urb, urb->status, urb->actual_length);
@@ -586,9 +587,9 @@ static void btusb_tx_complete(struct urb *urb)
hdev->stat.err_tx++;
done:
- spin_lock(&data->txlock);
+ spin_lock_irqsave(&data->txlock, flags);
data->tx_in_flight--;
- spin_unlock(&data->txlock);
+ spin_unlock_irqrestore(&data->txlock, flags);
kfree(urb->setup_packet);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 23/50] BT: bfusb: read_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
read_lock_irqsave().
Cc: Marcel Holtmann
Cc: Gustavo Padovan
Cc: Johan Hedberg
Cc: linux-blueto...@vger.kernel.org
Signed-off-by: Ming Lei
---
drivers/bluetooth/bfusb.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 995aee9..2e93501 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -186,6 +186,7 @@ static void bfusb_tx_complete(struct urb *urb)
{
struct sk_buff *skb = (struct sk_buff *) urb->context;
struct bfusb_data *data = (struct bfusb_data *) skb->dev;
+ unsigned long flags;
BT_DBG("bfusb %p urb %p skb %p len %d", data, urb, skb, skb->len);
@@ -199,14 +200,14 @@ static void bfusb_tx_complete(struct urb *urb)
else
data->hdev->stat.err_tx++;
- read_lock(&data->lock);
+ read_lock_irqsave(&data->lock, flags);
skb_unlink(skb, &data->pending_q);
skb_queue_tail(&data->completed_q, skb);
bfusb_tx_wakeup(data);
- read_unlock(&data->lock);
+ read_unlock_irqrestore(&data->lock, flags);
}
@@ -347,10 +348,11 @@ static void bfusb_rx_complete(struct urb *urb)
unsigned char *buf = urb->transfer_buffer;
int count = urb->actual_length;
int err, hdr, len;
+ unsigned long flags;
BT_DBG("bfusb %p urb %p skb %p len %d", data, urb, skb, skb->len);
- read_lock(&data->lock);
+ read_lock_irqsave(&data->lock, flags);
if (!test_bit(HCI_RUNNING, &data->hdev->flags))
goto unlock;
@@ -392,7 +394,7 @@ static void bfusb_rx_complete(struct urb *urb)
bfusb_rx_submit(data, urb);
- read_unlock(&data->lock);
+ read_unlock_irqrestore(&data->lock, flags);
return;
@@ -406,7 +408,7 @@ resubmit:
}
unlock:
- read_unlock(&data->lock);
+ read_unlock_irqrestore(&data->lock, flags);
}
static int bfusb_open(struct hci_dev *hdev)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
