Re: [PATCH v4 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver

[Bjorn Helgaas]

On Tue, Jan 28, 2020 at 06:14:43PM +0530, Bharat Kumar Gogada wrote:
> - Add support for Versal CPM as Root Port.
> - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
>   block for CPM along with the integrated bridge can function
>   as PCIe Root Port.
> - CPM Versal uses GICv3 ITS feature for achieving assigning MSI/MSI-X
>   vectors and handling MSI/MSI-X interrupts.
> - Bridge error and legacy interrupts in Versal CPM are handled using
>   Versal CPM specific MISC interrupt line.
> 
> Changes v4:
> - change commit subject.
> - Remove unnecessary comments and type cast.
> - Added comments for CPM block register access using readl/writel.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> ...

> +static bool xilinx_cpm_pcie_valid_device(struct pci_bus *bus,
> +					 unsigned int devfn)
> +{
> +	struct xilinx_cpm_pcie_port *port = bus->sysdata;
> +
> +	/* Only one device down on each root port */
> +	if (bus->number == port->root_busno && devfn > 0)
> +		return false;

This whole *_valid_device() thing is a mess.  We shouldn't need it at
all.  But if we *do* need it, I don't think you should check the
entire devfn because that means you can't attach a multifunction
device.

Several other drivers with similar *_valid_device() implementations
check only PCI_SLOT():

  dw_pcie_valid_device()
  advk_pcie_valid_device()
  pci_dw_valid_device()
  altera_pcie_valid_device()
  mobiveil_pcie_valid_device()
  rockchip_pcie_valid_device()

Even checking just PCI_SLOT() is problematic because I think an ARI
device with more than 8 functions will not work correctly.

What exactly happens if you omit this function, i.e., if we just go
ahead and attempt config accesses when the device is not present?  We
*should* get something like an Unsupported Request completion, and
that *should* be a recoverable error.  Most hardware turns this error
into read data of 0xffffffff.  The OS should be able to figure out
that there's no device there and continue with no ill effects.

> +	return true;
> +}
> +
> +/**
> + * xilinx_cpm_pcie_map_bus - Get configuration base
> + * @bus: PCI Bus structure
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + *	   accessed.
> + */
> +static void __iomem *xilinx_cpm_pcie_map_bus(struct pci_bus *bus,
> +					     unsigned int devfn, int where)
> +{
> +	struct xilinx_cpm_pcie_port *port = bus->sysdata;
> +	int relbus;
> +
> +	if (!xilinx_cpm_pcie_valid_device(bus, devfn))
> +		return NULL;
> +
> +	relbus = (bus->number << ECAM_BUS_NUM_SHIFT) |
> +		 (devfn << ECAM_DEV_NUM_SHIFT);
> +
> +	return port->reg_base + relbus + where;
> +}
> +
> +/* PCIe operations */
> +static struct pci_ops xilinx_cpm_pcie_ops = {
> +	.map_bus = xilinx_cpm_pcie_map_bus,
> +	.read	= pci_generic_config_read,
> +	.write	= pci_generic_config_write,
> +};