* [PATCH 0/3] ACPI: Remove unnecessary acpi_has_method() calls
@ 2019-07-22 2:35 Kelsey Skunberg
2019-07-22 2:35 ` [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c Kelsey Skunberg
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Kelsey Skunberg @ 2019-07-22 2:35 UTC (permalink / raw)
To: rjw, lenb, linux-acpi, linux-kernel
Cc: skhan, linux-kernel-mentees, bjorn, skunberg.kelsey
Remove acpi_has_method() calls which return an error a following call will
provide anyways. Removing the unnecessary acpi_has_method() calls help to
clean up code and remove extra work.
Kelsey Skunberg (3):
ACPI: Remove acpi_has_method() call from acpi_adxl.c
ACPI: Remove acpi_has_method() call from scan.c
ACPI: Remove acpi_has_method() calls from thermal.c
drivers/acpi/acpi_adxl.c | 5 -----
drivers/acpi/scan.c | 3 ---
drivers/acpi/thermal.c | 11 +++--------
3 files changed, 3 insertions(+), 16 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c 2019-07-22 2:35 [PATCH 0/3] ACPI: Remove unnecessary acpi_has_method() calls Kelsey Skunberg @ 2019-07-22 2:35 ` Kelsey Skunberg 2019-07-22 8:31 ` Rafael J. Wysocki 2019-07-22 2:35 ` [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 3/3] ACPI: Remove acpi_has_method() calls from thermal.c Kelsey Skunberg 2 siblings, 1 reply; 9+ messages in thread From: Kelsey Skunberg @ 2019-07-22 2:35 UTC (permalink / raw) To: rjw, lenb, linux-acpi, linux-kernel Cc: skhan, linux-kernel-mentees, bjorn, skunberg.kelsey acpi_check_dsm() will already return an error if the DSM method does not exist. Checking if the DSM method exists before the acpi_check_dsm() call is not needed. Remove acpi_has_method() call to avoid additional work. Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> --- drivers/acpi/acpi_adxl.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c index 13c8f7b50c46..89aac15663fd 100644 --- a/drivers/acpi/acpi_adxl.c +++ b/drivers/acpi/acpi_adxl.c @@ -148,11 +148,6 @@ static int __init adxl_init(void) return -ENODEV; } - if (!acpi_has_method(handle, "_DSM")) { - pr_info("No DSM method\n"); - return -ENODEV; - } - if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, ADXL_IDX_GET_ADDR_PARAMS | ADXL_IDX_FORWARD_TRANSLATE)) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c 2019-07-22 2:35 ` [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c Kelsey Skunberg @ 2019-07-22 8:31 ` Rafael J. Wysocki 2019-07-22 18:29 ` Bjorn Helgaas 0 siblings, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2019-07-22 8:31 UTC (permalink / raw) To: Kelsey Skunberg Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List, Linux Kernel Mailing List, skhan, linux-kernel-mentees, bjorn On Mon, Jul 22, 2019 at 4:36 AM Kelsey Skunberg <skunberg.kelsey@gmail.com> wrote: > > acpi_check_dsm() will already return an error if the DSM method does not > exist. Checking if the DSM method exists before the acpi_check_dsm() call > is not needed. Remove acpi_has_method() call to avoid additional work. > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> > --- > drivers/acpi/acpi_adxl.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c > index 13c8f7b50c46..89aac15663fd 100644 > --- a/drivers/acpi/acpi_adxl.c > +++ b/drivers/acpi/acpi_adxl.c > @@ -148,11 +148,6 @@ static int __init adxl_init(void) > return -ENODEV; > } > > - if (!acpi_has_method(handle, "_DSM")) { > - pr_info("No DSM method\n"); And why is printing the message not useful? > - return -ENODEV; > - } > - > if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, > ADXL_IDX_GET_ADDR_PARAMS | > ADXL_IDX_FORWARD_TRANSLATE)) { > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c 2019-07-22 8:31 ` Rafael J. Wysocki @ 2019-07-22 18:29 ` Bjorn Helgaas 2019-09-02 21:08 ` Rafael J. Wysocki 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2019-07-22 18:29 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Kelsey Skunberg, Rafael J. Wysocki, Len Brown, ACPI Devel Mailing List, Linux Kernel Mailing List, skhan, linux-kernel-mentees, bjorn, Tony Luck, Borislav Petkov [+cc Tony (original author), Borislav (merged original patch)] On Mon, Jul 22, 2019 at 10:31:11AM +0200, Rafael J. Wysocki wrote: > On Mon, Jul 22, 2019 at 4:36 AM Kelsey Skunberg > <skunberg.kelsey@gmail.com> wrote: > > > > acpi_check_dsm() will already return an error if the DSM method does not > > exist. Checking if the DSM method exists before the acpi_check_dsm() call > > is not needed. Remove acpi_has_method() call to avoid additional work. > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> > > --- > > drivers/acpi/acpi_adxl.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c > > index 13c8f7b50c46..89aac15663fd 100644 > > --- a/drivers/acpi/acpi_adxl.c > > +++ b/drivers/acpi/acpi_adxl.c > > @@ -148,11 +148,6 @@ static int __init adxl_init(void) > > return -ENODEV; > > } > > > > - if (!acpi_has_method(handle, "_DSM")) { > > - pr_info("No DSM method\n"); > > And why is printing the message not useful? > > > - return -ENODEV; > > - } > > - > > if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, > > ADXL_IDX_GET_ADDR_PARAMS | > > ADXL_IDX_FORWARD_TRANSLATE)) { The next line of context (not included in the patch): pr_info("DSM method does not support forward translate\n"); IMHO kernel messages that are just a constant string, with no context or variable part (device ID, path, error code, etc) are questionable in general. Is there any dev_printk()-like thing that takes an acpi_handle? Seems like that would be useful for cases like this. This message *does* include an "ADXL: " prefix (from the pr_fmt definition), and from reading the code you can see that the only possible method is "\_SB.ADXL._DSM". There's nothing an end user can do with these messages, so I suspect their value is for debugging during platform bringup, and it would be sufficient to drop the first one (as Kelsey's patch does) and change the second one like this: - pr_info("DSM method does not support forward translate\n"); + pr_info("%s DSM missing or does not support forward translate\n", + path); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c 2019-07-22 18:29 ` Bjorn Helgaas @ 2019-09-02 21:08 ` Rafael J. Wysocki 2019-09-05 4:11 ` Kelsey Skunberg 0 siblings, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2019-09-02 21:08 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Kelsey Skunberg, Rafael J. Wysocki, Len Brown, ACPI Devel Mailing List, Linux Kernel Mailing List, skhan, linux-kernel-mentees, bjorn, Tony Luck, Borislav Petkov Sorry for the delayed reply. On Mon, Jul 22, 2019 at 8:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Tony (original author), Borislav (merged original patch)] > > On Mon, Jul 22, 2019 at 10:31:11AM +0200, Rafael J. Wysocki wrote: > > On Mon, Jul 22, 2019 at 4:36 AM Kelsey Skunberg > > <skunberg.kelsey@gmail.com> wrote: > > > > > > acpi_check_dsm() will already return an error if the DSM method does not > > > exist. Checking if the DSM method exists before the acpi_check_dsm() call > > > is not needed. Remove acpi_has_method() call to avoid additional work. > > > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> > > > --- > > > drivers/acpi/acpi_adxl.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c > > > index 13c8f7b50c46..89aac15663fd 100644 > > > --- a/drivers/acpi/acpi_adxl.c > > > +++ b/drivers/acpi/acpi_adxl.c > > > @@ -148,11 +148,6 @@ static int __init adxl_init(void) > > > return -ENODEV; > > > } > > > > > > - if (!acpi_has_method(handle, "_DSM")) { > > > - pr_info("No DSM method\n"); > > > > And why is printing the message not useful? > > > > > - return -ENODEV; > > > - } > > > - > > > if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, > > > ADXL_IDX_GET_ADDR_PARAMS | > > > ADXL_IDX_FORWARD_TRANSLATE)) { > > The next line of context (not included in the patch): > > pr_info("DSM method does not support forward translate\n"); > > IMHO kernel messages that are just a constant string, with no context > or variable part (device ID, path, error code, etc) are questionable > in general. Is there any dev_printk()-like thing that takes an > acpi_handle? Seems like that would be useful for cases like this. > > This message *does* include an "ADXL: " prefix (from the pr_fmt > definition), and from reading the code you can see that the only > possible method is "\_SB.ADXL._DSM". > > There's nothing an end user can do with these messages, so I suspect > their value is for debugging during platform bringup, and it would be > sufficient to drop the first one (as Kelsey's patch does) and change > the second one like this: > > - pr_info("DSM method does not support forward translate\n"); > + pr_info("%s DSM missing or does not support forward translate\n", > + path); You have a point, but then I would expect the changelog to mention that. As it stands, the patch does more than the changelog says, which isn't nice. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c 2019-09-02 21:08 ` Rafael J. Wysocki @ 2019-09-05 4:11 ` Kelsey Skunberg 0 siblings, 0 replies; 9+ messages in thread From: Kelsey Skunberg @ 2019-09-05 4:11 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Rafael J. Wysocki, Len Brown, ACPI Devel Mailing List, Linux Kernel Mailing List, skhan, linux-kernel-mentees, bjorn, Tony Luck, Borislav Petkov On Mon, Sep 02, 2019 at 11:08:08PM +0200, Rafael J. Wysocki wrote: > Sorry for the delayed reply. > > On Mon, Jul 22, 2019 at 8:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > [+cc Tony (original author), Borislav (merged original patch)] > > > > On Mon, Jul 22, 2019 at 10:31:11AM +0200, Rafael J. Wysocki wrote: > > > On Mon, Jul 22, 2019 at 4:36 AM Kelsey Skunberg > > > <skunberg.kelsey@gmail.com> wrote: > > > > > > > > acpi_check_dsm() will already return an error if the DSM method does not > > > > exist. Checking if the DSM method exists before the acpi_check_dsm() call > > > > is not needed. Remove acpi_has_method() call to avoid additional work. > > > > > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> > > > > --- > > > > drivers/acpi/acpi_adxl.c | 5 ----- > > > > 1 file changed, 5 deletions(-) > > > > > > > > diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c > > > > index 13c8f7b50c46..89aac15663fd 100644 > > > > --- a/drivers/acpi/acpi_adxl.c > > > > +++ b/drivers/acpi/acpi_adxl.c > > > > @@ -148,11 +148,6 @@ static int __init adxl_init(void) > > > > return -ENODEV; > > > > } > > > > > > > > - if (!acpi_has_method(handle, "_DSM")) { > > > > - pr_info("No DSM method\n"); > > > > > > And why is printing the message not useful? > > > > > > > - return -ENODEV; > > > > - } > > > > - > > > > if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, > > > > ADXL_IDX_GET_ADDR_PARAMS | > > > > ADXL_IDX_FORWARD_TRANSLATE)) { > > > > The next line of context (not included in the patch): > > > > pr_info("DSM method does not support forward translate\n"); > > > > IMHO kernel messages that are just a constant string, with no context > > or variable part (device ID, path, error code, etc) are questionable > > in general. Is there any dev_printk()-like thing that takes an > > acpi_handle? Seems like that would be useful for cases like this. > > > > This message *does* include an "ADXL: " prefix (from the pr_fmt > > definition), and from reading the code you can see that the only > > possible method is "\_SB.ADXL._DSM". > > > > There's nothing an end user can do with these messages, so I suspect > > their value is for debugging during platform bringup, and it would be > > sufficient to drop the first one (as Kelsey's patch does) and change > > the second one like this: > > > > - pr_info("DSM method does not support forward translate\n"); > > + pr_info("%s DSM missing or does not support forward translate\n", > > + path); > > You have a point, but then I would expect the changelog to mention that. > > As it stands, the patch does more than the changelog says, which isn't nice. You're right, the changelog should include this information. I'll get an updated version made. Thank you for getting back. -Kelsey ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c 2019-07-22 2:35 [PATCH 0/3] ACPI: Remove unnecessary acpi_has_method() calls Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c Kelsey Skunberg @ 2019-07-22 2:35 ` Kelsey Skunberg 2019-07-22 8:29 ` Rafael J. Wysocki 2019-07-22 2:35 ` [PATCH 3/3] ACPI: Remove acpi_has_method() calls from thermal.c Kelsey Skunberg 2 siblings, 1 reply; 9+ messages in thread From: Kelsey Skunberg @ 2019-07-22 2:35 UTC (permalink / raw) To: rjw, lenb, linux-acpi, linux-kernel Cc: skhan, linux-kernel-mentees, bjorn, skunberg.kelsey acpi_evaluate_reference() will return an error if the DEP method does not exist. Checking if the DEP method exists before the acpi_evaluate_reference() call is not needed. Remove acpi_has_method() call to avoid additional work. Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> --- drivers/acpi/scan.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 0e28270b0fd8..4f2b0b481cee 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1803,9 +1803,6 @@ static void acpi_device_dep_initialize(struct acpi_device *adev) adev->dep_unmet = 0; - if (!acpi_has_method(adev->handle, "_DEP")) - return; - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, &dep_devices); if (ACPI_FAILURE(status)) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c 2019-07-22 2:35 ` [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c Kelsey Skunberg @ 2019-07-22 8:29 ` Rafael J. Wysocki 0 siblings, 0 replies; 9+ messages in thread From: Rafael J. Wysocki @ 2019-07-22 8:29 UTC (permalink / raw) To: Kelsey Skunberg Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List, Linux Kernel Mailing List, skhan, linux-kernel-mentees, bjorn On Mon, Jul 22, 2019 at 4:36 AM Kelsey Skunberg <skunberg.kelsey@gmail.com> wrote: > > acpi_evaluate_reference() will return an error if the DEP method > does not exist. Checking if the DEP method exists before the > acpi_evaluate_reference() call is not needed. Remove acpi_has_method() > call to avoid additional work. > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> > --- > drivers/acpi/scan.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 0e28270b0fd8..4f2b0b481cee 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -1803,9 +1803,6 @@ static void acpi_device_dep_initialize(struct acpi_device *adev) > > adev->dep_unmet = 0; > > - if (!acpi_has_method(adev->handle, "_DEP")) > - return; > - As you can see, this just returns. > status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > &dep_devices); > if (ACPI_FAILURE(status)) { Where this does something more on fallure. Therefore the code after the change is not equivalent to the code before it, so the changes is questionable. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] ACPI: Remove acpi_has_method() calls from thermal.c 2019-07-22 2:35 [PATCH 0/3] ACPI: Remove unnecessary acpi_has_method() calls Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c Kelsey Skunberg @ 2019-07-22 2:35 ` Kelsey Skunberg 2 siblings, 0 replies; 9+ messages in thread From: Kelsey Skunberg @ 2019-07-22 2:35 UTC (permalink / raw) To: rjw, lenb, linux-acpi, linux-kernel Cc: skhan, linux-kernel-mentees, bjorn, skunberg.kelsey The following acpi_has_method() calls are unnecessary since acpi_execute_simple_method() and acpi_evaluate_reference() will return an error if the given method does not exist. Remove acpi_has_method() calls to avoid additional work. Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> --- drivers/acpi/thermal.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 00f12a86ecbd..d831a61e0010 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -225,13 +225,9 @@ static int acpi_thermal_set_cooling_mode(struct acpi_thermal *tz, int mode) if (!tz) return -EINVAL; - if (!acpi_has_method(tz->device->handle, "_SCP")) { - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "_SCP not present\n")); + if (ACPI_FAILURE(acpi_execute_simple_method(tz->device->handle, + "_SCP", mode))) return -ENODEV; - } else if (ACPI_FAILURE(acpi_execute_simple_method(tz->device->handle, - "_SCP", mode))) { - return -ENODEV; - } return 0; } @@ -463,8 +459,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) break; } - if ((flag & ACPI_TRIPS_DEVICES) - && acpi_has_method(tz->device->handle, "_TZD")) { + if (flag & ACPI_TRIPS_DEVICES) { memset(&devices, 0, sizeof(devices)); status = acpi_evaluate_reference(tz->device->handle, "_TZD", NULL, &devices); -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-05 4:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-22 2:35 [PATCH 0/3] ACPI: Remove unnecessary acpi_has_method() calls Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 1/3] ACPI: Remove acpi_has_method() call from acpi_adxl.c Kelsey Skunberg 2019-07-22 8:31 ` Rafael J. Wysocki 2019-07-22 18:29 ` Bjorn Helgaas 2019-09-02 21:08 ` Rafael J. Wysocki 2019-09-05 4:11 ` Kelsey Skunberg 2019-07-22 2:35 ` [PATCH 2/3] ACPI: Remove acpi_has_method() call from scan.c Kelsey Skunberg 2019-07-22 8:29 ` Rafael J. Wysocki 2019-07-22 2:35 ` [PATCH 3/3] ACPI: Remove acpi_has_method() calls from thermal.c Kelsey Skunberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).