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

From: Marc Zyngier <maz@kernel.org>
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: "Marek Behún" <kabel@kernel.org>,
	"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, tglx@linutronix.de
Subject: Re: [PATCH 03/11] PCI: aardvark: Add support for DLLSC and hotplug interrupt
Date: Sat, 17 Sep 2022 10:05:59 +0100	[thread overview]
Message-ID: <87r10al6a0.wl-maz@kernel.org> (raw)
In-Reply-To: <YxtUR0+dBZut8QZH@lpieralisi>

Hi Lorenzo,

On Fri, 09 Sep 2022 15:57:11 +0100,
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.
> 
> That aside, the interaction of the timer and the IRQ domain code
> must be reviewed by Marc and Thomas to make sure this is not
> a gross violation of the respective subsystems usage.

I don't see anything being a "gross violation" here, at least from an
interrupt subsystem perspective. In a way, this is synthesising an
interrupt on the back of some other event, and as long as the context
is somehow appropriate (something that looks like an interrupt when
pretending there is one), this should be OK. Other subsystems such as
i2c GPIO expanders do similar things.

The one thing I'm dubious about is the frequency of the timer. Asking
for a poll of the link every jiffy is bound to be expensive, and it
would be good to relax this as much as possible, specially on low-end
HW such as this, where every cycle counts. It is always going to be a
"best effort" thing, and the commit message doesn't say what's the
actual grace period to handle this (the spec probably has one).

I guess this patch could do with being split between handling link
down and link up events, but that's for you to decide.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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