[PATCH 0/8] usb: typec: fusb302: cleanup device driver probe

[PATCH 0/8] usb: typec: fusb302: cleanup device driver probe

[Sebastian Reichel]

Move fusb302 to be fully converted to device managed resources
instead of having a mix risking leaks and clean up the interrupt
setup.

These have been tested for over half a year locally with different
Rockchip boards. I waited for [0] to land, so that the DRM HPD auxillary
device registration can also be dropped. As that series drags forever,
let's get these cleanups processed independently.

Note, that this first cleanups the GPIO interrupt handling and then drops
it completley. The rationale is, that the last patch has a (low) chance of
regressing somebody's system and having to be reverted. In that case the
custom interrupt GPIO API should be properly documented (as deprecated)
and we want at least the cleanups.

The series is based on 7.4-rc2 instead of Greg's usb-next branch (based
on 7.4-rc1 and still empty), which is relevant for the first patch as
older versions had devm_kzalloc() instead of devm_kzalloc_obj().

[0] https://lore.kernel.org/linux-rockchip/20261020072816.180-3-kernel@airkyi.com/

Greetings,

-- Sebastian

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Sebastian Reichel (8):
      usb: typec: tcpm: allocate pd_list entries non device managed
      usb: typec: tcpm: add device managed port registration
      usb: typec: fusb302: Switch to device managed resources
      usb: typec: fusb302: switch to devm_pm_set_wake_irq
      usb: typec: fusb302: rename init_gpio into fusb302_init_irq
      usb: typec: fusb302: move gpio_int_n into function local scope
      usb: typec: fusb302: rework gpio_int_n_irq handling
      usb: typec: fusb302: drop custom gpio interrupt logic

 drivers/usb/typec/tcpm/fusb302.c | 160 +++++++++++++++++----------------------
 drivers/usb/typec/tcpm/tcpm.c    |  29 ++++++-
 include/linux/usb/tcpm.h         |   3 +
 3 files changed, 101 insertions(+), 91 deletions(-)
---
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
change-id: 20261113-fusb302-probe-cleanup-9b1adac8b81f

Best regards,
--  
Sebastian Reichel <sebastian.reichel@collabora.com>

[PATCH 1/8] usb: typec: tcpm: allocate pd_list entries non device managed

[Sebastian Reichel]

The data is free'd manually, so there is no need for the device
managed overhead. At the same time it conflicts when the TCPM
itself is registered device managed (not yet supported).

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index bb90211660ca..07d2a03d2b8d 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -8092,7 +8092,7 @@ static void tcpm_port_unregister_pd(struct tcpm_port *port)
 	for (i = 0; i < port->pd_count; i++) {
 		usb_power_delivery_unregister_capabilities(port->pd_list[i]->sink_cap);
 		usb_power_delivery_unregister_capabilities(port->pd_list[i]->source_cap);
-		devm_kfree(port->dev, port->pd_list[i]);
+		kfree(port->pd_list[i]);
 		port->pd_list[i] = NULL;
 		usb_power_delivery_unregister(port->pds[i]);
 		port->pds[i] = NULL;
@@ -8391,7 +8391,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode
 	}
 
 	for (i = 0; i < port->pd_count; i++) {
-		port->pd_list[i] = devm_kzalloc_obj(port->dev, struct pd_data);
+		port->pd_list[i] = kzalloc_obj(*port->pd_list[i]);
 		if (!port->pd_list[i]) {
 			ret = -ENOMEM;
 			goto put_capabilities;

-- 
2.53.0

Re: [PATCH 1/8] usb: typec: tcpm: allocate pd_list entries non device managed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: tcpm: leak of pd_list entries on
  tcpm_fw_get_caps() error paths
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: typec: tcpm: fwnode reference leak in tcpm_fw_get_caps()
  [1]
--

commit b86bde7535f2bafb293f7e234db00124b8e22755
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: typec: tcpm: allocate pd_list entries non device managed

This commit replaces devm_kzalloc_obj() and devm_kfree() with unmanaged
kzalloc_obj() and kfree() for port->pd_list[i] allocations.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index bb90211660cae..07d2a03d2b8d7 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c

[ ... ]

> @@ -8391,7 +8391,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode
>  	}
>  
>  	for (i = 0; i < port->pd_count; i++) {
> -		port->pd_list[i] = devm_kzalloc_obj(port->dev, struct pd_data);
> +		port->pd_list[i] = kzalloc_obj(*port->pd_list[i]);
>  		if (!port->pd_list[i]) {
>  			ret = -ENOMEM;
>  			goto put_capabilities;

[Severity: Medium]
Does switching port->pd_list[i] to unmanaged kzalloc_obj() leak those
entries when tcpm_fw_get_caps() or tcpm_register_port() fails?

In tcpm_fw_get_caps(), neither put_caps nor put_capabilities frees already
allocated port->pd_list[0..i] entries if a later loop iteration or PDO
validation fails.  Even when tcpm_fw_get_caps() succeeds,
tcpm_port_unregister_pd() (where kfree(port->pd_list[i]) runs) is only
reached via out_unregister_pd at the end of tcpm_register_port(), so earlier
failures such as -EPROBE_DEFER from usb_role_switch_get() jump to
out_destroy_wq or out_role_sw_put and leak port->pd_list[i]:

drivers/usb/typec/tcpm/tcpm.c:tcpm_register_port() {
    ...
	if (IS_ERR(port->role_sw)) {
		err = PTR_ERR(port->role_sw);
		goto out_destroy_wq;
	}

	err = devm_tcpm_psy_register(port);
	if (err)
		goto out_role_sw_put;
    ...
out_unregister_pd:
	tcpm_port_unregister_pd(port);
out_role_sw_put:
	usb_role_switch_put(port->role_sw);
out_destroy_wq:
	tcpm_debugfs_exit(port);
	kthread_destroy_worker(port->wq);
	return ERR_PTR(err);
}

[Severity: Medium]
This problem wasn't introduced by this patch, but jumping to
put_capabilities when kzalloc_obj() fails on iteration i > 0 leaks the caps
fwnode reference [1] acquired by fwnode_get_next_child_node() in the
previous iteration, because it bypasses put_caps where fwnode_handle_put(caps)
is called.

[1] https://sashiko.dev/#/bug/linux-d106a4e9-4871-4f7c-8575-50cb448be123

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=1

[PATCH 2/8] usb: typec: tcpm: add device managed port registration

[Sebastian Reichel]

A few drivers would benefit from having device managed TCPM
port registration, so that their probe routine does not mix
device managed and non-device managed calls.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 25 +++++++++++++++++++++++++
 include/linux/usb/tcpm.h      |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 07d2a03d2b8d..7d6378acf018 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -9013,6 +9013,31 @@ void tcpm_unregister_port(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_unregister_port);
 
+static void devm_tcpm_unregister_port(void *data)
+{
+	struct tcpm_port *port = data;
+
+	tcpm_unregister_port(port);
+}
+
+struct tcpm_port *devm_tcpm_register_port(struct device *dev,
+					  struct tcpc_dev *tcpc)
+{
+	struct tcpm_port *result;
+	int ret;
+
+	result = tcpm_register_port(dev, tcpc);
+	if (IS_ERR(result))
+		return result;
+
+	ret = devm_add_action_or_reset(dev, devm_tcpm_unregister_port, result);
+	if (ret  < 0)
+		return ERR_PTR(ret);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(devm_tcpm_register_port);
+
 MODULE_AUTHOR("Guenter Roeck <groeck@chromium.org>");
 MODULE_DESCRIPTION("USB Type-C Port Manager");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 93079450bba0..fc60980f9f41 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -177,6 +177,9 @@ struct tcpm_port;
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc);
 void tcpm_unregister_port(struct tcpm_port *port);
 
+struct tcpm_port *devm_tcpm_register_port(struct device *dev,
+					  struct tcpc_dev *tcpc);
+
 void tcpm_vbus_change(struct tcpm_port *port);
 void tcpm_cc_change(struct tcpm_port *port);
 void tcpm_sink_frs(struct tcpm_port *port);

-- 
2.53.0

Re: [PATCH 2/8] usb: typec: tcpm: add device managed port registration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: tcpm: leak of port->pd_list[i] on
  tcpm_register_port() error paths
--

commit 4732c69a74cc6f816813133462e97f8deba7b034
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: typec: tcpm: add device managed port registration

This commit adds devm_tcpm_register_port() to support device-managed TCPM port
registration.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 07d2a03d2b8d7..7d6378acf0183 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -9013,6 +9013,31 @@ void tcpm_unregister_port(struct tcpm_port *port)
>  }
>  EXPORT_SYMBOL_GPL(tcpm_unregister_port);
>  
> +static void devm_tcpm_unregister_port(void *data)
> +{
> +	struct tcpm_port *port = data;
> +
> +	tcpm_unregister_port(port);
> +}
> +
> +struct tcpm_port *devm_tcpm_register_port(struct device *dev,
> +					  struct tcpc_dev *tcpc)
> +{
> +	struct tcpm_port *result;
> +	int ret;
> +
> +	result = tcpm_register_port(dev, tcpc);
> +	if (IS_ERR(result))
> +		return result;

[Severity: Medium]
Does this leak port->pd_list[i] when tcpm_register_port() fails before
tcpm_port_register_pd()?

To prepare for devm_tcpm_register_port(), the preceding commit "usb: typec:
tcpm: allocate pd_list entries non device managed" switched port->pd_list[i]
in tcpm_fw_get_caps() to non-managed kzalloc_obj() and freed the entries in
tcpm_port_unregister_pd().

However, if tcpm_fw_get_caps() fails after allocating one or more
port->pd_list[i] entries, or if a subsequent step in tcpm_register_port()
such as usb_role_switch_get() returning -EPROBE_DEFER fails and jumps to
out_destroy_wq or out_role_sw_put, tcpm_port_unregister_pd() is bypassed and
devm_tcpm_register_port() returns before registering its cleanup action:

drivers/usb/typec/tcpm/tcpm.c:tcpm_register_port() {
	...
	port->role_sw = fwnode_usb_role_switch_get(tcpc->fwnode);
	if (!port->role_sw)
		port->role_sw = usb_role_switch_get(port->dev);
	if (IS_ERR(port->role_sw)) {
		err = PTR_ERR(port->role_sw);
		goto out_destroy_wq;
	}
	...
out_unregister_pd:
	tcpm_port_unregister_pd(port);
out_role_sw_put:
	usb_role_switch_put(port->role_sw);
out_destroy_wq:
	tcpm_debugfs_exit(port);
	kthread_destroy_worker(port->wq);
	return ERR_PTR(err);
}

Each failed or deferred probe on a PD-capable port then leaks the allocated
port->pd_list[i] entries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=2

[PATCH 3/8] usb: typec: fusb302: Switch to device managed resources

[Sebastian Reichel]

The driver's probe routine is currently mixing registration calls for
device managed resources with unmananged ones. This results in issues
such as resource leaks or use-after-free. Fix this up by fully
converting the driver to device managed resources.

Resource acquisition has been reordered a bit, so that the work is
initialized after the TCPM port is registered as the work uses the
TCPM devices.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 112 ++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 83bd287dadc7..a7a1dce6cf86 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -8,6 +8,7 @@
 #include <drm/bridge/aux-bridge.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
+#include <linux/devm-helpers.h>
 #include <linux/errno.h>
 #include <linux/extcon.h>
 #include <linux/gpio/consumer.h>
@@ -210,20 +211,28 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
 
-static void fusb302_debugfs_init(struct fusb302_chip *chip)
+static void fusb302_debugfs_exit(void *data)
+{
+	struct fusb302_chip *chip = data;
+
+	debugfs_remove(chip->dentry);
+}
+
+static int fusb302_debugfs_init(struct fusb302_chip *chip)
 {
 	char name[NAME_MAX];
+	int ret;
+
+	ret = devm_mutex_init(chip->dev, &chip->logbuffer_lock);
+	if (ret < 0)
+		return ret;
 
-	mutex_init(&chip->logbuffer_lock);
 	snprintf(name, NAME_MAX, "fusb302-%s", dev_name(chip->dev));
 	chip->dentry = debugfs_create_dir(name, usb_debug_root);
 	debugfs_create_file("log", S_IFREG | 0444, chip->dentry, chip,
 			    &fusb302_debug_fops);
-}
 
-static void fusb302_debugfs_exit(struct fusb302_chip *chip)
-{
-	debugfs_remove(chip->dentry);
+	return devm_add_action_or_reset(chip->dev, fusb302_debugfs_exit, chip);
 }
 
 #else
@@ -231,7 +240,6 @@ static void fusb302_debugfs_exit(struct fusb302_chip *chip)
 static void fusb302_log(const struct fusb302_chip *chip,
 			const char *fmt, ...) { }
 static void fusb302_debugfs_init(const struct fusb302_chip *chip) { }
-static void fusb302_debugfs_exit(const struct fusb302_chip *chip) { }
 
 #endif
 
@@ -1686,6 +1694,13 @@ static struct fwnode_handle *fusb302_fwnode_get(struct device *dev)
 	return fwnode;
 }
 
+static void fusb302_fwnode_put(void *data)
+{
+	struct fusb302_chip *chip = data;
+
+	fwnode_handle_put(chip->tcpc_dev.fwnode);
+}
+
 static int fusb302_probe(struct i2c_client *client)
 {
 	struct fusb302_chip *chip;
@@ -1706,7 +1721,10 @@ static int fusb302_probe(struct i2c_client *client)
 
 	chip->i2c_client = client;
 	chip->dev = &client->dev;
-	mutex_init(&chip->lock);
+
+	ret = devm_mutex_init(dev, &chip->lock);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * Devicetree platforms should get extcon via phandle (not yet
@@ -1725,67 +1743,68 @@ static int fusb302_probe(struct i2c_client *client)
 	if (IS_ERR(chip->vbus))
 		return PTR_ERR(chip->vbus);
 
-	chip->wq = create_singlethread_workqueue(dev_name(chip->dev));
+	chip->wq = devm_alloc_ordered_workqueue(dev, dev_name(dev), 0);
 	if (!chip->wq)
 		return -ENOMEM;
 
 	spin_lock_init(&chip->irq_lock);
-	INIT_WORK(&chip->irq_work, fusb302_irq_work);
-	INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
 	init_tcpc_dev(&chip->tcpc_dev);
-	fusb302_debugfs_init(chip);
+
+	ret = fusb302_debugfs_init(chip);
+	if (ret < 0)
+		return ret;
 
 	if (client->irq) {
 		chip->gpio_int_n_irq = client->irq;
 	} else {
 		ret = init_gpio(chip);
 		if (ret < 0)
-			goto destroy_workqueue;
+			return ret;
 	}
 
 	chip->tcpc_dev.fwnode = fusb302_fwnode_get(dev);
 	if (IS_ERR(chip->tcpc_dev.fwnode)) {
 		ret = PTR_ERR(chip->tcpc_dev.fwnode);
-		goto destroy_workqueue;
+		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, fusb302_fwnode_put, chip);
+	if (ret < 0)
+		return ret;
+
 	bridge_dev = devm_drm_dp_hpd_bridge_alloc(chip->dev, to_of_node(chip->tcpc_dev.fwnode));
-	if (IS_ERR(bridge_dev)) {
-		ret = dev_err_probe(chip->dev, PTR_ERR(bridge_dev),
-				    "failed to alloc bridge\n");
-		goto fwnode_put;
-	}
+	if (IS_ERR(bridge_dev))
+		return dev_err_probe(chip->dev, PTR_ERR(bridge_dev),
+				     "failed to alloc bridge\n");
 
-	chip->tcpm_port = tcpm_register_port(&client->dev, &chip->tcpc_dev);
-	if (IS_ERR(chip->tcpm_port)) {
-		ret = dev_err_probe(dev, PTR_ERR(chip->tcpm_port),
+	chip->tcpm_port = devm_tcpm_register_port(&client->dev, &chip->tcpc_dev);
+	if (IS_ERR(chip->tcpm_port))
+		return dev_err_probe(dev, PTR_ERR(chip->tcpm_port),
 				    "cannot register tcpm port\n");
-		goto fwnode_put;
-	}
 
-	ret = devm_drm_dp_hpd_bridge_add(chip->dev, bridge_dev);
-	if (ret)
-		goto tcpm_unregister_port;
+	ret = devm_delayed_work_autocancel(dev, &chip->bc_lvl_handler,
+					   fusb302_bc_lvl_handler_work);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_work_autocancel(dev, &chip->irq_work, fusb302_irq_work);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, chip->gpio_int_n_irq, NULL,
+					fusb302_irq_intn,
+					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
+					"fsc_interrupt_int_n", chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to request IRQ");
 
-	ret = request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
-				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
-				   "fsc_interrupt_int_n", chip);
-	if (ret < 0) {
-		dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
-		goto tcpm_unregister_port;
-	}
-	enable_irq_wake(chip->gpio_int_n_irq);
 	i2c_set_clientdata(client, chip);
 
-	return 0;
+	ret = devm_drm_dp_hpd_bridge_add(chip->dev, bridge_dev);
+	if (ret)
+		return ret;
 
-tcpm_unregister_port:
-	tcpm_unregister_port(chip->tcpm_port);
-fwnode_put:
-	fwnode_handle_put(chip->tcpc_dev.fwnode);
-destroy_workqueue:
-	fusb302_debugfs_exit(chip);
-	destroy_workqueue(chip->wq);
+	enable_irq_wake(chip->gpio_int_n_irq);
 
 	return ret;
 }
@@ -1795,13 +1814,6 @@ static void fusb302_remove(struct i2c_client *client)
 	struct fusb302_chip *chip = i2c_get_clientdata(client);
 
 	disable_irq_wake(chip->gpio_int_n_irq);
-	free_irq(chip->gpio_int_n_irq, chip);
-	cancel_work_sync(&chip->irq_work);
-	cancel_delayed_work_sync(&chip->bc_lvl_handler);
-	tcpm_unregister_port(chip->tcpm_port);
-	fwnode_handle_put(chip->tcpc_dev.fwnode);
-	destroy_workqueue(chip->wq);
-	fusb302_debugfs_exit(chip);
 }
 
 static int fusb302_pm_suspend(struct device *dev)

-- 
2.53.0

Re: [PATCH 3/8] usb: typec: fusb302: Switch to device managed resources

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] usb: typec: fusb302: build failure when CONFIG_DEBUG_FS is
  disabled
--

commit a5e9f3a79e68305810f439c482d64544f3449258
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: typec: fusb302: Switch to device managed resources

This commit converts fusb302_probe() and fusb302_remove() to use
device-managed resources for mutexes, workqueues, debugfs, fwnode, TCPM
port registration, work items, and interrupts.

> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 83bd287dadc7e..a7a1dce6cf86a 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
[ ... ]
> @@ -210,20 +211,28 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
>  
> -static void fusb302_debugfs_init(struct fusb302_chip *chip)
> +static void fusb302_debugfs_exit(void *data)
> +{
> +	struct fusb302_chip *chip = data;
> +
> +	debugfs_remove(chip->dentry);
> +}
> +
> +static int fusb302_debugfs_init(struct fusb302_chip *chip)
>  {
>  	char name[NAME_MAX];
> +	int ret;
> +
> +	ret = devm_mutex_init(chip->dev, &chip->logbuffer_lock);
> +	if (ret < 0)
> +		return ret;
>  
> -	mutex_init(&chip->logbuffer_lock);
>  	snprintf(name, NAME_MAX, "fusb302-%s", dev_name(chip->dev));
>  	chip->dentry = debugfs_create_dir(name, usb_debug_root);
>  	debugfs_create_file("log", S_IFREG | 0444, chip->dentry, chip,
>  			    &fusb302_debug_fops);
> -}
>  
> -static void fusb302_debugfs_exit(struct fusb302_chip *chip)
> -{
> -	debugfs_remove(chip->dentry);
> +	return devm_add_action_or_reset(chip->dev, fusb302_debugfs_exit, chip);
>  }
>  
>  #else
>  
>  static void fusb302_log(const struct fusb302_chip *chip,
>  			const char *fmt, ...) { }
>  static void fusb302_debugfs_init(const struct fusb302_chip *chip) { }
> -static void fusb302_debugfs_exit(const struct fusb302_chip *chip) { }
>  
>  #endif

[Severity: High]
Does the !CONFIG_DEBUG_FS stub for fusb302_debugfs_init() also need to
return int (0)?  fusb302_probe() now assigns its return value to ret,
which fails to build when CONFIG_DEBUG_FS is disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=3

[PATCH 4/8] usb: typec: fusb302: switch to devm_pm_set_wake_irq

[Sebastian Reichel]

Use the modern device managed wake IRQ helper provided by the PM layer
instead of directly calling the interrupt wake enable/disable functions.

This results in the wake capability only being enabled once system suspend
is entered as its not supposed to be set while the system is running
normally. Also it now properly reacts to device_may_wakeup().

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index a7a1dce6cf86..8b83414c5819 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -19,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/proc_fs.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched/clock.h>
@@ -1804,16 +1805,11 @@ static int fusb302_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	enable_irq_wake(chip->gpio_int_n_irq);
-
-	return ret;
-}
-
-static void fusb302_remove(struct i2c_client *client)
-{
-	struct fusb302_chip *chip = i2c_get_clientdata(client);
+	ret = devm_device_init_wakeup(chip->dev);
+	if (ret)
+		return ret;
 
-	disable_irq_wake(chip->gpio_int_n_irq);
+	return devm_pm_set_wake_irq(chip->dev, chip->gpio_int_n_irq);
 }
 
 static int fusb302_pm_suspend(struct device *dev)
