Le 23/09/2021 à 14:01, Mike Rapoport a écrit :
On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote:
Le 23/09/2021 à 09:43, Mike Rapoport a écrit :
From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
For ages memblock_free() interface dealt with physical addresses even
despite the existence of memblock_alloc_xx() functions that return a
virtual pointer.
Introduce memblock_phys_free() for freeing physical ranges and repurpose
memblock_free() to free virtual pointers to make the following pairing
abundantly clear:
int memblock_phys_free(phys_addr_t base, phys_addr_t size);
phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size);
void *memblock_alloc(phys_addr_t size, phys_addr_t align);
void memblock_free(void *ptr, size_t size);
Replace intermediate memblock_free_ptr() with memblock_free() and drop
unnecessary aliases memblock_free_early() and memblock_free_early_nid().
Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>
---
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 1a04e5bdf655..37826d8c4f74 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void)
/* Get the CPU registers */
smp_save_cpu_regs(sa, addr, is_boot_cpu, page);
}
- memblock_free(page, PAGE_SIZE);
+ memblock_phys_free(page, PAGE_SIZE);
diag_amode31_ops.diag308_reset();
pcpu_set_smt(0);
}
@@ -880,7 +880,7 @@ void __init smp_detect_cpus(void)
/* Add CPUs present at boot */
__smp_rescan_cpus(info, true);
- memblock_free_early((unsigned long)info, sizeof(*info));
+ memblock_free(info, sizeof(*info));
}
/*
I'm a bit lost. IIUC memblock_free_early() and memblock_free() where
identical.
Yes, they were, but all calls to memblock_free_early() were using
__pa(vaddr) because they had a virtual address at hand.
I'm still not following. In the above memblock_free_early() was taking
(unsigned long)info . Was it a bug ? It looks odd to hide bug fixes in
such a big patch, should that bug fix go in patch 2 ?
In the first hunk memblock_free() gets replaced by memblock_phys_free()
In the second hunk memblock_free_early() gets replaced by memblock_free()
In the first hunk the memory is allocated with memblock_phys_alloc() and we
have a physical range to free. In the second hunk the memory is allocated
with memblock_alloc() and we are freeing a virtual pointer.
I think it would be easier to follow if you could split it in several
patches:
It was an explicit request from Linus to make it a single commit:
but the actual commit can and should be just a single commit that just
fixes 'memblock_free()' to have sane interfaces.
I don't feel strongly about splitting it (except my laziness really
objects), but I don't think doing the conversion in several steps worth the
churn.
The commit is quite big (55 files changed, approx 100 lines modified).
If done in the right order the change should be minimal.
It is rather not-easy to follow and review when a function that was
existing (namely memblock_free() ) disappears and re-appears in the same
commit but to do something different.
You do:
- memblock_free() ==> memblock_phys_free()
- memblock_free_ptr() ==> memblock_free()
At least you could split in two patches, the advantage would be that
between first and second patch memblock() doesn't exist anymore so you
can check you really don't have anymore user.
- First patch: Create memblock_phys_free() and change all relevant
memblock_free() to memblock_phys_free() - Or change memblock_free() to
memblock_phys_free() and make memblock_free() an alias of it.
- Second patch: Make memblock_free_ptr() become memblock_free() and change
all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr))
becomes memblock_free(ptr) and make memblock_free_ptr() an alias of
memblock_free()
- Fourth patch: Replace and drop memblock_free_ptr()
- Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All
users should have been upgraded to memblock_free_phys() in patch 1 or
memblock_free() in patch 2)
Christophe
