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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux