Received-SPF: pass (google.com: domain of linux-kernel+bounces-176058-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0cdf0af6-851b-4781-83fe-99320c35544f@amd.com>
Date: Fri, 10 May 2024 12:40:05 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the
 namespace root
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 Armin Wolf <W_Armin@gmx.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
 "Rafael J. Wysocki" <rjw@rjwysocki.net>,
 Linux ACPI <linux-acpi@vger.kernel.org>, LKML
 <linux-kernel@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>,
 Heikki Krogerus <heikki.krogerus@linux.intel.com>
References: <5787281.DvuYhMxLoT@kreacher> <4926735.31r3eYUQgx@kreacher>
 <ac04c433-b0ac-4b82-b8eb-98ac16f872d8@gmx.de>
 <CAJZ5v0g_NjGHRvhm-N5vQFnOsqnxExSq99v8n_B_6ANoaCga0w@mail.gmail.com>
 <568291fc-fd79-4f08-9eb7-aed7f5a32345@gmx.de>
 <Zj5ZdcQeaTo9ImT4@smile.fi.intel.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <Zj5ZdcQeaTo9ImT4@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Z1BKbkljNTNBcllYSmxOZkRFWTZwV2hRcUhZbHU2cVpwRnVteHNiZWE1MThX?=
 =?utf-8?B?c0JvbGlnRVJJNmxNUHR0aDlRZGVLK2w3VE80aXRPMXNSblgwcWpSL2YyRE9W?=
 =?utf-8?B?aUpJMkFuNXk5TVVQMkc3aTUrWVFVM1liTTc2TTlCd1FQVWIyVG5WVEo4N3Ux?=
 =?utf-8?B?MkozWXQvV0wxUGg0R2UwbGh5ZG03cGxLam5qdW9LM3BrSWhDRXlZWlhhNFRZ?=
 =?utf-8?B?dzRsU3IwMUFWa3JDL3JFbUhLcmhldFlyWUhDeUlhTlZsZmRNL0xNUzMxNFhp?=
 =?utf-8?B?RlhCblk4UExaTzhYSkcwR1gwWEF6NW82ZmJUeDZYNVd3NkxlTXBzVDBqbnFU?=
 =?utf-8?B?c2I5aFRGbzJqYVpVd3h2dnRselRIbnh1YndvbnV0RzZOWkt2bjh4dzErMDNk?=
 =?utf-8?B?RTZaSjNpM2JCZ012UGZTZ2xtUWRkS29ucml5L2UvUzdlWXJ4TDNDNFlIY0tH?=
 =?utf-8?B?WkJXZCtxdE5tNEMvNlJSbGtMMnN0OExMMEpBeUx2dVh5aWtBYW1DTVJoWHhn?=
 =?utf-8?B?dFJwbkFpbTJZZGhDYmdsMWdrYnlBQm40UWZnSkw0TTRSbFhEY2dadGpGdTRW?=
 =?utf-8?B?RkNNRW5lNWhXUWYraTZGSmxJVTVEKzU3Rjk3STFSVWZIdjVNWGhPOFY3NFB3?=
 =?utf-8?B?RXFrWE9zSlJMb3VERS9uaExINXJkWGxmMXhPejZFOXM3UU9jSjg2c1FCa29I?=
 =?utf-8?B?V2ZpWi9MTFppZC84RGRicDhmMUhiQitXOHlsUmQ3U2NVWkdkNUlkcmFsbHU5?=
 =?utf-8?B?aWJGTElsZVlpR1N4M2VBZytoaTFpUXduQWQzc2FnMkZFek9hZDh5MlVKV0F2?=
 =?utf-8?B?em8zNlBYa0g5WWdyejBzWUNZSkE4Rm14cEFVSTFDOHp5Zjh6dmpnUTZxYUIv?=
 =?utf-8?B?SWQ4dWdKdFIzUnZZUTFYNmtyN0dWVzZrSE1ZeUdrUFVJNDRERDhVSlJqRDgx?=
 =?utf-8?B?Q2FQdzkyeVkzQWN1SXZQSmNRNFZkcFBkOUQzU29JSUEwb3E5QjdlYkVMSHFB?=
 =?utf-8?B?d2N3TVJoM01obzYrWEFxeEl1WVgvNGJyMjJwU21wZjBEeGdRNXZUNlFCbi9T?=
 =?utf-8?B?dytaeWdLL2w4RGl4eGphYzVwV0tialJLSVdDVXV3cUdrR3lDNG9HbG81SDFZ?=
 =?utf-8?B?UUlJdE5Kd2FsQW80NEk4eXNzZFBuNk94Z2FJKzVIRGdxOXF0dWdGOWxuMHZt?=
 =?utf-8?B?Z1pCcjVvWXdwSFRyQ0htalZpU3MvRTVjYWF4UC84QVpITmJwUUxkZnIvL1gv?=
 =?utf-8?B?UXhuMzVOUG5ScmxJTGdPYzNvN0lXbXdPMXl5aUw5UUo0ZWpUcUVGMzFLSndR?=
 =?utf-8?B?MzZ6bzRkSkJMWWJCUlkzTUlmeEY0OTFaK2lpUWd4UGFONVBtUy84QWo5Znhk?=
 =?utf-8?B?ZVlmSzFVMEh4QUJiUTRmSkpoa2RsRXdQUWZHajB2ZkJtMFRLTk1nS0JMY1NR?=
 =?utf-8?B?aEQ2UHZ1czNORmhXL3dKT3d0SEFQaTE5OWI1blpiaU5xazZTUHdOOWJ3S0JS?=
 =?utf-8?B?MDJlOE9pS0UvVDUvNHVIdlRVM2lqQVo3VnNVanFKb3IxOEt5VGtVdk9ZRTRB?=
 =?utf-8?B?dmtYNjAvdjNQY3JZa0FTY01oQUFjaVZ5NExvN1BacjNVdFpFWHR5aEwwenl4?=
 =?utf-8?B?aWtkUHh3QUd6akdzODJURkRiN3ZRV3dPN0lrYmRQNG16NVQ2SmZMV0QzY1kx?=
 =?utf-8?B?Q0xpeURnZzZmcnlIOEUzWDVsZ1pzdGVtMTVsRDIyQjZKYjJaTjZPVjNQT3BX?=
 =?utf-8?B?UW1ZUzg1ZXJuK21TUTVrc3VCT2FicDZFcS82MGQwdDl5dGs1MmFEc0FCNVdx?=
 =?utf-8?B?UjdVOWJnRlQ2MmlOSTlmcTNlWjY2T2ZSaHFWaENqV0VlamdPd2pFYlplQVFQ?=
 =?utf-8?B?Z0kydVpGTTVpQnZ6QTl3V2pxbEp6RVVCZUYrVlFtdEhBcmNqYkxzR05lMWY0?=
 =?utf-8?B?TFNrZGNobm1NRWZIV29ldmtSaXRTRUNkZ2o4OGhyZ3Z3ZjBPTndhd0xhQi9C?=
 =?utf-8?B?a25BWEZDbStaQzJSL3I5eFhaTXkwUHFJTFR2OTRCVisvOXFNcWcya0crYzBN?=
 =?utf-8?B?ZGowY0hrZ3hZTENMM2FFa2NwUkZkaitZUlpmbzdFYmg1WlZTQWt3YnFWUjBx?=
 =?utf-8?Q?B/qR0rI5NpnodSAtMnpCy+xfG?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ef309e5-2eb8-4dd6-c855-08dc71183bce
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2024 17:40:07.7728
 (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: 6VxOmnMbpcO4F1TG/MeLoGivrIK4w6WfJVlMbEigK48Z9+fnwrDsw6E5Fy/RvBso4pWA//jX83PV1NgaMC3qFA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB6127

On 5/10/2024 12:29, Andy Shevchenko wrote:
> On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
>> Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
>>> On Fri, May 10, 2024 at 6:10 PM Armin Wolf <W_Armin@gmx.de> wrote:
>>>> Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
>>>>
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
>>>>> IdeaPad Pro 5 due to a missing address space handler for the EC address
>>>>> space:
>>>>>
>>>>>     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
>>>>>
>>>>> This happens because the EC driver only registers the EC address space
>>>>> handler for operation regions defined in the EC device scope of the
>>>>> ACPI namespace while the operation region being accessed by the _DSM
>>>>> in question is located beyond that scope.
>>>>>
>>>>> To address this, modify the ACPI EC driver to install the EC address
>>>>> space handler at the root of the ACPI namespace.
>>>>>
>>>>> Note that this change is consistent with some examples in the ACPI
>>>>> specification in which EC operation regions located outside the EC
>>>>> device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
>>>>> so the current behavior of the EC driver is arguably questionable.
>>>> Hi,
>>>>
>>>> the patch itself looks good to me, but i wonder what happens if multiple
>>>> ACPI EC devices are present. How would we handle such a situation?
>>> I'm wondering if this is a theoretical question or do you have any
>>> existing or planned systems in mind?
>>>
>>> ec_read(), ec_write() and ec_transaction() use only the first EC that
>>> has been found anyway.
>>
>> Its a theoretical question, i do not know of any systems which have more than
>> one ACPI EC device.
> 
> The specification is clear about this case in the "ACPI Embedded Controller
> Interface Specification":
> 
>   "The ACPI standard supports multiple embedded controllers in a system,
>    each with its own resources. Each embedded controller has a flat
>    byte-addressable I/O space, currently defined as 256 bytes."
> 
> However, I haven't checked deeper, so it might be a leftover in the documentation.
> 
> The OperationRegion() has no reference to the EC (or in general, device) which
> we need to speak to. The only possibility to declare OpRegion() for the second+
> EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> RegionSpace.
> 
> That said, the commit message might be extended to summarize this, but at
> the same time I see no way how this series can break anything even in 2+ ECs
> environments.

It's deviating from the patch, but in practice /why/ would you even want 
to have a design with two ECs?  In general that is going to mean a much 
more complex state machine with synchronizing the interaction between 
both of them and the host.

Understanding the benefit of such a design might make it easier to 
hypothesize impacts.

> 
>> This patch would prevent any ACPI ECs other than the first one from probing,
>> since they would fail to register their address space handler.
>> I am just curious if/how we want to handle such situations.
>