Re: [PATCH V1] PCI: dwc: Use dev_info for PCIe link down event logging

From: Bjorn Helgaas <helgaas@kernel.org>
To: Jon Hunter <jonathanh@nvidia.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Vidya Sagar <vidyas@nvidia.com>,
	jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
	lpieralisi@kernel.org, robh@kernel.org, kw@linux.com,
	bhelgaas@google.com, treding@nvidia.com,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	kthota@nvidia.com, mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V1] PCI: dwc: Use dev_info for PCIe link down event logging
Date: Tue, 13 Sep 2022 15:07:46 -0500	[thread overview]
Message-ID: <20220913200746.GA619956@bhelgaas> (raw)
In-Reply-To: <b1c243b0-2e6e-3254-eff0-a5276020a320@nvidia.com>

On Tue, Sep 13, 2022 at 06:00:30PM +0100, Jon Hunter wrote:
> On 13/09/2022 17:51, Manivannan Sadhasivam wrote:
> > On Tue, Sep 13, 2022 at 03:42:37PM +0530, Vidya Sagar wrote:
> > > Some of the platforms (like Tegra194 and Tegra234) have open slots and
> > > not having an endpoint connected to the slot is not an error.
> > > So, changing the macro from dev_err to dev_info to log the event.
> > 
> > But the link up not happening is an actual error and -ETIMEDOUT is being
> > returned. So I don't think the log severity should be changed.
> 
> Yes it is an error in the sense it is a timeout, but reporting an error
> because nothing is attached to a PCI slot seems a bit noisy. Please note
> that a similar change was made by the following commit and it also seems
> appropriate here ...
> 
> commit 4b16a8227907118e011fb396022da671a52b2272
> Author: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Date:   Tue Jun 18 23:32:06 2019 +0530
> 
>     PCI: tegra: Change link retry log level to debug
> 
> 
> BTW, we check for error messages in the dmesg output and this is a new error
> seen as of Linux v6.0 and so this was flagged in a test. We can ignore the
> error, but in this case it seem more appropriate to make this a info or
> debug level print.

Can you tell whether there's a device present, e.g., via Slot Status
Presence Detect?  If there's nothing in the slot, I don't know why we
would print anything at all.  If a card is present but there's no
link, that's probably worthy of dev_info() or even dev_err().

I guess if you can tell the slot is empty, there's no point in even
trying to start the link, so you could avoid both the message and the
timeout by not even calling dw_pcie_wait_for_link().

Bjorn