Messages in this thread | | | Subject | Re: [PATCH V2 4/4] PCI: tegra: Handle error conditions properly | From | Vidya Sagar <> | Date | Wed, 4 Nov 2020 14:21:58 +0530 |
| |
On 11/4/2020 2:18 AM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > Hi Vidya, > > Can you update the subject to replace "properly" with more details > about what the patch is doing? "Properly" is really meaningless in > usages like this -- nobody writes patches to do the *wrong* thing, so > it goes without saying that every patch is intended to things > "properly". > > It would also help to have some context. My first thought was that > "error conditions" referred to PCIe errors like completion timeouts, > completer abort, etc. > > Maybe something like: > > PCI: tegra: Continue unconfig sequence even if parts fail Thanks for reviewing the change. Sure. I'll go with the above subject line.
> PCI: tegra: Return init error (not unconfig error) on init failure > > On Thu, Oct 29, 2020 at 10:48:39AM +0530, Vidya Sagar wrote: >> Currently the driver checks for error value of different APIs during the >> uninitialization sequence. It just returns from there if there is any error >> observed for one of those calls. Comparatively it is better to continue the >> uninitialization sequence irrespective of whether some of them are >> returning error. That way, it is more closer to complete uninitialization. >> It also adds checking return value for error for a cleaner exit path. > > This paragraph uses "it" to refer to both "the driver" (second > sentence) and "this patch" (last sentence). That's confusing. > There's no reason to refer to "this patch" at all. I'd rather have > "Add checking ..." than "It adds checking ..." > > I think that last sentence must be referring to the > tegra_pcie_init_controller() change to return the initialization error > rather than the error from __deinit_controller(). That seems right, > but should be a separate patch. Sure. I'll push a new patch for this.
> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V2: >> * None >> >> drivers/pci/controller/dwc/pcie-tegra194.c | 45 ++++++++++------------ >> 1 file changed, 20 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >> index 253d91033bc3..8c08998b9ce1 100644 >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c >> @@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, >> return ret; >> } >> >> -static int __deinit_controller(struct tegra_pcie_dw *pcie) >> +static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie) >> { >> int ret; >> >> ret = reset_control_assert(pcie->core_rst); >> - if (ret) { >> - dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", >> - ret); >> - return ret; >> - } >> + if (ret) >> + dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", ret); >> >> tegra_pcie_disable_phy(pcie); >> >> ret = reset_control_assert(pcie->core_apb_rst); >> - if (ret) { >> + if (ret) >> dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret); >> - return ret; >> - } >> >> clk_disable_unprepare(pcie->core_clk); >> >> ret = regulator_disable(pcie->pex_ctl_supply); >> - if (ret) { >> + if (ret) >> dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret); >> - return ret; >> - } >> >> tegra_pcie_disable_slot_regulators(pcie); >> >> ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false); >> - if (ret) { >> + if (ret) >> dev_err(pcie->dev, "Failed to disable controller %d: %d\n", >> pcie->cid, ret); >> - return ret; >> - } >> - >> - return ret; >> } >> >> static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie) >> @@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie) >> return 0; >> >> fail_host_init: >> - return __deinit_controller(pcie); >> + tegra_pcie_unconfig_controller(pcie); >> + return ret; >> } >> >> static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie) >> @@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie) >> appl_writel(pcie, data, APPL_PINMUX); >> } >> >> -static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie) >> +static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie) >> { >> tegra_pcie_downstream_dev_to_D0(pcie); >> dw_pcie_host_deinit(&pcie->pci.pp); >> tegra_pcie_dw_pme_turnoff(pcie); >> - >> - return __deinit_controller(pcie); >> + tegra_pcie_unconfig_controller(pcie); >> } >> >> static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie) >> @@ -1590,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie) >> goto fail_pm_get_sync; >> } >> >> - tegra_pcie_init_controller(pcie); >> + ret = tegra_pcie_init_controller(pcie); >> + if (ret < 0) { >> + dev_err(dev, "Failed to initialize controller: %d\n", ret); >> + goto fail_pm_get_sync; >> + } >> >> pcie->link_state = tegra_pcie_dw_link_up(&pcie->pci); >> if (!pcie->link_state) { >> @@ -2238,8 +2231,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev) >> PORT_LOGIC_MSI_CTRL_INT_0_EN); >> tegra_pcie_downstream_dev_to_D0(pcie); >> tegra_pcie_dw_pme_turnoff(pcie); >> + tegra_pcie_unconfig_controller(pcie); >> >> - return __deinit_controller(pcie); >> + return 0; >> } >> >> static int tegra_pcie_dw_resume_noirq(struct device *dev) >> @@ -2267,7 +2261,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev) >> return 0; >> >> fail_host_init: >> - return __deinit_controller(pcie); >> + tegra_pcie_unconfig_controller(pcie); >> + return ret; >> } >> >> static int tegra_pcie_dw_resume_early(struct device *dev) >> @@ -2305,7 +2300,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev) >> disable_irq(pcie->pci.pp.msi_irq); >> >> tegra_pcie_dw_pme_turnoff(pcie); >> - __deinit_controller(pcie); >> + tegra_pcie_unconfig_controller(pcie); >> } >> >> static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = { >> -- >> 2.17.1 >>
| |