Message-ID: <20220801135457.GN93763@thinkpad>
Date:   Mon, 1 Aug 2022 19:24:57 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>
Cc:     Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Rahul Tanwar <rtanwar@...linear.com>,
        Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Frank Li <Frank.Li@....com>, Rob Herring <robh+dt@...nel.org>,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v4 14/15] PCI: dwc: Check iATU in/outbound ranges
 setup methods status

On Fri, Jun 24, 2022 at 05:39:46PM +0300, Serge Semin wrote:
> Let's make the DWC PCIe RC/EP safer and more verbose for the invalid or
> failed inbound and outbound iATU windows setups. Needless to say that
> silently ignoring iATU regions setup errors may cause unpredictable
> errors. For instance if for some reason a cfg or IO window fails to be
> activated, then any CFG/IO requested won't reach target PCIe devices and
> the corresponding accessors will return platform-specific random values.
> 
> First of all we need to convert dw_pcie_ep_outbound_atu() method to check
> whether the specified outbound iATU range is successfully setup. That
> method is called by the pci_epc_ops.map_addr callback. Thus we'll make the
> EP-specific CPU->PCIe memory mappings saver.
> 
> Secondly since the iATU outbound range programming method now returns the
> operation status, it will be handy to take that status into account in the
> pci_ops.{map_bus,read,write} methods. Thus any failed mapping will be
> immediately noticeable by the PCIe CFG operations requesters.
> 
> Finally we need to convert the dw_pcie_setup_rc() method to returning the
> operation status, since the iATU outbound ranges setup procedure may now
> fail. It will be especially handy in case if the DW PCIe RC DT-node has
> invalid/unsupported (dma-)ranges property. Note since the suggested
> modification causes having too wide code indentation, it is reasonable
> from maintainability and readability points of view to move the outbound
> ranges setup procedure in the separate function.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>

Thanks,
Mani

