linux-kernel - Re: [PATCH] usb: cdns3: Fix dequeue implementation.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191009023141.GJ5670@b29397-desktop>
Date:   Wed, 9 Oct 2019 02:31:57 +0000
From:   Peter Chen <peter.chen@....com>
To:     Pawel Laszczak <pawell@...ence.com>
CC:     "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "rogerq@...com" <rogerq@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        Suresh Punnoose <sureshp@...ence.com>,
        Rahul Kumar <kurahul@...ence.com>
Subject: Re: [PATCH] usb: cdns3: Fix dequeue implementation.

On 19-10-08 08:01:19, Pawel Laszczak wrote:
> >>
> >> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> >> index 2ca280f4c054..9050b380ab83 100644
> >> --- a/drivers/usb/cdns3/gadget.c
> >> +++ b/drivers/usb/cdns3/gadget.c
> >> @@ -1145,6 +1145,14 @@ static void cdns3_transfer_completed(struct cdns3_device *priv_dev,
> >>  		request = cdns3_next_request(&priv_ep->pending_req_list);
> >>  		priv_req = to_cdns3_request(request);
> >>
> >> +		trb = priv_ep->trb_pool + priv_ep->dequeue;
> >> +
> >> +		/* Request was dequeued and TRB was changed to TRB_LINK. */
> >> +		if (TRB_FIELD_TO_TYPE(trb->control) == TRB_LINK) {
> >> +			trace_cdns3_complete_trb(priv_ep, trb);
> >> +			cdns3_move_deq_to_next_trb(priv_req);
> >> +		}
> >
> >If the request was dequeued, it should not be at request list, isn't it?
> 
> Yes
> The dequeued request was removed from pending list but TRB associated with it 
> was changed to Link TRB so it should be ignored in cdns3_transfer_completed function. 
> 

I see, you are right. Except for the typo, otherwise:

Reviewed-by: Peter Chen <peter.chen@....com>

Peter


> Pawel
> 
> >
> >Peter
> >> +
> >>  		/* Re-select endpoint. It could be changed by other CPU during
> >>  		 * handling usb_gadget_giveback_request.
> >>  		 */
> >> @@ -2067,6 +2075,7 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,
> >>  	struct usb_request *req, *req_temp;
> >>  	struct cdns3_request *priv_req;
> >>  	struct cdns3_trb *link_trb;
> >> +	u8 req_on_hw_ring = 0;
> >>  	unsigned long flags;
> >>  	int ret = 0;
> >>
> >> @@ -2083,8 +2092,10 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,
> >>
> >>  	list_for_each_entry_safe(req, req_temp, &priv_ep->pending_req_list,
> >>  				 list) {
> >> -		if (request == req)
> >> +		if (request == req) {
> >> +			req_on_hw_ring = 1;
> >>  			goto found;
> >> +		}
> >>  	}
> >>
> >>  	list_for_each_entry_safe(req, req_temp, &priv_ep->deferred_req_list,
> >> @@ -2096,27 +2107,21 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,
> >>  	goto not_found;
> >>
> >>  found:
> >> -
> >> -	if (priv_ep->wa1_trb == priv_req->trb)
> >> -		cdns3_wa1_restore_cycle_bit(priv_ep);
> >> -
> >>  	link_trb = priv_req->trb;
> >> -	cdns3_move_deq_to_next_trb(priv_req);
> >> -	cdns3_gadget_giveback(priv_ep, priv_req, -ECONNRESET);
> >> -
> >> -	/* Update ring */
> >> -	request = cdns3_next_request(&priv_ep->deferred_req_list);
> >> -	if (request) {
> >> -		priv_req = to_cdns3_request(request);
> >>
> >> +	/* Update ring only if removed request is on pending_req_list list */
> >> +	if (req_on_hw_ring) {
> >>  		link_trb->buffer = TRB_BUFFER(priv_ep->trb_pool_dma +
> >>  					      (priv_req->start_trb * TRB_SIZE));
> >>  		link_trb->control = (link_trb->control & TRB_CYCLE) |
> >> -				    TRB_TYPE(TRB_LINK) | TRB_CHAIN | TRB_TOGGLE;
> >> -	} else {
> >> -		priv_ep->flags |= EP_UPDATE_EP_TRBADDR;
> >> +				    TRB_TYPE(TRB_LINK) | TRB_CHAIN;
> >> +
> >> +		if (priv_ep->wa1_trb == priv_req->trb)
> >> +			cdns3_wa1_restore_cycle_bit(priv_ep);
> >>  	}
> >>
> >> +	cdns3_gadget_giveback(priv_ep, priv_req, -ECONNRESET);
> >> +
> >>  not_found:
> >>  	spin_unlock_irqrestore(&priv_dev->lock, flags);
> >>  	return ret;
> >
> >--
> >
> >Thanks,
> >Peter Chen

-- 

Thanks,
Peter Chen