linux-kernel - Re: [PATCH v5 2/2] usb: typec: hd3ss3220: Enable VBUS based on ID pin state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aP8_oZlJ4466BEf0@kuha.fi.intel.com>
Date: Mon, 27 Oct 2025 11:47:13 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Biju Das <biju.das.jz@...renesas.com>,
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
	linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] usb: typec: hd3ss3220: Enable VBUS based on ID
 pin state

Hi Krishna,

> +static int hd3ss3220_get_vbus_supply(struct hd3ss3220 *hd3ss3220)
> +{
> +	struct device_node *hd3ss3220_node = hd3ss3220->dev->of_node;
> +	struct device_node *np;
> +
> +	np = of_graph_get_remote_node(hd3ss3220_node, 0, 0);
> +	if (!np) {
> +		dev_err(hd3ss3220->dev, "failed to get device node");
> +		return -ENODEV;
> +	}

So I guess that's the connector node. Why can't you just place the
regulator reference to the hd3ss3220 controller node instead of the
connector like the port controllers do?

That would allow us to do a simple devm_regulator_get_optional() call
that's not tied to DT only.

> +	hd3ss3220->vbus = of_regulator_get_optional(hd3ss3220->dev, np, "vbus");
> +	if (IS_ERR(hd3ss3220->vbus))
> +		hd3ss3220->vbus = NULL;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +}
> +
>  static int hd3ss3220_probe(struct i2c_client *client)
>  {
>  	struct typec_capability typec_cap = { };

<snip>

> +	ret = hd3ss3220_get_vbus_supply(hd3ss3220);
> +	if (ret)
> +		return dev_err_probe(hd3ss3220->dev, ret, "failed to get vbus\n");

I think you have resource leaks here. I'm pretty sure you need to do
regulator_put() somewhere. You are also leaking the connector fwnode
that was acquired just before this point..

>  	if (IS_ERR(hd3ss3220->role_sw)) {
>  		ret = PTR_ERR(hd3ss3220->role_sw);
>  		goto err_put_fwnode;

Get the regulator here after the above condition. Then add a label for
the regulator_put(). And you already have the handle to the connector
fwnode so use that one instead of getting it again:

        hd3ss3220->vbus = of_regulator_get_optional(hd3ss3220->dev, to_of_node(connector), "vbus");

But do it like that only if you really can't place the vbus regulator
reference to the controller node. I would really prefer that we could
do a simple:

        hd3ss3220->vbus = devm_regulator_get_optional(hd3ss3220->dev, "vbus");

thanks,

-- 
heikki