| Message ID | 20221026075653.105387-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: bcma/brcm80211: Use the proper include | expand |
On Wed, 26 Oct 2022 at 10:05, Linus Walleij <linus.walleij@linaro.org> 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 <arend.vanspriel@broadcom.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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 <jonas.gorski@gmail.com> 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
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