Received-SPF: pass (google.com: domain of linux-kernel+bounces-28479-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <34101c32-65cd-4433-974f-23a16f9981fa@amd.com>
Date: Tue, 16 Jan 2024 21:00:03 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] Fix stuck UCSI controller on DELL
To: "Christian A. Ehrhardt" <lk@c--e.de>,
 Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: linux-usb@vger.kernel.org, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>, Neil Armstrong <neil.armstrong@linaro.org>,
 Hans de Goede <hdegoede@redhat.com>, Saranya Gopal
 <saranya.gopal@intel.com>, linux-kernel@vger.kernel.org
References: <20240103100635.57099-1-lk@c--e.de>
 <ZZadhlh3q9ZInxvU@kuha.fi.intel.com> <ZaV/kwuh2MBNY5d2@cae.in-ulm.de>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <ZaV/kwuh2MBNY5d2@cae.in-ulm.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?ZTFEU0FNZXdtR2pPSmhaUWI1dldBMzZudnd2YnZpdGp4eEdNMVp4amFLMVNz?=
 =?utf-8?B?NFVhYnJnRGhkaFBXMURiYVY2YjFncEE4MkdnSUxJK0pld3BWWS9nWUFrdHU0?=
 =?utf-8?B?U25NWmI1U1F2NXQrRDRRNHA1YVdRSnBMKzFybzg5V0h4dHRWWlpHTEdIZ2JV?=
 =?utf-8?B?eUxnV2luVjl0ZmNsMlpMTWRZOGF1UWdCYjRoQXZTMVdQUnhlY0NlMGtvcTNi?=
 =?utf-8?B?NW0vWkhocjBTVGpUUE9IczYrcng2WUdKbnRXcU92eTkwanRKanl0NmxRWUZh?=
 =?utf-8?B?OHZCNDMyZzB1cTRNWEFrNVJTVXVIaVNEcUZFS3JnOXhJWllubmhodlI2dTlX?=
 =?utf-8?B?ZmhNb0xJaVFvblJaemFtOG5CRVV6ZG4xZTlYRFZvUWdMbjVjZ05NTGpXSEEv?=
 =?utf-8?B?UkRiNkxXVlM1Vk84SStCcEV3Q2llWkFXU0I2S0ZhZVAyYlJtQ0tlMVNCamZN?=
 =?utf-8?B?NDhlUmU4a3RZRVVoaDNjaSt5bVlGTlFPbUY4VlNVR0c3a05Dbm5Mby9DRncx?=
 =?utf-8?B?bGoyWjhYZ0cwdlZsaDNIeHdJSTZFYWRZMFJqd0VkR0l0L2tma1ZtVW1IenZQ?=
 =?utf-8?B?ZnpWbXIxTDAxZHlGbnY3aW9kcEFiT21VLzAvazNPcUY0emI2bWk3SHJva2J4?=
 =?utf-8?B?QzRxVTJaOEpxY3NadmJLWWdabkhhcTRyUkRDYzFNYlVMUlpDYlBEN0czN3pN?=
 =?utf-8?B?TmM0L0pHYXp5a1UwZWR1V0JHU2RZcTBvaDMxdTBPZVN3M0ZpNmZQYUtaRk1T?=
 =?utf-8?B?SzNzV1BQRzQwK0VTMUJBN1hIdjlwS1JQMkt5R2ZnL2tTdmZzaThPZ01IZW1m?=
 =?utf-8?B?UlZWTUQrMHdVcDZyUUgwQ2VrYWo0MTNpd1pxR2Q4UXNmZmc3Z3BoK21vUlYv?=
 =?utf-8?B?R25tSHdpN3UxYklpNlR3dE1yYkJiR3ZrUm5QdnNMU0dNSEt2MjFBMlBoaFV4?=
 =?utf-8?B?MVdQTm1rM05taG9zUnJLODI3MUduMUcrbEZlRVFqdmIyd0FrNFdiMExOdVRq?=
 =?utf-8?B?UkVTdGlIQTZqZ2dLajFadDNBOG5kYjRIdU82NGNpbHB3OS9HRVZEWS9aMG1w?=
 =?utf-8?B?NGFGOXRuN1c3MG9XZ3g5cEFIVm9GZkQyTzQycWNkbGFSc1FFVWpKeEhHeHQw?=
 =?utf-8?B?bjBRNXZuQXZqSzlHOHV4Q3FFN2E3WXZTMVljZmcyNldQc0xVUy80aCtVSmVi?=
 =?utf-8?B?MXUzQVFZRHEwVC93UEJaY2VtbmlYR01XUnZPQ2ovUXRnOERVRTRtWHovamhs?=
 =?utf-8?B?ZGJORFlmZ3krOHI0di9iMG81Q09mLzVreHNuaTIzdC9kRW1XbmQ3Nk1URTRl?=
 =?utf-8?B?RWdPei9BL0E3Yis5aWtRMFlzaUUxdDBuSUNSZjNTd0szeUJ1NytNcW9DZDlj?=
 =?utf-8?B?UmRnUWNGd2NzdVRPUTExTXFTMXdOL2tudk80ZjZYU1QyRTR3Z3NjcndsUDlH?=
 =?utf-8?B?TUNTa0FLcVNxS1ZOR2pzUytxMmFZRTVJNG5Zb3E0TnBIa0xhck9jR00vbXZt?=
 =?utf-8?B?dDdlZmpxREhKMWJ5Tjd3SVNqdnE3c2s2c09ic0YxcnVHcXNxR3J3THZyeEJM?=
 =?utf-8?B?cWpQbzVTRThjYnN5a2I4cmRpUnZtUmkydjdZeUg2eEI3UnJPY2FuWmZ5TzBR?=
 =?utf-8?B?MHpGdENrRUZUMXRLVFFrYTNWNkhzZ0FTcVkydVpCSitrNHJ3RVIxYVZiQkhH?=
 =?utf-8?B?TGs0WHhoODhLVHRndFRJOFdFSmhlejJGRnM0WnZTbk9lYm1UVW5RWEliQ3Vv?=
 =?utf-8?B?UXViakttUmMycWpWSlJHR2orQ3NhMGxGa1Vzamx6WmhrNFVnaFFzKzRyYWVK?=
 =?utf-8?B?cGdyVjgxVlE4K0Z5UllML05aMGlQalRZbGhkOHpjM0xTQ3pWL1J6UGZlQ0k3?=
 =?utf-8?B?djN3UG9tYW9Ea0hTcGhkZVhrMC9TcFgraW83a2xtSEY5Mlg0c3RpRTI0VkxY?=
 =?utf-8?B?dEJYVXdFcWtpOGowTWpVRFBjYXRBMGRKK3pueVcxdnBiQTR4ZVNuT0pxaU5v?=
 =?utf-8?B?UTFQYXcrY3BUUUFxd0daUjVBVkdkK3A5U2NWbi9oOXpQRnJ6NGJucERaTmkw?=
 =?utf-8?B?cHZ6ZU9ST2FTYzhlZ1pWUG9OYnNnV25xWkJxeHNRZXBsUkszWmFxREFnbnE2?=
 =?utf-8?Q?4maTcSfliBOBilgDf59oz4wju?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a319a779-0ab5-4950-870d-08dc1708685a
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 03:00:05.9295
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bhA96jgl73kS1BD1m/NEsRGoxAmT14KnF1BpKYziPOPZJU7V8V6Vk4x1OQHIHwlFX1niVH8/edy77tjLaJYDUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8588

