Re: [bug report] PCI: Protect pci_reassign_bridge_resources() against addition/removal

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Sep 09, 2019 at 09:13:28AM +0100, Benjamin Herrenschmidt wrote:
> On Wed, 2019-09-04 at 13:03 +0300, Dan Carpenter wrote:
> > Hello Benjamin Herrenschmidt,
> > 
> > The patch 540f62d26f02: "PCI: Protect pci_reassign_bridge_resources()
> > against addition/removal" from Jun 24, 2019, leads to the following
> > static checker warning:
> > 
> > 	drivers/pci/setup-bus.c:2158 pci_reassign_bridge_resources()
> > 	warn: inconsistent returns 'read_sem:&pci_bus_sem'.
> 
> Thanks, I missed that. I'll try to send something but from Plumbers
> might be difficult...

I dropped this for now.

> > drivers/pci/setup-bus.c
> >   2066          unsigned int i;
> >   2067          int ret;
> >   2068  
> >   2069          down_read(&pci_bus_sem);
> > 
> > We added some new locking here.
> > 
> >   2070  
> >   2071          /* Walk to the root hub, releasing bridge BARs when
> > possible */
> >   2072          next = bridge;
> >   2073          do {
> >   2074                  bridge = next;
> >   2075                  for (i = PCI_BRIDGE_RESOURCES; i <
> > PCI_BRIDGE_RESOURCE_END;
> >   2076                       i++) {
> >   2077                          struct resource *res = &bridge-
> > >resource[i];
> >   2078  
> >   2079                          if ((res->flags ^ type) &
> > PCI_RES_TYPE_MASK)
> >   2080                                  continue;
> >   2081  
> >   2082                          /* Ignore BARs which are still in use
> > */
> >   2083                          if (res->child)
> >   2084                                  continue;
> >   2085  
> >   2086                          ret = add_to_list(&saved, bridge,
> > res, 0, 0);
> >   2087                          if (ret)
> >   2088                                  goto cleanup;
> >   2089  
> >   2090                          pci_info(bridge, "BAR %d: releasing
> > %pR\n",
> >   2091                                   i, res);
> >   2092  
> >   2093                          if (res->parent)
> >   2094                                  release_resource(res);
> >   2095                          res->start = 0;
> >   2096                          res->end = 0;
> >   2097                          break;
> >   2098                  }
> >   2099                  if (i == PCI_BRIDGE_RESOURCE_END)
> >   2100                          break;
> >   2101  
> >   2102                  next = bridge->bus ? bridge->bus->self :
> > NULL;
> >   2103          } while (next);
> >   2104  
> >   2105          if (list_empty(&saved))
> >   2106                  return -ENOENT;
> > 
> > This needs an unlock.  It's not clear to me if any other clean up is
> > required, but possibly it's worth looking at.
> > 
> >   2107  
> >   2108          __pci_bus_size_bridges(bridge->subordinate, &added);
> >   2109          __pci_bridge_assign_resources(bridge, &added,
> > &failed);
> > 
> > regards,
> > dan carpenter
>