| Message ID | 20190114132424.6445-19-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 |
Hi Kishon, On 14/01/2019 13:24, 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. > > 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(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index d5144781005b..cd51b008858c 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; > + Supposedly this was already verified by the function that calls this one. > 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; This fix seems to be a bit overdone, especially when you only need to swap the if blocks order to achieve the desired goal. > 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; > - Why remove it? If pointer is null, why to jump to another function to check is the the same pointer is null? > return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap); > } > >
Hi Gustavo, On 29/01/19 2:55 PM, Gustavo Pimentel wrote: > Hi Kishon, > > On 14/01/2019 13:24, 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. >> >> 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(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c >> index d5144781005b..cd51b008858c 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; >> + > > Supposedly this was already verified by the function that calls this one. Right, with with this fix cap_ptr is checked only once. This being a recursive function, it makes sense to have the check only here instead of once in the calling function and once here. > >> 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; > > This fix seems to be a bit overdone, especially when you only need to swap the > if blocks order to achieve the desired goal. No, cap_id > PCI_CAP_ID_MAX is a base error case and it should checked before returning the offset IMO. > >> 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; >> - > > Why remove it? > If pointer is null, why to jump to another function to check is the the same > pointer is null? so that we check cap_ptr only once. Thanks Kishon
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index d5144781005b..cd51b008858c 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); }
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(-)