On Sun, Nov 24, 2019 at 07:37:01AM -0700, Warner Losh wrote: > On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov <kostik...@gmail.com> > wrote: > > > On Sat, Nov 23, 2019 at 11:43:52PM +0000, Warner Losh wrote: > > > Author: imp > > > Date: Sat Nov 23 23:43:52 2019 > > > New Revision: 355037 > > > URL: https://svnweb.freebsd.org/changeset/base/355037 > > > > > > Log: > > > Push Giant down one layer > > > > > > The /dev/pci device doesn't need GIANT, per se. However, one routine > > > that it calls, pci_find_dbsf implicitly does. It walks a list that can > > > change when PCI scans a new bus. With hotplug, this means we could > > > have a race with that scanning. To prevent that, take out Giant around > > > scanning the list. > > > > > > However, given that we have places in the tree that drop giant, if > > > held when we call into them, the whole use of Giant to protect newbus > > > may be less effective that we desire, so add a comment about why we're > > > talking it out, and we'll address the issue when we lock newbus with > > > something other than Giant. > > > > > > Modified: > > > head/sys/dev/pci/pci.c > > > head/sys/dev/pci/pci_user.c > > > > > > Modified: head/sys/dev/pci/pci.c > > > > > ============================================================================== > > > --- head/sys/dev/pci/pci.c Sat Nov 23 23:41:21 2019 (r355036) > > > +++ head/sys/dev/pci/pci.c Sat Nov 23 23:43:52 2019 (r355037) > > > @@ -445,18 +445,21 @@ pci_find_bsf(uint8_t bus, uint8_t slot, uint8_t > > func) > > > device_t > > > pci_find_dbsf(uint32_t domain, uint8_t bus, uint8_t slot, uint8_t func) > > > { > > > - struct pci_devinfo *dinfo; > > > + struct pci_devinfo *dinfo = NULL; > > > > > > + /* Giant because newbus is Giant locked revisit with newbus > > locking */ > > > + mtx_lock(&Giant); > > > STAILQ_FOREACH(dinfo, &pci_devq, pci_links) { > > > if ((dinfo->cfg.domain == domain) && > > > (dinfo->cfg.bus == bus) && > > > (dinfo->cfg.slot == slot) && > > > (dinfo->cfg.func == func)) { > > > - return (dinfo->cfg.dev); > > > + break; > > > } > > > } > > > + mtx_unlock(&Giant); > > > > > > - return (NULL); > > > + return (dinfo != NULL ? dinfo->cfg.dev : NULL); > > I do not think this change is correct. If the parallel hotplug, or > > rather, hot-unplug event occurs, then dinfo potentially becomes invalid > > right after the Giant unlock, which makes both this function and its > > callers to access freed memory. Having caller to lock a newbus lock > > around both the call and consumption of the returned data is required. > > > There are many data lifetime issues. If anything the PCI user device calls > drops Giant and then picks it back up again we are in the same boat... I > totally agree this is a bad situation, but can only really be fixed by > locking newbus with a different lock than Giant and likely using some kind > of reference count for device_t that are handed out... > > In the mean time, I'll move giant back up into the ioctl routine and hope > it isn't dropped by things it calls..m
I think we can start at least by marking the Giant acqusitions that are related to newbus. I never saw anybody talking publically why the naive translation of newbus Giant into a sleepable lock, e.g. sx in exclusive mode, cannot work. From my memory, one of the big issues is that many sleeps done at probe/attach, need to drop the newbus lock. _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
