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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 19-Nov-24 3:20 AM, Shinichiro Kawasaki wrote:
> 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.

Thank you for looking into this.

Daniel can you give this a try? It should fix the regression you are seeing
without needing to rework your code (reworking your code to be cleaner
might still be a good idea though).

Shinichiro, can you turn this into a proper patch without all the debug
prints and maybe follow Andy's suggestion to just make this a separate
function.

Regards,

Hans




> 
> 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);