netdev - RE: [PATCH v2 net-next 0/2] net: dsa: mv88e6xxx: Improve indirect addressing performance

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: <87k0ejc0ol.fsf@waldekranz.com>
Date:   Fri, 28 Jan 2022 16:58:02 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 net-next 0/2] net: dsa: mv88e6xxx: Improve indirect
 addressing performance

On Fri, Jan 28, 2022 at 14:10, David Laight <David.Laight@...LAB.COM> wrote:
> From: Tobias Waldekranz
>> Sent: 28 January 2022 10:50
>> 
>> The individual patches have all the details. This work was triggered
>> by recent work on a platform that took 16s (sic) to load the mv88e6xxx
>> module.
>> 
>> The first patch gets rid of most of that time by replacing a very long
>> delay with a tighter poll loop to wait for the busy bit to clear.
>> 
>> The second patch shaves off some more time by avoiding redundant
>> busy-bit-checks, saving 1 out of 4 MDIO operations for every register
>> read/write in the optimal case.
>
> I don't think you should fast-poll for the entire timeout period.
> Much better to drop to a usleep_range() after the first 2 (or 3)
> reads fail.

You could, I suppose. Andrew, do you want a v3?

> Also I presume there is some lock that ensures this is all single threaded?

Yes, all callers must hold chip->reg_lock, which is asserted by
mv88e6xxx_{read,write}.

> If you remember the 'busy state' you can defer the 'busy check' after
> a write until the next request.
> That may well reduce the number of 'double checks' needed.

With 2/2 in place, we have already reduced it to:

read:
  Write command
  Poll  command
  Read  data

write:
  Write data
  Write command
  Poll  command

The poll in read is always required to that you know that the value in
the data register is valid. The poll in write is always required because
most of the writes are going to have side-effects on how the fabric
operates. So you want callers to be able to rely on the data being in
place once the function returns.

Am I missing something?