Re: [PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix Log
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
> > > ---
> > > 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.
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
> > ---
> > 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.
Re: [PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix Log
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
> ---
> 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
> out:
> if (info->id && info->error_dev_num > 1 && info->id == id)
> @@ -796,7 +790,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
> aer->uncor_severity);
>
> if (tlp_header_valid)
> - __print_tlp_header(dev, &aer->header_log);
> + pcie_print_tlp_log(dev, &aer->header_log, " ");
>
> trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> aer_severity, tlp_header_valid, &aer->header_log);
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 5056cc6961ec..598f74384471 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -220,10 +220,7 @@ static void dpc_process_rp_pio_error(struct pci_dev
> *pdev)
> pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
> cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
> dpc_tlp_log_len(pdev), &tlp_log);
> - pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
> - tlp_log.dw[0], tlp_log.dw[1], tlp_log.dw[2], tlp_log.dw[3]);
> - for (i = 0; i < pdev->dpc_rp_log_size - 5; i++)
> - pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i,
> tlp_log.prefix[i]);
> + pcie_print_tlp_log(pdev, &tlp_log, "");
>
> if (pdev->dpc_rp_log_size < 5)
> goto clear_status;
> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index def9dd7b73e8..097ac8514e96 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -6,6 +6,7 @@
> */
>
> #include
> +#include
> #include
> #include
>
> @@ -76,3 +77,33 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int
> where2,
>
> return 0;
> }
> +
> +/**
> + * pcie_print_tlp_log - Print TLP Header / Prefix Log contents
> + * @dev: PCIe device
> + * @log: TLP Log structure
> + * @pfx: String prefix (for print out indentation)
> + *
> + * Prints TLP Header and Prefix Log information held by @log.
> + */
> +void pcie_print_tlp_log(const struct pci_dev *dev,
> +
[PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix Log
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
---
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, " ");
out:
if (info->id && info->error_dev_num > 1 && info->id == id)
@@ -796,7 +790,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
aer->uncor_severity);
if (tlp_header_valid)
- __print_tlp_header(dev, &aer->header_log);
+ pcie_print_tlp_log(dev, &aer->header_log, " ");
trace_aer_event(dev_name(&dev->dev), (status & ~mask),
aer_severity, tlp_header_valid, &aer->header_log);
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 5056cc6961ec..598f74384471 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -220,10 +220,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
dpc_tlp_log_len(pdev), &tlp_log);
- pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
- tlp_log.dw[0], tlp_log.dw[1], tlp_log.dw[2], tlp_log.dw[3]);
- for (i = 0; i < pdev->dpc_rp_log_size - 5; i++)
- pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i,
tlp_log.prefix[i]);
+ pcie_print_tlp_log(pdev, &tlp_log, "");
if (pdev->dpc_rp_log_size < 5)
goto clear_status;
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index def9dd7b73e8..097ac8514e96 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -6,6 +6,7 @@
*/
#include
+#include
#include
#include
@@ -76,3 +77,33 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int
where2,
return 0;
}
+
+/**
+ * pcie_print_tlp_log - Print TLP Header / Prefix Log contents
+ * @dev: PCIe device
+ * @log: TLP Log structure
+ * @pfx: String prefix (for print out indentation)
+ *
+ * Prints TLP Header and Prefix Log information held by @log.
+ */
+void pcie_print_tlp_log(const struct pci_dev *dev,
+ const struct pcie_tlp_log *log, const char *pfx)
+{
+ char buf[(10 + 1) * (4 + ARRAY_SIZE(log->prefix)) + 14 + 1];
+ unsigned int i;
+ int len;
+
+ len = scnprintf(buf, sizeof(buf), "%#010x %#010x %#010x %#010x",
+ log->dw[0], log->dw[1], log->dw[2], log->dw[3]);
+
+ if (log->prefix[0])
+ len += scnprintf(buf + len, sizeof(buf) - len, " E-E
Prefixes:");
+ for (i = 0; i < ARRAY_SIZE(log->prefix); i++) {
+ if (!log->prefix[i])
+ break;
+ len += scnprintf(buf + len, sizeof(buf) - len,
+" %#010x", log->prefix[i]);
+ }
+
+ pci_err(dev, "%sTLP Header: %s\n", pfx, buf);
+}
