linux-kernel - Re: [PATCH] sound: sof: ioc4-topology: avoid extra dai

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <d988fe89-104e-46ce-94b7-6754f2c7a455@app.fastmail.com>
Date: Wed, 07 Aug 2024 17:18:35 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Mark Brown" <broonie@...nel.org>, "Arnd Bergmann" <arnd@...nel.org>
Cc: "Pierre-Louis Bossart" <pierre-louis.bossart@...ux.intel.com>,
 "Liam Girdwood" <lgirdwood@...il.com>,
 "Peter Ujfalusi" <peter.ujfalusi@...ux.intel.com>,
 "Bard Liao" <yung-chuan.liao@...ux.intel.com>,
 "Ranjani Sridharan" <ranjani.sridharan@...ux.intel.com>,
 "Daniel Baluta" <daniel.baluta@....com>,
 "Seppo Ingalsuo" <seppo.ingalsuo@...ux.intel.com>,
 "Kai Vehmanen" <kai.vehmanen@...ux.intel.com>,
 "Jaroslav Kysela" <perex@...ex.cz>, "Takashi Iwai" <tiwai@...e.com>,
 "Nathan Chancellor" <nathan@...nel.org>,
 "Nick Desaulniers" <ndesaulniers@...gle.com>,
 "Bill Wendling" <morbo@...gle.com>, "Justin Stitt" <justinstitt@...gle.com>,
 "Brent Lu" <brent.lu@...el.com>, sound-open-firmware@...a-project.org,
 linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
 llvm@...ts.linux.dev
Subject: Re: [PATCH] sound: sof: ioc4-topology: avoid extra dai_params copy

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