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

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: <b0484353-8ba5-02c2-e78c-d51aba55bbb7@linux.intel.com>
Date: Mon, 2 Sep 2024 20:20:41 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>, 
    Mahesh J Salgaonkar <mahesh@...ux.ibm.com>, 
    Oliver O'Halloran <oohall@...il.com>, Lukas Wunner <lukas@...ner.de>, 
    LKML <linux-kernel@...r.kernel.org>, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix
 Log

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@...ux.intel.com>
> > ---
> >  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?

-- 
 i.