@@ -1870,7 +1866,6 @@ static struct i2c_driver fusb302_driver = {
 		   .of_match_table = of_match_ptr(fusb302_dt_match),
 		   },
 	.probe = fusb302_probe,
-	.remove = fusb302_remove,
 	.id_table = fusb302_i2c_device_id,
 };
 module_i2c_driver(fusb302_driver);

-- 
2.53.0

Re: [PATCH 4/8] usb: typec: fusb302: switch to devm_pm_set_wake_irq

[sashiko-bot]

> Use the modern device managed wake IRQ helper provided by the PM layer
> instead of directly calling the interrupt wake enable/disable functions.
> 
> This results in the wake capability only being enabled once system suspend
> is entered as its not supposed to be set while the system is running
> normally. Also it now properly reacts to device_may_wakeup().
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=4

[PATCH 5/8] usb: typec: fusb302: rename init_gpio into fusb302_init_irq

[Sebastian Reichel]

Add the fusb302_ prefix to init_gpio, since it is used by all
the other functions and allows easy figuring out that it is a
local function.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 8b83414c5819..1b3ee5715a74 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1643,7 +1643,7 @@ static void fusb302_irq_work(struct work_struct *work)
 	enable_irq(chip->gpio_int_n_irq);
 }
 
-static int init_gpio(struct fusb302_chip *chip)
+static int fusb302_init_irq(struct fusb302_chip *chip)
 {
 	struct device *dev = chip->dev;
 	int ret = 0;
@@ -1758,7 +1758,7 @@ static int fusb302_probe(struct i2c_client *client)
 	if (client->irq) {
 		chip->gpio_int_n_irq = client->irq;
 	} else {
-		ret = init_gpio(chip);
+		ret = fusb302_init_irq(chip);
 		if (ret < 0)
 			return ret;
 	}

-- 
2.53.0

Re: [PATCH 5/8] usb: typec: fusb302: rename init_gpio into fusb302_init_irq

[sashiko-bot]

> Add the fusb302_ prefix to init_gpio, since it is used by all
> the other functions and allows easy figuring out that it is a
> local function.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=5

[PATCH 6/8] usb: typec: fusb302: move gpio_int_n into function local scope

[Sebastian Reichel]

The driver only cares about the interrupt and the GPIO itself is
device managed, so drop the reference from the device structure
and only keep it in the function scope. While at it cleanup the
error handling by using dev_err_probe.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 1b3ee5715a74..76851e2b9ce5 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -86,7 +86,6 @@ struct fusb302_chip {
 	struct work_struct irq_work;
 	bool irq_suspended;
 	bool irq_while_suspended;
-	struct gpio_desc *gpio_int_n;
 	int gpio_int_n_irq;
 	struct extcon_dev *extcon;
 
@@ -1646,19 +1645,17 @@ static void fusb302_irq_work(struct work_struct *work)
 static int fusb302_init_irq(struct fusb302_chip *chip)
 {
 	struct device *dev = chip->dev;
+	struct gpio_desc *int_gpio;
 	int ret = 0;
 
-	chip->gpio_int_n = devm_gpiod_get(dev, "fcs,int_n", GPIOD_IN);
-	if (IS_ERR(chip->gpio_int_n)) {
-		dev_err(dev, "failed to request gpio_int_n\n");
-		return PTR_ERR(chip->gpio_int_n);
-	}
-	ret = gpiod_to_irq(chip->gpio_int_n);
-	if (ret < 0) {
-		dev_err(dev,
-			"cannot request IRQ for GPIO Int_N, ret=%d", ret);
-		return ret;
-	}
+	int_gpio = devm_gpiod_get(dev, "fcs,int_n", GPIOD_IN);
+	if (IS_ERR(int_gpio))
+		return dev_err_probe(dev, PTR_ERR(int_gpio), "failed to request interrupt GPIO\n");
+
+	ret = gpiod_to_irq(int_gpio);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot request IRQ for GPIO\n");
+
 	chip->gpio_int_n_irq = ret;
 	return 0;
 }

-- 
2.53.0

Re: [PATCH 6/8] usb: typec: fusb302: move gpio_int_n into function local scope

[sashiko-bot]

> The driver only cares about the interrupt and the GPIO itself is
> device managed, so drop the reference from the device structure
> and only keep it in the function scope. While at it cleanup the
> error handling by using dev_err_probe.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=6

[PATCH 7/8] usb: typec: fusb302: rework gpio_int_n_irq handling

[Sebastian Reichel]

Improve the code readability, so that one does not need to jump
into fusb302_init_irq() to understand that chip->gpio_int_n_irq
is guranteed to be initialized.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 76851e2b9ce5..6105f09f96aa 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1656,8 +1656,7 @@ static int fusb302_init_irq(struct fusb302_chip *chip)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "cannot request IRQ for GPIO\n");
 
-	chip->gpio_int_n_irq = ret;
-	return 0;
+	return ret;
 }
 
 #define PDO_FIXED_FLAGS \
