[PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Markus Elfring]

Date: Fri, 17 Mar 2023 20:02:34 +0100

The label “err_free” was used to jump to another pointer check despite of
the detail in the implementation of the function “sama7g5_pmc_setup”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed memory allocation).

* Thus use additional labels.

* Delete an extra pointer check at the end which became unnecessary
  with this refactoring.

This issue was detected by using the Coccinelle software.

Fixes: cb783bbbcf54c36256006895c215e86c5e7266d8 ("clk: at91: sama7g5: add clock support for sama7g5")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/at91/sama7g5.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
index f135b662f1ff..224b1f2ebef2 100644
--- a/drivers/clk/at91/sama7g5.c
+++ b/drivers/clk/at91/sama7g5.c
@@ -927,25 +927,25 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
 			    (ARRAY_SIZE(sama7g5_mckx) + ARRAY_SIZE(sama7g5_gck)),
 			    GFP_KERNEL);
 	if (!alloc_mem)
-		goto err_free;
+		goto free_pmc;

 	hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
 					   50000000);
 	if (IS_ERR(hw))
-		goto err_free;
+		goto free_alloc_mem;

 	bypass = of_property_read_bool(np, "atmel,osc-bypass");

 	hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name,
 					bypass);
 	if (IS_ERR(hw))
-		goto err_free;
+		goto free_alloc_mem;

 	parent_names[0] = "main_rc_osc";
 	parent_names[1] = "main_osc";
 	hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, 2);
 	if (IS_ERR(hw))
-		goto err_free;
+		goto free_alloc_mem;

 	sama7g5_pmc->chws[PMC_MAIN] = hw;

@@ -987,7 +987,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
 			}

 			if (IS_ERR(hw))
-				goto err_free;
+				goto free_alloc_mem;

 			if (sama7g5_plls[i][j].eid)
 				sama7g5_pmc->chws[sama7g5_plls[i][j].eid] = hw;
@@ -999,7 +999,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
 					  &mck0_layout, &mck0_characteristics,
 					  &pmc_mck0_lock, CLK_GET_RATE_NOCACHE, 5);
 	if (IS_ERR(hw))
-		goto err_free;
+		goto free_alloc_mem;

 	sama7g5_pmc->chws[PMC_MCK] = hw;

@@ -1128,12 +1128,11 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
 	return;

 err_free:
-	if (alloc_mem) {
-		for (i = 0; i < alloc_mem_size; i++)
-			kfree(alloc_mem[i]);
-		kfree(alloc_mem);
-	}
-
+	for (i = 0; i < alloc_mem_size; i++)
+		kfree(alloc_mem[i]);
+free_alloc_mem:
+	kfree(alloc_mem);
+free_pmc:
 	kfree(sama7g5_pmc);
 }

--
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Nicolas Ferre]

On 25/03/2023 at 15:05, Markus Elfring wrote:
> Date: Fri, 17 Mar 2023 20:02:34 +0100
> 
> The label “err_free” was used to jump to another pointer check despite of
> the detail in the implementation of the function “sama7g5_pmc_setup”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed memory allocation).
> 
> * Thus use additional labels.
> 
> * Delete an extra pointer check at the end which became unnecessary
>    with this refactoring.
> 
> This issue was detected by using the Coccinelle software.

Fine, but I'm sorry that it complexity the function for no real value. 
Other clk drivers have the same pattern so I want them to all stay the same.

This is a NACK, sorry about that.

Regards,
   Nicolas

