| Message ID | 20241217033243.3144184-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt() | expand |
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
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
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
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
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); } /**
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(-)