linux-kernel - Re: [PATCH V2 mips/linux.git] firmware: bcm47xx

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: <CAAdtpL5CMTaB6qCR=nZj+1MoGC97_BVd-r30E2RRYOhiktOiZQ@mail.gmail.com>
Date:   Fri, 5 Mar 2021 12:47:59 +0100
From:   Philippe Mathieu-Daudé <f4bug@...at.org>
To:     Rafał Miłecki <rafal@...ecki.pl>
Cc:     Rafał Miłecki <zajec5@...il.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        "open list:BROADCOM NVRAM DRIVER" <linux-mips@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivek Unune <npcomplete13@...il.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        open list <linux-kernel@...r.kernel.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH V2 mips/linux.git] firmware: bcm47xx_nvram: refactor
 finding & reading NVRAM

On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@...ecki.pl> wrote:
>
> Hi,
>
> On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote:
> > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@...il.com> wrote:
> >>
> >> From: Rafał Miłecki <rafal@...ecki.pl>
> >>
> >> 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead
> >>     of e.g. "iobase", "end")
> >> 2. Always operate on "offset" instead of mix of start, end, size, etc.
> >
> > "instead of a mix"
> >
> >> 3. Add helper checking for NVRAM to avoid duplicating code
> >> 4. Use "found" variable instead of goto
> >> 5. Use simpler checking of offsets and sizes (2 nested loops with
> >>     trivial check instead of extra function)
> >
> > This could be a series of trivial patches, why did you choose to make a mixed
> > bag harder to review?
>
> It's a subjective thing and often a matter of maintainer taste. I can
> say that after contributing to various Linux subsystems. If you split a
> similar patch for MTD subsystem you'll get complains about making
> changes too small & too hard to review (sic!).

Fine. MTD subsystem developers are probably smarter than I'm :)

> This isn't a bomb really: 63 insertions(+), 48 deletions(-)

Too many changes at once for my brain stack doesn't mean others are
willing to review it. But to me that means each time I'll have to pass over
it while bisecting or reviewing git history I'll suffer the same overflow.
Anyway, matter of taste as you said.

>
> That said I admit I don't know MIPS tree habits. Thomas: do you prefer
> smaller patches in case like this?