Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: Fix incorrect use of platform profile on AMD platforms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 1/26/22 17:49, Mark Pearson wrote:
> 
> On 1/24/22 05:42, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 1/21/22 21:17, Mark Pearson wrote:
>>> Lenovo AMD based platforms have been offering platform_profiles but they
>>> are not working correctly. This is because the mode we are using on the
>>> Intel platforms (MMC) is not available on the AMD platforms.
>>>
>>> This commit adds checking of the functional capabilities returned by the
>>> BIOS to confirm if MMC is supported or not. Profiles will not be
>>> available if the platform is not MMC capable.
>>>
>>> I'm investigating and working on an alternative for AMD platforms but
>>> that is still work-in-progress.
>>>
>>> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
>>> ---
>>>   drivers/platform/x86/thinkpad_acpi.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index 9c632df734bb..42a04e44135b 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -10026,6 +10026,9 @@ static struct ibm_struct proxsensor_driver_data = {
>>>   #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
>>>   #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>>>   +#define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
>>> +#define DYTC_FC_MMC           27 /* MMC Mode supported */
>>> +
>>>   #define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
>>>   #define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
>>>   @@ -10253,6 +10256,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>>       if (dytc_version >= 5) {
>>>           dbg_printk(TPACPI_DBG_INIT,
>>>                   "DYTC version %d: thermal mode available\n", dytc_version);
>> This code has been refactored and this will not apply as is:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86/thinkpad_acpi.c?id=0b0d2fba4f3302b601c429c9286e66b3af2d29cb
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86/thinkpad_acpi.c?id=798682e236893a20e5674de02ede474373dd342d
>>
>> Please rebase on top of 5.17-rc1
> My apologies - I thought I was on the latest. Will rebase

No problem.


>>
>>> +
>>> +        /* Check what capabilities are supported. Currently MMC is needed */
>>> +        err = dytc_command(DYTC_CMD_FUNC_CAP, &output);
>>> +        if (err)
>>> +            return err;
>>> +        if (!test_bit(DYTC_FC_MMC, (void *)&output)) {
>>> +            dbg_printk(TPACPI_DBG_INIT, " DYTC MMC mode not supported\n");
>>> +            return 0;
>> This should be return -ENODEV;
> 
> I'll double check, but I don't think we want an error here as we want to continue on, it's just the feature is not supported so 0 is OK?

-ENODEV is treated as "feature not supported" and will not cause any
errors be printed and probing will continue normally with an -ENODEV
error.

Where as 0 will still cause the corresponding exit function to get
called on module unload, causing platform_profile_remove() to get
called even though we never registered the platform_profile handler.
So -ENODEV is better.

Regards,

Hans




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux