HI,
On Tue, Mar 26, 2013 at 09:24:30PM +0100, Michael Grzeschik wrote:
> On Tue, Mar 26, 2013 at 07:55:46PM +0200, Felipe Balbi wrote:
> > Hi,
> >
> > On Tue, Mar 26, 2013 at 05:58:45PM +0100, Michael Grzeschik wrote:
> > > A static count of transfer descriptors was used everywhere in the driver
> > > with the fixed number 5. This patch adds a define, named TD_PAGE_COUNT,
> > > and replaces all users of this value. This way its possible to have only
> > > one parameter to change and limit the amount of buffer pointers per TD.
> > >
> > > Signed-off-by: Michael Grzeschik <m.grzesc...@pengutronix.de>
> > > ---
> > > Changes since v1:
> > > - renamed TD_COUNT to more precise TD_PAGE_COUNT
> > > - changed TD_PAGE_COUNT to 5 based on the previous patch
> > > - fixed patch description to address the tds buffers not the td itself
> > >
> > > 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 7dcd390..4ca3373 100644
> > > --- a/drivers/usb/chipidea/ci.h
> > > +++ b/drivers/usb/chipidea/ci.h
> > > @@ -21,6 +21,7 @@
> > >
> > > /******************************************************************************
> > > * DEFINE
> > >
> > > *****************************************************************************/
> > > +#define TD_PAGE_COUNT 5
> > > #define CI13XXX_PAGE_SIZE 4096ul /* page size for TD's */
> > > #define ENDPT_MAX 32
> > > #define RX 0 /* similar to USB_DIR_OUT but can be used as an
> > > index */
> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > index e779870..4294c2c 100644
> > > --- a/drivers/usb/chipidea/udc.c
> > > +++ b/drivers/usb/chipidea/udc.c
> > > @@ -445,7 +445,7 @@ _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_PAGE_COUNT; i++)
> > > mReq->ptr->page[i] =
> > > (mReq->req.dma + i * CI13XXX_PAGE_SIZE) &
> > > ~TD_RESERVED_MASK;
> > >
> > > @@ -697,8 +697,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_PAGE_COUNT - 1) * CI13XXX_PAGE_SIZE) {
> > > + req->length = (TD_PAGE_COUNT - 1) * CI13XXX_PAGE_SIZE;
> >
> > this is a bug. You shouldn't change req->length, you might completely
> > screw up a gadget driver. Don't change anything what the gadget driver
> > passes to you, ever. Change some private structure instead.
> >
> > (yeah, I know it's not part of $subject)
>
> Beside that bug get fixed by the patch
> "usb: chipidea: udc: add multiple td support to hardware_{en,de}queue"
> in that series.*ALL BUG FIXES DESERVE THEIR OWN PATCH*. Burn this into non-volatile memory. When writing patches, whenever you find a bug fix, you create a separate branch on top of newest -rc (or Greg's usb-linus or my 'fixes' branch depending on which driver you're fixing) fix the bug with as few lines as possible and send the patch so it can be applied during the -rc, without creating dependencies with patches going for next merge window. If bug was introduced prior to last merge window (e.g. you figure bug was introduced in v3.6, instead of v3.8) then you add Cc sta...@vger.kernel.org to make sure folks using older kernels also have the fix. I would suggest Alex doesn't take this series until it's properly split into a bugfixes series and another series of cleanups/new features for next. cheers -- balbi
signature.asc
Description: Digital signature
