svn commit: r355037 - head/sys/dev/pci

[Warner Losh]

On Sun, Nov 24, 2019, 9:45 AM Konstantin Belousov <kostikbel at gmail.com>
wrote:

> On Sun, Nov 24, 2019 at 07:37:01AM -0700, Warner Losh wrote:
> > On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov <kostikbel at 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.
>

If we are holding a topology lock, we can't drop it... otherwise someone
else can acquire it and change things out from us...

Also, locks are useless for device_t lifetimes. Too many places cache them
in non obvious ways, so we need to add some kind of ref count. And since
these relationships are often not parent / child, we'd likely need a
spoilage call to release refs. You shouldn't complete your detaching if
others holding references.

Finally, we walk both up and down the tree. When I tried a super naive lock
in the past, I ran into a bunch of LORs due to this. And what do you do if
multiple nodes in the tree want to start a detach/delete at the same time?
Or if there is a suspend going on at the same time...

These are all solvable problems, but simply replacing giant with a sx lock
won't solve them and may introduce issues because giant is special in many
ways... it would, however, fix the one aspect of giant we don't want:
random parts of the kernel dropping it.

Warner

>