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

[Pierre-Louis Bossart]

On 8/7/19 3:09 PM, Sridharan, Ranjani wrote:
> 
> 
> On Wed, Aug 7, 2019 at 12:32 PM Cezary Rojewski 
> <cezary.rojewski at intel.com <mailto:cezary.rojewski at intel.com>> wrote:
> 
>     On 2019-08-07 16:52, Pierre-Louis Bossart wrote:
>      > From: Jaska Uimonen <jaska.uimonen at intel.com
>     <mailto:jaska.uimonen at 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.

I took a look at this and there's really only 2/3 places where we could 
use a goto, but we'd have to use 2 labels depending on whether we free 
process/wdata so not sure if we'd make the code more self-explanatory in 
the end.

Jaska, can you take a look?