| Message ID | 1306186597-15080-2-git-send-email-seth.forshee@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 05/23/2011 02:36 PM, Seth Forshee wrote: > From: Linus Torvalds<torvalds@linux-foundation.org> > > BugLink: http://bugs.launchpad.net/bugs/424142 > > I'm not entirely sure it needs to go into 32, but it's probably the right > thing to do. Another way of explaining the patch is: > > - we currently pick the _first_ exactly matching bus resource entry, but > the _last_ inexactly matching one. Normally first/last shouldn't > matter, but bus resource entries aren't actually all created equal: in > a transparent bus, the last resources will be the parent resources, > which we should generally try to avoid unless we have no choice. So > "first matching" is the thing we should always aim for. > > - the patch is a bit bigger than it needs to be, because I simplified the > logic at the same time. It used to be a fairly incomprehensible > > if ((res->flags& IORESOURCE_PREFETCH)&& !(r->flags& IORESOURCE_PREFETCH)) > best = r; /* Approximating prefetchable by non-prefetchable */ > > and technically, all the patch did was to make that complex choice be > even more complex (it basically added a "&& !best" to say that if we > already gound a non-prefetchable window for the prefetchable resource, > then we won't override an earlier one with that later one: remember > "first matching"). > > - So instead of that complex one with three separate conditionals in one, > I split it up a bit, and am taking advantage of the fact that we > already handled the exact case, so if 'res->flags' has the PREFETCH > bit, then we already know that 'r->flags' will _not_ have it. So the > simplified code drops the redundant test, and does the new '!best' test > separately. It also uses 'continue' as a way to ignore the bus > resource we know doesn't work (ie a prefetchable bus resource is _not_ > acceptable for anything but an exact match), so it turns into: > > /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > if (r->flags& IORESOURCE_PREFETCH) > continue; > /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > if (!best) > best = r; > > instead. With the comments, it's now six lines instead of two, but it's > conceptually simpler, and I _could_ have written it as two lines: > > if ((res->flags& IORESOURCE_PREFETCH)&& !best) > best = r; /* Approximating prefetchable by non-prefetchable */ > > but I thought that was too damn subtle. > > Signed-off-by: Linus Torvalds<torvalds@linux-foundation.org> > Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org> > (cherry picked from commit 8c8def26bfaa704db67d515da3eb92cf26067548) > > Signed-off-by: Seth Forshee<seth.forshee@canonical.com> > --- > drivers/pci/pci.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 812d4ac..0d3326d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -373,8 +373,12 @@ pci_find_parent_resource(const struct pci_dev *dev, struct resource *res) > continue; /* Wrong type */ > if (!((res->flags ^ r->flags)& IORESOURCE_PREFETCH)) > return r; /* Exact match */ > - if ((res->flags& IORESOURCE_PREFETCH)&& !(r->flags& IORESOURCE_PREFETCH)) > - best = r; /* Approximating prefetchable by non-prefetchable */ > + /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > + if (r->flags& IORESOURCE_PREFETCH) > + continue; > + /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > + if (!best) > + best = r; > } > return best; > } For Lucid: Acked-by: Brad Figg <brad.figg@canonical.com>
On 05/23/2011 02:36 PM, Seth Forshee wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > BugLink: http://bugs.launchpad.net/bugs/424142 > > I'm not entirely sure it needs to go into 32, but it's probably the right > thing to do. Another way of explaining the patch is: > > - we currently pick the _first_ exactly matching bus resource entry, but > the _last_ inexactly matching one. Normally first/last shouldn't > matter, but bus resource entries aren't actually all created equal: in > a transparent bus, the last resources will be the parent resources, > which we should generally try to avoid unless we have no choice. So > "first matching" is the thing we should always aim for. > > - the patch is a bit bigger than it needs to be, because I simplified the > logic at the same time. It used to be a fairly incomprehensible > > if ((res->flags & IORESOURCE_PREFETCH) && !(r->flags & IORESOURCE_PREFETCH)) > best = r; /* Approximating prefetchable by non-prefetchable */ > > and technically, all the patch did was to make that complex choice be > even more complex (it basically added a "&& !best" to say that if we > already gound a non-prefetchable window for the prefetchable resource, > then we won't override an earlier one with that later one: remember > "first matching"). > > - So instead of that complex one with three separate conditionals in one, > I split it up a bit, and am taking advantage of the fact that we > already handled the exact case, so if 'res->flags' has the PREFETCH > bit, then we already know that 'r->flags' will _not_ have it. So the > simplified code drops the redundant test, and does the new '!best' test > separately. It also uses 'continue' as a way to ignore the bus > resource we know doesn't work (ie a prefetchable bus resource is _not_ > acceptable for anything but an exact match), so it turns into: > > /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > if (r->flags & IORESOURCE_PREFETCH) > continue; > /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > if (!best) > best = r; > > instead. With the comments, it's now six lines instead of two, but it's > conceptually simpler, and I _could_ have written it as two lines: > > if ((res->flags & IORESOURCE_PREFETCH) && !best) > best = r; /* Approximating prefetchable by non-prefetchable */ > > but I thought that was too damn subtle. > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > (cherry picked from commit 8c8def26bfaa704db67d515da3eb92cf26067548) > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> As has been already pointed out, for Lucid Acked-by: John Johansen <john.johansen@canonical.com> > --- > drivers/pci/pci.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 812d4ac..0d3326d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -373,8 +373,12 @@ pci_find_parent_resource(const struct pci_dev *dev, struct resource *res) > continue; /* Wrong type */ > if (!((res->flags ^ r->flags) & IORESOURCE_PREFETCH)) > return r; /* Exact match */ > - if ((res->flags & IORESOURCE_PREFETCH) && !(r->flags & IORESOURCE_PREFETCH)) > - best = r; /* Approximating prefetchable by non-prefetchable */ > + /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > + if (r->flags & IORESOURCE_PREFETCH) > + continue; > + /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > + if (!best) > + best = r; > } > return best; > }
Applied to Lucid On Mon, 2011-05-23 at 16:36 -0500, Seth Forshee wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > BugLink: http://bugs.launchpad.net/bugs/424142 > > I'm not entirely sure it needs to go into 32, but it's probably the right > thing to do. Another way of explaining the patch is: > > - we currently pick the _first_ exactly matching bus resource entry, but > the _last_ inexactly matching one. Normally first/last shouldn't > matter, but bus resource entries aren't actually all created equal: in > a transparent bus, the last resources will be the parent resources, > which we should generally try to avoid unless we have no choice. So > "first matching" is the thing we should always aim for. > > - the patch is a bit bigger than it needs to be, because I simplified the > logic at the same time. It used to be a fairly incomprehensible > > if ((res->flags & IORESOURCE_PREFETCH) && !(r->flags & IORESOURCE_PREFETCH)) > best = r; /* Approximating prefetchable by non-prefetchable */ > > and technically, all the patch did was to make that complex choice be > even more complex (it basically added a "&& !best" to say that if we > already gound a non-prefetchable window for the prefetchable resource, > then we won't override an earlier one with that later one: remember > "first matching"). > > - So instead of that complex one with three separate conditionals in one, > I split it up a bit, and am taking advantage of the fact that we > already handled the exact case, so if 'res->flags' has the PREFETCH > bit, then we already know that 'r->flags' will _not_ have it. So the > simplified code drops the redundant test, and does the new '!best' test > separately. It also uses 'continue' as a way to ignore the bus > resource we know doesn't work (ie a prefetchable bus resource is _not_ > acceptable for anything but an exact match), so it turns into: > > /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > if (r->flags & IORESOURCE_PREFETCH) > continue; > /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > if (!best) > best = r; > > instead. With the comments, it's now six lines instead of two, but it's > conceptually simpler, and I _could_ have written it as two lines: > > if ((res->flags & IORESOURCE_PREFETCH) && !best) > best = r; /* Approximating prefetchable by non-prefetchable */ > > but I thought that was too damn subtle. > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > (cherry picked from commit 8c8def26bfaa704db67d515da3eb92cf26067548) > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > drivers/pci/pci.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 812d4ac..0d3326d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -373,8 +373,12 @@ pci_find_parent_resource(const struct pci_dev *dev, struct resource *res) > continue; /* Wrong type */ > if (!((res->flags ^ r->flags) & IORESOURCE_PREFETCH)) > return r; /* Exact match */ > - if ((res->flags & IORESOURCE_PREFETCH) && !(r->flags & IORESOURCE_PREFETCH)) > - best = r; /* Approximating prefetchable by non-prefetchable */ > + /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ > + if (r->flags & IORESOURCE_PREFETCH) > + continue; > + /* .. but we can put a prefetchable resource inside a non-prefetchable one */ > + if (!best) > + best = r; > } > return best; > } > -- > 1.7.0.4 > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 812d4ac..0d3326d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -373,8 +373,12 @@ pci_find_parent_resource(const struct pci_dev *dev, struct resource *res) continue; /* Wrong type */ if (!((res->flags ^ r->flags) & IORESOURCE_PREFETCH)) return r; /* Exact match */ - if ((res->flags & IORESOURCE_PREFETCH) && !(r->flags & IORESOURCE_PREFETCH)) - best = r; /* Approximating prefetchable by non-prefetchable */ + /* We can't insert a non-prefetch resource inside a prefetchable parent .. */ + if (r->flags & IORESOURCE_PREFETCH) + continue; + /* .. but we can put a prefetchable resource inside a non-prefetchable one */ + if (!best) + best = r; } return best; }