On 22/11/2018 17:48, Lorenzo Pieralisi wrote:
> 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,
I'll reword the description and put it more clear.
There is already a Fixes with tag on the description, have you missed?
> this is not a minor bug and should go to stable kernels.
How can I do that? Should I put this on the patch?
Cc: stable@xxxxxxxxxxxxxxx # 4.19
and send to the mailing list as well?
>
>>>> 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).
Ok.
Regards,
Gustavo
>
> 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
>>>>
>>
