On Thu, Oct 27, 2022 at 5:20 PM Jonas Gorski <jonas.gorski@xxxxxxxxx> 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