> Fixes: cb783bbbcf54c36256006895c215e86c5e7266d8 ("clk: at91: sama7g5: add clock support for sama7g5")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/clk/at91/sama7g5.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index f135b662f1ff..224b1f2ebef2 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c
> @@ -927,25 +927,25 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                              (ARRAY_SIZE(sama7g5_mckx) + ARRAY_SIZE(sama7g5_gck)),
>                              GFP_KERNEL);
>          if (!alloc_mem)
> -               goto err_free;
> +               goto free_pmc;
> 
>          hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
>                                             50000000);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          bypass = of_property_read_bool(np, "atmel,osc-bypass");
> 
>          hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name,
>                                          bypass);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          parent_names[0] = "main_rc_osc";
>          parent_names[1] = "main_osc";
>          hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, 2);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MAIN] = hw;
> 
> @@ -987,7 +987,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                          }
> 
>                          if (IS_ERR(hw))
> -                               goto err_free;
> +                               goto free_alloc_mem;
> 
>                          if (sama7g5_plls[i][j].eid)
>                                  sama7g5_pmc->chws[sama7g5_plls[i][j].eid] = hw;
> @@ -999,7 +999,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                                            &mck0_layout, &mck0_characteristics,
>                                            &pmc_mck0_lock, CLK_GET_RATE_NOCACHE, 5);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MCK] = hw;
> 
> @@ -1128,12 +1128,11 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>          return;
> 
>   err_free:
> -       if (alloc_mem) {
> -               for (i = 0; i < alloc_mem_size; i++)
> -                       kfree(alloc_mem[i]);
> -               kfree(alloc_mem);
> -       }
> -
> +       for (i = 0; i < alloc_mem_size; i++)
> +               kfree(alloc_mem[i]);
> +free_alloc_mem:
> +       kfree(alloc_mem);
> +free_pmc:
>          kfree(sama7g5_pmc);
>   }
> 
> --
> 2.40.0
> 

-- 
Nicolas Ferre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Markus Elfring]

>> The label “err_free” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “sama7g5_pmc_setup”
>> that it was determined already that the corresponding variable contained
>> a null pointer (because of a failed memory allocation).
>>
>> * Thus use additional labels.
>>
>> * Delete an extra pointer check at the end which became unnecessary
>>    with this refactoring.
>>
>> This issue was detected by using the Coccinelle software.
>
> Fine, but I'm sorry that it complexity the function for no real value.

Under which circumstances can advice be taken better into account
also from another information source?
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29



> Other clk drivers have the same pattern so I want them to all stay the same.
> This is a NACK, sorry about that.

I am curious if other contributors (or code reviewers) would like to influence
the software situation a bit more.

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Alexandre Belloni]

On 28/03/2023 21:24:00+0200, Markus Elfring wrote:
> >> The label “err_free” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “sama7g5_pmc_setup”
> >> that it was determined already that the corresponding variable contained
> >> a null pointer (because of a failed memory allocation).
> >>
> >> * Thus use additional labels.
> >>
> >> * Delete an extra pointer check at the end which became unnecessary
> >>    with this refactoring.
> >>
> >> This issue was detected by using the Coccinelle software.
> >
> > Fine, but I'm sorry that it complexity the function for no real value.
> 
> Under which circumstances can advice be taken better into account
> also from another information source?
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29
> 
> 
> 
> > Other clk drivers have the same pattern so I want them to all stay the same.
> > This is a NACK, sorry about that.
> 
> I am curious if other contributors (or code reviewers) would like to influence
> the software situation a bit more.
> 

I agree with Nicolas here, this is useless churn.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Markus Elfring]

Date: Wed, 5 Apr 2023 22:00:18 +0200

The label “out” was used to jump to another pointer check despite of
the detail in the implementation of the function “ti_sci_probe”
that it was determined already that the corresponding variable
contained an error pointer because of a failed call of
the function “mbox_request_channel_byname”.

* Thus use more appropriate labels instead.

* Delete two redundant checks.


This issue was detected by using the Coccinelle software.

Fixes: aa276781a64a5f15ecc21e920960c5b1f84e5fee ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/firmware/ti_sci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 039d92a595ec..77012d2f4160 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -3433,18 +3433,18 @@ static int ti_sci_probe(struct platform_device *pdev)
 	info->chan_rx = mbox_request_channel_byname(cl, "rx");
 	if (IS_ERR(info->chan_rx)) {
 		ret = PTR_ERR(info->chan_rx);
-		goto out;
+		goto remove_debugfs;
 	}

 	info->chan_tx = mbox_request_channel_byname(cl, "tx");
 	if (IS_ERR(info->chan_tx)) {
 		ret = PTR_ERR(info->chan_tx);
-		goto out;
+		goto free_mbox_channel_rx;
 	}
 	ret = ti_sci_cmd_get_revision(info);
 	if (ret) {
 		dev_err(dev, "Unable to communicate with TISCI(%d)\n", ret);
-		goto out;
+		goto free_mbox_channel_tx;
 	}

 	ti_sci_setup_ops(info);
@@ -3456,7 +3456,7 @@ static int ti_sci_probe(struct platform_device *pdev)
 		ret = register_restart_handler(&info->nb);
 		if (ret) {
 			dev_err(dev, "reboot registration fail(%d)\n", ret);
-			goto out;
+			goto free_mbox_channel_tx;
 		}
 	}

@@ -3470,11 +3470,12 @@ static int ti_sci_probe(struct platform_device *pdev)
 	mutex_unlock(&ti_sci_list_mutex);

 	return of_platform_populate(dev->of_node, NULL, NULL, dev);
-out:
-	if (!IS_ERR(info->chan_tx))
-		mbox_free_channel(info->chan_tx);
-	if (!IS_ERR(info->chan_rx))
-		mbox_free_channel(info->chan_rx);
+
+free_mbox_channel_tx:
+	mbox_free_channel(info->chan_tx);
+free_mbox_channel_rx:
+	mbox_free_channel(info->chan_rx);
+remove_debugfs:
 	debugfs_remove(info->d);
 	return ret;
 }
--
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] remoteproc: imx_dsp_rproc: Improve exception handling in imx_dsp_rproc_mbox_alloc()

[Markus Elfring]

Date: Thu, 6 Apr 2023 22:00:24 +0200

The label “err_out” was used to jump to another pointer check
despite of the detail in the implementation of the function
“imx_dsp_rproc_mbox_alloc” that it was determined already
that the corresponding variable contained an error pointer
because of a failed call of the function “mbox_request_channel_byname”.

Thus perform the following adjustments:

1. Return directly after a call of the function
   “mbox_request_channel_byname” failed for the input parameter “tx”.

2. Use more appropriate labels instead.

3. Reorder jump targets at the end.

4. Omit a function call and three extra checks.


This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/remoteproc/imx_dsp_rproc.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 21759d9e5b7b..a8ad15ef1da0 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -530,7 +530,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
 		ret = PTR_ERR(priv->tx_ch);
 		dev_dbg(cl->dev, "failed to request tx mailbox channel: %d\n",
 			ret);
-		goto err_out;
+		return ret;
 	}

 	/* Channel for receiving message */
@@ -539,7 +539,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
 		ret = PTR_ERR(priv->rx_ch);
 		dev_dbg(cl->dev, "failed to request rx mailbox channel: %d\n",
 			ret);
-		goto err_out;
+		goto free_channel_tx;
 	}

 	cl = &priv->cl_rxdb;
@@ -555,19 +555,15 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
 		ret = PTR_ERR(priv->rxdb_ch);
 		dev_dbg(cl->dev, "failed to request mbox chan rxdb, ret %d\n",
 			ret);
-		goto err_out;
+		goto free_channel_rx;
 	}

 	return 0;

-err_out:
-	if (!IS_ERR(priv->tx_ch))
-		mbox_free_channel(priv->tx_ch);
-	if (!IS_ERR(priv->rx_ch))
-		mbox_free_channel(priv->rx_ch);
-	if (!IS_ERR(priv->rxdb_ch))
-		mbox_free_channel(priv->rxdb_ch);
-
+free_channel_rx:
+	mbox_free_channel(priv->rx_ch);
+free_channel_tx:
+	mbox_free_channel(priv->tx_ch);
 	return ret;
 }

--
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] spi: atmel: Improve exception handling in atmel_spi_configure_dma()

[Markus Elfring]

Date: Fri, 7 Apr 2023 08:08:59 +0200

The label “error” was used to jump to another pointer check despite of
the detail in the implementation of the function “atmel_spi_configure_dma”
that it was determined already that the corresponding variable
contained an error pointer because of a failed call of
the function “dma_request_chan”.

* Thus use more appropriate labels instead.

* Delete two redundant checks.


This issue was detected by using the Coccinelle software.

Fixes: 398b6b310ec85eef9d98df5963d5ded18aa92ad8 ("spi: atmel: switch to use modern name")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/spi/spi-atmel.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 7f06305e16cb..ed8dc93c73e5 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -511,12 +511,12 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
 		 * requested tx channel.
 		 */
 		dev_dbg(dev, "No RX DMA channel, DMA is disabled\n");
-		goto error;
+		goto release_channel_tx;
 	}

 	err = atmel_spi_dma_slave_config(as, 8);
 	if (err)
-		goto error;
+		goto release_channel_rx;

 	dev_info(&as->pdev->dev,
 			"Using %s (tx) and %s (rx) for DMA transfers\n",
@@ -524,11 +524,11 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
 			dma_chan_name(host->dma_rx));

 	return 0;
-error:
-	if (!IS_ERR(host->dma_rx))
-		dma_release_channel(host->dma_rx);
-	if (!IS_ERR(host->dma_tx))
-		dma_release_channel(host->dma_tx);
+
+release_channel_rx:
+	dma_release_channel(host->dma_rx);
+release_channel_tx:
+	dma_release_channel(host->dma_tx);
 error_clear:
 	host->dma_tx = host->dma_rx = NULL;
 	return err;
--
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Improve exception handling in atmel_spi_configure_dma()

[Nicolas Ferre]

On 07/04/2023 at 08:22, Markus Elfring wrote:
> Date: Fri, 7 Apr 2023 08:08:59 +0200
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “atmel_spi_configure_dma”
> that it was determined already that the corresponding variable
> contained an error pointer because of a failed call of
> the function “dma_request_chan”.
> 
> * Thus use more appropriate labels instead.
> 
> * Delete two redundant checks.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 398b6b310ec85eef9d98df5963d5ded18aa92ad8 ("spi: atmel: switch to use modern name")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

It's becoming a pattern, but still:
NACK.

Regards,
   Nicolas

> ---
>   drivers/spi/spi-atmel.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 7f06305e16cb..ed8dc93c73e5 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -511,12 +511,12 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
>                   * requested tx channel.
>                   */
>                  dev_dbg(dev, "No RX DMA channel, DMA is disabled\n");
> -               goto error;
> +               goto release_channel_tx;
>          }
> 
>          err = atmel_spi_dma_slave_config(as, 8);
>          if (err)
> -               goto error;
> +               goto release_channel_rx;
> 
>          dev_info(&as->pdev->dev,
>                          "Using %s (tx) and %s (rx) for DMA transfers\n",
> @@ -524,11 +524,11 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
>                          dma_chan_name(host->dma_rx));
> 
>          return 0;
> -error:
> -       if (!IS_ERR(host->dma_rx))
> -               dma_release_channel(host->dma_rx);
> -       if (!IS_ERR(host->dma_tx))
> -               dma_release_channel(host->dma_tx);
> +
> +release_channel_rx:
> +       dma_release_channel(host->dma_rx);
> +release_channel_tx:
> +       dma_release_channel(host->dma_tx);
>   error_clear:
>          host->dma_tx = host->dma_rx = NULL;
>          return err;
> --
> 2.40.0
> 

-- 
Nicolas Ferre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] spi: atmel: Improve exception handling in atmel_spi_configure_dma()

[Markus Elfring]

>> The label “error” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “atmel_spi_configure_dma”
>> that it was determined already that the corresponding variable
>> contained an error pointer because of a failed call of
>> the function “dma_request_chan”.
>>
>> * Thus use more appropriate labels instead.
>>
>> * Delete two redundant checks.
>>
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 398b6b310ec85eef9d98df5963d5ded18aa92ad8 ("spi: atmel: switch to use modern name")
…
> It's becoming a pattern, but still:
> NACK.

What does hinder you to work with more jump labels for improved exception handling?

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] remoteproc: imx_dsp_rproc: Improve exception handling in imx_dsp_rproc_mbox_alloc()

[Mathieu Poirier]

On Thu, Apr 06, 2023 at 10:12:50PM +0200, Markus Elfring wrote:
> Date: Thu, 6 Apr 2023 22:00:24 +0200
> 
> The label “err_out” was used to jump to another pointer check
> despite of the detail in the implementation of the function
> “imx_dsp_rproc_mbox_alloc” that it was determined already
> that the corresponding variable contained an error pointer
> because of a failed call of the function “mbox_request_channel_byname”.
> 
> Thus perform the following adjustments:
> 
> 1. Return directly after a call of the function
>    “mbox_request_channel_byname” failed for the input parameter “tx”.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Reorder jump targets at the end.
> 
> 4. Omit a function call and three extra checks.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>

Applied

Thanks,
Mathieu

> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 21759d9e5b7b..a8ad15ef1da0 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -530,7 +530,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->tx_ch);
>  		dev_dbg(cl->dev, "failed to request tx mailbox channel: %d\n",
>  			ret);
> -		goto err_out;
> +		return ret;
>  	}
> 
>  	/* Channel for receiving message */
> @@ -539,7 +539,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->rx_ch);
>  		dev_dbg(cl->dev, "failed to request rx mailbox channel: %d\n",
>  			ret);
> -		goto err_out;
> +		goto free_channel_tx;
>  	}
> 
>  	cl = &priv->cl_rxdb;
> @@ -555,19 +555,15 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->rxdb_ch);
>  		dev_dbg(cl->dev, "failed to request mbox chan rxdb, ret %d\n",
>  			ret);
> -		goto err_out;
> +		goto free_channel_rx;
>  	}
> 
>  	return 0;
> 
> -err_out:
> -	if (!IS_ERR(priv->tx_ch))
> -		mbox_free_channel(priv->tx_ch);
> -	if (!IS_ERR(priv->rx_ch))
> -		mbox_free_channel(priv->rx_ch);
> -	if (!IS_ERR(priv->rxdb_ch))
> -		mbox_free_channel(priv->rxdb_ch);
> -
> +free_channel_rx:
> +	mbox_free_channel(priv->rx_ch);
> +free_channel_tx:
> +	mbox_free_channel(priv->tx_ch);
>  	return ret;
>  }
> 
> --
> 2.40.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [cocci] [PATCH] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Nishanth Menon]

On 22:10-20230405, Markus Elfring wrote:
> Date: Wed, 5 Apr 2023 22:00:18 +0200

B4 does'nt pick this patch up cleanly. And for some reason, I get
mangled patch from public-inbox as well :( a clean git-send-email might
help.

> 
> The label “out” was used to jump to another pointer check despite of

Please use " for quotes.

> the detail in the implementation of the function “ti_sci_probe”
> that it was determined already that the corresponding variable
> contained an error pointer because of a failed call of
> the function “mbox_request_channel_byname”.

> 
> * Thus use more appropriate labels instead.
> 
> * Delete two redundant checks.
> 

How about this:

Optimize out the redundant pointer check in exit path of "out" using
appropriate labels to jump in the error path
> 
Drop the extra EoL

> This issue was detected by using the Coccinelle software.

Curious: what rule of coccicheck caught this?

> 
> Fixes: aa276781a64a5f15ecc21e920960c5b1f84e5fee ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")

12 char sha please. Please read Documentation/process/submitting-patches.rst

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/firmware/ti_sci.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 039d92a595ec..77012d2f4160 100644
> --- a/drivers/firmware/ti_sci.c
turns out as =2D-- instead of -- (might check the git format-patch
output closer).

> +++ b/drivers/firmware/ti_sci.c
> @@ -3433,18 +3433,18 @@ static int ti_sci_probe(struct platform_device *pdev)
>  	info->chan_rx = mbox_request_channel_byname(cl, "rx");
>  	if (IS_ERR(info->chan_rx)) {
>  		ret = PTR_ERR(info->chan_rx);
> -		goto out;
> +		goto remove_debugfs;
>  	}
> 
>  	info->chan_tx = mbox_request_channel_byname(cl, "tx");
>  	if (IS_ERR(info->chan_tx)) {
>  		ret = PTR_ERR(info->chan_tx);
> -		goto out;
> +		goto free_mbox_channel_rx;
>  	}
>  	ret = ti_sci_cmd_get_revision(info);
>  	if (ret) {
>  		dev_err(dev, "Unable to communicate with TISCI(%d)\n", ret);
> -		goto out;
> +		goto free_mbox_channel_tx;
>  	}
> 
>  	ti_sci_setup_ops(info);
> @@ -3456,7 +3456,7 @@ static int ti_sci_probe(struct platform_device *pdev)
>  		ret = register_restart_handler(&info->nb);
>  		if (ret) {
>  			dev_err(dev, "reboot registration fail(%d)\n", ret);
> -			goto out;
> +			goto free_mbox_channel_tx;
>  		}
>  	}
> 
> @@ -3470,11 +3470,12 @@ static int ti_sci_probe(struct platform_device *pdev)
>  	mutex_unlock(&ti_sci_list_mutex);
> 
>  	return of_platform_populate(dev->of_node, NULL, NULL, dev);
> -out:
> -	if (!IS_ERR(info->chan_tx))
> -		mbox_free_channel(info->chan_tx);
> -	if (!IS_ERR(info->chan_rx))
> -		mbox_free_channel(info->chan_rx);
> +
> +free_mbox_channel_tx:
> +	mbox_free_channel(info->chan_tx);
> +free_mbox_channel_rx:
> +	mbox_free_channel(info->chan_rx);
> +remove_debugfs:
>  	debugfs_remove(info->d);
>  	return ret;
>  }
> --
> 2.40.0
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [cocci] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Markus Elfring]

