On Mon, Nov 25, 2024 at 01:23:26PM +0900, Shin'ichiro Kawasaki wrote:
> When drivers access P2SB device resources, it calls p2sb_bar(). Before
> the commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
> during PCI device probe"), p2sb_bar() obtained the resources and then
> called pci_stop_and_remove_bus_device() for clean up. Then the P2SB
> device disappeared. The commit 5913320eb0b3 introduced the P2SB device
> resource cache feature in the boot process. During the resource cache,
> pci_stop_and_remove_bus_device() is called for the P2SB device, then the
> P2SB device disappears regardless of whether p2sb_bar() is called or
> not. Such P2SB device disappearance caused a confusion [1]. To avoid the
> confusion, avoid the pci_stop_and_remove_bus_device() call when the BIOS
> does not hide the P2SB device.
>
> For that purpose, cache the P2SB device resources only if the P2SB
> device is hidden. When p2sb_cache_resources() is called, check if the
> device is hidden and store the result in the global flag p2sb_hidden.
> Check the flag in p2sb_bar() and if the device is hidden, refer to the
> cached resources. Otherwise, read the resources from the unhidden P2SB
> device.
...
> +static bool p2sb_hidden;
I would call it p2sb_was_hidden or p2sb_hidden_by_bios.
> + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> + pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
We already pass the devfn_p2sb to that call, perhaps you can simply move these
configuration writes there?
> - /* Hide the P2SB device, if it was hidden */
> - if (value & P2SBC_HIDE)
> - pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
Thinking of how this change is done, I'm wondering if we better split it to
some preliminary refactoring to make it clearer of what's going on on each
step. Note, it's not a problem to have a series of patches to fix something and
it does not rarely occur (I believe Hans had done this many times in the past).
...
> - cache = &p2sb_resources[PCI_FUNC(devfn)];
> - if (cache->bus_dev_id != bus->dev.id)
> - return -ENODEV;
> + /*
> + * If the P2SB device is hidden, refer to the cached resources.
> + * Otherwise, read the resources on the fly.
> + */
> + if (p2sb_hidden) {
> + cache = &p2sb_resources[PCI_FUNC(devfn)];
> + if (cache->bus_dev_id != bus->dev.id)
> + return -ENODEV;
>
> - if (!p2sb_valid_resource(&cache->res))
> - return -ENOENT;
> + if (!p2sb_valid_resource(&cache->res))
> + return -ENOENT;
>
> - memcpy(mem, &cache->res, sizeof(*mem));
> - return 0;
> + memcpy(mem, &cache->res, sizeof(*mem));
> + } else {
> + pdev = pci_get_slot(bus, devfn);
> + if (!pdev)
> + return -ENODEV;
> +
> + if (p2sb_valid_resource(pci_resource_n(pdev, 0)))
> + p2sb_read_bar0(pdev, mem);
> + else
> + ret = -ENOENT;
> +
> + pci_dev_put(pdev);
> + }
Can you split these two branches to two helper functions. In the result it will
look better in my opinion.
--
With Best Regards,
Andy Shevchenko
