Re: [PATCH V3 2/4] phy: tegra: xusb: Use devm_clk_get_optional to fetch USB2 tracking clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Double-free of the `usb2` pad structure when `tegra_xusb_pad_init()` fails.
- [High] Invalid `device_unregister()` call on `device_add()` failure path.
- [High] NULL pointer dereference and memory leak in `tegra_xusb_port_init()` error path.
--

commit cbc965ddabaa3e83cea8e6b268dbf34122daa3cd
Author: Sing-Han Chen <singhanc@nvidia.com>

phy: tegra: xusb: Use devm_clk_get_optional to fetch USB2 tracking clock
    
The USB2 PAD tracking clock is an optional clock so use
devm_clk_get_optional() so a missing clock in devicetree is
represented as NULL and probe can continue. clk_prepare_enable()
and clk_disable_unprepare() already treat a NULL clock as a no-op.

> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index 60156aea27071..800be0cbb3f9f 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -1045,7 +1045,7 @@ tegra186_usb2_pad_probe(struct tegra_xusb_padctl *padctl,
>  		goto out;
>  	}
>  
> -	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
> +	priv->usb2_trk_clk = devm_clk_get_optional(&pad->dev, "trk");
>  	if (IS_ERR(priv->usb2_trk_clk)) {

[Severity: High]
This isn't a bug introduced by this patch, but is there a double-free of the
usb2 pad structure if tegra_xusb_pad_init() fails?

Looking at tegra186_usb2_pad_probe() just above this change:

drivers/phy/tegra/xusb-tegra186.c:tegra186_usb2_pad_probe() {
	...
	err = tegra_xusb_pad_init(pad, padctl, np);
	if (err < 0) {
		kfree(usb2);
		goto out;
	}
	...
}

If device_add() fails inside tegra_xusb_pad_init(), it calls
device_unregister(), dropping the device reference count to zero and
triggering the release callback, which eventually calls
tegra186_usb2_pad_remove() and frees usb2. Does the explicit kfree(usb2)
here result in freeing the same pointer twice?

[Severity: High]
This is a pre-existing issue, but does tegra_xusb_pad_init() call
device_unregister() incorrectly on its failure path?

drivers/phy/tegra/xusb.c:tegra_xusb_pad_init() {
	...
	err = device_add(&pad->dev);
	if (err < 0)
		goto unregister;

	return 0;

unregister:
	device_unregister(&pad->dev);
	return err;
}

If device_add() fails, the device was never added to the system. Does
calling device_unregister() (which executes device_del() before
put_device()) attempt to tear down sysfs entries that were never created,
potentially leading to kernel warnings or crashes? Should it call
put_device() instead?

[Severity: High]
This is a pre-existing issue, but is there a NULL pointer dereference in the
error path for port initialization?

drivers/phy/tegra/xusb.c:tegra_xusb_add_usb2_port() {
	...
	err = tegra_xusb_port_init(&usb2->base, padctl, np, "usb2", index);
	if (err < 0)
		goto out;

	usb2->base.ops = padctl->soc->ports.usb2.ops;
	...
}

If tegra_xusb_port_init() fails (e.g., due to device_add() failing), it rolls
back by dropping the device reference, triggering the port release callback:

drivers/phy/tegra/xusb.c:tegra_xusb_port_release() {
	struct tegra_xusb_port *port = to_tegra_xusb_port(dev);

	if (port->ops->release)
		port->ops->release(port);
}

Since port->ops is only assigned after tegra_xusb_port_init() returns
successfully, it will be NULL during the failure rollback. Does the
unconditional dereference of port->ops in tegra_xusb_port_release() lead to
a kernel panic?

>  		err = PTR_ERR(priv->usb2_trk_clk);
>  		dev_dbg(&pad->dev, "failed to get usb2 trk clock: %d\n", err);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903071937.533646-1-waynec@nvidia.com?part=2