@@ -1752,13 +1751,13 @@ static int fusb302_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	if (client->irq) {
+	if (client->irq)
 		chip->gpio_int_n_irq = client->irq;
-	} else {
-		ret = fusb302_init_irq(chip);
-		if (ret < 0)
-			return ret;
-	}
+	else
+		chip->gpio_int_n_irq = fusb302_init_irq(chip);
+
+	if (chip->gpio_int_n_irq < 0)
+		return chip->gpio_int_n_irq;
 
 	chip->tcpc_dev.fwnode = fusb302_fwnode_get(dev);
 	if (IS_ERR(chip->tcpc_dev.fwnode)) {

-- 
2.53.0

Re: [PATCH 7/8] usb: typec: fusb302: rework gpio_int_n_irq handling

[sashiko-bot]

> Improve the code readability, so that one does not need to jump
> into fusb302_init_irq() to understand that chip->gpio_int_n_irq
> is guranteed to be initialized.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=7

[PATCH 8/8] usb: typec: fusb302: drop custom gpio interrupt logic

[Sebastian Reichel]

When the driver was upstreamed it contained the logic to fetch a
"fcs,int_n" GPIO from device-tree, convert it into an interrupt and use
it. This was never part of the binding and there was only a single
upstream user, which got converted to follow the proper bindings in 2020
in commit 89ee3ace7292 ("arm64: dts: rockchip: Fix Pinebook Pro FUSB302
interrupt"). Drop the custom logic and only allow the properly
documented ABI.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 39 +++++++++------------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 6105f09f96aa..c392134cdc0c 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -11,7 +11,6 @@
 #include <linux/devm-helpers.h>
 #include <linux/errno.h>
 #include <linux/extcon.h>
-#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -86,7 +85,7 @@ struct fusb302_chip {
 	struct work_struct irq_work;
 	bool irq_suspended;
 	bool irq_while_suspended;
-	int gpio_int_n_irq;
+	int irq;
 	struct extcon_dev *extcon;
 
 	struct workqueue_struct *wq;
@@ -1495,7 +1494,7 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
 	unsigned long flags;
 
 	/* Disable our level triggered IRQ until our irq_work has cleared it */
-	disable_irq_nosync(chip->gpio_int_n_irq);
+	disable_irq_nosync(chip->irq);
 
 	spin_lock_irqsave(&chip->irq_lock, flags);
 	if (chip->irq_suspended)
@@ -1639,24 +1638,7 @@ static void fusb302_irq_work(struct work_struct *work)
 	}
 done:
 	mutex_unlock(&chip->lock);
-	enable_irq(chip->gpio_int_n_irq);
-}
-
-static int fusb302_init_irq(struct fusb302_chip *chip)
-{
-	struct device *dev = chip->dev;
-	struct gpio_desc *int_gpio;
-	int ret = 0;
-
-	int_gpio = devm_gpiod_get(dev, "fcs,int_n", GPIOD_IN);
-	if (IS_ERR(int_gpio))
-		return dev_err_probe(dev, PTR_ERR(int_gpio), "failed to request interrupt GPIO\n");
-
-	ret = gpiod_to_irq(int_gpio);
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "cannot request IRQ for GPIO\n");
-
-	return ret;
+	enable_irq(chip->irq);
 }
 
 #define PDO_FIXED_FLAGS \
