Sashiko says:
> commit e4f495a9118b40f8a77e8dc097821d2c846ed1fc
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> PCI: Remove device links to PHY
> This commit removes device links between the PCIe controllers and PHY
> providers across several drivers, since the PHY core already creates consumer
> device links.
>
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> > index b067a3296dd37..fc39c01b79648 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> [ ... ]
> > @@ -122,9 +121,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
> > pm_runtime_put_sync(dev);
> > pm_runtime_disable(dev);
> > cdns_pcie_disable_phy(cdns_plat_pcie->pcie);
> > - phy_count = cdns_plat_pcie->pcie->phy_count;
> > - while (phy_count--)
> > - device_link_del(cdns_plat_pcie->pcie->link[phy_count]);
> >
> > return 0;
>
> This isn't a bug introduced by your patch, but I noticed the error path in
> cdns_plat_pcie_probe() returns 0.
> Does this cause the driver core to consider the probe successful even when
> host or endpoint setup steps fail? It looks like the device would be left
> in an uninitialized and disabled state while still being bound to the driver.
> Should this return ret instead?
>
The condition does not appear intentional. Commit bd22885aa188 ("PCI:
cadence: Refactor driver to use as a core library") changed
cdns_pcie_host_probe() which properly returned ret on error with
cdns_plat_pcie_probe() which doesn't.
I will add a preparatory patch which replaces "return 0" with "return ret"
in cdns_plat_pcie_probe(), blaming that commit.
> > }
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index cd904659c3216..9c1aef4d0bd1c 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> [ ... ]
> > @@ -852,10 +840,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
> > pm_runtime_disable(dev);
> > dra7xx_pcie_disable_phy(dra7xx);
> >
> > -err_link:
> > - while (--i >= 0)
> > - device_link_del(link[i]);
> > -
> > return ret;
> > }
>
> This isn't a bug introduced by your changes, but looking closely at the error
> path right above this hunk, there is a call to pm_runtime_put(dev) immediately
> followed by pm_runtime_disable(dev):
> err_gpio:
> err_get_sync:
> pm_runtime_put(dev);
> pm_runtime_disable(dev);
> Does this sequence create a power management race condition? The pending
> asynchronous idle/suspend work can be abruptly cancelled by
> pm_runtime_disable(), leaving the hardware in an incorrect or unpredictable
> power state.
> Should this use pm_runtime_put_sync() instead?
>
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > index fe4836925c4e3..cab9fad212add 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> [ ... ]
> > @@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)
> > pm_runtime_disable(dev);
> > ks_pcie_disable_phy(ks_pcie);
> >
> > -err_link:
> > - while (--i >= 0 && link[i])
> > - device_link_del(link[i]);
> > -
> > +err:
> > return ret;
> > }
> >
> > static void ks_pcie_remove(struct platform_device *pdev)
> > {
> > struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
> > - struct device_link **link = ks_pcie->link;
> > - int num_lanes = ks_pcie->num_lanes;
> > struct device *dev = &pdev->dev;
> >
> > pm_runtime_put(dev);
> > pm_runtime_disable(dev);
>
> Similar to the question above, is it possible this asynchronous put causes
> a race when immediately followed by pm_runtime_disable() during removal?
>
> > ks_pcie_disable_phy(ks_pcie);
> > - while (num_lanes--)
> > - device_link_del(link[num_lanes]);
> > }
It seems plausible that the pm_runtime_put() -> pm_runtime_disable()
pattern is ineffective, i.e. one of two things can happen: either
pm_runtime_put() runs to completion by chance, or pm_runtime_disable()
cancels it. However I am not very familiar with the runtime PM API and
its effects, and unless a maintainer tells me to, I would prefer leaving
these code paths alone.
@@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
struct pci_host_bridge *bridge;
struct cdns_pcie_ep *ep;
struct cdns_pcie_rc *rc;
- int phy_count;
bool is_rc;
int ret;
@@ -122,9 +121,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
cdns_pcie_disable_phy(cdns_plat_pcie->pcie);
- phy_count = cdns_plat_pcie->pcie->phy_count;
- while (phy_count--)
- device_link_del(cdns_plat_pcie->pcie->link[phy_count]);
return 0;
}
@@ -222,7 +222,6 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
struct device_node *np = dev->of_node;
int phy_count;
struct phy **phy;
- struct device_link **link;
int i;
int ret;
const char *name;
@@ -238,10 +237,6 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
if (!phy)
return -ENOMEM;
- link = devm_kcalloc(dev, phy_count, sizeof(*link), GFP_KERNEL);
- if (!link)
- return -ENOMEM;
-
for (i = 0; i < phy_count; i++) {
of_property_read_string_index(np, "phy-names", i, &name);
phy[i] = devm_phy_get(dev, name);
@@ -249,17 +244,10 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
ret = PTR_ERR(phy[i]);
goto err_phy;
}
- link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
- if (!link[i]) {
- devm_phy_put(dev, phy[i]);
- ret = -EINVAL;
- goto err_phy;
- }
}
pcie->phy_count = phy_count;
pcie->phy = phy;
- pcie->link = link;
ret = cdns_pcie_enable_phy(pcie);
if (ret)
@@ -268,10 +256,8 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
return 0;
err_phy:
- while (--i >= 0) {
- device_link_del(link[i]);
+ while (--i >= 0)
devm_phy_put(dev, phy[i]);
- }
return ret;
}
@@ -82,7 +82,6 @@ struct cdns_plat_pcie_of_data {
* @is_rc: tell whether the PCIe controller mode is Root Complex or Endpoint.
* @phy_count: number of supported PHY devices
* @phy: list of pointers to specific PHY control blocks
- * @link: list of pointers to corresponding device link representations
* @ops: Platform-specific ops to control various inputs from Cadence PCIe
* wrapper
* @cdns_pcie_reg_offsets: Register bank offsets for different SoC
@@ -95,7 +94,6 @@ struct cdns_pcie {
bool is_rc;
int phy_count;
struct phy **phy;
- struct device_link **link;
const struct cdns_pcie_ops *ops;
const struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
};
@@ -9,7 +9,6 @@
#include <linux/clk.h>
#include <linux/delay.h>
-#include <linux/device.h>
#include <linux/err.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
@@ -679,7 +678,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
int i;
int phy_count;
struct phy **phy;
- struct device_link **link;
void __iomem *base;
struct dw_pcie *pci;
struct dra7xx_pcie *dra7xx;
@@ -727,10 +725,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
if (!phy)
return -ENOMEM;
- link = devm_kcalloc(dev, phy_count, sizeof(*link), GFP_KERNEL);
- if (!link)
- return -ENOMEM;
-
dra7xx->clk = devm_clk_get_optional(dev, NULL);
if (IS_ERR(dra7xx->clk))
return dev_err_probe(dev, PTR_ERR(dra7xx->clk),
@@ -745,12 +739,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
phy[i] = devm_phy_get(dev, name);
if (IS_ERR(phy[i]))
return PTR_ERR(phy[i]);
-
- link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
- if (!link[i]) {
- ret = -EINVAL;
- goto err_link;
- }
}
dra7xx->base = base;
@@ -852,10 +840,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
pm_runtime_disable(dev);
dra7xx_pcie_disable_phy(dra7xx);
-err_link:
- while (--i >= 0)
- device_link_del(link[i]);
-
return ret;
}
@@ -130,7 +130,6 @@ struct keystone_pcie {
int num_lanes;
u32 num_viewport;
struct phy **phy;
- struct device_link **link;
struct device_node *msi_intc_np;
struct irq_domain *intx_irq_domain;
struct device_node *np;
@@ -1130,7 +1129,6 @@ static int ks_pcie_probe(struct platform_device *pdev)
enum dw_pcie_device_mode mode;
struct dw_pcie *pci;
struct keystone_pcie *ks_pcie;
- struct device_link **link;
struct gpio_desc *gpiod;
struct resource *res;
void __iomem *base;
@@ -1201,31 +1199,17 @@ static int ks_pcie_probe(struct platform_device *pdev)
if (!phy)
return -ENOMEM;
- link = devm_kcalloc(dev, num_lanes, sizeof(*link), GFP_KERNEL);
- if (!link)
- return -ENOMEM;
-
for (i = 0; i < num_lanes; i++) {
snprintf(name, sizeof(name), "pcie-phy%d", i);
phy[i] = devm_phy_optional_get(dev, name);
if (IS_ERR(phy[i])) {
ret = PTR_ERR(phy[i]);
- goto err_link;
- }
-
- if (!phy[i])
- continue;
-
- link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
- if (!link[i]) {
- ret = -EINVAL;
- goto err_link;
+ goto err;
}
}
ks_pcie->np = np;
ks_pcie->pci = pci;
- ks_pcie->link = link;
ks_pcie->num_lanes = num_lanes;
ks_pcie->phy = phy;
@@ -1235,7 +1219,7 @@ static int ks_pcie_probe(struct platform_device *pdev)
ret = PTR_ERR(gpiod);
if (ret != -EPROBE_DEFER)
dev_err(dev, "Failed to get reset GPIO\n");
- goto err_link;
+ goto err;
}
/* Obtain references to the PHYs */
@@ -1250,7 +1234,7 @@ static int ks_pcie_probe(struct platform_device *pdev)
if (ret) {
dev_err(dev, "failed to enable phy\n");
- goto err_link;
+ goto err;
}
platform_set_drvdata(pdev, ks_pcie);
@@ -1337,25 +1321,18 @@ static int ks_pcie_probe(struct platform_device *pdev)
pm_runtime_disable(dev);
ks_pcie_disable_phy(ks_pcie);
-err_link:
- while (--i >= 0 && link[i])
- device_link_del(link[i]);
-
+err:
return ret;
}
static void ks_pcie_remove(struct platform_device *pdev)
{
struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
- struct device_link **link = ks_pcie->link;
- int num_lanes = ks_pcie->num_lanes;
struct device *dev = &pdev->dev;
pm_runtime_put(dev);
pm_runtime_disable(dev);
ks_pcie_disable_phy(ks_pcie);
- while (num_lanes--)
- device_link_del(link[num_lanes]);
}
static struct platform_driver ks_pcie_driver = {