Re: [PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix Log

[Bjorn Helgaas]

On Mon, Sep 02, 2024 at 08:20:41PM +0300, Ilpo Järvinen wrote:
> On Fri, 30 Aug 2024, Bjorn Helgaas wrote:
> 
> > On Tue, May 14, 2024 at 02:31:09PM +0300, Ilpo Järvinen wrote:
> > > Add pcie_print_tlp_log() helper to print TLP Header and Prefix Log.
> > > Print End-End Prefixes only if they are non-zero.
> > > 
> > > Consolidate the few places which currently print TLP using custom
> > > formatting.
> > > 
> > > The first attempt used pr_cont() instead of building a string first but
> > > it turns out pr_cont() is not compatible with pci_err() and prints on a
> > > separate line. When I asked about this, Andy Shevchenko suggested
> > > pr_cont() should not be used in the first place (to eventually get rid
> > > of it) so pr_cont() is now replaced with building the string first.
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/pci/pci.h      |  2 ++
> > >  drivers/pci/pcie/aer.c | 10 ++--------
> > >  drivers/pci/pcie/dpc.c |  5 +----
> > >  drivers/pci/pcie/tlp.c | 31 +++++++++++++++++++++++++++++++
> > >  4 files changed, 36 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 7afdd71f9026..45083e62892c 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -423,6 +423,8 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> > >  int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
> > >  		      unsigned int tlp_len, struct pcie_tlp_log *log);
> > >  unsigned int aer_tlp_log_len(struct pci_dev *dev);
> > > +void pcie_print_tlp_log(const struct pci_dev *dev,
> > > +			const struct pcie_tlp_log *log, const char *pfx);
> > >  #endif	/* CONFIG_PCIEAER */
> > >  
> > >  #ifdef CONFIG_PCIEPORTBUS
> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > index ecc1dea5a208..efb9e728fe94 100644
> > > --- a/drivers/pci/pcie/aer.c
> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -664,12 +664,6 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
> > >  	}
> > >  }
> > >  
> > > -static void __print_tlp_header(struct pci_dev *dev, struct pcie_tlp_log *t)
> > > -{
> > > -	pci_err(dev, "  TLP Header: %08x %08x %08x %08x\n",
> > > -		t->dw[0], t->dw[1], t->dw[2], t->dw[3]);
> > > -}
> > > -
> > >  static void __aer_print_error(struct pci_dev *dev,
> > >  			      struct aer_err_info *info)
> > >  {
> > > @@ -724,7 +718,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
> > >  	__aer_print_error(dev, info);
> > >  
> > >  	if (info->tlp_header_valid)
> > > -		__print_tlp_header(dev, &info->tlp);
> > > +		pcie_print_tlp_log(dev, &info->tlp, "  ");
> > 
> > I see you went to some trouble to preserve the previous output, down
> > to the number of spaces prefixing it.
> > 
> > But more than the leading spaces, I think what people will notice is
> > that previously AER and DPC dmesgs contain the "AER: " or "DPC: "
> > prefixes implicitly added by the dev_fmt definitions [1], where now
> > IIUC they won't.
> > 
> > I think adding dev_fmt("") here should take care of that, e.g.,
> > 
> >   pcie_print_tlp_log(dev, &info->tlp, dev_fmt(""));
> > 
> > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1990272
> 
> Okay, I'll fix it and resend but looking into that output, it doesn't 
> look very consistent when it comes to prefixes as the lines in between do 
> not start with "AER: " either. Perhaps those lines should be changed as 
> well?

True.  Possibility for future patches.