在 2018年09月27日 22:03, Bjorn Helgaas 写道: > On Thu, Sep 27, 2018 at 01:27:41PM +0800, lijiang wrote: >> 在 2018年09月25日 06:15, Bjorn Helgaas 写道: >>> 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 >>> and hard to use correctly. >>> >>> All callers 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 callers (__walk_iomem_res_desc() and walk_system_ram_range()) do not >>> restore res->flags, so if the caller is searching for flags of >>> IORESOURCE_MEM | IORESOURCE_BUSY and finds a resource with flags of >>> IORESOURCE_MEM | IORESOURCE_BUSY | IORESOURCE_SYSRAM, the next search will >>> find only resources marked as IORESOURCE_SYSRAM. >>> >>> Fix this by restructuring the interface so it takes explicit "start, end, >>> flags" parameters and uses "*res" only as an output parameter. >> >> Hi, Bjorn >> I personally suggest that some comments might be added in the code, make it clear >> and easy to understand, then which could avoid the old confusion and more code changes. > > Since I think the current interface (using *res as both input and > output parameters that have very different meanings) is confusing, > it's hard for *me* to write comments that make it less confusing, but > of course, you're welcome to propose something. > > My opinion (probably not universally shared) is that my proposal would > make the code more readable, and it's worth doing even though the diff > is larger. > > Anyway, I'll post these patches independently and see if anybody else > has an opinion. > I don't mind at all, because that is just my own opinion about more code changes. Anyway, thank you to improve this patch. Regards, Lianbo > Bjorn > >>> 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); >>> >>> /* >>> - * 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. >>> */ >>> -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); >>> >>> if (first_level_children_only) >>> @@ -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); >>> 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 *)) >>> { >>> - 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 *)) >>> { >>> - 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 *)) >>> { >>> + resource_size_t start, end; >>> + unsigned long flags; >>> struct resource res; >>> unsigned long pfn, end_pfn; >>> - u64 orig_end; >>> int ret = -1; >>> >>> - res.start = (u64) start_pfn << PAGE_SHIFT; >>> - res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1; >>> - res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; >>> - orig_end = res.end; >>> - while ((res.start < res.end) && >>> - (find_next_iomem_res(&res, IORES_DESC_NONE, true) >= 0)) { >>> + start = (u64) start_pfn << PAGE_SHIFT; >>> + end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1; >>> + flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; >>> + while (start < end && >>> + !find_next_iomem_res(start, end, flags, IORES_DESC_NONE, >>> + true, &res)) { >>> pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT; >>> end_pfn = (res.end + 1) >> PAGE_SHIFT; >>> if (end_pfn > pfn) >>> ret = (*func)(pfn, end_pfn - pfn, arg); >>> if (ret) >>> break; >>> - res.start = res.end + 1; >>> - res.end = orig_end; >>> + start = res.end + 1; >>> } >>> return ret; >>> } >>> >>> >>> _______________________________________________ >>> kexec mailing list >>> kexec@xxxxxxxxxxxxxxxxxxx >>> http://lists.infradead.org/mailman/listinfo/kexec >>> _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