>> This issue was detected by using the Coccinelle software.
>
> Curious: what rule of coccicheck caught this?

None. (So far)

See also:
Reconsidering repeated checks for error pointers (with SmPL)
2023-04-04
https://lore.kernel.org/cocci/8f785de5-ebe2-edd9-2155-f440acacc643@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00008.html

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [cocci] [PATCH] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Dan Carpenter]

On Tue, May 16, 2023 at 10:20:43AM -0500, Nishanth Menon wrote:
> On 22:10-20230405, Markus Elfring wrote:
> > Date: Wed, 5 Apr 2023 22:00:18 +0200
> 
> B4 does'nt pick this patch up cleanly. And for some reason, I get
> mangled patch from public-inbox as well :( a clean git-send-email might
> help.
> 

It's an awkward thing.  B4 doesn't work because Markus was banned from
LKML because he doesn't listen to feedback.

> > 
> > The label “out” was used to jump to another pointer check despite of
> 
> Please use " for quotes.
> 
> > the detail in the implementation of the function “ti_sci_probe”
> > that it was determined already that the corresponding variable
> > contained an error pointer because of a failed call of
> > the function “mbox_request_channel_byname”.
> 
> > 
> > * Thus use more appropriate labels instead.
> > 
> > * Delete two redundant checks.
> > 
> 
> How about this:
> 
> Optimize out the redundant pointer check in exit path of "out" using
> appropriate labels to jump in the error path
> > 
> Drop the extra EoL
> 
> > This issue was detected by using the Coccinelle software.
> 
> Curious: what rule of coccicheck caught this?
> 
> > 
> > Fixes: aa276781a64a5f15ecc21e920960c5b1f84e5fee ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
> 
> 12 char sha please. Please read Documentation/process/submitting-patches.rst
> 

For example, Markus has been told to use 12 char shas several times
before.

> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/firmware/ti_sci.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> > index 039d92a595ec..77012d2f4160 100644
> > --- a/drivers/firmware/ti_sci.c
> turns out as =2D-- instead of -- (might check the git format-patch
> output closer).
> 
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -3433,18 +3433,18 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  	info->chan_rx = mbox_request_channel_byname(cl, "rx");
> >  	if (IS_ERR(info->chan_rx)) {
> >  		ret = PTR_ERR(info->chan_rx);
> > -		goto out;
> > +		goto remove_debugfs;
> >  	}
> > 
> >  	info->chan_tx = mbox_request_channel_byname(cl, "tx");
> >  	if (IS_ERR(info->chan_tx)) {
> >  		ret = PTR_ERR(info->chan_tx);
> > -		goto out;
> > +		goto free_mbox_channel_rx;
> >  	}
> >  	ret = ti_sci_cmd_get_revision(info);
> >  	if (ret) {
> >  		dev_err(dev, "Unable to communicate with TISCI(%d)\n", ret);
> > -		goto out;
> > +		goto free_mbox_channel_tx;
> >  	}
> > 
> >  	ti_sci_setup_ops(info);
> > @@ -3456,7 +3456,7 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  		ret = register_restart_handler(&info->nb);
> >  		if (ret) {
> >  			dev_err(dev, "reboot registration fail(%d)\n", ret);
> > -			goto out;
> > +			goto free_mbox_channel_tx;
> >  		}
> >  	}
> > 
> > @@ -3470,11 +3470,12 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  	mutex_unlock(&ti_sci_list_mutex);
> > 
> >  	return of_platform_populate(dev->of_node, NULL, NULL, dev);

There is a bug here because the resources are not freed if
of_platform_populate() fails.  The "info" struct is devm_ allocated but
it's still on the &ti_sci_list list, so that will lead to a use after
free.

regards,
dan carpenter

> > -out:
> > -	if (!IS_ERR(info->chan_tx))
> > -		mbox_free_channel(info->chan_tx);
> > -	if (!IS_ERR(info->chan_rx))
> > -		mbox_free_channel(info->chan_rx);
> > +
> > +free_mbox_channel_tx:
> > +	mbox_free_channel(info->chan_tx);
> > +free_mbox_channel_rx:
> > +	mbox_free_channel(info->chan_rx);
> > +remove_debugfs:
> >  	debugfs_remove(info->d);
> >  	return ret;
> >  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel