Re: [PATCH] ASoC: soc-pcm: Shrink stack frame for __soc_pcm_hw_params

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 8/24/23 04:33, Charles Keepax wrote:
> On Wed, Aug 23, 2023 at 04:39:35PM +0000, Charles Keepax wrote:
>> On Wed, Aug 23, 2023 at 05:19:31PM +0100, Mark Brown wrote:
>>> On Wed, Aug 23, 2023 at 03:49:58PM +0000, Charles Keepax wrote:
>>>> On Wed, Aug 23, 2023 at 10:21:13AM +0100, Charles Keepax wrote:
>>>>> Commit ac950278b087 ("ASoC: add N cpus to M codecs dai link support")
>>>>> added an additional local params in __soc_pmc_hw_params, for the
>>>>> CPU side of the DAI. The snd_pcm_hw_params struct is pretty large (604
>>>>> bytes) and keeping two local copies of it makes the stack frame for
>>>>> __soc_pcm_hw_params really large. As the two copies are only used
>>>>> sequentially combine these into a single local variable to shrink the
>>>>> stack frame.
>>>
>>>> Hmm... this might need a little more thought its not clear why this
>>>> should change the frame size and it only seems to change the frame
>>>> size on the ARM cross compiler I am using, not x86.
>>>
>>> Isn't that just going to be a function of the compiler being smart
>>> enough to work out that there aren't overlapping uses of the two
>>> variables and they can share stack space?  There's no reason not to help
>>> it figure that out.
>>
>> Yeah I think my only concern here was I no longer was certain I
>> understood what was happening. I don't think the patch can do any
>> harm, well except for the names being slightly less clear in the
>> code. It is starting to look like the mostly comes down to the
>> compiler being smart enough, although both were GCC in my case
>> so the difference is still a little surprising to me.
>>
> 
> Ah ok I see what is going on here, it depends on if you have
> -Os or -O2 set. -O2 will merge the two variables and give a
> smaller stack frame, -Os does not.
> 
> I would be inclined to say merge the patch, since it does help
> if some is trying to size optimise their kernel, but I don't
> feel strongly. Also I could respin to put this in the commit
> message if people prefer?

v2 with an updated commit message sounds good to me.