On Tue, Apr 5, 2016 at 2:21 PM, Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> 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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
