linux-kernel - Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Aug 2019 20:40:27 +0200
From:   Krzysztof Wilczynski <kswilczynski@...il.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Krzysztof Wilczynski <kw@...ux.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/PCI: Add missing log facility and move to use pr_
 macros in pcbios.c

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