| Message ID | 20180226113431.3e43a032@endymion (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Feb 26, 2018 at 11:34 AM, Jean Delvare <jdelvare@suse.de> wrote: > When ACPI debugging is disabled, I see warnings like this one: > > drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap": > drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body] > drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body] > > It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving > to nothing. Make them resolve to the classic "do {} while (0)" > construct instead if the compiler likes that, or just {} if not, to > silent all such warnings. So first of all, acpi_smbus_cmi_add_cap() shouldn't really use ACPI_DEBUG_PRINT() and similar. They belong to ACPICA and their use should be limited to it. I know that they are used in the other parts of the ACPI subsystem, but they really should be replaced with the kernel's proper debug statements in there. 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
Hi Rafael, On Mon, 26 Feb 2018 23:11:37 +0100, Rafael J. Wysocki wrote: > On Mon, Feb 26, 2018 at 11:34 AM, Jean Delvare <jdelvare@suse.de> wrote: > > When ACPI debugging is disabled, I see warnings like this one: > > > > drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap": > > drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body] > > drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body] > > > > It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving > > to nothing. Make them resolve to the classic "do {} while (0)" > > construct instead if the compiler likes that, or just {} if not, to > > silent all such warnings. > > So first of all, acpi_smbus_cmi_add_cap() shouldn't really use > ACPI_DEBUG_PRINT() and similar. They belong to ACPICA and their use > should be limited to it. OK, thanks for the information. I'll update the i2c-scmi driver to no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as well though (xo15-ebook and panasonic-laptop, as well as xen-acpi-memhotplug - not sure if you consider that one as more legitimate.) > I know that they are used in the other parts of the ACPI subsystem, > but they really should be replaced with the kernel's proper debug > statements in there. Ideally all these macros shouldn't be exposed to drivers which are not supposed to use them. Could they be moved to a header file internal to at least acpi?
On Tue, Feb 27, 2018 at 10:23 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Rafael, Hi, > On Mon, 26 Feb 2018 23:11:37 +0100, Rafael J. Wysocki wrote: >> On Mon, Feb 26, 2018 at 11:34 AM, Jean Delvare <jdelvare@suse.de> wrote: >> > When ACPI debugging is disabled, I see warnings like this one: >> > >> > drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap": >> > drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body] >> > drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body] >> > >> > It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving >> > to nothing. Make them resolve to the classic "do {} while (0)" >> > construct instead if the compiler likes that, or just {} if not, to >> > silent all such warnings. >> >> So first of all, acpi_smbus_cmi_add_cap() shouldn't really use >> ACPI_DEBUG_PRINT() and similar. They belong to ACPICA and their use >> should be limited to it. > > OK, thanks for the information. I'll update the i2c-scmi driver to > no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as > well though (xo15-ebook and panasonic-laptop, as well as > xen-acpi-memhotplug - not sure if you consider that one as more > legitimate.) None of them is valid IMO. ACPI_DEBUG_PRINT() really should only be used in the ACPICA code. And we have acpi_handle_debug() even for everybody else. >> I know that they are used in the other parts of the ACPI subsystem, >> but they really should be replaced with the kernel's proper debug >> statements in there. > > Ideally all these macros shouldn't be exposed to drivers which are not > supposed to use them. Could they be moved to a header file internal to > at least acpi? We get that code from the upstream and I don't want to differ from it in arbitrary ways. I'm not sure about the possible consequences of that change in the upstream code ATM, though. -- 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 Tue, 27 Feb 2018 11:14:07 +0100, Rafael J. Wysocki wrote: > On Tue, Feb 27, 2018 at 10:23 AM, Jean Delvare <jdelvare@suse.de> wrote: > > OK, thanks for the information. I'll update the i2c-scmi driver to > > no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as > > well though (xo15-ebook and panasonic-laptop, as well as > > xen-acpi-memhotplug - not sure if you consider that one as more > > legitimate.) > > None of them is valid IMO. ACPI_DEBUG_PRINT() really should only be > used in the ACPICA code. What about ACPI_ERROR()? The i2c-scmi driver uses these as well. Should I convert them to dev_err()? Thanks,
On Tue, Feb 27, 2018 at 1:37 PM, Jean Delvare <jdelvare@suse.de> wrote: > On Tue, 27 Feb 2018 11:14:07 +0100, Rafael J. Wysocki wrote: >> On Tue, Feb 27, 2018 at 10:23 AM, Jean Delvare <jdelvare@suse.de> wrote: >> > OK, thanks for the information. I'll update the i2c-scmi driver to >> > no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as >> > well though (xo15-ebook and panasonic-laptop, as well as >> > xen-acpi-memhotplug - not sure if you consider that one as more >> > legitimate.) >> >> None of them is valid IMO. ACPI_DEBUG_PRINT() really should only be >> used in the ACPICA code. > > What about ACPI_ERROR()? The i2c-scmi driver uses these as well. Should > I convert them to dev_err()? I think so. That or acpi_handle_error(). -- 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
--- linux-4.16-rc3.orig/include/acpi/acoutput.h 2018-02-26 10:57:56.803381047 +0100 +++ linux-4.16-rc3/include/acpi/acoutput.h 2018-02-26 11:16:41.569185359 +0100 @@ -249,6 +249,17 @@ #endif /* ACPI_NO_ERROR_MESSAGES */ /* + * Note: the ACPI_DO_WHILE0 macro is used to prevent some compilers from + * complaining about these constructs. On other compilers the do...while + * adds some extra code, so this feature is optional. + */ +#ifdef ACPI_USE_DO_WHILE_0 +#define ACPI_DO_WHILE0(a) do a while (0) +#else +#define ACPI_DO_WHILE0(a) a +#endif + +/* * Debug macros that are conditionally compiled */ #ifdef ACPI_DEBUG_OUTPUT @@ -297,16 +308,7 @@ * debug message outside of the print function itself. This improves overall * performance at a relatively small code cost. Implementation involves the * use of variadic macros supported by C99. - * - * Note: the ACPI_DO_WHILE0 macro is used to prevent some compilers from - * complaining about these constructs. On other compilers the do...while - * adds some extra code, so this feature is optional. */ -#ifdef ACPI_USE_DO_WHILE_0 -#define ACPI_DO_WHILE0(a) do a while(0) -#else -#define ACPI_DO_WHILE0(a) a -#endif /* DEBUG_PRINT functions */ @@ -458,23 +460,23 @@ * This is the non-debug case -- make everything go away, * leaving no executable debug code! */ -#define ACPI_DEBUG_PRINT(pl) -#define ACPI_DEBUG_PRINT_RAW(pl) -#define ACPI_DEBUG_EXEC(a) -#define ACPI_DEBUG_ONLY_MEMBERS(a) -#define ACPI_FUNCTION_NAME(a) -#define ACPI_FUNCTION_TRACE(a) -#define ACPI_FUNCTION_TRACE_PTR(a, b) -#define ACPI_FUNCTION_TRACE_U32(a, b) -#define ACPI_FUNCTION_TRACE_STR(a, b) -#define ACPI_FUNCTION_ENTRY() -#define ACPI_DUMP_STACK_ENTRY(a) -#define ACPI_DUMP_OPERANDS(a, b, c) -#define ACPI_DUMP_ENTRY(a, b) -#define ACPI_DUMP_PATHNAME(a, b, c, d) -#define ACPI_DUMP_BUFFER(a, b) +#define ACPI_DEBUG_PRINT(pl) ACPI_DO_WHILE0({}) +#define ACPI_DEBUG_PRINT_RAW(pl) ACPI_DO_WHILE0({}) +#define ACPI_DEBUG_EXEC(a) ACPI_DO_WHILE0({}) +#define ACPI_DEBUG_ONLY_MEMBERS(a) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_NAME(a) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_TRACE(a) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_TRACE_PTR(a, b) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_TRACE_U32(a, b) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_TRACE_STR(a, b) ACPI_DO_WHILE0({}) +#define ACPI_FUNCTION_ENTRY() ACPI_DO_WHILE0({}) +#define ACPI_DUMP_STACK_ENTRY(a) ACPI_DO_WHILE0({}) +#define ACPI_DUMP_OPERANDS(a, b, c) ACPI_DO_WHILE0({}) +#define ACPI_DUMP_ENTRY(a, b) ACPI_DO_WHILE0({}) +#define ACPI_DUMP_PATHNAME(a, b, c, d) ACPI_DO_WHILE0({}) +#define ACPI_DUMP_BUFFER(a, b) ACPI_DO_WHILE0({}) #define ACPI_IS_DEBUG_ENABLED(level, component) 0 -#define ACPI_TRACE_POINT(a, b, c, d) +#define ACPI_TRACE_POINT(a, b, c, d) ACPI_DO_WHILE0({}) /* Return macros must have a return statement at the minimum */
When ACPI debugging is disabled, I see warnings like this one: drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap": drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body] drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body] It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving to nothing. Make them resolve to the classic "do {} while (0)" construct instead if the compiler likes that, or just {} if not, to silent all such warnings. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Len Brown <lenb@kernel.org> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> Cc: Robert Moore <robert.moore@intel.com> Cc: Erik Schmauss <erik.schmauss@intel.com> --- include/acpi/acoutput.h | 52 ++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 25 deletions(-)