| Message ID | 20190821183204.23576-3-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: MMU enabled kexec relocation | expand |
Hi Pavel, Nit: The pattern for the subject prefix should be "arm64: hibernate:".. Its usually possible to spot the pattern from "git log --oneline $file". On 21/08/2019 19:31, Pavel Tatashin wrote: > create_safe_exec_page is a local function that uses the > get_safe_page() to allocate page table and pages and one pages > that is getting mapped. I can't parse this. create_safe_exec_page() uses hibernate's allocator to create a set of page table to map a single page that will contain the relocation code. > Remove the allocator related arguments, and use get_safe_page > directly, as it is done in other local functions in this > file. ... because kexec can't use this as it doesn't have a working allocator. Removing this function pointer makes it easier to refactor the code later. (this thing is only a function pointer so kexec could use it too ... It looks like you're creating extra work. Patch 7 moves these new calls out to a new file... presumably so another patch can remove them again) As stand-alone cleanup the patch looks fine, but you probably don't need to do this. Thanks, James
On Fri, Sep 6, 2019 at 11:17 AM James Morse <james.morse@arm.com> wrote: > > Hi Pavel, > > Nit: The pattern for the subject prefix should be "arm64: hibernate:".. > Its usually possible to spot the pattern from "git log --oneline $file". Sure, I will change here and in other places to "arm64: hibernate:" > > On 21/08/2019 19:31, Pavel Tatashin wrote: > > create_safe_exec_page is a local function that uses the > > get_safe_page() to allocate page table and pages and one pages > > that is getting mapped. > > I can't parse this. > > create_safe_exec_page() uses hibernate's allocator to create a set of page table to map a > single page that will contain the relocation code. Thanks I will rephrase it with your suggestion. > > > > Remove the allocator related arguments, and use get_safe_page > > directly, as it is done in other local functions in this > > file. > ... because kexec can't use this as it doesn't have a working allocator. > Removing this function pointer makes it easier to refactor the code later. Thanks, I will add it to the description. > > (this thing is only a function pointer so kexec could use it too ... It looks like you're > creating extra work. Patch 7 moves these new calls out to a new file... presumably so > another patch can remove them again) > > As stand-alone cleanup the patch looks fine, but you probably don't need to do this. Without this clean-up moving to common code becomes messier. So, I would like to keep this change. Thank you, Pasha
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index 9341fcc6e809..4bb4d17a6a7c 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -196,16 +196,14 @@ EXPORT_SYMBOL(arch_hibernation_header_restore); */ static int create_safe_exec_page(void *src_start, size_t length, unsigned long dst_addr, - phys_addr_t *phys_dst_addr, - void *(*allocator)(gfp_t mask), - gfp_t mask) + phys_addr_t *phys_dst_addr) { int rc = 0; pgd_t *pgdp; pud_t *pudp; pmd_t *pmdp; pte_t *ptep; - unsigned long dst = (unsigned long)allocator(mask); + unsigned long dst = get_safe_page(GFP_ATOMIC); if (!dst) { rc = -ENOMEM; @@ -215,9 +213,9 @@ static int create_safe_exec_page(void *src_start, size_t length, memcpy((void *)dst, src_start, length); __flush_icache_range(dst, dst + length); - pgdp = pgd_offset_raw(allocator(mask), dst_addr); + pgdp = pgd_offset_raw((void *)get_safe_page(GFP_ATOMIC), dst_addr); if (pgd_none(READ_ONCE(*pgdp))) { - pudp = allocator(mask); + pudp = (void *)get_safe_page(GFP_ATOMIC); if (!pudp) { rc = -ENOMEM; goto out; @@ -227,7 +225,7 @@ static int create_safe_exec_page(void *src_start, size_t length, pudp = pud_offset(pgdp, dst_addr); if (pud_none(READ_ONCE(*pudp))) { - pmdp = allocator(mask); + pmdp = (void *)get_safe_page(GFP_ATOMIC); if (!pmdp) { rc = -ENOMEM; goto out; @@ -237,7 +235,7 @@ static int create_safe_exec_page(void *src_start, size_t length, pmdp = pmd_offset(pudp, dst_addr); if (pmd_none(READ_ONCE(*pmdp))) { - ptep = allocator(mask); + ptep = (void *)get_safe_page(GFP_ATOMIC); if (!ptep) { rc = -ENOMEM; goto out; @@ -523,8 +521,7 @@ int swsusp_arch_resume(void) */ rc = create_safe_exec_page(__hibernate_exit_text_start, exit_size, (unsigned long)hibernate_exit, - &phys_hibernate_exit, - (void *)get_safe_page, GFP_ATOMIC); + &phys_hibernate_exit); if (rc) { pr_err("Failed to create safe executable page for hibernate_exit code.\n"); goto out;
create_safe_exec_page is a local function that uses the get_safe_page() to allocate page table and pages and one pages that is getting mapped. Remove the allocator related arguments, and use get_safe_page directly, as it is done in other local functions in this file. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/hibernate.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)