linux-kernel - Re: [net-next PATCH v11 5/9] mfd: an8855: Add support for Airoha AN8855 Switch MFD

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: <675da041.050a0220.a8e65.af0e@mx.google.com>
Date: Sat, 14 Dec 2024 16:11:54 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	upstream@...oha.com
Subject: Re: [net-next PATCH v11 5/9] mfd: an8855: Add support for Airoha
 AN8855 Switch MFD

On Wed, Dec 11, 2024 at 01:48:03AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 10, 2024 at 11:32:17PM +0100, Christian Marangi wrote:
> > Doesn't regmap add lots of overhead tho? Maybe I should really change
> > the switch regmap to apply a save/restore logic?
> > 
> > With an implementation like that current_page is not needed anymore.
> > And I feel additional read/write are ok for switch OP.
> > 
> > On mdio I can use the parent-mdio-bus property to get the bus directly
> > without using MFD priv.
> > 
> > What do you think?
> 
> If performance is a relevant factor at all, it will be hard to measure it, other
> than with synthetic tests (various mixes of switch and PHY register access).
> Though since you mention it, it would be interesting to see a comparison of the
> 3 arbitration methods. This could probably be all done from the an8855_mfd_probe()
> calling context: read a switch register and a PHY register 100K times and see how
> long it took, then read 2 switch registers and a PHY register 100K times, then
> 3 switch registers.... At some point, we should start seeing the penalty of the
> page restoration in Andrew's proposal, because that will be done after each switch
> register read. Just curious to put it into perspective and see how soon it starts
> to make a difference. And this test will also answer the regmap overhead issue.

Ok sorry for the delay as I had to tackle an annoying crypto driver...

I was also curious about this and I hope I tested this correctly...

The testing code is this. Following Vladimir testing and simple time
comparision before and after. I used 100 times as 100k was very big.
>From the results we can derive our conclusions.

static void test(struct an8855_mfd_priv *priv, struct regmap *regmap, struct regmap *regmap_phy)
{
	ktime_t start_time, end_time;
	// struct mii_bus *bus = priv->bus;
    	s64 elapsed_ns;
	u32 val;
	int times = 1;
	int i, j;

	start_time = ktime_get();
	for (i = 0; i < 100; i++) {
		for (j = 0; j < times; j++) {
			regmap_read(regmap, 0x10005000, &val);
		}
		// mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
		// // an8855_mii_set_page(priv, priv->switch_addr, 0);
		// __mdiobus_read(bus, priv->switch_addr, 0x1);
		// mutex_unlock(&bus->mdio_lock);
		regmap_read(regmap_phy,
			   FIELD_PREP(GENMASK(20, 16), priv->switch_addr) |
			   FIELD_PREP(GENMASK(15, 0), 0x1),
			   &val);
		times++;
	}

	end_time = ktime_get();

	elapsed_ns = ktime_to_ns(ktime_sub(end_time, start_time));

	pr_info("Time spent in the code block: %lld ns\n", elapsed_ns);
}

With the code changed accordingly.

switch regmap + page (proposed implementation)
Time spent in the code block:  866179846 ns

switch regmap + phy regmap (proposed implementation + PHY regmap)
Time spent in the code block:  861326846 ns

switch regmap restore (switch regmap read/set/restore page)
Time spent in the code block: 1151011308 ns

switch regmap restore + phy regmap (switch regmap read/set/restore pgae
+ PHY regmap)
Time spent in the code block: 1147400462 ns

We can see that:
- as suggested regmap doesn't cause any performance penality. It does
  even produce better result.
- the read/set/restore implementation gives worse performance.

So I guess I will follow the path of regmap + cache page. What do you
think?

-- 
	Ansuel