Re: [PATCH 3/3] net: ravb: Fix error handling in ravb_probe

[Marek Vasut]

On 3/5/25 9:39 PM, Paul Barker wrote:

Hi Marek,

Hi,

On 05/03/2025 19:55, Marek Vasut wrote:

On 3/4/25 9:07 PM, Paul Barker wrote:

In ravb_probe(), we were missing a couple of things in the error
handling path:

    * We must unregister the MDIO bus before freeing the corresponding
      struct mii_dev instance to avoid the potential for use-after-free
      bugs.

    * We must free the resources acquired by clk_get_bulk() even if the
      clocks have not yet been enabled.

Fixes: 8ae51b6f324e ("net: ravb: Add Renesas Ethernet RAVB driver")
Signed-off-by: Paul Barker <paul.barker...@bp.renesas.com>
---
   drivers/net/ravb.c | 14 ++++++++------
   1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 6bd94cf6bb1b..83cf6e0d19f4 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -591,7 +591,7 @@ static int ravb_probe(struct udevice *dev)

ret = clk_get_bulk(dev, &eth->clks);

        if (ret < 0)
-               goto err_mdio_alloc;
+               goto err_clk_get;

mdiodev = mdio_alloc();

        if (!mdiodev) {
@@ -613,23 +613,25 @@ static int ravb_probe(struct udevice *dev)
        /* Bring up PHY */
        ret = clk_enable_bulk(&eth->clks);
        if (ret)
-               goto err_mdio_register;
+               goto err_clk_enable;

ret = ravb_reset(dev);

        if (ret)
-               goto err_mdio_reset;
+               goto err_clk_enable;

Shouldn't this also assure clk_disable_bulk() is called ?

Looking at include/clk.h, the comment for clk_release_bulk() says:

     For each clock contained in the clock bulk struct, this function will check
     if clock has been previously requested and then will disable and free it.

So we shouldn't need to separately call clk_disable_bulk().

Understood. Thank you for checking.