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

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
	harshapriya.n@intel.com, marcin.barlik@intel.com,
	zwisler@google.com, lgirdwood@gmail.com, tiwai@suse.com,
	filip.proborszcz@intel.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
	cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
	ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations
Date: Wed, 19 Aug 2020 17:21:28 +0300	[thread overview]
Message-ID: <20200819142128.GP1891694@smile.fi.intel.com> (raw)
In-Reply-To: <9a733e30-8d44-edf7-1bae-5b6f935628d2@intel.com>

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.

-- 
With Best Regards,
Andy Shevchenko