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  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)

Powered by blists - more mailing lists