mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/2] readline_simple: return -1 if getc fails
@ 2017-08-07 22:10 Gaël PORTAY
  2017-08-07 22:10 ` [PATCH 1/2] " Gaël PORTAY
  2017-08-07 22:10 ` [PATCH 2/2] readline_simple: remove obsolete documentation Gaël PORTAY
  0 siblings, 2 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-07 22:10 UTC (permalink / raw)
  To: barebox; +Cc: 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-07 22:10 [PATCH 0/2] readline_simple: return -1 if getc fails Gaël PORTAY
@ 2017-08-07 22:10 ` Gaël PORTAY
  2017-08-08  7:51   ` Lucas Stach
  2017-08-07 22:10 ` [PATCH 2/2] readline_simple: remove obsolete documentation Gaël PORTAY
  1 sibling, 1 reply; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-07 22:10 UTC (permalink / raw)
  To: barebox; +Cc: 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index c4d3d240e..1283c9602 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
 
 	for (;;) {
 		c = getchar();
+		if (c < 0)
+			return (-1);
 
 		/*
 		 * Special character handling
-- 
2.13.2


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] readline_simple: remove obsolete documentation
  2017-08-07 22:10 [PATCH 0/2] readline_simple: return -1 if getc fails Gaël PORTAY
  2017-08-07 22:10 ` [PATCH 1/2] " Gaël PORTAY
@ 2017-08-07 22:10 ` Gaël PORTAY
  1 sibling, 0 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-07 22:10 UTC (permalink / raw)
  To: barebox; +Cc: 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 | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index 1283c9602..80e075bc5 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
  */
 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-07 22:10 ` [PATCH 1/2] " Gaël PORTAY
