linux-kernel - Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

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: <20190730141935.GF4264@sirena.org.uk>
Date:   Tue, 30 Jul 2019 15:19:35 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Thomas Preston <thomas.preston@...ethink.co.uk>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Paul Cercueil <paul@...pouillou.net>,
        Kirill Marinushkin <kmarinushkin@...dec.tech>,
        Cheng-Yi Chiang <cychiang@...omium.org>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Vinod Koul <vkoul@...nel.org>,
        Annaliese McDermond <nh6z@...z.net>,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:

> +	struct dentry *debugfs;
> +	struct mutex diagnostic_mutex;
> +};

It is unclear what this mutex usefully protects, it only gets taken when
writing to the debugfs file to trigger this diagnostic mode but doesn't
do anything to control interactions with any other code path in the
driver.

> +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
> +{
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err_status, err;
> +	unsigned int val;
> +	u8 state[NUM_IB];

> +	/* We must wait 20ms for device to settle, otherwise diagnostics will
> +	 * not start and regmap poll will timeout.
> +	 */
> +	msleep(DIAGNOSTIC_SETTLE_MS);

The comment and define might go out of sync...

> +	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
> +	if (err < 0) {
> +		dev_err(dev, "Could not read channel status, %d\n", err);
> +		goto diagnostic_restore;
> +	}

...but here we use a magic number for the array size :(

> +static int tda7802_diagnostic_show(struct seq_file *f, void *p)
> +{
> +	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

We neither use nor free buf?

> +static int tda7802_probe(struct snd_soc_component *component)
> +{
> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err;

Why is this done at the component level?

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