| Message ID | 20210208174114.615811-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: dwc: exynos: Check the phy_power_on() return value | expand |
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
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
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.
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
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);
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(-)