On Mon, Feb 20, 2023 at 01:53:34PM +0100, Niklas Schnelle wrote:
> On Fri, 2023-02-17 at 17:15 -0600, Bjorn Helgaas wrote:
> > On Tue, Feb 14, 2023 at 10:49:10AM +0100, Niklas Schnelle wrote:
> > > ...
> > What happens when zpci_bus_release() calls
> > pci_free_resource_list() on &zbus->resources? It looks like that
> > ultimately calls kfree(), which is OK for the
> > zpci_setup_bus_resources() stuff, but what about the
> > zbus->bus_resource that was not kalloc'ed?
>
> As far as I can see pci_free_resource_list() only calls kfree() on the
> entry not on entry->res. The resources set up in
> zpci_setup_bus_resources() are freed in zpci_cleanup_bus_resources()
> explicitly.
So I guess the zbus->resources are allocated in zpci_bus_scan_device()
where zpci_setup_bus_resources() adds a zbus resource for every
zpci_dev BAR, and freed in zpci_bus_release() when the last zpci_dev
is unregistered.
Does that mean that if you add device A, add device B, and remove A,
the zbus retains A's resources even though A is gone? What if you
then add device C whose resources partially overlap A's?
> > > static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
> > > {
> > > + struct resource *res;
> > > int i;
> > >
> > > + pci_lock_rescan_remove();
> >
> > What exactly is this protecting? This doesn't seem like quite the
> > right place since we're not adding/removing a pci_dev here. Is this
> > to protect the bus->resources list in pci_bus_remove_resource()?
>
> Yes I did not find a lock that is specifically for bus->resources but
> it seemed to me that changes to resources would only affect things
> running under the rescan/remove lock.
Yeah, OK.
> > > for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > > - if (!zdev->bars[i].size || !zdev->bars[i].res)
> > > + res = zdev->bars[i].res;
> > > + if (!res)
> > > continue;
> > >
> > > + release_resource(res);
> > > + pci_bus_remove_resource(zdev->zbus->bus, res);
> > > zpci_free_iomap(zdev, zdev->bars[i].map_idx);
> > > - release_resource(zdev->bars[i].res);
> > > - kfree(zdev->bars[i].res);
> > > + zdev->bars[i].res = NULL;
> > > + kfree(res);
> > > }
> > > zdev->has_resources = 0;
> > > + pci_unlock_rescan_remove();
