Skip to content

kernel: mem_domain: test max available partitions on add_partition #35273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

kernel: mem_domain: test max available partitions on add_partition #35273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4341e51
tests: mem_protect: clean up overmax test case on failed exit
dleach02 May 13, 2021
7df881b
kernel: mem_domain: test max available partitions on add_partition
dleach02 May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions kernel/mem_domain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ void k_mem_domain_add_partition(struct k_mem_domain *domain,
}
}

__ASSERT((p_idx < arch_mem_domain_max_partitions_get()),
"no free partition slots available");
Comment on lines +158 to +159
Copy link
Member

@dcpleung dcpleung May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary because of max_partitions = arch_mem_domain_max_partitions_get() at line 274.

Copy link
Member Author

@dleach02 dleach02 May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes.... good point

Copy link
Member Author

@dleach02 dleach02 May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, I know during my debugging that I saw this value be something else because the underlying drivers for the NXP MPU had not been run yet to set the static_regions_num that is subtracted from the number of available regions. So the initial value would be 9 then after running it would be 4.

But now I've just pulled, again, from the tip of master and the case is no longer failing when it was this morning and last night.

Do we have a race condition for when init_mem_domain_module() is called and when the MPU driver may get run to set the static fields?

Copy link
Member

@dcpleung dcpleung May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... init_mem_domain_module() is called via SYS_INIT() at PRE_KERNEL_1. This should be after the call to arch_kernel_init() in arch/arm/include/aarch32/kernel_arch_func.h where it calls z_arm_mpu_init().

Copy link
Member

@dcpleung dcpleung May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static_regions_num is updated via mpu_configure_static_mpu_regions(). Maybe this is why?

Copy link
Member Author

@dleach02 dleach02 May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some bisecting and this PR #34086 moved some things around that may have fixed this. I'm trying to understand what I was using earlier this week when I originally bisected the problem because the tip should have had this PR in it.

Copy link
Contributor

@enjiamai enjiamai May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dleach02 I think we don't need this, right?

Copy link
Member Author

@dleach02 dleach02 May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response. Was on PTO.

No we don't. Given the PR I referenced above moved the initialization of the mpu_configure_static_mpu_regions() to occur before the PRE_KERNEL_1 initialization of the kernel/mem_domain.c:init_mem_domain_module() the variable in question is now initialized before being referenced.

__ASSERT(p_idx < max_partitions,
"no free partition slots available");

Expand Down
5 changes: 4 additions & 1 deletion tests/kernel/mem_protect/mem_protect/src/mem_domain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,10 @@ void test_mem_part_assert_add_overmax(void)
/* Add one more partition will trigger assert due to exceeding */
k_mem_domain_add_partition(&test_domain, &exceed_part);

/* should not reach here */
/* should not reach here but if it does clean up flag before
* indicating failure
*/
need_recover_spinlock = false;
Copy link
Contributor

@enjiamai enjiamai May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indeed should be added to clear that flag when it did not trigger the exception, I missed it. thank you!

Copy link
Member Author

@dleach02 dleach02 May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is needed but the overall PR is not needed as the bug in question has been fixed by a previous change.

ztest_test_fail();
}

Expand Down