| Message ID | 20250210201656.4165-1-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] platform/x86: thinkpad_acpi: Fix registration of tpacpi platform driver | expand |
On Mon Feb 10, 2025 at 3:16 PM -05, Mark Pearson wrote: > When reviewing and testing the recent platform profile changes I had > missed that they prevent the tpacpi platform driver from registering. > This error is seen in the kernel logs, and the various tpacpi entries > are not created: > [ 7550.642171] platform thinkpad_acpi: Resources present before probing > > This happens because devm_platform_profile_register() is called before > tpacpi_pdev probes (thanks to Kurt Borja for identifying root cause) > > For now revert back to the old platform_profile_register to fix the > issue. Will work on re-implementing this later as more testing is needed > for full solution. If you don't mind I can work on this patch on the for-next branch, so we may use devm_ on the next release cycle to avoid merge conflicts. > > Tested on X1 Carbon G12. > > Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()") > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> Reviewed-by: Kurt Borja <kuurtb@gmail.com> > --- > Changes in v2: > Modified approach to instead revert to old platform_profile_register > method. Will revisit using devm_ version in the future as more testing > needed. > > drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 1fcb0f99695a..85d01bd5dc31 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -10646,8 +10646,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > "DYTC version %d: thermal mode available\n", dytc_version); > > /* Create platform_profile structure and register */ > - tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi", > - NULL, &dytc_profile_ops); > + tpacpi_pprof = platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi-profile", > + NULL, &dytc_profile_ops); > /* > * If for some reason platform_profiles aren't enabled > * don't quit terminally. > @@ -10665,8 +10665,14 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > return 0; > } > > +static void dytc_profile_exit(void) > +{ > + platform_profile_remove(tpacpi_pprof); > +} > + > static struct ibm_struct dytc_profile_driver_data = { > .name = "dytc-profile", > + .exit = dytc_profile_exit, > }; > > /*************************************************************************
Hi Kurt, On Mon, Feb 10, 2025, at 4:07 PM, Kurt Borja wrote: > On Mon Feb 10, 2025 at 3:16 PM -05, Mark Pearson wrote: >> When reviewing and testing the recent platform profile changes I had >> missed that they prevent the tpacpi platform driver from registering. >> This error is seen in the kernel logs, and the various tpacpi entries >> are not created: >> [ 7550.642171] platform thinkpad_acpi: Resources present before probing >> >> This happens because devm_platform_profile_register() is called before >> tpacpi_pdev probes (thanks to Kurt Borja for identifying root cause) >> >> For now revert back to the old platform_profile_register to fix the >> issue. Will work on re-implementing this later as more testing is needed >> for full solution. > > If you don't mind I can work on this patch on the for-next branch, so we > may use devm_ on the next release cycle to avoid merge conflicts. > For sure :) Holler if you need anything. >> >> Tested on X1 Carbon G12. >> >> Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()") >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > > Reviewed-by: Kurt Borja <kuurtb@gmail.com> Thanks for the review Mark > >> --- >> Changes in v2: >> Modified approach to instead revert to old platform_profile_register >> method. Will revisit using devm_ version in the future as more testing >> needed. >> >> drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 1fcb0f99695a..85d01bd5dc31 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -10646,8 +10646,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) >> "DYTC version %d: thermal mode available\n", dytc_version); >> >> /* Create platform_profile structure and register */ >> - tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi", >> - NULL, &dytc_profile_ops); >> + tpacpi_pprof = platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi-profile", >> + NULL, &dytc_profile_ops); >> /* >> * If for some reason platform_profiles aren't enabled >> * don't quit terminally. >> @@ -10665,8 +10665,14 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) >> return 0; >> } >> >> +static void dytc_profile_exit(void) >> +{ >> + platform_profile_remove(tpacpi_pprof); >> +} >> + >> static struct ibm_struct dytc_profile_driver_data = { >> .name = "dytc-profile", >> + .exit = dytc_profile_exit, >> }; >> >> /*************************************************************************
On Mon Feb 10, 2025 at 3:16 PM -05, Mark Pearson wrote: > When reviewing and testing the recent platform profile changes I had > missed that they prevent the tpacpi platform driver from registering. > This error is seen in the kernel logs, and the various tpacpi entries > are not created: > [ 7550.642171] platform thinkpad_acpi: Resources present before probing > > This happens because devm_platform_profile_register() is called before > tpacpi_pdev probes (thanks to Kurt Borja for identifying root cause) > > For now revert back to the old platform_profile_register to fix the > issue. Will work on re-implementing this later as more testing is needed > for full solution. > > Tested on X1 Carbon G12. > > Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()") > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > Changes in v2: > Modified approach to instead revert to old platform_profile_register > method. Will revisit using devm_ version in the future as more testing > needed. > > drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 1fcb0f99695a..85d01bd5dc31 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -10646,8 +10646,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > "DYTC version %d: thermal mode available\n", dytc_version); > > /* Create platform_profile structure and register */ > - tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi", > - NULL, &dytc_profile_ops); > + tpacpi_pprof = platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi-profile", > + NULL, &dytc_profile_ops); > /* > * If for some reason platform_profiles aren't enabled > * don't quit terminally. > @@ -10665,8 +10665,14 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > return 0; > } > > +static void dytc_profile_exit(void) > +{ > + platform_profile_remove(tpacpi_pprof); I just noticed this will be called even if platform_profile_register() fails, as the comment suggests. We need to check for IS_ERR_OR_NULL() before unregistering.
Hi Kurt On Tue, Feb 11, 2025, at 12:18 AM, Kurt Borja wrote: > On Mon Feb 10, 2025 at 3:16 PM -05, Mark Pearson wrote: >> When reviewing and testing the recent platform profile changes I had >> missed that they prevent the tpacpi platform driver from registering. >> This error is seen in the kernel logs, and the various tpacpi entries >> are not created: >> [ 7550.642171] platform thinkpad_acpi: Resources present before probing >> >> This happens because devm_platform_profile_register() is called before >> tpacpi_pdev probes (thanks to Kurt Borja for identifying root cause) >> >> For now revert back to the old platform_profile_register to fix the >> issue. Will work on re-implementing this later as more testing is needed >> for full solution. >> >> Tested on X1 Carbon G12. >> >> Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()") >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> Changes in v2: >> Modified approach to instead revert to old platform_profile_register >> method. Will revisit using devm_ version in the future as more testing >> needed. >> >> drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 1fcb0f99695a..85d01bd5dc31 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -10646,8 +10646,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) >> "DYTC version %d: thermal mode available\n", dytc_version); >> >> /* Create platform_profile structure and register */ >> - tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi", >> - NULL, &dytc_profile_ops); >> + tpacpi_pprof = platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi-profile", >> + NULL, &dytc_profile_ops); >> /* >> * If for some reason platform_profiles aren't enabled >> * don't quit terminally. >> @@ -10665,8 +10665,14 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) >> return 0; >> } >> >> +static void dytc_profile_exit(void) >> +{ >> + platform_profile_remove(tpacpi_pprof); > > I just noticed this will be called even if platform_profile_register() > fails, as the comment suggests. > > We need to check for IS_ERR_OR_NULL() before unregistering. Agreed - will add. Thanks Mark
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 1fcb0f99695a..85d01bd5dc31 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -10646,8 +10646,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) "DYTC version %d: thermal mode available\n", dytc_version); /* Create platform_profile structure and register */ - tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi", - NULL, &dytc_profile_ops); + tpacpi_pprof = platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi-profile", + NULL, &dytc_profile_ops); /* * If for some reason platform_profiles aren't enabled * don't quit terminally. @@ -10665,8 +10665,14 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) return 0; } +static void dytc_profile_exit(void) +{ + platform_profile_remove(tpacpi_pprof); +} + static struct ibm_struct dytc_profile_driver_data = { .name = "dytc-profile", + .exit = dytc_profile_exit, }; /*************************************************************************
When reviewing and testing the recent platform profile changes I had missed that they prevent the tpacpi platform driver from registering. This error is seen in the kernel logs, and the various tpacpi entries are not created: [ 7550.642171] platform thinkpad_acpi: Resources present before probing This happens because devm_platform_profile_register() is called before tpacpi_pdev probes (thanks to Kurt Borja for identifying root cause) For now revert back to the old platform_profile_register to fix the issue. Will work on re-implementing this later as more testing is needed for full solution. Tested on X1 Carbon G12. Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()") Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- Changes in v2: Modified approach to instead revert to old platform_profile_register method. Will revisit using devm_ version in the future as more testing needed. drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)