linux-kernel - Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers

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: <20131126134411.GQ14725@sirena.org.uk>
Date:	Tue, 26 Nov 2013 13:44:11 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Xiubo Li <Li.Xiubo@...escale.com>
Cc:	lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.de,
	fabio.estevam@...escale.com, LW@...O-electronics.de,
	oskar@...ra.com, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting
 regulator consumers

On Tue, Nov 26, 2013 at 05:55:13PM +0800, Xiubo Li wrote:

> +static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec)
> +{
> +	struct regulator *consumer;
> +	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> +
> +	consumer = regulator_get(codec->dev, sgtl5000->supplies[VDDD].supply);
> +	if (IS_ERR(consumer)) {
> +		return 0;
> +	}
> +	regulator_put(consumer);
> +
> +	return 1;
> +}

This is going to fail to do what you expect if a dummy regulator is
substituted which it will almost all of the time when the supply is
genuinely absent.  It's also going to interact poorly with probe
deferral.

> -	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
> -				sgtl5000->supplies);
> -	if (!ret)
> +	if (sgtl5000_external_vddd_used(codec))
>  		external_vddd = 1;
>  	else {
>  		ret = sgtl5000_replace_vddd_with_ldo(codec);

This looks buggy, it removes the regulator_get() for most of the
supplies for the device.

I think the driver may want to use regulator_get_optional(), though
looking at the fact that there's an external_vdd flag there already it
seems like there's already some attempt to handle these configuratins in
the driver that isn't working so perhaps that just needs to be fixed.
Or possibly DTs need to be changed to describe the supply.  

I'd expect to see a commit description that describes how the driver
currently tries to handle this, why it doesn't work and how the patch
fixes it.  Right now it seems like the change is jumping to a solution
without understanding the problem and making some already problematic
code more confused.

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