Re: platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Nov 18, 2024 / 17:15, Daniel Walker (danielwa) wrote:
> On Mon, Nov 18, 2024 at 05:00:52PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 18-Nov-24 4:55 PM, Andy Shevchenko wrote:
[...]
> > > Hans, there will be no need to fix anything if they implement correct access
> > > to the GPIO, i.e. via driver and board code with GPIO lookup tables.
> > 
> > Agreed, still I'm not sure how I feel about us hiding the previously unhidden P2SB.
> > 
> > OTOH I guess it may have only been unhidden in the BIOS to make the hack they
> > are using possible in the first place.
> 
> From a flexibility POV I would suggest if you can not hide it if it's not already
> hidden by the BIOS that would be better since some company may have a good
> reason to make a custom driver or to export the pci device to userspace thru
> UIO. The current situation is you can't make a custom driver if p2sb is enable
> with this additional patch even if you unhide the device inside the BIOS.
> 
> In our case it seems like we could use the already existing solution with
> pinctrl, but others may not be able to do that or may not want to for different
> reasons.

I don't have strong opinion about the choice, but I wonder how the p2sb code
will be if we keep the unhidden P2SB. I created a trial patch below. If the
device is not hidden, it does not call pci_scan_single_device() and
pci_stop_and_remove_bus_device(). Instead, it calls pci_get_slot() and
pci_dev_put(). I don't have the environment which unhides P2SB. Daniel, if you
have time to afford, please try it out.

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 31f38309b389..dec3d43ce929 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -79,29 +79,49 @@ static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
 	mem->desc = bar0->desc;
 }
 
-static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn)
+static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn,
+				      bool hidden)
 {
 	struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)];
-	struct pci_dev *pdev;
+	struct pci_dev *pdev = NULL;
+
+	if (!hidden)
+		pdev = pci_get_slot(bus, devfn);
+
+	pr_info("%s: hidden=%d pci_get_slot=%px\n", __func__, hidden, pdev);
+
+	if (!pdev) {
+		hidden = true;
+		pdev = pci_scan_single_device(bus, devfn);
+	}
 
-	pdev = pci_scan_single_device(bus, devfn);
 	if (!pdev)
 		return;
 
 	p2sb_read_bar0(pdev, &cache->res);
+
+	pr_info("%s: devfn=%x.%x\n", __func__,
+		PCI_SLOT(devfn), PCI_FUNC(devfn));
+	pr_info("%s: %llx-%llx: %lx\n", __func__,
+		cache->res.start, cache->res.end, cache->res.flags);
+
 	cache->bus_dev_id = bus->dev.id;
 
-	pci_stop_and_remove_bus_device(pdev);
+	if (hidden)
+		pci_stop_and_remove_bus_device(pdev);
+	else
+		pci_dev_put(pdev);
 }
 
-static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
+static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn,
+			       bool hidden)
 {
 	/* Scan the P2SB device and cache its BAR0 */
-	p2sb_scan_and_cache_devfn(bus, devfn);
+	p2sb_scan_and_cache_devfn(bus, devfn, hidden);
 
 	/* On Goldmont p2sb_bar() also gets called for the SPI controller */
 	if (devfn == P2SB_DEVFN_GOLDMONT)
-		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
+		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT, hidden);
 
 	if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
 		return -ENOENT;
@@ -127,9 +147,12 @@ static int p2sb_cache_resources(void)
 	unsigned int devfn_p2sb;
 	u32 value = P2SBC_HIDE;
 	struct pci_bus *bus;
+	bool hidden;
 	u16 class;
 	int ret;
 
+	pr_info("%s\n", __func__);
+
 	/* Get devfn for P2SB device itself */
 	p2sb_get_devfn(&devfn_p2sb);
 
@@ -157,13 +180,15 @@ static int p2sb_cache_resources(void)
 	 * Unhide the P2SB device here, if needed.
 	 */
 	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
-	if (value & P2SBC_HIDE)
+	hidden = value & P2SBC_HIDE;
+	pr_info("%s: P2SBC_HIDE=%u\n", __func__, value & hidden);
+	if (hidden)
 		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
 
-	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
+	ret = p2sb_scan_and_cache(bus, devfn_p2sb, hidden);
 
 	/* Hide the P2SB device, if it was hidden */
-	if (value & P2SBC_HIDE)
+	if (hidden)
 		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
 
 	pci_unlock_rescan_remove();
@@ -189,6 +214,8 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 {
 	struct p2sb_res_cache *cache;
 
+	pr_info("%s\n", __func__);
+
 	bus = p2sb_get_bus(bus);
 	if (!bus)
 		return -ENODEV;
@@ -204,6 +231,12 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 		return -ENOENT;
 
 	memcpy(mem, &cache->res, sizeof(*mem));
+
+	pr_info("%s: devfn=%x.%x\n", __func__,
+		PCI_SLOT(devfn), PCI_FUNC(devfn));
+	pr_info("%s: %llx-%llx: %lx\n", __func__,
+		cache->res.start, cache->res.end, cache->res.flags);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(p2sb_bar);