> Date: Mon, 7 Sep 2020 13:33:14 -0500 > From: Jordan Hargrave <jordan_hargr...@hotmail.com> > > Attaching the full diff
ok kettenis@ > On Mon, Sep 07, 2020 at 01:09:12PM -0500, Jordan Hargrave wrote: > > 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; > > > > > >diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c > index bf75f875e..4e8d06389 100644 > --- a/sys/dev/pci/pci.c > +++ b/sys/dev/pci/pci.c > @@ -677,6 +677,38 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t > tag, int capid, > return (0); > } > > +int > +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 PCI Express device */ > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0) > + return (0); > + > + /* 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) > + *offset = ofs; > + if (value) > + *value = reg; > + return (1); > + } > + ofs = PCI_PCIE_ECAP_NEXT(reg); > + } > + > + 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..1a19996f5 100644 > --- a/sys/dev/pci/pcivar.h > +++ b/sys/dev/pci/pcivar.h > @@ -237,6 +237,8 @@ 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; > >