Dan Williams <dan.j.williams@xxxxxxxxx> writes:
> Huang, Ying wrote:
>> Dan Williams <dan.j.williams@xxxxxxxxx> writes:
>> > Huang Ying wrote:
> [..]
>> >> kernel/resource.c | 44 +++++++++++++++++++++++++++++++++++++-------
>> >> 1 file changed, 37 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/kernel/resource.c b/kernel/resource.c
>> >> index 14777afb0a99..c97a5add9394 100644
>> >> --- a/kernel/resource.c
>> >> +++ b/kernel/resource.c
> [..]
[snip]
>
>> However, I admit that the original code is hard to be understood,
>> whether is something like below better?
>
> I like that this proposal defers incrementing @other rather than
> decrement after the fact.
>
>>
>> for (p = parent->child; p ; p = p->sibling) {
>> if (!resource_overlaps(p, &res))
>> continue;
>> is_type = (((p->flags & flags) == flags) &&
>> ((desc == IORES_DESC_NONE) || (desc == p->desc)));
>> if (is_type) {
>> type++;
>> continue;
>> }
>> /*
>> * Continue to search in descendant resources. Unless
>> * the matched descendant resources cover the whole
>> * overlapped range, increase 'other', because it
>> * overlaps with 'p' at least.
>> */
>> covered = false;
>
> I would call @covered, @single_descendant. Since @covered is ambiguous.
Sorry, I don't understand why this is called @single_descendant. It's
possible that the checked region is overlapped with 2 descendants, and
the result is REGION_INTERSECTS. For example,
490000000-52fffffff : CXL Window 0
490000000-50fffffff : region0
490000000-50fffffff : dax0.0
490000000-50fffffff : System RAM (kmem)
510000000-52fffffff : region1
510000000-52fffffff : dax0.1
510000000-52fffffff : System RAM (kmem)
region_intersects(, 0x50ffff000, 2 * PAGE_SIZE, IORESOURCE_SYSTEM_RAM,
IORES_DESC_NONE) => REGION_INTERSECTS
>> ostart = max(res.start, p->start);
>> oend = min(res.end, p->end);
>> for_each_resource(p, dp, false) {
>> if (!resource_overlaps(dp, &res))
>> continue;
>> is_type = (((dp->flags & flags) == flags) &&
>> ((desc == IORES_DESC_NONE) ||
>> (desc == dp->desc)));
>> if (is_type) {
>> type++;
>> if (dp->start > ostart)
>
> ...this should have a comment:
>
> /* partial descendant overlap indicates overlap with a descendant hole */
Yes. Some comments should help.
>> break;
>> if (dp->end >= oend) {
>> covered = true;
>> break;
>
> ...then per above this because easier to read as:
>
> single_descendant = true;
>
>> }
>> ostart = dp->end + 1;
>> }
>> }
>> if (!covered)
>> other++;
>> }
>>
[snip]
--
Best Regards,
Huang, Ying
