linux-kernel - Re: [alsa-devel] [PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register access operations

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: <33ec14ef-7b05-544d-58ba-0e14aa2a97d4@maciej.szmigiero.name>
Date:   Tue, 21 Nov 2017 12:27:50 +0100
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Nicolin Chen <nicoleotsuka@...il.com>
Cc:     Timur Tabi <timur@...i.org>, Xiubo Li <Xiubo.Lee@...il.com>,
        alsa-devel@...a-project.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Fabio Estevam <fabio.estevam@....com>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [alsa-devel] [PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register
 access operations

On 21.11.2017 02:52, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:16:07PM +0100, Maciej S. Szmigiero wrote:
(..)
>>  static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
>> @@ -1287,16 +1295,18 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
>>  {
>>  	struct regmap *regs = fsl_ac97_data->regs;
>>  
>> -	unsigned short val = -1;
>> +	unsigned short val = 0;
>>  	u32 reg_val;
>>  	unsigned int lreg;
>>  	int ret;
>>  
>> +	mutex_lock(&fsl_ac97_data->ac97_reg_lock);
>> +
>>  	ret = clk_prepare_enable(fsl_ac97_data->clk);
>>  	if (ret) {
>>  		pr_err("ac97 read clk_prepare_enable failed: %d\n",
>>  			ret);
>> -		return -1;
>> +		goto ret_unlock;
> 
> It will return val (== 0) in this case. Will this be correctly
> handled by callers? I find sound/ac97/bus.c checks if ret < 0
> for ops->read().

Both 0 and -1 (0xffff really) are valid register values.

AC'97 code that is used in companion with fsl_ssi lives in sound/pci/ac97
directory and does register reads via snd_ac97_read() function in
ac97_codec.c file (located in that directory).
This function has no such check.

The reason why AC'97 specs call for unknown register read to return zero
is that if there is some capability bit in a register then this bit is
set when a CODEC has such capability.
If we return -1 then it means that in this case a CODEC would be
detected as having all possible capabilities that are exposed via this
register (including these that aren't really supported) while if we
return 0 instead then we merely wouldn't make use of some that are actually
present.

> 
> So it might be better to add "val = ret;" before goto? Or use
> val instead of ret directly?

Then we would be returning an error number from clk_prepare_enable()
as a register value which would be almost certainly wrong.

Note that this method returns unsigned short - a type with the same width
as an AC'97 register, so there are no unused values in this type which
could be reliably used to signify that an error had happened.

Maciej