linux-kernel - Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses

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: <s5h7dh51thw.wl-tiwai@suse.de>
Date:   Sun, 01 Aug 2021 10:03:55 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Vitaly Rodionov <vitalyr@...nsource.cirrus.com>
Cc:     Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>,
        Lucas Tanure <tanureal@...nsource.cirrus.com>,
        Stefan Binding <sbinding@...nsource.cirrus.com>
Subject: Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses

On Fri, 30 Jul 2021 17:18:30 +0200,
Vitaly Rodionov wrote:
> 
> From: Lucas Tanure <tanureal@...nsource.cirrus.com>
> 
> Only disable I2C clock 25 ms after not being used.
> 
> The current implementation enables and disables the I2C clock for each
> I2C transaction. Each enable/disable call requires two verb transactions.
> This means each I2C transaction requires a total of four verb transactions
> to enable and disable the clock.
> However, if there are multiple consecutive I2C transactions, it is not
> necessary to enable and disable the clock each time, instead it is more
> efficient to enable the clock for the first transaction, and disable it
> after the final transaction, which would improve performance.
> This is achieved by using a timeout which disables the clock if no request
> to enable the clock has occurred for 25 ms.
> 
> Signed-off-by: Lucas Tanure <tanureal@...nsource.cirrus.com>
> Signed-off-by: Vitaly Rodionov <vitalyr@...nsource.cirrus.com>
> Signed-off-by: Stefan Binding <sbinding@...nsource.cirrus.com>
> ---
> 
> Changes in v2:
> - Improved delayed work start/cancel implementation, and re-worked commit message
>  adding more explanation why this was required. 
> 
> Changes in v3:
> - Cancel the disable timer, but do not wait for any running disable functions to finish.
>  If the disable timer runs out before cancel, the delayed work thread will be blocked,
>  waiting for the mutex to become unlocked. This mutex will be locked for the duration of
>  any i2c transaction, so the disable function will run to completion immediately
>  afterwards in the scenario. The next enable call will re-enable the clock, regardless.

This looks almost fine, but just a couple of thoughts:

- cancel_delayed_work_sync() means to it might keep the i2c enabled
  after that point (just cancel the pending work).
  Would it cause a inconsistency afterwards?

- A similar procedure is needed for suspend callback to cancel / flush
  the work.
  The shutdown is another question, but usually it's fine to without
  any special handling as long as the resource is kept.


thanks,

Takashi