Re: [PATCH] Force set console baudrate

[Sascha Hauer <s.hauer@pengutronix.de>]

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox