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

The function will be needed elsewhere in a few commits.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/pci.h              |  2 ++
 drivers/pci/pcie/portdrv_pci.c | 23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

On 11/10/21 2:14 PM, Jim Quinlan wrote:
> The function will be needed elsewhere in a few commits.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/pci.h              |  2 ++
>  drivers/pci/pcie/portdrv_pci.c | 23 ++++++++++++++++++-----
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 1cce56c2aea0..c2bd1995d3a9 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -744,4 +744,6 @@ extern const struct attribute_group aspm_ctrl_attr_group;
>  
>  extern const struct attribute_group pci_dev_reset_method_attr_group;
>  
> +bool pcie_is_port_dev(struct pci_dev *dev);

Looks like you need an inline stub here when CONFIG_PCIEPORTBUS is
disabled to avoid the linking failure reported by the kbuild robot:

static inline bool pcie_is_port_dev(struct pci_dev *dev)
{
	return false;
}

Thanks!
--
Florian

On Thu, Nov 11, 2021 at 3:51 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 11/10/21 2:14 PM, Jim Quinlan wrote:
> > The function will be needed elsewhere in a few commits.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/pci.h              |  2 ++
> >  drivers/pci/pcie/portdrv_pci.c | 23 ++++++++++++++++++-----
> >  2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 1cce56c2aea0..c2bd1995d3a9 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -744,4 +744,6 @@ extern const struct attribute_group aspm_ctrl_attr_group;
> >
> >  extern const struct attribute_group pci_dev_reset_method_attr_group;
> >
> > +bool pcie_is_port_dev(struct pci_dev *dev);
>
> Looks like you need an inline stub here when CONFIG_PCIEPORTBUS is
> disabled to avoid the linking failure reported by the kbuild robot:

Probably always an inline function. It has nothing to do with the
driver, so portdrv_pci.c is not the right place.

Rob

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.

	Krzysztof

On Thu, Nov 11, 2021 at 6:50 PM Krzysztof Wilczyński <kw@linux.com> 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