Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Bjorn Helgaas]

On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> 
> [...]
> 
> > +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> > +				  u32 *val)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	/*
> > +	 * there is a bug of MESON AXG pcie controller that software can not
> > +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> > +	 * value to ensure driver could probe successfully.
> > +	 */
> > +	if (where == PCI_CLASS_REVISION) {
> > +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> > +		/* keep revision id */
> > +		*val &= PCI_CLASS_REVISION_MASK;
> > +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> > +		return PCIBIOS_SUCCESSFUL;
> > +	}
> 
> As I said before, this looks broken. If this code (or other drivers with
> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> it according to your comment above.
> 
> I would like to pick Bjorn's brain on this to see what we can really do
> to fix this (and other) drivers.

  - Check to see whether you're reading anything in the 32-bit dword at
    offset 0x08.

  - Do the 32-bit readl().

  - Insert the correct Sub-Class and Base Class code (you also throw
    away the Programming Interface; not sure why that is)

  - If you're reading something smaller than 32 bits, mask & shift as
    needed.  pci_bridge_emul_conf_read() does something similar that
    you might be able to copy.

Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
are several places in the kernel that currently depend on it, but I
think several of them *should* be checking dev->hdr_type to identify a
type 1 header instead.

Bjorn