Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue
Hi, On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote: > From: "Felipe F. Tonello" > > _ep_queue() didn't check for errors when using add_td_to_list() > which can fail if dma_pool_alloc fails, thus causing a kernel > panic when lastnode->ptr is NULL. > > Signed-off-by: Felipe F. Tonello this can still be split down further. > --- > drivers/usb/chipidea/udc.c | 26 +++--- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 764f668..7169113e 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep) > } > > /** > - * _hardware_queue: configures a request at hardware level > - * @gadget: gadget > + * _hardware_enqueue: configures a request at hardware level > * @hwep: endpoint > + * @hwreq: request this is a cleanup and you shouldn't have a fix depending on a cleanup. Fixes are merged during the -rc cycle, while cleanups will be deferred to the following merge window. > * > * This function returns an error code > */ > @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, > struct ci_hw_req *hwreq) > if (hwreq->req.dma % PAGE_SIZE) > pages--; > > - if (rest == 0) > - add_td_to_list(hwep, hwreq, 0); > + if (rest == 0) { > + ret = add_td_to_list(hwep, hwreq, 0); > + if (ret < 0) > + goto done; > + } this is your fix. > > while (rest > 0) { > unsigned count = min(hwreq->req.length - hwreq->req.actual, > (unsigned)(pages * CI_HDRC_PAGE_SIZE)); > - add_td_to_list(hwep, hwreq, count); > + ret = add_td_to_list(hwep, hwreq, count); > + if (ret < 0) > + goto done; and this > rest -= count; > } > > if (hwreq->req.zero && hwreq->req.length > - && (hwreq->req.length % hwep->ep.maxpacket == 0)) > - add_td_to_list(hwep, hwreq, 0); > + && (hwreq->req.length % hwep->ep.maxpacket == 0)) { > + ret = add_td_to_list(hwep, hwreq, 0); > + if (ret < 0) > + goto done; > + } > and this. > firstnode = list_first_entry(&hwreq->tds, struct td_node, td); > > @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, > struct usb_request *req) > > /** > * _ep_queue: queues (submits) an I/O request to an endpoint > + * @ep:endpoint > + * @req: request > + * @gfp_flags: GFP flags (not used) cleanup > * > * Caller must hold lock > + * This function returns an error code somewhat pointless, but could come with the cleanup, no strong feelings. > */ > static int _ep_queue(struct usb_ep *ep, struct usb_request *req, > gfp_t __maybe_unused gfp_flags) > -- > 2.1.4 > -- balbi signature.asc Description: Digital signature
Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue
Hi Felipe, On Fri, Sep 18, 2015 at 6:17 PM, Felipe Balbi wrote: > Hi, > > On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote: >> From: "Felipe F. Tonello" >> >> _ep_queue() didn't check for errors when using add_td_to_list() >> which can fail if dma_pool_alloc fails, thus causing a kernel >> panic when lastnode->ptr is NULL. >> >> Signed-off-by: Felipe F. Tonello > > this can still be split down further. > >> --- >> drivers/usb/chipidea/udc.c | 26 +++--- >> 1 file changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> index 764f668..7169113e 100644 >> --- a/drivers/usb/chipidea/udc.c >> +++ b/drivers/usb/chipidea/udc.c >> @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep) >> } >> >> /** >> - * _hardware_queue: configures a request at hardware level >> - * @gadget: gadget >> + * _hardware_enqueue: configures a request at hardware level >> * @hwep: endpoint >> + * @hwreq: request > > this is a cleanup and you shouldn't have a fix depending on a > cleanup. Fixes are merged during the -rc cycle, while cleanups will be > deferred to the following merge window. Got it. > >> * >> * This function returns an error code >> */ >> @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, >> struct ci_hw_req *hwreq) >> if (hwreq->req.dma % PAGE_SIZE) >> pages--; >> >> - if (rest == 0) >> - add_td_to_list(hwep, hwreq, 0); >> + if (rest == 0) { >> + ret = add_td_to_list(hwep, hwreq, 0); >> + if (ret < 0) >> + goto done; >> + } > > this is your fix. > >> >> while (rest > 0) { >> unsigned count = min(hwreq->req.length - hwreq->req.actual, >> (unsigned)(pages * CI_HDRC_PAGE_SIZE)); >> - add_td_to_list(hwep, hwreq, count); >> + ret = add_td_to_list(hwep, hwreq, count); >> + if (ret < 0) >> + goto done; > > and this > >> rest -= count; >> } >> >> if (hwreq->req.zero && hwreq->req.length >> - && (hwreq->req.length % hwep->ep.maxpacket == 0)) >> - add_td_to_list(hwep, hwreq, 0); >> + && (hwreq->req.length % hwep->ep.maxpacket == 0)) { >> + ret = add_td_to_list(hwep, hwreq, 0); >> + if (ret < 0) >> + goto done; >> + } >> > > > and this. > >> firstnode = list_first_entry(&hwreq->tds, struct td_node, td); >> >> @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, >> struct usb_request *req) >> >> /** >> * _ep_queue: queues (submits) an I/O request to an endpoint >> + * @ep:endpoint >> + * @req: request >> + * @gfp_flags: GFP flags (not used) > > cleanup > >> * >> * Caller must hold lock >> + * This function returns an error code > > somewhat pointless, but could come with the cleanup, no strong > feelings. > >> */ >> static int _ep_queue(struct usb_ep *ep, struct usb_request *req, >> gfp_t __maybe_unused gfp_flags) >> -- >> 2.1.4 >> > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue
On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote: > From: "Felipe F. Tonello" > > _ep_queue() didn't check for errors when using add_td_to_list() > which can fail if dma_pool_alloc fails, thus causing a kernel Would you find the root cause why dma_pool_alloc fails? > panic when lastnode->ptr is NULL. > > Signed-off-by: Felipe F. Tonello > --- > drivers/usb/chipidea/udc.c | 26 +++--- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 764f668..7169113e 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep) > } > > /** > - * _hardware_queue: configures a request at hardware level > - * @gadget: gadget > + * _hardware_enqueue: configures a request at hardware level > * @hwep: endpoint > + * @hwreq: request > * > * This function returns an error code > */ > @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, > struct ci_hw_req *hwreq) > if (hwreq->req.dma % PAGE_SIZE) > pages--; > > - if (rest == 0) > - add_td_to_list(hwep, hwreq, 0); > + if (rest == 0) { > + ret = add_td_to_list(hwep, hwreq, 0); > + if (ret < 0) > + goto done; > + } > > while (rest > 0) { > unsigned count = min(hwreq->req.length - hwreq->req.actual, > (unsigned)(pages * CI_HDRC_PAGE_SIZE)); > - add_td_to_list(hwep, hwreq, count); > + ret = add_td_to_list(hwep, hwreq, count); > + if (ret < 0) > + goto done; > rest -= count; > } > > if (hwreq->req.zero && hwreq->req.length > - && (hwreq->req.length % hwep->ep.maxpacket == 0)) > - add_td_to_list(hwep, hwreq, 0); > + && (hwreq->req.length % hwep->ep.maxpacket == 0)) { > + ret = add_td_to_list(hwep, hwreq, 0); > + if (ret < 0) > + goto done; > + } > > firstnode = list_first_entry(&hwreq->tds, struct td_node, td); > > @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, > struct usb_request *req) > > /** > * _ep_queue: queues (submits) an I/O request to an endpoint > + * @ep:endpoint > + * @req: request > + * @gfp_flags: GFP flags (not used) > * > * Caller must hold lock > + * This function returns an error code > */ > static int _ep_queue(struct usb_ep *ep, struct usb_request *req, > gfp_t __maybe_unused gfp_flags) > -- > 2.1.4 > -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue
Hi Peter, On Mon, Sep 21, 2015 at 7:29 AM, Peter Chen wrote: > On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote: >> From: "Felipe F. Tonello" >> >> _ep_queue() didn't check for errors when using add_td_to_list() >> which can fail if dma_pool_alloc fails, thus causing a kernel > > Would you find the root cause why dma_pool_alloc fails? Partially. For some reason the udc_irq() is not been called after a f_midi_transmit(). I am looking into that. Felipe -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html