Messages in this thread | | | Date | Sun, 28 Apr 2019 19:36:18 -0500 | From | Bjorn Helgaas <> | 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@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, "...");
| |