Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2020-08-19 4:21 PM, Andy Shevchenko wrote:
> On Wed, Aug 19, 2020 at 03:46:30PM +0200, Cezary Rojewski wrote:
>> On 2020-08-18 1:50 PM, Andy Shevchenko wrote:
>>> On Mon, Aug 17, 2020 at 01:12:01PM +0200, Cezary Rojewski wrote:
>>>> On 2020-08-13 8:51 PM, Andy Shevchenko wrote:
>>>>> On Wed, Aug 12, 2020 at 10:57:42PM +0200, Cezary Rojewski wrote:

>>>>>> +	bool lp;
>>>>>> +
>>>>>> +	if (list_empty(&cdev->stream_list))
>>>>>> +		return catpt_dsp_select_lpclock(cdev, true, true);
>>>>>> +
>>>>>> +	lp = true;
>>>>>> +	list_for_each_entry(stream, &cdev->stream_list, node) {
>>>>>> +		if (stream->prepared) {
>>>>>> +			lp = false;
>>>>>> +			break;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return catpt_dsp_select_lpclock(cdev, lp, true);
>>>>>
>>>>> Seems too much duplication.
>>>>>
>>>>> 	struct catpt_stream_runtime *stream;
>>>>>
>>>>> 	list_for_each_entry(stream, &cdev->stream_list, node) {
>>>>> 		if (stream->prepared)
>>>>> 			return catpt_dsp_select_lpclock(cdev, false, true);
>>>>> 	}
>>>>>
>>>>> 	return catpt_dsp_select_lpclock(cdev, true, true);
>>>>>
>>>>>
>>>>> Better?
>>>>
>>>> list_first_entry (part of list_for_each_entry) expects list to be non-empty.
>>>> ->streal_list may be empty when invoking catpt_dsp_update_lpclock().
>>>
>>> I didn't get this. Can you point out where is exactly problematic place?
>>>
>>
>> list_for_each_entry makes use of list_first_entry when initializing 'pos'
>> index variable.
> 
> Correct.
> 
>> Documentation for list_first_entry reads: "Note, that list
>> is expected to be not empty"
> 
> Correct.
> 
>> so I'm validating list's status before moving
>> on to the loop as stream_list may be empty when catpt_dsp_update_lpclock()
>> gets called.
> 
> But here you missed the second part of the for-loop, i.e. exit conditional.
> 
> If your assumption (that list_for_each_*() is not empty-safe) is correct,
> it would be disaster in global kernel source level.
> 

We want no disasters here : )
safety-out. Ack.