On Tue, Jan 07, 2025 at 04:51:29PM +0800, Ma Ke wrote:
> The reference count of the device incremented in device_initialize()
> is not decremented properly when device_add() fails. Change
> device_unregister() to a put_device() call before returning from the
> function to decrement reference count for cleanup. Or it could cause
> memory leak.
>
> As comment of device_add() says, 'if device_add() succeeds, you should
> call device_del() when you want to get rid of it. If device_add() has
> not succeeded, use only put_device() to drop the reference count'.
>
> Found by code review.
While the patch looks correct, the commit message seems confusing to me.
device_unregister() will also call put_device() after first calling
device_del(). So the reference count /is/ properly balanced.
Instead, the kerneldoc comment for device_add() says this:
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up your
* reference instead.
*
* Rule of thumb is: if device_add() succeeds, you should call
* device_del() when you want to get rid of it. If device_add() has
* *not* succeeded, use *only* put_device() to drop the reference
* count.
So the real reason that this patch is correct is because
device_unregister() should only be called after device_add() succeeded
because device_del() undoes what device_add() does if successful. In
this case we only need to undo what device_initialize() does, and that
is what put_device() will do.
Thierry
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 53d2a715c240 ("phy: Add Tegra XUSB pad controller support")
> Signed-off-by: Ma Ke <make24@xxxxxxxxxxx>
> ---
> drivers/phy/tegra/xusb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 79d4814d758d..c89df95aa6ca 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -548,16 +548,16 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>
> err = dev_set_name(&port->dev, "%s-%u", name, index);
> if (err < 0)
> - goto unregister;
> + goto put_device;
>
> err = device_add(&port->dev);
> if (err < 0)
> - goto unregister;
> + goto put_device;
>
> return 0;
>
> -unregister:
> - device_unregister(&port->dev);
> +put_device:
> + put_device(&port->dev);
> return err;
> }
>
> --
> 2.25.1
>
Attachment:
signature.asc
Description: PGP signature
