linux-kernel - Re: [PATCH 1/3] ASoC: rockchip: add support for spdifrx receiver

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: <YXaae+SnnZea1C2C@sirena.org.uk>
Date:   Mon, 25 Oct 2021 12:52:27 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Yunhao Tian <t123yh.xyz@...il.com>
Cc:     Heiko Stuebner <heiko@...ech.de>,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        alsa-devel@...a-project.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/3] ASoC: rockchip: add support for spdifrx receiver

On Sun, Oct 24, 2021 at 05:43:15PM +0800, Yunhao Tian wrote:

> --- /dev/null
> +++ b/sound/soc/rockchip/rockchip_spdifrx.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ALSA SoC Audio Layer - Rockchip SPDIF_RX Controller driver
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static int rk_spdifrx_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	return 0;
> +}

Just remove empty callbacks, if it's safe to omit a callback the
framework will let you.

> +	spin_lock_irqsave(&spdifrx->irq_lock, flags);
> +
> +	/* the access property of controls don't have to
> +	 * be volatile, as it will be notified by interrupt handler
> +	 */
> +	spdifrx->dai = dai;
> +	control = (struct snd_kcontrol_new){
> +		/* Sample Rate Control */
> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
> +		.name = SNDRV_CTL_NAME_IEC958("", CAPTURE, NONE) "Rate",
> +		/* access don't have to be volatile, as it will be notified by intr */
> +		.access = SNDRV_CTL_ELEM_ACCESS_READ,
> +		.info = rk_spdifrx_rate_info,
> +		.get = rk_spdifrx_rate_get,
> +	};
> +	spdifrx->snd_kctl_rate = snd_ctl_new1(&control, dai);

You really shouldn't be calling snd_ctl_new1() with interrupts disabled,
nor can I see a reason why you'd want to do this.  I'd be surprised if
it doesn't do any allocations or other operations that are not permitted
while in atomic context.

I also don't understand why the control is declared in this way rather
than just being a normal const static struct, there's no variable data
in any of these structs that I can see.

> +static irqreturn_t rk_spdifrx_isr(int irq, void *dev_id)
> +{

> +	spin_lock_irqsave(&spdifrx->irq_lock, flags);
> +	if (spdifrx->dai) {

You're in the interrupt handler here so this should just be a regular
spin_lock().

> +MODULE_DESCRIPTION("ROCKCHIP SPDIF Receiver Interface");
> +MODULE_AUTHOR("Sugar Zhang <sugar.zhang@...k-chips.com>");

Given that Sugar is active upstream it would be good to keep them in the
CCs.

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