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]

Message-ID: <dc8febfc-b8de-abbf-8e0f-659a27fa0fa2@microchip.com>
Date:   Thu, 10 Feb 2022 08:42:45 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
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()

On 2/10/22 10:16, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

you can put the 
+       if (nor->flags & SNOR_F_USE_CLSR)
+               nor->params->ready = spi_nor_sr_ready_and_clear;

above the size check and get rid of the prerequisite requirement, no?
But it will look ugly indeed.

If these two are so tightly related, how about squashing them?

> 
> 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.
> 

You definitely care about it if you wrote such a long email :). I find
the first argument the strongest, these two are biased IMO. I'm waiting
for v2 with this change included! :)

Cheers,
ta