Re: [PATCH v9 02/14] ASoC: Intel: catpt: Implement IPC protocol

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: "Rojewski, Cezary" <cezary.rojewski@intel.com>
Cc: "pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"Kaczmarski, Filip" <filip.kaczmarski@intel.com>,
	"N, Harshapriya" <harshapriya.n@intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"Barlik, Marcin" <marcin.barlik@intel.com>,
	"zwisler@google.com" <zwisler@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"Proborszcz, Filip" <filip.proborszcz@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"amadeuszx.slawinski@linux.intel.com"
	<amadeuszx.slawinski@linux.intel.com>,
	"Wasko, Michal" <michal.wasko@intel.com>,
	"cujomalainey@chromium.org" <cujomalainey@chromium.org>,
	"Hejmowski, Krzysztof" <krzysztof.hejmowski@intel.com>,
	"ppapierkowski@habana.ai" <ppapierkowski@habana.ai>,
	"Gopal, Vamshi Krishna" <vamshi.krishna.gopal@intel.com>
Subject: Re: [PATCH v9 02/14] ASoC: Intel: catpt: Implement IPC protocol
Date: Tue, 29 Sep 2020 13:43:24 +0300	[thread overview]
Message-ID: <20200929104324.GC3956970@smile.fi.intel.com> (raw)
In-Reply-To: <39c11ae42aa84cd3b03e8cc376e57317@intel.com>

On Tue, Sep 29, 2020 at 09:39:36AM +0000, Rojewski, Cezary wrote:
> On 2020-09-29 10:46 AM, Andy Shevchenko wrote:
> > On Mon, Sep 28, 2020 at 04:52:41PM +0000, Rojewski, Cezary wrote:
> >> On 2020-09-28 3:44 PM, Andy Shevchenko wrote:
> >>> On Sat, Sep 26, 2020 at 10:58:58AM +0200, Cezary Rojewski wrote:

...

> >>>> +	ret = ipc->rx.rsp.status;
> >>>> +	if (reply) {
> >>>> +		reply->header = ipc->rx.header;
> >>>> +		if (!ret && reply->data)
> >>>> +			memcpy(reply->data, ipc->rx.data, reply->size);
> >>>> +	}
> >>>> +
> >>>> +	return ret;
> >>>
> >>> One more looking into this... What about
> >>>
> >>> 	if (reply)
> >>> 		reply->header = ipc->rx.header;
> >>>
> >>> 	ret = ipc->rx.rsp.status; // or even directly if (status) return status.
> >>> 	if (ret)
> >>> 		return ret;
> >>>
> >>> 	if (reply->data)
> >>> 		memcpy(reply->data, ipc->rx.data, reply->size);
> >>>
> >>> 	return 0;
> >>>
> >>> It may be verbose but I think readability is better here.
> >>
> >> In your example, last 'if' will cause exception if reply==NULL.
> > 
> > Yeah, should be reply && reply->data.
> > 
> >> Got your point, although that's just few lines which already involve
> >> 'if' with { } spacing. After removing size-checks you suggested this
> >> code is quite thin already.
> > 
> > Yes, sometimes too thin is not good in terms of readability.
> > 
> 
> This will cost us additional check (2x reply==NULL instead of 1x). Maybe
> a newline between:
> 
> 	reply->header = ipc->rx.header;
> 
> 	if (!ret && reply->data)
> 		memcpy(reply->data, ipc->rx.data, reply->size);
> 
> suffices?

I don't like the !ret piece, TBH.
But I wouldn't object too much if you leave it as is. And double check for
reply at least makes it cleaner in my opinion than compressing in few lines.

...

> If there are no other concerns, either a quick spinoff (v10) or delay
> those readability improvements till series with resource_union() re-use?

I guess we may do v10 w/o waiting. Let me look at the last two patches I
haven't commented yet.

> While catpt is a big upgrade when compared to existing /haswell/
> (obviously) there are more fruits of this rewrite: house cleaning -
> follow-up series targeting /haswell/, /baytrail/ and /common/ of
> sound/soc/intel. Guess everyone would like to see those in 5.10.

Yes!

-- 
With Best Regards,
Andy Shevchenko