On Wed, Aug 7, 2024, at 17:09, Mark Brown wrote:
On Wed, Aug 07, 2024 at 10:02:27AM +0200, Arnd Bergmann wrote:
From what I can tell, this was unintentional, as both sof_ipc4_prepare_dai_copier() and sof_ipc4_prepare_copier_module() make a copy for the same purpose, but copying it once has the exact same effect.
Remove the extra copy and change the direct struct assignment to an explicit memcpy() call to make it clearer to the reader that this is what happens. Note that gcc treats struct assignment as a memcpy() that may be inlined anyway, so the resulting object code is the same.
The effect of the copy is to ensure that if the function fails the argument is unmodified - did you do the analysis to check that it's OK to modify on error? Your commit log says "the same purpose" but never specifies what that purpose is.
There is always a chance that I misunderstood the code, but yes, I did understand that the idea is to not modify the parameters inside of sof_ipc4_prepare_dai_copier.
The only caller is in sof_ipc4_prepare_copier_module(), which achieves the exact same bit by doing the same:
/* * Use the fe_params as a base for the copier configuration. * The ref_params might get updated to reflect what format is * supported by the copier on the DAI side. * * In case of capture the ref_params returned will be used to * find the input configuration of the copier. */ memcpy(&ref_params, fe_params, sizeof(ref_params)); ret = sof_ipc4_prepare_dai_copier(sdev, dai, &ref_params, dir); if (ret < 0) return ret;
So when sof_ipc4_prepare_dai_copier() fails, the caller's local 'ref_params' structure is no longer used anywhere.
Arnd