On Nov 20, 2024 / 15:43, Andy Shevchenko wrote:
> On Wed, Nov 20, 2024 at 03:40:55PM +0900, Shin'ichiro Kawasaki wrote:
>
> The subject probably wants to say "unhidden".
Oops, thanks.
>
> > When drivers access P2SB device resources, it calls p2sb_bar() to obtain
> > the resources. 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 even when the BIOS does not hide
> > the P2SB device. The commit introduced the P2SB device resource cache
> > feature in the boot process. During the resource cache process,
> > 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, add a new helper function p2sb_read_and_cache(), which
> > does the same work as p2sb_scan_and_cache() but does not call
> > pci_stop_and_remove_bus_device(). These two functions are almost same
> > except the device scan and remove. Then make them call the single
> > function p2sb_cache_devfn(), which takes the argument "bool scan".
> >
> > If the BIOS does not hide the P2SB device, just call
> > p2sb_read_and_cache() to cache the resources. If not, do additional
> > preparations (lock "rescan remove" for parallel scan, and unhide the
> > P2SB device), then call p2sb_scan_and_cache().
>
> Even for the simple read case we have to do that under rescan lock.
> Yes, it might be just a theoretical issue, but that's how makes code
> robust against possible enumeration changes at boot time.
Got it, thanks.
>
> ...
>
> > -static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> > +static int p2sb_cache_devfn(struct pci_bus *bus, unsigned int devfn, bool scan)
> > {
> > - /* Scan the P2SB device and cache its BAR0 */
> > - p2sb_scan_and_cache_devfn(bus, devfn);
> > + /* Scan or read the P2SB device and cache its BAR0 */
> > + __p2sb_cache_devfn(bus, devfn, scan);
>
> Strictly speaking we don't need to cache values when the device is unhidden.
> Moreover, the rescan can happen in between and the resource relocation to
> another place, which makes cached value invalid.
I see. I will create and post v2 patch based on your comments.
