Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] x86/e820: Use pr_debug to avoid spamming dmesg log
 with debug messages
To:     Joe Perches <joe@perches.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <f527618e-54f2-c2fb-e267-8065ac34e462@gmail.com>
 <6d55670d-2f06-d768-699f-5a79cece6ce0@gmail.com>
 <d181b674-66fb-1719-e3c6-e4217cf5519c@akamai.com>
 <59985635-665b-773f-de8f-b15fe3f60196@gmail.com>
 <1789b1da-7a0f-9fc9-6ed2-ff68e2306342@akamai.com>
 <204a99e1-de7a-4434-0932-45f1b507e9ec@gmail.com>
 <5a416031-ddcd-7ef4-ec33-47134bf064bb@akamai.com>
 <7cc30a8a6644bb60e5f3358e89253d9fc783fe54.camel@perches.com>
From:   Jason Baron <jbaron@akamai.com>
Message-ID: <e0116c3e-21a3-50a4-e9fd-cb227ef0b63b@akamai.com>
Date:   Fri, 14 May 2021 15:56:01 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <7cc30a8a6644bb60e5f3358e89253d9fc783fe54.camel@perches.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/14/21 1:38 PM, Joe Perches wrote:
> On Tue, 2021-05-11 at 17:31 -0400, Jason Baron wrote:
> 
>> That said, I do see the value in not having to open code the branch stuff, and
>> making pr_debug() consistent with printk which does return a value. So that
>> makes sense to me.
> 
> IMO: printk should not return a value.
> 

Ok, the issue we are trying to resolve is to control whether a 'pr_debug()' statement
is enabled and thus use that to control subsequent output. The proposed patch does:


+	printed = pr_debug("e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
+	if (printed > 0) {
+		e820_print_type(old_type);
+		pr_cont(" ==> ");
+		e820_print_type(new_type);
+		pr_cont("\n");
+	}

I do think pr_debug() here is different from printk() b/c it can be explicitly
toggled.

I also suggested an alternative, which is possible with the current code which
is to use DYNAMIC_DEBUG_BRANCH().

if (DYNAMIC_DEBUG_BRANCH(e820_debg)) {
    printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
    e820_print_type(old_type);
    pr_cont(" ==> ");
    e820_print_type(new_type);
    pr_cont("\n");
}

That however does require one to do something like this first:

DEFINE_DYNAMIC_DEBUG_METADATA(e820_dbg, "e820 verbose mode");

So I don't feel too strongly either way, but maybe others do?

Thanks,

-Jason