Re: [PATCH v2 04/12] PCI: iproc: Fix exception with multi-function devices

[Bjorn Helgaas]

On Mon, Oct 31, 2016 at 05:38:33PM -0700, Ray Jui wrote:
> During enumeration with multi-function EP devices, access to the
> configuration space of a non-exist function results in an unsupported
> request being returned as expected. By default the PAXB based iProc
> PCIe controller forwards this as an APB error to the host system and
> that causes an exception, which is undesired
> 
> This patch disables this undesired behaviour and lets the kernel PCI
> stack deals with an access to the non-exist function, in which case
> a vendor ID of 0xffff is returned and handled gracefully
> 
> Reported-by: JD Zheng <jiandong.zheng@xxxxxxxxxxxx>
> Signed-off-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
> Reviewed-by: JD Zheng <jiandong.zheng@xxxxxxxxxxxx>
> Reviewed-by: Oza Oza <oza.oza@xxxxxxxxxxxx>
> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
> ---
>  drivers/pci/host/pcie-iproc.c | 58 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/pci/host/pcie-iproc.h |  3 +++
>  2 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index f3b3340..07ec478 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -58,6 +58,9 @@
>  #define PCIE_DL_ACTIVE_SHIFT         2
>  #define PCIE_DL_ACTIVE               BIT(PCIE_DL_ACTIVE_SHIFT)
>  
> +#define APB_ERR_EN_SHIFT             0
> +#define APB_ERR_EN                   BIT(APB_ERR_EN_SHIFT)
> +
>  #define OARR_VALID_SHIFT             0
>  #define OARR_VALID                   BIT(OARR_VALID_SHIFT)
>  #define OARR_SIZE_CFG_SHIFT          1
> @@ -96,6 +99,9 @@ enum iproc_pcie_reg {
>  	/* link status */
>  	IPROC_PCIE_LINK_STATUS,
>  
> +	/* enable APB error for unsupported requests */
> +	IPROC_PCIE_APB_ERR_EN,
> +
>  	/* total number of core registers */
>  	IPROC_PCIE_MAX_NUM_REG,
>  };
> @@ -124,6 +130,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>  	[IPROC_PCIE_OMAP_LO]      = 0xd40,
>  	[IPROC_PCIE_OMAP_HI]      = 0xd44,
>  	[IPROC_PCIE_LINK_STATUS]  = 0xf0c,
> +	[IPROC_PCIE_APB_ERR_EN]   = 0xf40,
>  };
>  
>  /* iProc PCIe PAXC v1 registers */
> @@ -181,6 +188,28 @@ static inline void iproc_pcie_write_reg(struct iproc_pcie *pcie,
>  	writel(val, pcie->base + offset);
>  }
>  
> +/**
> + * APB error forwarding can be disabled during access of configuration
> + * registers of the endpoint device, to prevent unsupported requests
> + * (typically seen during enumeration with multi-function devices) from
> + * triggering a system exception
> + */
> +static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
> +					      bool disable)
> +{
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	u32 val;
> +
> +	if (bus->number && pcie->has_apb_err_disable) {
> +		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_APB_ERR_EN);
> +		if (disable)
> +			val &= ~APB_ERR_EN;
> +		else
> +			val |= APB_ERR_EN;
> +		iproc_pcie_write_reg(pcie, IPROC_PCIE_APB_ERR_EN, val);
> +	}
> +}
> +
>  static inline void iproc_pcie_ob_write(struct iproc_pcie *pcie,
>  				       enum iproc_pcie_reg reg,
>  				       unsigned window, u32 val)
> @@ -244,10 +273,34 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>  		return (pcie->base + offset);
>  }
>  
> +static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	int ret;
> +
> +	iproc_pcie_apb_err_disable(bus, true);
> +	ret = pci_generic_config_read32(bus, devfn, where, size, val);
> +	iproc_pcie_apb_err_disable(bus, false);

I guess this means that any other APB errors (unrelated to this config
access, e.g., a driver doing MMIo to a device at the same time the user is
doing "lspci") that happen to occur during this window will also be
ignored.

> +
> +	return ret;
> +}
> +
> +static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
> +				     int where, int size, u32 val)
> +{
> +	int ret;
> +
> +	iproc_pcie_apb_err_disable(bus, true);
> +	ret = pci_generic_config_write32(bus, devfn, where, size, val);
> +	iproc_pcie_apb_err_disable(bus, false);
> +
> +	return ret;
> +}
> +
>  static struct pci_ops iproc_pcie_ops = {
>  	.map_bus = iproc_pcie_map_cfg_bus,
> -	.read = pci_generic_config_read32,
> -	.write = pci_generic_config_write32,
> +	.read = iproc_pcie_config_read32,
> +	.write = iproc_pcie_config_write32,
>  };
>  
>  static void iproc_pcie_reset(struct iproc_pcie *pcie)
> @@ -485,6 +538,7 @@ static int iproc_pcie_rev_init(struct iproc_pcie *pcie)
>  		break;
>  	case IPROC_PCIE_PAXB:
>  		regs = iproc_pcie_reg_paxb;
> +		pcie->has_apb_err_disable = true;
>  		break;
>  	case IPROC_PCIE_PAXC:
>  		regs = iproc_pcie_reg_paxc;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index 768be05..711dd3a 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -57,6 +57,8 @@ struct iproc_msi;
>   * @phy: optional PHY device that controls the Serdes
>   * @map_irq: function callback to map interrupts
>   * @ep_is_internal: indicates an internal emulated endpoint device is connected
> + * @has_apb_err_disable: indicates the controller can be configured to prevent
> + * unsupported request from being forwarded as an APB bus error
>   * @need_ob_cfg: indicates SW needs to configure the outbound mapping window
>   * @ob: outbound mapping parameters
>   * @msi: MSI data
> @@ -74,6 +76,7 @@ struct iproc_pcie {
>  	struct phy *phy;
>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>  	bool ep_is_internal;
> +	bool has_apb_err_disable;
>  	bool need_ob_cfg;
>  	struct iproc_pcie_ob ob;
>  	struct iproc_msi *msi;
> -- 
> 2.1.4
>