linux-kernel - Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late

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]

Date:   Thu, 10 Feb 2022 09:16:48 +0100
From:   Michael Walle <michael@...le.cc>
To:     Tudor.Ambarus@...rochip.com
Cc:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        p.yadav@...com, miquel.raynal@...tlin.com, richard@....at,
        vigneshr@...com
Subject: Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control
 flow in late_init()

Am 2022-02-10 04:26, schrieb Tudor.Ambarus@...rochip.com:
> On 2/2/22 16:58, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Increase readability of the code. Instead of returning early if the
>> flash size is smaller or equal than 16MiB and then do the fixups for
>> larger flashes, do it within the condition.
>> 
> 
> mm, no, I'm not sure this improves readability, I see the two 
> equivalent.
> The original version has the benefit of no indentation. Pratyush?

This is a preparation patch for 12/14, where the current version isn't
working anyway. If that is not enough reason why this is bad IMHO, I'll
give you two more.

I'd agree with you if that function was called
spansion_late_init_smaller_flashes() or something like that. But it is
a generic function valid for all flashes. And if you read it you might
get the impression there are only flashes smaller or equal than 16MiB.
You have to look twice to notice it was the intention that the
assignment afterwards are just for the smaller flashes (and you will
need to notice that there aren't any assignments for all spansion
flashes). There is no direct connection between the assignment and
the condition. Whereas with
   if (condition) {
     some_action();
   }
It is clear that some_action() was intended to only execute if
condition is true.

Also - and that is worse IMHO - it might easily be missed as someone
just add stuff to the end of the function which might goes unnoticed
but it won't work for flashes >16MiB.

-michael