[PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[Fabio Estevam]

phy_power_on() may fail, so we should better check its return
value and propagate it in the error case.

This fixes the following Coverity error:

	CID 1472841:  Error handling issues  (CHECKED_RETURN)
	Calling "phy_power_on" without checking return value (as is done elsewhere 40 out of 50 times).
	phy_power_on(ep->phy);
	phy_init(ep->phy);
           
Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index c24dab383654..eabedc0529cb 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -254,13 +254,17 @@ static int exynos_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
 	phy_reset(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_power_on(ep->phy);
+	if (ret < 0)
+		return ret;
+
 	phy_init(ep->phy);
 
 	exynos_pcie_deassert_core_reset(ep);
-- 
2.25.1

Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[Krzysztof Wilczyński]

Hi Fabio,

Thank you for sending the patch over!

[...]
> This fixes the following Coverity error:
> 
> 	CID 1472841:  Error handling issues  (CHECKED_RETURN)
> 	Calling "phy_power_on" without checking return value (as is done elsewhere 40 out of 50 times).
> 	phy_power_on(ep->phy);
> 	phy_init(ep->phy);

This is good, however, you would need to wrap long lines, and that would
make the message from Coverity harder to read, etc.  Thus, it might be
better to use the "Addresses-Coverity-ID" which is becoming a de-facto
standard for referencing Coverity defects.  Check the following for some
examples:

   git log drivers/pci | grep 'Addresses-Coverity-ID:'

[...]         
> +	ret = phy_power_on(ep->phy);
> +	if (ret < 0)
> +		return ret;

I wonder if you would also have to call phy_exit() here, even though
eventually exynos_pcie_probe() would call it once the error propagates
all the way up the call stack.

Additionally, exynos_pcie_resume_noirq() does not do any error checking
after calling exynos_pcie_host_init() and does not call phy_exit()
either, and I am not sure if it should, though.

See some comments below.

> +
>  	phy_init(ep->phy);
[...]

A small nit here.  You can check for any non-zero return value, as
anything would indicate an error here.

I also have a suggestion.  Would you also be interested in addressing
two Coverity defects that were detected in exynos_pcie_host_init()?

These would be the one you addressed here (CID 1472841) in this patch
and the other would be:

  CID 1471267 (#1 of 1): Unchecked return value (CHECKED_RETURN)

Which is about checking return value from phy_init() that is called
immediately after phy_power_on() in exynos_pcie_host_init().

The error propagates from exynos_pcie_host_init() as follows:

  struct exynos_pcie_host_ops{}
    .host_init = exynos_pcie_host_init

  exynos_pcie_probe()              <-- phy_exit() called here if exynos_add_pcie_port() fails.
    exynos_add_pcie_port()
        dw_pcie_host_init()
          exynos_pcie_host_init()  <-- phy_power_on() and phy_init() called here.
            dw_pcie_host_init()
              struct pcie_port{}
                struct dw_pcie_host_ops{}
                  .host_init       <-- exynos_pcie_host_init() called via struct exynos_pcie_host_ops{}.

  struct exynos_pcie_pm_ops{}
    .suspend_noirq = exynos_pcie_suspend_noirq
    .resume_noirq = exynos_pcie_resume_noirq

  exynos_pcie_resume_noirq()
    exynos_pcie_host_init()        <-- called here, but without any error checking.

Thus, we could handle propagating error from both the phy_power_on() and
phy_init() in the same time, perhaps even in a single patch, or a small
series.

Also, since there is no error checking and/or handling that might be
returned from exynos_pcie_host_init() in the exynos_pcie_resume_noirq()
callback, then perhaps adding some error messages to be printed should
something bad happens regarding power management.  But this would
becompletely optional as there there is also no error checking and
handling in exynos_pcie_suspend_noirq() either.

Krzysztof

Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[Lorenzo Pieralisi]

On Tue, Feb 23, 2021 at 10:17:59PM +0100, Krzysztof Wilczyński wrote:
> Hi Fabio,
> 
> Thank you for sending the patch over!
> 
> [...]
> > This fixes the following Coverity error:
> > 
> > 	CID 1472841:  Error handling issues  (CHECKED_RETURN)
> > 	Calling "phy_power_on" without checking return value (as is done elsewhere 40 out of 50 times).
> > 	phy_power_on(ep->phy);
> > 	phy_init(ep->phy);
> 
> This is good, however, you would need to wrap long lines, and that would
> make the message from Coverity harder to read, etc.  Thus, it might be
> better to use the "Addresses-Coverity-ID" which is becoming a de-facto
> standard for referencing Coverity defects.  Check the following for some
> examples:
> 
>    git log drivers/pci | grep 'Addresses-Coverity-ID:'
> 
> [...]         
> > +	ret = phy_power_on(ep->phy);
> > +	if (ret < 0)
> > +		return ret;
> 
> I wonder if you would also have to call phy_exit() here, even though
> eventually exynos_pcie_probe() would call it once the error propagates
> all the way up the call stack.
> 
> Additionally, exynos_pcie_resume_noirq() does not do any error checking
> after calling exynos_pcie_host_init() and does not call phy_exit()
> either, and I am not sure if it should, though.
> 
> See some comments below.
> 
> > +
> >  	phy_init(ep->phy);
> [...]
> 
> A small nit here.  You can check for any non-zero return value, as
> anything would indicate an error here.
> 
> I also have a suggestion.  Would you also be interested in addressing
> two Coverity defects that were detected in exynos_pcie_host_init()?
> 
> These would be the one you addressed here (CID 1472841) in this patch
> and the other would be:
> 
>   CID 1471267 (#1 of 1): Unchecked return value (CHECKED_RETURN)
> 
> Which is about checking return value from phy_init() that is called
> immediately after phy_power_on() in exynos_pcie_host_init().
> 
> The error propagates from exynos_pcie_host_init() as follows:
> 
>   struct exynos_pcie_host_ops{}
>     .host_init = exynos_pcie_host_init
> 
>   exynos_pcie_probe()              <-- phy_exit() called here if exynos_add_pcie_port() fails.
>     exynos_add_pcie_port()
>         dw_pcie_host_init()
>           exynos_pcie_host_init()  <-- phy_power_on() and phy_init() called here.
>             dw_pcie_host_init()
>               struct pcie_port{}
>                 struct dw_pcie_host_ops{}
>                   .host_init       <-- exynos_pcie_host_init() called via struct exynos_pcie_host_ops{}.
> 
>   struct exynos_pcie_pm_ops{}
>     .suspend_noirq = exynos_pcie_suspend_noirq
>     .resume_noirq = exynos_pcie_resume_noirq
> 
>   exynos_pcie_resume_noirq()
>     exynos_pcie_host_init()        <-- called here, but without any error checking.
> 
> Thus, we could handle propagating error from both the phy_power_on() and
> phy_init() in the same time, perhaps even in a single patch, or a small
> series.
> 
> Also, since there is no error checking and/or handling that might be
> returned from exynos_pcie_host_init() in the exynos_pcie_resume_noirq()
> callback, then perhaps adding some error messages to be printed should
> something bad happens regarding power management.  But this would
> becompletely optional as there there is also no error checking and
> handling in exynos_pcie_suspend_noirq() either.

Fabio, what's the plan with this patch ?

Lorenzo

Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[Fabio Estevam]

Hi Lorenzo,

On Tue, Mar 23, 2021 at 8:10 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:

> Fabio, what's the plan with this patch ?

I will let someone who has access to this platform handle it.

Sorry, I have no time to address Krzystof's feedback.

Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value

[Lorenzo Pieralisi]

On Tue, Mar 23, 2021 at 08:33:31AM -0300, Fabio Estevam wrote:
> Hi Lorenzo,
> 
> On Tue, Mar 23, 2021 at 8:10 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> 
> > Fabio, what's the plan with this patch ?
> 
> I will let someone who has access to this platform handle it.

Jingoo, this requires your feedback please.

> Sorry, I have no time to address Krzystof's feedback.

Understood - since you posted the patch I asked, thank you
anyway.

Lorenzo