Re: [PATCH 1/1] PCI: Cleanup link activation wait logic

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org,  LKML <linux-kernel@vger.kernel.org>,
	 Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH 1/1] PCI: Cleanup link activation wait logic
Date: Fri, 16 Feb 2024 16:23:29 +0200 (EET)	[thread overview]
Message-ID: <853a63bd-74cb-e19b-24b7-426a0fdd9003@linux.intel.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2402161349350.3971@angie.orcam.me.uk>

[-- Attachment #1: Type: text/plain, Size: 2191 bytes --]

On Fri, 16 Feb 2024, Maciej W. Rozycki wrote:
> On Fri, 16 Feb 2024, Ilpo Järvinen wrote:
> 
> > >  You change the logic here in that the second conditional isn't run if the 
> > > first has not.  This is wrong, unclamping is not supposed to rely on LBMS. 
> > > It is supposed to be always run and any failure has to be reported too, as 
> > > a retraining error.
> > 
> > Now that (I think) I fully understand the intent of the second 
> > condition/block one additional question occurred to me.
> > 
> > How is the 2nd condition even supposed to work in the current place when 
> > firmware has pre-arranged the 2.5GT/s resctriction? Wouldn't the link come 
> > up fine in that case and the quirk code is not called at all since the 
> > link came up successfully?
> 
>  The quirk is called unconditionally from `pci_device_add', so an attempt 
> to unclamp will always happen with a working link for qualifying devices.

Ah, thanks. I'd stared the other two calls enough of times I'd forgotten 
the 3rd one even existed.

> > Yet another thing in this quirk code I don't like is how it can leaves the 
> > target speed to 2.5GT/s when the quirk fails to get the link working 
> > (which actually does happen in the disconnection cases because DLLLA won't 
> > be set so the target speed will not be restored).
> 
>  I chose to leave the target speed at the most recent setting, because the 
> link doesn't work in that case anyway, so I concluded it doesn't matter, 
> but reduces messing with the device; technically you should retrain again 
> afterwards.  I'm not opposed to changing this if you have a use case.

It remains suboptimally set in a case where something is plugged again 
into that port, for Thunderbolt it doesn't matter as the PCIe speed picked 
is quite bogus anyway, but disconnect then plug something again is not 
limited to Thunderbolt.

I've no immediate plans on changing it now but it may come relevant when 
attempting to make the bandwidth controller to trigger the quirk. To me 
there are two quirks, not just one so I might have to split them to make 
it better suited for triggering them from bwctrl.

-- 
 i.