linux-kernel - Re: [PATCH v2 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

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: <20160328190143.GC2350@sirena.org.uk>
Date:	Mon, 28 Mar 2016 20:01:43 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Andreas Dannenberg <dannenberg@...com>
Cc:	alsa-devel@...a-project.org, devicetree@...r.kernel.org,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ASoC: codecs: add support for TAS5720 digital
 amplifier

On Mon, Mar 21, 2016 at 12:08:27PM -0500, Andreas Dannenberg wrote:

> +static int tas5720_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> +				  unsigned int freq, int dir)
> +{
> +	/*
> +	 * Nothing to configure here for TAS5720. It's a simple codec slave.
> +	 * However we need to keep this function in here otherwise the ASoC
> +	 * platform driver will throw an ENOTSUPP at us when trying to play
> +	 * audio.
> +	 */
> +
> +	return 0;
> +}

Remove empty funnctions, -ENOTSUPP is expected behaviour for anything
that isn't explicitly supported by a driver.

> +	if (unlikely(!tx_mask)) {

unlikely() is for optimising hot paths, just write the logic clearly
unless there's a reason for it.

> +static irqreturn_t tas5720_irq_handler(int irq, void *_dev)
> +{
> +	/*
> +	 * Immediately disable TAS5720 FAULTZ interrupts inside the low-level
> +	 * handler to prevent the system getting saturated or even overrun
> +	 * by interrupt requests. Normally the fact that we create a threaded
> +	 * interrupt with IRQF_ONESHOT should take care of this as by design
> +	 * it masks interrupts while the thread is processed however testing
> +	 * has shown that at the high frequency the FAULTZ signal triggers
> +	 * (every 300us!) occasionally the system would lock up even with a
> +	 * threaded handler that's completely empty until the Kernel breaks the
> +	 * cycle, disables that interrupt, and reports a "nobody cared" error.
> +	 *
> +	 * Disabling the interrupt here combined with a deferred re-enabling
> +	 * after the thread has run not only prevents this lock condition but
> +	 * also helps to rate-limit the processing of FAULTZ interrupts.
> +	 */
> +	disable_irq_nosync(irq);

No, this is completely broken.  Whatever is going on in your system with
the interrupt core you need to address that at the appropriate level not
by putting a nonsensical bodge in here.  The interrupt is disabled while
the threaded handler is running, if that's not having the desired effect
then whatever causes that needs to be fixed.

> +static int tas5720_dapm_post_event(struct snd_soc_dapm_widget *w,
> +				   struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	int ret;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		/*
> +		 * Check if the codec is still powered up in which case exit
> +		 * right away also skipping the shutdown-to-active wait time.
> +		 */
> +		ret = snd_soc_test_bits(codec, TAS5720_POWER_CTRL_REG,
> +					TAS5720_SDZ, 0);

I don't understand this.  Why on earth would we be calling the PMU
handler if the widget was not previously powered? 

> +		/*
> +		 * Take TAS5720 out of shutdown mode in preparation for widget
> +		 * power up.
> +		 */
> +		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
> +					  TAS5720_SDZ, TAS5720_SDZ);
> +		if (ret < 0) {
> +			dev_err(codec->dev, "error waking codec: %d\n", ret);
> +			return ret;
> +		}

This is a _POST_PMU handler not a pre-PMU handler...

> +	/* Events used to control the TAS5720 SHUTDOWN state */
> +	SND_SOC_DAPM_PRE("Pre Event", tas5720_dapm_pre_event),
> +	SND_SOC_DAPM_POST("Post Event", tas5720_dapm_post_event),

Oh, we're using _PRE() and _POST() events...  this almost certainly
indicates a problem, there are very few circumstances where these are a
good idea and I'm not seeing anything in this driver which indicates
that this is going on.  Please just use normal DAPM widgets (I'm
guessing a PGA) to represent the device and work within DAPM, don't
shoehorn some bodge around the side.

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