* [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()
@ 2024-12-17 3:32 Joe Hattori
2024-12-17 10:58 ` Krzysztof Kozlowski
2024-12-17 11:27 ` Dan Carpenter
0 siblings, 2 replies; 5+ messages in thread
From: Joe Hattori @ 2024-12-17 3:32 UTC (permalink / raw)
To: alexandre.torgue, joabreu, andrew+netdev, davem, edumazet, kuba,
pabeni, mcoquelin.stm32
Cc: netdev, Joe Hattori
Current implementation of stmmac_probe_config_dt() does not release the
OF node reference obtained by of_parse_phandle() when stmmac_dt_phy()
fails, thus call stmmac_remove_config_dt(). The error_hw_init and
error_pclk_get labels can be removed as just calling
stmmac_remove_config_dt() suffices. Also, remove the first argument of
stmmac_remove_config_dt() as it is not used.
This bug was found by an experimental static analysis tool that I am
developing.
Fixes: 4838a5405028 ("net: stmmac: Fix wrapper drivers not detecting PHY")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
.../ethernet/stmicro/stmmac/stmmac_platform.c | 37 +++++++------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 3ac32444e492..4f1a9f7aae6e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -407,13 +407,11 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
/**
* stmmac_remove_config_dt - undo the effects of stmmac_probe_config_dt()
- * @pdev: platform_device structure
* @plat: driver data platform structure
*
* Release resources claimed by stmmac_probe_config_dt().
*/
-static void stmmac_remove_config_dt(struct platform_device *pdev,
- struct plat_stmmacenet_data *plat)
+static void stmmac_remove_config_dt(struct plat_stmmacenet_data *plat)
{
clk_disable_unprepare(plat->stmmac_clk);
clk_disable_unprepare(plat->pclk);
@@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
struct plat_stmmacenet_data *plat;
struct stmmac_dma_cfg *dma_cfg;
int phy_mode;
- void *ret;
int rc;
plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
@@ -490,8 +487,10 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
rc = stmmac_mdio_setup(plat, np, &pdev->dev);
- if (rc)
+ if (rc) {
+ stmmac_remove_config_dt(plat);
return ERR_PTR(rc);
+ }
of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size);
@@ -581,7 +580,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
GFP_KERNEL);
if (!dma_cfg) {
- stmmac_remove_config_dt(pdev, plat);
+ stmmac_remove_config_dt(plat);
return ERR_PTR(-ENOMEM);
}
plat->dma_cfg = dma_cfg;
@@ -610,7 +609,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
rc = stmmac_mtl_setup(pdev, plat);
if (rc) {
- stmmac_remove_config_dt(pdev, plat);
+ stmmac_remove_config_dt(plat);
return ERR_PTR(rc);
}
@@ -627,8 +626,8 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
plat->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
if (IS_ERR(plat->pclk)) {
- ret = plat->pclk;
- goto error_pclk_get;
+ stmmac_remove_config_dt(plat);
+ return ERR_CAST(plat->pclk);
}
clk_prepare_enable(plat->pclk);
@@ -646,33 +645,23 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
STMMAC_RESOURCE_NAME);
if (IS_ERR(plat->stmmac_rst)) {
- ret = plat->stmmac_rst;
- goto error_hw_init;
+ stmmac_remove_config_dt(plat);
+ return ERR_CAST(plat->stmmac_rst);
}
plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
&pdev->dev, "ahb");
if (IS_ERR(plat->stmmac_ahb_rst)) {
- ret = plat->stmmac_ahb_rst;
- goto error_hw_init;
+ stmmac_remove_config_dt(plat);
+ return ERR_CAST(plat->stmmac_ahb_rst);
}
return plat;
-
-error_hw_init:
- clk_disable_unprepare(plat->pclk);
-error_pclk_get:
- clk_disable_unprepare(plat->stmmac_clk);
-
- return ret;
}
static void devm_stmmac_remove_config_dt(void *data)
{
- struct plat_stmmacenet_data *plat = data;
-
- /* Platform data argument is unused */
- stmmac_remove_config_dt(NULL, plat);
+ stmmac_remove_config_dt(data);
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()
2024-12-17 3:32 [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt() Joe Hattori
@ 2024-12-17 10:58 ` Krzysztof Kozlowski
2024-12-18 3:24 ` Joe Hattori
2024-12-17 11:27 ` Dan Carpenter
1 sibling, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-17 10:58 UTC (permalink / raw)
To: Joe Hattori, alexandre.torgue, joabreu, andrew+netdev, davem,
edumazet, kuba, pabeni, mcoquelin.stm32
Cc: netdev
On 17/12/2024 04:32, Joe Hattori wrote:
> Current implementation of stmmac_probe_config_dt() does not release the
> OF node reference obtained by of_parse_phandle() when stmmac_dt_phy()
> fails, thus call stmmac_remove_config_dt(). The error_hw_init and
> error_pclk_get labels can be removed as just calling
> stmmac_remove_config_dt() suffices. Also, remove the first argument of
> stmmac_remove_config_dt() as it is not used.
>
> This bug was found by an experimental static analysis tool that I am
> developing.
>
> Fixes: 4838a5405028 ("net: stmmac: Fix wrapper drivers not detecting PHY")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> ---
> .../ethernet/stmicro/stmmac/stmmac_platform.c | 37 +++++++------------
> 1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 3ac32444e492..4f1a9f7aae6e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -407,13 +407,11 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
>
> /**
> * stmmac_remove_config_dt - undo the effects of stmmac_probe_config_dt()
> - * @pdev: platform_device structure
> * @plat: driver data platform structure
> *
> * Release resources claimed by stmmac_probe_config_dt().
> */
> -static void stmmac_remove_config_dt(struct platform_device *pdev,
> - struct plat_stmmacenet_data *plat)
> +static void stmmac_remove_config_dt(struct plat_stmmacenet_data *plat)
> {
> clk_disable_unprepare(plat->stmmac_clk);
> clk_disable_unprepare(plat->pclk);
> @@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
> struct plat_stmmacenet_data *plat;
> struct stmmac_dma_cfg *dma_cfg;
> int phy_mode;
> - void *ret;
> int rc;
>
> plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> @@ -490,8 +487,10 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
> dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
>
> rc = stmmac_mdio_setup(plat, np, &pdev->dev);
> - if (rc)
> + if (rc) {
> + stmmac_remove_config_dt(plat);
> return ERR_PTR(rc);
You now double unprepare the clocks. This is either buggy or just
confusing (assuming clocks are NULL).
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()
2024-12-17 3:32 [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt() Joe Hattori
2024-12-17 10:58 ` Krzysztof Kozlowski
@ 2024-12-17 11:27 ` Dan Carpenter
2024-12-18 3:27 ` Joe Hattori
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-12-17 11:27 UTC (permalink / raw)
To: Joe Hattori
Cc: alexandre.torgue, joabreu, andrew+netdev, davem, edumazet, kuba,
pabeni, mcoquelin.stm32, netdev
On Tue, Dec 17, 2024 at 12:32:43PM +0900, Joe Hattori wrote:
> Also, remove the first argument of stmmac_remove_config_dt() as it is not
> used.
Don't do that. It makes the patch difficult to review.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()
2024-12-17 10:58 ` Krzysztof Kozlowski
@ 2024-12-18 3:24 ` Joe Hattori
0 siblings, 0 replies; 5+ messages in thread
From: Joe Hattori @ 2024-12-18 3:24 UTC (permalink / raw)
To: Krzysztof Kozlowski, alexandre.torgue, joabreu, andrew+netdev,
davem, edumazet, kuba, pabeni, mcoquelin.stm32
Cc: netdev
On 12/17/24 19:58, Krzysztof Kozlowski wrote:
> On 17/12/2024 04:32, Joe Hattori wrote:
>> Current implementation of stmmac_probe_config_dt() does not release the
>> OF node reference obtained by of_parse_phandle() when stmmac_dt_phy()
>> fails, thus call stmmac_remove_config_dt(). The error_hw_init and
>> error_pclk_get labels can be removed as just calling
>> stmmac_remove_config_dt() suffices. Also, remove the first argument of
>> stmmac_remove_config_dt() as it is not used.
>>
>> This bug was found by an experimental static analysis tool that I am
>> developing.
>>
>> Fixes: 4838a5405028 ("net: stmmac: Fix wrapper drivers not detecting PHY")
>> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
>> ---
>> .../ethernet/stmicro/stmmac/stmmac_platform.c | 37 +++++++------------
>> 1 file changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 3ac32444e492..4f1a9f7aae6e 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -407,13 +407,11 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
>>
>> /**
>> * stmmac_remove_config_dt - undo the effects of stmmac_probe_config_dt()
>> - * @pdev: platform_device structure
>> * @plat: driver data platform structure
>> *
>> * Release resources claimed by stmmac_probe_config_dt().
>> */
>> -static void stmmac_remove_config_dt(struct platform_device *pdev,
>> - struct plat_stmmacenet_data *plat)
>> +static void stmmac_remove_config_dt(struct plat_stmmacenet_data *plat)
>> {
>> clk_disable_unprepare(plat->stmmac_clk);
>> clk_disable_unprepare(plat->pclk);
>> @@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>> struct plat_stmmacenet_data *plat;
>> struct stmmac_dma_cfg *dma_cfg;
>> int phy_mode;
>> - void *ret;
>> int rc;
>>
>> plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>> @@ -490,8 +487,10 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>> dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
>>
>> rc = stmmac_mdio_setup(plat, np, &pdev->dev);
>> - if (rc)
>> + if (rc) {
>> + stmmac_remove_config_dt(plat);
>> return ERR_PTR(rc);
> You now double unprepare the clocks. This is either buggy or just
> confusing (assuming clocks are NULL).
Confusing indeed. Replaced with of_node_put() in the v2 patch.
>
> Best regards,
> Krzysztof
Best,
Joe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()
2024-12-17 11:27 ` Dan Carpenter
@ 2024-12-18 3:27 ` Joe Hattori
0 siblings, 0 replies; 5+ messages in thread
From: Joe Hattori @ 2024-12-18 3:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: alexandre.torgue, joabreu, andrew+netdev, davem, edumazet, kuba,
pabeni, mcoquelin.stm32, netdev
On 12/17/24 20:27, Dan Carpenter wrote:
> On Tue, Dec 17, 2024 at 12:32:43PM +0900, Joe Hattori wrote:
>> Also, remove the first argument of stmmac_remove_config_dt() as it is not
>> used.
>
> Don't do that. It makes the patch difficult to review.
True, the patch is now split into two in v2.
>
> regards,
> dan carpenter
>
Best,
Joe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-18 3:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-17 3:32 [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt() Joe Hattori
2024-12-17 10:58 ` Krzysztof Kozlowski
2024-12-18 3:24 ` Joe Hattori
2024-12-17 11:27 ` Dan Carpenter
2024-12-18 3:27 ` Joe Hattori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).