Re: [PATCH] pci: pcie_dw_rockchip: release resources on failing probe

On 2023/4/13 22:11, Eugen Hristev wrote:

Implement a resource release mechanism on failing probe. Without this, a strange situation can happen e.g. when init port fails, or attempting to get the PHY fails, because the gpios have been requested first, and if the user tries to do 'pci enum' again, the driver will fail with 'can't find reset gpios' even if the gpios are there, just because they were blocked by a previous probe attempt. It is only natural to release the acquired resources if the probe fails, just for consistency if nothing else. This way on subsequent probe attempts, the user will get the same error message, and not something different that doesn't make sense.

Signed-off-by: Eugen Hristev eugen.hristev@collabora.com

Reviewed-by: Kever Yang kever.yang@rock-chips.com

Thanks, - Kever

---

drivers/pci/pcie_dw_rockchip.c | 41 +++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c index 9322e735b9c3..6155710a9f5f 100644 --- a/drivers/pci/pcie_dw_rockchip.c +++ b/drivers/pci/pcie_dw_rockchip.c @@ -375,29 +375,39 @@ static int rockchip_pcie_parse_dt(struct udevice *dev) ret = reset_get_bulk(dev, &priv->rsts); if (ret) { dev_err(dev, "Can't get reset: %d\n", ret);

• return ret;
  

• goto rockchip_pcie_parse_dt_err_reset_get_bulk;
  

  }

  ret = clk_get_bulk(dev, &priv->clks); if (ret) { dev_err(dev, "Can't get clock: %d\n", ret);

• return ret;
  

• goto rockchip_pcie_parse_dt_err_clk_get_bulk;
  

  }

  ret = device_get_supply_regulator(dev, "vpcie3v3-supply", &priv->vpcie3v3); if (ret && ret != -ENOENT) { dev_err(dev, "failed to get vpcie3v3 supply (ret=%d)\n", ret);

• return ret;
  

• goto rockchip_pcie_parse_dt_err_supply_regulator;
  

  }

  ret = generic_phy_get_by_index(dev, 0, &priv->phy); if (ret) { dev_err(dev, "failed to get pcie phy (ret=%d)\n", ret);

• return ret;
  

• goto rockchip_pcie_parse_dt_err_phy_get_by_index;
  

  }

  return 0;

+rockchip_pcie_parse_dt_err_phy_get_by_index:

• /* regulators don't need release */

+rockchip_pcie_parse_dt_err_supply_regulator:

• clk_release_bulk(&priv->clks);

+rockchip_pcie_parse_dt_err_clk_get_bulk:

• reset_release_bulk(&priv->rsts);

+rockchip_pcie_parse_dt_err_reset_get_bulk:

• dm_gpio_free(dev, &priv->rst_gpio);

• return ret; }

  /**

@@ -426,7 +436,7 @@ static int rockchip_pcie_probe(struct udevice *dev)

ret = rockchip_pcie_init_port(dev); if (ret)

• return ret;
  

• goto rockchip_pcie_probe_err_init_port;
  
  

  dev_info(dev, "PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev), pcie_dw_get_link_speed(&priv->dw),

@@ -434,12 +444,21 @@ static int rockchip_pcie_probe(struct udevice *dev) hose->first_busno);

• return pcie_dw_prog_outbound_atu_unroll(&priv->dw,

• 				PCIE_ATU_REGION_INDEX0,
  

• 				PCIE_ATU_TYPE_MEM,
  

• 				priv->dw.mem.phys_start,
  

• 				priv->dw.mem.bus_start,
  

• 				priv->dw.mem.size);
  

• ret = pcie_dw_prog_outbound_atu_unroll(&priv->dw,

• 			       PCIE_ATU_REGION_INDEX0,
  

• 			       PCIE_ATU_TYPE_MEM,
  

• 			       priv->dw.mem.phys_start,
  

• 			       priv->dw.mem.bus_start,
  

• 			       priv->dw.mem.size);
  

• if (!ret)

• return ret;
  

+rockchip_pcie_probe_err_init_port:

• clk_release_bulk(&priv->clks);

• reset_release_bulk(&priv->rsts);

• dm_gpio_free(dev, &priv->rst_gpio);

• return ret; }

  static const struct dm_pci_ops rockchip_pcie_ops = {