Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt

From: "Marek Behún" <kabel@kernel.org>
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Bjorn Helgaas" <helgaas@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	pali@kernel.org, linux-pci@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, maz@kernel.org,
	tglx@linutronix.de
Subject: Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt
Date: Fri, 16 Sep 2022 18:23:02 +0200	[thread overview]
Message-ID: <20220916182302.4eba1b48@dellmb> (raw)
In-Reply-To: <YxtUR0+dBZut8QZH@lpieralisi>

On Fri, 9 Sep 2022 16:57:11 +0200
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:

> [+Marc, Thomas - I can't merge this code without them reviewing it,
> I am not sure at all you can mix the timer/IRQ code the way you do]
> 
> On Thu, Aug 18, 2022 at 03:51:32PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Add support for Data Link Layer State Change in the emulated slot
> > registers and hotplug interrupt via the emulated root bridge.
> > 
> > This is mainly useful for when an error causes link down event. With
> > this change, drivers can try recovery.
> > 
> > Link down state change can be implemented because Aardvark supports Link
> > Down event interrupt. Use it for signaling that Data Link Layer Link is
> > not active anymore via Hot-Plug Interrupt on emulated root bridge.
> > 
> > Link up interrupt is not available on Aardvark, but we check for whether
> > link is up in the advk_pcie_link_up() function. By triggering Hot-Plug
> > Interrupt from this function we achieve Link up event, so long as the
> > function is called (which it is after probe and when rescanning).
> > Although it is not ideal, it is better than nothing.  
> 
> So before even coming to the code review: this patch does two things.
> 
> 1) It adds support for handling the Link down state
> 2) It adds some code to emulate a Link-up event
> 
> Now, for (2). IIUC you are adding code to make sure that an HP
> event is triggered if advk_pcie_link_up() is called and it
> detects a Link-down->Link-up transition, that has to be notified
> through an HP event.
> 
> If that's correct, you have to explain to me please what this is
> actually achieving and a specific scenario where we want this to be
> implemented, in fine details; then we add it to the commit log.

Hello Lorenzo, sorry for not replying earlier.

Would something like this be sufficient?

  DLLSC is needed by the pciehp driver, which handles hotplug, but also
  link state change events. AFAIK no Aardvark devices support hotplug,
  but link state change events are required for graceful driver
  unbinding in case of link down event.

  So with this change we achieve graceful driver unbind for example
  when WiFi card PCIe link goes down (we've seen this with ath10k and
  mt76 WiFi cards). Before the WiFi driver started spitting out errors,
  or even taking the whole system down.

  Since after link goes down, it can come back up if the WiFi card can
  recover (or if reset pin is used to reset the card), we need to be
  able to recognize link up event. Since AFAIK Aardvark does not have
  interrupt for link up event, the best thing we can do is simulate it
  - whenever we read the link state, find it is up, and have cached
  value saying it is down, we trigger the link up event. We read link
  state whenever the configuration space is read, for example by
  writing 1 to /sys/bus/pci/rescan.

Marek

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel