Re: [PATCH v1 05/14] xen/riscv: add early_printk_hnum() function

From: Jan Beulich <jbeulich@suse.com>
To: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Gianluca Guida <gianluca@rivosinc.com>,
	Bob Eshleman <bobbyeshleman@gmail.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	Connor Davis <connojdavis@gmail.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v1 05/14] xen/riscv: add early_printk_hnum() function
Date: Mon, 23 Jan 2023 12:10:05 +0100	[thread overview]
Message-ID: <35c3175c-47f7-80c9-c417-1320aab02de7@suse.com> (raw)
In-Reply-To: <633ced21788a3abf5079c9a191794616bb1ad351.1674226563.git.oleksii.kurochko@gmail.com>

On 20.01.2023 15:59, Oleksii Kurochko wrote:
> Add ability to print hex number.
> It might be useful to print register value as debug information
> in BUG(), WARN(), etc...
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Orthogonal to Andrew's reply (following which I think would be best)
a couple of comments which may be applicable elsewhere as well:

> --- a/xen/arch/riscv/early_printk.c
> +++ b/xen/arch/riscv/early_printk.c
> @@ -43,3 +43,42 @@ void early_printk(const char *str)
>          str++;
>      }
>  }
> +
> +static void reverse(char *s, int length)

Please can you get things const-correct (const char *s) and signedness-
correct (unsigned int length) from the beginning. We're converting other
code as we touch it, but this is extremely slow going and hence would
better be avoided in the first place in new code.

> +{
> +    int c;
> +    char *begin, *end, temp;
> +
> +    begin  = s;
> +    end    = s + length - 1;
> +
> +    for ( c = 0; c < length/2; c++ )

Style: Blanks around binary operators.

> +    {
> +        temp   = *end;
> +        *end   = *begin;
> +        *begin = temp;
> +
> +        begin++;
> +        end--;
> +    }
> +}
> +
> +void early_printk_hnum(const register_t reg_val)

Likely this function wants to be __init? (All functions that can be
should also be made so.) With that, reverse() then would also want
to become __init.

As to the const here vs the remark further up: In cases like this one
we typically don't use const. You're free to keep it of course, but
I think it should at least be purged from the declaration (and maybe
also the stub).

> +{
> +    char hex[] = "0123456789ABCDEF";

static const char __initconst?

> +    char buf[17] = {0};
> +
> +    register_t num = reg_val;
> +    unsigned int count = 0;
> +
> +    for ( count = 0; num != 0; count++, num >>= 4 )
> +        buf[count] = hex[num & 0x0000000f];

Just 0xf?

Jan