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