@ 2017-08-08  7:51   ` Lucas Stach
  2017-08-08 10:07     ` Ian Abbott
  2017-08-08 15:20     ` Gaël PORTAY
  0 siblings, 2 replies; 16+ messages in thread
From: Lucas Stach @ 2017-08-08  7:51 UTC (permalink / raw)
  To: Gaël PORTAY; +Cc: barebox

Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> index c4d3d240e..1283c9602 100644
> --- a/lib/readline_simple.c
> +++ b/lib/readline_simple.c
> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>  
>  	for (;;) {
>  		c = getchar();
> +		if (c < 0)
> +			return (-1);

I don't like made up error codes. Is there any reason why we couldn't
just pass through the negative error code from getchar?

Regards,
Lucas


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08  7:51   ` Lucas Stach
@ 2017-08-08 10:07     ` Ian Abbott
  2017-08-08 15:21       ` Gaël PORTAY
  2017-08-08 15:20     ` Gaël PORTAY
  1 sibling, 1 reply; 16+ messages in thread
From: Ian Abbott @ 2017-08-08 10:07 UTC (permalink / raw)
  To: Lucas Stach, Gaël PORTAY; +Cc: barebox

On 08/08/17 08:51, Lucas Stach wrote:
> Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
>> index c4d3d240e..1283c9602 100644
>> --- a/lib/readline_simple.c
>> +++ b/lib/readline_simple.c
>> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>>   
>>   	for (;;) {
>>   		c = getchar();
>> +		if (c < 0)
>> +			return (-1);
> 
> I don't like made up error codes. Is there any reason why we couldn't
> just pass through the negative error code from getchar?

And also change the type of the 'c' variable, as it is currently a plain 
'char'?

> Regards,
> Lucas

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08  7:51   ` Lucas Stach
  2017-08-08 10:07     ` Ian Abbott
@ 2017-08-08 15:20     ` Gaël PORTAY
  2017-08-08 15:36       ` Lucas Stach
  1 sibling, 1 reply; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-08 15:20 UTC (permalink / raw)
  To: Lucas Stach; +Cc: barebox

Hi Lucas,

On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > index c4d3d240e..1283c9602 100644
> > --- a/lib/readline_simple.c
> > +++ b/lib/readline_simple.c
> > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> >  
> >  	for (;;) {
> >  		c = getchar();
> > +		if (c < 0)
> > +			return (-1);
> 
> I don't like made up error codes. Is there any reason why we couldn't
> just pass through the negative error code from getchar?
> 

The thing here is that getchar() may return an error, and that error is not
tested. This causes readline to print the character 0xea (-EINVAL) which is not
printable.

Maybe another solutions would be to print the errno string and continue; or to
return the number of characters already read.

Regards,
Gaël

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08 10:07     ` Ian Abbott
@ 2017-08-08 15:21       ` Gaël PORTAY
  0 siblings, 0 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-08 15:21 UTC (permalink / raw)
  To: Ian Abbott; +Cc: barebox

Hi Ian,

On Tue, Aug 08, 2017 at 11:07:54AM +0100, Ian Abbott wrote:
> On 08/08/17 08:51, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >   	for (;;) {
> > >   		c = getchar();
> > > +		if (c < 0)
> > > +			return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> 
> And also change the type of the 'c' variable, as it is currently a plain
> 'char'?
> 

Indeed :)

Regards,
Gaël

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08 15:20     ` Gaël PORTAY
@ 2017-08-08 15:36       ` Lucas Stach
  2017-08-08 16:05         ` Ian Abbott
  2017-08-08 16:14         ` Gaël PORTAY
  0 siblings, 2 replies; 16+ messages in thread
From: Lucas Stach @ 2017-08-08 15:36 UTC (permalink / raw)
  To: Gaël PORTAY; +Cc: barebox

Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> Hi Lucas,
> 
> On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >  
> > >  	for (;;) {
> > >  		c = getchar();
> > > +		if (c < 0)
> > > +			return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> > 
> 
> The thing here is that getchar() may return an error, and that error is not
> tested. This causes readline to print the character 0xea (-EINVAL) which is not
> printable.

So why wouldn't the following fix the issue?

signed char c;

if (c < 0)
	return c;

Regards,
Lucas



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08 15:36       ` Lucas Stach
@ 2017-08-08 16:05         ` Ian Abbott
  2017-08-08 16:49           ` Gaël PORTAY
  2017-08-08 16:14         ` Gaël PORTAY
  1 sibling, 1 reply; 16+ messages in thread
From: Ian Abbott @ 2017-08-08 16:05 UTC (permalink / raw)
  To: Lucas Stach, Gaël PORTAY; +Cc: barebox

On 08/08/17 16:36, Lucas Stach wrote:
> Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
>> Hi Lucas,
>>
>> On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
>>> Am Montag, den 07.08.2017, 18:10 -0400 schrieb 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 | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
>>>> index c4d3d240e..1283c9602 100644
>>>> --- a/lib/readline_simple.c
>>>> +++ b/lib/readline_simple.c
>>>> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>>>>   
>>>>   	for (;;) {
>>>>   		c = getchar();
>>>> +		if (c < 0)
>>>> +			return (-1);
>>>
>>> I don't like made up error codes. Is there any reason why we couldn't
>>> just pass through the negative error code from getchar?
>>>
>>
>> The thing here is that getchar() may return an error, and that error is not
>> tested. This causes readline to print the character 0xea (-EINVAL) which is not
>> printable.
> 
> So why wouldn't the following fix the issue?
> 
> signed char c;

`int` would be better to allow non-ASCII characters.

> 
> if (c < 0)
> 	return c;

There are places where the return value is checked for `-1` for example 
in get_user_input() ("common/hush.c"), and in run_shell() 
("common/parser.c").

I think Gaël's patch is reasonable, although perhaps it should also set 
`line[0] = '\0';` before returning.

Off topic: there is another oddity in the the "simple" version of 
readline(). It ignores the `len` parameter and uses `CONFIG_CBSIZE` instead.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08 15:36       ` Lucas Stach
  2017-08-08 16:05         ` Ian Abbott
@ 2017-08-08 16:14         ` Gaël PORTAY
  1 sibling, 0 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-08 16:14 UTC (permalink / raw)
  To: Lucas Stach; +Cc: barebox

On Tue, Aug 08, 2017 at 05:36:14PM +0200, Lucas Stach wrote:
> Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> > Hi Lucas,
> > 
> > On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > > index c4d3d240e..1283c9602 100644
> > > > --- a/lib/readline_simple.c
> > > > +++ b/lib/readline_simple.c
> > > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > > >  
> > > >  	for (;;) {
> > > >  		c = getchar();
> > > > +		if (c < 0)
> > > > +			return (-1);
> > > 
> > > I don't like made up error codes. Is there any reason why we couldn't
> > > just pass through the negative error code from getchar?
> > > 
> > 
> > The thing here is that getchar() may return an error, and that error is not
> > tested. This causes readline to print the character 0xea (-EINVAL) which is not
> > printable.
> 
> So why wouldn't the following fix the issue?
> 
> signed char c;
> 
> if (c < 0)
> 	return c;
> 

Okay. I do prefer your solution.

I returned -1 mainly because the function comment says it returns -1 when it
breaks; and because parser.c and hush.c test readline function against -1 and
not against a negative value.

Also readline returns -1 if character is 0x03. Maybe it should return -EINTR;
-1 is EPERM: Operation not permitted.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] readline_simple: return -1 if getc fails
  2017-08-08 16:05         ` Ian Abbott
@ 2017-08-08 16:49           ` Gaël PORTAY
  0 siblings, 0 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-08 16:49 UTC (permalink / raw)
  To: Ian Abbott; +Cc: barebox

