Re: [PATCH v3 3/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support

From: Bjorn Helgaas <helgaas@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Wolfram Sang <wsa@kernel.org>, Jean Delvare <jdelvare@suse.de>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Tan Jui Nee <jui.nee.tan@intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Hans de Goede <hdegoede@redhat.com>, Kate Hsuan <hpa@redhat.com>,
	Jonathan Yong <jonathan.yong@intel.com>,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
	linux-pci@vger.kernel.org, linux-gpio@vger.kernel.org,
	platform-driver-x86@vger.kernel.org,
	Jean Delvare <jdelvare@suse.com>,
	Peter Tyser <ptyser@xes-inc.com>,
	Andy Shevchenko <andy@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Gross <markgross@kernel.org>,
	Henning Schild <henning.schild@siemens.com>
Subject: Re: [PATCH v3 3/8] platform/x86/intel: Add Primary to Sideband (P2SB) bridge support
Date: Wed, 2 Feb 2022 14:36:51 -0600	[thread overview]
Message-ID: <20220202203651.GA40446@bhelgaas> (raw)
In-Reply-To: <YfmBZvQ28y/Mh60J@smile.fi.intel.com>

On Tue, Feb 01, 2022 at 08:52:22PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 01, 2022 at 12:14:01PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 28, 2022 at 08:30:20PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jan 07, 2022 at 11:11:08AM -0600, Bjorn Helgaas wrote:
> > > > On Fri, Jan 07, 2022 at 04:56:42PM +0200, Andy Shevchenko wrote:
> > > 
> > > ...
> > > 
> > > > My point is that the unhide is architecturally messed up.  The OS
> > > > runs on the platform as described by ACPI.  Devices that cannot be
> > > > enumerated are described in the ACPI namespace.
> > > 
> > > This device may or may not be _partially_ or _fully_ (due to being
> > > multifunctional) described in ACPI. I agree, that ideally the
> > > devices in question it has behind should be represented properly by
> > > firmware.  However, the firmwares in the wild for selected products
> > > / devices don't do that. We need to solve (work around) it in the
> > > software.
> > > 
> > > This is already done for a few devices. This series consolidates
> > > that and enables it for very known GPIO IPs.
> > 
> > Consolidating the code to unhide the device and size the BAR is fine.
> > 
> > I would prefer the PCI core to be involved as little as possible
> > because we're violating some key assumptions and we could trip over
> > those later.  We're assuming the existence of P2SB based on the fact
> > that we found some *other* device, we're assuming firmware isn't using
> > P2SB (may be true now, but impossible to verify), we're assuming the
> > P2SB BAR contains a valid address that's not used elsewhere but also
> > won't be assigned to anything.
> > 
> > > PCI core just provides a code that is very similar to what we need
> > > here. Are you specifically suggesting that we have to copy'n'paste
> > > that rather long function and maintain in parallel with PCI?
> > 
> > I think we're talking about __pci_read_base(), which is currently an
> > internal PCI interface.  This series adds pci_bus_info/warn/etc(),
> 
> The patch that adds those macros is good on its own, if you think so...
> I tried to submit it separately, but it was no response, so I don't know.

I'd rather not add pci_bus_info(), etc.  There are only a couple
places that could use it, and if we cared enough, I think those places
could avoid it by doing pci_alloc_dev() first.

What if you used pci_alloc_dev() and then called the current
__pci_read_base() on the pci_dev *?

The caller would still have the ugly #include path, but I guess you're
OK with that.

Since the P2SB BAR is not included in the host bridge _CRS, the
pcibios_bus_to_resource() done by __pci_read_base() won't work
correctly, so this only "works" on host bridges with no address
translation.  But that's also the case with your current series.
This is an example of one of the PCI core assumptions being violated,
so things can break.

Bjorn