linux-kernel - Re: [PATCH v3 4/5] ASoC: dwc: Add devicetree support for Designware I2S

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]

Date:	Mon, 22 Dec 2014 14:10:14 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Andrew Jackson <andrew.jackson@....com>
Cc:	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Rajeev Kumar <rajeevkumar.linux@...il.com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Arnd Bergmann <arnd@...db.de>,
	linux-arm-kernel@...ts.infradead.org, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/5] ASoC: dwc: Add devicetree support for Designware
 I2S

On Fri, Dec 19, 2014 at 04:18:08PM +0000, Andrew Jackson wrote:

> +union snd_dma_data {
> +	struct i2s_dma_data pd;
> +	struct snd_dmaengine_dai_dma_data dt;
> +};
> +

This is a driver local union with a very generic name, it seems likely
that this will collide in future causing build breaks

> -	ret = dev->i2s_clk_cfg(config);
> -	if (ret < 0) {
> -		dev_err(dev->dev, "runtime audio clk config fail\n");
> -		return ret;
> +		/* TODO: Validate sample rate against permissible set */
> +		bitclk = config->sample_rate * config->data_width * 2;
> +		clk_set_rate(dev->clk, bitclk);
>  	}

This is ignoring errors in clk_set_rate().

> +/* Maximum resolution of a channel - not uniformly spaced */
> +static const u32 fifo_width[] = {
> +	12, 16, 20, 24, 32, 0, 0, 0
> +};
> +
> +/* Width of (DMA) bus */
> +static const u32 bus_widths[] = {
> +	DMA_SLAVE_BUSWIDTH_1_BYTE,
> +	DMA_SLAVE_BUSWIDTH_2_BYTES,
> +	DMA_SLAVE_BUSWIDTH_4_BYTES,
> +	DMA_SLAVE_BUSWIDTH_UNDEFINED
> +};

> +	u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
> +	u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
> +	u32 max_size;

I'd feel a lot more comfortable if there were bounds checking on these
array indexes, especially since the arrays aren't explicitly sized and
instead just have the number of elements that is (hopefully) safe with
no comments or anything.  As things stand this is all using really
fraigle idioms, this could easily be broken if someone is updating the
driver for new IP features or even just cleaning up the code.

> -	dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
> -	dev->clk = clk_get(&pdev->dev, NULL);
> +		dev->clk = clk_get(&pdev->dev, NULL);
> +	} else {
> +		dw_configure_dai_by_dt(dev, dw_i2s_dai, res);
> +
> +		dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
> +	}

This changes from clk_get() to devm_clk_get() but I'm not seeing
anything that removes clk_put() calls.

> +#ifdef CONFIG_OF
> +		.of_match_table = dw_i2s_of_match,
> +#endif

of_match_ptr().

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