linux-kernel - Re: [PATCH] ASoC: cs42l42: Implement system suspend

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]

Message-ID: <YajAwvruDCzaR5wS@sirena.org.uk>
Date:   Thu, 2 Dec 2021 12:49:06 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH] ASoC: cs42l42: Implement system suspend

On Thu, Dec 02, 2021 at 09:53:33AM +0000, Charles Keepax wrote:
> On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote:

> > ...that's based on the assumption that this is all about the magic write
> > sequences you're using for shutdown potentially conflicting with default
> > values you've got in the cache?  If it's not those then the assumption
> > is that either the device was reset in which case it has reset values or
> > the device was not reset and held it's previous value, in which case the
> > cache sync is redundant.

> This isn't quite accurate though, as whilst the device was
> suspended the user may have touched ALSA controls which will have

> updated the state of the cache. Thus the cache requires a sync
> regardless of if the hardware turned off.

Right, an actual description of an actual issue.  Though how would they
have touched the ALSA controls during system suspend?  Userspace should
halted before we start suspending devices so there shouldn't be anything
new coming in from the application layer while the device is in cache
only mode.  The sound card as a whole should've been suspended first so
nothing should be coming in from there either.

> I suspect we do need to have a think about the write sequences,
> but there is also a more general issue here. The sequence looks
> like this:

> 1) Device enters cache only mode.
> 2) User writes an ALSA control causing a register to return to
> its default value.
> 3) Device exits cache only and does a cache sync.

This is a thing that happens for runtime suspend but for runtime suspend
we have good tracking of if the device lost power so we shouldn't just
be marking the cache as dirty unconditionally.  For system suspend there
shouldn't be a need to worry about userspace changing anything, and
anything coming in from the rest of the kernel should be very limited.

In any case this isn't something that should be hacked around in an
individual driver, like I say whatever the driver is trying to do needs
to be written in a clear and obvious fashion.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)