@@ -1751,13 +1733,10 @@ static int fusb302_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	if (client->irq)
-		chip->gpio_int_n_irq = client->irq;
-	else
-		chip->gpio_int_n_irq = fusb302_init_irq(chip);
-
-	if (chip->gpio_int_n_irq < 0)
-		return chip->gpio_int_n_irq;
+	chip->irq = client->irq;
+	if (!chip->irq)
+		return dev_err_probe(chip->dev, -ENXIO,
+				     "missing interrupt\n");
 
 	chip->tcpc_dev.fwnode = fusb302_fwnode_get(dev);
 	if (IS_ERR(chip->tcpc_dev.fwnode)) {
@@ -1788,7 +1767,7 @@ static int fusb302_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = devm_request_threaded_irq(dev, chip->gpio_int_n_irq, NULL,
+	ret = devm_request_threaded_irq(dev, chip->irq, NULL,
 					fusb302_irq_intn,
 					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 					"fsc_interrupt_int_n", chip);
@@ -1805,7 +1784,7 @@ static int fusb302_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	return devm_pm_set_wake_irq(chip->dev, chip->gpio_int_n_irq);
+	return devm_pm_set_wake_irq(chip->dev, chip->irq);
 }
 
 static int fusb302_pm_suspend(struct device *dev)

-- 
2.53.0

Re: [PATCH 8/8] usb: typec: fusb302: drop custom gpio interrupt logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: fusb302: void stub for fusb302_debugfs_init()
  breaks !CONFIG_DEBUG_FS build
--

commit 68b5d98996b526119f13f5957d4fd6cf07fb01e3
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: typec: fusb302: drop custom gpio interrupt logic

This commit removes the undocumented "fcs,int_n" GPIO fallback from the
fusb302 driver and requires client->irq to be populated.

> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 6105f09f96aad..c392134cdc0c5 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c

[ ... ]

> @@ -1751,13 +1733,10 @@ static int fusb302_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;

[Severity: Medium]
Does fusb302_probe() fail to build when CONFIG_DEBUG_FS is disabled?

Earlier in this series, commit "usb: typec: fusb302: Switch to device
managed resources" changed fusb302_debugfs_init() to return int and
assigned its return value to ret right above this check, but the
!CONFIG_DEBUG_FS stub still returns void:

drivers/usb/typec/tcpm/fusb302.c:
#ifdef CONFIG_DEBUG_FS
...
static int fusb302_debugfs_init(struct fusb302_chip *chip)
...
#else
...
static void fusb302_debugfs_init(const struct fusb302_chip *chip) { }

#endif

Returning 0 from the !CONFIG_DEBUG_FS stub of fusb302_debugfs_init()
avoids the build failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=8