| Message ID | 20241028020131.8031-6-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
On Sun, 27 Oct 2024, Mario Limonciello wrote: > The sanity check that the platform handler had choices set doesn't > need the mutex taken. Move it to earlier in the registration. > > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/platform_profile.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 0c60fc970b6e8..c20256bb39579 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof) > { > int err; > > + /* Sanity check the profile handler field are set */ > + if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) || > + !pprof->profile_set || !pprof->profile_get) { While moving the line, please change the misleading indentation too (you'd have probably done it based on my comment on v1 too but since this series has changed things a bit, I'm noting it also against this patch).
On 10/29/2024 05:12, Ilpo Järvinen wrote: > On Sun, 27 Oct 2024, Mario Limonciello wrote: > >> The sanity check that the platform handler had choices set doesn't >> need the mutex taken. Move it to earlier in the registration. >> >> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/acpi/platform_profile.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> index 0c60fc970b6e8..c20256bb39579 100644 >> --- a/drivers/acpi/platform_profile.c >> +++ b/drivers/acpi/platform_profile.c >> @@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof) >> { >> int err; >> >> + /* Sanity check the profile handler field are set */ >> + if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) || >> + !pprof->profile_set || !pprof->profile_get) { > > While moving the line, please change the misleading indentation too > (you'd have probably done it based on my comment on v1 too but since > this series has changed things a bit, I'm noting it also against this > patch). > Ack, thanks!
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 0c60fc970b6e8..c20256bb39579 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -180,6 +180,12 @@ int platform_profile_register(struct platform_profile_handler *pprof) { int err; + /* Sanity check the profile handler field are set */ + if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) || + !pprof->profile_set || !pprof->profile_get) { + return -EINVAL; + } + mutex_lock(&profile_lock); /* We can only have one active profile */ if (cur_profile) { @@ -187,13 +193,6 @@ int platform_profile_register(struct platform_profile_handler *pprof) return -EEXIST; } - /* Sanity check the profile handler field are set */ - if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) || - !pprof->profile_set || !pprof->profile_get) { - mutex_unlock(&profile_lock); - return -EINVAL; - } - err = sysfs_create_group(acpi_kobj, &platform_profile_group); if (err) { mutex_unlock(&profile_lock);