diff mbox series

[v3,20/26] PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability offset

Message ID 20190325093947.32633-21-kishon@ti.com (mailing list archive)
State New, archived
Headers show
Series Add support for PCIe RC and EP mode in TI's AM654 SoC | expand

Commit Message

Kishon Vijay Abraham I March 25, 2019, 9:39 a.m. UTC
commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
__dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.

However if MSI or MSIX capability is the last capability (i.e there are
no additional items in the capabilities list and the Next Capability
Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
even though MSI or MSIX capability may be present. This is because of
incorrect ordering of "next_cap_ptr" check. Fix it here.

Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas April 13, 2019, 3:44 p.m. UTC | #1
On Mon, Mar 25, 2019 at 03:09:41PM +0530, Kishon Vijay Abraham I wrote:
> commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
> adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
> __dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.
> 
> However if MSI or MSIX capability is the last capability (i.e there are
> no additional items in the capabilities list and the Next Capability
> Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
> even though MSI or MSIX capability may be present. This is because of
> incorrect ordering of "next_cap_ptr" check. Fix it here.

Nice fix.  It looks like dw_pcie_ep_find_capability() is currently
only used for MSI and MSI-X, but IIUC, it *could* be used for any
capability, so this commit message seems a little too specific.

It would be slightly less confusing if beb4641a787d ("PCI: dwc: Add
MSI-X callbacks handler") had been split into a commit that added the
general-purpose dw_pcie_ep_find_capability() and a commit that used it
for MSI and MSI-X, but obviously we can't change history.

> Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")

These commit references are longer than necessary (and different -
19-char SHA1 here and 16 chars above).  I typically use 12 chars via

  gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'


> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index dc6a4bbd3ace..74477ad7467f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -46,16 +46,19 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>  	u8 cap_id, next_cap_ptr;
>  	u16 reg;
>  
> +	if (!cap_ptr)
> +		return 0;
> +
>  	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> -	next_cap_ptr = (reg & 0xff00) >> 8;
>  	cap_id = (reg & 0x00ff);
>  
> -	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> +	if (cap_id > PCI_CAP_ID_MAX)
>  		return 0;
>  
>  	if (cap_id == cap)
>  		return cap_ptr;
>  
> +	next_cap_ptr = (reg & 0xff00) >> 8;
>  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>  }
>  
> @@ -67,9 +70,6 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
>  	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>  	next_cap_ptr = (reg & 0x00ff);
>  
> -	if (!next_cap_ptr)
> -		return 0;
> -
>  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>  }
>  
> -- 
> 2.17.1
>
Lorenzo Pieralisi April 16, 2019, 2:11 p.m. UTC | #2
On Sat, Apr 13, 2019 at 10:44:37AM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 25, 2019 at 03:09:41PM +0530, Kishon Vijay Abraham I wrote:
> > commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
> > adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
> > __dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.
> > 
> > However if MSI or MSIX capability is the last capability (i.e there are
> > no additional items in the capabilities list and the Next Capability
> > Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
> > even though MSI or MSIX capability may be present. This is because of
> > incorrect ordering of "next_cap_ptr" check. Fix it here.
> 
> Nice fix.  It looks like dw_pcie_ep_find_capability() is currently
> only used for MSI and MSI-X, but IIUC, it *could* be used for any
> capability, so this commit message seems a little too specific.
> 
> It would be slightly less confusing if beb4641a787d ("PCI: dwc: Add
> MSI-X callbacks handler") had been split into a commit that added the
> general-purpose dw_pcie_ep_find_capability() and a commit that used it
> for MSI and MSI-X, but obviously we can't change history.
> 
> > Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")
> 
> These commit references are longer than necessary (and different -
> 19-char SHA1 here and 16 chars above).  I typically use 12 chars via
> 
>   gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'

Fixed while queueing it.

Thanks,
Lorenzo

> > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index dc6a4bbd3ace..74477ad7467f 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -46,16 +46,19 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> >  	u8 cap_id, next_cap_ptr;
> >  	u16 reg;
> >  
> > +	if (!cap_ptr)
> > +		return 0;
> > +
> >  	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> > -	next_cap_ptr = (reg & 0xff00) >> 8;
> >  	cap_id = (reg & 0x00ff);
> >  
> > -	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> > +	if (cap_id > PCI_CAP_ID_MAX)
> >  		return 0;
> >  
> >  	if (cap_id == cap)
> >  		return cap_ptr;
> >  
> > +	next_cap_ptr = (reg & 0xff00) >> 8;
> >  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> >  }
> >  
> > @@ -67,9 +70,6 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> >  	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> >  	next_cap_ptr = (reg & 0x00ff);
> >  
> > -	if (!next_cap_ptr)
> > -		return 0;
> > -
> >  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> >  }
> >  
> > -- 
> > 2.17.1
> >
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index dc6a4bbd3ace..74477ad7467f 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -46,16 +46,19 @@  static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
 	u8 cap_id, next_cap_ptr;
 	u16 reg;
 
+	if (!cap_ptr)
+		return 0;
+
 	reg = dw_pcie_readw_dbi(pci, cap_ptr);
-	next_cap_ptr = (reg & 0xff00) >> 8;
 	cap_id = (reg & 0x00ff);
 
-	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
+	if (cap_id > PCI_CAP_ID_MAX)
 		return 0;
 
 	if (cap_id == cap)
 		return cap_ptr;
 
+	next_cap_ptr = (reg & 0xff00) >> 8;
 	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
 }
 
@@ -67,9 +70,6 @@  static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
 	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
 	next_cap_ptr = (reg & 0x00ff);
 
-	if (!next_cap_ptr)
-		return 0;
-
 	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
 }