| Message ID | 20250203182322.384883-5-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: wmi: Rework WMI device enabling | expand |
On Mon, 3 Feb 2025, Armin Wolf wrote: > The WMI core already knows the instance count of a WMI guid. > Use this information instead of querying all possible instances > which is slow and might be unreliable. > > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > index 0b277b7e37dd..63c78b4d8258 100644 > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > @@ -388,16 +388,13 @@ union acpi_object *hp_get_wmiobj_pointer(int instance_id, const char *guid_strin > */ > int hp_get_instance_count(const char *guid_string) > { > - union acpi_object *wmi_obj = NULL; > - int i = 0; > + int ret; > > - do { > - kfree(wmi_obj); > - wmi_obj = hp_get_wmiobj_pointer(i, guid_string); > - i++; > - } while (wmi_obj); > + ret = wmi_instance_count(guid_string); > + if (ret < 0) > + return 0; > > - return i - 1; > + return ret; > } Hi Armin, While it is the existing way of doing things, I don't like how the error is not properly passed on here. And if the error handling is pushed upwards to the calling sites, then this entire function becomes useless and wmi_instance_count() could be used directly in the callers.
Am 04.02.25 um 11:37 schrieb Ilpo Järvinen: > On Mon, 3 Feb 2025, Armin Wolf wrote: > >> The WMI core already knows the instance count of a WMI guid. >> Use this information instead of querying all possible instances >> which is slow and might be unreliable. >> >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 13 +++++-------- >> 1 file changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >> index 0b277b7e37dd..63c78b4d8258 100644 >> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >> @@ -388,16 +388,13 @@ union acpi_object *hp_get_wmiobj_pointer(int instance_id, const char *guid_strin >> */ >> int hp_get_instance_count(const char *guid_string) >> { >> - union acpi_object *wmi_obj = NULL; >> - int i = 0; >> + int ret; >> >> - do { >> - kfree(wmi_obj); >> - wmi_obj = hp_get_wmiobj_pointer(i, guid_string); >> - i++; >> - } while (wmi_obj); >> + ret = wmi_instance_count(guid_string); >> + if (ret < 0) >> + return 0; >> >> - return i - 1; >> + return ret; >> } > Hi Armin, > > While it is the existing way of doing things, I don't like how the error > is not properly passed on here. And if the error handling is pushed > upwards to the calling sites, then this entire function becomes useless > and wmi_instance_count() could be used directly in the callers. > The thing is that for the hp-bioscfg driver, a missing WMI GUID is not an error. In this case 0 instances are available. I would keep this function for now. Thanks, Armin Wolf
On Tue, 4 Feb 2025, Armin Wolf wrote: > Am 04.02.25 um 11:37 schrieb Ilpo Järvinen: > > > On Mon, 3 Feb 2025, Armin Wolf wrote: > > > > > The WMI core already knows the instance count of a WMI guid. > > > Use this information instead of querying all possible instances > > > which is slow and might be unreliable. > > > > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > > > --- > > > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > > > b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > > > index 0b277b7e37dd..63c78b4d8258 100644 > > > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c > > > @@ -388,16 +388,13 @@ union acpi_object *hp_get_wmiobj_pointer(int > > > instance_id, const char *guid_strin > > > */ > > > int hp_get_instance_count(const char *guid_string) > > > { > > > - union acpi_object *wmi_obj = NULL; > > > - int i = 0; > > > + int ret; > > > > > > - do { > > > - kfree(wmi_obj); > > > - wmi_obj = hp_get_wmiobj_pointer(i, guid_string); > > > - i++; > > > - } while (wmi_obj); > > > + ret = wmi_instance_count(guid_string); > > > + if (ret < 0) > > > + return 0; > > > > > > - return i - 1; > > > + return ret; > > > } > > Hi Armin, > > > > While it is the existing way of doing things, I don't like how the error > > is not properly passed on here. And if the error handling is pushed > > upwards to the calling sites, then this entire function becomes useless > > and wmi_instance_count() could be used directly in the callers. > > The thing is that for the hp-bioscfg driver, a missing WMI GUID is not an > error. > In this case 0 instances are available. > > I would keep this function for now. Okay, fine.
diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c index 0b277b7e37dd..63c78b4d8258 100644 --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c @@ -388,16 +388,13 @@ union acpi_object *hp_get_wmiobj_pointer(int instance_id, const char *guid_strin */ int hp_get_instance_count(const char *guid_string) { - union acpi_object *wmi_obj = NULL; - int i = 0; + int ret; - do { - kfree(wmi_obj); - wmi_obj = hp_get_wmiobj_pointer(i, guid_string); - i++; - } while (wmi_obj); + ret = wmi_instance_count(guid_string); + if (ret < 0) + return 0; - return i - 1; + return ret; } /**
The WMI core already knows the instance count of a WMI guid. Use this information instead of querying all possible instances which is slow and might be unreliable. Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) -- 2.39.5