linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vidya Sagar <vidyas@nvidia.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: <lorenzo.pieralisi@arm.com>, <robh+dt@kernel.org>,
	<bhelgaas@google.com>, <thierry.reding@gmail.com>,
	<jonathanh@nvidia.com>, <amanharitsh123@gmail.com>,
	<dinghao.liu@zju.edu.cn>, <kw@linux.com>,
	<linux-pci@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <kthota@nvidia.com>,
	<mmaddireddy@nvidia.com>, <sagar.tv@gmail.com>
Subject: Re: [PATCH V2 4/4] PCI: tegra: Handle error conditions properly
Date: Wed, 4 Nov 2020 14:21:58 +0530	[thread overview]
Message-ID: <2a0536d2-8603-0e55-ac61-c21ef36847c2@nvidia.com> (raw)
In-Reply-To: <20201103204835.GA262610@bjorn-Precision-5520>



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

      reply	other threads:[~2020-11-04  8:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29  5:18 [PATCH V2 0/4] Enhancements to Tegra194 PCIe driver Vidya Sagar
2020-10-29  5:18 ` [PATCH V2 1/4] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
2020-10-29  5:18 ` [PATCH V2 2/4] PCI: tegra: Map configuration space as nGnRnE Vidya Sagar
2020-10-29  5:18 ` [PATCH V2 3/4] PCI: tegra: Set DesignWare IP version Vidya Sagar
2020-10-29  5:18 ` [PATCH V2 4/4] PCI: tegra: Handle error conditions properly Vidya Sagar
2020-11-03 20:48   ` Bjorn Helgaas
2020-11-04  8:51     ` Vidya Sagar [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2a0536d2-8603-0e55-ac61-c21ef36847c2@nvidia.com \
    --to=vidyas@nvidia.com \
    --cc=amanharitsh123@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=dinghao.liu@zju.edu.cn \
    --cc=helgaas@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=kthota@nvidia.com \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=robh+dt@kernel.org \
    --cc=sagar.tv@gmail.com \
    --cc=thierry.reding@gmail.com \
    --subject='Re: [PATCH V2 4/4] PCI: tegra: Handle error conditions properly' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).