Hello,
On Wed, Apr 06, 2016 at 12:27:29PM -0700, Peter Hurley wrote:
> On 04/06/2016 12:14 PM, Uwe Kleine-König wrote:
> > 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?
>
> Since v2.6.23:
>
> commit 79492689e40d4f4d3d8a7262781d56fb295b4b86
> Author: Yinghai Lu <Yinghai.Lu@xxxxxxx>
> Date: Sun Jul 15 23:37:25 2007 -0700
>
> serial: assert DTR for serial console devices
>
> Some RS-232 devices require DTR to be asserted before they can be used. DTR
> is normally asserted in uart_startup() when the port is opened. But we don't
> actually open serial console ports, so assert DTR when the port is added.
>
> BTW:
> earlyprintk and early_uart are hard coded to set DTR/RTS.
>
> rmk says
>
> The only issue I can think of is the possibility for an attached modem to
> auto-answer or maybe even auto-dial before the system is ready for it to do
> so. Might have an undesirable cost implication for some running with such a
> setup.
>
> Apart from that, I can't think of any other side effect of this specific
> patch.
>
> Signed-off-by: Yinghai Lu <yinghai.lu@xxxxxxx>
> Acked-by: Russell King <rmk@xxxxxxxxxxxxxxxx>
>
>
> >> 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.
>
> Which is exactly the opposite argument you just waged 2 emails before.
>
> Ok, so if "mctrl_gpio_init() cannot return NULL", and gpios is a
> self-made value, then I see no problem simply not calling
> mctrl_gpio_set() if up->gpios is NULL.
No, the rule is: Only call mctrl_gpio_* with a value returned by
mctrl_gpio_init. Currently this implies you're doing something wrong
when you pass NULL. When one day NULL might be returned by
mctrl_gpio_init (likely meaning "there are no gpios to be controlled")
you can of course pass NULL. But only if you called mctrl_gpio_init
before and it gave you NULL. But this is all internal knowledge that
shouldn't be used in a serial driver. And you shouldn't make it possible
to let mctrl_gpio_init return NULL just to bless your current usage. The
hard rule is: Only call mctrl_gpio_* after mctrl_gpio_init and pass the
value returned by 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
