* Re: [PATCH] bluetooth: hci_h5: avoid unused variable warnings
2018-08-13 22:04 [PATCH] bluetooth: hci_h5: avoid unused variable warnings Arnd Bergmann
@ 2018-08-13 22:36 ` Kees Cook
2018-08-14 7:02 ` Marcel Holtmann
2018-08-14 7:22 ` Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2018-08-13 22:36 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Marcel Holtmann, Johan Hedberg, Hans de Goede, Jeremy Cline,
linux-bluetooth, LKML
On Mon, Aug 13, 2018 at 3:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> When CONFIG_BT_HCIUART_RTL is disabled, the hci_h5 driver produces a build
> warning because of an incorrect set of #ifdef guards:
>
> drivers/bluetooth/hci_h5.c:920:22: error: 'rtl_vnd' defined but not used [-Werror=unused-variable]
>
> Replacing the #ifdef with an IS_ENABLED() check shuts up the error
> and simplifies the logic.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> drivers/bluetooth/hci_h5.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 8eede1197cd2..b10e64ba0ff4 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -841,7 +841,6 @@ static void h5_serdev_remove(struct serdev_device *serdev)
> hci_uart_unregister_device(&h5->serdev_hu);
> }
>
> -#ifdef CONFIG_BT_HCIUART_RTL
> static int h5_btrtl_setup(struct h5 *h5)
> {
> struct btrtl_device_info *btrtl_dev;
> @@ -923,24 +922,21 @@ static struct h5_vnd rtl_vnd = {
> .close = h5_btrtl_close,
> .acpi_gpio_map = acpi_btrtl_gpios,
> };
> -#endif
>
> -#ifdef CONFIG_ACPI
> static const struct acpi_device_id h5_acpi_match[] = {
> -#ifdef CONFIG_BT_HCIUART_RTL
> { "OBDA8723", (kernel_ulong_t)&rtl_vnd },
> -#endif
> { },
> };
> MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
> -#endif
>
> static struct serdev_device_driver h5_serdev_driver = {
> .probe = h5_serdev_probe,
> .remove = h5_serdev_remove,
> .driver = {
> .name = "hci_uart_h5",
> - .acpi_match_table = ACPI_PTR(h5_acpi_match),
> + .acpi_match_table = (IS_ENABLED(CONFIG_BT_HCIUART_RTL) &&
> + IS_ENABLED(CONFIG_ACPI)) ?
> + h5_acpi_match : 0,
> },
> };
>
> --
> 2.18.0
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] bluetooth: hci_h5: avoid unused variable warnings
2018-08-13 22:04 [PATCH] bluetooth: hci_h5: avoid unused variable warnings Arnd Bergmann
2018-08-13 22:36 ` Kees Cook
@ 2018-08-14 7:02 ` Marcel Holtmann
2018-08-14 7:22 ` Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2018-08-14 7:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Johan Hedberg, Hans de Goede, Jeremy Cline, Kees Cook,
linux-bluetooth, linux-kernel
Hi Arnd,
> When CONFIG_BT_HCIUART_RTL is disabled, the hci_h5 driver produces a build
> warning because of an incorrect set of #ifdef guards:
>
> drivers/bluetooth/hci_h5.c:920:22: error: 'rtl_vnd' defined but not used [-Werror=unused-variable]
>
> Replacing the #ifdef with an IS_ENABLED() check shuts up the error
> and simplifies the logic.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/bluetooth/hci_h5.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 8eede1197cd2..b10e64ba0ff4 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -841,7 +841,6 @@ static void h5_serdev_remove(struct serdev_device *serdev)
> hci_uart_unregister_device(&h5->serdev_hu);
> }
>
> -#ifdef CONFIG_BT_HCIUART_RTL
> static int h5_btrtl_setup(struct h5 *h5)
> {
> struct btrtl_device_info *btrtl_dev;
> @@ -923,24 +922,21 @@ static struct h5_vnd rtl_vnd = {
> .close = h5_btrtl_close,
> .acpi_gpio_map = acpi_btrtl_gpios,
> };
> -#endif
>
> -#ifdef CONFIG_ACPI
> static const struct acpi_device_id h5_acpi_match[] = {
> -#ifdef CONFIG_BT_HCIUART_RTL
> { "OBDA8723", (kernel_ulong_t)&rtl_vnd },
> -#endif
> { },
> };
> MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
> -#endif
>
> static struct serdev_device_driver h5_serdev_driver = {
> .probe = h5_serdev_probe,
> .remove = h5_serdev_remove,
> .driver = {
> .name = "hci_uart_h5",
> - .acpi_match_table = ACPI_PTR(h5_acpi_match),
> + .acpi_match_table = (IS_ENABLED(CONFIG_BT_HCIUART_RTL) &&
> + IS_ENABLED(CONFIG_ACPI)) ?
> + h5_acpi_match : 0,
> },
I do not like this construct at all. It will also blow up once we have a second vendor with H:5 support. We need to find another way to solve this.
Regards
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] bluetooth: hci_h5: avoid unused variable warnings
2018-08-13 22:04 [PATCH] bluetooth: hci_h5: avoid unused variable warnings Arnd Bergmann
2018-08-13 22:36 ` Kees Cook
2018-08-14 7:02 ` Marcel Holtmann
@ 2018-08-14 7:22 ` Hans de Goede
2018-08-14 8:42 ` Arnd Bergmann
2 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2018-08-14 7:22 UTC (permalink / raw)
To: Arnd Bergmann, Marcel Holtmann, Johan Hedberg
Cc: Jeremy Cline, Kees Cook, linux-bluetooth, linux-kernel
Hi Arnd,
On 14-08-18 00:04, Arnd Bergmann wrote:
> When CONFIG_BT_HCIUART_RTL is disabled, the hci_h5 driver produces a build
> warning because of an incorrect set of #ifdef guards:
>
> drivers/bluetooth/hci_h5.c:920:22: error: 'rtl_vnd' defined but not used [-Werror=unused-variable]
>
> Replacing the #ifdef with an IS_ENABLED() check shuts up the error
> and simplifies the logic.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Hmm, so now you're counting on dead code elimination to remove
references to unavailable symbols. I'm not sure I like this and
since Marcel has also indicated the does not like this fix I
believe we need another fix.
I will write another fix for this.
Regards,
Hans
> ---
> drivers/bluetooth/hci_h5.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 8eede1197cd2..b10e64ba0ff4 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -841,7 +841,6 @@ static void h5_serdev_remove(struct serdev_device *serdev)
> hci_uart_unregister_device(&h5->serdev_hu);
> }
>
> -#ifdef CONFIG_BT_HCIUART_RTL
> static int h5_btrtl_setup(struct h5 *h5)
> {
> struct btrtl_device_info *btrtl_dev;
> @@ -923,24 +922,21 @@ static struct h5_vnd rtl_vnd = {
> .close = h5_btrtl_close,
> .acpi_gpio_map = acpi_btrtl_gpios,
> };
> -#endif
>
> -#ifdef CONFIG_ACPI
> static const struct acpi_device_id h5_acpi_match[] = {
> -#ifdef CONFIG_BT_HCIUART_RTL
> { "OBDA8723", (kernel_ulong_t)&rtl_vnd },
> -#endif
> { },
> };
> MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
> -#endif
>
> static struct serdev_device_driver h5_serdev_driver = {
> .probe = h5_serdev_probe,
> .remove = h5_serdev_remove,
> .driver = {
> .name = "hci_uart_h5",
> - .acpi_match_table = ACPI_PTR(h5_acpi_match),
> + .acpi_match_table = (IS_ENABLED(CONFIG_BT_HCIUART_RTL) &&
> + IS_ENABLED(CONFIG_ACPI)) ?
> + h5_acpi_match : 0,
> },
> };
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] bluetooth: hci_h5: avoid unused variable warnings
2018-08-14 7:22 ` Hans de Goede
@ 2018-08-14 8:42 ` Arnd Bergmann
0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2018-08-14 8:42 UTC (permalink / raw)
To: Hans de Goede
Cc: Marcel Holtmann, Johan Hedberg, jeremy, Kees Cook,
Bluez mailing list, Linux Kernel Mailing List
On Tue, Aug 14, 2018 at 9:22 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Arnd,
>
> On 14-08-18 00:04, Arnd Bergmann wrote:
> > When CONFIG_BT_HCIUART_RTL is disabled, the hci_h5 driver produces a build
> > warning because of an incorrect set of #ifdef guards:
> >
> > drivers/bluetooth/hci_h5.c:920:22: error: 'rtl_vnd' defined but not used [-Werror=unused-variable]
> >
> > Replacing the #ifdef with an IS_ENABLED() check shuts up the error
> > and simplifies the logic.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Hmm, so now you're counting on dead code elimination to remove
> references to unavailable symbols.
Generally depending on dead code elimination is very reliable, as this
is easy for gcc to get right. However, you are right that this is actually
an exception since the MODULE_DEVICE_TABLE() entry will keep
a reference around that prevents the compiler from eliminating it.
> I'm not sure I like this and
> since Marcel has also indicated the does not like this fix I
> believe we need another fix.
>
> I will write another fix for this.
>
Ok, thanks!
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread