Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <d574a393-1512-41ec-9fb0-ca747d6bd7c8@amd.com>
Date:   Tue, 28 Nov 2023 08:27:57 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [RFT][PATCH v1] ACPI: OSL: Use a threaded interrupt handler for
 SCI
Content-Language: en-US
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Michal Wilczynski <michal.wilczynski@intel.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>
References: <5745568.DvuYhMxLoT@kreacher>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <5745568.DvuYhMxLoT@kreacher>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YXZKZ3JCRUpVblRtZHVqWUt3dWhBaUtiOFliYXEzVVhhQWdEOEMxb05RQzNJ?=
 =?utf-8?B?MVVJUzlGV3Q0S2VWUERjUEE5SzkwRDZUZzhMVWF1Mm80azE0Sk1WNGgvZEQ3?=
 =?utf-8?B?WlBOUjBRTkNnakJmNmpIVTFkWHRTRnlmUGpWRmJHV3NOeCt5ME9VTGVwUTRm?=
 =?utf-8?B?aTZkajhiR2JSTHdZcGJoWWxJUGhqN2JSdzN6cWdMVTdsRVVXbjF4ZEw0N2pk?=
 =?utf-8?B?aVAvT2YvcGFlMHVoZGMzZU5XTkxVTE9jR0RmUHZJRVhqTzlYMmd3cmM2Zkxv?=
 =?utf-8?B?Z2JLTUZvcUZSb21kSjR0dHNoVm1QaThRSE1oUTE5NURaZEJuVDRYcmoxMWJQ?=
 =?utf-8?B?a0hWREpPeEh5WXNpcjl5YXZqcnZZQkErYktQRXRPVzlreUs1RUxnWGo1Y3Vx?=
 =?utf-8?B?M214SkNwdHhLais3WlhhZXJnSkdyZmROZDdZbk5RaExlRlEvNTFGSEFYWjJ3?=
 =?utf-8?B?a0hJWDZta2F4VnNNeTdZaEpibkRUMnlKSXliY3FjNGV5c0F6SDJMN3FwVHE0?=
 =?utf-8?B?eG9UUmFpd1BvQXArUFc2RW91WGJWbFlvTldVUjF3U2VPRERndS9scWZpdGdE?=
 =?utf-8?B?MWRQbW00VFc4OER2TzR4WUd0R2FLeCtMNUVCa1NtMHA2RS9UNUN0V1dMYmZy?=
 =?utf-8?B?NWFUV2JDSWU5MHgveFg1Ym1mZDFneG5QUm9zOW9ycUhjRTNsYmpFTGVDNTFE?=
 =?utf-8?B?RERuaEpwUklKaENjZms0STBIRTRrbjVLMUtnOXdBaWtxY3ZXWU9oTGVWTTAx?=
 =?utf-8?B?Zkt6SENTNUxSNHNOalRqUDAxWlFSY2FxZkNLTnVCUG94Y0pBV2pBWWprVlVp?=
 =?utf-8?B?OTF3djUwcEdFYW5YckhxcTExOUVZTGtUdGRDTjVaTk5IbS9IWGpsYXhOb21h?=
 =?utf-8?B?cDBSTUdaTFNEZE9ZRjNLcURsVXVyelFiTHdYWWJnQ3R5WEZ4em9idTlqZENz?=
 =?utf-8?B?SUZHc3pFMEdhdUU4c1BkSHRyZSttWGxqNGxrTmw0cURDV3daamNmbSsrRENN?=
 =?utf-8?B?R3F0TjV6SGhiUkxIT1ZkQjlmZzJkeXQ5bUU4SFVFTHNnTkxWcEl3dXdxT3J6?=
 =?utf-8?B?aE96L0Z6ekhreTRVcGFxRzg3U0diQUZGWDRYVGxWS1ZZc1dIVTFzU2pmdlJn?=
 =?utf-8?B?eVNhR28yd2RONUxETEhDYUd6Zm9XMGpuT0VvMHYvL2xYN1BObVdOMzhqSEJP?=
 =?utf-8?B?cUJVSTdLQlJON2JXaWhkNy9GL3VyNnAybUIxUDF0eEdvSlU0R3MrRHk3SFdH?=
 =?utf-8?B?Vk9WY0dUTFBSUDFHREJBZFNKbnF6RGx3UjJRZVlZdG4zenBkZ0llMFI2YUdQ?=
 =?utf-8?B?eVZCVDNyd1BEWi9LbnlHNmFWclBPVFIwcWNSTWtLbVFwTU0xdFJsK3FmL05h?=
 =?utf-8?B?NDQ1cGpHNnVPZVI4N2R3djRUQmQxMVcxNEk4eUQvWStlWVBBRStZeFkvS1Uv?=
 =?utf-8?B?OEEybzVxR01IQmU0MXlVdzZMQ0xUOUFvMUxWS1RNUFdUWUlzZXFJVFFOcElx?=
 =?utf-8?B?bk9vVFkvR0haOFlBa2V4RW9lTzdybkovc1RlNTZ3OGpVMDBpYU9yd0xSeGlT?=
 =?utf-8?B?cFlxNUdDbWdqaTJlUU5mM3dUaEI4VHdaYUJFS3Y0WmZPL213ZnN5SncwTTBK?=
 =?utf-8?B?enM2UVh6cXBqVmVnUWJ2RDB3RDlpQ0YzV3J3dDdVMWN1WjMzUmJwdWF3UkFW?=
 =?utf-8?B?Yk1TTS9CTmdVckhOV3RaenZsL1JyRDFualp0WGUrT3V1NHJzcDNDOW8wRzh1?=
 =?utf-8?B?dy9WMGlYbG9hSTRxQXhRa2pRUFh2Z3NtQnpmY0MxK0FsNUN5SnNlcnJsaW9s?=
 =?utf-8?B?c3IzZVVON0dUUThhZS9WdHg3bkJTT0N0WUhjMUpBSzhBQm0vQUUyTm0rbnYy?=
 =?utf-8?B?Z0FQZStmV3U3TkhWVU5pMXdxankwdlVETENSclVDTlFhUG5iVWlVTzdPK1lJ?=
 =?utf-8?B?K2NxNVBGb2J1d2tZVFpaYmJHU3ZSWmY2QnNMOXE1bmZvSUZuclUwL3FMZWFL?=
 =?utf-8?B?Q0NIVkp5TGM5R3RjaTQ4WGcxdGJ2bHc4eFhrWHlFN1dNd29pMGJOdEJYa3dZ?=
 =?utf-8?B?Qlc4TDd1UzcwMzJUVklPcUV4ZndHc2RLMWNwRDcwd0NKQWhuTEdvSksyblRr?=
 =?utf-8?Q?rJcBYLAmC34ZYaHEkyrQwsP3C?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 501ea755-0a24-4fb1-4f78-08dbf01e3965
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Nov 2023 14:28:00.7191
 (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: f0zFzsumLvDMGJOdoNaostOUb85tk8857dw9saHGtfZGX2fT5EzZagOSgMHtvrqpLjn/PRDG9O0Sq/jkqh8KGw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB8173
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Tue, 28 Nov 2023 06:28:19 -0800 (PST)

On 11/27/2023 13:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In the current arrangement, all of the acpi_ev_sci_xrupt_handler() code
> is run as an interrupt handler for the SCI, in interrupt context.  Among
> other things, this causes it to run with local interrupts off which
> can be problematic if many GPEs are enabled and they are located in the
> I/O address space, for example (because in that case local interrupts
> will be off for the duration of all of the GPE hardware accesses carried
> out while handling an SCI combined and that may be quite a bit of time
> in extreme scenarios).
> 
> However, there is no particular reason why the code in question really
> needs to run in interrupt context and in particular, it has no specific
> reason to run with local interrupts off.  The only real requirement is
> to prevent multiple instences of it from running in parallel with each
> other, but that can be achieved regardless.
> 
> For this reason, use request_threaded_irq() instead of request_irq() for
> the ACPI SCI and pass IRQF_ONESHOT to it in flags to indicate that the
> interrupt needs to be masked while its handling thread is running so as
> to prevent it from re-triggering while it is being handled (and in
> particular until the final handled/not handled outcome is determined).
> 
> While at it, drop a redundant local variable from acpi_irq().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> The code inspection and (necessarily limited) testing carried out by me
> are good indications that this should just always work, but there may
> be still some really odd platform configurations I'm overlooking, so if
> you have a way to give it a go, please do so.

Thanks for looping me in.

I tested it on a few different laptops I have on hand from different SoC 
generations and manufacturers and ensured that SCI was working correctly 
for usage and wakeup.  My base kernel was 6.7-rc3 plus some unrelated 
RTC timeout handling patches.

Tested-by: Mario Limonciello <mario.limonciello@amd.com>

> 
> ---
>   drivers/acpi/osl.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/acpi/osl.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/osl.c
> +++ linux-pm/drivers/acpi/osl.c
> @@ -544,11 +544,7 @@ acpi_os_predefined_override(const struct
>   
>   static irqreturn_t acpi_irq(int irq, void *dev_id)
>   {
> -	u32 handled;
> -
> -	handled = (*acpi_irq_handler) (acpi_irq_context);
> -
> -	if (handled) {
> +	if ((*acpi_irq_handler)(acpi_irq_context)) {
>   		acpi_irq_handled++;
>   		return IRQ_HANDLED;
>   	} else {
> @@ -582,7 +578,8 @@ acpi_os_install_interrupt_handler(u32 gs
>   
>   	acpi_irq_handler = handler;
>   	acpi_irq_context = context;
> -	if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) {
> +	if (request_threaded_irq(irq, NULL, acpi_irq, IRQF_SHARED | IRQF_ONESHOT,
> +			         "acpi", acpi_irq)) {
>   		pr_err("SCI (IRQ%d) allocation failed\n", irq);
>   		acpi_irq_handler = NULL;
>   		return AE_NOT_ACQUIRED;
> 
> 
>