Re: [PATCH] phy: Fix error handling in tegra_xusb_port_init

[Thierry Reding]

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