Re: [PATCH v5] PCI: dwc: Wait for link up only if link is started

[Ajay Agarwal <ajayagarwal@google.com>]

On Thu, Feb 15, 2024 at 07:39:08PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Feb 14, 2024 at 04:02:28PM -0600, Bjorn Helgaas wrote:
> > On Tue, Feb 06, 2024 at 10:40:43PM +0530, Manivannan Sadhasivam wrote:
> > > ...
> > 
> > > ... And for your usecase, allowing the controller driver to start
> > > the link post boot just because a device on your Pixel phone comes
> > > up later is not a good argument. You _should_not_ define the
> > > behavior of a controller driver based on one platform, it is really
> > > a bad design.
> > 
> > I haven't followed the entire discussion, and I don't know much about
> > the specifics of Ajay's situation, but from the controller driver's
> > point of view, shouldn't a device coming up later look like a normal
> > hot-add?
> > 
> 
> Yes, but most of the form factors that these drivers work with do not support
> native hotplug. So users have to rescan the bus through sysfs.
> 
> > I think most drivers are designed with the assumption that Endpoints
> > are present and powered up at the time of host controller probe, which
> > seems a little stronger than necessary.
> > 
> 
> Most of the drivers work with endpoints that are fixed in the board design (like
> M.2), so the endpoints would be up when the controller probes.
> 
> > I think the host controller probe should initialize the Root Port such
> > that its LTSSM enters the Detect state, and that much should be
> > basically straight-line code with no waiting.  If no Endpoint is
> > attached, i.e., "the slot is empty", it would be nice if the probe
> > could then complete immediately without waiting at all.
> > 
> 
> Atleast on Qcom platforms, the LTSSM would be in "Detect" state even if no
> endpoints are found during probe. Then once an endpoint comes up later, link
> training happens and user can rescan the bus through sysfs.
> 
> But, I don't know the real need of 1s loop to wait for the link. It predates my
> work on DWC drivers. Maybe Lorenzo could shed some light. I could not find the
> reference in both DWC and PCIe specs (maybe my grep was bad).
> 
> > If the link comes up later, could we handle it as a hot-add?  This
> > might be an actual hot-add, or it might be that an Endpoint was
> > present at boot but link training didn't complete until later.
> > 
> > I admit it doesn't look trivial to actually implement this.  We would
> > need to be able to detect link-up events, e.g., via hotplug or other
> > link management interrupts.  Lacking that hardware functionality, we
> > might need driver-specific code to wait for the link to come up
> > (possibly drivers could skip the wait if they can detect the "slot
> > empty" case).
> > 
> > Also, the hotplug functionality (pciehp or acpiphp) is currently
> > initialized later and there's probably a race with enabling and
> > detecting hot-add events in the "slot occupied" case.
> > 
> 
> As I mentioned above, hotplug is not possible in all the cases. There is a
> series floating to add GPIO based hotplug, but still that requires board
> designers to route a dedicated GPIO to the endpoint.
> 
> To conclude, we do need to check for the existence of the endpoints during
> probe. But whether the driver should wait for 1s for the link to come up,
> should be clarified by Lorenzo.
> 
Lorenzo himself applied my patch [1] which had caused the regression on
QCOM platforms. So I am assuming that he is okay with removing this 1s
delay.

 [1] https://lore.kernel.org/all/168509076553.135117.7288121992217982937.b4-ty@kernel.org/

> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்