Hi Bjorn and Pratyush, thanks for reviewing > -----Original Message----- > From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci- > owner@xxxxxxxxxxxxxxx] On Behalf Of Pratyush Anand > Sent: Friday, September 04, 2015 7:06 AM > To: Bjorn Helgaas > Cc: Gabriele Paoloni; linux-pci@xxxxxxxxxxxxxxx; Wangzhou (B); Jingoo > Han; Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write > > Hi Bjorn, > > > On Fri, Sep 4, 2015 at 2:29 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > wrote: > > On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote: > >> From: gabriele paoloni <gabriele.paoloni@xxxxxxxxxx> > >> > >> Currently in dw_pcie_cfg_write() if the input size is 2 bytes > >> the address offset is wrongly calculated as we mask also bit0 > >> of "where" input parameter. Instead we should mask all bits of > >> "where" except bit0 and bit1. > > > > I think there are two problems in this area: > > > > 1) Most calls of dw_pcie_cfg_write() are of the form: > > > > dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...) > > dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...) > > > > This is needlessly repetitive because the caller has to mention > > "where" twice and do the masking itself. We could simply pass > > "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the > > required masking internally. > > I think just by passing "pp->dbi_base" and "where" we can not take > care of all cases. For example, if I have to read PCI_CACHE_LINE_SIZE > (8 bit value at offset 0x0C), I will have to pass "pp->dbi_base" + > PCI_CACHE_LINE_SIZE, 1 Say you call dw_pcie_cfg_write(pp->dbi_base, PCI_CACHE_LINE_SIZE, 1, ...) you can have: int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val) { addr += where; if (size == 4) { if (addr & 3) return PCIBIOS_BAD_REGISTER_NUMBER; writel(val, addr); } else if (size == 2) { if (addr & 2) return PCIBIOS_BAD_REGISTER_NUMBER; writew(val, addr + (where & 2)); } else if (size == 1) writeb(val, addr + (where & 3)); else return PCIBIOS_BAD_REGISTER_NUMBER; return PCIBIOS_SUCCESSFUL; } So we can clean all the callers that Bjorn pointed out.... > > So, I think User of this function will need to pass "addr","size" and > "val", where > "addr" as "pp->dbi_base" + "where" > "size" and "val" as it is. > > one more thing IIRC, there was some discussion (however I am unable to > find any reference), and not sure if it is still true with any > platform: issues with non word aligned PCI register read. > If a platform is not able to read non word aligned register correctly, > then in that case we will have to pass where and size both :( > > > > > 2) Pratyush, this one's for you :) spear13xx_pcie_establish_link() > > calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times > like > > this: > > > > dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, > 4, ...) > > dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, > 4, ...) > > > > I think these are incorrect because dw_pcie_cfg_read() and > > dw_pcie_cfg_write() don't add anything to the "addr" argument > > ("pp->dbi_base" in this case) except the 0-3 byte select offset, > so > > they use the correct alignment, but access the wrong registers. > > hummm..so for sure the version which was up-streamed has never been > tested with buggy gen1 card and with any endpoint which would have > generated a read request of more than 128 bytes :( .. Will correct. If you are happy with the solution above I can create a patchset, where the first patch fixes this and the second changes dw_pcie_cfg_write(), dw_pcie_cfg_read and respective function calls... > > Thanks a lot for pointing this out :-) > > ~Pratyush > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
