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

["Krzysztof Wilczyński" <kw@linux.com>]

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