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 v2] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1
Thread-Topic: [PATCH v2] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1
Thread-Index: AQHY57Ge68RMVQuhaUeO0fOrXMci/K4h2uCAgAAVsiCAD7z/gIABUoKQ
Date:   Mon, 7 Nov 2022 05:39:08 +0000
Message-ID: <BYAPR07MB5381CD42617915D95122D56FDD3C9@BYAPR07MB5381.namprd07.prod.outlook.com>
References: <1666620275-139704-1-git-send-email-pawell@cadence.com>
 <20221027072421.GA75844@nchen-desktop>
 <BYAPR07MB5381482129407B849BA9A616DD339@BYAPR07MB5381.namprd07.prod.outlook.com>
 <20221106090221.GA152143@nchen-desktop>
In-Reply-To: <20221106090221.GA152143@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?fsLF/uS2DEvpzir5/4OAt94aEW4MaJczYsYnrkFPHtoy4gsxK+k/bU4NRCKj?=
 =?us-ascii?Q?C6lm54EZzfHn6Lkot/zSfC/yfDrHot1F/CGiw9IANV3in+JYTTD9UjhrE8FP?=
 =?us-ascii?Q?zSQvgIeCaGcMr5Ce8nw00GD5KcxGZyHPpT9uGVJOrxdTePhCOUwqrV4IRMZ3?=
 =?us-ascii?Q?y7uP4Se9psRuahSVyOZY6le81bjrMNMd48ed6Eqozj5+oCOaOcopLAZ2S3dG?=
 =?us-ascii?Q?Qq59G0gKMBkKPxqdxZqswRgaz5vXPUxN4UVteskw677jjdoT407Ipx8+/Tni?=
 =?us-ascii?Q?Tw2NECJkm9pbYl19g9iv86RoqSMIamnHMcLE5dkxCWBFK+zISzTJYAhXzY61?=
 =?us-ascii?Q?Di/YHneYrPbIsBszpbE4aYx860O+VchW2KdbuE7od9yWZNMv6heh7SyUeLNL?=
 =?us-ascii?Q?TKt7n8UNO7oLWoTD4WTCOjwiWrh/smWwzYl/xrmMZw4ft6yCUqK2+1sqpBwC?=
 =?us-ascii?Q?bKGt6b5BXTns8AyFuYG4EJgU+neNZl+YIGrMvzWvMNRzPXhiJ9oguPcis86d?=
 =?us-ascii?Q?b29iFG+HUWWIdyyG0BArtSd1g1N8KI47mXvBRihEmvtn2mnBzBeY0El0HN5z?=
 =?us-ascii?Q?oRla/oDBu2m37e5chomfABAQC91pIArtygTuhXxRbbGpSmGpWpb+KkTb+pCO?=
 =?us-ascii?Q?8ScGEKWQdQq4DwFTlNEXIIF0OHRfLe2+8kcfQj7JdQsGEpv9hf3xwJC6SWlH?=
 =?us-ascii?Q?qhlX64f7zCBU6ap+eDlbQvI7ameOxg6Po9KQ8K3aGAqilFbzyFQPkzJvL3T2?=
 =?us-ascii?Q?UHKDACSPRFnMps8NSSsGyHnNTdlMts8+U7XLJ0ou4sn1SuhwJneYpE3gq9lz?=
 =?us-ascii?Q?LSnCZ5RFFpTUnaxCecPo/yPug39/p3biL//vTtS3WCiIKcAwuI6lNss9Ha8t?=
 =?us-ascii?Q?z+YlAGGR8EmL3qbIkH/v1j52WJyb9cX8jCcZrE74kjd6hahAYnv4dMmgWBrL?=
 =?us-ascii?Q?s9Lr2knXMlEBWaA7WO3SBkYQAyJb2dkknDl9V6qecuu+NL/essI+Wt+SynoY?=
 =?us-ascii?Q?b8AFTrSdpNttVCLT/+OjzO7H4cTyAX4aFnVQqm2URSns7KHT7a82Dzu3WYmr?=
 =?us-ascii?Q?bPrLx8qGXFgBGhRcfAjKQTWBDzU6gSA9zYvh1U15v7Hio3/tGgXxTD9YUcfu?=
 =?us-ascii?Q?Q58Vsd7Z/7SoQz38XvhoGE8tAPDbKDFyq/EOIiRwoNr+JKXmzzrdhy1/d/Pa?=
 =?us-ascii?Q?TOROuO6MHqn6GdZc/m3P70GCvmxbEH6qS7V1db5WFC6h/nAFx65mm7IM0RE+?=
 =?us-ascii?Q?eGdXMjZAKcO29yecvjlOJ+98VoRfveFg0kKG2eAjMXoTIFoAVOsREv5+dlTd?=
 =?us-ascii?Q?TjV5oquXeTVGaVsLyVwi49FkzM3Pyzcla8b8lXZNpjf4EE2gyLNEz/sjl+Kd?=
 =?us-ascii?Q?1Id+R8EOv30wnCZSSyxF7gg2JutfC9E/hPP93BJLkfxXwCpX94C9BJ8v8YSx?=
 =?us-ascii?Q?V7p6og6F4F/YOlIMit3fv9pZ1beAbFnlvFyRWGfrL1WK4Y93tErJDAntJ02O?=
 =?us-ascii?Q?Exi2hfRZvEH7bQ+9XepfozCVxgy/P/llapdjSu3ZNIBruYjhMpOcJhaUg/5p?=
 =?us-ascii?Q?e0fbivjAE2MapKZ4xy/WXp5eOGmB1Jn5k2u2qQUwQKOiVsUAA2qfO55Q1LBA?=
 =?us-ascii?Q?aw=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: be407443-e5ca-4c95-1cb7-08dac0826417
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Nov 2022 05:39:08.3239
 (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: gZDZioLrXPL3UXTxoHvnZ2GI3Arkvfba9bU7P0a392uu5PPiNWIEuhhOaHV7PmrVSpKtcY3IhQnmN9JfccraOrKCloPAfXOwNKJbPv3bJmg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR07MB5594
X-Proofpoint-GUID: tbw37VKf4uvLTa_-KEXCY-Lm-Ig5mXvR
X-Proofpoint-ORIG-GUID: tbw37VKf4uvLTa_-KEXCY-Lm-Ig5mXvR
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 clxscore=1011
 bulkscore=0 suspectscore=0 adultscore=0 priorityscore=1501 phishscore=0
 mlxscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0 spamscore=0
 mlxlogscore=533 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2211070047
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-27 08:46:17, Pawel Laszczak wrote:
>>
>> >
>> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> processing ZLP TRB by controller.
>> >>
>> >> 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>
>> >>
>> >> ---
>> >> Changelog:
>> >> v2:
>> >> - returned value for last TRB must be 0
>> >>
>> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
>> >> 100644
>> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> cdnsp_device *pdev,
>> >>
>> >>  	/* One TRB with a zero-length data packet. */
>> >>  	if (!more_trbs_coming || (transferred =3D=3D 0 && trb_buff_len =3D=
=3D 0) ||
>> >> -	    trb_buff_len =3D=3D td_total_len)
>> >> +	    trb_buff_len =3D=3D td_total_len) {
>> >> +		/* Before ZLP driver needs set TD_SIZE=3D1. */
>> >> +		if (more_trbs_coming)
>> >> +			return 1;
>> >> +
>> >>  		return 0;
>> >> +	}
>> >
>> >Does that fix the issue you want at bulk transfer, which has
>> >zero-length packet at the last packet? It seems not align with your pre=
vious
>fix.
>> >Would you mind explaining more?
>>
>> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
>> TRB.
>>
>> The last TRB in TD should have TD_SIZE=3D0, so trb for ZLP should have
>> set also TD_SIZE=3D0. If driver set TD_SIZE=3D0 on before the last one T=
RB
>> then the controller stops the transfer and ignore trb for ZLP packet.
>>
>> To fix this, the driver in such case must set TD_SIZE =3D 1 before the
>> last TRB.
>
>  	if (!more_trbs_coming || (transferred =3D=3D 0 && trb_buff_len =3D=3D 0=
) ||
> -	    trb_buff_len =3D=3D td_total_len)
> +	    trb_buff_len =3D=3D td_total_len) {
> +		/* Before ZLP driver needs set TD_SIZE=3D1. */
> +		if (more_trbs_coming)
> +			return 1;
> +
>  		return 0;
> +	}
>
>How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>Which conditions are satisfied?

For last non-ZLP TRB TD_SIZE should be 0 or 1.

We have three casess:=20
1. =20
	TRB1 - length > 1
	TRb2 - ZLP

In this case TRB1 should have set TD_SIZE =3D 1. In this case the condition
	if (more_trbs_coming)
		return 1;

returns TD_SIZE=3D1. In this case more_trb_comming for TRB1 is 1 and for TR=
B2 is 0


2.=20
	TRB1 - length >1 and we don't except ZLP

In this case TD_SIZE should be set to 0 for TRB1 and function returns 0, mo=
re_trbs_comming for TRB1 will be set to 0.

3 More TRBs without ZLP:
	e.g.
	TRB1  -  length > 0,  more_trbs_comming =3D 1 - TD_SIZE  > 0
	TRB2 -  length > 0, more_trbs_comming =3D 1  - TD_SIZE > 0
	TRB3 - length >=3D 0, more_trbs_comming =3D 0 -  TD_SIZE  =3D 0

Pawel

>
>Peter
>
>> e.g.
>>
>> TD -> TRB1  transfer_length =3D 64KB, TD_SIZE =3D0
>>           TRB2 transfer_length =3D0, TD_SIZE =3D 0  - controller will
>> 		    ignore this transfer and stop transfer on previous one
>>
>> TD -> TRB1  transfer_length =3D 64KB, TD_SIZE =3D1
>>           TRB2 transfer_length =3D0, TD_SIZE =3D 0  - controller will
>> 		    execute this trb and send ZLP
>>
>> As you noticed previously, previous fix for last TRB returned TD_SIZE
>> =3D 1 in some cases.
>> Previous fix was working correct but was not compliance with
>> controller specification.
>>
>> >
>> >>
>> >>  	maxp =3D usb_endpoint_maxp(preq->pep->endpoint.desc);
>> >>  	total_packet_count =3D DIV_ROUND_UP(td_total_len, maxp);
>> >> --
>> >> 2.25.1
>> >>
>> >
>> >--
>> >
>>
>> Thanks,
>> Pawel Laszczak
>
>--
>
>Thanks,
>Peter Chen

Regards,
Pawel Laszczak=20