[PATCH 1/4] usb: typec: tipd: Cleanup resources if devm_tps6598_psy_register fails

[PATCH 1/4] usb: typec: tipd: Cleanup resources if devm_tps6598_psy_register fails

[Sven Peter]

We can't just return if devm_tps6598_psy_register fails since previous
resources are not devres managed and have yet to be cleaned up.

Fixes: 10eb0b6ac63a ("usb: typec: tps6598x: Export some power supply properties")
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 748ff4f6b5f6..ebc786d728e2 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -827,7 +827,7 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	ret = devm_tps6598_psy_register(tps);
 	if (ret)
-		return ret;
+		goto err_role_put;
 
 	tps->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(tps->port)) {
-- 
2.25.1

[PATCH 2/4] usb: typec: tipd: Fix spurious fwnode_handle_put in error path

[Sven Peter]

The err_role_put error path always calls fwnode_handle_put to release
the fwnode. This path can be reached after probe itself has already
released that fwnode though. Fix that by moving fwnode_handle_put in the
happy path to the very end.

Fixes: 18a6c866bb19 ("usb: typec: tps6598x: Add USB role switching logic")
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index ebc786d728e2..824e573af570 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -834,7 +834,6 @@ static int tps6598x_probe(struct i2c_client *client)
 		ret = PTR_ERR(tps->port);
 		goto err_role_put;
 	}
-	fwnode_handle_put(fwnode);
 
 	if (tps->status & TPS_STATUS_PLUG_PRESENT) {
 		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
@@ -858,6 +857,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	}
 
 	i2c_set_clientdata(client, tps);
+	fwnode_handle_put(fwnode);
 
 	return 0;
 
-- 
2.25.1

Re: [PATCH 2/4] usb: typec: tipd: Fix spurious fwnode_handle_put in error path

[Heikki Krogerus]

On Mon, Nov 14, 2022 at 06:44:47PM +0100, Sven Peter wrote:
> The err_role_put error path always calls fwnode_handle_put to release
> the fwnode. This path can be reached after probe itself has already
> released that fwnode though. Fix that by moving fwnode_handle_put in the
> happy path to the very end.
> 
> Fixes: 18a6c866bb19 ("usb: typec: tps6598x: Add USB role switching logic")
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

This looks like stable material as well.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index ebc786d728e2..824e573af570 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -834,7 +834,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  		ret = PTR_ERR(tps->port);
>  		goto err_role_put;
>  	}
> -	fwnode_handle_put(fwnode);
>  
>  	if (tps->status & TPS_STATUS_PLUG_PRESENT) {
>  		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
> @@ -858,6 +857,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	}
>  
>  	i2c_set_clientdata(client, tps);
> +	fwnode_handle_put(fwnode);
>  
>  	return 0;
>  

thanks,

-- 
heikki

[PATCH 3/4] usb: typec: tipd: Fix typec_unregister_port error paths

[Sven Peter]

typec_unregister_port is only called for some error paths after
typec_register_port was successful. Ensure it's called in all
cases.

Fixes: 92440202a880 ("usb: typec: tipd: Only update power status on IRQ")
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 824e573af570..c35501a92b4d 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -839,7 +839,7 @@ static int tps6598x_probe(struct i2c_client *client)
 		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
 		if (ret < 0) {
 			dev_err(tps->dev, "failed to read power status: %d\n", ret);
-			goto err_role_put;
+			goto err_unregister_port;
 		}
 		ret = tps6598x_connect(tps);
 		if (ret)
@@ -852,8 +852,7 @@ static int tps6598x_probe(struct i2c_client *client)
 					dev_name(&client->dev), tps);
 	if (ret) {
 		tps6598x_disconnect(tps, 0);
-		typec_unregister_port(tps->port);
-		goto err_role_put;
+		goto err_unregister_port;
 	}
 
 	i2c_set_clientdata(client, tps);
@@ -861,6 +860,8 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	return 0;
 
+err_unregister_port:
+	typec_unregister_port(tps->port);
 err_role_put:
 	usb_role_switch_put(tps->role_sw);
 err_fwnode_put:
-- 
2.25.1

Re: [PATCH 3/4] usb: typec: tipd: Fix typec_unregister_port error paths

[Heikki Krogerus]