> Reviewed-by: Rob Herring <robh@...nel.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |   9 +-
>  .../pci/controller/dwc/pcie-designware-host.c | 153 ++++++++++++------
>  drivers/pci/controller/dwc/pcie-designware.h  |   5 +-
>  drivers/pci/controller/dwc/pcie-intel-gw.c    |   6 +-
>  4 files changed, 114 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2e91222f7c98..627c4b69878c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -184,8 +184,9 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
>  				   phys_addr_t phys_addr,
>  				   u64 pci_addr, size_t size)
>  {
> -	u32 free_win;
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	u32 free_win;
> +	int ret;
>  
>  	free_win = find_first_zero_bit(ep->ob_window_map, pci->num_ob_windows);
>  	if (free_win >= pci->num_ob_windows) {
> @@ -193,8 +194,10 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
>  		return -EINVAL;
>  	}
>  
> -	dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> -				     phys_addr, pci_addr, size);
> +	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> +					   phys_addr, pci_addr, size);
> +	if (ret)
> +		return ret;
>  
>  	set_bit(free_win, ep->ob_window_map);
>  	ep->outbound_addr[free_win] = phys_addr;
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index e0a2819608c6..6993ce9e856d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -412,7 +412,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	dw_pcie_iatu_detect(pci);
>  
> -	dw_pcie_setup_rc(pp);
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto err_free_msi;
>  
>  	if (!dw_pcie_link_up(pci)) {
>  		ret = dw_pcie_start_link(pci);
> @@ -466,10 +468,10 @@ EXPORT_SYMBOL_GPL(dw_pcie_host_deinit);
>  static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>  						unsigned int devfn, int where)
>  {
> -	int type;
> -	u32 busdev;
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	int type, ret;
> +	u32 busdev;
>  
>  	/*
>  	 * Checking whether the link is up here is a last line of defense
> @@ -490,8 +492,10 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>  	else
>  		type = PCIE_ATU_TYPE_CFG1;
>  
> -
> -	dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev, pp->cfg0_size);
> +	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> +					pp->cfg0_size);
> +	if (ret)
> +		return NULL;
>  
>  	return pp->va_cfg0_base + where;
>  }
> @@ -499,33 +503,45 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>  static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
>  				 int where, int size, u32 *val)
>  {
> -	int ret;
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	int ret;
>  
>  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> +	if (ret != PCIBIOS_SUCCESSFUL)
> +		return ret;
>  
> -	if (!ret && pp->cfg0_io_shared)
> -		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO, pp->io_base,
> -					  pp->io_bus_addr, pp->io_size);
> +	if (pp->cfg0_io_shared) {
> +		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> +						pp->io_base, pp->io_bus_addr,
> +						pp->io_size);
> +		if (ret)
> +			return PCIBIOS_SET_FAILED;
> +	}
>  
> -	return ret;
> +	return PCIBIOS_SUCCESSFUL;
>  }
>  
>  static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
>  				 int where, int size, u32 val)
>  {
> -	int ret;
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	int ret;
>  
>  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> +	if (ret != PCIBIOS_SUCCESSFUL)
> +		return ret;
>  
> -	if (!ret && pp->cfg0_io_shared)
> -		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO, pp->io_base,
> -					  pp->io_bus_addr, pp->io_size);
> +	if (pp->cfg0_io_shared) {
> +		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> +						pp->io_base, pp->io_bus_addr,
> +						pp->io_size);
> +		if (ret)
> +			return PCIBIOS_SET_FAILED;
> +	}
>  
> -	return ret;
> +	return PCIBIOS_SUCCESSFUL;
>  }
>  
>  static struct pci_ops dw_child_pcie_ops = {
> @@ -552,10 +568,72 @@ static struct pci_ops dw_pcie_ops = {
>  	.write = pci_generic_config_write,
>  };
>  
> -void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> +static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  {
> -	u32 val, ctrl, num_ctrls;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct resource_entry *entry;
> +	int i, ret;
> +
> +	/* Note the very first outbound ATU is used for CFG IOs */
> +	if (!pci->num_ob_windows) {
> +		dev_err(pci->dev, "No outbound iATU found\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Ensure all outbound windows are disabled before proceeding with
> +	 * the MEM/IO ranges setups.
> +	 */
> +	for (i = 0; i < pci->num_ob_windows; i++)
> +		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
> +
> +	i = 0;
> +	resource_list_for_each_entry(entry, &pp->bridge->windows) {
> +		if (resource_type(entry->res) != IORESOURCE_MEM)
> +			continue;
> +
> +		if (pci->num_ob_windows <= ++i)
> +			break;
> +
> +		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> +						entry->res->start,
> +						entry->res->start - entry->offset,
> +						resource_size(entry->res));
> +		if (ret) {
> +			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> +				entry->res);
> +			return ret;
> +		}
> +	}
> +
> +	if (pp->io_size) {
> +		if (pci->num_ob_windows > ++i) {
> +			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> +							pp->io_base,
> +							pp->io_bus_addr,
> +							pp->io_size);
> +			if (ret) {
> +				dev_err(pci->dev, "Failed to set IO range %pr\n",
> +					entry->res);
> +				return ret;
> +			}
> +		} else {
> +			pp->cfg0_io_shared = true;
> +		}
> +	}
> +
> +	if (pci->num_ob_windows <= i)
> +		dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
> +			 pci->num_ob_windows);
> +
> +	return 0;
> +}
> +
> +int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u32 val, ctrl, num_ctrls;
> +	int ret;
>  
>  	/*
>  	 * Enable DBI read-only registers for writing/updating configuration.
> @@ -610,42 +688,9 @@ void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  	 * ATU, so we should not program the ATU here.
>  	 */
>  	if (pp->bridge->child_ops == &dw_child_pcie_ops) {
> -		int i, atu_idx = 0;
> -		struct resource_entry *entry;
> -
> -		/*
> -		 * Ensure all outbound windows are disabled so there are
> -		 * multiple matches
> -		 */
> -		for (i = 0; i < pci->num_ob_windows; i++)
> -			dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
> -
> -		/* Get last memory resource entry */
> -		resource_list_for_each_entry(entry, &pp->bridge->windows) {
> -			if (resource_type(entry->res) != IORESOURCE_MEM)
> -				continue;
> -
> -			if (pci->num_ob_windows <= ++atu_idx)
> -				break;
> -
> -			dw_pcie_prog_outbound_atu(pci, atu_idx,
> -						  PCIE_ATU_TYPE_MEM, entry->res->start,
> -						  entry->res->start - entry->offset,
> -						  resource_size(entry->res));
> -		}
> -
> -		if (pp->io_size) {
> -			if (pci->num_ob_windows > ++atu_idx)
> -				dw_pcie_prog_outbound_atu(pci, atu_idx,
> -							  PCIE_ATU_TYPE_IO, pp->io_base,
> -							  pp->io_bus_addr, pp->io_size);
> -			else
> -				pp->cfg0_io_shared = true;
> -		}
> -
> -		if (pci->num_ob_windows <= atu_idx)
> -			dev_warn(dev, "Resources exceed number of ATU entries (%d)\n",
> -				 pci->num_ob_windows);
> +		ret = dw_pcie_iatu_setup(pp);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
> @@ -658,5 +703,7 @@ void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_setup_rc);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 60f1ddc54933..c3e73ed9aff5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -387,7 +387,7 @@ static inline void dw_pcie_stop_link(struct dw_pcie *pci)
>  
>  #ifdef CONFIG_PCIE_DW_HOST
>  irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
> -void dw_pcie_setup_rc(struct dw_pcie_rp *pp);
> +int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
>  int dw_pcie_host_init(struct dw_pcie_rp *pp);
>  void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
>  int dw_pcie_allocate_domains(struct dw_pcie_rp *pp);
> @@ -399,8 +399,9 @@ static inline irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp)
>  	return IRQ_NONE;
>  }
>  
> -static inline void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> +static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  {
> +	return 0;
>  }
>  
>  static inline int dw_pcie_host_init(struct dw_pcie_rp *pp)
> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> index a44f685ec94d..c3481200e86a 100644
> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> @@ -302,7 +302,11 @@ static int intel_pcie_host_setup(struct intel_pcie *pcie)
>  	intel_pcie_ltssm_disable(pcie);
>  	intel_pcie_link_setup(pcie);
>  	intel_pcie_init_n_fts(pci);
> -	dw_pcie_setup_rc(&pci->pp);
> +
> +	ret = dw_pcie_setup_rc(&pci->pp);
> +	if (ret)
> +		goto app_init_err;
> +
>  	dw_pcie_upconfig_setup(pci);
>  
>  	intel_pcie_device_rst_deassert(pcie);
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists