| Message ID | d66501819cfa8f3d3b07708d3f20e3e20d07e4f8.1448282995.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
>>acpi_dev_present Do you really want to be walking the ACPICA namespace for every call? > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Sent: Monday, November 23, 2015 2:35 PM > To: Lukas Wunner > Cc: linux-acpi@vger.kernel.org; devel@acpica.org; platform-driver- > x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa- > devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv; > Moore, Robert; David Box > Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present > > On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote: > > There are 7 drivers which call acpi_get_devices to check for the > > presence of a particular ACPI HID, each defining its own copy of a > > mostly identical callback. > > > > Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing > > us to deduplicate all that boilerplate in the drivers. > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > We have a general rule for ACPICA patches that they go in through upstream > ACPICA. > > So if you really want this changes in ACPICA, this is the way to handle > it. > > Now, of course, for that to happen, you should have CCed the ACPICA > maintainers too (now done). > > Thanks, > Rafael > > > > --- > > drivers/acpi/acpica/nsxfeval.c | 46 > ++++++++++++++++++++++++++++++++++++++++++ > > include/acpi/acpixf.h | 7 +++++++ > > 2 files changed, 53 insertions(+) > > > > diff --git a/drivers/acpi/acpica/nsxfeval.c > > b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644 > > --- a/drivers/acpi/acpica/nsxfeval.c > > +++ b/drivers/acpi/acpica/nsxfeval.c > > @@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices) > > > > > /************************************************************************* > ****** > > * > > + * FUNCTION: acpi_ns_dev_present_callback > > + * > > + * PARAMETERS: Callback from acpi_get_devices > > + * > > + * RETURN: Status > > + * > > + * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which > > + * performs no further filtering and terminates the search > > + * immediately. > > + * > > + > > +********************************************************************* > > +*********/ static acpi_status > > +acpi_ns_dev_present_callback(acpi_handle handle, u32 level, > > + void *context, void **retval) > > +{ > > + *(bool *)context = true; > > + return AE_CTRL_TERMINATE; > > +} > > + > > +/******************************************************************** > > +*********** > > + * > > + * FUNCTION: acpi_dev_present > > + * > > + * PARAMETERS: HID - HID to search for. > > + * > > + * RETURNS True if a matching object of type Device was found. > > + * > > + * DESCRIPTION: Performs a walk of the namespace tree. When a matching > object > > + * of type Device is found, the search is terminated > immediately. > > + * > > + > > +********************************************************************* > > +*********/ > > + > > +bool > > +acpi_dev_present(const char *HID) > > +{ > > + acpi_status status; > > + bool found = false; > > + > > + status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found, > > + NULL); > > + return ACPI_SUCCESS(status) && found; } > > + > > +ACPI_EXPORT_SYMBOL(acpi_dev_present) > > + > > +/******************************************************************** > > +*********** > > + * > > * FUNCTION: acpi_attach_data > > * > > * PARAMETERS: obj_handle - Namespace node > > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index > > 3aaaa86..f299347 100644 > > --- a/include/acpi/acpixf.h > > +++ b/include/acpi/acpixf.h > > @@ -115,6 +115,11 @@ > > prototype; > > #endif > > > > +#ifndef ACPI_EXTERNAL_RETURN_BOOL > > +#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \ > > + prototype; > > +#endif > > + > > > /************************************************************************* > **** > > * > > * Public globals and runtime configuration options @@ -483,6 +488,8 > > @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status > > acpi_walk_callback user_function, > > void *context, > > void **return_value)) > > +ACPI_EXTERNAL_RETURN_BOOL(bool > > + acpi_dev_present(const char *HID)) > > ACPI_EXTERNAL_RETURN_STATUS(acpi_status > > acpi_get_name(acpi_handle object, u32 name_type, > > struct acpi_buffer *ret_path_ptr)) > >
On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote: > There are 7 drivers which call acpi_get_devices to check for the > presence of a particular ACPI HID, each defining its own copy of > a mostly identical callback. > > Add acpi_dev_present, the ACPI equivalent to pci_dev_present, > allowing us to deduplicate all that boilerplate in the drivers. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> We have a general rule for ACPICA patches that they go in through upstream ACPICA. So if you really want this changes in ACPICA, this is the way to handle it. Now, of course, for that to happen, you should have CCed the ACPICA maintainers too (now done). Thanks, Rafael > --- > drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++ > include/acpi/acpixf.h | 7 +++++++ > 2 files changed, 53 insertions(+) > > diff --git a/drivers/acpi/acpica/nsxfeval.c b/drivers/acpi/acpica/nsxfeval.c > index 6ee1e52..19293fa 100644 > --- a/drivers/acpi/acpica/nsxfeval.c > +++ b/drivers/acpi/acpica/nsxfeval.c > @@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices) > > /******************************************************************************* > * > + * FUNCTION: acpi_ns_dev_present_callback > + * > + * PARAMETERS: Callback from acpi_get_devices > + * > + * RETURN: Status > + * > + * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which > + * performs no further filtering and terminates the search > + * immediately. > + * > + ******************************************************************************/ > +static acpi_status acpi_ns_dev_present_callback(acpi_handle handle, u32 level, > + void *context, void **retval) > +{ > + *(bool *)context = true; > + return AE_CTRL_TERMINATE; > +} > + > +/******************************************************************************* > + * > + * FUNCTION: acpi_dev_present > + * > + * PARAMETERS: HID - HID to search for. > + * > + * RETURNS True if a matching object of type Device was found. > + * > + * DESCRIPTION: Performs a walk of the namespace tree. When a matching object > + * of type Device is found, the search is terminated immediately. > + * > + ******************************************************************************/ > + > +bool > +acpi_dev_present(const char *HID) > +{ > + acpi_status status; > + bool found = false; > + > + status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found, > + NULL); > + return ACPI_SUCCESS(status) && found; > +} > + > +ACPI_EXPORT_SYMBOL(acpi_dev_present) > + > +/******************************************************************************* > + * > * FUNCTION: acpi_attach_data > * > * PARAMETERS: obj_handle - Namespace node > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h > index 3aaaa86..f299347 100644 > --- a/include/acpi/acpixf.h > +++ b/include/acpi/acpixf.h > @@ -115,6 +115,11 @@ > prototype; > #endif > > +#ifndef ACPI_EXTERNAL_RETURN_BOOL > +#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \ > + prototype; > +#endif > + > /***************************************************************************** > * > * Public globals and runtime configuration options > @@ -483,6 +488,8 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status > acpi_walk_callback user_function, > void *context, > void **return_value)) > +ACPI_EXTERNAL_RETURN_BOOL(bool > + acpi_dev_present(const char *HID)) > ACPI_EXTERNAL_RETURN_STATUS(acpi_status > acpi_get_name(acpi_handle object, u32 name_type, > struct acpi_buffer *ret_path_ptr)) > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robert, On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote: > >>acpi_dev_present > > Do you really want to be walking the ACPICA namespace for every call? That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback. What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster? Sorry for not cc'ing you right away, if you want to take a look at the patches for the drivers they're browsable on GitHub or in the acpica.org mailing list archive: https://github.com/l1k/linux/commits/acpi_dev_present https://lists.acpica.org/pipermail/devel/2015-November/000851.html Patches [0/5] and [1/5] are not visible in the mailing list archive: They landed in the moderation queue because they had too many recipients. I guess if I had cc'ed you all, none of the patches would have come through. Thanks, Lukas > > > -----Original Message----- > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > > Sent: Monday, November 23, 2015 2:35 PM > > To: Lukas Wunner > > Cc: linux-acpi@vger.kernel.org; devel@acpica.org; platform-driver- > > x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa- > > devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv; > > Moore, Robert; David Box > > Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present > > > > On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote: > > > There are 7 drivers which call acpi_get_devices to check for the > > > presence of a particular ACPI HID, each defining its own copy of a > > > mostly identical callback. > > > > > > Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing > > > us to deduplicate all that boilerplate in the drivers. > > > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > > We have a general rule for ACPICA patches that they go in through upstream > > ACPICA. > > > > So if you really want this changes in ACPICA, this is the way to handle > > it. > > > > Now, of course, for that to happen, you should have CCed the ACPICA > > maintainers too (now done). > > > > Thanks, > > Rafael > > > > > > > --- > > > drivers/acpi/acpica/nsxfeval.c | 46 > > ++++++++++++++++++++++++++++++++++++++++++ > > > include/acpi/acpixf.h | 7 +++++++ > > > 2 files changed, 53 insertions(+) > > > > > > diff --git a/drivers/acpi/acpica/nsxfeval.c > > > b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644 > > > --- a/drivers/acpi/acpica/nsxfeval.c > > > +++ b/drivers/acpi/acpica/nsxfeval.c > > > @@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices) > > > > > > > > /************************************************************************* > > ****** > > > * > > > + * FUNCTION: acpi_ns_dev_present_callback > > > + * > > > + * PARAMETERS: Callback from acpi_get_devices > > > + * > > > + * RETURN: Status > > > + * > > > + * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which > > > + * performs no further filtering and terminates the search > > > + * immediately. > > > + * > > > + > > > +********************************************************************* > > > +*********/ static acpi_status > > > +acpi_ns_dev_present_callback(acpi_handle handle, u32 level, > > > + void *context, void **retval) > > > +{ > > > + *(bool *)context = true; > > > + return AE_CTRL_TERMINATE; > > > +} > > > + > > > +/******************************************************************** > > > +*********** > > > + * > > > + * FUNCTION: acpi_dev_present > > > + * > > > + * PARAMETERS: HID - HID to search for. > > > + * > > > + * RETURNS True if a matching object of type Device was found. > > > + * > > > + * DESCRIPTION: Performs a walk of the namespace tree. When a matching > > object > > > + * of type Device is found, the search is terminated > > immediately. > > > + * > > > + > > > +********************************************************************* > > > +*********/ > > > + > > > +bool > > > +acpi_dev_present(const char *HID) > > > +{ > > > + acpi_status status; > > > + bool found = false; > > > + > > > + status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found, > > > + NULL); > > > + return ACPI_SUCCESS(status) && found; } > > > + > > > +ACPI_EXPORT_SYMBOL(acpi_dev_present) > > > + > > > +/******************************************************************** > > > +*********** > > > + * > > > * FUNCTION: acpi_attach_data > > > * > > > * PARAMETERS: obj_handle - Namespace node > > > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index > > > 3aaaa86..f299347 100644 > > > --- a/include/acpi/acpixf.h > > > +++ b/include/acpi/acpixf.h > > > @@ -115,6 +115,11 @@ > > > prototype; > > > #endif > > > > > > +#ifndef ACPI_EXTERNAL_RETURN_BOOL > > > +#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \ > > > + prototype; > > > +#endif > > > + > > > > > /************************************************************************* > > **** > > > * > > > * Public globals and runtime configuration options @@ -483,6 +488,8 > > > @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status > > > acpi_walk_callback user_function, > > > void *context, > > > void **return_value)) > > > +ACPI_EXTERNAL_RETURN_BOOL(bool > > > + acpi_dev_present(const char *HID)) > > > ACPI_EXTERNAL_RETURN_STATUS(acpi_status > > > acpi_get_name(acpi_handle object, u32 name_type, > > > struct acpi_buffer *ret_path_ptr)) > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/11/24 7:32, Lukas Wunner wrote: > Hi Robert, > > On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote: >>>> acpi_dev_present >> Do you really want to be walking the ACPICA namespace for every call? > That's what the drivers currently do. Typically this is called only once > on initialization by the driver's ->probe callback. > > What did you have in mind instead, cache the result? Or store the HIDs > in the namespace in a hash that can be queried faster? Will those drivers be loaded before the acpi namespace is scanned? if not, I think those IDs already cached, in acpi_init_device_object(), INIT_LIST_HEAD(&device->pnp.ids); ... acpi_set_pnp_ids(handle, &device->pnp, type); please see API acpi_device_hid(), so I think you can introduce a API with acpi_device and HID passed as arguments in scan.c Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Hanjun Guo [mailto:guohanjun@huawei.com] > Sent: Monday, November 23, 2015 8:41 PM > To: Lukas Wunner; Moore, Robert > Cc: Rafael J. Wysocki; linux-acpi@vger.kernel.org; devel@acpica.org; > platform-driver-x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee, > Chun-Yi; alsa-devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown; > Zheng, Lv; David Box > Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present > > On 2015/11/24 7:32, Lukas Wunner wrote: > > Hi Robert, > > > > On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote: > >>>> acpi_dev_present > >> Do you really want to be walking the ACPICA namespace for every call? > > That's what the drivers currently do. Typically this is called only > > once on initialization by the driver's ->probe callback. > > > > What did you have in mind instead, cache the result? Or store the HIDs > > in the namespace in a hash that can be queried faster? > > Will those drivers be loaded before the acpi namespace is scanned? if not, > I think those IDs already cached, in acpi_init_device_object(), I was thinking about something like this. Traversing the namespace over and over is truly brute force. > > INIT_LIST_HEAD(&device->pnp.ids); > ... > acpi_set_pnp_ids(handle, &device->pnp, type); > > please see API acpi_device_hid(), so I think you can introduce a API with > acpi_device and HID passed as arguments in scan.c > > Thanks > Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 24, 2015 12:40:51 PM Hanjun Guo wrote: > On 2015/11/24 7:32, Lukas Wunner wrote: > > Hi Robert, > > > > On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote: > >>>> acpi_dev_present > >> Do you really want to be walking the ACPICA namespace for every call? > > That's what the drivers currently do. Typically this is called only once > > on initialization by the driver's ->probe callback. > > > > What did you have in mind instead, cache the result? Or store the HIDs > > in the namespace in a hash that can be queried faster? > > Will those drivers be loaded before the acpi namespace is scanned? if not, I think > those IDs already cached, in acpi_init_device_object(), > > INIT_LIST_HEAD(&device->pnp.ids); > ... > acpi_set_pnp_ids(handle, &device->pnp, type); > > please see API acpi_device_hid(), so I think you can introduce a API with > acpi_device and HID passed as arguments in scan.c I'd prefer that to go to utils.c to be honest, even if the namespace needs to be walked. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/24/2015 10:22 PM, Rafael J. Wysocki wrote: > On Tuesday, November 24, 2015 12:40:51 PM Hanjun Guo wrote: >> On 2015/11/24 7:32, Lukas Wunner wrote: >>> Hi Robert, >>> >>> On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote: >>>>>> acpi_dev_present >>>> Do you really want to be walking the ACPICA namespace for every call? >>> That's what the drivers currently do. Typically this is called only once >>> on initialization by the driver's ->probe callback. >>> >>> What did you have in mind instead, cache the result? Or store the HIDs >>> in the namespace in a hash that can be queried faster? >> >> Will those drivers be loaded before the acpi namespace is scanned? if not, I think >> those IDs already cached, in acpi_init_device_object(), >> >> INIT_LIST_HEAD(&device->pnp.ids); >> ... >> acpi_set_pnp_ids(handle, &device->pnp, type); >> >> please see API acpi_device_hid(), so I think you can introduce a API with >> acpi_device and HID passed as arguments in scan.c > > I'd prefer that to go to utils.c to be honest, even if the namespace needs to > be walked. I agree, utils.c is the better place to go. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/acpica/nsxfeval.c b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644 --- a/drivers/acpi/acpica/nsxfeval.c +++ b/drivers/acpi/acpica/nsxfeval.c @@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices) /******************************************************************************* * + * FUNCTION: acpi_ns_dev_present_callback + * + * PARAMETERS: Callback from acpi_get_devices + * + * RETURN: Status + * + * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which + * performs no further filtering and terminates the search + * immediately. + * + ******************************************************************************/ +static acpi_status acpi_ns_dev_present_callback(acpi_handle handle, u32 level, + void *context, void **retval) +{ + *(bool *)context = true; + return AE_CTRL_TERMINATE; +} + +/******************************************************************************* + * + * FUNCTION: acpi_dev_present + * + * PARAMETERS: HID - HID to search for. + * + * RETURNS True if a matching object of type Device was found. + * + * DESCRIPTION: Performs a walk of the namespace tree. When a matching object + * of type Device is found, the search is terminated immediately. + * + ******************************************************************************/ + +bool +acpi_dev_present(const char *HID) +{ + acpi_status status; + bool found = false; + + status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found, + NULL); + return ACPI_SUCCESS(status) && found; +} + +ACPI_EXPORT_SYMBOL(acpi_dev_present) + +/******************************************************************************* + * * FUNCTION: acpi_attach_data * * PARAMETERS: obj_handle - Namespace node diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 3aaaa86..f299347 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -115,6 +115,11 @@ prototype; #endif +#ifndef ACPI_EXTERNAL_RETURN_BOOL +#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \ + prototype; +#endif + /***************************************************************************** * * Public globals and runtime configuration options @@ -483,6 +488,8 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_walk_callback user_function, void *context, void **return_value)) +ACPI_EXTERNAL_RETURN_BOOL(bool + acpi_dev_present(const char *HID)) ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_get_name(acpi_handle object, u32 name_type, struct acpi_buffer *ret_path_ptr))
There are 7 drivers which call acpi_get_devices to check for the presence of a particular ACPI HID, each defining its own copy of a mostly identical callback. Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing us to deduplicate all that boilerplate in the drivers. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++ include/acpi/acpixf.h | 7 +++++++ 2 files changed, 53 insertions(+)