On Thu, Nov 22, 2018 at 05:31:06PM +0000, Gustavo Pimentel wrote:
> Hi Lorenzo,
>
> On 22/11/2018 17:08, Lorenzo Pieralisi wrote:
> > On Thu, Nov 22, 2018 at 05:24:28PM +0100, Gustavo Pimentel wrote:
> >> Remove 3-bit right rotation on MSI-X table offset address calculation on
> >> dw_pcie_ep_raise_msix_irq().
> >>
> >> By default, the offset address of the MSI-X table is zero, so that even with a
> >> 3-bit right rotation, the computed result would still be zero, so still valid.
> >>
> >> Fixes: beb4641a787d ("PCI: dwc: Add MSI-X callbacks handler")
> >>
> >> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
> >> ---
> >> drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> index 1e7b022..cdb2005 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> >> @@ -440,7 +440,6 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> >> tbl_offset = dw_pcie_readl_dbi(pci, reg);
> >> bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> >> tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> >> - tbl_offset >>= 3;
> >
> > This looks wrong.>
> > - If tlb_offset is always 0 there would no point reading it (but I think
> > that's a completely wrong assumption)
> > - If tlb_offset can be != 0 you are introducing a bug
>
> That is the bug. I misunderstood the spec and I though that was
> required to perform 3-bit right rotation. Since my setup during the
> developing had the MSI-X table offset set to zero, this bug passed
> undetected. Now that my current setup has a MSI-X table offset
> different from zero I was able to detect this bug.
Correct, now I got it. You should reword the commit log since it is
not really clear that you are fixing a bug and add a Fixes: tag please,
this is not a minor bug and should go to stable kernels.
> >> reg = PCI_BASE_ADDRESS_0 + (4 * bir);
> >> bar_addr_upper = 0;
> >> @@ -466,8 +465,10 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>
> >> iounmap(msix_tbl);
> >>
> >> - if (vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> >> + if (vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT) {
> >> + dev_err(pci->dev, "MSI-X entry ctrl set\n");
> >
> > Unrelated change.
>
> Ups, that's right. It's just a debug message in case of error.
> Do you prefer a patch version 2 with the description including this debug
> message or do you prefer a second patch only with this debug message?
One patch one logical change. So I expect this patch to be split into
two (more so because this is a significant fix).
Thanks,
Lorenzo
> >> return -EPERM;
> >> + }
> >>
> >> ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr,
> >> epc->mem->page_size);
> >> --
> >> 2.7.4
> >>
>
