The <linux/bcma/bcma_driver_chipcommon.h> is including the legacy
header <linux/gpio.h> to obtain struct gpio_chip. Instead, include
<linux/gpio/driver.h> where this struct is defined.
It turns out that the brcm80211 brcmsmac depends on this to
bring in the symbol gpio_is_valid().
The driver looks up the BCMA parent GPIO driver and checks that
this succeeds, but then it goes on to use the deprecated GPIO
call gpio_is_valid() to check the consistency of the .base
member of the BCMA GPIO struct. Surely this belongs in the
BCMA driver: we cannot have all drivers performing cosistency
checks on the internals of things they are passed.
Acked-by: Arend van Spriel <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
ChangeLog v1->v2:
- Combine two co-dependent patches
- Collect Arend's ACK
---
drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c | 2 +-
include/linux/bcma/bcma_driver_chipcommon.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
index c1b9ac692d26..1cce92c5780f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
@@ -63,7 +63,7 @@ int brcms_led_register(struct brcms_info *wl)
int hwnum = -1;
enum gpio_lookup_flags lflags = GPIO_ACTIVE_HIGH;
- if (!bcma_gpio || !gpio_is_valid(bcma_gpio->base))
+ if (!bcma_gpio)
return -ENODEV;
/* find radio enabled LED */
diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h
index 2d94c30ed439..0cb6638b55e5 100644
--- a/include/linux/bcma/bcma_driver_chipcommon.h
+++ b/include/linux/bcma/bcma_driver_chipcommon.h
@@ -4,7 +4,7 @@
#include <linux/platform_device.h>
#include <linux/platform_data/brcmnand.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
/** ChipCommon core registers. **/
#define BCMA_CC_ID 0x0000
--
2.34.1
On Wed, 26 Oct 2022 at 10:05, Linus Walleij <[email protected]> wrote:
>
> The <linux/bcma/bcma_driver_chipcommon.h> is including the legacy
> header <linux/gpio.h> to obtain struct gpio_chip. Instead, include
> <linux/gpio/driver.h> where this struct is defined.
>
> It turns out that the brcm80211 brcmsmac depends on this to
> bring in the symbol gpio_is_valid().
>
> The driver looks up the BCMA parent GPIO driver and checks that
> this succeeds, but then it goes on to use the deprecated GPIO
> call gpio_is_valid() to check the consistency of the .base
> member of the BCMA GPIO struct. Surely this belongs in the
> BCMA driver: we cannot have all drivers performing cosistency
> checks on the internals of things they are passed.
>
> Acked-by: Arend van Spriel <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ChangeLog v1->v2:
> - Combine two co-dependent patches
> - Collect Arend's ACK
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c | 2 +-
> include/linux/bcma/bcma_driver_chipcommon.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> index c1b9ac692d26..1cce92c5780f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> @@ -63,7 +63,7 @@ int brcms_led_register(struct brcms_info *wl)
> int hwnum = -1;
> enum gpio_lookup_flags lflags = GPIO_ACTIVE_HIGH;
>
> - if (!bcma_gpio || !gpio_is_valid(bcma_gpio->base))
> + if (!bcma_gpio)
> return -ENODEV;
A few lines above bcma_gpio is initialized with
struct gpio_chip *bcma_gpio = &cc_drv->gpio;
and thus can never be NULL, so the whole condition can be then dropped.
I guess the intention here was to make sure the the gpio chip was
registered successfully. The bcma bus driver ignores any gpio chip
init/registration failures, so this is (in theory) a possible state.
Doing no validity check can then theoretically lead to a NULL pointer
access further down. brcms_led_register() calls
gpiochip_request_own_desc(), which calls gpiochip_get_desc() which
tries to access gc->gpiodev->ngpio. If the gpio_chip registration
failed for any reason, gc->gpiodev will be NULL => oops. The
gpio_is_valid() might have caught that as base might have been left as
-1, depending on where it failed.
So adding a check to gpiochip_get_desc() for gc->gpiodev being
populated should avoid this issue, and gpiochip_get_desc() doesn't
look like a function used in hot paths where the check would add a
performance degradation.
Regards
Jonas
On Wed, 2022-10-26 at 09:56 +0200, Linus Walleij wrote:
> Surely this belongs in the
> BCMA driver: we cannot have all drivers performing cosistency
> checks on the internals of things they are passed.
>
"Surely this doesn't belong"?
johannes
On Thu, Oct 27, 2022 at 5:20 PM Jonas Gorski <[email protected]> wrote:
> A few lines above bcma_gpio is initialized with
>
> struct gpio_chip *bcma_gpio = &cc_drv->gpio;
>
> and thus can never be NULL, so the whole condition can be then dropped.
I will send a v3 deleting the whole check.
> I guess the intention here was to make sure the the gpio chip was
> registered successfully. The bcma bus driver ignores any gpio chip
> init/registration failures, so this is (in theory) a possible state.
>
> Doing no validity check can then theoretically lead to a NULL pointer
> access further down. brcms_led_register() calls
> gpiochip_request_own_desc(), which calls gpiochip_get_desc() which
> tries to access gc->gpiodev->ngpio. If the gpio_chip registration
> failed for any reason, gc->gpiodev will be NULL => oops. The
> gpio_is_valid() might have caught that as base might have been left as
> -1, depending on where it failed.
>
> So adding a check to gpiochip_get_desc() for gc->gpiodev being
> populated should avoid this issue, and gpiochip_get_desc() doesn't
> look like a function used in hot paths where the check would add a
> performance degradation.
Hmmm that sounds like adding consistency checks to gpiolib for
fixing the fact that the BCMA driver ignores registration failures.
I would rather fix the BCMA driver to not do that, I'll check if I can make
a separate patch for this.
Yours,
Linus Walleij