On Friday 13 July 2007, Yoshihiro Shimoda wrote:
quoted text
> David Brownell wrote:
> > On Thursday 12 July 2007, Greg KH wrote:
> >> On Thu, Jul 12, 2007 at 05:23:41PM -0700, David Brownell wrote:
> > 
> >>> What I'll do is provide a patch to update this to the latest
> >>> version of this code which I have (as soon as Linus pulls),
> >>> and ask Renesas to expedite addressing the other review issues.
> >> Sounds good to me.
> > 
> > Here ... I just did a test build.  Powernow-k8 needs a patch too.
> 
> Thank you very much for your feedback.
> I tested in my development environment.
> When I tested "usbtest", there was the problem that
> NULL pointer access in transfer_complete().
> I made the following patch. I would appreciate it if you could
> check this patch.

I think I see what's going on.  Since requests passed into
usb_ep_queue() must have a completion handler, I thought your
original code -- testing for a null handler -- was wrong.

But I didn't notice that your internal GET_STATUS request
didn't have a completion handler.  Good thing I didn't add
a test for non-null completion in  m66592_queue()!

A better fix would be to supply a NOP completion handler
for that internal request.  That way there would be less risk
of code working on m66592-udc which would not work with other
controllers.

When looking at this, I observed a glitch in the locking
I added ... I'm assuming you still didn't run with locking
checks enabled, or you would have seen that issue.

Does the following patch behave, with all the locking test
options in the kernel debug menu enabled?

- Dave


======	CUT HERE
Fix more glitches in how status requests are handled.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/usb/gadget/m66592-udc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--- g26.orig/drivers/usb/gadget/m66592-udc.c	2007-07-13 10:44:10.000000000 -0700
+++ g26/drivers/usb/gadget/m66592-udc.c	2007-07-13 10:55:13.000000000 -0700
@@ -916,8 +916,10 @@ static void irq_pipe_empty(struct m66592
 }
 
 static void get_status(struct m66592 *m66592, struct usb_ctrlrequest *ctrl)
+__releases(m66592->lock)
+__acquires(m66592->lock)
 {
-	struct m66592_ep *ep;
+	struct m66592_ep *ep = NULL;
 	u16 pid;
 	u16 status = 0;
 
@@ -944,7 +946,9 @@ static void get_status(struct m66592 *m6
 	*m66592->ep0_buf = status;
 	m66592->ep0_req->buf = m66592->ep0_buf;
 	m66592->ep0_req->length = 2;
+	spin_unlock(&ep->m66592->lock);
 	m66592_queue(m66592->gadget.ep0, m66592->ep0_req, GFP_KERNEL);
+	spin_lock(&ep->m66592->lock);
 }
 
 static void clear_feature(struct m66592 *m66592, struct usb_ctrlrequest *ctrl)
@@ -1480,6 +1484,10 @@ static int __exit m66592_remove(struct p
 	return 0;
 }
 
+static void nop_completion(struct usb_ep *ep, struct usb_request *r)
+{
+}
+
 #define resource_len(r) (((r)->end - (r)->start) + 1)
 
 static int __init m66592_probe(struct platform_device *pdev)
@@ -1578,6 +1586,7 @@ static int __init m66592_probe(struct pl
 	m66592->ep0_req = m66592_alloc_request(&m66592->ep[0].ep, GFP_KERNEL);
 	if (m66592->ep0_req == NULL)
 		goto clean_up;
+	m66592->ep0_req->complete = nop_completion;
 	m66592->ep0_buf = kmalloc(2, GFP_KERNEL);
 	if (m66592->ep0_buf == NULL)
 		goto clean_up;

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel