RE: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root Port driver

[Bharat Kumar Gogada]

> Subject: Re: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root Port
> driver
> 
> [+cc other drivers that use the broken "is link up" test in config access]
> 
> On Fri, Dec 20, 2019 at 05:11:12PM +0530, Bharat Kumar Gogada wrote:
> > - Adding 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.
> > - Versal CPM uses GICv3 ITS feature for 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.
> 
> "Versal" appears to be a brand name and should be capitalized consistently.
> 
> > +#define INTX_NUM                        4
> 
> Don't we have a generic PCI core definition for this?
Thanks Bjorn, will fix this with core definition.
> 
> > +static inline bool cpm_pcie_link_is_up(struct xilinx_cpm_pcie_port
> > +*port)
> 
> Please follow the example of other drivers and name this "cpm_pcie_link_up()".
Agreed, will change the name.
> 
> > +{
> > +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> > +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0; }
> 
> > +/**
> > + * xilinx_cpm_pcie_valid_device - Check if a valid device is present
> > +on bus
> > + * @bus: PCI Bus structure
> > + * @devfn: device/function
> > + *
> > + * Return: 'true' on success and 'false' if invalid device is found
> > +*/ static bool xilinx_cpm_pcie_valid_device(struct pci_bus *bus,
> > +					 unsigned int devfn)
> > +{
> > +	struct xilinx_cpm_pcie_port *port = bus->sysdata;
> > +
> > +	/* Check if link is up when trying to access downstream ports */
> > +	if (bus->number != port->root_busno)
> > +		if (!cpm_pcie_link_is_up(port))
> > +			return false;
> 
> This check for the link being up is racy and should be removed.  The link may go
> down after the check and before the config access.
> 
> A config access to a device where the link is down is an error, but it's an error we
> expect to happen because of enumeration, surprise unplug, or electrical issue.
> It's impossible to avoid so we must be able to handle and recover from it.
> 
> I know there are other drivers that do this (dwc, altera, xilinix-nwl, xilinx), and
> I've pointed this out many times in the past.  This needs to be fixed.
Agreed, will fix this check.
> 
> > +
> > +	/* Only one device down on each root port */
> > +	if (bus->number == port->root_busno && devfn > 0)
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> > +{
> > +	struct xilinx_cpm_pcie_port *port =
> > +				(struct xilinx_cpm_pcie_port *)data;
> 
>   struct device *dev = port->dev;
> 
> to reduce repetition of "port->dev" below.
Agreed, will fix this.

> > +	u32 val, mask, status, bit;
> > +	unsigned long intr_val;
> > +
> > +	/* Read interrupt decode and mask registers */
> > +	val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> > +	mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> > +
> > +	status = val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> > +		dev_warn(port->dev, "Link Down\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> > +		dev_info(port->dev, "Hot reset\n");
> > ...