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:   Sun, 28 Apr 2019 19:36:18 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     fred@fredlawl.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, mika.westerberg@linux.intel.com,
        andriy.shevchenko@linux.intel.com, keith.busch@intel.com,
        mr.nuke.me@gmail.com, liudongdong3@huawei.com, thesven73@gmail.com
Subject: Re: [PATCH 2/4] PCI: pciehp: Replace ctrl_*() with pci_*()
Message-ID: <20190429003618.GN14616@google.com>
References: <20190427191304.32502-1-fred@fredlawl.com>
 <20190427191304.32502-3-fred@fredlawl.com>
 <20190427200301.tujp2535jxmlqttr@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190427200301.tujp2535jxmlqttr@wunner.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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, "...");