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;
Message-ID: <4de5c2da-5bdd-68fe-fa80-e96f778f7fd2@intel.com>
Date:   Fri, 30 Jun 2023 11:18:37 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM
 capabilities
Content-Language: en-US
To:     "Rafael J. Wysocki" <rafael@kernel.org>
CC:     <linux-acpi@vger.kernel.org>, <andriy.shevchenko@intel.com>,
        <artem.bityutskiy@linux.intel.com>, <mingo@redhat.com>,
        <bp@alien8.de>, <dave.hansen@linux.intel.com>, <hpa@zytor.com>,
        <lenb@kernel.org>, <jgross@suse.com>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>
References: <20230613161034.3496047-1-michal.wilczynski@intel.com>
 <20230613161034.3496047-5-michal.wilczynski@intel.com>
 <CAJZ5v0ig9=5wgQbH__q6BJU=p2uryReS_Lmq7s7HdWoDX6eXsQ@mail.gmail.com>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <CAJZ5v0ig9=5wgQbH__q6BJU=p2uryReS_Lmq7s7HdWoDX6eXsQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aHJaT2gzdG1QR1FUa0QyY2hLNzJMY2t4MUJLTGU5NWdnZ1N2TFNwNmpZUU1C?=
 =?utf-8?B?Qm5rWWJCaEdnUmFNK2ZRczFuMi90NXJaVGJXRnIvRHhNaGJ1VzFvVmlnbHRi?=
 =?utf-8?B?WEpHWno0MjYwYm03bHZHVDFRbFFmbW05M0pwM0NsYnhuZkkvRk0rMVRxOGM5?=
 =?utf-8?B?YUhFMjBLOGNhTWljaFdhaENQVk5WUll0bTRyOER1aXNsOGNrTG1GMjZ2UWtx?=
 =?utf-8?B?cTZsakE1Ry9sZWRwbDd6Wk9WTmtQOVZUUU5xWW1KMTI1UUIzNWJVUGdEWWJ0?=
 =?utf-8?B?ZEFIbnZOYWN6QXZ6cGs4QVpNQ0JEMDFxUjVOUXBYNWRJQy9qaFg0V2RnTUlh?=
 =?utf-8?B?YzZUV2FQQlNId0prSlZOMFE1cExNOUtsN01ndjZVT1VEUzkyZ1dGMHY5U3dK?=
 =?utf-8?B?VURQY0hKV0ZUTlIzSFlDOWlhdFUyQ3IyaG5xb25NV25xaHc1VUhXRmQxd3E1?=
 =?utf-8?B?K3RELzJVNmdSOVpzYmJYMjZoVHppZHFCMG9hUm44NzFBZkFQYmVTWm1KSjJN?=
 =?utf-8?B?eUd6c0FFNlVxeHpHRlFzcXRiV2lxUHBNd0NxZzJtbFpneW5xSDE1SHZFN2tJ?=
 =?utf-8?B?S3RrRGZKek9SYXk0WWYrM09MTVEzWnFxZkNYcm5nVSttYThBZDFVSmUxRVNO?=
 =?utf-8?B?eGExOC93a2x5QTIwREM1eWlWYW1mck1KaVdCTEVyWnRMSWNCa0ozOUpjQjhH?=
 =?utf-8?B?dkJhNld6cjM1cXh0UDV2MTN1bm1LbG0rMk9wcFdwcVJtMndnWjhkMXNYN3RL?=
 =?utf-8?B?eWpqWm54d0U3SUlkc0NuVnRLQWxBc1puLzNkNk1UaERCNXl6NlRHR3o3R2Vz?=
 =?utf-8?B?alN3ZGdVS0JWQjFVWlVDaUtMU0p4ZVBUMVRSd1Fld1FyQW14ZWc2UkNvcEdV?=
 =?utf-8?B?SWFxVU9UVE9YTXMvODRtTkp6WXpzKzBPOHdEUzNsWUdUM2hEYVhrNS9FOTVT?=
 =?utf-8?B?aXlOSGYyWEp2UGhNckJyTG5RTmVHbHFWcDEvVlpNaGczdGlQMkFRaSs3YXRt?=
 =?utf-8?B?enh2dzBsVndMa3p1VU1LK3FpMFF3REgrMlhKK080cldscGw2b1doMHhsNUlk?=
 =?utf-8?B?dWs4RFQ3aDVXMUd5Szlrck5OcE5rMFBTeHRMQndKdXJVcTRyS2pPelVpSjBn?=
 =?utf-8?B?d1dOVHZJMlJGRGV5em1sNndmYVhtQUlzSnR3ZktpOWxWV2VGaGNINDdMYnZK?=
 =?utf-8?B?RjJCSTlGejFRUUh4cDZiNkU5S1hvcWFZM2Q3akFEelpjZkxqRWtMZnpTMElD?=
 =?utf-8?B?dDVyRjRVcGQzcldkaU1yUUVXNGJ5UW5NdkY5cUpEWU1ldXNuWHdDZXhOanpN?=
 =?utf-8?B?MEw1bHJ3Vm9xTnQ2ZjhsMzJxWWJ5NXcwdkMzK2dLelFVcC84cFZUSWl6Um5n?=
 =?utf-8?B?MDdVbjM0T1N5VTFVRUVHSHo5MFBIdDFVQksyeVFBZlFLQ2hpaTArZnVHdG8r?=
 =?utf-8?B?bU9uNTFsSEdkd1M3WXBjWVZtRi9SdnpwSERjNHhZMVQxTkMyUDZJZW9VeWgv?=
 =?utf-8?B?RHpiaU5xUzd4QzhqVUJsamtId1dnSVptQ05xT0gwT1Z6a1JmK0hwVlpZN2JE?=
 =?utf-8?B?TENza3p5ZTBYU3RtbElXYkFjQ2VvMXl0bG9wVXBaTUdkT3ZEbUtGakVSQjBR?=
 =?utf-8?B?T0pIMVpObE40djRHM29obGo3Qkozek1DWjlPTngyZ3ZzSjBwR3QzZm9Oa0NC?=
 =?utf-8?B?cE1CYy9CbXRQRVpKeWdwckVQSDRzV3pyTTZCQzZSMXBDd04vZEpDYi85L2VL?=
 =?utf-8?B?UFZsNms0OVpLbXNNUUhRNVJjc0w1R1hUUUVaVlROZVRXaDU2bTRYWXdQRW43?=
 =?utf-8?B?UGJ5TitKeGV4aysrZVY1Q2dYSVNmd0EwS1F2QWhiTFlZUS9BQ05MZmNKNU5j?=
 =?utf-8?B?SWQ4Ulp6aDhKbkc5QU1qK2ZId3dNZHZXbmowUUlKU21ycXRYYngvUnVHUjFS?=
 =?utf-8?B?bFQxN3E0UnhycDFDZ1RwOXo3ZFFpZlUvMW51ZXpxbDdVczdGeWNGM0M5Q3Fp?=
 =?utf-8?B?UW95UExWeXI5NnR5NWZuY0pXcjdPWDBmajFVMEo2WVJTc0V5UjdwSmRBQUsy?=
 =?utf-8?B?T3QxOVFSN0gxbVQ1ZTZMN09ocUliQnh2bDNBemJnNDNPMUlVK0FsRWxrazNL?=
 =?utf-8?B?ME9FV256Ny82a1UyaGMrM0dTWXgwalBWTk9HYXpndWQvelZjMmtURU9lRi83?=
 =?utf-8?B?R3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6d6e5290-a15a-4709-f6f9-08db794b0084
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2023 09:18:44.3653
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 12E5Xiilly1Pjn4Sf0Os1kaNwB1ime4TchEJmOM/zcLpZ4Fi1Du93S5yYtrx/hjqoRF+056zkcJjjuvsvZMiV0p0WmiERItyzsOe1vW/ihY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4989
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED 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 6/29/2023 4:23 PM, Rafael J. Wysocki wrote:
> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
> <michal.wilczynski@intel.com> wrote:
>> Change acpi_early_processor_osc() to return value in case of the failure.
>> Make it more generic - previously it served only to execute workaround
>> for buggy BIOS in Skylake systems. Now it will walk through ACPI
>> namespace looking for processor objects and will convey OSPM processor
>> capabilities using _OSC method.
>>
>> Prefer using _OSC method over deprecated _PDC in the acpi_bus_init(). In
>> case of the failure of the _OSC, try using  _PDC as a fallback.
>>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/acpi/acpi_processor.c | 23 +++++++++++++----------
>>  drivers/acpi/bus.c            | 13 +++++++++----
>>  drivers/acpi/internal.h       |  9 +--------
>>  3 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 0de0b05b6f53..8965e01406e0 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -669,17 +669,20 @@ static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
>>         return AE_OK;
>>  }
>>
>> -void __init acpi_early_processor_osc(void)
> I would rename this to something like
> acpi_early_processor_control_setup() and would make it attempt to call
> _PDC if _OSC doesn't work.
>
> Then it could remain void and it could be put under a
> CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC #ifdef.

