linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value
@ 2021-02-08 17:41 Fabio Estevam
  2021-02-23 21:17 ` Krzysztof Wilczyński
  0 siblings, 1 reply; 5+ messages in thread
From: Fabio Estevam @ 2021-02-08 17:41 UTC (permalink / raw)
  To: bhelgaas
  Cc: jingoohan1, lorenzo.pieralisi, robh, linux-pci, Fabio Estevam,
	Bjorn Helgaas

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value
  2021-02-08 17:41 [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value Fabio Estevam
@ 2021-02-23 21:17 ` Krzysztof Wilczyński
  2021-03-23 11:10   ` Lorenzo Pieralisi
  0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-23 21:17 UTC (permalink / raw)
  To: Fabio Estevam; +Cc: helgaas, jingoohan1, lorenzo.pieralisi, robh, linux-pci

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value
  2021-02-23 21:17 ` Krzysztof Wilczyński
@ 2021-03-23 11:10   ` Lorenzo Pieralisi
  2021-03-23 11:33     ` Fabio Estevam
  0 siblings, 1 reply; 5+ messages in thread
From: Lorenzo Pieralisi @ 2021-03-23 11:10 UTC (permalink / raw)
  To: Krzysztof Wilczyński, Fabio Estevam
  Cc: helgaas, jingoohan1, robh, linux-pci

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value
  2021-03-23 11:10   ` Lorenzo Pieralisi
@ 2021-03-23 11:33     ` Fabio Estevam
  2021-03-23 11:49       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 5+ messages in thread
From: Fabio Estevam @ 2021-03-23 11:33 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Krzysztof Wilczyński, Bjorn Helgaas, Jingoo Han,
	Rob Herring, linux-pci

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value
  2021-03-23 11:33     ` Fabio Estevam
@ 2021-03-23 11:49       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Pieralisi @ 2021-03-23 11:49 UTC (permalink / raw)
  To: Fabio Estevam, Jingoo Han
  Cc: Krzysztof Wilczyński, Bjorn Helgaas, Rob Herring, linux-pci

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-03-23 11:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 17:41 [PATCH] PCI: dwc: exynos: Check the phy_power_on() return value Fabio Estevam
2021-02-23 21:17 ` Krzysztof Wilczyński
2021-03-23 11:10   ` Lorenzo Pieralisi
2021-03-23 11:33     ` Fabio Estevam
2021-03-23 11:49       ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).