Re: [alsa-devel] [PATCH] ASoC: SOF: topology: use set_get_data in process load

On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski cezary.rojewski@intel.com wrote:

On 2019-08-07 16:52, Pierre-Louis Bossart wrote:

...

From: Jaska Uimonen jaska.uimonen@intel.com

...

  process = kzalloc(ipc_size, GFP_KERNEL);

• if (!process)
  

• if (!process) {
  

•         kfree(wdata);
          return -ENOMEM;
  

• }
  
  /* configure iir IPC message */
  process->comp.hdr.size = ipc_size;
  

@@ -1835,7 +1890,9 @@ static int sof_process_load(struct

snd_soc_component *scomp, int index,

...

  if (ret != 0) {
          dev_err(sdev->dev, "error: parse process.cfg tokens failed

%d\n",

...

                  le32_to_cpu(private->size));

•         goto err;
  

•         kfree(wdata);
  

•         kfree(process);
  

•         return ret;
  }
  
  

...

@@ -1886,10 +1916,36 @@ static int sof_process_load(struct

snd_soc_component *scomp, int index,

...

  ret = sof_ipc_tx_message(sdev->ipc, process->comp.hdr.cmd, process,
                           ipc_size, r, sizeof(*r));

• if (ret >= 0)
  

• if (ret < 0) {
  

•         dev_err(sdev->dev, "error: create process failed\n");
  

•         kfree(wdata);
  

•         kfree(process);
          return ret;
  

-err:

• kfree(process);
  

• }
  

• /* we sent the data in single message so return */
  

• if (ipc_data_size) {
  

•         kfree(wdata);
  

•         return ret;
  

• }
  

• /* send control data with large message supported method */
  

• for (i = 0; i < widget->num_kcontrols; i++) {
  

•         wdata[i].control->readback_offset = 0;
  

•         ret = snd_sof_ipc_set_get_comp_data(sdev->ipc,
  

wdata[i].control,

...

•                                             wdata[i].ipc_cmd,
  

•                                             wdata[i].ctrl_type,
  

•                                             wdata[i].control->cmd,
  

•                                             true);
  

•         if (ret != 0) {
  

•                 dev_err(sdev->dev, "error: send control failed\n");
  

•                 kfree(process);
  

•                 break;
  

•         }
  

• }
  

• kfree(wdata);
  return ret;
  

  }

On several occasions you've added individual error paths instead of a unified one. Personally I don't find it easier to read and understand function's flow at all.

<ifs with goto err>

err: kfree(process); kfree(wdata); return ret;

doesn't look that bad..

Thanks for pointing out. Perhaps, the error handling can be improved a little. We can fix in v2.

Thanks, Ranjani

---

Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel