On Fri, Dec 17, 2021 at 04:13:48PM +0100, Cezary Rojewski wrote:
> On 2021-12-17 4:00 PM, Dan Carpenter wrote:
> > This code frees "graph" and then dereferences to save the error code.
> > Save the error code first and then use gotos to unwind the allocation.
> >
> > Fixes: 59716aa3f976 ("ASoC: qdsp6: Fix an IS_ERR() vs NULL bug")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > sound/soc/qcom/qdsp6/q6apm.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> > index 3e007d609a9b..f424d7aa389a 100644
> > --- a/sound/soc/qcom/qdsp6/q6apm.c
> > +++ b/sound/soc/qcom/qdsp6/q6apm.c
> > @@ -615,7 +615,7 @@ struct q6apm_graph *q6apm_graph_open(struct device *dev, q6apm_cb cb,
> > graph = kzalloc(sizeof(*graph), GFP_KERNEL);
> > if (!graph) {
> > ret = -ENOMEM;
> > - goto err;
> > + goto put_ar_graph;
> > }
> > graph->apm = apm;
> > @@ -631,13 +631,15 @@ struct q6apm_graph *q6apm_graph_open(struct device *dev, q6apm_cb cb,
> > graph->port = gpr_alloc_port(apm->gdev, dev, graph_callback, graph);
> > if (IS_ERR(graph->port)) {
> > - kfree(graph);
> > ret = PTR_ERR(graph->port);
> > - goto err;
> > + goto free_graph;
> > }
> > return graph;
> > -err:
> > +
> > +free_graph:
> > + kfree(graph);
> > +put_ar_graph:
>
> Hello Dan,
>
> The patch looks good! My only suggestion is a readability improvement, but
> I'm unaware of the convention chosen for qcom directory so you may choose to
> ignore it:
>
> Function q6apm_graph_open() has two separate return paths: a happy path
> ending in 'return graph' and an error path which eventually ends with
> 'return ERR_PTR(ret)'. Current goto label-naming convention suggests it's a
> happy path nonetheless.
>
> s/free_graph/err_alloc_port/ and s/put_ar_graph/err_alloc_graph/ tells
> reader upfront that they are in the error path.
>
Generally when code is indented two tabs that's an error path. The
relevant pattern is "Do error handling, not success handling". I guess
the if (IS_ERR()) check means it's an error as well.
regards,
dan carpenter
