| Message ID | 20190802100349.8659-2-frieder.schrempf@kontron.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/4] serial: mctrl_gpio: Avoid probe failures in case of missing gpiolib | expand |
On Fri, Aug 02, 2019 at 10:04:10AM +0000, Schrempf Frieder wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > As it is allowed to use the mctrl_gpio_* functions before > initialization (as the 8250 driver does according to 434be0ae7aa7), Actually I was surprised some time ago that 8250 used serial_mctrl without first initializing it and expecting it to work. I didn't look in detail, but I wouldn't go so far to call this "allowed". The commit itself calls it "workaround" which seems a better match. > it seems appropriate to have a NULL check in all of the functions. > Otherwise the mctrl_gpio_to_gpiod() function is prone to be used > in a context that can lead to a NULL pointer dereference. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> Best regards Uwe
On 02.08.19 14:12, Uwe Kleine-König wrote: > On Fri, Aug 02, 2019 at 10:04:10AM +0000, Schrempf Frieder wrote: >> From: Frieder Schrempf <frieder.schrempf@kontron.de> >> >> As it is allowed to use the mctrl_gpio_* functions before >> initialization (as the 8250 driver does according to 434be0ae7aa7), > > Actually I was surprised some time ago that 8250 used serial_mctrl > without first initializing it and expecting it to work. I didn't look in > detail, but I wouldn't go so far to call this "allowed". The commit > itself calls it "workaround" which seems a better match. Ok, but if this is considered to be a workaround and as the 8250 driver does not use mctrl_gpio_to_gpiod(), we should maybe just drop this patch instead of encouraging others to use mctrl_gpio before initialization. I'm really not sure what's best, so depending on what you will propose, I'll send a new version of this patch with adjusted commit message or not. By the way, Uwe and Fabio: Thanks for your reviews! > >> it seems appropriate to have a NULL check in all of the functions. >> Otherwise the mctrl_gpio_to_gpiod() function is prone to be used >> in a context that can lead to a NULL pointer dereference. >> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > Best regards > Uwe >
Hello Frieder, On Mon, Aug 05, 2019 at 09:01:39AM +0000, Schrempf Frieder wrote: > On 02.08.19 14:12, Uwe Kleine-König wrote: > > On Fri, Aug 02, 2019 at 10:04:10AM +0000, Schrempf Frieder wrote: > >> From: Frieder Schrempf <frieder.schrempf@kontron.de> > >> > >> As it is allowed to use the mctrl_gpio_* functions before > >> initialization (as the 8250 driver does according to 434be0ae7aa7), > > > > Actually I was surprised some time ago that 8250 used serial_mctrl > > without first initializing it and expecting it to work. I didn't look in > > detail, but I wouldn't go so far to call this "allowed". The commit > > itself calls it "workaround" which seems a better match. > > Ok, but if this is considered to be a workaround and as the 8250 driver > does not use mctrl_gpio_to_gpiod(), we should maybe just drop this patch > instead of encouraging others to use mctrl_gpio before initialization. > > I'm really not sure what's best, so depending on what you will propose, > I'll send a new version of this patch with adjusted commit message or not. I wouldn't encourage usage of mctrl-gpio before it's initialized. So I suggest to drop this patch. Best regards Uwe
On 06.08.19 09:45, Uwe Kleine-König wrote: > Hello Frieder, > > On Mon, Aug 05, 2019 at 09:01:39AM +0000, Schrempf Frieder wrote: >> On 02.08.19 14:12, Uwe Kleine-König wrote: >>> On Fri, Aug 02, 2019 at 10:04:10AM +0000, Schrempf Frieder wrote: >>>> From: Frieder Schrempf <frieder.schrempf@kontron.de> >>>> >>>> As it is allowed to use the mctrl_gpio_* functions before >>>> initialization (as the 8250 driver does according to 434be0ae7aa7), >>> >>> Actually I was surprised some time ago that 8250 used serial_mctrl >>> without first initializing it and expecting it to work. I didn't look in >>> detail, but I wouldn't go so far to call this "allowed". The commit >>> itself calls it "workaround" which seems a better match. >> >> Ok, but if this is considered to be a workaround and as the 8250 driver >> does not use mctrl_gpio_to_gpiod(), we should maybe just drop this patch >> instead of encouraging others to use mctrl_gpio before initialization. >> >> I'm really not sure what's best, so depending on what you will propose, >> I'll send a new version of this patch with adjusted commit message or not. > > I wouldn't encourage usage of mctrl-gpio before it's initialized. So I > suggest to drop this patch. Ok, thanks.
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index 2b400189be91..54c43e02e375 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -61,6 +61,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set); struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, enum mctrl_gpio_idx gidx) { + if (gpios == NULL) + return NULL; + return gpios->gpio[gidx]; } EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod);