On Wed, Jul 10, 2013 at 02:36:33PM +0600, Alexey Galakhov wrote:
> 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.
Which makes the hardware state consistent with the logical state inside
the data structures. This can't be too wrong.
Anyway, maybe Jean-Christophe is right and we should do this in
console_std_set by adding:
if(flag && cdev->setbrg)
cdev->setbrg(cdev, cdev->baudrate);
This makes sure the hardware is only ever touched when the console is
actually used.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
