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]

Message-ID: <SJ0PR11MB56618B8B6DE3922613E83C15E7A89@SJ0PR11MB5661.namprd11.prod.outlook.com>
Date:   Tue, 28 Sep 2021 16:43:19 +0000
From:   Ryan Lee <RyanS.Lee@...imintegrated.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>
CC:     "guennadi.liakhovetski@...ux.intel.com" 
        <guennadi.liakhovetski@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "ryan.lee.maxim@...il.com" <ryan.lee.maxim@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "sathya.prakash.m.r@...el.com" <sathya.prakash.m.r@...el.com>,
        "yung-chuan.liao@...ux.intel.com" <yung-chuan.liao@...ux.intel.com>
Subject: RE: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Monday, September 27, 2021 10:23 AM
> To: Mark Brown <broonie@...nel.org>
> Cc: guennadi.liakhovetski@...ux.intel.com; alsa-devel@...a-project.org;
> ryan.lee.maxim@...il.com; Ryan Lee <RyanS.Lee@...imintegrated.com>;
> linux-kernel@...r.kernel.org; tiwai@...e.com; lgirdwood@...il.com;
> sathya.prakash.m.r@...el.com; yung-chuan.liao@...ux.intel.com
> Subject: Re: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty
> before entering sleep
> 
> EXTERNAL EMAIL
> 
> 
> 
> On 9/27/21 12:10 PM, Mark Brown wrote:
> > On Mon, Sep 27, 2021 at 11:48:56AM -0500, Pierre-Louis Bossart wrote:
> >> On 9/27/21 11:06 AM, Mark Brown wrote:
> >
> >>> More specifically what it does is make the invalidation of the
> >>> register cache unconditional.  It doesn't really matter if the
> >>> invalidation is done on suspend or resume, so long as it happens
> >>> before we attempt to resync - this could also be done by deleting the
> first_hw_init check.
> >
> >> Mark, that's exactly my point: if the amp rejoins the bus, we will
> >> *always* mark the cache as dirty, before the resync is done in the
> >> resume sequence.
> >
> > Ah, yes - I see.
> >
> >> I am really trying to figure out if we have a major flaw in the
> >> resume sequence and why things are different in the case of the Maxim
> amp.
> >
> >> 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.
> >
> > A quick survey of other drivers suggests that this pattern should be
> > factored out into some helpers as it looks like there's several ways
> > of implementing it that look very similar but not quite the same...
> 
> No disagreement here, we tried really hard to enforce a common pattern for
> suspend-resume, but i just noticed that the maxim amp driver is different on
> suspend (resume is consistent with the rest).

OK. I believe it was similar before. But it looks like 'regcache_mark_dirty' is being
disappeared on suspend function.

static int __maybe_unused rt5682_dev_suspend(struct device *dev)
{
	struct rt5682_priv *rt5682 = dev_get_drvdata(dev);

	if (!rt5682->hw_init)
		return 0;

	cancel_delayed_work_sync(&rt5682->jack_detect_work);

	regcache_cache_only(rt5682->regmap, true);
	regcache_mark_dirty(rt5682->regmap);

	return 0;
}

> 
> 
> static int __maybe_unused rt711_dev_suspend(struct device *dev) {
>         struct rt711_priv *rt711 = dev_get_drvdata(dev);
> 
>         if (!rt711->hw_init)
>                 return 0;
> 
>         cancel_delayed_work_sync(&rt711->jack_detect_work);
>         cancel_delayed_work_sync(&rt711->jack_btn_check_work);
>         cancel_work_sync(&rt711->calibration_work);
> 
>         regcache_cache_only(rt711->regmap, true);
> 
>         return 0;
> }
> 
> static int __maybe_unused rt1308_dev_suspend(struct device *dev) {
>         struct rt1308_sdw_priv *rt1308 = dev_get_drvdata(dev);
> 
>         if (!rt1308->hw_init)
>                 return 0;
> 
>         regcache_cache_only(rt1308->regmap, true);
> 
>         return 0;
> }
> 
> static __maybe_unused int max98373_suspend(struct device *dev) {
>         struct max98373_priv *max98373 = dev_get_drvdata(dev);
>         int i;
> 
> <<<< missing test
> 
>         /* cache feedback register values before suspend */
>         for (i = 0; i < max98373->cache_num; i++)
>                 regmap_read(max98373->regmap, max98373->cache[i].reg,
> &max98373->cache[i].val);
> 
> <<<< why is this needed???
[] 
It looks like this was added to get a last ADC values when ADC value read is not available during suspension.
https://www.spinics.net/lists/alsa-devel/msg119808.html

> 
>         regcache_cache_only(max98373->regmap, true);
> 
>         return 0;
> }
> 
>