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 Peter, hello Yegor,

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.

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

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

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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux