Re: [PATCH pci-fixes 2/2] Revert "PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge"

From: "Marek Behún" <kabel@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, pali@kernel.org
Subject: Re: [PATCH pci-fixes 2/2] Revert "PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge"
Date: Wed, 1 Dec 2021 10:50:45 +0100	[thread overview]
Message-ID: <20211201105045.41228d82@thinkpad> (raw)
In-Reply-To: <20211201015308.GA2791148@bhelgaas>

On Tue, 30 Nov 2021 19:53:08 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, Nov 30, 2021 at 11:29:37AM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Nov 25, 2021 at 05:01:48PM +0100, Marek Behún wrote:  
> > > This reverts commit 239edf686c14a9ff926dec2f350289ed7adfefe2.
> > > 
> > > PCI Bridge which represents aardvark's PCIe Root Port has Expansion ROM
> > > Base Address register at offset 0x30, but its meaning is different than
> > > PCI's Expansion ROM BAR register, although the layout is the same.
> > > (This is why we thought it does the same thing.)
> > > 
> > > First: there is no ROM (or part of BootROM) in the A3720 SOC dedicated
> > > for PCIe Root Port (or controller in RC mode) containing executable code
> > > that would initialize the Root Port, suitable for execution in
> > > bootloader (this is how Expansion ROM BAR is used on x86).
> > > 
> > > Second: in A3720 spec the register (address D0070030) is not documented
> > > at all for Root Complex mode, but similar to other BAR registers, it has
> > > an "entangled partner" in register D0075920, which does address
> > > translation for the BAR in D0070030:
> > > - the BAR register sets the address from the view of PCIe bus
> > > - the translation register sets the address from the view of the CPU
> > > 
> > > The other BAR registers also have this entangled partner, and they
> > > can be used to:
> > > - in RC mode: address-checking on the receive side of the RC (they
> > >   can define address ranges for memory accesses from remote Endpoints
> > >   to the RC)
> > > - in Endpoint mode: allow the remote CPU to access memory on A3720
> > > 
> > > The Expansion ROM BAR has only the Endpoint part documented, but from
> > > the similarities we think that it can also be used in RC mode in that
> > > way.
> > > 
> > > So either Expansion ROM BAR has different meaning (if the hypothesis
> > > above is true), or we don't know it's meaning (since it is not
> > > documented for RC mode).
> > > 
> > > Remove the register from the emulated bridge accessing functions.
> > > 
> > > Fixes: 239edf686c14 ("PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge")
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 9 ---------
> > >  1 file changed, 9 deletions(-)  
> > 
> > Bjorn,
> > 
> > this reverts a commit we merged the last merge window so it is
> > a candidate for one of the upcoming -rcX.  
> 
> Sure, happy to apply the revert.
> 
> What problem does the revert fix?  I assume 239edf686c14 ("PCI:
> aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge") broke
> something, but the commit log for the revert doesn't say *what*.  How
> would one notice that something broke?

Hello Bjorn,

It doesn't break any real functionality that I know of, although it
might, since the register is read pci/probe.c pci_setup_device()
(pci_read_bases()).

But allowing the access to the register when it has different meaning
is wrong in a similar sense that a memory leak is wrong (a memory leak
also does not necessarily cause real problems, if it is small, but
still we should fix it).

Marek