Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Moving the entire driver implementation into a header file (`core.h`) and including it directly causes code bloat and violates C best practices.
- [Medium] The module license string was changed from strict 'GPL v2' to 'GPL', broadening the license terms and conflicting with the SPDX tag.

Pre-existing issues:
- [High] Delayed works (`wq_poll` and `update_work`) are not cancelled on the probe error path, leading to a use-after-free.
- [High] Suspend/resume functions blindly modify IRQ 0 if `wakeup-source` is set but no interrupt is provided, and the IRQ wake count is unbalanced.
- [High] The device is accessed via I2C before its reset line is deasserted during resume.
--

--- Patch [2]: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 6db23dbbea249ebf73709095210ec56e8a1e7da9
Author: Alyssa Milburn <amilburn@zall.org>

usb: typec: tipd: Factor out i2c specifics

Make the core driver more bus-agnostic to prepare for SPMI variants of
the tipd chip

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.h
> similarity index 95%
> rename from drivers/usb/typec/tipd/core.c
> rename to drivers/usb/typec/tipd/core.h
> index d5ee0af9058b9..1ba29e439e01c 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.h

[Severity: Medium]
Is it intentional to move the entire core driver implementation into a header
file? 

If this file is included from multiple bus variant C files, won't it
result in duplicate copies of all these static functions in every object file,
causing significant kernel binary bloat? 

Would it be better to leave the core logic in a separate C file and export
the necessary functions?

[ ... ]
> @@ -1860,13 +1842,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_disconnect;

[Severity: High]
This is a pre-existing issue, but does this error path cause a use-after-free?

In tps6598x_probe(), if a plug is present, tps->data->connect() may queue
cd321x->update_work, or if tps->irq == 0, wq_poll is queued. 

If a subsequent step fails (like devm_request_threaded_irq) or connect()
itself fails when there is no interrupt, the code jumps to err_disconnect:

err_disconnect:
	tps6598x_disconnect(tps, 0);
err_unregister_port:
	...

This error path does not appear to cancel wq_poll or update_work, meaning
they could execute after the tps structure is freed by devres.

[ ... ]
> @@ -1913,17 +1892,16 @@ static void tps6598x_remove(struct i2c_client *client)
>  
>  static int __maybe_unused tps6598x_suspend(struct device *dev)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
> +	struct tps6598x *tps = tps6598x_from_device(dev);
>  
>  	if (tps->wakeup) {
> -		disable_irq(client->irq);
> -		enable_irq_wake(client->irq);

[Severity: High]
This isn't a bug introduced by this patch, but what happens here if a device
tree specifies "wakeup-source" but does not provide an interrupt line?

During tps6598x_probe(), tps->wakeup would be set to true. Then in
tps6598x_suspend(), this check unconditionally calls disable_irq() and
enable_irq_wake() even if tps->irq is 0. 

Could modifying irq 0 disable critical system timers and hang the system
during suspend?

Also, enable_irq_wake() is called during probe when tps->wakeup and
tps->irq are set, but there appears to be no corresponding disable_irq_wake()
in tps6598x_remove(), which might leak the wake count.

>  	} else if (tps->reset) {
>  		gpiod_set_value_cansleep(tps->reset, 1);
>  	}

[ ... ]
> @@ -1931,8 +1909,7 @@ static int __maybe_unused tps6598x_suspend(struct device *dev)
>  
>  static int __maybe_unused tps6598x_resume(struct device *dev)
>  {
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct tps6598x *tps = i2c_get_clientdata(client);
> +	struct tps6598x *tps = tps6598x_from_device(dev);
>  	int ret;
>  
>  	ret = tps6598x_check_mode(tps);

[Severity: High]
This is a pre-existing issue, but does this i2c transaction happen too early?

If this is a non-wakeup device with a configured reset gpio, system suspend
asserts the hardware reset by setting the gpio to 1. 

During system resume, tps6598x_resume() attempts to communicate with the
device via tps6598x_check_mode() here, before the reset gpio is deasserted
later in the function. 

Will this cause the i2c read to fail and leave the device unusable?

>  	if (ret < 0)
>  		return ret;

[ ... ]
> diff --git a/drivers/usb/typec/tipd/i2c.c b/drivers/usb/typec/tipd/i2c.c
> new file mode 100644
> index 0000000000000..0233e19290ea2
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/i2c.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for TI TPS6598x USB Power Delivery controller family
> + *
> + * Copyright (C) 2017, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/module.h>
> +
> +#include "core.h"

[Severity: Medium]
By including "core.h" directly instead of linking object files, aren't we
embedding the entire core driver implementation directly into this
compilation unit?

[ ... ]
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> +MODULE_LICENSE("GPL");

[Severity: Medium]
Is it intentional to change the module license string from "GPL v2" to "GPL"?

In the kernel, "GPL" implies "GPL v2 or later", which broadens the license
terms and seems to conflict with the SPDX tag at the top of the file.

> +MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Driver");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-tipd-ace3-v1-0-73202d36138a@chaosmail.tech?part=2