Re: [PATCH V2 4/4] PCI: tegra: Handle error conditions properly

[Vidya Sagar]

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@xxxxxxxxxx>
> > ---
> > 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