linux-kernel - Re: [PATCH] i2c: mediatek: Optimize master

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] [day] [month] [year] [list]

Date:   Fri, 8 Apr 2022 13:11:12 -0400
From:   Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     qii.wang@...iatek.com, matthias.bgg@...il.com,
        linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        wsa@...nel.org, kernel@...labora.com
Subject: Re: [PATCH] i2c: mediatek: Optimize master_xfer() and avoid circular
 locking

Hi Angelo,

On Fri, Apr 01, 2022 at 02:01:20PM +0200, AngeloGioacchino Del Regno wrote:
> Especially (but not only) during probe, it may happen that multiple
> devices are communicating via i2c (or multiple i2c busses) and
> sometimes while others are probing asynchronously.
> For example, a Cr50 TPM may be filling entropy (or userspace may be
> reading random data) while the rt5682 (i2c) codec driver reads/sets
> some registers, like while getting/setting a clock's rate, which
> happens both during probe and during system operation.
> 
> In this driver, the mtk_i2c_transfer() function (which is the i2c
> .master_xfer() callback) was granularly managing the clocks by
> performing a clk_bulk_prepare_enable() to start them and its inverse.
> This is not only creating possible circular locking dependencies in
> the some cases (like former explaination), but it's also suboptimal,

s/explaination/explanation/

> as clk_core prepare/unprepare operations are using mutex locking,
> which creates a bit of unwanted overhead (for example, i2c trackpads
> will call master_xfer() every few milliseconds!).
> 
> With this commit, we avoid both the circular locking and additional
> overhead by changing how we handle the clocks in this driver:
> - Prepare the clocks during probe (and PM resume)
> - Enable/disable clocks in mtk_i2c_transfer()
> - Unprepare the clocks only for driver removal (and PM suspend)
> 
> For the sake of providing a full explaination: during probe, the

Ditto.

> clocks are not only prepared but also enabled, as this is needed
> for somehardware initialization but, after that, we are disabling

s/somehardware/some hardware/

> but not unpreparing them, leaving an expected state for the
> aforementioned clock handling strategy.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>

I couldn't see any performance improvements on mt8192-asurada-spherion with
this, although I didn't do any serious benchmark testing. Besides, on spherion
the TPM is instead wired through SPI, so I guess most of the boot overhead
wouldn't be visible there.

In any case it did get rid of the frequent deadlocks when booting on spherion,
which is already great on its own, so

Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>

Thanks,
Nícolas