| Message ID | a9c24a70e23dbc5a2d0300452588dc6a0b5e218a.1542903868.git.gustavo.pimentel@synopsys.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | PCI: dwc: Remove 3-bit right rotation on MSI-X table offset address calculation | expand |
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@synopsys.com> > --- > 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 > 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. 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 >
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@synopsys.com> >> --- >> 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 >>
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@synopsys.com> > >> --- > >> 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 > >> >
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@synopsys.com> >>>> --- >>>> 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@vger.kernel.org # 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 >>>> >>
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@synopsys.com> > >>>> --- > >>>> 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@vger.kernel.org # 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@vger.kernel.org in the email CC list for the patch, so please make sure you do NOT have stable@vger.kernel.org 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 > >>>> > >> >
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; 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"); return -EPERM; + } ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr, epc->mem->page_size);
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@synopsys.com> --- drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)