| Message ID | CAGm1_kuTAozujz_tHthr0QUZGcKnU0aSc-AMUZsw7odwvViOuA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Yegor, On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: > I've got a kernel crash from kernel robot. If we use UART before > general initialization (earlyprintk), then any call to mctrl API would > result in NULL pointer dereference. One solution would be to check, if > gpios IS_ERR_OR_NULL(). See below: > > --- a/drivers/tty/serial/serial_mctrl_gpio.c > +++ b/drivers/tty/serial/serial_mctrl_gpio.c > @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, > unsigned int mctrl) > int value_array[UART_GPIO_MAX]; > unsigned int count = 0; > > + if (IS_ERR_OR_NULL(gpios)) > + return; > + > for (i = 0; i < UART_GPIO_MAX; i++) > if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { > desc_array[count] = gpios->gpio[i]; IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value that was returned by mctrl_gpio_init, this never returns NULL and if it returns an error you're supposed to not register the port. And for early printk there is AFAIK no mctrl involved. > > static inline int serial8250_in_MCR(struct uart_8250_port *up) > > { > > - return serial_in(up, UART_MCR); > > + int mctrl, mctrl_gpio = 0; > > + > > + mctrl = serial_in(up, UART_MCR); > > + > > + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio); I forgot where mctrl_gpio_get_outputs came from, is it better than/different from mctrl_gpio_get? > > + > > + if (mctrl_gpio & TIOCM_RTS) > > + mctrl |= UART_MCR_RTS; > > + else > > + mctrl &= ~UART_MCR_RTS; > > + > > + if (mctrl_gpio & TIOCM_DTR) > > + mctrl |= UART_MCR_DTR; > > + else > > + mctrl &= ~UART_MCR_DTR; > > + > > + if (mctrl_gpio & TIOCM_OUT1) > > + mctrl |= UART_MCR_OUT1; > > + else > > + mctrl &= ~UART_MCR_OUT1; > > + > > + if (mctrl_gpio & TIOCM_OUT2) > > + mctrl |= UART_MCR_OUT2; > > + else > > + mctrl &= ~UART_MCR_OUT2; > > Should we perhaps check, if particular signal is really implemented as GPIO? The intended usage is: mctrl = read_mctrl_signals_from_hardware(port); return mctrl_gpio_get(gpios, &mctrl); mctrl_gpio_get then overwrites all values it is responsible for. Best regards Uwe
On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Yegor, > > On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >> I've got a kernel crash from kernel robot. If we use UART before >> general initialization (earlyprintk), then any call to mctrl API would >> result in NULL pointer dereference. One solution would be to check, if >> gpios IS_ERR_OR_NULL(). See below: >> >> --- a/drivers/tty/serial/serial_mctrl_gpio.c >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >> unsigned int mctrl) >> int value_array[UART_GPIO_MAX]; >> unsigned int count = 0; >> >> + if (IS_ERR_OR_NULL(gpios)) >> + return; >> + >> for (i = 0; i < UART_GPIO_MAX; i++) >> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >> desc_array[count] = gpios->gpio[i]; > > IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value > that was returned by mctrl_gpio_init, this never returns NULL and if it > returns an error you're supposed to not register the port. And for early > printk there is AFAIK no mctrl involved. You're right. it was console_init stuff. It happens before serial8250_register_8250_port(). Perhaps I should introduce one more gpio_init invocation in univ8250_console_setup(). [ 0.000000] [<ffffffff810d9e16>] ? lock_acquire+0xa6/0xe0 [ 0.000000] [<ffffffff816f6e06>] ? serial8250_do_set_termios+0xf6/0x3f0 [ 0.000000] [<ffffffff816f3627>] ? default_serial_dl_write+0x27/0x30 [ 0.000000] [<ffffffff816f65d3>] serial8250_do_set_mctrl+0xa3/0xb0 [ 0.000000] [<ffffffff816f65f6>] serial8250_set_mctrl+0x16/0x20 [ 0.000000] [<ffffffff816f6fec>] serial8250_do_set_termios+0x2dc/0x3f0 [ 0.000000] [<ffffffff816f7116>] serial8250_set_termios+0x16/0x20 [ 0.000000] [<ffffffff816f1054>] uart_set_options+0xf4/0x150 [ 0.000000] [<ffffffff816f754f>] serial8250_console_setup+0x7f/0x130 [ 0.000000] [<ffffffff816f22c9>] univ8250_console_setup+0x39/0x50 [ 0.000000] [<ffffffff810e4075>] register_console+0x295/0x370 [ 0.000000] [<ffffffff832c4c52>] univ8250_console_init+0x1e/0x28 [ 0.000000] [<ffffffff832c4328>] console_init+0x1c/0x25 [ 0.000000] [<ffffffff83289dc2>] start_kernel+0x2cc/0x44c [ 0.000000] [<ffffffff83289120>] ? early_idt_handler_array+0x120/0x120 [ 0.000000] [<ffffffff83289354>] x86_64_start_reservations+0x38/0x3a [ 0.000000] [<ffffffff83289441>] x86_64_start_kernel+0xeb/0xf8 >> > static inline int serial8250_in_MCR(struct uart_8250_port *up) >> > { >> > - return serial_in(up, UART_MCR); >> > + int mctrl, mctrl_gpio = 0; >> > + >> > + mctrl = serial_in(up, UART_MCR); >> > + >> > + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio); > > I forgot where mctrl_gpio_get_outputs came from, is it better > than/different from mctrl_gpio_get? mctrl_gpio_get - returns inputs (DTS, CTS etc.), mctrl_gpio_get_outputs - returns output signals RTS. DTR etc. >> > + >> > + if (mctrl_gpio & TIOCM_RTS) >> > + mctrl |= UART_MCR_RTS; >> > + else >> > + mctrl &= ~UART_MCR_RTS; >> > + >> > + if (mctrl_gpio & TIOCM_DTR) >> > + mctrl |= UART_MCR_DTR; >> > + else >> > + mctrl &= ~UART_MCR_DTR; >> > + >> > + if (mctrl_gpio & TIOCM_OUT1) >> > + mctrl |= UART_MCR_OUT1; >> > + else >> > + mctrl &= ~UART_MCR_OUT1; >> > + >> > + if (mctrl_gpio & TIOCM_OUT2) >> > + mctrl |= UART_MCR_OUT2; >> > + else >> > + mctrl &= ~UART_MCR_OUT2; >> >> Should we perhaps check, if particular signal is really implemented as GPIO? > > The intended usage is: > > mctrl = read_mctrl_signals_from_hardware(port); > return mctrl_gpio_get(gpios, &mctrl); > > mctrl_gpio_get then overwrites all values it is responsible for. You're right. I'll rework this. Yegor -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/05/2016 06:25 AM, Yegor Yefremov wrote: > On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> Hello Yegor, >> >> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>> I've got a kernel crash from kernel robot. If we use UART before >>> general initialization (earlyprintk), then any call to mctrl API would >>> result in NULL pointer dereference. One solution would be to check, if >>> gpios IS_ERR_OR_NULL(). See below: >>> >>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>> unsigned int mctrl) >>> int value_array[UART_GPIO_MAX]; >>> unsigned int count = 0; >>> >>> + if (IS_ERR_OR_NULL(gpios)) >>> + return; >>> + >>> for (i = 0; i < UART_GPIO_MAX; i++) >>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>> desc_array[count] = gpios->gpio[i]; >> >> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >> that was returned by mctrl_gpio_init, this never returns NULL and if it >> returns an error you're supposed to not register the port. And for early >> printk there is AFAIK no mctrl involved. > > You're right. it was console_init stuff. It happens before > serial8250_register_8250_port(). Perhaps I should introduce one more > gpio_init invocation in univ8250_console_setup(). No. There is no dev at console_init time. Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios > > [ 0.000000] [<ffffffff810d9e16>] ? lock_acquire+0xa6/0xe0 > [ 0.000000] [<ffffffff816f6e06>] ? serial8250_do_set_termios+0xf6/0x3f0 > [ 0.000000] [<ffffffff816f3627>] ? default_serial_dl_write+0x27/0x30 > [ 0.000000] [<ffffffff816f65d3>] serial8250_do_set_mctrl+0xa3/0xb0 > [ 0.000000] [<ffffffff816f65f6>] serial8250_set_mctrl+0x16/0x20 > [ 0.000000] [<ffffffff816f6fec>] serial8250_do_set_termios+0x2dc/0x3f0 > [ 0.000000] [<ffffffff816f7116>] serial8250_set_termios+0x16/0x20 > [ 0.000000] [<ffffffff816f1054>] uart_set_options+0xf4/0x150 > [ 0.000000] [<ffffffff816f754f>] serial8250_console_setup+0x7f/0x130 > [ 0.000000] [<ffffffff816f22c9>] univ8250_console_setup+0x39/0x50 > [ 0.000000] [<ffffffff810e4075>] register_console+0x295/0x370 > [ 0.000000] [<ffffffff832c4c52>] univ8250_console_init+0x1e/0x28 > [ 0.000000] [<ffffffff832c4328>] console_init+0x1c/0x25 > [ 0.000000] [<ffffffff83289dc2>] start_kernel+0x2cc/0x44c > [ 0.000000] [<ffffffff83289120>] ? early_idt_handler_array+0x120/0x120 > [ 0.000000] [<ffffffff83289354>] x86_64_start_reservations+0x38/0x3a > [ 0.000000] [<ffffffff83289441>] x86_64_start_kernel+0xeb/0xf8 > >>>> static inline int serial8250_in_MCR(struct uart_8250_port *up) >>>> { >>>> - return serial_in(up, UART_MCR); >>>> + int mctrl, mctrl_gpio = 0; >>>> + >>>> + mctrl = serial_in(up, UART_MCR); >>>> + >>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio); >> >> I forgot where mctrl_gpio_get_outputs came from, is it better >> than/different from mctrl_gpio_get? > > mctrl_gpio_get - returns inputs (DTS, CTS etc.), > mctrl_gpio_get_outputs - returns output signals RTS. DTR etc. > >>>> + >>>> + if (mctrl_gpio & TIOCM_RTS) >>>> + mctrl |= UART_MCR_RTS; >>>> + else >>>> + mctrl &= ~UART_MCR_RTS; >>>> + >>>> + if (mctrl_gpio & TIOCM_DTR) >>>> + mctrl |= UART_MCR_DTR; >>>> + else >>>> + mctrl &= ~UART_MCR_DTR; >>>> + >>>> + if (mctrl_gpio & TIOCM_OUT1) >>>> + mctrl |= UART_MCR_OUT1; >>>> + else >>>> + mctrl &= ~UART_MCR_OUT1; >>>> + >>>> + if (mctrl_gpio & TIOCM_OUT2) >>>> + mctrl |= UART_MCR_OUT2; >>>> + else >>>> + mctrl &= ~UART_MCR_OUT2; >>> >>> Should we perhaps check, if particular signal is really implemented as GPIO? >> >> The intended usage is: >> >> mctrl = read_mctrl_signals_from_hardware(port); >> return mctrl_gpio_get(gpios, &mctrl); >> >> mctrl_gpio_get then overwrites all values it is responsible for. > > You're right. I'll rework this. > > Yegor > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Peter, hello Yegor, On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: > On 04/05/2016 06:25 AM, Yegor Yefremov wrote: > > On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > >> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: > >>> I've got a kernel crash from kernel robot. If we use UART before > >>> general initialization (earlyprintk), then any call to mctrl API would > >>> result in NULL pointer dereference. One solution would be to check, if > >>> gpios IS_ERR_OR_NULL(). See below: > >>> > >>> --- a/drivers/tty/serial/serial_mctrl_gpio.c > >>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c > >>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, > >>> unsigned int mctrl) > >>> int value_array[UART_GPIO_MAX]; > >>> unsigned int count = 0; > >>> > >>> + if (IS_ERR_OR_NULL(gpios)) > >>> + return; > >>> + > >>> for (i = 0; i < UART_GPIO_MAX; i++) > >>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { > >>> desc_array[count] = gpios->gpio[i]; > >> > >> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value > >> that was returned by mctrl_gpio_init, this never returns NULL and if it > >> returns an error you're supposed to not register the port. And for early > >> printk there is AFAIK no mctrl involved. > > > > You're right. it was console_init stuff. It happens before > > serial8250_register_8250_port(). Perhaps I should introduce one more > > gpio_init invocation in univ8250_console_setup(). If the port isn't registered yet, nobody should call the port's .set_mctrl. So your plan sounds wrong for this reason, too. > No. There is no dev at console_init time. Ack. > Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios This would work, but sounds wrong for the above reason, too. I'd like to reserve gpios=NULL for the case where no gpio has to be controlled, so please don't use it as indication if mctrl_gpio_init was called. > >>>> static inline int serial8250_in_MCR(struct uart_8250_port *up) > >>>> { > >>>> - return serial_in(up, UART_MCR); > >>>> + int mctrl, mctrl_gpio = 0; > >>>> + > >>>> + mctrl = serial_in(up, UART_MCR); > >>>> + > >>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio); > >> > >> I forgot where mctrl_gpio_get_outputs came from, is it better > >> than/different from mctrl_gpio_get? > > > > mctrl_gpio_get - returns inputs (DTS, CTS etc.), > > mctrl_gpio_get_outputs - returns output signals RTS. DTR etc. You shouldn't need mctrl_gpio_get_outputs. What for do you think you need it? Best regards Uwe
On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: > On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: >> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: >>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>>>> I've got a kernel crash from kernel robot. If we use UART before >>>>> general initialization (earlyprintk), then any call to mctrl API would >>>>> result in NULL pointer dereference. One solution would be to check, if >>>>> gpios IS_ERR_OR_NULL(). See below: >>>>> >>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>>>> unsigned int mctrl) >>>>> int value_array[UART_GPIO_MAX]; >>>>> unsigned int count = 0; >>>>> >>>>> + if (IS_ERR_OR_NULL(gpios)) >>>>> + return; >>>>> + >>>>> for (i = 0; i < UART_GPIO_MAX; i++) >>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>>>> desc_array[count] = gpios->gpio[i]; >>>> >>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >>>> that was returned by mctrl_gpio_init, this never returns NULL and if it >>>> returns an error you're supposed to not register the port. And for early >>>> printk there is AFAIK no mctrl involved. >>> >>> You're right. it was console_init stuff. It happens before >>> serial8250_register_8250_port(). Perhaps I should introduce one more >>> gpio_init invocation in univ8250_console_setup(). > > If the port isn't registered yet, nobody should call the port's > .set_mctrl. So your plan sounds wrong for this reason, too. The 8250 driver initializes MCR from mctrl in its set_termios method: uart_set_options mctrl |= TIOCM_DTR ->set_termios => serial8250_set_termios serial8250_set_mctrl >> No. There is no dev at console_init time. > > Ack. > >> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios > > This would work, but sounds wrong for the above reason, too. I'd like to > reserve gpios=NULL for the case where no gpio has to be controlled, so > please don't use it as indication if mctrl_gpio_init was called. I'm confused; what operations will be different if gpios==NULL? And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), performing no action in that case? >>>>>> static inline int serial8250_in_MCR(struct uart_8250_port *up) >>>>>> { >>>>>> - return serial_in(up, UART_MCR); >>>>>> + int mctrl, mctrl_gpio = 0; >>>>>> + >>>>>> + mctrl = serial_in(up, UART_MCR); >>>>>> + >>>>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio); >>>> >>>> I forgot where mctrl_gpio_get_outputs came from, is it better >>>> than/different from mctrl_gpio_get? >>> >>> mctrl_gpio_get - returns inputs (DTS, CTS etc.), >>> mctrl_gpio_get_outputs - returns output signals RTS. DTR etc. > > You shouldn't need mctrl_gpio_get_outputs. What for do you think you > need it? > > Best regards > Uwe > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: > On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: > > On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: > >> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: > >>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König > >>> <u.kleine-koenig@pengutronix.de> wrote: > >>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: > >>>>> I've got a kernel crash from kernel robot. If we use UART before > >>>>> general initialization (earlyprintk), then any call to mctrl API would > >>>>> result in NULL pointer dereference. One solution would be to check, if > >>>>> gpios IS_ERR_OR_NULL(). See below: > >>>>> > >>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c > >>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c > >>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, > >>>>> unsigned int mctrl) > >>>>> int value_array[UART_GPIO_MAX]; > >>>>> unsigned int count = 0; > >>>>> > >>>>> + if (IS_ERR_OR_NULL(gpios)) > >>>>> + return; > >>>>> + > >>>>> for (i = 0; i < UART_GPIO_MAX; i++) > >>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { > >>>>> desc_array[count] = gpios->gpio[i]; > >>>> > >>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value > >>>> that was returned by mctrl_gpio_init, this never returns NULL and if it > >>>> returns an error you're supposed to not register the port. And for early > >>>> printk there is AFAIK no mctrl involved. > >>> > >>> You're right. it was console_init stuff. It happens before > >>> serial8250_register_8250_port(). Perhaps I should introduce one more > >>> gpio_init invocation in univ8250_console_setup(). > > > > If the port isn't registered yet, nobody should call the port's > > .set_mctrl. So your plan sounds wrong for this reason, too. > > The 8250 driver initializes MCR from mctrl in its set_termios method: MCR is a register and mctrl a member of struct port, right? > uart_set_options > mctrl |= TIOCM_DTR > ->set_termios => serial8250_set_termios > serial8250_set_mctrl Then maybe the bug is that uart_set_options calls serial8250_set_mctrl which is supposed to be only called after the device is probed? > >> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios > > > > This would work, but sounds wrong for the above reason, too. I'd like to > > reserve gpios=NULL for the case where no gpio has to be controlled, so > > please don't use it as indication if mctrl_gpio_init was called. > > I'm confused; what operations will be different if gpios==NULL? > And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), > performing no action in that case? OK, the right thing would happen. Still I'd prefer if a serial driver did not try to interpret what a certain value means or not. I'd say the only allowed operations on a gpios value are calling mctrl_gpio functions and use IS_ERR and PTR_ERR during probe. Best regards Uwe
On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: > Hello, > > On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: >> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: >>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: >>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: >>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König >>>>> <u.kleine-koenig@pengutronix.de> wrote: >>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>>>>>> I've got a kernel crash from kernel robot. If we use UART before >>>>>>> general initialization (earlyprintk), then any call to mctrl API would >>>>>>> result in NULL pointer dereference. One solution would be to check, if >>>>>>> gpios IS_ERR_OR_NULL(). See below: >>>>>>> >>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>>>>>> unsigned int mctrl) >>>>>>> int value_array[UART_GPIO_MAX]; >>>>>>> unsigned int count = 0; >>>>>>> >>>>>>> + if (IS_ERR_OR_NULL(gpios)) >>>>>>> + return; >>>>>>> + >>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) >>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>>>>>> desc_array[count] = gpios->gpio[i]; >>>>>> >>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it >>>>>> returns an error you're supposed to not register the port. And for early >>>>>> printk there is AFAIK no mctrl involved. >>>>> >>>>> You're right. it was console_init stuff. It happens before >>>>> serial8250_register_8250_port(). Perhaps I should introduce one more >>>>> gpio_init invocation in univ8250_console_setup(). >>> >>> If the port isn't registered yet, nobody should call the port's >>> .set_mctrl. So your plan sounds wrong for this reason, too. >> >> The 8250 driver initializes MCR from mctrl in its set_termios method: > > MCR is a register and mctrl a member of struct port, right? Yes. mctrl is the serial core's abstraction of the _modem control_ line state. MCR is the hardware implementation of that abstraction. >> uart_set_options >> mctrl |= TIOCM_DTR >> ->set_termios => serial8250_set_termios >> serial8250_set_mctrl > > Then maybe the bug is that uart_set_options calls serial8250_set_mctrl > which is supposed to be only called after the device is probed? Nope; DTR should be asserted when the console is initialized. I understand that is not possible with the mctrl helpers right now, but that's no reason to break other setups that do the right thing. >>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios >>> >>> This would work, but sounds wrong for the above reason, too. I'd like to >>> reserve gpios=NULL for the case where no gpio has to be controlled, so >>> please don't use it as indication if mctrl_gpio_init was called. >> >> I'm confused; what operations will be different if gpios==NULL? >> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), >> performing no action in that case? > > OK, the right thing would happen. Still I'd prefer if a serial driver > did not try to interpret what a certain value means or not. I'd say the > only allowed operations on a gpios value are calling mctrl_gpio > functions and use IS_ERR and PTR_ERR during probe. Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() instead, right? Because that's "the case where no gpio has to be controlled" because there is no gpio yet. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote: > On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: > > On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: > >> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: > >>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: > >>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: > >>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König > >>>>> <u.kleine-koenig@pengutronix.de> wrote: > >>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: > >>>>>>> I've got a kernel crash from kernel robot. If we use UART before > >>>>>>> general initialization (earlyprintk), then any call to mctrl API would > >>>>>>> result in NULL pointer dereference. One solution would be to check, if > >>>>>>> gpios IS_ERR_OR_NULL(). See below: > >>>>>>> > >>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c > >>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c > >>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, > >>>>>>> unsigned int mctrl) > >>>>>>> int value_array[UART_GPIO_MAX]; > >>>>>>> unsigned int count = 0; > >>>>>>> > >>>>>>> + if (IS_ERR_OR_NULL(gpios)) > >>>>>>> + return; > >>>>>>> + > >>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) > >>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { > >>>>>>> desc_array[count] = gpios->gpio[i]; > >>>>>> > >>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value > >>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it > >>>>>> returns an error you're supposed to not register the port. And for early > >>>>>> printk there is AFAIK no mctrl involved. > >>>>> > >>>>> You're right. it was console_init stuff. It happens before > >>>>> serial8250_register_8250_port(). Perhaps I should introduce one more > >>>>> gpio_init invocation in univ8250_console_setup(). > >>> > >>> If the port isn't registered yet, nobody should call the port's > >>> .set_mctrl. So your plan sounds wrong for this reason, too. > >> > >> The 8250 driver initializes MCR from mctrl in its set_termios method: > >> uart_set_options > >> mctrl |= TIOCM_DTR > >> ->set_termios => serial8250_set_termios > >> serial8250_set_mctrl > > > > Then maybe the bug is that uart_set_options calls serial8250_set_mctrl > > which is supposed to be only called after the device is probed? > > Nope; DTR should be asserted when the console is initialized. Oh, that's news to me. I thought console communication is supposed to never use handshaking. Who can give an authorative answer here? Greg? Russell? > I understand that is not possible with the mctrl helpers right now, > but that's no reason to break other setups that do the right thing. > > >>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios > >>> > >>> This would work, but sounds wrong for the above reason, too. I'd like to > >>> reserve gpios=NULL for the case where no gpio has to be controlled, so > >>> please don't use it as indication if mctrl_gpio_init was called. > >> > >> I'm confused; what operations will be different if gpios==NULL? > >> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), > >> performing no action in that case? > > > > OK, the right thing would happen. Still I'd prefer if a serial driver > > did not try to interpret what a certain value means or not. I'd say the > > only allowed operations on a gpios value are calling mctrl_gpio > > functions and use IS_ERR and PTR_ERR during probe. > > Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() > instead, right? > > Because that's "the case where no gpio has to be controlled" because > there is no gpio yet. I don't agree. It's a layer violation if you pass a "self made" value (in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL. Best regards Uwe
On 04/06/2016 12:14 PM, Uwe Kleine-König wrote: > Hello, > > On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote: >> On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: >>> On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: >>>> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: >>>>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: >>>>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: >>>>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König >>>>>>> <u.kleine-koenig@pengutronix.de> wrote: >>>>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>>>>>>>> I've got a kernel crash from kernel robot. If we use UART before >>>>>>>>> general initialization (earlyprintk), then any call to mctrl API would >>>>>>>>> result in NULL pointer dereference. One solution would be to check, if >>>>>>>>> gpios IS_ERR_OR_NULL(). See below: >>>>>>>>> >>>>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>>>>>>>> unsigned int mctrl) >>>>>>>>> int value_array[UART_GPIO_MAX]; >>>>>>>>> unsigned int count = 0; >>>>>>>>> >>>>>>>>> + if (IS_ERR_OR_NULL(gpios)) >>>>>>>>> + return; >>>>>>>>> + >>>>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) >>>>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>>>>>>>> desc_array[count] = gpios->gpio[i]; >>>>>>>> >>>>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >>>>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it >>>>>>>> returns an error you're supposed to not register the port. And for early >>>>>>>> printk there is AFAIK no mctrl involved. >>>>>>> >>>>>>> You're right. it was console_init stuff. It happens before >>>>>>> serial8250_register_8250_port(). Perhaps I should introduce one more >>>>>>> gpio_init invocation in univ8250_console_setup(). >>>>> >>>>> If the port isn't registered yet, nobody should call the port's >>>>> .set_mctrl. So your plan sounds wrong for this reason, too. >>>> >>>> The 8250 driver initializes MCR from mctrl in its set_termios method: >>>> uart_set_options >>>> mctrl |= TIOCM_DTR >>>> ->set_termios => serial8250_set_termios >>>> serial8250_set_mctrl >>> >>> Then maybe the bug is that uart_set_options calls serial8250_set_mctrl >>> which is supposed to be only called after the device is probed? >> >> Nope; DTR should be asserted when the console is initialized. > > Oh, that's news to me. I thought console communication is supposed to > never use handshaking. Who can give an authorative answer here? Greg? > Russell? Since v2.6.23: commit 79492689e40d4f4d3d8a7262781d56fb295b4b86 Author: Yinghai Lu <Yinghai.Lu@Sun.COM> Date: Sun Jul 15 23:37:25 2007 -0700 serial: assert DTR for serial console devices Some RS-232 devices require DTR to be asserted before they can be used. DTR is normally asserted in uart_startup() when the port is opened. But we don't actually open serial console ports, so assert DTR when the port is added. BTW: earlyprintk and early_uart are hard coded to set DTR/RTS. rmk says The only issue I can think of is the possibility for an attached modem to auto-answer or maybe even auto-dial before the system is ready for it to do so. Might have an undesirable cost implication for some running with such a setup. Apart from that, I can't think of any other side effect of this specific patch. Signed-off-by: Yinghai Lu <yinghai.lu@sun.com> Acked-by: Russell King <rmk@arm.linux.org.uk> >> I understand that is not possible with the mctrl helpers right now, >> but that's no reason to break other setups that do the right thing. >> >>>>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios >>>>> >>>>> This would work, but sounds wrong for the above reason, too. I'd like to >>>>> reserve gpios=NULL for the case where no gpio has to be controlled, so >>>>> please don't use it as indication if mctrl_gpio_init was called. >>>> >>>> I'm confused; what operations will be different if gpios==NULL? >>>> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), >>>> performing no action in that case? >>> >>> OK, the right thing would happen. Still I'd prefer if a serial driver >>> did not try to interpret what a certain value means or not. I'd say the >>> only allowed operations on a gpios value are calling mctrl_gpio >>> functions and use IS_ERR and PTR_ERR during probe. >> >> Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() >> instead, right? >> >> Because that's "the case where no gpio has to be controlled" because >> there is no gpio yet. > > I don't agree. It's a layer violation if you pass a "self made" value > (in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init > cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL. Which is exactly the opposite argument you just waged 2 emails before. Ok, so if "mctrl_gpio_init() cannot return NULL", and gpios is a self-made value, then I see no problem simply not calling mctrl_gpio_set() if up->gpios is NULL. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Apr 06, 2016 at 12:27:29PM -0700, Peter Hurley wrote: > On 04/06/2016 12:14 PM, Uwe Kleine-König wrote: > > On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote: > >> On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: > >>> On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: > >>>> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: > >>>>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: > >>>>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: > >>>>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König > >>>>>>> <u.kleine-koenig@pengutronix.de> wrote: > >>>>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: > >>>>>>>>> I've got a kernel crash from kernel robot. If we use UART before > >>>>>>>>> general initialization (earlyprintk), then any call to mctrl API would > >>>>>>>>> result in NULL pointer dereference. One solution would be to check, if > >>>>>>>>> gpios IS_ERR_OR_NULL(). See below: > >>>>>>>>> > >>>>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c > >>>>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c > >>>>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, > >>>>>>>>> unsigned int mctrl) > >>>>>>>>> int value_array[UART_GPIO_MAX]; > >>>>>>>>> unsigned int count = 0; > >>>>>>>>> > >>>>>>>>> + if (IS_ERR_OR_NULL(gpios)) > >>>>>>>>> + return; > >>>>>>>>> + > >>>>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) > >>>>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { > >>>>>>>>> desc_array[count] = gpios->gpio[i]; > >>>>>>>> > >>>>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value > >>>>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it > >>>>>>>> returns an error you're supposed to not register the port. And for early > >>>>>>>> printk there is AFAIK no mctrl involved. > >>>>>>> > >>>>>>> You're right. it was console_init stuff. It happens before > >>>>>>> serial8250_register_8250_port(). Perhaps I should introduce one more > >>>>>>> gpio_init invocation in univ8250_console_setup(). > >>>>> > >>>>> If the port isn't registered yet, nobody should call the port's > >>>>> .set_mctrl. So your plan sounds wrong for this reason, too. > >>>> > >>>> The 8250 driver initializes MCR from mctrl in its set_termios method: > >>>> uart_set_options > >>>> mctrl |= TIOCM_DTR > >>>> ->set_termios => serial8250_set_termios > >>>> serial8250_set_mctrl > >>> > >>> Then maybe the bug is that uart_set_options calls serial8250_set_mctrl > >>> which is supposed to be only called after the device is probed? > >> > >> Nope; DTR should be asserted when the console is initialized. > > > > Oh, that's news to me. I thought console communication is supposed to > > never use handshaking. Who can give an authorative answer here? Greg? > > Russell? > > Since v2.6.23: > > commit 79492689e40d4f4d3d8a7262781d56fb295b4b86 > Author: Yinghai Lu <Yinghai.Lu@Sun.COM> > Date: Sun Jul 15 23:37:25 2007 -0700 > > serial: assert DTR for serial console devices > > Some RS-232 devices require DTR to be asserted before they can be used. DTR > is normally asserted in uart_startup() when the port is opened. But we don't > actually open serial console ports, so assert DTR when the port is added. > > BTW: > earlyprintk and early_uart are hard coded to set DTR/RTS. > > rmk says > > The only issue I can think of is the possibility for an attached modem to > auto-answer or maybe even auto-dial before the system is ready for it to do > so. Might have an undesirable cost implication for some running with such a > setup. > > Apart from that, I can't think of any other side effect of this specific > patch. > > Signed-off-by: Yinghai Lu <yinghai.lu@sun.com> > Acked-by: Russell King <rmk@arm.linux.org.uk> > > > >> I understand that is not possible with the mctrl helpers right now, > >> but that's no reason to break other setups that do the right thing. > >> > >>>>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios > >>>>> > >>>>> This would work, but sounds wrong for the above reason, too. I'd like to > >>>>> reserve gpios=NULL for the case where no gpio has to be controlled, so > >>>>> please don't use it as indication if mctrl_gpio_init was called. > >>>> > >>>> I'm confused; what operations will be different if gpios==NULL? > >>>> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), > >>>> performing no action in that case? > >>> > >>> OK, the right thing would happen. Still I'd prefer if a serial driver > >>> did not try to interpret what a certain value means or not. I'd say the > >>> only allowed operations on a gpios value are calling mctrl_gpio > >>> functions and use IS_ERR and PTR_ERR during probe. > >> > >> Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() > >> instead, right? > >> > >> Because that's "the case where no gpio has to be controlled" because > >> there is no gpio yet. > > > > I don't agree. It's a layer violation if you pass a "self made" value > > (in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init > > cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL. > > Which is exactly the opposite argument you just waged 2 emails before. > > Ok, so if "mctrl_gpio_init() cannot return NULL", and gpios is a > self-made value, then I see no problem simply not calling > mctrl_gpio_set() if up->gpios is NULL. No, the rule is: Only call mctrl_gpio_* with a value returned by mctrl_gpio_init. Currently this implies you're doing something wrong when you pass NULL. When one day NULL might be returned by mctrl_gpio_init (likely meaning "there are no gpios to be controlled") you can of course pass NULL. But only if you called mctrl_gpio_init before and it gave you NULL. But this is all internal knowledge that shouldn't be used in a serial driver. And you shouldn't make it possible to let mctrl_gpio_init return NULL just to bless your current usage. The hard rule is: Only call mctrl_gpio_* after mctrl_gpio_init and pass the value returned by it. Best regards Uwe
Hello, On 04/06/2016 12:38 PM, Uwe Kleine-König wrote: > On Wed, Apr 06, 2016 at 12:27:29PM -0700, Peter Hurley wrote: >> On 04/06/2016 12:14 PM, Uwe Kleine-König wrote: >>> On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote: >>>> On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: >>>>> On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: >>>>>> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: >>>>>>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: >>>>>>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: >>>>>>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König >>>>>>>>> <u.kleine-koenig@pengutronix.de> wrote: >>>>>>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>>>>>>>>>> I've got a kernel crash from kernel robot. If we use UART before >>>>>>>>>>> general initialization (earlyprintk), then any call to mctrl API would >>>>>>>>>>> result in NULL pointer dereference. One solution would be to check, if >>>>>>>>>>> gpios IS_ERR_OR_NULL(). See below: >>>>>>>>>>> >>>>>>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>>>>>>>>>> unsigned int mctrl) >>>>>>>>>>> int value_array[UART_GPIO_MAX]; >>>>>>>>>>> unsigned int count = 0; >>>>>>>>>>> >>>>>>>>>>> + if (IS_ERR_OR_NULL(gpios)) >>>>>>>>>>> + return; >>>>>>>>>>> + >>>>>>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) >>>>>>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>>>>>>>>>> desc_array[count] = gpios->gpio[i]; >>>>>>>>>> >>>>>>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >>>>>>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it >>>>>>>>>> returns an error you're supposed to not register the port. And for early >>>>>>>>>> printk there is AFAIK no mctrl involved. >>>>>>>>> >>>>>>>>> You're right. it was console_init stuff. It happens before >>>>>>>>> serial8250_register_8250_port(). Perhaps I should introduce one more >>>>>>>>> gpio_init invocation in univ8250_console_setup(). >>>>>>> >>>>>>> If the port isn't registered yet, nobody should call the port's >>>>>>> .set_mctrl. So your plan sounds wrong for this reason, too. >>>>>> >>>>>> The 8250 driver initializes MCR from mctrl in its set_termios method: >>>>>> uart_set_options >>>>>> mctrl |= TIOCM_DTR >>>>>> ->set_termios => serial8250_set_termios >>>>>> serial8250_set_mctrl >>>>> >>>>> Then maybe the bug is that uart_set_options calls serial8250_set_mctrl >>>>> which is supposed to be only called after the device is probed? >>>> >>>> Nope; DTR should be asserted when the console is initialized. >>> >>> Oh, that's news to me. I thought console communication is supposed to >>> never use handshaking. Who can give an authorative answer here? Greg? >>> Russell? >> >> Since v2.6.23: >> >> commit 79492689e40d4f4d3d8a7262781d56fb295b4b86 >> Author: Yinghai Lu <Yinghai.Lu@Sun.COM> >> Date: Sun Jul 15 23:37:25 2007 -0700 >> >> serial: assert DTR for serial console devices >> >> Some RS-232 devices require DTR to be asserted before they can be used. DTR >> is normally asserted in uart_startup() when the port is opened. But we don't >> actually open serial console ports, so assert DTR when the port is added. >> >> BTW: >> earlyprintk and early_uart are hard coded to set DTR/RTS. >> >> rmk says >> >> The only issue I can think of is the possibility for an attached modem to >> auto-answer or maybe even auto-dial before the system is ready for it to do >> so. Might have an undesirable cost implication for some running with such a >> setup. >> >> Apart from that, I can't think of any other side effect of this specific >> patch. >> >> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com> >> Acked-by: Russell King <rmk@arm.linux.org.uk> >> >> >>>> I understand that is not possible with the mctrl helpers right now, >>>> but that's no reason to break other setups that do the right thing. >>>> >>>>>>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios >>>>>>> >>>>>>> This would work, but sounds wrong for the above reason, too. I'd like to >>>>>>> reserve gpios=NULL for the case where no gpio has to be controlled, so >>>>>>> please don't use it as indication if mctrl_gpio_init was called. >>>>>> >>>>>> I'm confused; what operations will be different if gpios==NULL? >>>>>> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), >>>>>> performing no action in that case? >>>>> >>>>> OK, the right thing would happen. Still I'd prefer if a serial driver >>>>> did not try to interpret what a certain value means or not. I'd say the >>>>> only allowed operations on a gpios value are calling mctrl_gpio >>>>> functions and use IS_ERR and PTR_ERR during probe. >>>> >>>> Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() >>>> instead, right? >>>> >>>> Because that's "the case where no gpio has to be controlled" because >>>> there is no gpio yet. >>> >>> I don't agree. It's a layer violation if you pass a "self made" value >>> (in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init >>> cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL. >> >> Which is exactly the opposite argument you just waged 2 emails before. >> >> Ok, so if "mctrl_gpio_init() cannot return NULL", and gpios is a >> self-made value, then I see no problem simply not calling >> mctrl_gpio_set() if up->gpios is NULL. > > No, the rule is: Only call mctrl_gpio_* with a value returned by > mctrl_gpio_init. Currently this implies you're doing something wrong > when you pass NULL. When one day NULL might be returned by > mctrl_gpio_init (likely meaning "there are no gpios to be controlled") > you can of course pass NULL. But only if you called mctrl_gpio_init > before and it gave you NULL. But this is all internal knowledge that > shouldn't be used in a serial driver. And you shouldn't make it possible > to let mctrl_gpio_init return NULL just to bless your current usage. The > hard rule is: Only call mctrl_gpio_* after mctrl_gpio_init and pass the > value returned by it. I'm not adding a bunch of driver state for an inadequately designed interface, sorry. The initial state before mctrl_gpio_init() is ever called is already NULL, so there will be no way to distinguish between whether mctrl_gpio_init() has ever been called or not. The mctrl helpers will not be able to assert their own invariances, which imho, is broken. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 6, 2016 at 9:48 PM, Peter Hurley <peter@hurleysoftware.com> wrote: > Hello, > > On 04/06/2016 12:38 PM, Uwe Kleine-König wrote: >> On Wed, Apr 06, 2016 at 12:27:29PM -0700, Peter Hurley wrote: >>> On 04/06/2016 12:14 PM, Uwe Kleine-König wrote: >>>> On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote: >>>>> On 04/06/2016 10:48 AM, Uwe Kleine-König wrote: >>>>>> On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote: >>>>>>> On 04/05/2016 11:20 PM, Uwe Kleine-König wrote: >>>>>>>> On Tue, Apr 05, 2016 at 09:58:09AM -0700, Peter Hurley wrote: >>>>>>>>> On 04/05/2016 06:25 AM, Yegor Yefremov wrote: >>>>>>>>>> On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König >>>>>>>>>> <u.kleine-koenig@pengutronix.de> wrote: >>>>>>>>>>> On Tue, Apr 05, 2016 at 12:32:53PM +0200, Yegor Yefremov wrote: >>>>>>>>>>>> I've got a kernel crash from kernel robot. If we use UART before >>>>>>>>>>>> general initialization (earlyprintk), then any call to mctrl API would >>>>>>>>>>>> result in NULL pointer dereference. One solution would be to check, if >>>>>>>>>>>> gpios IS_ERR_OR_NULL(). See below: >>>>>>>>>>>> >>>>>>>>>>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>>>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>>>>>>>>>> @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, >>>>>>>>>>>> unsigned int mctrl) >>>>>>>>>>>> int value_array[UART_GPIO_MAX]; >>>>>>>>>>>> unsigned int count = 0; >>>>>>>>>>>> >>>>>>>>>>>> + if (IS_ERR_OR_NULL(gpios)) >>>>>>>>>>>> + return; >>>>>>>>>>>> + >>>>>>>>>>>> for (i = 0; i < UART_GPIO_MAX; i++) >>>>>>>>>>>> if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) { >>>>>>>>>>>> desc_array[count] = gpios->gpio[i]; >>>>>>>>>>> >>>>>>>>>>> IS_ERR_OR_NULL(gpios) should never be true. gpios should be the value >>>>>>>>>>> that was returned by mctrl_gpio_init, this never returns NULL and if it >>>>>>>>>>> returns an error you're supposed to not register the port. And for early >>>>>>>>>>> printk there is AFAIK no mctrl involved. >>>>>>>>>> >>>>>>>>>> You're right. it was console_init stuff. It happens before >>>>>>>>>> serial8250_register_8250_port(). Perhaps I should introduce one more >>>>>>>>>> gpio_init invocation in univ8250_console_setup(). >>>>>>>> >>>>>>>> If the port isn't registered yet, nobody should call the port's >>>>>>>> .set_mctrl. So your plan sounds wrong for this reason, too. >>>>>>> >>>>>>> The 8250 driver initializes MCR from mctrl in its set_termios method: >>>>>>> uart_set_options >>>>>>> mctrl |= TIOCM_DTR >>>>>>> ->set_termios => serial8250_set_termios >>>>>>> serial8250_set_mctrl >>>>>> >>>>>> Then maybe the bug is that uart_set_options calls serial8250_set_mctrl >>>>>> which is supposed to be only called after the device is probed? >>>>> >>>>> Nope; DTR should be asserted when the console is initialized. >>>> >>>> Oh, that's news to me. I thought console communication is supposed to >>>> never use handshaking. Who can give an authorative answer here? Greg? >>>> Russell? >>> >>> Since v2.6.23: >>> >>> commit 79492689e40d4f4d3d8a7262781d56fb295b4b86 >>> Author: Yinghai Lu <Yinghai.Lu@Sun.COM> >>> Date: Sun Jul 15 23:37:25 2007 -0700 >>> >>> serial: assert DTR for serial console devices >>> >>> Some RS-232 devices require DTR to be asserted before they can be used. DTR >>> is normally asserted in uart_startup() when the port is opened. But we don't >>> actually open serial console ports, so assert DTR when the port is added. >>> >>> BTW: >>> earlyprintk and early_uart are hard coded to set DTR/RTS. >>> >>> rmk says >>> >>> The only issue I can think of is the possibility for an attached modem to >>> auto-answer or maybe even auto-dial before the system is ready for it to do >>> so. Might have an undesirable cost implication for some running with such a >>> setup. >>> >>> Apart from that, I can't think of any other side effect of this specific >>> patch. >>> >>> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com> >>> Acked-by: Russell King <rmk@arm.linux.org.uk> >>> >>> >>>>> I understand that is not possible with the mctrl helpers right now, >>>>> but that's no reason to break other setups that do the right thing. >>>>> >>>>>>>>> Just skip mctrl_gpio_set() and mctrl_gpio_get*() if !up->gpios >>>>>>>> >>>>>>>> This would work, but sounds wrong for the above reason, too. I'd like to >>>>>>>> reserve gpios=NULL for the case where no gpio has to be controlled, so >>>>>>>> please don't use it as indication if mctrl_gpio_init was called. >>>>>>> >>>>>>> I'm confused; what operations will be different if gpios==NULL? >>>>>>> And wouldn't that argue for checking gpios==NULL in mctrl_gpio_set(), >>>>>>> performing no action in that case? >>>>>> >>>>>> OK, the right thing would happen. Still I'd prefer if a serial driver >>>>>> did not try to interpret what a certain value means or not. I'd say the >>>>>> only allowed operations on a gpios value are calling mctrl_gpio >>>>>> functions and use IS_ERR and PTR_ERR during probe. >>>>> >>>>> Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set() >>>>> instead, right? >>>>> >>>>> Because that's "the case where no gpio has to be controlled" because >>>>> there is no gpio yet. >>>> >>>> I don't agree. It's a layer violation if you pass a "self made" value >>>> (in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init >>>> cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL. >>> >>> Which is exactly the opposite argument you just waged 2 emails before. >>> >>> Ok, so if "mctrl_gpio_init() cannot return NULL", and gpios is a >>> self-made value, then I see no problem simply not calling >>> mctrl_gpio_set() if up->gpios is NULL. >> >> No, the rule is: Only call mctrl_gpio_* with a value returned by >> mctrl_gpio_init. Currently this implies you're doing something wrong >> when you pass NULL. When one day NULL might be returned by >> mctrl_gpio_init (likely meaning "there are no gpios to be controlled") >> you can of course pass NULL. But only if you called mctrl_gpio_init >> before and it gave you NULL. But this is all internal knowledge that >> shouldn't be used in a serial driver. And you shouldn't make it possible >> to let mctrl_gpio_init return NULL just to bless your current usage. The >> hard rule is: Only call mctrl_gpio_* after mctrl_gpio_init and pass the >> value returned by it. > > I'm not adding a bunch of driver state for an inadequately designed > interface, sorry. > > The initial state before mctrl_gpio_init() is ever called is already NULL, > so there will be no way to distinguish between whether mctrl_gpio_init() > has ever been called or not. > > The mctrl helpers will not be able to assert their own invariances, which > imho, is broken. We have basically two issues, where up->gpios is NULL could be very useful. 1. the problem with console, i.e. when mctrl API is used before mctrl_gpio_init() 2. systems configured without GPIOLIB. In this case mctrl_gpio_init() returns ERR_PTR(-ENOSYS) static inline struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx) { return ERR_PTR(-ENOSYS); } And this is deadly for a console device. I'd suggest, that mctrl_gpio_init() returns either NULL or real pointer, if everything could be initialized properly. mctrl_gpio_init() should report all errors via dev_err. This way, if mctrl API is not working, one can at least find the cause in dmesg. Then all directly available mctrl routines would check for up->gpios is NULL and exit, if gpios is not available. Yegor -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, Apr 08, 2016 at 08:07:51AM +0200, Yegor Yefremov wrote: > We have basically two issues, where up->gpios is NULL could be very useful. > > 1. the problem with console, i.e. when mctrl API is used before > mctrl_gpio_init() Either mctrl_gpio should be able to control the handshaking lines, or it's not important and can be skipped. In the first case you have to ensure that mctrl_gpio_init is called before. In the second, don't even try and don't use mctrl_gpio. > 2. systems configured without GPIOLIB. In this case mctrl_gpio_init() > returns ERR_PTR(-ENOSYS) > > static inline > struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx) > { > return ERR_PTR(-ENOSYS); > } > > And this is deadly for a console device. And consequently so. If it's important that the handshaking lines are working in console mode and you don't know if they do (because GPIOLIB is off), what do you suggest? Maybe select GPIOLIB (but that won't work for all platforms I think)? > I'd suggest, that mctrl_gpio_init() returns either NULL or real > pointer, if everything could be initialized properly. It does always return a real pointer then. (If GPIOLIB is off, mctrl_gpio_init cannot be sure that everything was properly initialized, so it returns an error code. The same holds true for the gpiod functions btw.) > mctrl_gpio_init() should report all errors via dev_err. This way, if > mctrl API is not working, one can at least find the cause in dmesg. I'm open for patches that implement this. Best regards Uwe
--- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl) int value_array[UART_GPIO_MAX]; unsigned int count = 0; + if (IS_ERR_OR_NULL(gpios)) + return; + for (i = 0; i < UART_GPIO_MAX; i++) if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {