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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YaejghraYE6lD7FD@sirena.org.uk>
Date:   Wed, 1 Dec 2021 16:32:02 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH] ASoC: cs42l42: Implement system suspend

On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
> Add system suspend functions to handle clean power-down on suspend and
> restoring state on resume.
> 
> The jack state could change during suspend. Plug->unplug and unplug->plug
> are straightforward because this looks no different from any other plug
> state change. However, the jack could be unplugged and a different type

This fiddling about with the jack detect feels like it should be at
least one separate change, possibly multiple changes - the reporting of
button states feels like it should be a good cleanup/fix separately for
example.

> of jack plugged, and on resume the plug state would not have changed.
> Some code changes are needed to the jack handling so that on resume a
> plugged state will be re-evaluated instead of filtered out. In this case

It would be helpful to elaborate on what these code changes might be.

> +	/*
> +	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
> +	 * detect a reset during system suspend.
> +	 */
> +	case CS42L42_PWR_CTL2:

This feels a bit hackish - is the cost of doing the cache sync really so
expensive that it's worth the effort of trying to skip it?

> +	if (cs42l42->suspended) {
> +		mutex_unlock(&cs42l42->irq_lock);
> +		return IRQ_NONE;
> +	}

Given that you're using disable_irq() to force the interrupt off (which
is a bit rude but often the best one can do) how might we be getting an
interrupt while suspended?  This seems to indicate an error condition.

>  			case CS42L42_PLUG_OMTP:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
> -						    SND_JACK_HEADSET);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
>  				break;
>  			case CS42L42_PLUG_HEADPHONE:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
> -						    SND_JACK_HEADPHONE);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);

This unconditionally clears the button status - will something notice if
the buttons are pressed?

> +	} else {
> +		/*
> +		 * Only call regcache_mark_dirty() if cs42l42 reset, so
> +		 * regcache_sync() writes all non-default cached values.
> +		 * If it didn't reset we don't want to filter out syncing
> +		 * dirty cache entries that have default value.
> +		 * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset
> +		 * it will now be 0.
> +		 */

Something needs to tell regmap that the cache is dirty otherwise it
won't sync anything, including the non-default register values?  There's
currently an assumption coded in there that the cache is dirty because
the device was reset and everything has default values.

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