Re: [PATCH] Force set console baudrate

[Alexey Galakhov <agalakhov@gmail.com>]

On 07/10/2013 02:07 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:

> This does force the same baud rate on ALL console device at register time => wrong

I think it is correct, and I'll explain why.

If the baudrate for the device is set explicitly via "baudrate"
parameter, it WILL override any register time setting, so the register
time setting does not matter at all.

If the baudrate for the device is not set, CONFIG_BAUDRATE is
applicable. If the "baudrate" property is read, it even returns
CONFIG_BAUDRATE value. The device is expected to work at CONFIG_BAUDRATE
if "baudrate" is not set.

Calling setbrg() at registration time just ensures that the "baudrate"
formal parameter matches actual device settings.

>> There is "chicken and egg" problem. barebox_banner() requires valid
>> baudrate setting and may deadlock without it. (It WILL deadlock on most
>> serial drivers and really deadlocks on S3C one).
> 
> so do is at enable time not register time

Maybe, but why then we have if (cdev->f_active) ... else
cdev->setbrg(...) in console_baudrate_set()? Doesn't this mean that
setbrg() is meant to be called BEFORE enable time?

>> If the device does not have setbrg() function defined, nothing will be
>> called. This all is under if (newcdev->setbrg) anyway.
> 
> this will result is a call of a NULL pointer -> crash

No, it won't of course! The pointer IS checked before setbrg() call by
an existing if(newcdev->setbrg), so no NULL-pointer reference will be
called.

Please look at the code one more time:

// If the device being registered has setbrg function defined...
if (newcdev->setbrg) {
        // ... then set its default baudrate...
        newcdev->baudrate = CONFIG_BAUDRATE;
        // ... add "baudrate" parameter to allow this to be changed...
        dev_add_param_int(dev, "baudrate", console_baudrate_set,
                NULL, &newcdev->baudrate, "%u", newcdev);
        // ... and MY ADDITION - actually activate this parameter.
        newcdev->setbrg(newcdev, newcdev->baudrate);
}
NULL-pointer dereference is guaranteed not to happen here.

> If the console is not enable there no need to do anything
> you need to set the baud rate ONLY if the device is enable

To be more precice: if the device that was previously disabled is
enabled. Correct. This is better than the current approach. Calling
setbrg() in the else clause of console_baudrate_set() is incorrect as
well so it should be removed (to be more precise, moved to init time).

Regards,
Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox