linux-kernel - Re: [PATCH v4 12/22] ASoC: tegra20: spdif: Filter out unsupported rates

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:   Thu, 16 Dec 2021 16:30:29 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Mark Brown <broonie@...nel.org>, Takashi Iwai <tiwai@...e.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Liam Girdwood <lgirdwood@...il.com>,
        Agneli <poczt@...tonmail.ch>, linux-tegra@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v4 12/22] ASoC: tegra20: spdif: Filter out unsupported
 rates

On Sat, Dec 04, 2021 at 05:37:15PM +0300, Dmitry Osipenko wrote:
> SPDIF and other SoC components share audio PLL on Tegra, thus only one
> component may set the desired base clock rate. This creates problem for
> HDMI audio because it uses SPDIF and audio may not work if SPDIF's clock
> doesn't exactly match standard audio rate since some receivers may reject
> audio in that case. Filter out audio rates which SPDIF output can't
> support, assuming that other components won't change rate at runtime.
> 
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  sound/soc/tegra/tegra20_spdif.c | 61 +++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
> index a4aa5614aef4..d09cd7ee6879 100644
> --- a/sound/soc/tegra/tegra20_spdif.c
> +++ b/sound/soc/tegra/tegra20_spdif.c
> @@ -79,6 +79,7 @@ static int tegra20_spdif_hw_params(struct snd_pcm_substream *substream,
>  	struct tegra20_spdif *spdif = dev_get_drvdata(dai->dev);
>  	unsigned int mask = 0, val = 0;
>  	int ret, spdifclock;
> +	long rate;

Perhaps we should change spdifclock to be "long" as well, even though
they're the same on Tegra20. SND_SOC_TEGRA depends on || COMPILE_TEST,
so technically somebody could build this on 64-bit and then these are
no longer the same types.

Still shouldn't be problematic given the numbers that are passed in,
but it'd still be nice to keep consistent types for these.

>  
>  	mask |= TEGRA20_SPDIF_CTRL_PACK |
>  		TEGRA20_SPDIF_CTRL_BIT_MODE_MASK;
> @@ -133,6 +134,12 @@ static int tegra20_spdif_hw_params(struct snd_pcm_substream *substream,
>  		return ret;
>  	}
>  
> +	rate = clk_get_rate(spdif->clk_spdif_out);
> +	if (rate != spdifclock)
> +		dev_warn_once(dai->dev,
> +			      "SPDIF clock rate %d doesn't match requested rate %lu\n",

That should be %ld since it's not unsigned.

Otherwise, looks good. Probably not worth respinning for just these
minor comments, so:

Acked-by: Thierry Reding <treding@...dia.com>

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