Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Bjorn Helgaas]

On Wed, Jan 06, 2021 at 02:57:19PM -0500, Jim Quinlan wrote:
> On Wed, Jan 6, 2021 at 2:42 PM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
> >
> > ---------- Forwarded message ---------
> > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > Date: Wed, Jan 6, 2021 at 2:19 PM
> > Subject: Re: [PATCH v2 5/6] PCI: brcmstb: Add panic/die handler to RC driver
> > To: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
> > Cc: <linux-pci@xxxxxxxxxxxxxxx>, Nicolas Saenz Julienne
> > <nsaenzjulienne@xxxxxxx>, <broonie@xxxxxxxxxx>,
> > <bcm-kernel-feedback-list@xxxxxxxxxxxx>, Lorenzo Pieralisi
> > <lorenzo.pieralisi@xxxxxxx>, Rob Herring <robh@xxxxxxxxxx>, Bjorn
> > Helgaas <bhelgaas@xxxxxxxxxx>, Florian Fainelli
> > <f.fainelli@xxxxxxxxx>, moderated list:BROADCOM BCM2711/BCM2835 ARM
> > ARCHITECTURE <linux-rpi-kernel@xxxxxxxxxxxxxxxxxxx>, moderated
> > list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE
> > <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, open list
> > <linux-kernel@xxxxxxxxxxxxxxx>
> >
> >
> > On Mon, Nov 30, 2020 at 04:11:42PM -0500, Jim Quinlan wrote:
> > > Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
> > > by default Broadcom's STB PCIe controller effects an abort.  This simple
> > > handler determines if the PCIe controller was the cause of the abort and if
> > > so, prints out diagnostic info.
> > >
> > > Example output:
> > >   brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
> > >   brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0
> >
> > What does this mean for all the other PCI core code that expects
> > 0xffffffff data returns?  Does it work?  Does it break differently on
> > STB than on other platforms?
> Hi Bjorn,
> 
> Our PCIe HW causes a CPU abort when this happens.  Occasionally a
> customer will have a fault handler try to fix up the abort and
> continue on, but we recommend solving the root problem.  This commit
> just gives us a chance to glean info about the problem.  Our newer
> SOCs have a mode that doesn't abort and instead returns 0xffffffff.
> 
> BTW, can you point me to example files where "PCI core code that
> expects  0xffffffff data returns" [on bad accesses]?

The most important case is during enumeration.  A config read to a
device that doesn't exist normally terminates as an Unsupported
Request, and pci_bus_generic_read_dev_vendor_id() depends on reading
0xffffffff in that case.  I assume this particular case does work that
way for brcm-pcie, because I assume enumeration does work.

pci_cfg_space_size_ext() is similar.  I assume this also works for
brcm-pcie for the same reason.

pci_raw_set_power_state() looks for ~0, which it may see if it does a
config read to a device in D3cold.  pci_dev_wait(), dpc_irq(),
pcie_pme_work_fn(), pcie_pme_irq() are all similar.

Yes, this is ugly and we should check for these more consistently.

The above are all for config reads.  The PCI core doesn't do MMIO
accesses except for a few cases like MSI-X.  But drivers do, and if
they check for PCIe errors on MMIO reads, they do it by looking for
0xffffffff, e.g., pci_mmio_enabled() (in hfi1),
qib_pci_mmio_enabled(), bnx2x_get_hwinfo(), etc.

Bjorn