On 1/15/2024 12:55, Christian A. Ehrhardt wrote:
> 
> Hi Heikki,
> 
> sorry to bother you again with this but I'm afraid there's
> a misunderstanding wrt. the nature of the quirk. See below:
> 
> On Thu, Jan 04, 2024 at 01:59:02PM +0200, Heikki Krogerus wrote:
>> Hi Christian,
>>
>> On Wed, Jan 03, 2024 at 11:06:35AM +0100, Christian A. Ehrhardt wrote:
>>> I have a DELL Latitude 5431 where typec only works somewhat.
>>> After the first plug/unplug event the PPM seems to be stuck and
>>> commands end with a timeout (GET_CONNECTOR_STATUS failed (-110)).
>>>
>>> This patch fixes it for me but according to my reading it is in
>>> violation of the UCSI spec. On the other hand searching through
>>> the net it appears that many DELL models seem to have timeout problems
>>> with UCSI.
>>>
>>> Do we want some kind of quirk here? There does not seem to be a quirk
>>> framework for this part of the code, yet. Or is it ok to just send the
>>> additional ACK in all cases and hope that the PPM will do the right
>>> thing?
>>
>> We can use DMI quirks. Something like the attached diff (not tested).
>>
>> thanks,
>>
>> -- 
>> heikki
> 
>> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
>> index 6bbf490ac401..7e8b1fcfa024 100644
>> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
>> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
>> @@ -113,18 +113,44 @@ ucsi_zenbook_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t val_
>>   	return 0;
>>   }
>>   
>> -static const struct ucsi_operations ucsi_zenbook_ops = {
>> -	.read = ucsi_zenbook_read,
>> -	.sync_write = ucsi_acpi_sync_write,
>> -	.async_write = ucsi_acpi_async_write
>> -};
>> +static int ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
>> +				const void *val, size_t val_len)
>> +{
>> +	u64 ctrl = *(u64 *)val;
>> +	int ret;
>> +
>> +	ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
>> +	if (ret && (ctrl & (UCSI_ACK_CC_CI | UCSI_ACK_CONNECTOR_CHANGE))) {
>> +		ctrl= UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
>> +
>> +		dev_dbg(ucsi->dev->parent, "%s: ACK failed\n", __func__);
>> +		ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
>> +	}
> 
> Unfortunately, this has the logic reversed. The quirk (i.e. the
> additional UCSI_ACK_COMMAND_COMPLETE) is required after a _successful_
> UCSI_ACK_CONNECTOR_CHANGE. Otherwise, _subsequent_ commands will timeout
> (usually the next GET_CONNECTOR_CHANGE).
> 
> This means the quirk must be applied _before_ we detect any failure.
> Consequently, the quirk has the potential to break working systems.
> 
> Sorry, if that wasn't clear from my original mail. Please let me know
> if this changes how you want the quirks handled.
> 
>       Thanks    Christian
> 

For the problematic scenario have you tried to play with it a bit to see 
if it's too short of a timeout (raise timeout) or to output the response 
bits to see if anything else surprising is sent?

Does it always fail on the same command, or does it happen to a bunch of 
them?