On Fri, Sep 28, 2018 at 11:42 AM Borislav Petkov <bp@xxxxxxx> wrote: > > On Thu, Sep 27, 2018 at 09:22:09AM -0500, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > > > Previously find_next_iomem_res() used "*res" as both an input parameter for > > the range to search and the type of resource to search for, and an output > > parameter for the resource we found, which makes the interface confusing. > > > > The current callers use find_next_iomem_res() incorrectly because they > > allocate a single struct resource and use it for repeated calls to > > find_next_iomem_res(). When find_next_iomem_res() returns a resource, it > > overwrites the start, end, flags, and desc members of the struct. If we > > call find_next_iomem_res() again, we must update or restore these fields. > > The previous code restored res.start and res.end, but not res.flags or > > res.desc. > > ... which is a sure sign that the design of this thing is not the best one. > > > > > Since the callers did not restore res.flags, if they searched for flags > > IORESOURCE_MEM | IORESOURCE_BUSY and found a resource with flags > > IORESOURCE_MEM | IORESOURCE_BUSY | IORESOURCE_SYSRAM, the next search would > > incorrectly skip resources unless they were also marked as > > IORESOURCE_SYSRAM. > > Nice example! > > > Fix this by restructuring the interface so it takes explicit "start, end, > > flags" parameters and uses "*res" only as an output parameter. > > > > Original-patch: http://lore.kernel.org/lkml/20180921073211.20097-2-lijiang@xxxxxxxxxx > > Based-on-patch-by: Lianbo Jiang <lijiang@xxxxxxxxxx> > > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > --- > > kernel/resource.c | 94 +++++++++++++++++++++++------------------------------ > > 1 file changed, 41 insertions(+), 53 deletions(-) > > > > diff --git a/kernel/resource.c b/kernel/resource.c > > index 155ec873ea4d..9891ea90cc8f 100644 > > --- a/kernel/resource.c > > +++ b/kernel/resource.c > > @@ -319,23 +319,26 @@ int release_resource(struct resource *old) > > EXPORT_SYMBOL(release_resource); > > > > /* > > I guess this could be made kernel-doc, since you're touching it... > > > - * Finds the lowest iomem resource existing within [res->start..res->end]. > > - * The caller must specify res->start, res->end, res->flags, and optionally > > - * desc. If found, returns 0, res is overwritten, if not found, returns -1. > > - * This function walks the whole tree and not just first level children until > > - * and unless first_level_children_only is true. > > + * Finds the lowest iomem resource that covers part of [start..end]. The > > + * caller must specify start, end, flags, and desc (which may be > > + * IORES_DESC_NONE). > > + * > > + * If a resource is found, returns 0 and *res is overwritten with the part > > + * of the resource that's within [start..end]; if none is found, returns > > + * -1. > > + * > > + * This function walks the whole tree and not just first level children > > + * unless first_level_children_only is true. > > ... and then prepend that with '@' - @first_level_children_only to refer > to the function parameter. > > > */ > > -static int find_next_iomem_res(struct resource *res, unsigned long desc, > > - bool first_level_children_only) > > +static int find_next_iomem_res(resource_size_t start, resource_size_t end, > > + unsigned long flags, unsigned long desc, > > + bool first_level_children_only, > > + struct resource *res) > > { > > - resource_size_t start, end; > > struct resource *p; > > bool sibling_only = false; > > > > BUG_ON(!res); > > - > > - start = res->start; > > - end = res->end; > > BUG_ON(start >= end); > > And since we're touching this, maybe replace that BUG_ON() fun with > simply return -EINVAL or some error code... > > > > > if (first_level_children_only) > > if (first_level_children_only) > sibling_only = true; > > So this is just silly - replacing a bool function param with a local bool > var. > > You could kill that, shorten first_level_children_only's name and use it > directly. > > Depending on how much cleanup it amounts to, you could make that a > separate cleanup patch ontop, to keep the changes from the cleanup > separate. > > > @@ -344,7 +347,7 @@ static int find_next_iomem_res(struct resource *res, unsigned long desc, > > read_lock(&resource_lock); > > > > for (p = iomem_resource.child; p; p = next_resource(p, sibling_only)) { > > - if ((p->flags & res->flags) != res->flags) > > + if ((p->flags & flags) != flags) > > continue; > > if ((desc != IORES_DESC_NONE) && (desc != p->desc)) > > continue; > > @@ -359,32 +362,31 @@ static int find_next_iomem_res(struct resource *res, unsigned long desc, > > read_unlock(&resource_lock); > > if (!p) > > return -1; > > + > > /* copy data */ > > - if (res->start < p->start) > > - res->start = p->start; > > - if (res->end > p->end) > > - res->end = p->end; > > + res->start = max(start, p->start); > > + res->end = min(end, p->end); > > Should we use the min_t and max_t versions here for typechecking? > > > res->flags = p->flags; > > res->desc = p->desc; > > return 0; > > } > > > > -static int __walk_iomem_res_desc(struct resource *res, unsigned long desc, > > - bool first_level_children_only, > > - void *arg, > > +static int __walk_iomem_res_desc(resource_size_t start, resource_size_t end, > > + unsigned long flags, unsigned long desc, > > + bool first_level_children_only, void *arg, > > int (*func)(struct resource *, void *)) > > { > > - u64 orig_end = res->end; > > + struct resource res; > > int ret = -1; > > > > - while ((res->start < res->end) && > > - !find_next_iomem_res(res, desc, first_level_children_only)) { > > - ret = (*func)(res, arg); > > + while (start < end && > > + !find_next_iomem_res(start, end, flags, desc, > > + first_level_children_only, &res)) { > > + ret = (*func)(&res, arg); > > if (ret) > > break; > > > > - res->start = res->end + 1; > > - res->end = orig_end; > > + start = res.end + 1; > > } > > > > return ret; > > @@ -407,13 +409,7 @@ static int __walk_iomem_res_desc(struct resource *res, unsigned long desc, > > int walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, > > u64 end, void *arg, int (*func)(struct resource *, void *)) > > Align that function's parameters on the opening brace, pls, while you're > at it. > > > { > > - struct resource res; > > - > > - res.start = start; > > - res.end = end; > > - res.flags = flags; > > - > > - return __walk_iomem_res_desc(&res, desc, false, arg, func); > > + return __walk_iomem_res_desc(start, end, flags, desc, false, arg, func); > > } > > EXPORT_SYMBOL_GPL(walk_iomem_res_desc); > > > > @@ -427,13 +423,9 @@ EXPORT_SYMBOL_GPL(walk_iomem_res_desc); > > int walk_system_ram_res(u64 start, u64 end, void *arg, > > int (*func)(struct resource *, void *)) > > Ditto. > > > { > > - struct resource res; > > - > > - res.start = start; > > - res.end = end; > > - res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > > + unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > > > > - return __walk_iomem_res_desc(&res, IORES_DESC_NONE, true, > > + return __walk_iomem_res_desc(start, end, flags, IORES_DESC_NONE, true, > > arg, func); > > } > > > > @@ -444,13 +436,9 @@ int walk_system_ram_res(u64 start, u64 end, void *arg, > > int walk_mem_res(u64 start, u64 end, void *arg, > > int (*func)(struct resource *, void *)) > > { > > - struct resource res; > > - > > - res.start = start; > > - res.end = end; > > - res.flags = IORESOURCE_MEM | IORESOURCE_BUSY; > > + unsigned long flags = IORESOURCE_MEM | IORESOURCE_BUSY; > > > > - return __walk_iomem_res_desc(&res, IORES_DESC_NONE, true, > > + return __walk_iomem_res_desc(start, end, flags, IORES_DESC_NONE, true, > > arg, func); > > } > > > > @@ -464,25 +452,25 @@ int walk_mem_res(u64 start, u64 end, void *arg, > > int walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages, > > void *arg, int (*func)(unsigned long, unsigned long, void *)) > > Ditto. > > With that addressed: > > Reviewed-by: Borislav Petkov <bp@xxxxxxx> > > All good stuff and a charm to review, lemme know if I should take them > or you can carry them. Sorry, I don't know what happened here because I didn't see these comments until today. I suspect what happened was that my Gmail filter auto-filed them in my linux-kernel folder, which I don't read. On my @google.com account, I have another filter that pull out things addressed directly to me, which I *do* read. But this thread didn't cc that account until the tip-bot message, which *did* cc my @google account because that's how I signed off the patches. Sigh. Anyway, it looks like this stuff is on its way; let me know (bhelgaas@xxxxxxxxxx) if I should do anything else. I would address your comments above, but since this seems to be applied and I saw a cleanup patch from you, I assume you already took care of them. Bjorn _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
