On Thu, Nov 22, 2018 at 06:47:56PM +0000, Gustavo Pimentel wrote:
> 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?
I missed it since you left an empty line above the Signed-off-by,
apologies.
> > 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?
You can omit the kernel version since it will be inferred from the
Fixes: tag. If this patch does not apply cleanly to v4.19 you will
be asked to backport it.
Please note: the CC tag above does not mean that you should have
stable@xxxxxxxxxxxxxxx in the email CC list for the patch, so please
make sure you do NOT have stable@xxxxxxxxxxxxxxx in the email CC headers
while posting a v2, it will confuse stable maintainers.
Thanks,
Lorenzo
> >>>> 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
> >>>>
> >>
>
