Re: [PATCH] Calgary: Find nearest matching Calgary while walkingup the PCI tree

[Darrick J. Wong]

On Thu, Dec 03, 2009 at 08:49:48AM +0200, Muli Ben-Yehuda wrote:
> On Wed, Dec 02, 2009 at 05:51:19PM -0600, Jon Mason wrote:
> 
> > > diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
> > > index 971a3be..e6ec8a2 100644
> > > --- a/arch/x86/kernel/pci-calgary_64.c
> > > +++ b/arch/x86/kernel/pci-calgary_64.c
> > > @@ -318,13 +318,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
> > >
> > >        pdev = to_pci_dev(dev);
> > >
> > > +       /* search up the device tree for an iommu */
> > >        pbus = pdev->bus;
> > > -
> > > -       /* is the device behind a bridge? Look for the root bus */
> > > -       while (pbus->parent)
> > > +       do {
> > > +               tbl = pci_iommu(pbus);
> > > +               if (tbl && tbl->it_busno == pbus->number)
> > > +                       break;
> > > +               tbl = NULL;
> > 
> > I believe the NULL assignment is unnecessary.  If not, then the if
> > check and BUG_ON are busted.
> 
> I think the NULL assignment is needed for the case where the loop ends
> (pbus is NULL) and we did not find the right tbl. You don't want to
> leave the tbl pointing to the last tbl we saw while walking the bus.

Correct.  I suspect that you'd only hit this situation in a fairly pathological
corner case, but configuring the wrong Calgary for IO could (in theory) be used
to breach the Calgary protections, whereas failing to set up the necessary
permissions will simply crash the system.  In any case it seems cleaner to me to
throw away the tbl pointer once we've decided that we're done with it.

--D
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/