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.
Bjorn
--
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