On Mon, Nov 14, 2022 at 06:44:48PM +0100, Sven Peter wrote:
> typec_unregister_port is only called for some error paths after
> typec_register_port was successful. Ensure it's called in all
> cases.
> 
> Fixes: 92440202a880 ("usb: typec: tipd: Only update power status on IRQ")
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

Also direct stable material.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 824e573af570..c35501a92b4d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -839,7 +839,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
>  		if (ret < 0) {
>  			dev_err(tps->dev, "failed to read power status: %d\n", ret);
> -			goto err_role_put;
> +			goto err_unregister_port;
>  		}
>  		ret = tps6598x_connect(tps);
>  		if (ret)
> @@ -852,8 +852,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  					dev_name(&client->dev), tps);
>  	if (ret) {
>  		tps6598x_disconnect(tps, 0);
> -		typec_unregister_port(tps->port);
> -		goto err_role_put;
> +		goto err_unregister_port;
>  	}
>  
>  	i2c_set_clientdata(client, tps);
> @@ -861,6 +860,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> +err_unregister_port:
> +	typec_unregister_port(tps->port);
>  err_role_put:
>  	usb_role_switch_put(tps->role_sw);
>  err_fwnode_put:

thanks,

-- 
heikki

[PATCH 4/4] usb: typec: tipd: Move tps6598x_disconnect error path to its own label

[Sven Peter]

While the code currently correctly calls tps6598x_disconnect before jumping
to the error cleanup label it's inconsistent compared to all the other cleanup
actions and prone to introduce bugs if any more resources are added.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index c35501a92b4d..22ff212e05e6 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -850,16 +850,16 @@ static int tps6598x_probe(struct i2c_client *client)
 					irq_handler,
 					IRQF_SHARED | IRQF_ONESHOT,
 					dev_name(&client->dev), tps);
-	if (ret) {
-		tps6598x_disconnect(tps, 0);
-		goto err_unregister_port;
-	}
+	if (ret)
+		goto err_disconnect;
 
 	i2c_set_clientdata(client, tps);
 	fwnode_handle_put(fwnode);
 
 	return 0;
 
+err_disconnect:
+	tps6598x_disconnect(tps, 0);
 err_unregister_port:
 	typec_unregister_port(tps->port);
 err_role_put:
-- 
2.25.1

Re: [PATCH 4/4] usb: typec: tipd: Move tps6598x_disconnect error path to its own label

[Heikki Krogerus]

On Mon, Nov 14, 2022 at 06:44:49PM +0100, Sven Peter wrote:
> While the code currently correctly calls tps6598x_disconnect before jumping
> to the error cleanup label it's inconsistent compared to all the other cleanup
> actions and prone to introduce bugs if any more resources are added.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index c35501a92b4d..22ff212e05e6 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -850,16 +850,16 @@ static int tps6598x_probe(struct i2c_client *client)
>  					irq_handler,
>  					IRQF_SHARED | IRQF_ONESHOT,
>  					dev_name(&client->dev), tps);
> -	if (ret) {
> -		tps6598x_disconnect(tps, 0);
> -		goto err_unregister_port;
> -	}
> +	if (ret)
> +		goto err_disconnect;
>  
>  	i2c_set_clientdata(client, tps);
>  	fwnode_handle_put(fwnode);
>  
>  	return 0;
>  
> +err_disconnect:
> +	tps6598x_disconnect(tps, 0);
>  err_unregister_port:
>  	typec_unregister_port(tps->port);
>  err_role_put:

thanks,

-- 
heikki

Re: [PATCH 1/4] usb: typec: tipd: Cleanup resources if devm_tps6598_psy_register fails

[Heikki Krogerus]

On Mon, Nov 14, 2022 at 06:44:46PM +0100, Sven Peter wrote:
> We can't just return if devm_tps6598_psy_register fails since previous
> resources are not devres managed and have yet to be cleaned up.
> 
> Fixes: 10eb0b6ac63a ("usb: typec: tps6598x: Export some power supply properties")
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

I think this should also automatically go to the stable tree.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 748ff4f6b5f6..ebc786d728e2 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -827,7 +827,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	ret = devm_tps6598_psy_register(tps);
>  	if (ret)
> -		return ret;
> +		goto err_role_put;
>  
>  	tps->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(tps->port)) {

thanks,

-- 
heikki