Re: PCI resource allocation mismatch with BIOS

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Nov 29, 2022 at 07:48:12AM +0100, Lukas Wunner wrote:
> On Mon, Nov 28, 2022 at 03:06:17PM -0700, Alex Williamson wrote:
> > Agreed.  Is this convoluted removal process being used to force a SBR,
> > versus a FLR or PM reset that might otherwise be used by twiddling the
> > reset attribute of the GPU directly?  If so, the reset_method attribute
> > can be used to force a bus reset and perform all the state save/restore
> > handling to avoid reallocating BARs.  A reset from the upstream switch
> > port would only be necessary if you have some reason to also reset the
> > switch downstream ports.  Thanks,
> 
> A Secondary Bus Reset is only offered as a reset_method if the
> device to be reset is the *only* child of the upstream bridge.
> I.e. if the device to be reset has siblings or children,
> a Secondary Bus Reset is not permitted.
> 
> Modern GPUs (including the one Mika is referring to) consist of
> a PCIe switch with the GPU, HD audio and telemetry devices below
> Downstream Bridges.  A Secondary Bus Reset of the Root Port is
> not allowed in this case because the Switch Upstream Port has
> children.
> 
> See this code in pci_parent_bus_reset():
> 
> 	if (pci_is_root_bus(dev->bus) || dev->subordinate ||
> 	    !dev->bus->self || dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
> 		return -ENOTTY;
> 
> The dev->subordinate check disallows a SBR if there are children.
> Note that the code should probably instead check for...
> (dev->subordinate && !list_empty(dev->subordinate->devices))
> ...because the port may have a subordinate bus without children
> (may have been removed for example).
> 
> The "no siblings" rule is enforced by:
> 
> 	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> 		if (pdev != dev)
> 			return -ENOTTY;
> 
> Note that the devices list is iterated without holding pci_bus_sem,
> which looks fishy.
> 
> That said, it *is* possible that a Secondary Bus Reset is erroneously
> offered despite these checks because we perform them early on device
> enumeration when the subordinate bus hasn't been scanned yet.

Thanks Lukas for the good explanation :) I think because of the above
our GPU folks do Secondary Bus Reset directly by poking the config space
through setpci or so, and I guess this is why they need the "remove"
phase as well.

> So if the Root Port offers other reset methods besides SBR and the
> user switches to one of them, then reinstates the defaults,
> suddenly SBR will disappear because the subordinate bus has since
> been scanned.  What's missing here is that we re-check availability
> of the reset methods on siblings and the parent when a device is
> added or removed.  This is also necessary to make reset_method
> work properly with hotplug.  However, the result may be that the
> reset_method attribute in sysfs may become invisible after adding
> a device (because there is no reset method available) and reappear
> after removing a device.
> 
> So the reset_method logic is pretty broken right now I'm afraid.
> 
> In any case, for Mika's use case it would be useful to have a
> "reset_subordinate" attribute on ports capable of a SBR such that
> the entire hierarchy below is reset.  The "reset" attribute is
> insufficient.

Yes, that would be useful indeed.