linux-kernel - Re: [alsa-devel] [RESEND PATCH v5 2/6] ASoC: amd: Refactoring of DAI from DMA 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]

Message-ID: <40d1690c-175c-c6ee-ee44-390c30cccc05@linux.intel.com>
Date:   Wed, 13 Nov 2019 10:22:19 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Ravulapati Vishnu vardhan rao 
        <Vishnuvardhanrao.Ravulapati@....com>
Cc:     "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>,
        Maruthi Bayyavarapu <maruthi.bayyavarapu@....com>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        open list <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Liam Girdwood <lgirdwood@...il.com>, Akshu.Agrawal@....com,
        Sanju R Mehta <sanju.mehta@....com>,
        Mark Brown <broonie@...nel.org>, djkurtz@...gle.com,
        Vijendar Mukunda <Vijendar.Mukunda@....com>,
        Alex Deucher <alexander.deucher@....com>,
        Colin Ian King <colin.king@...onical.com>
Subject: Re: [alsa-devel] [RESEND PATCH v5 2/6] ASoC: amd: Refactoring of DAI
 from DMA driver


> +	val = rv_readl(adata->acp3x_base + mmACP_BTTDM_ITER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_BTTDM_ITER);
> +	val = rv_readl(adata->acp3x_base + mmACP_BTTDM_IRER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_BTTDM_IRER);
> +
> +	val = (FRM_LEN | (slots << 15) | (slot_len << 18));

nit-pick: you have extra parentheses that are not needed for (val | 
0x02) and the outer ones on the previous line


> +static int acp3x_i2s_trigger(struct snd_pcm_substream *substream,
> +		int cmd, struct snd_soc_dai *dai)
> +{
> +	int ret = 0;

nit-pick: move last, xmas-style

> +	struct i2s_stream_instance *rtd = substream->runtime->private_data;
> +	u32 val, period_bytes;

> +static int acp3x_dai_probe(struct platform_device *pdev)
> +{
> +	int status;
> +	struct resource *res;
> +	struct i2s_dev_data *adata;
> +
> +	adata = devm_kzalloc(&pdev->dev, sizeof(struct i2s_dev_data),
> +			GFP_KERNEL);
> +	if (!adata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
> +		goto err;
> +	}
> +
> +	adata->acp3x_base = devm_ioremap(&pdev->dev, res->start,
> +			resource_size(res));
> +	if (IS_ERR(adata->acp3x_base))
> +		return PTR_ERR(adata->acp3x_base);
> +
> +	adata->i2s_irq = res->start;
> +	dev_set_drvdata(&pdev->dev, adata);
> +	status = devm_snd_soc_register_component(&pdev->dev,
> +			&acp3x_dai_component,
> +			&acp3x_i2s_dai, 1);
> +	if (status) {
> +		dev_err(&pdev->dev, "Fail to register acp i2s dai\n");
> +		goto dev_err;
> +	}
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 10000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	return 0;
> +err:
> +	kfree(adata);
> +	return -ENOMEM;
> +dev_err:
> +	kfree(adata->acp3x_base);
> +	kfree(adata);
> +	kfree(res);
> +	return -ENODEV;

this can be improved a bit by using ret = -ENOMEM/-ENODEV before the 
goto, and organizing the labels and the kfree calls in the reverse order 
of the initialization/allocation steps.


> @@ -666,7 +461,24 @@ static int acp3x_audio_probe(struct platform_device *pdev)
>   	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_enable(&pdev->dev);
>   	return 0;
> +
> +err:
> +	kfree(res);
> +	return -ENOMEM;
> +base_err:
> +	kfree(res);
> +	kfree(adata);
> +	return -ENOMEM;
> +io_irq:
> +	kfree(res);
> +	kfree(adata->acp3x_base);
> +	kfree(adata);
> +	return -ENOMEM;
> +
>   dev_err:
> +	kfree(res);
> +	kfree(adata->acp3x_base);
> +	kfree(adata);
>   	status = acp3x_deinit(adata->acp3x_base);
>   	if (status)
>   		dev_err(&pdev->dev, "ACP de-init failed\n");
same here, you should have all the kfrees in the reverse order of the 
kzalloc, and labels pointing straight at the sequence that needs to be 
executed. Duplicating the error flow makes it hard to maintain the code 
and check for memory leaks.