On Tue, Aug 08, 2017 at 05:05:35PM +0100, Ian Abbott wrote:
> On 08/08/17 16:36, Lucas Stach wrote:
> > Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> > > Hi Lucas,
> > > 
> > > On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > > > (...)
> > > > I don't like made up error codes. Is there any reason why we couldn't
> > > > just pass through the negative error code from getchar?
> > > > 
> > > 
> > > The thing here is that getchar() may return an error, and that error is not
> > > tested. This causes readline to print the character 0xea (-EINVAL) which is not
> > > printable.
> > 
> > So why wouldn't the following fix the issue?
> > 
> > signed char c;
> 
> `int` would be better to allow non-ASCII characters.
> 
> > 
> > if (c < 0)
> > 	return c;
> 
> There are places where the return value is checked for `-1` for example in
> get_user_input() ("common/hush.c"), and in run_shell() ("common/parser.c").
> 
> I think Gaël's patch is reasonable, although perhaps it should also set
> `line[0] = '\0';` before returning.
> 

Indeed, or `line[n] = '\0';` to preserve characters already entered.

BTW, it already performed by get_user_input in hush.c (n is reset to 0).

console_buffer[n] = '\n';
console_buffer[n + 1]= '\0';

> Off topic: there is another oddity in the the "simple" version of
> readline(). It ignores the `len` parameter and uses `CONFIG_CBSIZE` instead.
> 

I made a patch for this; but I have not tested yet all cases.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/2] readline_simple: return -1 if getc fails
  2017-08-09 18:58   ` Gaël PORTAY
  2017-08-09 19:41     ` Oleksij Rempel
@ 2017-08-10  5:51     ` Antony Pavlov
  1 sibling, 0 replies; 16+ messages in thread
From: Antony Pavlov @ 2017-08-10  5:51 UTC (permalink / raw)
  To: Gaël PORTAY; +Cc: barebox, Oleksij Rempel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/2] readline_simple: return -1 if getc fails
  2017-08-09 18:58   ` Gaël PORTAY
@ 2017-08-09 19:41     ` Oleksij Rempel
  2017-08-10  5:51     ` Antony Pavlov
  1 sibling, 0 replies; 16+ messages in thread
From: Oleksij Rempel @ 2017-08-09 19:41 UTC (permalink / raw)
  To: Gaël PORTAY; +Cc: barebox


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/2] readline_simple: return -1 if getc fails
  2017-08-09  6:05 ` Oleksij Rempel
@ 2017-08-09 18:58   ` Gaël PORTAY
  2017-08-09 19:41     ` Oleksij Rempel
  2017-08-10  5:51     ` Antony Pavlov
  0 siblings, 2 replies; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-09 18:58 UTC (permalink / raw)
  To: Oleksij Rempel; +Cc: barebox

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/2] readline_simple: return -1 if getc fails
  2017-08-09  4:13 [PATCH 0/2] readline_simple: return -1 if getc fails Gaël PORTAY
@ 2017-08-09  6:05 ` Oleksij Rempel
  2017-08-09 18:58   ` Gaël PORTAY
  0 siblings, 1 reply; 16+ messages in thread
From: Oleksij Rempel @ 2017-08-09  6:05 UTC (permalink / raw)
  To: barebox


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 0/2] readline_simple: return -1 if getc fails
@ 2017-08-09  4:13 Gaël PORTAY
  2017-08-09  6:05 ` Oleksij Rempel
  0 siblings, 1 reply; 16+ messages in thread
From: Gaël PORTAY @ 2017-08-09  4:13 UTC (permalink / raw)
  To: barebox; +Cc: 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-08-10  5:41 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-07 22:10 [PATCH 0/2] readline_simple: return -1 if getc fails Gaël PORTAY
2017-08-07 22:10 ` [PATCH 1/2] " Gaël PORTAY
2017-08-08  7:51   ` Lucas Stach
2017-08-08 10:07     ` Ian Abbott
2017-08-08 15:21       ` Gaël PORTAY
2017-08-08 15:20     ` Gaël PORTAY
2017-08-08 15:36       ` Lucas Stach
2017-08-08 16:05         ` Ian Abbott
2017-08-08 16:49           ` Gaël PORTAY
2017-08-08 16:14         ` Gaël PORTAY
2017-08-07 22:10 ` [PATCH 2/2] readline_simple: remove obsolete documentation Gaël PORTAY
2017-08-09  4:13 [PATCH 0/2] readline_simple: return -1 if getc fails Gaël PORTAY
2017-08-09  6:05 ` Oleksij Rempel
2017-08-09 18:58   ` Gaël PORTAY
2017-08-09 19:41     ` Oleksij Rempel
2017-08-10  5:51     ` Antony Pavlov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox