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