Re: [PATCH V2 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 23, 2015 at 03:23:26PM -0400, Alan Cooper wrote:
> On Wed, Jul 22, 2015 at 10:03 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> > Hi,
> >
> > On Wed, Jul 22, 2015 at 06:27:29PM -0400, Alan Cooper wrote:
> >> On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> >> > On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote:
> >> >> On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote:
> >> >> > V2 - Fix a compiler bug that happend when the config options
> >> >> > CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE
> >> >> > were enabled.
> >> >> >
> >> >> > ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer
> >> >> > incorrectly by reading the wrong register for the upper 32 bits.
> >> >> > The header file defining the registers was incorrect.
> >> >>
> >> >> btw, the header file was really "incorrect" as long as you passed 0 to
> >> >> the argument :-p
> >> >
> >> > in fact, the minimal fix for this bug would be the one below:
> >> >
> >> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c
> >> > index b04980cf6dc4..1efa61265d8d 100644
> >> > --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
> >> > +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
> >> > @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct bdc_req *req)
> >> >         /* The current hw dequeue pointer */
> >> >         tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(0));
> >> >         deq_ptr_64 = tmp_32;
> >> > -       tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(1));
> >> > +       tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS1(0));
> >> >         deq_ptr_64 |= ((u64)tmp_32 << 32);
> >> >
> >> >         /* we have the dma addr of next bd that will be fetched by hardware */
> >> >
> >> > And $subject becomes a cleanup patch for v4.3. Can you make these
> >> > changes, please ?
> >>
> >> Yes, I can make these changes, but first let me explain why I did it this way.
> >> The 8 End Point Status registers are 32 bit consecutive registers, so
> >> defining them as:
> >> #define BDC_EPSTS0(n)  (0x60 + (n * 0x10))
> >> #define BDC_EPSTS1(n)  (0x64 + (n * 0x10))
> >> #define BDC_EPSTS2(n)  (0x68 + (n * 0x10))
> >> #define BDC_EPSTS3(n)  (0x6c + (n * 0x10))
> >> #define BDC_EPSTS4(n)  (0x70 + (n * 0x10))
> >> #define BDC_EPSTS5(n)  (0x74 + (n * 0x10))
> >> #define BDC_EPSTS6(n)  (0x78 + (n * 0x10))
> >> #define BDC_EPSTS7(n)  (0x7c + (n * 0x10))
> >> seems misleading, does not reflect the hardware and using anything
> >> other than (0) would get you to some other unexpected register and
> >> should be considered a coding error.
> >
> > it sure is, but the minimal patch for -rc is what I sent above :-) As
> > long as you pass 0 as parameters, all your offsets are correct, so
> > removing the parameter (which must be always zero) is, actually,
> > refactoring happening.
> >
> 
> OK, I think I understand. You're asking me to split the bug fix patch
> into two patches, one is a minimal bug fix patch for -rc and the other
> is a cleanup patch (fix the header file) that will go into 4.3. Is
> that correct?

correct.

> >> I think the original hardware spec had each End Point Status as a
> >> block of registers but the first silicon had them as single registers
> >> and they are expected to stay that way.
> >
> > I believe they wanted to have:
> >
> > #define BDC_EPSTS(n)            (0x60 + (n * 0x4))
> >
> > and that way you can access each EP by passing the correct argument to
> > that macro.
> >
> 
> Actually, the 8 EPSTS registers are all different and together contain
> the info for 1 end point. The original plan was to have 4 32bit
> registers per end point and enough sets of these registers for all
> endpoints, but this turned out to be too expensive in silicon so they
> changed it to 1 set of 8 registers that are updated when an end point
> is stopped.

oh ok. thanks

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux