Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Pawel Laszczak <pawell@cadence.com>
To:     Peter Chen <peter.chen@nxp.com>
CC:     "felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jbergsagel@ti.com" <jbergsagel@ti.com>,
        "nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
        Suresh Punnoose <sureshp@cadence.com>,
        Rahul Kumar <kurahul@cadence.com>
Subject: RE: [PATCH] usb: cdns3: Fix dequeue implementation.
Thread-Topic: [PATCH] usb: cdns3: Fix dequeue implementation.
Thread-Index: AQHVfQekfRknIN1IBke0BYrE5ibzAKdQW4qAgAAGl3A=
Date:   Tue, 8 Oct 2019 08:01:19 +0000
Message-ID: <BN7PR07MB4705E02D18AE939D16BDDBC0DD9A0@BN7PR07MB4705.namprd07.prod.outlook.com>
References: <1570449978-19145-1-git-send-email-pawell@cadence.com>
 <20191008073405.GF5670@b29397-desktop>
In-Reply-To: <20191008073405.GF5670@b29397-desktop>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: cadence.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b54a479a-1cee-45db-fb5e-08d74bc5b3c8
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Oct 2019 08:01:19.2608
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9fXSLSlET9R94cYwTV6e9wAW8g3A/4Mn3j2+gMfY2aLT16Ogc+pV6N+omhkvefCPp5WmsrUt0zun881dfChANAqtaRH2a/WtoVKHAMzfZ8I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR07MB4355
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8
 definitions=2019-10-08_03:2019-10-07,2019-10-08 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 malwarescore=0
 adultscore=0 impostorscore=0 clxscore=1015 priorityscore=1501 mlxscore=0
 lowpriorityscore=0 bulkscore=0 mlxlogscore=999 phishscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-1908290000 definitions=main-1910080081
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


>-----Original Message-----
>From: Peter Chen <peter.chen@nxp.com>
>Sent: Tuesday, October 8, 2019 9:34 AM
>To: Pawel Laszczak <pawell@cadence.com>
>Cc: felipe.balbi@linux.intel.com; gregkh@linuxfoundation.org; linux-usb@vg=
er.kernel.org; rogerq@ti.com; linux-
>kernel@vger.kernel.org; jbergsagel@ti.com; nsekhar@ti.com; nm@ti.com; Sure=
sh Punnoose <sureshp@cadence.com>; Rahul Kumar
><kurahul@cadence.com>
>Subject: Re: [PATCH] usb: cdns3: Fix dequeue implementation.
>
>EXTERNAL MAIL
>
>
>On 19-10-07 13:06:18, Pawel Laszczak wrote:
>> Dequeuing implementation in cdns3_gadget_ep_dequeu gets first request fr=
om
>
>%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@nxp.com>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.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 =3D cdns3_next_request(&priv_ep->pending_req_list);
>>  		priv_req =3D to_cdns3_request(request);
>>
>> +		trb =3D priv_ep->trb_pool + priv_ep->dequeue;
>> +
>> +		/* Request was dequeued and TRB was changed to TRB_LINK. */
>> +		if (TRB_FIELD_TO_TYPE(trb->control) =3D=3D 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=20
was changed to Link TRB so it should be ignored in cdns3_transfer_completed=
 function.=20

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 =3D 0;
>>  	unsigned long flags;
>>  	int ret =3D 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 =3D=3D req)
>> +		if (request =3D=3D req) {
>> +			req_on_hw_ring =3D 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 =3D=3D priv_req->trb)
>> -		cdns3_wa1_restore_cycle_bit(priv_ep);
>> -
>>  	link_trb =3D priv_req->trb;
>> -	cdns3_move_deq_to_next_trb(priv_req);
>> -	cdns3_gadget_giveback(priv_ep, priv_req, -ECONNRESET);
>> -
>> -	/* Update ring */
>> -	request =3D cdns3_next_request(&priv_ep->deferred_req_list);
>> -	if (request) {
>> -		priv_req =3D to_cdns3_request(request);
>>
>> +	/* Update ring only if removed request is on pending_req_list list */
>> +	if (req_on_hw_ring) {
>>  		link_trb->buffer =3D TRB_BUFFER(priv_ep->trb_pool_dma +
>>  					      (priv_req->start_trb * TRB_SIZE));
>>  		link_trb->control =3D (link_trb->control & TRB_CYCLE) |
>> -				    TRB_TYPE(TRB_LINK) | TRB_CHAIN | TRB_TOGGLE;
>> -	} else {
>> -		priv_ep->flags |=3D EP_UPDATE_EP_TRBADDR;
>> +				    TRB_TYPE(TRB_LINK) | TRB_CHAIN;
>> +
>> +		if (priv_ep->wa1_trb =3D=3D 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