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.grzeschik@xxxxxxxxxxxxxx>
> > > ---
> > > 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
stable@xxxxxxxxxxxxxxx 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
Attachment:
signature.asc
Description: Digital signature
