On 2018/12/4 6:57, Bjorn Helgaas wrote:
> 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
>
> .
>
Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from 0xa/PCI_CLASS_DEVICE)
that I didn't considered.
As your suggestion, I consider some code below may help this issue:
1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
request all other *size* bytes will return error and dw_pcie_read()
will also check register alignment.
2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or 0xa/PCI_CLASS_REVISION,
we may need to correct class code.
As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from 0x8/PCI_CLASS_DEVICE
or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
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);
int ret;
ret = dw_pcie_read(pci->dbi_base + where, size, val);
if (ret != PCIBIOS_SUCCESSFUL)
return ret;
/*
* 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 && size == 4)
*val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0xffff);
else if (where == PCI_CLASS_DEVICE && size == 2)
*val = PCI_CLASS_BRIDGE_PCI;
return PCIBIOS_SUCCESSFUL;
}
3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
or pci_setup_device() will get failed:
...
class = pci_class(dev);
dev->revision = class & 0xff;
dev->class = class >> 8; /* upper 3 bytes */
....
switch (dev->hdr_type) { /* header type */
...
case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
if (class != PCI_CLASS_BRIDGE_PCI) /* class must be PCI_CLASS_BRIDGE_PCI */
goto bad;
thanks.
hanjie
