Re: [PATCH v5 1/2] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To: Klara Modin <klarasmodin@gmail.com>
Cc: "andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>,
	"ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"lukas@wunner.de" <lukas@wunner.de>,
	"platform-driver-x86@vger.kernel.org"
	<platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH v5 1/2] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe
Date: Thu, 4 Jan 2024 08:41:28 +0000	[thread overview]
Message-ID: <oe4cs5ptinmmdaxv6xa524whc7bppfqa7ern5jzc3aca5nffpm@xbmv34mjjxvv> (raw)
In-Reply-To: <CABq1_vjfyp_B-f4LAL6pg394bP6nDFyvg110TOLHHb0x4aCPeg@mail.gmail.com>

On Jan 03, 2024 / 22:31, Klara Modin wrote:
> Hi,
> 
> With this patch, ata_piix fails to detect my IDE controller (older P4
> Xeon board). Reverting this on top of 6.7-rc8 resolves the issue for
> me.

Thanks for the report. According the dmesg and lspci logs, it looks the IDE
controller has the PCI DEVFN 31:1 (0000:00:1f.1). So, I guess as follows:

- Klara's system has the old ICH (ICH4) which has IDE controller at DEVFN 31:1.
  This DEVFN is same as P2SB device that newer PCH Series 100 or later has.
- The system has ISA bridge 82801BA for LPC, then LPC_ICH driver is loaded.
  This driver depends on P2SB and calls p2sb_bar(). But p2sb_bar() is called
  only for new features of LPC_ICH driver (SPI flash and GPIO). Then p2sb_bar()
  is not called on the system, probably (I want to confirm it).
- Before the commit b28ff7a7c324 ("platform/x86: p2sb: Allow p2sb_bar() calls
  during PCI device probe"), the DEVFN 31:1 is accessed as P2SB only when
  p2sb_bar() is called. After the commit, if P2SB is enabled in Kconfig,
  DEVFN 31:1 is accessed as P2SB to cache P2SB resources. However, the device
  is not P2SB devcie but IDE controller, then it casued the IDE controller
  detection failure.

> 
> Please tell me if there's anything else you need. I'm willing to test
> any new patches.

Could you confirm that p2sb_bar() is not called during boot process on your
system? Applying the one liner printk patch below, let's confirm that the
string "p2sb_bar" does not appear in the dmesg log.

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 99a0e456f075..e034a58d7531 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -195,6 +195,7 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 	struct p2sb_res_cache *cache;
 	int ret;
 
+	printk("%s\n", __func__);
 	bus = p2sb_get_bus(bus);
 	if (!bus)
 		return -ENODEV;


If p2sb_bar() is not called on the system, I think P2SB device scan and resource
cache shall not run on such old systems. Before the commit 53eb64c88f17
("platform/x86: p2sb: Don't fail if unknown CPU is found"), p2sb.c had a
whitelist of Intel CPU IDs to access P2SB device. Its commit message indicates
that we can add blacklist of CPU IDs if needed. I think this blacklist will help
to avoid the failure.

So next question is: how to list the CPUs which do not need P2SB resource cache?
I don't have clear answer yet. According a P2SB document [1], P2SB support was
introduced since PCH Series 100, around 2010. Looking at ICH history [2], ICH9
removed PATA support, so I guess DEVFN 31:1 for IDE controller was removed since
ICH9, around 2007. So the end of the Intel CPU blacklist could be the CPU
introduced between 2007 and 2010.

[1] https://lab.whitequark.org/notes/2017-11-08/accessing-intel-ich-pch-gpios/
[2] https://en.wikipedia.org/wiki/I/O_Controller_Hub#ICH8

My mere idea was to just blacklist Intel CPUs with family != 6. If my guesses
are all correct, following patch will avoid the failure on the reported system,
since the system has CPU with family == 0xf. This will cover certain amount of
old CPUs which should not access DEVFN 31:1 as P2SB. But family 6 started around
2006, then it is not the complete blacklist, probably. I will think about the
better way.

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 99a0e456f075..c9bcef8e2c4c 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -46,6 +46,10 @@ static int p2sb_get_devfn(unsigned int *devfn)
 	unsigned int fn = P2SB_DEVFN_DEFAULT;
 	const struct x86_cpu_id *id;
 
+	/* ICH/PCHs for old CPUs do not have P2SB */
+	if (boot_cpu_data.x86 != 6)
+		return -ENODEV;
+
 	id = x86_match_cpu(p2sb_cpu_ids);
 	if (id)
 		fn = (unsigned int)id->driver_data;