Sure, makes sense to me

>
>> +acpi_status __init acpi_early_processor_osc(void)
>>  {
>> -       if (boot_cpu_has(X86_FEATURE_HWP)) {
>> -               acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
>> -                                   ACPI_UINT32_MAX,
>> -                                   acpi_hwp_native_thermal_lvt_osc,
>> -                                   NULL, NULL, NULL);
>> -               acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
>> -                                acpi_hwp_native_thermal_lvt_osc,
>> -                                NULL, NULL);
>> -       }
>> +       acpi_status status;
>> +
>> +       processor_dmi_check();
>> +
>> +       status = acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
>> +                                    ACPI_UINT32_MAX, acpi_processor_osc, NULL,
>> +                                    NULL, NULL);
>> +       if (ACPI_FAILURE(status))
>> +               return status;
>> +
>> +       return acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_osc,
>> +                               NULL, NULL);
>>  }
>>  #endif
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index d161ff707de4..e8d1f645224f 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1317,9 +1317,6 @@ static int __init acpi_bus_init(void)
>>                 goto error1;
>>         }
>>
>> -       /* Set capability bits for _OSC under processor scope */
>> -       acpi_early_processor_osc();
>> -
>>         /*
>>          * _OSC method may exist in module level code,
>>          * so it must be run after ACPI_FULL_INITIALIZATION
>> @@ -1335,7 +1332,15 @@ static int __init acpi_bus_init(void)
>>
>>         acpi_sysfs_init();
>>
>> -       acpi_early_processor_set_pdc();
>> +#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>> +       status = acpi_early_processor_osc();
>> +       if (ACPI_FAILURE(status)) {
>> +               pr_err("_OSC methods failed, trying _PDC\n");
>> +               acpi_early_processor_set_pdc();
>> +       } else {
>> +               pr_info("_OSC methods ran successfully\n");
>> +       }
>> +#endif
>>
>>         /*
>>          * Maybe EC region is required at bus_scan/acpi_get_devices. So it
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index f979a2f7077c..e7cc41313997 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -151,17 +151,10 @@ int acpi_wakeup_device_init(void);
>>     -------------------------------------------------------------------------- */
>>  #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>>  void acpi_early_processor_set_pdc(void);
>> +acpi_status acpi_early_processor_osc(void);
>>
>>  void processor_dmi_check(void);
>>  bool processor_physically_present(acpi_handle handle);
>> -#else
>> -static inline void acpi_early_processor_set_pdc(void) {}
>> -#endif
>> -
>> -#ifdef CONFIG_X86
>> -void acpi_early_processor_osc(void);
>> -#else
>> -static inline void acpi_early_processor_osc(void) {}
>>  #endif
>>
>>  /* --------------------------------------------------------------------------
>> --