On Nov 25, 2024 / 13:02, Andy Shevchenko wrote:
> 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.
Okay, will rename it.
>
> > + 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?
I see, will move the two pci_bus_write_config_dword() calls to
p2sb_scan_and_cache().
>
> > - /* 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).
I see, I will try it with v3.
>
> ...
>
> > - 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.
Okay, will do. Thanks for the comments.
