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?
How about something like the following?
pci: skip IOV BARs in __pci_enable_device_flags
We are seeing reports of resource collisions with SR-IOV capable
adapters on power, e.g.:
0000:98:00.1: device not available (can't reserve [mem 0xfffe0000-0x1001dffff 64bit])
This is because the generic PCI code is failing the device enable when a
VF collision occurs, even if no VFs are going to be used. Skip the VF
BARs when CONFIG_PCI_IOV in pci_select_bars and use the helper in
__pci_enable_device_flags.
Signed-off-by: Nishanth Aravamudan <nacc@xxxxxxxxxx>
---
This fixes the resource collision issues, but the SR-IOV capablea
adapters are still having issues with mainline, which I'm still tracking
down.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4e84fd4..4362964 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1109,7 +1109,7 @@ static int __pci_enable_device_flags(struct pci_dev *dev,
resource_size_t flags)
{
int err;
- int i, bars = 0;
+ int bars;
/*
* Power state could be unknown at this point, either due to a fresh
@@ -1126,10 +1126,7 @@ 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++)
- if (dev->resource[i].flags & flags)
- bars |= (1 << i);
-
+ bars = pci_select_bars(dev, flags);
err = do_pci_enable_device(dev, bars);
if (err < 0)
atomic_dec(&dev->enable_cnt);
@@ -3295,9 +3292,14 @@ out:
int pci_select_bars(struct pci_dev *dev, unsigned long flags)
{
int i, bars = 0;
- for (i = 0; i < PCI_NUM_RESOURCES; i++)
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+#ifdef CONFIG_PCI_IOV
+ if (i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END)
+ continue;
+#endif
if (pci_resource_flags(dev, i) & flags)
bars |= (1 << i);
+ }
return bars;
}
--
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
