On Fri, Sep 18, 2015 at 11:30:59PM +0100, Bjorn Helgaas wrote:
> On Thu, Sep 10, 2015 at 10:33:48AM +0100, Lorenzo Pieralisi wrote:
> > Commit 8505e729a2f6eb ("PCI: Add pci_claim_bridge_resource() to clip
> > window if necessary") introduced a new API to claim bridge resources.
> > pci_claim_bridge_resource() tries to claim a bridge resource, and if
> > the claiming fails the function tries to clip the resource to make
> > it fit within the parent resource window.
> > If the clipping succeeds the bridge aperture is set-up accordingly
> > and pci_claim_bridge_resource() tries to claim the resource again.
> >
> > Commit c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") added
> > code that sets the IORESOURCE_UNSET flag on claiming failure.
> >
> > This means that the second resource claiming after window clipping will
> > always fail, since the resource flags contain IORESOURCE_UNSET,
> > previously set on failure by pci_claim_resource(), so the subsequent
> > pci_claim_resource() call ends up spitting a log message and return
> > failure with no chance whatsoever to succeed.
> >
> > This patch clears the IORESOURCE_UNSET in the bridge resource flags
> > after clipping the bridge window successfully, so that the subsequent
> > pci_claim_resource() has a chance to succeed.
> >
> > Fixes: c770cb4cb505 ("PCI: Mark invalid BARs as unassigned")
> > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # 4.1+
> > ---
> > drivers/pci/setup-bus.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 508cc56..6de55d0 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -733,6 +733,13 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
> > return -EINVAL;
> > }
> >
> > + /*
> > + * Clear the IORESOURCE_UNSET flag set by the previous
> > + * pci_claim_resource() failure so that the resource
> > + * claiming can actually be carried out
> > + */
> > + bridge->resource[i].flags &= ~IORESOURCE_UNSET;
> > +
> > if (pci_claim_resource(bridge, i) == 0)
> > return 0; /* claimed a smaller window */
> >
>
> Thanks, Lorenzo, this is definitely a problem. I propose the
> patch below instead because:
>
> - I like clearing IORESOURCE_UNSET at the point where we actually
> update res->start and res->end (which will also fix any other
> future callers of pci_bus_clip_resource()), and
>
> - Doing it there means the printk will show how we changed the
> addresses, not just the size change.
>
> Can you try this out and see if it also solves the problem you're
> seeing?
Thanks Bjorn, yes that makes sense and it solves the problem.
> I wasn't sure about the provenance of your patch, Lorenzo. You had a
> Signed-off-by from Yinghai, but I didn't see the original posting. If
> it originally came from Yinghai, I should change the
> "Based-on-patch-by" below.
As Yinghai mentioned, I posted a patch to show that the second resource
claim in pci_claim_bridge_resource() was basically dead code in mainline,
to get your attention and obviously I knew what the real fix was,
I mentioned that when I inlined the patch.
You can keep patch below as is or give Yinghai Based-on-patch-by
accreditation and add a reported-by with my tag, I do not care as long
as we fix it.
You can also add:
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
Thanks,
Lorenzo
> commit a4ad03352739c96842af5d06387595665cdd875e
> Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Date: Fri Sep 18 17:15:01 2015 -0500
>
> PCI: Clear IORESOURCE_UNSET when clipping a bridge window
>
> c770cb4cb505 ("PCI: Mark invalid BARs as unassigned") sets IORESOURCE_UNSET
> if we fail to claim a resource. If we tried to claim a bridge window,
> failed, clipped the window, and tried to claim the clipped window, we
> failed again because of IORESOURCE_UNSET.
>
> When pci_bus_clip_resource() clips a bridge window to fit inside an
> upstream window, we're reassigning the window, so clear the
> IORESOURCE_UNSET flag. Also clear IORESOURCE_UNSET in our copy of the
> unclipped window so we can see exactly what the original window was and how
> it now fits inside the upstream window.
>
> Fixes: c770cb4cb505 ("PCI: Mark invalid BARs as unassigned")
> Based-on-patch-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> CC: stable@xxxxxxxxxxxxxxx # 4.1+
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 6fbd3f2..d3346d2 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -256,6 +256,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
>
> res->start = start;
> res->end = end;
> + res->flags &= ~IORESOURCE_UNSET;
> + orig_res.flags &= ~IORESOURCE_UNSET;
> dev_printk(KERN_DEBUG, &dev->dev, "%pR clipped to %pR\n",
> &orig_res, res);
>
>
--
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
