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 for Android: free password hash cracker in your pocket

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

Message-ID: <20191008073405.GF5670@b29397-desktop>
Date:   Tue, 8 Oct 2019 07:34:21 +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>,
        "sureshp@...ence.com" <sureshp@...ence.com>,
        "kurahul@...ence.com" <kurahul@...ence.com>
Subject: Re: [PATCH] usb: cdns3: Fix dequeue implementation.

On 19-10-07 13:06:18, Pawel Laszczak wrote:
> Dequeuing implementation in cdns3_gadget_ep_dequeu gets first request from

%s/cdns3_gadget_ep_dequeu/cdns3_gadget_ep_dequeue

> deferred_req_list and changed TRB associated with it to LINK TRB.
> This approach is incorrect because deferred_req_list contains requests
> that have not been placed on hardware RING.  In this case driver should
> just giveback this request to gadget driver.
> 
> The patch implements new approach that first checks where dequeuing
> request is located and only when it's on Transfer Ring then changes TRB
> associated with it to LINK TRB.
> During processing completed transfers such LINK TRB will be ignored.
> 
> Reported-by: Peter Chen <peter.chen@....com>
> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> ---
>  drivers/usb/cdns3/gadget.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> 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?

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