On Mon, Sep 26, 2011 at 07:28:56PM -0700, Nishanth Aravamudan wrote:
> On 23.09.2011 [16:09:22 -0600], Bjorn Helgaas wrote:
> > On Fri, Sep 23, 2011 at 2:47 PM, Nishanth Aravamudan <nacc@xxxxxxxxxx> wrote:
> > > On 23.09.2011 [13:46:38 -0700], Nishanth Aravamudan wrote:
> > >> On 21.09.2011 [17:58:49 -0600], Bjorn Helgaas wrote:
> > >> > On Tue, Sep 20, 2011 at 6:48 PM, Nishanth Aravamudan <nacc@xxxxxxxxxx> wrote:
> > >> > > Hi Jesse,
> > >> > >
> > >> > > We've gotten a few reports of the following situation: a SR-IOV capable
> > >> > > adapter in a ppc64 server (in some cases driven by the lpfc driver, in
> > >> > > others by the be2net driver, but I don't think it is driver specific)
> > >> > > fails to initialize due to a collision on BAR 7 (the first IOV
> > >> > > resource), e.g.:
> > >> > >
> > >> > > 0000:98:00.1: device not available (can't reserve [mem 0xfffe0000-0x1001dffff 64bit])
> > >> > >
> > >> > > I'm testing the following change:
> > >> > >
> > >> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > >> > > index 4e84fd4..17b651e 100644
> > >> > > --- a/drivers/pci/pci.c
> > >> > > +++ b/drivers/pci/pci.c
> > >> > > @@ -1126,9 +1126,14 @@ static int __pci_enable_device_flags(struct pci_dev *dev,
> > >> > > if (atomic_add_return(1, &dev->enable_cnt) > 1)
> > >> > > return 0; /* already enabled */
> > >> > >
> > >> > > - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> > >> > > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > >> > > +#ifdef CONFIG_PCI_IOV
> > >> > > + if (i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END)
> > >> > > + continue;
> > >> > > +#endif
> > >> > > if (dev->resource[i].flags & flags)
> > >> > > bars |= (1 << i);
> > >> > > + }
> > >> > >
> > >> > > err = do_pci_enable_device(dev, bars);
> > >> > > if (err < 0)
> > >> > >
> > >> > > With this change, the driver does load, although there do still appear
> > >> > > to be problems with upstream at that point that I'm still tracking down.
> > >> > >
> > >> > > The thinking is that it shouldn't be an error at this point in the code
> > >> > > if we fail to enable the IOV BARs as we're not enabling IOV here in the
> > >> > > first place. The failure point should be when the driver attempts to
> > >> > > create VFs if we can't use the IOV BARs.
> > >> > >
> > >> > > I have a few questions:
> > >> > >
> > >> > > 1) Does this make sense to you? :)
> > >> > >
> > >> > > 2) Presuming the fix above *isn't* ok, do you have thoughts on
> > >> > > a better approach? Keeping in mind that on power, we don't
> > >> > > control the device resource assignment, so we are a little more
> > >> > > stuck here, arguably.
> > >> > >
> > >> > > 3) pci_select_bars seems like it could be used by
> > >> > > __pci_enable_device_flags as a cleanup? Would the above change
> > >> > > be good to put there as well?
> > >> >
> > >> > I'm not Jesse (who's on vacation for a couple weeks), but this does
> > >> > make sense to me.
> > >> >
> > >> > The VF BARs don't consume resources until we set VF Enable and VF MSE,
> > >> > which happens in pci_enable_sriov(). I agree that the
> > >> > pci_enable_device() for the PF should succeed and that the PF should
> > >> > work as a normal non-SR-IOV device until the driver enables SR-IOV.
> > >> >
> > >> > It seems a bit weird that the IOV resources leaked out into struct
> > >> > pci_dev, resulting in this problem, #ifdefs like this to fix it, and
> > >> > wasting space in the pci_dev for every non-SR-IOV device. I suppose
> > >> > there's some reason they can't live in the struct pci_sriov?
> > >> >
> > >> > Good point about pci_select_bars(), too. That looks like another
> > >> > problem waiting to happen -- if a driver claiming the PF uses
> > >> > pci_select_bars(), then pci_request_selected_regions(), it will
> > >> > attempt to request the VF BARs, which it shouldn't.
> > >> >
> > >> > I think we should refactor so __pci_enable_device_flags() calls
> > >> > pci_select_bars(). If it's feasible to move the IOV resources out of
> > >> > the struct pci_dev, that would solve both problems. Otherwise, maybe
> > >> > just put your #ifdef in pci_select_bars().
> > >> >
> > >> > I'm assuming that this is post-3.1 material, right?
> > >
> > > Grr, meant to answer this, but hit send too quickly! Yes, this has been
> > > around for a while, but presuming the fix is acceptable, I think it
> > > would be good to send to the -stable trees where it applies (including
> > > 3.1 presumably).
> >
> > Did you look at whether it's possible to move those VF BARs into the
> > struct pci_sriov?
> >
> > It feels quite wasteful that every pci_dev contains a resource array
> > with space for 17 resources (6 standard, 1 ROM, 6 IOV, and 4 bridge
> > windows), when any given device requires at most 6.
>
> I still haven't gotten around to this -- however, given that this
> affects older versions too (i.e., as old as 2.6.29 when d1b054da went
> in which added PCI_IOV_RESOURCS), and presuming I'd like to see it fixed
> in some of those (e.g., 2.6.32), would it be possible to take my patch
> while I work on the more invasive change?
Nish,
Let me take a stab at this. Give me a week or so.
RP
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
