[PATCH] USB: usb-skeleton.c: fix blocked forever in skel_read
From: Du Xing duxing2...@gmail.com
In skel_read,the reader blocked in wait_for_completion before submit bulk in
urb.
Using processed_urb is totally unnecessary, remove it can fix the bug.
---
Signed-off-by: Du Xing duxing2...@gmail.com
---
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index ce31017..14e4889 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -61,7 +61,6 @@ struct usb_skel {
__u8 bulk_out_endpointAddr; /* the address of the bulk out endpoint */
int errors; /* the last request tanked */
bool ongoing_read; /* a read is going on */
- bool processed_urb; /* indicates we haven't processed the urb */
spinlock_t err_lock; /* lock for errors */
struct kref kref;
struct mutex io_mutex; /* synchronize I/O with disconnect */
@@ -269,17 +268,6 @@ retry:
* we must finish now
*/
dev-bulk_in_copied = 0;
- dev-processed_urb = 1;
- }
-
- if (!dev-processed_urb) {
- /*
- * the URB hasn't been processed
- * do it now
- */
- wait_for_completion(dev-bulk_in_completion);
- dev-bulk_in_copied = 0;
- dev-processed_urb = 1;
}
/* errors must be reported */
Re: Testing for hardware bug in EHCI controllers
On Sat, 2 Mar 2013, Clemens Ladisch wrote: At any rate, if you're interested in finding out what the problem might be, the best place to start would be with a usbmon trace. ehci-test: 8801e96c06c0 2335244662 S Bo:1:018:1 -115 31 = 55534243 6500 0400 0a28 0200 00 usb-storage: 88021037c480 3386301775 S Bo:1:021:1 -115 31 = 55534243 5400 0010 8a28 ef4f 3e08 00 With the patch below, it runs just fine. (And doesn't find EHCI bugs.) Once again, usbmon proves its worth. Thanks for the fix; it is obviously correct. --- ehci-test.c +++ ehci-test.c @@ -192,7 +192,7 @@ int send_READ10(void) (NUM_BLOCKS 1) 0xff, (NUM_BLOCKS 7) 0xff, (NUM_BLOCKS 15) 0xff, - 0, 0, 10, /* Flags, LUN, Length of CDB */ + 0x80, 0, 10,/* Flags, LUN, Length of CDB */ 0x28, 0,/* CDB: READ(10), LUN 0 */ 0, 0, 0, 0, /* LBA = 0 */ 0, /* Reserved */ 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] drivers/usb/gadget: using strlcpy instead of strncpy
Hi Felipe, On Sunday 03 March 2013 01:23:46 Felipe Balbi wrote: On Fri, Mar 01, 2013 at 08:47:34PM +0100, Laurent Pinchart wrote: On Wednesday 27 February 2013 10:26:23 Felipe Balbi wrote: On Sat, Feb 02, 2013 at 03:48:54PM +0800, Chen Gang wrote: for NUL terminated string, better notice '\0' in the end. Signed-off-by: Chen Gang gang.c...@asianux.com Laurent, are you taking this patch or should I ? I've taken the patch in my tree. I've just sent a consolidated series of most pending UVC gadget patches to the list, and I will send you a pull request as soon as I receive a Tested-by. I'll take them as patches. No problem. Bhupesh wrote that he would test the patches this week. Hopefully there will be no further issue. -- Regards, Laurent Pinchart signature.asc Description: This is a digitally signed message part.
Re: [PATCH 3/3] usb: Make sure each c67x00 TD has been executed
Dave == Dave Tubbs dave.tu...@portalislc.com writes: Dave From: Dave Tubbs dave.tu...@portalislc.com Dave Make sure each c67x00 TD has been executed or retry using the existing Dave retry mechanism. Reference Cypress Semiconductor BIOS User's Manual 1.2, Dave page 3-16 Dave Signed-off-by: Dave Tubbs dave.tu...@portalislc.com Acked-by: Peter Korsgaard jac...@sunsite.dk Dave --- Dave drivers/usb/c67x00/c67x00-sched.c |6 ++ Dave 1 files changed, 6 insertions(+), 0 deletions(-) Dave diff --git a/drivers/usb/c67x00/c67x00-sched.c b/drivers/usb/c67x00/c67x00-sched.c Dave index aa49162..dd5bdb4 100644 Dave --- a/drivers/usb/c67x00/c67x00-sched.c Dave +++ b/drivers/usb/c67x00/c67x00-sched.c Dave @@ -1033,6 +1033,12 @@ static inline void c67x00_check_td_list(struct c67x00_hcd *c67x00) Dave !td_acked(td)) Dave goto cont; Dave +/* Dave + * at this point, there are no errors, but it's still possible Dave + * that the td wasn't executed by the c67x00. Confirm it was Dave + * executed or force a retry Dave + */ Dave +if ((td-retry_cnt TD_RETRYCNTMASK_ACT_FLG) Dave +td-status == 0) Dave +goto cont; Dave + Dave /* Sequence ok and acked, don't need to fix toggle */ Dave ack_ok = 1; Dave -- Dave 1.7.1 -- Bye, Peter Korsgaard -- 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 0/5] usb: musb: am335x support
Hi Peter,
On 03.03.2013 23:24, Peter Korsgaard wrote:
Daniel == Daniel Mack zon...@gmail.com writes:
Hi,
Daniel On my board, the USB is purely used as host interface, with a
Daniel type B plug soldered. In the DT, I'm using the following
Daniel sniplet in accordance to the documentation of the bindings:
Daniel usb_otg_hs: usb@4740 {
Daniel compatible = ti,musb-am33xx;
Daniel reg = 0x4740 0x1000/* usbss */
Daniel 0x47401000 0x800 /* musb instance 0 */
Daniel 0x47401800 0x800; /* musb instance 1 */
Daniel interrupt-parent = intc;
Daniel interrupts = 17/* usbss */
Daniel18/* musb instance 0 */
19 ;/* musb instance 1 */
Daniel multipoint = 1;
Daniel num-eps = 16;
Daniel ram-bits = 12;
Daniel port0-mode = 3;
Daniel port1-mode = 3;
Daniel power = 250;
Daniel ti,hwmods = usb_otg_hs;
Daniel };
Daniel The relevant config options are
Daniel CONFIG_USB_MUSB_HDRC=y
Daniel # CONFIG_USB_MUSB_TUSB6010 is not set
Daniel # CONFIG_USB_MUSB_OMAP2PLUS is not set
Daniel # CONFIG_USB_MUSB_AM35X is not set
Daniel CONFIG_USB_MUSB_DSPS=y
Daniel CONFIG_MUSB_PIO_ONLY=y
Daniel CONFIG_USB_GADGET=y
Daniel CONFIG_USB_GADGET_DEBUG=y
Daniel CONFIG_USB_GADGET_DEBUG_FILES=y
Daniel CONFIG_USB_GADGET_DEBUG_FS=y
Daniel CONFIG_USB_GADGET_VBUS_DRAW=2
Daniel CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS=2
Daniel CONFIG_USB_GADGET_MUSB_HDRC=y
Ehh, I'm confused here. You talk about host mode, a single 'B' (gadget)
connector
I'm sorry for the confusion caused. This is a stupid mistake - I'm in
fact talking about an 'A' type plug of course (host mode).
and your device tree mentions 2 OTG ports.
That's what I said in a follow up - the same happens for mode '1'.
What is the configuration exactly?
Again to summarize: host-only mode, type 'A' plug.
The gadget driver is is enabled in the config because the only
occurrence of usb_add_hcd() in the musb driver is in the gagdget part,
so supposedly, there has to be a gadget driver loaded in order to make
the host interface going.
Thanks,
Daniel
--
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: Not enough resource for old configuration after USB bus reset
Hi, I try two experiment, 1. Test it on general desktop, the device will enum as CDC_ECM( 1 control, 2 bulk, 1 interrupt), and CDC_ECM driver will use interupt to polling link status. 2. Test it on embedded system, the device will controlled by a custom driver ( 1 control, 2 bulk, 1 interrupt). The configure endpoint will allocate bandwith for interrupt endpoint. But the driver nevery use interrupt endpoint. So you had your system set up like described in #2, and you still ran out of bandwidth after a lot of device resets? The host on #2 is as the same as #1. The major difference is the usb interface driver, one is customize driver(#2), another one is CDC_ECM from gadget(#1). Best regards, Soar -- 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 v2 1/5] usb: chipidea: udc: move _ep_queue into an unlocked function
On Fri, Mar 01, 2013 at 03:42:23PM +0100, Michael Grzeschik wrote:
There is no need to call ep_queue unlocked inside the own driver. We
move its functionionality into an unlocked version.
This patch removes potential unlocked timeslots inside
isr_setup_status_phase and isr_get_status_response, in which the lock
got released just before acquired again inside usb_ep_queue.
You mean before or after?
For other parts, it is ok, you can add:
Reviewed-by: Peter Chen peter.c...@freescale.com
Signed-off-by: Michael Grzeschik m.grzesc...@pengutronix.de
Signed-off-by: Marc Kleine-Budde m...@pengutronix.de
---
Changes since v1:
- changed patch description as mentioned by alexander
drivers/usb/chipidea/udc.c | 113
1 file changed, 63 insertions(+), 50 deletions(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 2f45bba..22d37d8 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -668,6 +668,66 @@ static void isr_get_status_complete(struct usb_ep *ep,
struct usb_request *req)
}
/**
+ * _ep_queue: queues (submits) an I/O request to an endpoint
+ *
+ * Caller must hold lock
+ */
+static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
+ gfp_t __maybe_unused gfp_flags)
+{
+ struct ci13xxx_ep *mEp = container_of(ep, struct ci13xxx_ep, ep);
+ struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
+ struct ci13xxx *ci = mEp-ci;
+ int retval = 0;
+
+ if (ep == NULL || req == NULL || mEp-ep.desc == NULL)
+ return -EINVAL;
+
+ if (mEp-type == USB_ENDPOINT_XFER_CONTROL) {
+ if (req-length)
+ mEp = (ci-ep0_dir == RX) ?
+ci-ep0out : ci-ep0in;
+ if (!list_empty(mEp-qh.queue)) {
+ _ep_nuke(mEp);
+ retval = -EOVERFLOW;
+ dev_warn(mEp-ci-dev, endpoint ctrl %X nuked\n,
+ _usb_addr(mEp));
+ }
+ }
+
+ /* first nuke then test link, e.g. previous status has not sent */
+ if (!list_empty(mReq-queue)) {
+ retval = -EBUSY;
+ dev_err(mEp-ci-dev, request already in queue\n);
+ goto done;
+ }
+
+ if (req-length 4 * CI13XXX_PAGE_SIZE) {
+ req-length = 4 * CI13XXX_PAGE_SIZE;
+ retval = -EMSGSIZE;
+ dev_warn(mEp-ci-dev, request length truncated\n);
+ }
+
+ dbg_queue(_usb_addr(mEp), req, retval);
+
+ /* push request */
+ mReq-req.status = -EINPROGRESS;
+ mReq-req.actual = 0;
+
+ retval = _hardware_enqueue(mEp, mReq);
+
+ if (retval == -EALREADY) {
+ dbg_event(_usb_addr(mEp), QUEUE, retval);
+ retval = 0;
+ }
+ if (!retval)
+ list_add_tail(mReq-queue, mEp-qh.queue);
+
+ done:
+ return retval;
+}
+
+/**
* isr_get_status_response: get_status request response
* @ci: ci struct
* @setup: setup request packet
@@ -714,9 +774,7 @@ __acquires(mEp-lock)
}
/* else do nothing; reserved for future use */
- spin_unlock(mEp-lock);
- retval = usb_ep_queue(mEp-ep, req, gfp_flags);
- spin_lock(mEp-lock);
+ retval = _ep_queue(mEp-ep, req, gfp_flags);
if (retval)
goto err_free_buf;
@@ -763,8 +821,6 @@ isr_setup_status_complete(struct usb_ep *ep, struct
usb_request *req)
* This function returns an error code
*/
static int isr_setup_status_phase(struct ci13xxx *ci)
-__releases(mEp-lock)
-__acquires(mEp-lock)
{
int retval;
struct ci13xxx_ep *mEp;
@@ -773,9 +829,7 @@ __acquires(mEp-lock)
ci-status-context = ci;
ci-status-complete = isr_setup_status_complete;
- spin_unlock(mEp-lock);
- retval = usb_ep_queue(mEp-ep, ci-status, GFP_ATOMIC);
- spin_lock(mEp-lock);
+ retval = _ep_queue(mEp-ep, ci-status, GFP_ATOMIC);
return retval;
}
@@ -1172,8 +1226,6 @@ static int ep_queue(struct usb_ep *ep, struct
usb_request *req,
gfp_t __maybe_unused gfp_flags)
{
struct ci13xxx_ep *mEp = container_of(ep, struct ci13xxx_ep, ep);
- struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
- struct ci13xxx *ci = mEp-ci;
int retval = 0;
unsigned long flags;
@@ -1182,47 +1234,8 @@ static int ep_queue(struct usb_ep *ep, struct
usb_request *req,
spin_lock_irqsave(mEp-lock, flags);
- if (mEp-type == USB_ENDPOINT_XFER_CONTROL) {
- if (req-length)
- mEp = (ci-ep0_dir == RX) ?
-ci-ep0out : ci-ep0in;
- if (!list_empty(mEp-qh.queue)) {
- _ep_nuke(mEp);
- retval = -EOVERFLOW;
- dev_warn(mEp-ci-dev, endpoint ctrl %X
Re: [PATCH v2 2/5] usb: chipidea: udc: configure iso endpoints
On Fri, Mar 01, 2013 at 03:42:24PM +0100, Michael Grzeschik wrote:
The implementation is derived from the fsl_udc_core code in
fsl_ep_enable and makes basic iso handling possible.
Signed-off-by: Michael Grzeschik m.grzesc...@pengutronix.de
Signed-off-by: Marc Kleine-Budde m...@pengutronix.de
---
Changes since v1:
- fixed coding style issues mentioned by Sergei
drivers/usb/chipidea/udc.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 22d37d8..45cce45 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1076,6 +1076,9 @@ static int ep_enable(struct usb_ep *ep,
int retval = 0;
unsigned long flags;
+ unsigned short max;
+ unsigned char mult = 0;
+
if (ep == NULL || desc == NULL)
return -EINVAL;
@@ -1093,6 +1096,7 @@ static int ep_enable(struct usb_ep *ep,
mEp-type = usb_endpoint_type(desc);
mEp-ep.maxpacket = usb_endpoint_maxp(desc);
+ max = usb_endpoint_maxp(desc);
Why not using mEp-ep.maxpacket directly?
dbg_event(_usb_addr(mEp), ENABLE, 0);
@@ -1100,8 +1104,12 @@ static int ep_enable(struct usb_ep *ep,
if (mEp-type == USB_ENDPOINT_XFER_CONTROL)
mEp-qh.ptr-cap |= QH_IOS;
- else if (mEp-type == USB_ENDPOINT_XFER_ISOC)
- mEp-qh.ptr-cap = ~QH_MULT;
+ else if (mEp-type == USB_ENDPOINT_XFER_ISOC) {
+ /* Calculate transactions needed for high bandwidth iso */
+ mult = (unsigned char)(1 + ((max 11) 0x03));
Better to use MACRO, not use number directly.
+ max = 0x7ff; /* bit 0~10 */
Useless line?
+ mEp-qh.ptr-cap |= mult 30;
Better to use MACRO, not use number directly.
+ }
else
mEp-qh.ptr-cap = ~QH_ZLT;
Do you plan to port all ISO supports from fsl_core_udc.c?
--
1.7.10.4
--
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: Testing for hardware bug in EHCI controllers
On Tue, Feb 26, 2013 at 4:54 AM, Alan Stern st...@rowland.harvard.edu wrote: I'd be interested to hear the results of testing on a variety of controllers. (This computer also has an NEC EHCI controller, and that one does not have the bug.) Do the EHCI controllers on current Intel chipsets pass the test? What about other vendors? No the 'EHCI hardware bug detected' error are observed on EHCI of OMAP4 pandaboard. 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 3/5] usb: chipidea: udc: add the define TD_COUNT and fix all users
On Fri, Mar 01, 2013 at 03:42:25PM +0100, Michael Grzeschik wrote:
A static count of transfer descriptors was used everywhere in the driver
with the fixed number 4. This patch adds a define, named TD_COUNT, and
replaces all users of this value. This way its possible to have only one
parameter to change and limit the amount of tds per transfer.
Signed-off-by: Michael Grzeschik m.grzesc...@pengutronix.de
Reviewed-by: Peter Chen peter.c...@freescale.com
---
drivers/usb/chipidea/ci.h |1 +
drivers/usb/chipidea/udc.c |6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index e25d126..1532f08 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -21,6 +21,7 @@
/**
* DEFINE
*/
+#define TD_COUNT 4
#define CI13XXX_PAGE_SIZE 4096ul /* page size for TD's */
#define ENDPT_MAX 32
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 45cce45..0b80228 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -457,7 +457,7 @@ static int _hardware_enqueue(struct ci13xxx_ep *mEp,
struct ci13xxx_req *mReq)
mReq-ptr-token |= TD_IOC;
}
mReq-ptr-page[0] = mReq-req.dma;
- for (i = 1; i 5; i++)
+ for (i = 1; i = TD_COUNT; i++)
mReq-ptr-page[i] =
(mReq-req.dma + i * CI13XXX_PAGE_SIZE)
~TD_RESERVED_MASK;
@@ -702,8 +702,8 @@ static int _ep_queue(struct usb_ep *ep, struct
usb_request *req,
goto done;
}
- if (req-length 4 * CI13XXX_PAGE_SIZE) {
- req-length = 4 * CI13XXX_PAGE_SIZE;
+ if (req-length TD_COUNT * CI13XXX_PAGE_SIZE) {
+ req-length = TD_COUNT * CI13XXX_PAGE_SIZE;
retval = -EMSGSIZE;
dev_warn(mEp-ci-dev, request length truncated\n);
}
--
1.7.10.4
--
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: Testing for hardware bug in EHCI controllers
Hi Alan, On 02/26/2013 04:54 AM, Alan Stern wrote: Sarah (and anyone else who's interested): A while ago I wrote about a hardware bug in my Intel ICH5 and ICH8 EHCI controllers. You pointed out that these are rather old components, not being used in current systems, which is quite true. I test this on Atmel at91sam9x5ek board with Linux-3.8. And get the similar information. So please indicate me more detail information about the bug. (Sorry for not catch the hardware bug e-mail) Thanks. Best Regards, Bo Shen -- 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: host: tegra: Reset Tegra USB controller before init
-Original Message- From: Stephen Warren [mailto:swar...@wwwdotorg.org] Sent: Thursday, February 28, 2013 11:47 PM To: Venu Byravarasu Cc: gre...@linuxfoundation.org; st...@rowland.harvard.edu; linux- u...@vger.kernel.org; linux-ker...@vger.kernel.org Subject: Re: [PATCH] usb: host: tegra: Reset Tegra USB controller before init On 02/27/2013 11:36 PM, Venu Byravarasu wrote: To clear any configurations made by U-Boot on Tegra USB controller, reset it before init in probe. diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c @@ -691,6 +692,10 @@ static int tegra_ehci_probe(struct platform_device *pdev) if (err) goto fail_clk; + tegra_periph_reset_assert(tegra-clk); + udelay(1); + tegra_periph_reset_deassert(tegra-clk); I think this patch might cause unintended consequences. When the Tegra PHY code is converted to a driver (i.e. has its own probe), the initial order of execution of the PHY and EHCI driver probes will not be guaranteed. In particular, since the EHCI probe will attempt to find the PHY device, and defer the EHCI probe until it can do so, this guarantees that the PHY's probe() will have completed before EHCI's probe() completes (although EHCI's probe may start running first some number of times, and be retried with -EPROBE_DEFERRED for a variety of reasons). Now, if the PHY driver's probe() actually touches HW and sets up some registers, isn't this reset call going to trash any of that register setup? Or, will PHY probe() not touch registers, but only do so during the standard PHY open/init op/API calls? Yes, PHY driver probe does not touch any registers. It just sets up the PHY API hooks. These APIs will be called from ehci-tegra.c as part of ehci tegra probe function, after getting PHY handle, which in turn happens after issuing above reset. Thanks to Stephen Alan, for the review comments. I think the way to solve this is to put the reset call into the PHY driver. I assume it has access to the appropriate clock object. -- 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
