> 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. > I see, so the bugfix can be more easily and quickly pushed, eliminating new bugs... No problem, you are right. > 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. Yes... harmful, but the bug is already there... :) Moreover, the config_ep_by_speed() sets the ep->desc, and when enabling the endpoint (gadget.h::usb_ep_enable) the ep->desc is sent to the udc. So hopefully, just clearing the ep->desc on disable is the only problem. So as first bugfix I will just set the endpoint descriptor to NULL on endpoint disable. Should I do it to all the UDCs ? all in one patch ? > -- > balbi > Ido -- Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
