Re: [PATCH v2 5/5] tty/serial/8250: use mctrl_gpio helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux