[PATCH 0/2] readline_simple: return -1 if getc fails

[PATCH 0/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Dear maintainers,

I came accross this issue when I was trying to add support for a new MIPS
board.

The getc() function from console_simple.c returns -EINVAL when no console is
registered.

It causes readline_simple to print a non-printable character (-EINVAL in this
case). Furthermore, it causes an infinite loop: readline never returns.

	barebox:/ ���(...)���

With this patch, the error is no more explicit but the readline function stops
to print a dummy character and returns to shell.

	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	(...)
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>

The second patch is a simple cleanup of the in-file documentation.

Changes since v1:
 - change c variable type from char in int
 - set *p to NUL before returning -1 when getchar returns an error
 - update function documentation

Regards,
Gaël PORTAY (2):
  readline_simple: return -1 if getc fails
  readline_simple: remove obsolete documentation

 lib/readline_simple.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.13.2


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

[PATCH v2 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

The getc function may return an errno code if an error happens.

This patch prevents readline from printing a non printable character and
from looping to infinity and beyong.

Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
---
 lib/readline_simple.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index c4d3d240e..7dd18e426 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -47,7 +47,7 @@ int readline (const char *prompt, char *line, int len)
 	int	n = 0;				/* buffer index		*/
 	int	plen = 0;			/* prompt length	*/
 	int	col;				/* output column cnt	*/
-	char	c;
+	int	c;
 
 	/* print prompt */
 	if (prompt) {
@@ -58,6 +58,10 @@ int readline (const char *prompt, char *line, int len)
 
 	for (;;) {
 		c = getchar();
+		if (c < 0) {
+			*p = '\0';
+			return (-1);
+		}
 
 		/*
 		 * Special character handling
-- 
2.13.2


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

[PATCH v2 2/2] readline_simple: remove obsolete documentation

[Gaël PORTAY]

The readline function does not return timed-out anymore since the
commit:

	be5d13bcb readline_simple: drop unusable u-boot legacy stuff

Signed-off-by: Gaël PORTAY <gael.portay@savoirfairelinux.com>
---
 lib/readline_simple.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index 7dd18e426..9ec00e7d8 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -35,11 +35,8 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen)
 
 /*
  * Prompt for input and read a line.
- * If  CONFIG_BOOT_RETRY_TIME is defined and retry_time >= 0,
- * time out when time goes past endtime (timebase time in ticks).
  * Return:	number of read characters
- *		-1 if break
- *		-2 if timed out
+ *		-1 if error or break
  */
 int readline (const char *prompt, char *line, int len)
 {
-- 
2.13.2


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

Re: [PATCH 0/2] readline_simple: return -1 if getc fails

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 276 bytes --]

Am 09.08.2017 um 06:13 schrieb Gaël PORTAY:
> Dear maintainers,
> 
> I came accross this issue when I was trying to add support for a new MIPS
> board.

Just out of curiosity, what MIPS board are you working on? If it is not
a secret :)


-- 
Regards,
Oleksij


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH 0/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Hi,

On Wed, Aug 09, 2017 at 08:05:57AM +0200, Oleksij Rempel wrote:
> Am 09.08.2017 um 06:13 schrieb Gaël PORTAY:
> > Dear maintainers,
> > 
> > I came accross this issue when I was trying to add support for a new MIPS
> > board.
> 
> Just out of curiosity, what MIPS board are you working on? If it is not
> a secret :)
> 

I am working on VoCore2[1]; it is a board based on a mediatek mt7628 CPU.

I am doing this to learn myself how works barebox 

Offtopic: For now it is booting on a really really old version of u-boot. I am
able de boot barebox from u-boot. I had to hack the mips_disable_interrupts to
remove the reset of flag ERL in CP0; but I don't know why yet. This flag is
supposed to be set when CPU reboots; but because I am running barebox from a the
command go of u-boot, this flag is unset.

Are you able to re-run barebox from barebox on your MIPS board?

	.macro	mips_disable_interrupts
	.set	push
	.set	noreorder
	mfc0	k0, CP0_STATUS
-	li	k1, ~(ST0_ERL | ST0_IE)
+	li	k1, ~ST0_IE
	and	k0, k1
	mtc0	k0, CP0_STATUS
	.set	pop
	.endm

For now, I have splitted mips_disable_interrupts into two macros. The second
macro mips_reset_error_level checks if the ERL flag is armed before resetting it
to 0.

	.macro	mips_reset_error_level
	.set	push
	.set	noreorder
	mfc0	k0, CP0_STATUS
	li	k1, ST0_ERL
	and	k1, k0
	bne	k1, zero, 1f
	li	k1, ~(ST0_ERL)
	and	k0, k1
	mtc0	k0, CP0_STATUS
1:	.set	pop
	.endm

But I don't know if this is the right thing to do.
 
[1]: https://www.indiegogo.com/projects/vocore2-4-coin-sized-linux-computer-with-wifi#/

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

Re: [PATCH 0/2] readline_simple: return -1 if getc fails

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 2291 bytes --]

Am 09.08.2017 um 20:58 schrieb Gaël PORTAY:
> Hi,
> 
> On Wed, Aug 09, 2017 at 08:05:57AM +0200, Oleksij Rempel wrote:
>> Am 09.08.2017 um 06:13 schrieb Gaël PORTAY:
>>> Dear maintainers,
>>>
>>> I came accross this issue when I was trying to add support for a new MIPS
>>> board.
>>
>> Just out of curiosity, what MIPS board are you working on? If it is not
>> a secret :)
>>
> 
> I am working on VoCore2[1]; it is a board based on a mediatek mt7628 CPU.
> 
> I am doing this to learn myself how works barebox 
> 
> Offtopic: For now it is booting on a really really old version of u-boot. I am
> able de boot barebox from u-boot. I had to hack the mips_disable_interrupts to
> remove the reset of flag ERL in CP0; but I don't know why yet. This flag is
> supposed to be set when CPU reboots; but because I am running barebox from a the
> command go of u-boot, this flag is unset.
> 
> Are you able to re-run barebox from barebox on your MIPS board?

Yes, usually i prefer to make a binaries wich are able to start from
RAM, Jtag and Flash. Currently we test if it is running from spi flash
adn skip most of the low level init:
https://github.com/olerem/barebox/blob/ar9344-2017.08.01.1/arch/mips/boards/tplink-mr3020/include/board/board_pbl_start.h#L29

On this board i decided to use SRAM for low level init testing:
https://github.com/olerem/barebox/blob/ar9344-2017.08.01.1/arch/mips/boards/tplink-wdr4300/include/board/board_pbl_start.h#L38

> 	.macro	mips_disable_interrupts
> 	.set	push
> 	.set	noreorder
> 	mfc0	k0, CP0_STATUS
> -	li	k1, ~(ST0_ERL | ST0_IE)
> +	li	k1, ~ST0_IE
> 	and	k0, k1
> 	mtc0	k0, CP0_STATUS
> 	.set	pop
> 	.endm
> 
> For now, I have splitted mips_disable_interrupts into two macros. The second
> macro mips_reset_error_level checks if the ERL flag is armed before resetting it
> to 0.
> 
> 	.macro	mips_reset_error_level
> 	.set	push
> 	.set	noreorder
> 	mfc0	k0, CP0_STATUS
> 	li	k1, ST0_ERL
> 	and	k1, k0
> 	bne	k1, zero, 1f
> 	li	k1, ~(ST0_ERL)
> 	and	k0, k1
> 	mtc0	k0, CP0_STATUS
> 1:	.set	pop
> 	.endm
> 
> But I don't know if this is the right thing to do.
>  
> [1]: https://www.indiegogo.com/projects/vocore2-4-coin-sized-linux-computer-with-wifi#/
> 


-- 
Regards,
Oleksij


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH 0/2] readline_simple: return -1 if getc fails

[Antony Pavlov]

On Wed, 9 Aug 2017 14:58:53 -0400
Gaël PORTAY <gael.portay@savoirfairelinux.com> wrote:

> Hi,
> 
> On Wed, Aug 09, 2017 at 08:05:57AM +0200, Oleksij Rempel wrote:
> > Am 09.08.2017 um 06:13 schrieb Gaël PORTAY:
> > > Dear maintainers,
> > > 
> > > I came accross this issue when I was trying to add support for a new MIPS
> > > board.
> > 
> > Just out of curiosity, what MIPS board are you working on? If it is not
> > a secret :)
> > 
> 
> I am working on VoCore2[1]; it is a board based on a mediatek mt7628 CPU.
> 
> I am doing this to learn myself how works barebox 

Please see this branch for mt7628:

  https://github.com/frantony/barebox/tree/20160815.mediatek



> 
> Offtopic: For now it is booting on a really really old version of u-boot. I am
> able de boot barebox from u-boot. I had to hack the mips_disable_interrupts to
> remove the reset of flag ERL in CP0; but I don't know why yet. This flag is
> supposed to be set when CPU reboots; but because I am running barebox from a the
> command go of u-boot, this flag is unset.
> 
> Are you able to re-run barebox from barebox on your MIPS board?
> 
> 	.macro	mips_disable_interrupts
> 	.set	push
> 	.set	noreorder
> 	mfc0	k0, CP0_STATUS
> -	li	k1, ~(ST0_ERL | ST0_IE)
> +	li	k1, ~ST0_IE
> 	and	k0, k1
> 	mtc0	k0, CP0_STATUS
> 	.set	pop
> 	.endm
> 
> For now, I have splitted mips_disable_interrupts into two macros. The second
> macro mips_reset_error_level checks if the ERL flag is armed before resetting it
> to 0.
> 
> 	.macro	mips_reset_error_level
> 	.set	push
> 	.set	noreorder
> 	mfc0	k0, CP0_STATUS
> 	li	k1, ST0_ERL
> 	and	k1, k0
> 	bne	k1, zero, 1f
> 	li	k1, ~(ST0_ERL)
> 	and	k0, k1
> 	mtc0	k0, CP0_STATUS
> 1:	.set	pop
> 	.endm
> 
> But I don't know if this is the right thing to do.
>  
> [1]: https://www.indiegogo.com/projects/vocore2-4-coin-sized-linux-computer-with-wifi#/
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


-- 
Best regards,
  Antony Pavlov

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

[PATCH 0/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Dear maintainers,

I came accross this issue when I was trying to add support for a new MIPS
board.

The getc() function from console_simple.c returns -EINVAL when no console is
registered.

It causes readline_simple to print a non-printable character (-EINVAL in this
case). Furthermore, it causes an infinite loop: readline never returns.

	barebox:/ ���(...)���

With this patch, the error is no more explicit but the readline function stops
to print a dummy character and returns to shell.

	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	(...)
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>
	barebox:/ <INTERRUPT>

The second patch is a simple cleanup of the in-file documentation.

Regards,
Gaël PORTAY (2):
  readline_simple: return -1 if getc fails
  readline_simple: remove obsolete documentation

 lib/readline_simple.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.13.2


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