Patchwork PCI: dwc: Remove 3-bit right rotation on MSI-X table offset address calculation

login
register
mail settings
Submitter Gustavo Pimentel
Date Nov. 22, 2018, 4:24 p.m.
Message ID <a9c24a70e23dbc5a2d0300452588dc6a0b5e218a.1542903868.git.gustavo.pimentel@synopsys.com>
Download mbox | patch
Permalink /patch/662941/
State New
Headers show

Comments

Gustavo Pimentel - Nov. 22, 2018, 4:24 p.m.
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(-)
Lorenzo Pieralisi - Nov. 22, 2018, 5:08 p.m.
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
>
Gustavo Pimentel - Nov. 22, 2018, 5:31 p.m.
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
>>
Lorenzo Pieralisi - Nov. 22, 2018, 5:48 p.m.
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
> >>
>
Gustavo Pimentel - Nov. 22, 2018, 6:47 p.m.
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
>>>>
>>
Lorenzo Pieralisi - Nov. 23, 2018, 11:01 a.m.
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
> >>>>
> >>
>

Patch

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);