linux-kernel - Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open

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: <5ef4351f-e9ed-1a38-b79e-53e62a70437e@sholland.org>
Date:   Mon, 5 Oct 2020 23:43:51 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Chen-Yu Tsai <wens@...e.org>, Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Ondrej Jirman <megous@...ous.com>, alsa-devel@...a-project.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while
 streams are open

On 10/5/20 7:01 AM, Maxime Ripard wrote:
> On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
>> The codec's clock input is shared among all AIFs, and shared with other
>> audio-related hardware in the SoC, including I2S and SPDIF controllers.
>> To ensure sample rates selected by userspace or by codec2codec DAI links
>> are maintained, the clock rate must be protected while it is in use.
>>
>> Signed-off-by: Samuel Holland <samuel@...lland.org>
>> ---
>>  sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
>> index 501af64d43a0..86065bee7cd3 100644
>> --- a/sound/soc/sunxi/sun8i-codec.c
>> +++ b/sound/soc/sunxi/sun8i-codec.c
>> @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned int slots,
>>  	unsigned int div = slots * slot_width;
>>  
>>  	if (div < 16 || div > 256)
>>  		return -EINVAL;
>>  
>>  	return order_base_2(div);
>>  }
>>  
>> +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
>> +{
>> +	return sample_rate % 4000 ? 22579200 : 24576000;
>> +}
>> +
>>  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>>  				 struct snd_pcm_hw_params *params,
>>  				 struct snd_soc_dai *dai)
>>  {
>>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
>>  	struct sun8i_codec_aif *aif = &scodec->aifs[dai->id];
>>  	unsigned int sample_rate = params_rate(params);
>>  	unsigned int slots = aif->slots ?: params_channels(params);
>>  	unsigned int slot_width = aif->slot_width ?: params_width(params);
>> -	unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
>> -	int lrck_div_order, word_size;
>> +	unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
>> +	int lrck_div_order, ret, word_size;
>>  	u8 bclk_div;
>>  
>>  	/* word size */
>>  	switch (params_width(params)) {
>>  	case 8:
>>  		word_size = 0x0;
>>  		break;
>>  	case 16:
>> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>>  			   (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
>>  
>>  	/* BCLK divider (SYSCLK/BCLK ratio) */
>>  	bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
>>  	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>>  			   SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
>>  			   bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
>>  
>> -	if (!aif->open_streams) {
>> +	/* SYSCLK rate */
>> +	if (aif->open_streams) {
>> +		ret = clk_set_rate(scodec->clk_module, sysclk_rate);
>> +		if (ret < 0)
>> +			return ret;
>> +	} else {
>> +		ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
> 
> It's not really clear to me why we wouldn't want to always protect the
> clock rate here?

>From Documentation/sound/kernel-api/writing-an-alsa-driver.rst:

    hw_params callback
    ...
    Note that this and ``prepare`` callbacks may be called multiple
    times per initialization. For example, the OSS emulation may call
    these callbacks at each change via its ioctl.

Clock rate protection is reference counted, so we must only take one
reference (or at least a known number of references) per stream.

>> +		if (ret == -EBUSY)
>> +			dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz "
>> +				"conflicts with other audio streams.\n",
> 
> This string creates a checkpatch warning.

I will put it on one line, though >100 columns is also a checkpatch warning.

> Maxime

Cheers,
Samuel