linux-kernel - Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge

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: <87zigrzg6s.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Sat, 11 Mar 2017 15:55:39 -0500
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next 04/14] net: dsa: mv88e6xxx: rework ATU Load/Purge

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

> On Thu, Mar 09, 2017 at 06:33:14PM -0500, Vivien Didelot wrote:
>> All Marvell switch chips have an ATU accessed using the same Global (1)
>> register layout. Only the handling of the FID differs as more bits were
>> necessary to support more and more databases.
>> 
>> Add and use a fresh documented implementation of the ATU Load/Purge.
>
> This is not really the Linux way of doing something. You don't throw
> something away and replace it. You incrementally modify what you have
> into something better.
>
> I really wished you had moved the code, unmodified, into
> global1_atu.c. Then made lots of easy to review small changes. I
> cannot just look at this patch and know it is correct. What i need to
> compare against is not in this patch. So it is a lot harder to review.

I've addressed all of your comments in this patchset except this one.

A patch file cannot guarantee that a chunk of code moved around has not
been altered in the process. This will just generate more diff for no
value, that needs to be updated afterwards anyway.

Plus you already complained in the first iteration I sent about
modifying lines that I previously added. I took care of logically
splitting the new ATU Load/Purge, GetNext, Flush and Remove operations
into incremental unmodified chunks in this series.

I have updated the commit messages to detail the addition of the static
helpers and their counterparts, but I will resend this patch as is.

Thanks,

        Vivien