linux-kernel - Re: [PATCH v3 1/8] of/fdt: Consolidate duplicate code into helper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251117115621.7537-1-yuntao.wang@linux.dev>
Date: Mon, 17 Nov 2025 19:56:20 +0800
From: Yuntao Wang <yuntao.wang@...ux.dev>
To: krzk@...nel.org
Cc: akpm@...ux-foundation.org,
	ardb@...nel.org,
	bhe@...hat.com,
	catalin.marinas@....com,
	changyuanl@...gle.com,
	devicetree@...r.kernel.org,
	geert+renesas@...der.be,
	geoff@...radead.org,
	graf@...zon.com,
	james.morse@....com,
	linux-kernel@...r.kernel.org,
	mark.rutland@....com,
	robh@...nel.org,
	rppt@...nel.org,
	saravanak@...gle.com,
	thunder.leizhen@...wei.com,
	yuntao.wang@...ux.dev
Subject: Re: [PATCH v3 1/8] of/fdt: Consolidate duplicate code into helper functions

On Mon, 17 Nov 2025 08:01:59 +0100, Krzysztof Kozlowski <krzk@...nel.org> wrote:

> On Sat, Nov 15, 2025 at 09:47:46PM +0800, Yuntao Wang wrote:
> > Currently, there are many pieces of nearly identical code scattered across
> > different places. Consolidate the duplicate code into helper functions to
> > improve maintainability and reduce the likelihood of errors.
>
> Not much improved. Please go to previous version and read the comments.
>
> Best regards,
> Krzysztof

Hi Krzysztof,

scripts/checkpatch.pl indeed still reports some warnings. I noticed them,
but I intentionally didn't fix them.

Below is a list of all the warnings, along with my reasons for leaving
them unaddressed.

1. WARNING: Missing a blank line after declarations
   #60: FILE: drivers/of/fdt.c:663:
   +	int entry_cells = dt_root_addr_cells + dt_root_size_cells;
   +	prop += entry_cells * entry_index;

The function that triggers this warning is:

void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
				      u64 *addr, u64 *size)
{
	int entry_cells = dt_root_addr_cells + dt_root_size_cells;
	prop += entry_cells * entry_index;

	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
}

The warning suggests adding a blank line before

	prop += entry_cells * entry_index;

I didn't add it because, logically,

	int entry_cells = dt_root_addr_cells + dt_root_size_cells;
	prop += entry_cells * entry_index;

forms a single block, just like

	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
	*size = dt_mem_next_cell(dt_root_size_cells, &prop);

I think the code is more readable without the blank line.

In fact, I initially combined these two lines

	int entry_cells = dt_root_addr_cells + dt_root_size_cells;
	prop += entry_cells * entry_index;

into a single line:

	prop += (dt_root_addr_cells + dt_root_size_cells) * entry_index;

I added the entry_cells local variable specifically to improve readability.

2. CHECK: extern prototypes should be avoided in .h files
   #78: FILE: include/linux/of_fdt.h:59:
   +extern const __be32 *of_flat_dt_get_addr_size_prop(unsigned long node,

This is another warning reported by `scripts/checkpatch.pl --strict`.

This warning says that `extern` should be removed.

The reason I didn't remove it was to maintain consistency with the existing
function declarations.

In include/linux/of_fdt.h, all function declarations use the `extern` keyword.

Yes, the Linux kernel coding style document

	https://docs.kernel.org/process/coding-style.html#function-prototypes

emphasizes

	Do not use the extern keyword with function declarations as this makes lines longer and isn’t strictly necessary.

I agree with this.

However, if extern needs to be removed, I think it should be done in a
separate patch that removes all instances of extern.

Thanks,
Yuntao