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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
