On Thu, Sep 03, 2020 at 08:37:56PM +0200, Mark Kettenis wrote:
> > Date: Wed, 2 Sep 2020 15:19:55 +1000
> > From: Jonathan Gray <j...@jsg.id.au>
> >
> > On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > > This patch adds a common function for scanning PCIE Express Capability
> > > list
> > > The PCIE Capability list starts at 0x100 in extended PCI configuration
> > > space.
> >
> > This seems to only handle extended capabilities?
> > Something like pcie_get_ext_capability() would be a better name.
>
> I think it should be pci_get_ext_capability() which follows the
> pattern set by pci_get_ht_capability().
>
> >
> > It is 'PCI Express' not 'PCIExpress'
> >
> > 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> > masks those bits.
> >
> > >
> > > ---
> > > sys/dev/pci/pci.c | 28 ++++++++++++++++++++++++++++
> > > sys/dev/pci/pcivar.h | 2 ++
> > > 2 files changed, 30 insertions(+)
> > >
> > > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > > index bf75f875e..8f9a5ef7a 100644
> > > --- a/sys/dev/pci/pci.c
> > > +++ b/sys/dev/pci/pci.c
> > > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t
> > > tag, int capid,
> > > return (0);
> > > }
> > >
> > > +int
> > > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > > + int *offset, pcireg_t *value)
> > > +{
> > > + pcireg_t reg;
> > > + unsigned int ofs;
> > > +
> > > + /* Make sure we support PCIExpress device */
>
> PCI Express like jsg@ already mentioned. Add a full stop at the end
> of the sentence.
>
> > > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
> > > + return (0);
> > > + /* Scan PCIExpress capabilities */
>
> Drop this comment and replace with a blank line such that it matches
> pci_get_ht_capability().
>
> > > + ofs = PCI_PCIE_ECAP;
> > > + while (ofs != 0) {
> > > + if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > > + return (0);
>
> Make this check #ifdef DIAGNOSTIC like pci_get_ht_capability() doesn.
> Dropping the (ofs & 3) bit is indeed a good idea.
>
> > > + reg = pci_conf_read(pc, tag, ofs);
> > > + if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > > + if (offset)
> > > + *offset = ofs;
> > > + if (value)
> > > + *value = reg;
> > > + return (1);
> > > + }
> > > + ofs = PCI_PCIE_ECAP_NEXT(reg);
> > > + }
>
> Blank line here please.
>
> > > + return (0);
> > > +}
> > > +
> > > uint16_t
> > > pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > > {
> > > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > > index bdfe0404f..0376ba992 100644
> > > --- a/sys/dev/pci/pcivar.h
> > > +++ b/sys/dev/pci/pcivar.h
> > > @@ -233,6 +233,8 @@ int pci_io_find(pci_chipset_tag_t, pcitag_t, int,
> > > bus_addr_t *,
> > > int pci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > > bus_size_t *, int *);
> > >
> > > +int pcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > + int *, pcireg_t *);
> > > int pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > int *, pcireg_t *);
> > > int pci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > > --
> > > 2.26.2
> > >
> > >
> >
> >
diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
index 8f9a5ef7a..f649b3e79 100644
--- a/sys/dev/pci/pci.c
+++ b/sys/dev/pci/pci.c
@@ -678,20 +678,22 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t tag,
int capid,
}
int
-pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
+pci_get_ext_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
int *offset, pcireg_t *value)
{
pcireg_t reg;
unsigned int ofs;
- /* Make sure we support PCIExpress device */
+ /* Make sure we support PCI Express device */
if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
return (0);
- /* Scan PCIExpress capabilities */
+ /* Scan PCI Express capabilities */
ofs = PCI_PCIE_ECAP;
while (ofs != 0) {
+#ifdef DIAGNOSTIC
if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
return (0);
+#endif
reg = pci_conf_read(pc, tag, ofs);
if (PCI_PCIE_ECAP_ID(reg) == capid) {
if (offset)
@@ -702,6 +704,7 @@ pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int
capid,
}
ofs = PCI_PCIE_ECAP_NEXT(reg);
}
+
return (0);
}
diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
index 0376ba992..1a19996f5 100644
--- a/sys/dev/pci/pcivar.h
+++ b/sys/dev/pci/pcivar.h
@@ -233,12 +233,12 @@ int pci_io_find(pci_chipset_tag_t, pcitag_t, int,
bus_addr_t *,
int pci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
bus_size_t *, int *);
-int pcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
- int *, pcireg_t *);
int pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
int *, pcireg_t *);
int pci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
int *, pcireg_t *);
+int pci_get_ext_capability(pci_chipset_tag_t, pcitag_t, int,
+ int *, pcireg_t *);
struct msix_vector;
