Re: [PATCH v21 3/4] PCI: microchip: Add host driver for Microchip PCIe controller

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Marc]

On Mon, Jan 25, 2021 at 04:29:33PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Add support for the Microchip PolarFire PCIe controller when
> configured in host (Root Complex) mode.

> +static void mc_handle_msi(struct irq_desc *desc)
> +{
> +	struct mc_port *port = irq_desc_get_handler_data(desc);
> +	struct device *dev = port->dev;
> +	struct mc_msi *msi = &port->msi;
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	unsigned long status;
> +	u32 bit;
> +	u32 virq;
> +
> +	status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> +	if (status & PM_MSI_INT_MSI_MASK) {
> +		status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> +		for_each_set_bit(bit, &status, msi->num_vectors) {
> +			virq = irq_find_mapping(msi->dev_domain, bit);
> +			if (virq)
> +				generic_handle_irq(virq);
> +			else
> +				dev_err_ratelimited(dev, "bad MSI IRQ %d\n", bit);
> +		}
> +	}
> +}
> +
> +static void mc_msi_bottom_irq_ack(struct irq_data *data)
> +{
> +	struct mc_port *port = irq_data_get_irq_chip_data(data);
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	u32 bitpos = data->hwirq;
> +	unsigned long status;
> +
> +	writel_relaxed(BIT(bitpos), bridge_base_addr + ISTATUS_MSI);
> +	status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> +	if (!status)
> +		writel_relaxed(BIT(PM_MSI_INT_MSI_SHIFT), bridge_base_addr + ISTATUS_LOCAL);

This looks like it might be racy.  What happens if we read 0 from
ISTATUS_MSI, but a new MSI is latched before we write ISTATUS_LOCAL?

Will mc_handle_msi() be called?  If so, will PM_MSI_INT_MSI_MASK be
set in the value it reads from ISTATUS_LOCAL?

Current code at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-microchip-host.c?id=v5.17-rc1#n406