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.
>
>> 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?
Gustavo
>
> 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
>>
