Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Pawel Laszczak <pawell@cadence.com>
To:     Peter Chen <peter.chen@kernel.org>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: RE: [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
Thread-Topic: [PATCH] usb: cdnsp: Fix issue with Clear Feature Halt Endpoint
Thread-Index: AQHY6R6Kb+CO20jKM0iAijOjlYxMN64xrF6AgAFZHNA=
Date:   Mon, 7 Nov 2022 06:01:10 +0000
Message-ID: <BYAPR07MB538173AA9BFCC5C7CE02A07CDD3C9@BYAPR07MB5381.namprd07.prod.outlook.com>
References: <20221026093710.449809-1-pawell@cadence.com>
 <20221106090815.GB152143@nchen-desktop>
In-Reply-To: <20221106090815.GB152143@nchen-desktop>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?dbPrPa7mqEUhRHnnGCAFtTvQzqGw/Ma/YG8KwpdYS1obQJOEu9xBUWejvxqN?=
 =?us-ascii?Q?SDnBn/YXdJLfFEkfR47GuCZC/p03+sTreZUjiGnynlmc1yqJYUyyHMZQHjnv?=
 =?us-ascii?Q?t+KTRS/idj3WdTQLHrBR2UhcDrVHORmNcbJQbLQoA+u+JUCc/XqGoPHJ76Ci?=
 =?us-ascii?Q?KFfN7C39QLntFO+JVfRBzkMvHa8Lf/U+0QCmHDyfXIW6limfgSB24oTe8g0H?=
 =?us-ascii?Q?Q28GJv1FDFI1OgSFtFWqGVPNmeZpTWmzmOkF8qXMaeGrsNqxY5TGjVyu56Jo?=
 =?us-ascii?Q?UYy+rg65Z6F7ZqY5pcHT2viFG/0LBY5KOGVEDCgV+fOHpyYtTSl3wByTHVlE?=
 =?us-ascii?Q?eeto8lyAdtsNY6WQj9ZWjJUQwPG2OIc8jUmpT7aSUpaUBV/Wz4+qXTrLEC47?=
 =?us-ascii?Q?zGcQ1J9dDnbDxTAvWvWct53tHbBVgKVfAURwElOFQ628KO+CEBnFYR5rQu/2?=
 =?us-ascii?Q?MgjD8L1gS51grwnoK3pT0JTxUPLIsuzv0Ut3W9DocHkKP6QyeAWPxSusMnLq?=
 =?us-ascii?Q?jKYUQVMgCw0NpUKTTaYLUXZD++o1dQZ5NXHM4aRebO9ZRIuRGa7QARaohizp?=
 =?us-ascii?Q?ujmvGxMeITqAitxTWnn13Vre0mP70kMRxxsdRMElhfIsTTSCpgWWuStNxTQi?=
 =?us-ascii?Q?eF5l2XTiA+tfNRFQTeFtZnnSE9wwRs0w881GJkMMP6QIKlpMmtTidRwDGvPn?=
 =?us-ascii?Q?ChhosRXpCG7Pl6Z0UR1oxvqCR+iz60ie9REqkMQDcWpdfqRdjcPaEuxfWrgT?=
 =?us-ascii?Q?QLUf07ilGGlNvCI2K1iFKQLGfq3EbkbehDyyvhJtwJRYaopq2XzI2+MTiRnH?=
 =?us-ascii?Q?HNtkPsrATsmNjeCek9mmUe3H+aYUJbOcxNDkrb6fDiXQ5iM47w0mY3ivYAoi?=
 =?us-ascii?Q?MtFOeCNVFahP0pCCUMx4/qoEc+qhm3NahWieNsEF7aVP/OO9Yhd6M4UjZkCp?=
 =?us-ascii?Q?hbYbxiQNcLpfVKcGnWtiskva5vdrTs/+HJeh9HMCIj+n5slam9Flv3DqkaUO?=
 =?us-ascii?Q?+hlbauWjwxA4nVfDdMoyyMvnjKq43yoBFABT/PR5mT9bFH9aUn4LH1k/a/qM?=
 =?us-ascii?Q?zDzr5+KifkCLm1nwUlV1QJXuKbwjMuiSVOa8Z4mFPf1XXdR9FebGgSEoSBpq?=
 =?us-ascii?Q?6JnyHcjLEK3CSWFVXT3CZwkfBJyEnkhQCzC4d3pwOSTpyGFdx8QUqtFOF1Ky?=
 =?us-ascii?Q?nyFmtdWLITFfUktci/JVkgBvSAaCJGha5U/4Dou4UyDmrJVavrrVfVl2Xr4E?=
 =?us-ascii?Q?8C4s0d9j7lEcadBus3vhEr6hWjAoJX7z/CnJcOCtQNWZA5NTtv3LjIoE9X0S?=
 =?us-ascii?Q?Rse4UsQKuHuzEhCudijSFjIcmAe6V1MNEpmNSBkmZWBmGJj4oKR3fklsIUOr?=
 =?us-ascii?Q?MGX8R9ZryLjyG4JsUM7PpJ6Sm79RJZNQsRSQ2aIvnjaP9BQi2vR8H1Od6kkn?=
 =?us-ascii?Q?EviqGbQLHAtCvXj9FCeQib1iXdnhZMM61hSY5HeH2/B3rwdkHf2Yxqa7G0h9?=
 =?us-ascii?Q?h/uzv41+gZbES1BqcFLxrTTSBzP6aQehQPBUGod8/4asIJbflWuBWqwR6IsQ?=
 =?us-ascii?Q?tLBCPl2fJCVB3lkM/VwjiOHok/F5jUrHbl7dQMD1oGC4KVqO7xhmapY+IobN?=
 =?us-ascii?Q?WA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: cadence.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BYAPR07MB5381.namprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cbf45496-48ff-4242-a29b-08dac0857872
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Nov 2022 06:01:10.9683
 (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: cgys4HBiP8j2aRkIcE9yxRzSzkFNfXsUn9YGQYJ/rjm5O/kX+DwKWYEfePw5vqEF3UreSyejBM/3JS3JywFL2CzqHCi+788WLPpjb6+NYt0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR07MB9179
X-Proofpoint-GUID: x639djqNcdnPtcgdCTdNizHlnsU5qwq8
X-Proofpoint-ORIG-GUID: x639djqNcdnPtcgdCTdNizHlnsU5qwq8
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-06_16,2022-11-03_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 malwarescore=0
 priorityscore=1501 clxscore=1015 lowpriorityscore=0 bulkscore=0
 impostorscore=0 adultscore=0 mlxscore=0 phishscore=0 mlxlogscore=605
 suspectscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2211070051
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>
>On 22-10-26 05:37:10, Pawel Laszczak wrote:
>> During handling Clear Halt Endpoint Feature request driver invokes
>> Reset Endpoint command. Because this command has some issue with
>
>What are issues? Would you please explain more?

I don't know the internal behavior of controller. I know that there is the =
issue with resetting endpoint
in running state, resulting in the  controller after rearming start handlin=
g incorrect TRB.

>
>> transition endpoint from Running to Idle state the driver must stop
>> the endpoint by using Stop Endpoint command.
>>
>> cc: <stable@vger.kernel.org>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-gadget.c | 12 ++++--------
>>  drivers/usb/cdns3/cdnsp-ring.c   |  3 ++-
>>  2 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
>> b/drivers/usb/cdns3/cdnsp-gadget.c
>> index e2e7d16f43f4..0576f9b0e4aa 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -600,11 +600,11 @@ int cdnsp_halt_endpoint(struct cdnsp_device
>> *pdev,
>>
>>  	trace_cdnsp_ep_halt(value ? "Set" : "Clear");
>>
>> -	if (value) {
>> -		ret =3D cdnsp_cmd_stop_ep(pdev, pep);
>> -		if (ret)
>> -			return ret;
>> +	ret =3D cdnsp_cmd_stop_ep(pdev, pep);
>> +	if (ret)
>> +		return ret;
>>
>
>In your change ,it call cdnsp_cmd_stop_ep unconditionally, no matter set o=
r
>clear halt? Is it your expectation? If it is, why?

No exactly unconditionally.

The below condition:
	if (ep_state =3D=3D EP_STATE_STOPPED || ep_state =3D=3D EP_STATE_DISABLED=
=20
		||   ep_state =3D=3D EP_STATE_HALTED) {
			goto ep_stopped;

Will decide whether command will be called or not.
It will be called only when endpoint is in RUNNING state.

Pawel

>
>> +	if (value) {
>>  		if (GET_EP_CTX_STATE(pep->out_ctx) =3D=3D
>EP_STATE_STOPPED) {
>>  			cdnsp_queue_halt_endpoint(pdev, pep->idx);
>>  			cdnsp_ring_cmd_db(pdev);
>> @@ -613,10 +613,6 @@ int cdnsp_halt_endpoint(struct cdnsp_device
>> *pdev,
>>
>>  		pep->ep_state |=3D EP_HALTED;
>>  	} else {
>> -		/*
>> -		 * In device mode driver can call reset endpoint command
>> -		 * from any endpoint state.
>> -		 */
>>  		cdnsp_queue_reset_ep(pdev, pep->idx);
>>  		cdnsp_ring_cmd_db(pdev);
>>  		ret =3D cdnsp_wait_for_cmd_compl(pdev); diff --git
>> a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
>> index 25e5e51cf5a2..aa79bce89d8a 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -2081,7 +2081,8 @@ int cdnsp_cmd_stop_ep(struct cdnsp_device
>*pdev, struct cdnsp_ep *pep)
>>  	u32 ep_state =3D GET_EP_CTX_STATE(pep->out_ctx);
>>  	int ret =3D 0;
>>
>> -	if (ep_state =3D=3D EP_STATE_STOPPED || ep_state =3D=3D
>EP_STATE_DISABLED) {
>> +	if (ep_state =3D=3D EP_STATE_STOPPED || ep_state =3D=3D
>EP_STATE_DISABLED ||
>> +	    ep_state =3D=3D EP_STATE_HALTED) {
>>  		trace_cdnsp_ep_stopped_or_disabled(pep->out_ctx);
>>  		goto ep_stopped;
>>  	}
>> --
>> 2.25.1
>>
>
>--
>
>Thanks,
>Peter Chen

Regards,
Pawel Laszczak