Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On Tue, Oct 09, 2018 at 12:30:33PM -0500, Bjorn Helgaas wrote: > 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. No worries, it all should be taken care of now! :-) > Anyway, it looks like this stuff is on its way; let me know > (bhelg...@google.com) 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. Yeah, I think I've covered them all but if you see something out of the ordinary, let me know so that I can fix it. Thanks! -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On Fri, Sep 28, 2018 at 11:42 AM Borislav Petkov wrote:
>
> On Thu, Sep 27, 2018 at 09:22:09AM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas
> >
> > 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-liji...@redhat.com
> > Based-on-patch-by: Lianbo Jiang
> > Signed-off-by: Bjorn Helgaas
> > ---
> > 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;
> > +
>
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On Thu, Sep 27, 2018 at 09:22:09AM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas
>
> 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-liji...@redhat.com
> Based-on-patch-by: Lianbo Jiang
> Signed-off-by: Bjorn Helgaas
> ---
> 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 he
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On Thu, Sep 27, 2018 at 09:03:51AM -0500, Bjorn Helgaas wrote: > Since I think the current interface (using *res as both input and > output parameters that have very different meanings) is confusing, FTR, I too, think that this whole machinery in resource.c with passing in a function and a struct resource pointer and is clumsy and could use a rewrite. When there's time... ;-\ -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
在 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
>>>
>>> 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-liji...@redhat.com
>>> Based-on-patch-by: Lianbo Jiang
>>> Signed-off-by: Bjorn Helgaas
>>> ---
>>> 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 <
[PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
From: Bjorn Helgaas
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.
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.
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-liji...@redhat.com
Based-on-patch-by: Lianbo Jiang
Signed-off-by: Bjorn Helgaas
---
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
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On Thu, Sep 27, 2018 at 01:27:41PM +0800, lijiang wrote:
> 在 2018年09月25日 06:15, Bjorn Helgaas 写道:
> > From: Bjorn Helgaas
> >
> > 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.
Bjorn
> > Original-patch:
> > http://lore.kernel.org/lkml/20180921073211.20097-2-liji...@redhat.com
> > Based-on-patch-by: Lianbo Jiang
> > Signed-off-by: Bjorn Helgaas
> > ---
> > 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
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
在 2018年09月25日 06:15, Bjorn Helgaas 写道:
> From: Bjorn Helgaas
>
> 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.
Thanks
Lianbo
> Original-patch:
> http://lore.kernel.org/lkml/20180921073211.20097-2-liji...@redhat.com
> Based-on-patch-by: Lianbo Jiang
> Signed-off-by: Bjorn Helgaas
> ---
> 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) &&
> -
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
On 09/25/18 at 04:58pm, Baoquan He wrote: > Hi Bjorn, > > On 09/24/18 at 05:15pm, Bjorn Helgaas wrote: > > @@ -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; > > I think this fix is good. However, is it OK to keep res->flags always, > never touch it in find_next_iomem_res()? We just iterate and update > region, its start and end. So just removing that "res->flags = p->flags;" > line might involve much less code changes. Rethink about it, I was wrong. Please ignore my comment. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
Hi Bjorn, On 09/24/18 at 05:15pm, Bjorn Helgaas wrote: > @@ -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; I think this fix is good. However, is it OK to keep res->flags always, never touch it in find_next_iomem_res()? We just iterate and update region, its start and end. So just removing that "res->flags = p->flags;" line might involve much less code changes. Thanks Baoquan ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 3/3] resource: Fix find_next_iomem_res() iteration issue
From: Bjorn Helgaas
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.
Original-patch:
http://lore.kernel.org/lkml/20180921073211.20097-2-liji...@redhat.com
Based-on-patch-by: Lianbo Jiang
Signed-off-by: Bjorn Helgaas
---
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;
-
