diff mbox

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

Message ID 1477960721-17649-5-git-send-email-ray.jui@broadcom.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Ray Jui Nov. 1, 2016, 12:38 a.m. UTC
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@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: JD Zheng <jiandong.zheng@broadcom.com>
Reviewed-by: Oza Oza <oza.oza@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pci/host/pcie-iproc.c | 58 +++++++++++++++++++++++++++++++++++++++++--
 drivers/pci/host/pcie-iproc.h |  3 +++
 2 files changed, 59 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Nov. 14, 2016, 9:52 p.m. UTC | #1
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@broadcom.com>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: JD Zheng <jiandong.zheng@broadcom.com>
> Reviewed-by: Oza Oza <oza.oza@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui Nov. 15, 2016, 3:39 a.m. UTC | #2
Hi Bjorn,

On 11/14/2016 1:52 PM, Bjorn Helgaas wrote:
> On Mon, Oct 31, 2016 at 05:38:33PM -0700, Ray Jui wrote:

[...]

>>  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

[...]

>> +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.
> 

I believe this only disables APB errors caused by unsupported request
generated from PCIe.

Thanks,

Ray
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
+
+	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;