Hi, On Tue, Feb 07, 2012 at 12:53:17PM -0800, Ido Shayevitz wrote: > > > Hi, > > > > On Tue, Feb 07, 2012 at 05:53:09PM +0200, Ido Shayevitz wrote: > >> Remove redundant pointers in all UDCs to struct usb_endpoint_descriptor. > >> This fix a bug in f_serial, which expect the ep->desc to be NULL after > >> disabling an endpoint. > >> > >> Signed-off-by: Ido Shayevitz <idos@xxxxxxxxxxxxxx> > >> --- > >> drivers/usb/dwc3/core.h | 1 - > >> drivers/usb/dwc3/ep0.c | 2 +- > >> drivers/usb/dwc3/gadget.c | 32 > >> +++++++++++++++--------------- > >> drivers/usb/gadget/amd5536udc.c | 14 ++++++------ > >> drivers/usb/gadget/amd5536udc.h | 1 - > >> drivers/usb/gadget/at91_udc.c | 16 +++++++------- > >> drivers/usb/gadget/at91_udc.h | 3 -- > >> drivers/usb/gadget/atmel_usba_udc.c | 25 ++++++++++++----------- > >> drivers/usb/gadget/atmel_usba_udc.h | 1 - > >> drivers/usb/gadget/ci13xxx_udc.c | 16 +++++++------- > >> drivers/usb/gadget/ci13xxx_udc.h | 1 - > >> drivers/usb/gadget/fsl_qe_udc.c | 20 +++++++++--------- > >> drivers/usb/gadget/fsl_qe_udc.h | 1 - > >> drivers/usb/gadget/fsl_udc_core.c | 26 ++++++++++++------------ > >> drivers/usb/gadget/fsl_usb2_udc.h | 1 - > >> drivers/usb/gadget/fusb300_udc.c | 4 +- > >> drivers/usb/gadget/fusb300_udc.h | 1 - > >> drivers/usb/gadget/goku_udc.c | 21 ++++++++++--------- > >> drivers/usb/gadget/goku_udc.h | 1 - > >> drivers/usb/gadget/langwell_udc.c | 36 > >> +++++++++++++++++----------------- > >> drivers/usb/gadget/langwell_udc.h | 1 - > >> drivers/usb/gadget/m66592-udc.c | 10 ++++---- > >> drivers/usb/gadget/m66592-udc.h | 2 +- > >> drivers/usb/gadget/mv_udc.h | 1 - > >> drivers/usb/gadget/mv_udc_core.c | 20 +++++++++--------- > >> drivers/usb/gadget/omap_udc.c | 20 +++++++++--------- > >> drivers/usb/gadget/omap_udc.h | 1 - > >> drivers/usb/gadget/pch_udc.c | 17 +++++++-------- > >> drivers/usb/gadget/pxa25x_udc.c | 28 +++++++++++++------------- > >> drivers/usb/gadget/pxa25x_udc.h | 1 - > >> drivers/usb/gadget/r8a66597-udc.c | 14 ++++++------ > >> drivers/usb/gadget/r8a66597-udc.h | 2 +- > >> drivers/usb/gadget/s3c-hsudc.c | 11 ++++----- > >> drivers/usb/gadget/s3c2410_udc.c | 16 +++++++------- > >> drivers/usb/gadget/s3c2410_udc.h | 1 - > >> 35 files changed, 176 insertions(+), 192 deletions(-) > > > > are you crazy ? To fix one single bug on f_serial you needed to change > > ALL UDCs ? > > No and No, I needed to change only the UDCs that works in my platform with > the f_serial, but I think that it is a bug on any platform that uses > f_serial, so I just want to help to stability of overall... of course, but you need not put two unrelated changes in one single patch. > Moreover, I think that the one in f_serial is an example for a bug that > might potential appear in other function drivers, so I just want to make > it right. and you are right there... the only problem is making a huge re-work on the same patch as the bugfix. > >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >> index 6d64351..c2fb969 100644 > >> --- a/drivers/usb/dwc3/gadget.c > >> +++ b/drivers/usb/dwc3/gadget.c > >> @@ -581,7 +581,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep > >> *dep) > >> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg); > >> > >> dep->stream_capable = false; > >> - dep->desc = NULL; > >> + dep->endpoint.desc = NULL; > > > > ok, so this is the only bug you have found (apparently on all UDCs). So > > fix this one alone by adding the extra needed line. Removing the local > > descriptor pointer from all UDCs is a different matter, different patch. > > > > One patch == One change. > > > Logically, this is one change, but do you want to make it for each UDC > seperatly ? I didn't say that. Well, let me rephrase it: bugfixes should be separated from rework/cleanups/new fetures and anything else. So only the bugfix as the first patch, and only the rework as the second patch. One thing that strikes me though, is that whatever Tanya did, will now become quite messy because now we have the gadget driver setting ep->desc (through config_ep_by_speed()) but the UDC needs the clear that pointer on ep_disable. This smeels like disaster to me, but go ahead. Split your patch and resend. -- balbi
Attachment:
signature.asc
Description: Digital signature
