[PATCH 0/2] PCI: imx: imx6_pcie_attach_pd

[PATCH 0/2] PCI: imx: imx6_pcie_attach_pd

[Leonard Crestez]

This fixes pci probing on imx6qp as reported by Lukas Hartmann.

The second patch fixes a check that shouldn't actually fail.

Leonard Crestez (2):
  PCI: imx: Fix probe failure without power domain
  PCI: imx: Fix checking pd_pcie_phy device_link

 drivers/pci/controller/dwc/pci-imx6.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 0/2] PCI: imx: imx6_pcie_attach_pd

[Leonard Crestez]

This fixes pci probing on imx6qp as reported by Lukas Hartmann.

The second patch fixes a check that shouldn't actually fail.

Leonard Crestez (2):
  PCI: imx: Fix probe failure without power domain
  PCI: imx: Fix checking pd_pcie_phy device_link

 drivers/pci/controller/dwc/pci-imx6.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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;
 	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

[PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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;
 	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

[PATCH 2/2] PCI: imx: Fix checking pd_pcie_phy device_link

[Leonard Crestez]

This check was completely wrong. It doesn't break anything specific
because adding the second device_link can't fail.

Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support")

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ac5f6ae0b254..80f843030e36 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -324,17 +324,17 @@ static int imx6_pcie_attach_pd(struct device *dev)
 
 	imx6_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy");
 	if (IS_ERR(imx6_pcie->pd_pcie_phy))
 		return PTR_ERR(imx6_pcie->pd_pcie_phy);
 
-	device_link_add(dev, imx6_pcie->pd_pcie_phy,
+	link = device_link_add(dev, imx6_pcie->pd_pcie_phy,
 			DL_FLAG_STATELESS |
 			DL_FLAG_PM_RUNTIME |
 			DL_FLAG_RPM_ACTIVE);
-	if (IS_ERR(link)) {
-		dev_err(dev, "Failed to add device_link to pcie_phy pd: %ld\n", PTR_ERR(link));
-		return PTR_ERR(link);
+	if (!link) {
+		dev_err(dev, "Failed to add device_link to pcie_phy pd.\n");
+		return -EINVAL;
 	}
 
 	return 0;
 }
 
-- 
2.17.1

[PATCH 2/2] PCI: imx: Fix checking pd_pcie_phy device_link

[Leonard Crestez]

This check was completely wrong. It doesn't break anything specific
because adding the second device_link can't fail.

Fixes: 3f7cceeab895 ("PCI: imx: Add multi-pd support")

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ac5f6ae0b254..80f843030e36 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -324,17 +324,17 @@ static int imx6_pcie_attach_pd(struct device *dev)
 
 	imx6_pcie->pd_pcie_phy = dev_pm_domain_attach_by_name(dev, "pcie_phy");
 	if (IS_ERR(imx6_pcie->pd_pcie_phy))
 		return PTR_ERR(imx6_pcie->pd_pcie_phy);
 
-	device_link_add(dev, imx6_pcie->pd_pcie_phy,
+	link = device_link_add(dev, imx6_pcie->pd_pcie_phy,
 			DL_FLAG_STATELESS |
 			DL_FLAG_PM_RUNTIME |
 			DL_FLAG_RPM_ACTIVE);
-	if (IS_ERR(link)) {
-		dev_err(dev, "Failed to add device_link to pcie_phy pd: %ld\n", PTR_ERR(link));
-		return PTR_ERR(link);
+	if (!link) {
+		dev_err(dev, "Failed to add device_link to pcie_phy pd.\n");
+		return -EINVAL;
 	}
 
 	return 0;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Uwe Kleine-König]

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
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Uwe Kleine-König]

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
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Uwe Kleine-König]

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/  |

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Uwe Kleine-König]

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/  |

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Uwe Kleine-König]

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/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

Re: [PATCH 1/2] PCI: imx: Fix probe failure without power domain

[Leonard Crestez]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] PCI: imx: imx6_pcie_attach_pd

[Lorenzo Pieralisi]

On Mon, Jan 21, 2019 at 10:50:03PM +0000, Leonard Crestez wrote:
> This fixes pci probing on imx6qp as reported by Lukas Hartmann.
> 
> The second patch fixes a check that shouldn't actually fail.
> 
> Leonard Crestez (2):
>   PCI: imx: Fix probe failure without power domain
>   PCI: imx: Fix checking pd_pcie_phy device_link
> 
>  drivers/pci/controller/dwc/pci-imx6.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

I have applied them to pci/controller-fixes and Bjorn already
queued them for one of the upcoming -rc*.

Thanks,
Lorenzo

Re: [PATCH 0/2] PCI: imx: imx6_pcie_attach_pd

[Lorenzo Pieralisi]

On Mon, Jan 21, 2019 at 10:50:03PM +0000, Leonard Crestez wrote:
> This fixes pci probing on imx6qp as reported by Lukas Hartmann.
> 
> The second patch fixes a check that shouldn't actually fail.
> 
> Leonard Crestez (2):
>   PCI: imx: Fix probe failure without power domain
>   PCI: imx: Fix checking pd_pcie_phy device_link
> 
>  drivers/pci/controller/dwc/pci-imx6.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

I have applied them to pci/controller-fixes and Bjorn already
queued them for one of the upcoming -rc*.

Thanks,
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel