[PATCH 3/3] ARM: early_printk: use printascii() rather than printch()

[Nicolas Pitre]

On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:

> On Tue, Oct 31, 2017 at 01:48:01PM -0400, Nicolas Pitre wrote:
> > On Tue, 31 Oct 2017, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Oct 31, 2017 at 01:06:35PM -0400, Nicolas Pitre wrote:
> > > > That's easy to veryfy with this patch:
> > > 
> > > Unfortunately not that easy, this patch breaks printch.
> > 
> > Right, missed that.
> > 
> > New patch below:
> > 
> > diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
> > index ea9646cc2a..0d1cef4b6e 100644
> > --- a/arch/arm/kernel/debug.S
> > +++ b/arch/arm/kernel/debug.S
> > @@ -79,25 +79,28 @@ hexbuf:		.space 16
> >  
> >  ENTRY(printascii)
> >  		addruart_current r3, r1, r2
> > -		b	2f
> > -1:		waituart r2, r3
> > -		senduart r1, r3
> > -		busyuart r2, r3
> > -		teq	r1, #'\n'
> > -		moveq	r1, #'\r'
> > -		beq	1b
> > -2:		teq	r0, #0
> > +1:		teq	r0, #0
> >  		ldrneb	r1, [r0], #1
> >  		teqne	r1, #0
> > -		bne	1b
> > -		ret	lr
> > +		reteq	lr
> > +		teq     r1, #'\n'
> > +		bne	2f
> > +		mov	r1, '\r'
> > +		waituart r2, r3
> > +		senduart r1, r3
> > +		busyuart r2, r3
> > +		mov	r1, '\n'
> > +2:		waituart r2, r3
> > +		senduart r1, r3
> > +		busyuart r2, r3
> > +		b	1b
> >  ENDPROC(printascii)
> >  
> >  ENTRY(printch)
> >  		addruart_current r3, r1, r2
> >  		mov	r1, r0
> >  		mov	r0, #0
> > -		b	1b
> > +		b	2b
> >  ENDPROC(printch)
> 
> Still, not quite!  printch('\n') with the old code would do this
> (in terms of executed instructions):
> 
>  		mov	r1, r0
>  		mov	r0, #0
> 		b	1b
> 1:		waituart r2, r3
> 		senduart r1, r3
> 		busyuart r2, r3
> 		teq	r1, #'\n'
> 		moveq	r1, #'\r'
> 		beq	1b
> 1:		waituart r2, r3
> 		senduart r1, r3
> 		busyuart r2, r3
> 		teq	r1, #'\n'
> 		...
> 2:		teq	r0, #0
> 		ret	lr
> 
> So a printch('\n') produces "\n\r" on the UART.  If we're fixing
> printascii() to emit "\r\n" instead of "\n\r" for a '\n', then
> printch() should have the same fix, and should not truncate to
> just '\n'.

OK... That's easy to achieve, but is it desirable?

That would imply that early_write() before my patch used to be wrong wrt 
its princh() expectation as it did the extra \r manually on top. This 
probably went unnoticed given that "\r\n\r" produces the same display 
result as "\r\n".

IMHO the semantics of a single character should be just that: a single 
character.  And if some user relied on printch('\n') inserting the \r 
automatically then this would have run into the same display issue we're 
attempting to fix here. That might be why early_write() did the extra \r 
itself.

Thing is: that only printch user user is now gone. So I'd fix the 
semantic issue by simply removing printch altogether.

What do you think?


Nicolas