Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Julien Grall <julien@xen.org>]

Hi,

On 16/09/2022 08:19, Michal Orzel wrote:
> Using dprintk results in printing additionally file name and line
> number. This is something we do not want when printing regular
> information unconditionally as it looks like as if there was some issue.
I am OK if you want to switch to a printk() but I disagree with this 
argument. dprintk() is not about error, it is about anything that 
doesn't matter in release build.

I don't think we should just switch to printk() because dprintk() add 
the line/file. There are message we don't necessarily want to have in 
release build. So dprintk(XENLOG_INFO, ...) would be right for them.

Personally, I find them useful as there no grep required and/or 
confusion (but that's a matter of taste). If it were me, I would add the 
line/file everywhere. But I understand this takes space in the binary 
(hence why this is not present in release build).

A better argument to switch to printk() is this information is useful to 
the user even outside of the debug build.

> 
> Fix this by switching to printk because this information may also be
> helpful on the release builds (it would still require setting loglvl to
> "info" or lower level).

I think we should drop XENLOG_INFO to be consistent with the other 
printk() in domain_build.c (after all this is a domain information like 
the other) or use XENLOG_INFO everywhere.

My preference will be the former because otherwise most of the 
information will not printed in release build by default.

> 
> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")

Fixes should only be used for bugs. This is not one.

> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Rationale for taking this patch for 4.17:
> Current code results in an abnormal behavior [1] and was introduced by

It is not abnormal (see above). This is an expected behavior when you 
use dprintk().

> the 4.17 feature (static shared memory). Even though it can only be seen
> on a debug build, it should be fixed now so that we have a consistent
> behavior across all the logs.

As I wrote above, I agree this should be printed in release build. But I 
disagree with your arguments.

Cheers,

-- 
Julien Grall