On Wed, Apr 6, 2016 at 9:48 PM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
> Hello,
>
> On 04/06/2016 12:38 PM, Uwe Kleine-König wrote:
>> 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.
>
> I'm not adding a bunch of driver state for an inadequately designed
> interface, sorry.
>
> The initial state before mctrl_gpio_init() is ever called is already NULL,
> so there will be no way to distinguish between whether mctrl_gpio_init()
> has ever been called or not.
>
> The mctrl helpers will not be able to assert their own invariances, which
> imho, is broken.
We have basically two issues, where up->gpios is NULL could be very useful.
1. the problem with console, i.e. when mctrl API is used before
mctrl_gpio_init()
2. systems configured without GPIOLIB. In this case mctrl_gpio_init()
returns ERR_PTR(-ENOSYS)
static inline
struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx)
{
return ERR_PTR(-ENOSYS);
}
And this is deadly for a console device. I'd suggest, that
mctrl_gpio_init() returns either NULL or real pointer, if everything
could be initialized properly. mctrl_gpio_init() should report all
errors via dev_err. This way, if mctrl API is not working, one can at
least find the cause in dmesg.
Then all directly available mctrl routines would check for up->gpios
is NULL and exit, if gpios is not available.
Yegor
--
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
