[PATCH] Force set console baudrate

[PATCH] Force set console baudrate

[Alexey Galakhov]

Most serial drivers require setbrg() to be called at least once. Call them
just after the device has been added.

Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
---
 common/console.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/common/console.c b/common/console.c
index a0a06f6..abf5322 100644
--- a/common/console.c
+++ b/common/console.c
@@ -147,6 +147,7 @@ int console_register(struct console_device *newcdev)
 		newcdev->baudrate = CONFIG_BAUDRATE;
 		dev_add_param_int(dev, "baudrate", console_baudrate_set,
 			NULL, &newcdev->baudrate, "%u", newcdev);
+		newcdev->setbrg(newcdev, newcdev->baudrate);
 	}
 
 	dev_add_param(dev, "active", console_std_set, NULL, 0);
-- 
1.7.10.4


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

Re: [PATCH] Force set console baudrate

[Sascha Hauer]

On Tue, Jul 09, 2013 at 09:22:10PM +0600, Alexey Galakhov wrote:
> Most serial drivers require setbrg() to be called at least once. Call them
> just after the device has been added.
> 
> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>

Applied, thanks.

Sascha

> ---
>  common/console.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/common/console.c b/common/console.c
> index a0a06f6..abf5322 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -147,6 +147,7 @@ int console_register(struct console_device *newcdev)
>  		newcdev->baudrate = CONFIG_BAUDRATE;
>  		dev_add_param_int(dev, "baudrate", console_baudrate_set,
>  			NULL, &newcdev->baudrate, "%u", newcdev);
> +		newcdev->setbrg(newcdev, newcdev->baudrate);
>  	}
>  
>  	dev_add_param(dev, "active", console_std_set, NULL, 0);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
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

Re: [PATCH] Force set console baudrate

[Jean-Christophe PLAGNIOL-VILLARD]

On Jul 10, 2013, at 1:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Tue, Jul 09, 2013 at 09:22:10PM +0600, Alexey Galakhov wrote:
>> Most serial drivers require setbrg() to be called at least once. Call them
>> just after the device has been added.
>> 
>> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>

This is wrong if you do this at boot which means you force the same baudrate
on all the console

We must do this only is enable by default of via shell

and some console may not need the setbrg call back (keyboard or fbdev as example)

Also if we want to use a rs232 for controlling a mcu the baud rate may not be the
CONFIG_BAUDRATE

so force it at register is wrong

Best Regards,
J.
> 
> Applied, thanks.
> 
> Sascha
> 
>> ---
>> common/console.c |    1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/common/console.c b/common/console.c
>> index a0a06f6..abf5322 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -147,6 +147,7 @@ int console_register(struct console_device *newcdev)
>> 		newcdev->baudrate = CONFIG_BAUDRATE;
>> 		dev_add_param_int(dev, "baudrate", console_baudrate_set,
>> 			NULL, &newcdev->baudrate, "%u", newcdev);
>> +		newcdev->setbrg(newcdev, newcdev->baudrate);
>> 	}
>> 
>> 	dev_add_param(dev, "active", console_std_set, NULL, 0);
>> -- 
>> 1.7.10.4
>> 
>> 
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>> 
> 
> -- 
> 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


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

Re: [PATCH] Force set console baudrate

[Alexey Galakhov]

Hi,

On 07/10/2013 12:57 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> On Jul 10, 2013, at 1:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
>> On Tue, Jul 09, 2013 at 09:22:10PM +0600, Alexey Galakhov wrote:
>>> Most serial drivers require setbrg() to be called at least once. Call them
>>> just after the device has been added.
>>>
>>> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
> 
> This is wrong if you do this at boot which means you force the same baudrate
> on all the console

This does NOT force the same baudrate. It just ensures that the default
baudrate will not be ignored. There is no problem setting another
baudrate later if desired by using "baudrate" parameter from the shell
or something.

> We must do this only is enable by default of via shell

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).

> and some console may not need the setbrg call back (keyboard or fbdev as example)

If the device does not have setbrg() function defined, nothing will be
called. This all is under if (newcdev->setbrg) anyway.

> Also if we want to use a rs232 for controlling a mcu the baud rate may not be the
> CONFIG_BAUDRATE

No problem, there is a parameter. Just set it before activating the device.

> so force it at register is wrong

Not setting it is much worse. It will result in deadlock at the first
attempt to write to the console since setbrg() will not be called at all.

The idea is, if there IS a function to set a baudrate and there IS a
default setting, the baudrate SHOULD be initialized before doing
anything else.  Changing the baudrate on an inactive device multiple
times is (or at least should be) harmless. Trying to activate the device
with baudrate unset is not.

Regards,
Alex

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

Re: [PATCH] Force set console baudrate

[Jean-Christophe PLAGNIOL-VILLARD]

On Jul 10, 2013, at 4:09 AM, Alexey Galakhov <agalakhov@gmail.com> wrote:

> Hi,
> 
> On 07/10/2013 12:57 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> 
>> On Jul 10, 2013, at 1:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> 
>>> On Tue, Jul 09, 2013 at 09:22:10PM +0600, Alexey Galakhov wrote:
>>>> Most serial drivers require setbrg() to be called at least once. Call them
>>>> just after the device has been added.
>>>> 
>>>> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
>> 
>> This is wrong if you do this at boot which means you force the same baudrate
>> on all the console
> 
> This does NOT force the same baudrate. It just ensures that the default
> baudrate will not be ignored. There is no problem setting another
> baudrate later if desired by using "baudrate" parameter from the shell
> or something.

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

