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] [day] [month] [year] [list]

Message-ID: <20251117133223.11242-1-yuntao.wang@linux.dev>
Date: Mon, 17 Nov 2025 21:32:23 +0800
From: Yuntao Wang <yuntao.wang@...ux.dev>
To: geert@...ux-m68k.org
Cc: krzk@...nel.org,
	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 13:34:20 +0100, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:

> Hi Yuntao,
>
> On Mon, 17 Nov 2025 at 12:57, Yuntao Wang <yuntao.wang@...ux.dev> wrote:
> > 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.
>
> What about:
>
>     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);
>     }
>
> ?
>
> 1. entry_cells is an intermediate variable,
> 2. prop is prepared just before its use.

Hi Geert,

Yes, if this warning really needs to be fixed, that's exactly how it should be done.

Thanks,
Yuntao

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds