linux-kernel - Re: [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver

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] [day] [month] [year] [list]

Date:   Thu, 20 Dec 2018 15:06:56 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Maruthi Srinivas Bayyavarapu 
        <maruthi.srinivas.bayyavarapu@...inx.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Vishal Sagar <vsagar@...inx.com>,
        Michal Simek <michal.simek@...inx.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        alsa-devel@...a-project.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver

On Wed, Dec 19, 2018 at 10:49:08PM +0530, Maruthi Srinivas Bayyavarapu wrote:

This looks mostly good, a few small things but all pretty trivial:

> +	switch (bits_per_sample) {
> +	case 8:
> +		val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 16:
> +		val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 20:
> +		val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 24:
> +		val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	case 32:
> +		val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		break;
> +	}

It'd be better to have a default case returning an error for unsupported
sample sizes.

> +		ret = devm_request_irq(dev, aud_drv_data->mm2s_irq,
> +				       xlnx_mm2s_irq_handler, 0,
> +				       "xlnx_formatter_pcm_mm2s_irq", dev);
> +		if (ret) {
> +			dev_err(dev, "xlnx audio mm2s irq request failed\n");
> +			goto clk_err;
> +		}
> +		ret = xlnx_formatter_pcm_reset(aud_drv_data->mmio +
> +					       XLNX_MM2S_OFFSET);
> +		if (ret) {
> +			dev_err(dev, "audio formatter reset failed\n");
> +			goto clk_err;
> +		}
> +		xlnx_formatter_disable_irqs(aud_drv_data->mmio +
> +					    XLNX_MM2S_OFFSET,
> +					    SNDRV_PCM_STREAM_PLAYBACK);

This requests the interrupt before we've reset and quiesced the hardware
meaning the interrupt could fire immediately on request with the
hardware in an unknown state; it'd be better to reset the hardware first
then request the interrupt to minimize the risk that something goes
wrong with the handler.

> +	dev_info(dev, "pcm platform device registered\n");
> +	return 0;

This is just adding noise to the log.

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