linux-kernel - Re: [PATCH v2 2/5] ASoC: samsung: i2s: add support for FSD I2S

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]

Message-ID: <0cb682bd-7f1b-009d-6f1a-1a5a46366fe8@linaro.org>
Date:   Tue, 3 Jan 2023 12:08:41 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Padmanabhan Rajanbabu <p.rajanbabu@...sung.com>,
        lgirdwood@...il.com, broonie@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, s.nawrocki@...sung.com,
        perex@...ex.cz, tiwai@...e.com, pankaj.dubey@...sung.com,
        alim.akhtar@...sung.com, rcsekar@...sung.com,
        aswani.reddy@...sung.com
Cc:     alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v2 2/5] ASoC: samsung: i2s: add support for FSD I2S

On 03/01/2023 05:56, Padmanabhan Rajanbabu wrote:
> Add support for enabling I2S controller on FSD platform.
> 
> FSD I2S controller is based on Exynos7 I2S controller, supporting
> 2CH playback/capture in I2S mode and 7.1CH playback/capture in TDM
> mode.
> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@...sung.com>
> ---
>  sound/soc/samsung/i2s-regs.h |  1 +
>  sound/soc/samsung/i2s.c      | 57 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
> index b4b5d6053503..4444c857d0c0 100644
> --- a/sound/soc/samsung/i2s-regs.h
> +++ b/sound/soc/samsung/i2s-regs.h
> @@ -132,6 +132,7 @@
>  #define EXYNOS7_MOD_RCLK_192FS	7
>  
>  #define PSR_PSREN		(1 << 15)
> +#define PSR_PSVAL(x)		(((x - 1) << 8) & 0x3f00)
>  
>  #define FIC_TX2COUNT(x)		(((x) >>  24) & 0xf)
>  #define FIC_TX1COUNT(x)		(((x) >>  16) & 0xf)
> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 9505200f3d11..dcb5c438cb2f 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -50,6 +50,10 @@ struct samsung_i2s_dai_data {
>  	u32 quirks;
>  	unsigned int pcm_rates;
>  	const struct samsung_i2s_variant_regs *i2s_variant_regs;
> +	void (*fixup_early)(struct snd_pcm_substream *substream,
> +					struct snd_soc_dai *dai);
> +	void (*fixup_late)(struct snd_pcm_substream *substream,
> +					struct snd_soc_dai *dai);
>  };
>  
>  struct i2s_dai {
> @@ -111,6 +115,10 @@ struct samsung_i2s_priv {
>  	u32 suspend_i2spsr;
>  
>  	const struct samsung_i2s_variant_regs *variant_regs;
> +	void (*fixup_early)(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai);
> +	void (*fixup_late)(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai);
>  	u32 quirks;
>  
>  	/* The clock provider's data */
> @@ -940,6 +948,10 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		pm_runtime_get_sync(dai->dev);
> +
> +		if (priv->fixup_early)
> +			priv->fixup_early(substream, dai);
> +
>  		spin_lock_irqsave(&priv->lock, flags);
>  
>  		if (config_setup(i2s)) {
> @@ -947,6 +959,13 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  			return -EINVAL;
>  		}
>  

Except several warnings this patch generates, this is a bit surprising:

> +		spin_unlock_irqrestore(&priv->lock, flags);

You have critical section which you now break into two. You cannot do
this usually. How the synchronization is now kept?

> +
> +		if (priv->fixup_late)
> +			priv->fixup_late(substream, dai);
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +
>  		if (capture)
>  			i2s_rxctrl(i2s, 1);
>  		else

Best regards,
Krzysztof