[PATCH v2] lib: sbi: fix missing '\r' for console

[Anup Patel]

On Mon, Feb 13, 2023 at 12:22 PM Xiang W <wxjstz at 126.com> wrote:
>
> 在 2023-02-13星期一的 09:25 +0530，Anup Patel写道：
> > On Mon, Feb 13, 2023 at 8:09 AM Xiang W <wxjstz at 126.com> wrote:
> > >
> > > print is finally implemented by sbi_putc or console_dev->puts. sbi_putc
> > > will add a \r before the output \n. This patch adds missing \r when
> > > outputting characters via console_dev->puts.
> > >
> > > Signed-off-by: Xiang W <wxjstz at 126.com>
> > >
> > > Changes since v2:
> > > - Fix the bug reported by Samuel. Prevent p from accessing memory other
> > >   than strings.
> > > ---
> > >  lib/sbi/sbi_console.c | 46 +++++++++++++++++++++++++++++--------------
> > >  1 file changed, 31 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > > index f3ac003..f8560c9 100644
> > > --- a/lib/sbi/sbi_console.c
> > > +++ b/lib/sbi/sbi_console.c
> > > @@ -46,27 +46,43 @@ void sbi_putc(char ch)
> > >         }
> > >  }
> > >
> > > +static unsigned long console_puts_all(const char *str, unsigned long len)
> > > +{
> > > +       const char *s = str;
> > > +       const char *e = s + len;
> > > +       while (s < e)
> > > +               s += console_dev->console_puts(s, e - s);
> > > +       return len;
> > > +}
> > > +
> > > +static unsigned long console_puts(const char *str, unsigned long len)
> > > +{
> > > +       const char *s, *p, *e;
> > > +       s = str;
> > > +       e = str + len;
> > > +       while (s < e) {
> > > +               p = sbi_strchr(s, '\n');
> > > +               p = p ? p : e;
> > > +               console_puts_all(s, p - s);
> > > +               if (p < e && *p == '\n')
> > > +                       console_puts_all("\r\n", 2);
> > > +               s = p + 1;
> > > +       }
> > > +       return len;
> > > +}
> > > +
> > >  static unsigned long nputs(const char *str, unsigned long len)
> > >  {
> > > -       unsigned long i, ret;
> > > +       unsigned long i;
> > >
> > >         if (console_dev && console_dev->console_puts) {
> > > -               ret = console_dev->console_puts(str, len);
> > > +               console_puts(str, len);
> > >         } else {
> > >                 for (i = 0; i < len; i++)
> > >                         sbi_putc(str[i]);
> > > -               ret = len;
> > >         }
> > >
> > > -       return ret;
> > > -}
> > > -
> > > -static void nputs_all(const char *str, unsigned long len)
> > > -{
> > > -       unsigned long p = 0;
> > > -
> > > -       while (p < len)
> > > -               p += nputs(&str[p], len - p);
> > > +       return len;
> >
> > This patch is already broken because it changes the semantics
> > of nputs() and nputs_all(). The nputs() is supposed is allowed
> > to do partial writes whereas nputs_all() only returns after printing
> > all characters.
> >
> > Regards,
> > Anup
> When the platform does not implement console_dev->console_puts, nputs and
> nputs_all are effectively the same. Also, I don't quite understand the
> circumstances under which partial writes are needed. Will there be a
> performance advantage for multiple sbi calls to complete the printing?

The semihosting driver implements console_puts() and the debugger
implementing it can do partial writes using non-blocking file I/O.

Until the original semantics of nputs() and nputs_all() are preserved,
it's a NACK from my side.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > >  }
> > >
> > >  void sbi_puts(const char *str)
> > > @@ -74,7 +90,7 @@ void sbi_puts(const char *str)
> > >         unsigned long len = sbi_strlen(str);
> > >
> > >         spin_lock(&console_out_lock);
> > > -       nputs_all(str, len);
> > > +       nputs(str, len);
> > >         spin_unlock(&console_out_lock);
> > >  }
> > >
> > > @@ -255,7 +271,7 @@ static int print(char **out, u32 *out_len, const char *format, va_list args)
> > >
> > >         for (; *format != 0; ++format) {
> > >                 if (use_tbuf && !console_tbuf_len) {
> > > -                       nputs_all(console_tbuf, CONSOLE_TBUF_MAX);
> > > +                       nputs(console_tbuf, CONSOLE_TBUF_MAX);
> > >                         console_tbuf_len = CONSOLE_TBUF_MAX;
> > >                         tout = console_tbuf;
> > >                 }
> > > @@ -386,7 +402,7 @@ literal:
> > >         }
> > >
> > >         if (use_tbuf && console_tbuf_len < CONSOLE_TBUF_MAX)
> > > -               nputs_all(console_tbuf, CONSOLE_TBUF_MAX - console_tbuf_len);
> > > +               nputs(console_tbuf, CONSOLE_TBUF_MAX - console_tbuf_len);
> > >
> > >         return pc;
> > >  }
> > > --
> > > 2.39.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi