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?
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
