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).
Thanks,
Nish
--
Nishanth Aravamudan <nacc@xxxxxxxxxx>
IBM Linux Technology Center
--
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
