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 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@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().

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@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