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