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

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

 



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.

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

> > +
> > +       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.

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