linux-kernel - Re: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before entering sleep

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]

Date:   Mon, 27 Sep 2021 14:34:09 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Ryan Lee <RyanS.Lee@...imintegrated.com>,
        Mark Brown <broonie@...nel.org>
Cc:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "yung-chuan.liao@...ux.intel.com" <yung-chuan.liao@...ux.intel.com>,
        "guennadi.liakhovetski@...ux.intel.com" 
        <guennadi.liakhovetski@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "sathya.prakash.m.r@...el.com" <sathya.prakash.m.r@...el.com>,
        "ryan.lee.maxim@...il.com" <ryan.lee.maxim@...il.com>
Subject: Re: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep

>> Instead of changing the suspend sequence, can we please try to modify the
>> max98373_io_init() routine to unconditionally flag the cache as dirty, maybe
>> this points to a problem with the management of the
>> max98373->first_hw_init flag.
> 
> max98373_io_init() is not called because ' sdw_slave_status' remains
> ' SDW_SLAVE_ATTACHED' and 'max98373->hw_init' is already true.
> Removing 'if (max98373->hw_init || status != SDW_SLAVE_ATTACHED)'
> condition in max98373_update_status() function instead of adding
> regcache_mark_dirty() into max98373_suspend() can be an alternative way.
> I think it is all about where regcache_mark_dirty() is called from.
> The difference is that max98373_io_init() really do the software reset and
> do amp initialization again which could be an overhead.

that description is aligned with my analysis that there's something very
wrong happening here, it's not just a simple miss in the regmap handling
but a major conceptual bug or misunderstanding in the way reset is handled.

First, there's the spec: on a reset initiated by the host or if the
device loses sync for ANY reason, its status cannot remain ATTACHED.
There's got to be a 16-frame period at least where the device has to
monitor the sync pattern and cannot drive anything on the bus.

Then there's the hardware behavior on resume: on resume by default the
Intel host will toggle the data pin for at least 4096 frames, which by
spec means severe reset.

And last, there's the software init: we also force the status as
UNATTACHED in drivers/soundwire/intel.c:

	/*
	 * make sure all Slaves are tagged as UNATTACHED and provide
	 * reason for reinitialization
	 */
	sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET);

But we've also seen the opposite effect of an amplifier reporting
attached but losing sync immediately after the end of enumeration and
never coming back on the bus, see issue
https://github.com/thesofproject/linux/issues/3063

In other words, we need to check what really happens on resume and why
the amplifier keeps reporting its status as ATTACHED despite the spec
requirements and software init, or loses this status after
enumeration....Something really does not add-up, again it's not just a
regmap management issue.