Hello,
On Wed, Apr 06, 2016 at 08:23:02AM -0700, Peter Hurley wrote:
> 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:
MCR is a register and mctrl a member of struct port, right?
> uart_set_options
> mctrl |= TIOCM_DTR
> ->set_termios => serial8250_set_termios
> serial8250_set_mctrl
Then maybe the bug is that uart_set_options calls serial8250_set_mctrl
which is supposed to be only called after the device is probed?
> >> 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?
OK, the right thing would happen. Still I'd prefer if a serial driver
did not try to interpret what a certain value means or not. I'd say the
only allowed operations on a gpios value are calling mctrl_gpio
functions and use IS_ERR and PTR_ERR during probe.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
