| Message ID | 20230717150047.15196-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: NHLT: Access and query helpers | expand |
On 2023-07-17 5:00 PM, Cezary Rojewski wrote: > While there is no strict limit to amount of NHLT tables present, usually > just the first one is utilized. To simplify implementation of sound > drivers, provide publicly accessible pointer. Accessing it after calling > acpi_nhlt_get_gbl_table() yields the first NHLT table met during the > scan. ... > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -594,6 +594,9 @@ config ACPI_PRMT > substantially increase computational overhead related to the > initialization of some server systems. > > +config ACPI_NHLT > + bool > + > endif # ACPI > > config X86_PM_TIMER > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index feb36c0b9446..8de34970e7db 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -93,6 +93,7 @@ obj-$(CONFIG_ACPI) += container.o > obj-$(CONFIG_ACPI_THERMAL) += thermal.o > obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o > obj-$(CONFIG_ACPI_NFIT) += nfit/ > +obj-$(CONFIG_ACPI_NHLT) += nhlt.o > obj-$(CONFIG_ACPI_NUMA) += numa/ > obj-$(CONFIG_ACPI) += acpi_memhotplug.o > obj-$(CONFIG_ACPI_HOTPLUG_IOAPIC) += ioapic.o > diff --git a/drivers/acpi/nhlt.c b/drivers/acpi/nhlt.c > new file mode 100644 > index 000000000000..90d74d0d803e > --- /dev/null > +++ b/drivers/acpi/nhlt.c > @@ -0,0 +1,13 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright(c) 2023 Intel Corporation. All rights reserved. > +// > +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> > +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > +// > + > +#include <linux/export.h> > +#include <acpi/nhlt.h> > + > +struct acpi_table_nhlt *acpi_gbl_NHLT; > +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, symbol is never defined. Proposed solution - modify drivers/acpi/tables.c with: +#include <acpi/nhlt.h> + +struct acpi_table_nhlt *acpi_gbl_NHLT; +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); as tables.c is always built the symbol is always there. The only other option I see is: -obj-$(CONFIG_ACPI_NHLT) += nhlt.o +obj-y += nhlt.o and modifying nhlt.c so it's essentially split in half with: #if IS_ENABLED(CONFIG_ACPI_NHLT) but such solutions stinks. I prefer the first approach. What to you find guys? > diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h > index af3ec45ba4f9..a2b93b08218f 100644 > --- a/include/acpi/nhlt.h > +++ b/include/acpi/nhlt.h > @@ -13,6 +13,24 @@ > #include <linux/overflow.h> > #include <linux/types.h> > > +/* System-wide pointer to the first NHLT table. */ > +extern struct acpi_table_nhlt *acpi_gbl_NHLT; > + > +/* > + * A sound driver may utilize the two below on its initialization and removal > + * respectively to avoid excessive mapping and unmapping of the memory > + * occupied by the table between streaming operations. > + */ > +static inline acpi_status acpi_nhlt_get_gbl_table(void) > +{ > + return acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)(&acpi_gbl_NHLT)); > +} > + > +static inline void acpi_nhlt_put_gbl_table(void) > +{ > + acpi_put_table((struct acpi_table_header *)acpi_gbl_NHLT); > +} > + > #define __acpi_nhlt_endpoint_cfg(ep) ((void *)((ep) + 1)) > > /*
On Wed, Jul 19, 2023 at 04:47:31PM +0200, Cezary Rojewski wrote: > On 2023-07-17 5:00 PM, Cezary Rojewski wrote: ... > > +++ b/drivers/acpi/nhlt.c > > @@ -0,0 +1,13 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// > > +// Copyright(c) 2023 Intel Corporation. All rights reserved. > > +// > > +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> > > +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > > +// > > + > > +#include <linux/export.h> > > +#include <acpi/nhlt.h> > > + > > +struct acpi_table_nhlt *acpi_gbl_NHLT; > > +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > > This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when > ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, > symbol is never defined. > > Proposed solution - modify drivers/acpi/tables.c with: > > +#include <acpi/nhlt.h> > + > +struct acpi_table_nhlt *acpi_gbl_NHLT; > +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > > as tables.c is always built the symbol is always there. > The only other option I see is: > > -obj-$(CONFIG_ACPI_NHLT) += nhlt.o > +obj-y += nhlt.o > > and modifying nhlt.c so it's essentially split in half with: > #if IS_ENABLED(CONFIG_ACPI_NHLT) > > but such solutions stinks. I prefer the first approach. > What to you find guys? I leave this to Rafael as it's his territory.
On 2023-07-19 5:36 PM, Andy Shevchenko wrote: > On Wed, Jul 19, 2023 at 04:47:31PM +0200, Cezary Rojewski wrote: >> On 2023-07-17 5:00 PM, Cezary Rojewski wrote: ... >>> +++ b/drivers/acpi/nhlt.c >>> @@ -0,0 +1,13 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +// >>> +// Copyright(c) 2023 Intel Corporation. All rights reserved. >>> +// >>> +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> >>> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> >>> +// >>> + >>> +#include <linux/export.h> >>> +#include <acpi/nhlt.h> >>> + >>> +struct acpi_table_nhlt *acpi_gbl_NHLT; >>> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); >> >> This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when >> ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, >> symbol is never defined. >> >> Proposed solution - modify drivers/acpi/tables.c with: >> >> +#include <acpi/nhlt.h> >> + >> +struct acpi_table_nhlt *acpi_gbl_NHLT; >> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); >> >> as tables.c is always built the symbol is always there. >> The only other option I see is: >> >> -obj-$(CONFIG_ACPI_NHLT) += nhlt.o >> +obj-y += nhlt.o >> >> and modifying nhlt.c so it's essentially split in half with: >> #if IS_ENABLED(CONFIG_ACPI_NHLT) >> >> but such solutions stinks. I prefer the first approach. >> What to you find guys? > > I leave this to Rafael as it's his territory. Rafael, which option do you prefer? Regardless of IKP and my CI returning success on compilation tests, clearly there is a problem when CONFIG_ACPI_NHLT.
On Thu, Jul 20, 2023 at 11:15 AM Cezary Rojewski <cezary.rojewski@intel.com> wrote: > > On 2023-07-19 5:36 PM, Andy Shevchenko wrote: > > On Wed, Jul 19, 2023 at 04:47:31PM +0200, Cezary Rojewski wrote: > >> On 2023-07-17 5:00 PM, Cezary Rojewski wrote: > > ... > > >>> +++ b/drivers/acpi/nhlt.c > >>> @@ -0,0 +1,13 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +// > >>> +// Copyright(c) 2023 Intel Corporation. All rights reserved. > >>> +// > >>> +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> > >>> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > >>> +// > >>> + > >>> +#include <linux/export.h> > >>> +#include <acpi/nhlt.h> > >>> + > >>> +struct acpi_table_nhlt *acpi_gbl_NHLT; > >>> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > >> > >> This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when > >> ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, > >> symbol is never defined. > >> > >> Proposed solution - modify drivers/acpi/tables.c with: > >> > >> +#include <acpi/nhlt.h> > >> + > >> +struct acpi_table_nhlt *acpi_gbl_NHLT; > >> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > >> > >> as tables.c is always built the symbol is always there. > >> The only other option I see is: > >> > >> -obj-$(CONFIG_ACPI_NHLT) += nhlt.o > >> +obj-y += nhlt.o > >> > >> and modifying nhlt.c so it's essentially split in half with: > >> #if IS_ENABLED(CONFIG_ACPI_NHLT) > >> > >> but such solutions stinks. I prefer the first approach. > >> What to you find guys? > > > > I leave this to Rafael as it's his territory. > > Rafael, which option do you prefer? > > Regardless of IKP and my CI returning success on compilation tests, > clearly there is a problem when CONFIG_ACPI_NHLT. Putting the definition of acpi_gbl_NHLT into tables.c would be fine with me, but in any case, because it is an exported symbol, it needs a description in a kerneldoc comment.
On Thu, Jul 20, 2023 at 7:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jul 20, 2023 at 11:15 AM Cezary Rojewski > <cezary.rojewski@intel.com> wrote: > > > > On 2023-07-19 5:36 PM, Andy Shevchenko wrote: > > > On Wed, Jul 19, 2023 at 04:47:31PM +0200, Cezary Rojewski wrote: > > >> On 2023-07-17 5:00 PM, Cezary Rojewski wrote: > > > > ... > > > > >>> +++ b/drivers/acpi/nhlt.c > > >>> @@ -0,0 +1,13 @@ > > >>> +// SPDX-License-Identifier: GPL-2.0-only > > >>> +// > > >>> +// Copyright(c) 2023 Intel Corporation. All rights reserved. > > >>> +// > > >>> +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> > > >>> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > > >>> +// > > >>> + > > >>> +#include <linux/export.h> > > >>> +#include <acpi/nhlt.h> > > >>> + > > >>> +struct acpi_table_nhlt *acpi_gbl_NHLT; > > >>> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > > >> > > >> This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when > > >> ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, > > >> symbol is never defined. > > >> > > >> Proposed solution - modify drivers/acpi/tables.c with: > > >> > > >> +#include <acpi/nhlt.h> > > >> + > > >> +struct acpi_table_nhlt *acpi_gbl_NHLT; No capitals in variable names, please. > > >> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); > > >> > > >> as tables.c is always built the symbol is always there. > > >> The only other option I see is: > > >> > > >> -obj-$(CONFIG_ACPI_NHLT) += nhlt.o > > >> +obj-y += nhlt.o > > >> > > >> and modifying nhlt.c so it's essentially split in half with: > > >> #if IS_ENABLED(CONFIG_ACPI_NHLT) > > >> > > >> but such solutions stinks. I prefer the first approach. > > >> What to you find guys? > > > > > > I leave this to Rafael as it's his territory. > > > > Rafael, which option do you prefer? > > > > Regardless of IKP and my CI returning success on compilation tests, > > clearly there is a problem when CONFIG_ACPI_NHLT. > > Putting the definition of acpi_gbl_NHLT into tables.c would be fine > with me, but in any case, because it is an exported symbol, it needs a > description in a kerneldoc comment. That said, you can also do something like this in a header file: #ifdef CONFIG_ACPI_NHLT extern struct acpi_table_nhlt *acpi_gbl_nhlt; #else #define acpi_gbl_nhlt NULL #endif and require the acpi_gbl_nhlt users to include it.
On 2023-07-20 7:05 PM, Rafael J. Wysocki wrote: > On Thu, Jul 20, 2023 at 7:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> >> On Thu, Jul 20, 2023 at 11:15 AM Cezary Rojewski >> <cezary.rojewski@intel.com> wrote: ... >>>>> This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when >>>>> ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, >>>>> symbol is never defined. >>>>> >>>>> Proposed solution - modify drivers/acpi/tables.c with: >>>>> >>>>> +#include <acpi/nhlt.h> >>>>> + >>>>> +struct acpi_table_nhlt *acpi_gbl_NHLT; > > No capitals in variable names, please. acpi_gbl_NHLT follows the path set by acpi_gbl_DSDT, _FADT and others. Why would NHLT be an exception? Is this because it's not defined under ACPICA? Uncapitalizing nonetheless in v3. >>>>> +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); >>>>> >>>>> as tables.c is always built the symbol is always there. >>>>> The only other option I see is: >>>>> >>>>> -obj-$(CONFIG_ACPI_NHLT) += nhlt.o >>>>> +obj-y += nhlt.o >>>>> >>>>> and modifying nhlt.c so it's essentially split in half with: >>>>> #if IS_ENABLED(CONFIG_ACPI_NHLT) >>>>> >>>>> but such solutions stinks. I prefer the first approach. >>>>> What to you find guys? >>>> >>>> I leave this to Rafael as it's his territory. >>> >>> Rafael, which option do you prefer? >>> >>> Regardless of IKP and my CI returning success on compilation tests, >>> clearly there is a problem when CONFIG_ACPI_NHLT. >> >> Putting the definition of acpi_gbl_NHLT into tables.c would be fine >> with me, but in any case, because it is an exported symbol, it needs a >> description in a kerneldoc comment. > > That said, you can also do something like this in a header file: > > #ifdef CONFIG_ACPI_NHLT > extern struct acpi_table_nhlt *acpi_gbl_nhlt; > #else > #define acpi_gbl_nhlt NULL > #endif > > and require the acpi_gbl_nhlt users to include it. Simplest solutions usually work the best. Surprised I haven't thought about it earlier!
On Fri, Jul 21, 2023 at 11:49 AM Cezary Rojewski <cezary.rojewski@intel.com> wrote: > > On 2023-07-20 7:05 PM, Rafael J. Wysocki wrote: > > On Thu, Jul 20, 2023 at 7:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > >> > >> On Thu, Jul 20, 2023 at 11:15 AM Cezary Rojewski > >> <cezary.rojewski@intel.com> wrote: > > ... > > >>>>> This approach generates a problem with undefined symbol "acpi_gbl_NHLT" when > >>>>> ACPI_NHLT is disabled. As nhlt.c is not built when said kconfig is disabled, > >>>>> symbol is never defined. > >>>>> > >>>>> Proposed solution - modify drivers/acpi/tables.c with: > >>>>> > >>>>> +#include <acpi/nhlt.h> > >>>>> + > >>>>> +struct acpi_table_nhlt *acpi_gbl_NHLT; > > > > No capitals in variable names, please. > > acpi_gbl_NHLT follows the path set by acpi_gbl_DSDT, _FADT and others. > Why would NHLT be an exception? Is this because it's not defined under > ACPICA? Yes, it is. ACPICA has its own rules, sort of.
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index ccbeab9500ec..01ce5d3533db 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -594,6 +594,9 @@ config ACPI_PRMT substantially increase computational overhead related to the initialization of some server systems. +config ACPI_NHLT + bool + endif # ACPI config X86_PM_TIMER diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index feb36c0b9446..8de34970e7db 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -93,6 +93,7 @@ obj-$(CONFIG_ACPI) += container.o obj-$(CONFIG_ACPI_THERMAL) += thermal.o obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o obj-$(CONFIG_ACPI_NFIT) += nfit/ +obj-$(CONFIG_ACPI_NHLT) += nhlt.o obj-$(CONFIG_ACPI_NUMA) += numa/ obj-$(CONFIG_ACPI) += acpi_memhotplug.o obj-$(CONFIG_ACPI_HOTPLUG_IOAPIC) += ioapic.o diff --git a/drivers/acpi/nhlt.c b/drivers/acpi/nhlt.c new file mode 100644 index 000000000000..90d74d0d803e --- /dev/null +++ b/drivers/acpi/nhlt.c @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2023 Intel Corporation. All rights reserved. +// +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> +// + +#include <linux/export.h> +#include <acpi/nhlt.h> + +struct acpi_table_nhlt *acpi_gbl_NHLT; +EXPORT_SYMBOL_GPL(acpi_gbl_NHLT); diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h index af3ec45ba4f9..a2b93b08218f 100644 --- a/include/acpi/nhlt.h +++ b/include/acpi/nhlt.h @@ -13,6 +13,24 @@ #include <linux/overflow.h> #include <linux/types.h> +/* System-wide pointer to the first NHLT table. */ +extern struct acpi_table_nhlt *acpi_gbl_NHLT; + +/* + * A sound driver may utilize the two below on its initialization and removal + * respectively to avoid excessive mapping and unmapping of the memory + * occupied by the table between streaming operations. + */ +static inline acpi_status acpi_nhlt_get_gbl_table(void) +{ + return acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)(&acpi_gbl_NHLT)); +} + +static inline void acpi_nhlt_put_gbl_table(void) +{ + acpi_put_table((struct acpi_table_header *)acpi_gbl_NHLT); +} + #define __acpi_nhlt_endpoint_cfg(ep) ((void *)((ep) + 1)) /*