Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Apr 27, 2019 at 10:03:01PM +0200, Lukas Wunner wrote:
> On Sat, Apr 27, 2019 at 02:13:02PM -0500, fred@fredlawl.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, "...");