Date:   Thu, 2 Dec 2021 12:49:06 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     Richard Fitzgerald <rf@opensource.cirrus.com>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com
Subject: Re: [PATCH] ASoC: cs42l42: Implement system suspend
Message-ID: <YajAwvruDCzaR5wS@sirena.org.uk>
References: <20211201153648.17259-1-rf@opensource.cirrus.com>
 <YaejghraYE6lD7FD@sirena.org.uk>
 <87930df9-6220-807e-a655-1c7d7ec020ab@opensource.cirrus.com>
 <YafyWnVA1J1rgCf1@sirena.org.uk>
 <20211202095333.GK18506@ediswmail.ad.cirrus.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="nCM9gNUjdUpogTrV"
Content-Disposition: inline
In-Reply-To: <20211202095333.GK18506@ediswmail.ad.cirrus.com>
Precedence: bulk


--nCM9gNUjdUpogTrV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--nCM9gNUjdUpogTrV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmGowMEACgkQJNaLcl1U
h9DBhQf/TTyORcnTZkHZB+p/0txthGanm/yQPncXofYRB8q3HBkAVmWZIrmPk7Wf
836TS2fc6cXWOdXCOg91On1weoFiimW3Tf+mr4uac6H3PT3seXnv/7T69kpYN735
EIVcqQdJDRrKFHw9wWUDIzq5i2Si31QiUNuud+L7HizkMsbUlZrjoeP9bREuJaYu
FgDOn2mddi4TE+Qib//o9cFik48P0vFZ7STVNoRad1PDsbnIJUhp0m7aNA3nUSNf
cVphB6f6cWKxhh6CS8TK/faraauQ69jv/yo00BWqmVxnJF/jU2D7o4fj5HqgGETS
aQzn3pUOWBxtCdzpd/7f0wd7fOXmFQ==
=Y5jC
-----END PGP SIGNATURE-----

--nCM9gNUjdUpogTrV--