linux-kernel - Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci

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]

Message-ID: <20190429003618.GN14616@google.com>
Date:   Sun, 28 Apr 2019 19:36:18 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     fred@...dlawl.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, mika.westerberg@...ux.intel.com,
        andriy.shevchenko@...ux.intel.com, keith.busch@...el.com,
        mr.nuke.me@...il.com, liudongdong3@...wei.com, thesven73@...il.com
Subject: Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()

On Sat, Apr 27, 2019 at 10:03:01PM +0200, Lukas Wunner wrote:
> On Sat, Apr 27, 2019 at 02:13:02PM -0500, fred@...dlawl.com wrote:
> > Hotplug useses custom ctrl_*() dev_*() printk wrappers for logging
> > messages. To make hotplug conform to pci logging, replace uses of these
> > wrappers with pci_*() printk wrappers. In addition, replace any
> > printk() calls with pr_*() wrappers.
> 
> A lot of pciehp's messages are preceded by "Slot(%s): ", where %s is
> replaced by the Physical Slot Number in the Slot Capabilities register
> (which is cached in struct controller) plus an optional suffix if the
> same PSN is used by multiple slots.  For some reason (probably a
> historic artefact), this prefix is included only in *some* of the
> messages.
> 
> I think it would be useful to make the messages consistent by *always*
> including the "Slot(%s): " prefix.  However the prefix is unknown until
> pci_hp_initialize() has been called.  I'd solve this by keeping the
> ctrl_*() wrappers and amending them to print the "Slot(%s): " prefix,
> then making sure that ctrl_*() is not called before pci_hp_initialize().
> (pci_*() has to be used instead).

I was hoping to get rid of the ctrl_*() wrappers completely, but the
"Slot(%s)" prefix is actually a pretty good reason to keep them.

Moving the prefix from the call site to the ctrl_*() wrappers should be a
separate patch that doesn't change the output at all (except maybe
adding the prefix to messages that don't currently include it).

So you probably need three steps (each in a separate patch):

  1) Convert ctrl_*() to use pci_dev instead of pcie_device, something
     like this:

        + #define pr_fmt(fmt) "pciehp: " fmt
	+ #define dev_fmt pr_fmt

	  #define ctrl_info(ctrl, format, arg...) \
	-   dev_info(&ctrl->pcie->device, format, ## arg)
	+   pci_info(&ctrl->pcie->port, format, ## arg)

  2) Convert any output before pci_hp_initialize() from ctrl_*() to
     pci_*().

  3) Centralize the "Slot(%s): " prefix, something like this:

	  #define ctrl_info(ctrl, format, arg...) \
	-   pci_info(&ctrl->pcie->port, format, ## arg)
	+   pci_info(&ctrl->pcie->port, "Slot(%s): " format, slot_name(ctrl), ## arg)

	- ctrl_info(ctrl, "Slot(%s): ...", slot_name(ctrl));
	+ ctrl_info(ctrl, "...");