Re: [PATCH v8 4/8] PCI/portdrv: Create pcie_is_port_dev() func from existing code

[Jim Quinlan]

On Thu, Nov 11, 2021 at 6:50 PM Krzysztof Wilczyński <kw@xxxxxxxxx> wrote:
>
> Hi Jim,
>
> [...]
> > +bool pcie_is_port_dev(struct pci_dev *dev)
> > +{
> > +     int type;
> > +
> > +     if (!dev)
> > +             return false;
> > +
> > +     type = pci_pcie_type(dev);
> > +
> > +     return pci_is_pcie(dev) &&
> > +             ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> > +              (type == PCI_EXP_TYPE_UPSTREAM) ||
> > +              (type == PCI_EXP_TYPE_DOWNSTREAM) ||
> > +              (type == PCI_EXP_TYPE_RC_EC));
> > +}
> > +EXPORT_SYMBOL_GPL(pcie_is_port_dev);
>
> It would be really nice to document what the above function does (not that
> some of the logic has been extracted from other function).  You know, for
> the future generations of kernel hackers.

Hi Krzysztof and others,

I gave this a second look and realized that the portdrv's
pci_device_id list for the probe is doing filtering that is not
included in the function.  So perhaps the code must be the following
in order to live up to its name:

static inline bool pci_is_port_dev(struct pci_dev *dev)
{
    int type, class;

    if (!dev || !pci_is_pcie(dev))
        return false;

    class = dev->class;

    /* This must be kept in sync with port_pci_ids[] of protdev_pci.c */
    if (!(class == ((PCI_CLASS_BRIDGE_PCI << 8) | 0x00) ||
          class == ((PCI_CLASS_BRIDGE_PCI << 8) | 0x01) ||
          class == ((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00)))
        return false;

    type = pci_pcie_type(dev);

    return ((type == PCI_EXP_TYPE_ROOT_PORT) ||
        (type == PCI_EXP_TYPE_UPSTREAM) ||
        (type == PCI_EXP_TYPE_DOWNSTREAM) ||
        (type == PCI_EXP_TYPE_RC_EC));
}

Kind of large for an inline, plus the code must be kept in sync with
the device list.   Suggestions?

As for a description, my understanding is that the code identifies a
pci_dev that is directly under a host bridge device.  I'm not really
sure about the PCI_CLASS_SYSTEM_RCEC though.

Regards,
Jim Quinlan
Broadcom STB

>
>         Krzysztof