| Message ID | 20220121201722.3423-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Fix incorrect use of platform profile on AMD platforms | expand |
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@lenovo.com> > --- > 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 > + > + /* 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; Regards, Hans > + } > + > /* > * Check if MMC_GET functionality available > * Version > 6 and return success from MMC_GET command >
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@lenovo.com> >> --- >> 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 > >> + >> + /* 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? Thanks Mark
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@lenovo.com> >>> --- >>> 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
On 1/26/22 15:03, Hans de Goede wrote: > 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@lenovo.com> >>>> --- >>>> 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. > Ah - my bad. I'll correct that. Thanks! Mark
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); + + /* 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; + } + /* * Check if MMC_GET functionality available * Version > 6 and return success from MMC_GET command
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@lenovo.com> --- drivers/platform/x86/thinkpad_acpi.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)