Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 28 Aug 2019 20:40:27 +0200
From:   Krzysztof Wilczynski <kswilczynski@gmail.com>
Subject: Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_
 macros in pcbios.c
To:     Joe Perches <joe@perches.com>
Cc:     Krzysztof Wilczynski <kw@linux.com>,
        Bjorn Helgaas <helgaas@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Message-Id: <1567017627.3507.0@gmail.com>
In-Reply-To: <a13a086c2dd6dd6259d28e5d1d360e2b4d04ca83.camel@perches.com>
References: <20190825182557.23260-1-kw@linux.com>
        <20190828175120.22164-1-kw@linux.com>
        <a13a086c2dd6dd6259d28e5d1d360e2b4d04ca83.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Joe,

Thank you for feedback.
[...]
>>    Move to pr_debug() over using DBG() from 
>> arch/x86/include/asm/pci_x86.h.
> 
> You might also consider the checkpatch output for this patch.
> 
> arch/x86/pci/pcbios.c:116: WARNING: line over 80 characters
> arch/x86/pci/pcbios.c:116: WARNING: Prefer using '"%s...", __func__' 
> to using 'bios32_service', this function's name, in a string
> arch/x86/pci/pcbios.c:119: WARNING: Prefer using '"%s...", __func__' 
> to using 'bios32_service', this function's name, in a string
> arch/x86/pci/pcbios.c:391: WARNING: line over 80 characters

Good point.

The lines over 80 characters wide would be taken care of when
moving to using the pr_ macros as the line length will now be
shorter contrary to when the e.g., printk(KERNEL_INFO ...),
etc., was used.

The other warnings I am going to address in v3.  I was thinking
of replacing the following:

pr_warn("bios32_service(0x%lx): not present\n", service);

With something that looks like this:

pr_warn("BIOS32 Service(0x%lx): not present\n", service);

Using "bios32_service" name directly or even moving to __func__
feels a lot like an implementation detail is exposed to the
end user.  I am not sure how useful that could be.  Also,
we are already using log lines starting with "BIOS32", thus
it seemed like following them would be the most sensible
choice, especially to keep messages consistent.

What do you think?

Krzysztof