| Message ID | 375a852ae9170a5f8f53fdd364a7fb7ea642e89c.1548110502.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: imx: imx6_pcie_attach_pd | expand |
On Mon, Jan 21, 2019 at 10:50:04PM +0000, Leonard Crestez wrote: > On chips without a separate power domain for PCI (such as 6q/6qp) the > imx6_pcie_attach_pd function incorrectly returns an error. > > Fix by returning 0 if dev_pm_domain_attach_by_name doesn't find > anything. > > Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support") > Reported-by: Lukas F.Hartmann <lukas@mntmn.com> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 52e47dac028f..ac5f6ae0b254 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -308,10 +308,13 @@ static int imx6_pcie_attach_pd(struct device *dev) > return 0; > > imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); > if (IS_ERR(imx6_pcie->pd_pcie)) > return PTR_ERR(imx6_pcie->pd_pcie); > + /* Do nothing when power domain missing */ > + if (!imx6_pcie->pd_pcie) > + return 0; As I said in the mail that proposed this patch for testing: I think it would be better to change dev_pm_domain_attach_by_name to not return an error indication by returning NULL or an ERR_PTR value. (Or change device_link_add to accept NULL if NULL is a dummy value.) Just repeating it here to have it near the actual patch. Best regards Uwe > link = device_link_add(dev, imx6_pcie->pd_pcie, > DL_FLAG_STATELESS | > DL_FLAG_PM_RUNTIME | > DL_FLAG_RPM_ACTIVE); > if (!link) { > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 1/22/2019 9:12 AM, Uwe Kleine-König wrote: > On Mon, Jan 21, 2019 at 10:50:04PM +0000, Leonard Crestez wrote: >> On chips without a separate power domain for PCI (such as 6q/6qp) the >> imx6_pcie_attach_pd function incorrectly returns an error. >> >> Fix by returning 0 if dev_pm_domain_attach_by_name doesn't find >> anything. >> >> Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support") >> >> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c >> @@ -308,10 +308,13 @@ static int imx6_pcie_attach_pd(struct device *dev) >> return 0; >> >> imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); >> if (IS_ERR(imx6_pcie->pd_pcie)) >> return PTR_ERR(imx6_pcie->pd_pcie); >> + /* Do nothing when power domain missing */ >> + if (!imx6_pcie->pd_pcie) >> + return 0; > > As I said in the mail that proposed this patch for testing: I think it > would be better to change dev_pm_domain_attach_by_name to not return an > error indication by returning NULL or an ERR_PTR value. (Or change > device_link_add to accept NULL if NULL is a dummy value.) > > Just repeating it here to have it near the actual patch. Link to that previous discussion: http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/626859.html I'm not sure what you mean. Should dev_pm_domain_attach_by_name return ERR_PTR(-ENOENT) for name not found? It would still require special handling for callers. The device_link_add function already returns NULL if the consumer or supplier is NULL but that's also the only way it signals failure. Maybe that function should be adjusted to return ERR_PTR instead? Changing core API for a driver bug seems inappropriate. The current imx6_pcie_attach_pd code is complicated because it tries to distinguish between "multi pd" and "no pd / single pd". I wish there was a way for these PM_RUNTIME device_links to be setup automatically in core and have multi-PD behave the same as a single PD. There aren't many multi-PD users but commit 6494a9ad86de ("usb: xhci: tegra: Add genpd support") seems to do pretty much the same thing. -- Regards, Leonard
On Tue, Jan 22, 2019 at 09:17:22AM +0000, Leonard Crestez wrote: > On 1/22/2019 9:12 AM, Uwe Kleine-König wrote: > > On Mon, Jan 21, 2019 at 10:50:04PM +0000, Leonard Crestez wrote: > >> On chips without a separate power domain for PCI (such as 6q/6qp) the > >> imx6_pcie_attach_pd function incorrectly returns an error. > >> > >> Fix by returning 0 if dev_pm_domain_attach_by_name doesn't find > >> anything. > >> > >> Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support") > >> > >> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > >> @@ -308,10 +308,13 @@ static int imx6_pcie_attach_pd(struct device *dev) > >> return 0; > >> > >> imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); > >> if (IS_ERR(imx6_pcie->pd_pcie)) > >> return PTR_ERR(imx6_pcie->pd_pcie); > >> + /* Do nothing when power domain missing */ > >> + if (!imx6_pcie->pd_pcie) > >> + return 0; > > > > As I said in the mail that proposed this patch for testing: I think it > > would be better to change dev_pm_domain_attach_by_name to not return an > > error indication by returning NULL or an ERR_PTR value. (Or change > > device_link_add to accept NULL if NULL is a dummy value.) > > > > Just repeating it here to have it near the actual patch. > > Link to that previous discussion: > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/626859.html > > I'm not sure what you mean. Should dev_pm_domain_attach_by_name return > ERR_PTR(-ENOENT) for name not found? It would still require special > handling for callers. Yes, callers would still need to handle an error, but they only need to check for IS_ERR(retval) instead of IS_ERR(retval) && retval != NULL. > The device_link_add function already returns NULL if the consumer or > supplier is NULL but that's also the only way it signals failure. Maybe > that function should be adjusted to return ERR_PTR instead? > > Changing core API for a driver bug seems inappropriate. Well, if the driver bug is there because the API is used in a wrong way and the right way is strange, fixing the API and checking other users for similar mistakes is the right thing to do. > The current imx6_pcie_attach_pd code is complicated because it tries to > distinguish between "multi pd" and "no pd / single pd". I wish there was > a way for these PM_RUNTIME device_links to be setup automatically in > core and have multi-PD behave the same as a single PD. > > There aren't many multi-PD users but commit 6494a9ad86de ("usb: xhci: > tegra: Add genpd support") seems to do pretty much the same thing. I don't know enough about power domains to follow this argumentation in the time I have available. Best regards Uwe
On 1/22/19 11:44 AM, Uwe Kleine-König wrote: > On Tue, Jan 22, 2019 at 09:17:22AM +0000, Leonard Crestez wrote: >> On 1/22/2019 9:12 AM, Uwe Kleine-König wrote: >>> On Mon, Jan 21, 2019 at 10:50:04PM +0000, Leonard Crestez wrote: >>>> On chips without a separate power domain for PCI (such as 6q/6qp) the >>>> imx6_pcie_attach_pd function incorrectly returns an error. >>>> >>>> Fix by returning 0 if dev_pm_domain_attach_by_name doesn't find >>>> anything. >>>> >>>> Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support") >>>> >>>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c >>>> @@ -308,10 +308,13 @@ static int imx6_pcie_attach_pd(struct device *dev) >>>> return 0; >>>> >>>> imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); >>>> if (IS_ERR(imx6_pcie->pd_pcie)) >>>> return PTR_ERR(imx6_pcie->pd_pcie); >>>> + /* Do nothing when power domain missing */ >>>> + if (!imx6_pcie->pd_pcie) >>>> + return 0; >>> >>> As I said in the mail that proposed this patch for testing: I think it >>> would be better to change dev_pm_domain_attach_by_name to not return an >>> error indication by returning NULL or an ERR_PTR value. (Or change >>> device_link_add to accept NULL if NULL is a dummy value.) >> >> I'm not sure what you mean. Should dev_pm_domain_attach_by_name return >> ERR_PTR(-ENOENT) for name not found? It would still require special >> handling for callers. > > Yes, callers would still need to handle an error, but they only need to > check for IS_ERR(retval) instead of IS_ERR(retval) && retval != NULL. But "not found" is not treated as an error here, it's what happens when no PD is assigned. The fix makes imx6_pcie_attach_pd return success in this case. Making dev_pm_domain_attach_by_name return ENOENT would still require a special case for ENOENT here. -- Regards, Leonard
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 52e47dac028f..ac5f6ae0b254 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -308,10 +308,13 @@ static int imx6_pcie_attach_pd(struct device *dev) return 0; imx6_pcie->pd_pcie = dev_pm_domain_attach_by_name(dev, "pcie"); if (IS_ERR(imx6_pcie->pd_pcie)) return PTR_ERR(imx6_pcie->pd_pcie); + /* Do nothing when power domain missing */ + if (!imx6_pcie->pd_pcie) + return 0; link = device_link_add(dev, imx6_pcie->pd_pcie, DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); if (!link) {
On chips without a separate power domain for PCI (such as 6q/6qp) the imx6_pcie_attach_pd function incorrectly returns an error. Fix by returning 0 if dev_pm_domain_attach_by_name doesn't find anything. Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support") Reported-by: Lukas F.Hartmann <lukas@mntmn.com> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 3 +++ 1 file changed, 3 insertions(+)