[alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support

[Rohit Kumar]

Hello Vinod,


On 6/19/2018 9:52 PM, Vinod wrote:
> Hi Rohit,
>
> On 19-06-18, 19:20, Rohit Kumar wrote:
>> On 6/19/2018 10:35 AM, Vinod wrote:
>>> On 18-06-18, 16:46, Rohit kumar wrote:
>>>
>>>> +struct sdm845_snd_data {
>>>> +	struct snd_soc_card *card;
>>>> +	struct regulator *vdd_supply;
>>>> +	struct snd_soc_dai_link dai_link[];
>>>> +};
>>>> +
>>>> +static struct mutex pri_mi2s_res_lock;
>>>> +static struct mutex quat_tdm_res_lock;
>>> any reason why the locks can't be part of sdm845_snd_data?
>>> Also why do we need two locks ?
>> No specific reason, I will move it to sdm845_snd_data.
>> These locks are used to protect enable/disable of bit clocks. We have
>> Primary MI2S RX/TX
>> and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have single
>> clock which is
>> synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
>> quat_tdm_res_lock.
>> We need two locks as we are protecting two different resources.
> I think bigger question is why do you need any locks? What is the race
> scenario you envision which needs protection
>

Below is one of the race condition:

Thread1                          |         Thread2
----------------------------------------------------------
startup()                         |
count++;                         |    startup()
read count (count = 1) |
enable_clock()               |    count++;   //count = 2
shutdown()                    |
count--;// count = 1      |
                                          | read count (count = 1)
                                          | enable_clock()

Here clock will be enabled twice but disable will be called only once 
when count = 0.

This will make the clock always enabled. So, I think we should keep 
either mutex lock or atomic variable to synchronize this.

Regards,
Rohit

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.