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,

On Wed, Apr 06, 2016 at 11:35:23AM -0700, Peter Hurley wrote:
> On 04/06/2016 10:48 AM, Uwe Kleine-König wrote:
> > 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:
> >>   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?
> 
> Nope; DTR should be asserted when the console is initialized.

Oh, that's news to me. I thought console communication is supposed to
never use handshaking. Who can give an authorative answer here? Greg?
Russell?

> I understand that is not possible with the mctrl helpers right now,
> but that's no reason to break other setups that do the right thing.
> 
> >>>> 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.
> 
> Ok, so then we're back to checking gpios == NULL in mctrl_gpio_set()
> instead, right?
> 
> Because that's "the case where no gpio has to be controlled" because
> there is no gpio yet.

I don't agree. It's a layer violation if you pass a "self made" value
(in this case NULL) to an mctrl_gpio function. Currently mctrl_gpio_init
cannot return NULL, so it's a bug to call mctrl_gpio_set with NULL.

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