> 
>> We must do this only is enable by default of via shell
> 
> 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

> 
>> and some console may not need the setbrg call back (keyboard or fbdev as example)
> 
> 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
> 
>> Also if we want to use a rs232 for controlling a mcu the baud rate may not be the
>> CONFIG_BAUDRATE
> 
> No problem, there is a parameter. Just set it before activating the device.
> 
>> so force it at register is wrong
> 
> Not setting it is much worse. It will result in deadlock at the first
> attempt to write to the console since setbrg() will not be called at all.
> 
> The idea is, if there IS a function to set a baudrate and there IS a
> default setting, the baudrate SHOULD be initialized before doing
> anything else.  Changing the baudrate on an inactive device multiple
> times is (or at least should be) harmless. Trying to activate the device
> with baudrate unset is not.
> 

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

> Regards,
> Alex


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

Re: [PATCH] Force set console baudrate

[Alexey Galakhov]

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

Re: [PATCH] Force set console baudrate

[Sascha Hauer]

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

Re: [PATCH] Force set console baudrate

[Alexey Galakhov]

On 07/10/2013 03:01 PM, Sascha Hauer wrote:

> 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
> 

Hi,

Agree. The only reason not do to so was that I saw the existing code,
especially the else clause in console_baudrate_set().

I just made v2 of the patch. I'm testing it right now. If it's Ok, I'll
submit it.

I did almost exactly this. The difference is that I do not call setbrg()
again if the device is already active. I also removed setbrg() call for
inactive devices from console_baudrate_set() as this would be duplicate
call now.

Regards,
Alex

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

[PATCH 1/2] Revert "Force set console baudrate"

[Alexey Galakhov]

This reverts commit 564414b6db90fabb2c1f529454f153badd75d5d5.
---
 common/console.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/common/console.c b/common/console.c
index abf5322..a0a06f6 100644
--- a/common/console.c
+++ b/common/console.c
@@ -147,7 +147,6 @@ int console_register(struct console_device *newcdev)
 		newcdev->baudrate = CONFIG_BAUDRATE;
 		dev_add_param_int(dev, "baudrate", console_baudrate_set,
 			NULL, &newcdev->baudrate, "%u", newcdev);
-		newcdev->setbrg(newcdev, newcdev->baudrate);
 	}
 
 	dev_add_param(dev, "active", console_std_set, NULL, 0);
-- 
1.7.10.4


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

[PATCH 2/2] Force set console baudrate at enable time

[Alexey Galakhov]

Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
---
 common/console.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/console.c b/common/console.c
index a0a06f6..28941d6 100644
--- a/common/console.c
+++ b/common/console.c
@@ -80,6 +80,12 @@ static int console_std_set(struct device_d *dev, struct param_d *param,
 		flag |= CONSOLE_STDERR;
 	}
 
+	if (flag && !cdev->f_active) {
+		/* The device is being activated, set its baudrate */
+		if (cdev->setbrg)
+			cdev->setbrg(cdev, cdev->baudrate);
+	}
+
 	active[i] = 0;
 	cdev->f_active = flag;
 
@@ -104,6 +110,8 @@ static int console_baudrate_set(struct param_d *param, void *priv)
 	struct console_device *cdev = priv;
 	unsigned char c;
 
+	/* If the device is already active, change its baudrate.
+	The baudrate of an inactive device will be set at activation time. */
 	if (cdev->f_active) {
 		printf("## Switch baudrate to %d bps and press ENTER ...\n",
 			cdev->baudrate);
@@ -113,8 +121,7 @@ static int console_baudrate_set(struct param_d *param, void *priv)
 		do {
 			c = getc();
 		} while (c != '\r' && c != '\n');
-	} else
-		cdev->setbrg(cdev, cdev->baudrate);
+	}
 
 	return 0;
 }
-- 
1.7.10.4


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

Re: [PATCH 2/2] Force set console baudrate at enable time

[Sascha Hauer]

On Wed, Jul 10, 2013 at 04:15:10PM +0600, Alexey Galakhov wrote:
> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
> ---
>  common/console.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index a0a06f6..28941d6 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -80,6 +80,12 @@ static int console_std_set(struct device_d *dev, struct param_d *param,
>  		flag |= CONSOLE_STDERR;
>  	}
>  
> +	if (flag && !cdev->f_active) {
> +		/* The device is being activated, set its baudrate */
> +		if (cdev->setbrg)
> +			cdev->setbrg(cdev, cdev->baudrate);
> +	}
> +
>  	active[i] = 0;
>  	cdev->f_active = flag;
>  
> @@ -104,6 +110,8 @@ static int console_baudrate_set(struct param_d *param, void *priv)
>  	struct console_device *cdev = priv;
>  	unsigned char c;
>  
> +	/* If the device is already active, change its baudrate.
> +	The baudrate of an inactive device will be set at activation time. */

	/*
	 * multiline comments
	 * like this please
	 */

Fixed this while applying.

Thanks
 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

Re: [PATCH 2/2] Force set console baudrate at enable time

[Alexey Galakhov]

> 	/*
> 	 * multiline comments
> 	 * like this please
> 	 */
> 
> Fixed this while applying.

Thanks. Used the coding style from other project, sorry.

Alex

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