linux-kernel - Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'

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: <3ed8da81-55cc-4fbc-cb01-9405ac9709d7@microchip.com>
Date:   Mon, 15 Mar 2021 11:24:32 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <joe@...ches.com>, <p.yadav@...com>
CC:     <vigneshr@...com>, <michael@...le.cc>,
        <linux-mtd@...ts.infradead.org>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'

On 3/15/21 8:53 AM, Joe Perches wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, 2021-03-08 at 11:58 +0530, Pratyush Yadav wrote:
>> On 06/03/21 11:50AM, Tudor Ambarus wrote:
>>> else is not generally useful after a break or return.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@...rochip.com>
>>
>> Reviewed-by: Pratyush Yadav <p.yadav@...com>
>>
> 
> I don't think this improves the code.
> 
> Generally, checkpatch is a stupid little script.
> 
> This code uses a form like:
>         if (foo)
>                 return bar;
>         else
>                 return baz;

Isn't else redundant? What are the benefits of keeping the else?

> 
> which checkpatch recognizes as OK and so checkpatch does not
> emit any warning message, but this code just adds comments
> before each return which confuses checkpatch.
> 
> I think better would be to change the code to use temporaries
> and convert the functions to bool.
> 
> Something like:
> ---
>  drivers/mtd/spi-nor/core.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..e174a2f1d621 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1798,36 +1798,41 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  }
> 
>  /*
> - * Return 1 if the entire region is locked (if @locked is true) or unlocked (if
> - * @locked is false); 0 otherwise
> + * Return true if the entire region is locked
> + * (if @locked is true) or unlocked (if @locked is false); false otherwise
>   */
> -static int spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
> +static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
>                                         uint64_t len, u8 sr, bool locked)
>  {
>         loff_t lock_offs;
>         uint64_t lock_len;
> +       uint64_t lock_max;
> +       uint64_t ofs_max;
> 
>         if (!len)
> -               return 1;
> +               return true;

returning one is wrong indeed, would you submit a patch for the conversion
of the functions to bool?

> 
>         spi_nor_get_locked_range_sr(nor, sr, &lock_offs, &lock_len);
> 
> +       lock_max = lock_offs + lock_len;
> +       ofs_max = ofs + len;
> +
>         if (locked)
>                 /* Requested range is a sub-range of locked range */
> -               return (ofs + len <= lock_offs + lock_len) && (ofs >= lock_offs);
> +               return (ofs_max <= lock_max) && (ofs >= lock_offs);
>         else
>                 /* Requested range does not overlap with locked range */
> -               return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
> +               return (ofs >= lock_max) || (ofs_max <= lock_offs);

This should be fine too.

Cheers,
ta