Re: [PATCH v3 1/3] x86/ioremap: introduce helper to implement xxx_is_setup_data()

[Borislav Petkov]

On Sat, Nov 23, 2024 at 07:42:19PM +0800, Baoquan He wrote:
> Functions memremap_is_setup_data() and early_memremap_is_setup_data()
> share completely the same process and handling, except of the
> different memremap/unmap invocations.
> 
> So add helper __memremap_is_setup_data() to extract the common part,
> parameter 'early' is used to decide what kind of memremap/unmap
> APIs are called. This simplifies codes a lot by removing the duplicated
> codes, and also removes the similar code comment above them.
> 
> And '__ref' is added to __memremap_is_setup_data() to suppress below
> section mismatch warning:
> 
> ARNING: modpost: vmlinux: section mismatch in reference: __memremap_is_setup_data+0x5f (section: .text) ->
> early_memunmap (section: .init.text)
> 
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> ---
>  arch/x86/mm/ioremap.c | 104 ++++++++++++++----------------------------
>  1 file changed, 35 insertions(+), 69 deletions(-)

Some touchups ontop:

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index aaf40a712b04..fe44e8180bdd 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -632,9 +632,9 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
  * Examine the physical address to determine if it is boot data by checking
  * it against the boot params setup_data chain.
  */
-static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
-						bool early)
+static bool __ref __memremap_is_setup_data(resource_size_t phys_addr, bool early)
 {
+	unsigned int setup_data_sz = sizeof(struct setup_data);
 	struct setup_indirect *indirect;
 	struct setup_data *data;
 	u64 paddr, paddr_next;
@@ -647,24 +647,23 @@ static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
 			return true;
 
 		if (early)
-			data = early_memremap_decrypted(paddr, sizeof(*data));
+			data = early_memremap_decrypted(paddr, setup_data_sz);
 		else
-			data = memremap(paddr, sizeof(*data),
-					MEMREMAP_WB | MEMREMAP_DEC);
+			data = memremap(paddr, setup_data_sz, MEMREMAP_WB | MEMREMAP_DEC);
 		if (!data) {
-			pr_warn("failed to memremap setup_data entry\n");
+			pr_warn("failed to remap setup_data entry\n");
 			return false;
 		}
 
-		size = sizeof(*data);
+		size = setup_data_sz;
 
 		paddr_next = data->next;
 		len = data->len;
 
 		if ((phys_addr > paddr) &&
-		    (phys_addr < (paddr + sizeof(*data) + len))) {
+		    (phys_addr < (paddr + setup_data_sz + len))) {
 			if (early)
-				early_memunmap(data, sizeof(*data));
+				early_memunmap(data, setup_data_sz);
 			else
 				memunmap(data);
 			return true;
@@ -673,15 +672,14 @@ static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
 		if (data->type == SETUP_INDIRECT) {
 			size += len;
 			if (early) {
-				early_memunmap(data, sizeof(*data));
+				early_memunmap(data, setup_data_sz);
 				data = early_memremap_decrypted(paddr, size);
 			} else {
 				memunmap(data);
-				data = memremap(paddr, size,
-						MEMREMAP_WB | MEMREMAP_DEC);
+				data = memremap(paddr, size, MEMREMAP_WB | MEMREMAP_DEC);
 			}
 			if (!data) {
-				pr_warn("failed to memremap indirect setup_data\n");
+				pr_warn("failed to remap indirect setup_data\n");
 				return false;
 			}
 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette