Re: [PATCH v9 2/9] PCI: Using PCI configuration space header type instead of class type to assign resource
From: Honghui Zhang
Date: Wed Oct 17 2018 - 21:25:41 EST
On Wed, 2018-10-17 at 08:22 -0500, Bjorn Helgaas wrote:
> On Tue, Oct 16, 2018 at 03:53:55PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Oct 16, 2018 at 06:44:43PM +0800, honghui.zhang@xxxxxxxxxxxx wrote:
> > > From: Honghui Zhang <honghui.zhang@xxxxxxxxxxxx>
> > >
> > > The PCI configuration space header type defines the layout of the rest
> > > of the header (PCI r3.0 sec 6.1, PCIe r4.0 sec 7.5.1.1.9) while the
> > > resource assignment is based on the configuration space layout instead
> > > of its class type. Using configuration space header type instead of
> > > class type for the resource assignment.
> > >
> > > Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > Signed-off-by: Honghui Zhang <honghui.zhang@xxxxxxxxxxxx>
> > > ---
> > > drivers/pci/pci.c | 3 +--
> > > drivers/pci/probe.c | 3 ---
> > > drivers/pci/setup-bus.c | 20 ++++++++++----------
> > > 3 files changed, 11 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 29ff961..7d379ca 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5908,8 +5908,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> > > * to enable the kernel to reassign new resource
> > > * window later on.
> > > */
> > > - if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> > > - (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> > > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > for (i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++) {
> > > r = &dev->resource[i];
> > > if (!(r->flags & IORESOURCE_MEM))
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ec78400..29a35c1 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1695,9 +1695,6 @@ int pci_setup_device(struct pci_dev *dev)
> > > break;
> > >
> > > case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
> > > - if (class != PCI_CLASS_BRIDGE_PCI)
> > > - goto bad;
> > > -
> > > /*
> > > * The PCI-to-PCI bridge spec requires that subtractive
> > > * decoding (i.e. transparent) bridge must have programming
> > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > > index 79b1824..69f90f4 100644
> > > --- a/drivers/pci/setup-bus.c
> > > +++ b/drivers/pci/setup-bus.c
> > > @@ -182,7 +182,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
> > > u16 class = dev->class >> 8;
> > >
> > > /* Don't touch classless devices or host bridges or ioapics. */
> > > - if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
> > > + if (class == PCI_CLASS_NOT_DEFINED)
> >
> > I think this check has been there since the first initial git commit,
> > whether that's _really_ needed or not in the current kernel it is very
> > hard to say.
> >
> > I am not that sure it is safe to remove it, especially given that we are at
> > -rc8 and close to a release, it would be good if this patch could sit in
> > next to give it some exposure to testing before merging it upstream.
>
I'm not sure why the first version has take care of
PCI_CLASS_BRIDGE_HOST separately, I have no idea whether there's some
host bridge device that has their resource fixed.
I agree that this patch should be hold for the moment since I have it
tested only in Mediatek's platform.
Thanks
> Yes, you're right; I think I think this is a little too risky at this
> point. I'll pull this patch out and queue it up for the next cycle
> (v4.21).
Thanks, I will follow up this patch.
>
> For v4.20, I think you should resurrect the class code patch [1]. That
> should be enough to make things work in v4.20, even without this hdr_type
> patch. It will also improve the lspci output, because I think it uses the
> class code to look up the generic description, e.g., in this output:
>
> 00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port (rev f1)
>
> I think the "PCI bridge" part is based on the class code.
>
> Bjorn
>
> [1] https://lore.kernel.org/linux-pci/1539590940-13355-3-git-send-email-honghui.zhang@xxxxxxxxxxxx
>
> > > return;
> > >
> > > /* Don't touch ioapic devices already enabled by firmware */
> > > @@ -1221,12 +1221,12 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
> > > if (!b)
> > > continue;
> > >
> > > - switch (dev->class >> 8) {
> > > - case PCI_CLASS_BRIDGE_CARDBUS:
> > > + switch (dev->hdr_type) {
> > > + case PCI_HEADER_TYPE_CARDBUS:
> > > pci_bus_size_cardbus(b, realloc_head);
> > > break;
> > >
> > > - case PCI_CLASS_BRIDGE_PCI:
> > > + case PCI_HEADER_TYPE_BRIDGE:
> > > default:
> > > __pci_bus_size_bridges(b, realloc_head);
> > > break;
> > > @@ -1237,12 +1237,12 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
> > > if (pci_is_root_bus(bus))
> > > return;
> > >
> > > - switch (bus->self->class >> 8) {
> > > - case PCI_CLASS_BRIDGE_CARDBUS:
> > > + switch (bus->self->hdr_type) {
> > > + case PCI_HEADER_TYPE_CARDBUS:
> > > /* don't size cardbuses yet. */
> > > break;
> > >
> > > - case PCI_CLASS_BRIDGE_PCI:
> > > + case PCI_HEADER_TYPE_BRIDGE:
> > > pci_bridge_check_ranges(bus);
> > > if (bus->self->is_hotplug_bridge) {
> > > additional_io_size = pci_hotplug_io_size;
> > > @@ -1391,13 +1391,13 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
> > >
> > > __pci_bus_assign_resources(b, realloc_head, fail_head);
> > >
> > > - switch (dev->class >> 8) {
> > > - case PCI_CLASS_BRIDGE_PCI:
> > > + switch (dev->hdr_type) {
> > > + case PCI_HEADER_TYPE_BRIDGE:
> > > if (!pci_is_enabled(dev))
> > > pci_setup_bridge(b);
> > > break;
> > >
> > > - case PCI_CLASS_BRIDGE_CARDBUS:
> > > + case PCI_HEADER_TYPE_CARDBUS:
> > > pci_setup_cardbus(b);
> > > break;
> > >
> > > --
> > > 2.6.4
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel