Re: [PATCH v2 1/2] PCI: dwc: Perform host_init() before registering msi

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Rob Herring <robh+dt@kernel.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Krzysztof Wilczy??ski <kw@linux.com>,
	Stanimir Varbanov <svarbanov@mm-sol.com>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	vidyas@nvidia.com
Subject: Re: [PATCH v2 1/2] PCI: dwc: Perform host_init() before registering msi
Date: Fri, 8 Oct 2021 12:08:25 -0700	[thread overview]
Message-ID: <YWCXKevPYiSkqapt@ripper> (raw)
In-Reply-To: <20211008174803.GA32277@lpieralisi>

On Fri 08 Oct 10:48 PDT 2021, Lorenzo Pieralisi wrote:

> [+Vidya]
> 
> On Tue, Aug 24, 2021 at 02:23:14PM -0700, Bjorn Andersson wrote:
> > On Tue 24 Aug 13:29 PDT 2021, Bjorn Helgaas wrote:
> > 
> > > On Tue, Aug 24, 2021 at 01:15:49PM -0700, Bjorn Andersson wrote:
> > > > On Tue 24 Aug 12:05 PDT 2021, Bjorn Helgaas wrote:
> > > > 
> > > > > On Mon, Aug 23, 2021 at 08:49:57AM -0700, Bjorn Andersson wrote:
> > > > > > On the Qualcomm sc8180x platform the bootloader does something related
> > > > > > to PCI that leaves a pending "msi" interrupt, which with the current
> > > > > > ordering often fires before init has a chance to enable the clocks that
> > > > > > are necessary for the interrupt handler to access the hardware.
> > > > > > 
> > > > > > Move the host_init() call before the registration of the "msi" interrupt
> > > > > > handler to ensure the host driver has a chance to enable the clocks.
> > > > > 
> > > > > Did you audit other drivers for similar issues?  If they do, we should
> > > > > fix them all at once.
> > > > 
> > > > I only looked at the DesignWware drivers, in an attempt to find any
> > > > issues the proposed reordering.
> > > > 
> > > > The set of bugs causes by drivers registering interrupts before critical
> > > > resources tends to be rather visible and I don't know if there's much
> > > > value in speculatively "fixing" drivers.
> > > > 
> > > > E.g. a quick look through the drivers I see a similar pattern in
> > > > pci-tegra.c, but it's unlikely that they have the similar problem in
> > > > practice and I have no way to validate that a change to the order would
> > > > have a positive effect - or any side effects.
> > > > 
> > > > Or am I misunderstanding your request?
> > > 
> > > That is exactly my request.
> > 
> > Okay.
> > 
> > > I'm not sure if the potential issue you
> > > noticed in pci-tegra.c is similar to the one I mentioned here:
> > > 
> > >   https://lore.kernel.org/linux-pci/20210624224040.GA3567297@bjorn-Precision-5520/
> > > 
> > 
> > As I still have the tegra driver open, I share your concern about the
> > use of potentially uninitialized variables.
> > 
> > The problem I was concerned about was however the same as in my patch
> > and the rockchip one, that if the tegra hardware isn't clocked the
> > pm_runtime_get_sync() (which would turn on power and clock) happens
> > after setting up the msi chain handler...
> > 
> > > but I am actually in favor of speculatively fixing drivers even though
> > > they're hard to test.  Code like this tends to get copied to other
> > > places, and fixing several drivers sometimes exposes opportunities for
> > > refactoring and sharing code.
> > > 
> > 
> > Looking through the other cases mentioned in your reply above certainly
> > gives a feeling that this problem has been inherited from driver to
> > driver...
> > 
> > I've added a ticket to my backlog to take a deeper look at this.
> 
> Vidya, can you look into this please ? In the meantime I would merge
> this series.
> 

I would greatly appreciate that, as it unlocks 5G connectivity and NVME
support on my laptop. I still intend to review the items Bjorn pointed
out, but I haven't gotten there yet.

Thanks,
Bjorn

> Thanks,
> Lorenzo
> 
> > 
> > Regards,
> > Bjorn