MIME-Version: 1.0
References: <20211110221456.11977-1-jim2101024@gmail.com> <20211110221456.11977-5-jim2101024@gmail.com>
 <YY2sVNEcVmQinbj8@rocinante>
In-Reply-To: <YY2sVNEcVmQinbj8@rocinante>
From:   Jim Quinlan <jim2101024@gmail.com>
Date:   Fri, 12 Nov 2021 13:14:28 -0500
Message-ID: <CANCKTBuor2xa9zCr1zrVPnFrfWe83qL8kR-pihRYn06rRw4xKw@mail.gmail.com>
Subject: Re: [PATCH v8 4/8] PCI/portdrv: Create pcie_is_port_dev() func from
 existing code
To:     =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>
Cc:     linux-pci <linux-pci@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Rob Herring <robh@kernel.org>, Mark Brown <broonie@kernel.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        Jim Quinlan <james.quinlan@broadcom.com>,
        Sean V Kelley <sean.v.kelley@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Qiuxu Zhuo <qiuxu.zhuo@intel.com>,
        Keith Busch <kbusch@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Jim Quinlan <jim2101024@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 11, 2021 at 6:50 PM Krzysztof Wilczy=C5=84ski <kw@linux.com> wr=
ote:
>
> Hi Jim,
>
> [...]
> > +bool pcie_is_port_dev(struct pci_dev *dev)
> > +{
> > +     int type;
> > +
> > +     if (!dev)
> > +             return false;
> > +
> > +     type =3D pci_pcie_type(dev);
> > +
> > +     return pci_is_pcie(dev) &&
> > +             ((type =3D=3D PCI_EXP_TYPE_ROOT_PORT) ||
> > +              (type =3D=3D PCI_EXP_TYPE_UPSTREAM) ||
> > +              (type =3D=3D PCI_EXP_TYPE_DOWNSTREAM) ||
> > +              (type =3D=3D 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 tha=
t
> 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 =3D dev->class;

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

    type =3D pci_pcie_type(dev);

    return ((type =3D=3D PCI_EXP_TYPE_ROOT_PORT) ||
        (type =3D=3D PCI_EXP_TYPE_UPSTREAM) ||
        (type =3D=3D PCI_EXP_TYPE_DOWNSTREAM) ||
        (type =3D=3D 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