Re: [PATCH 2/3] usb: typec: Add Hynetek HUSB320 Type-C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Masking interrupt status bits before Write-1-to-Clear (W1C) clearing causes unhandled or reserved interrupts to remain asserted, leading to an interrupt storm.
- [Medium] The driver incorrectly reports the power role as TYPEC_SOURCE when connected to a Charge-Through Audio Accessory.
--

--- Patch [2]: [PATCH 2/3] usb: typec: Add Hynetek HUSB320 Type-C controller driver ---
commit e0d1ae316e83fd4e771770397178a91c9673d5ac
Author: Hongyang Zhao <hongyang.zhao@163.com>

usb: typec: Add Hynetek HUSB320 Type-C controller driver

This commit adds a new USB Type-C controller driver for the Hynetek HUSB320.
The driver implements autonomous Configuration Channel handling, supporting
attach/detach detection, role switching, and accessory modes.

> diff --git a/drivers/usb/typec/husb320.c b/drivers/usb/typec/husb320.c
> new file mode 100644
> index 0000000000000..fd9f949d9b522
> --- /dev/null
> +++ b/drivers/usb/typec/husb320.c

[ ... ]

> +static enum husb320_state husb320_get_state(unsigned int status,
> +					    unsigned int type)
> +{
> +	if (!(status & HUSB320_STATUS_ATTACHED))
> +		return HUSB320_STATE_UNATTACHED;
> +
> +	if (type & HUSB320_TYPE_DEBUG_SOURCE)
> +		return HUSB320_STATE_DEBUG_SOURCE;
> +	if (type & HUSB320_TYPE_DEBUG_SINK)
> +		return HUSB320_STATE_DEBUG_SINK;
> +	if (type & HUSB320_TYPE_SINK)
> +		return HUSB320_STATE_SINK;
> +	if (type & HUSB320_TYPE_SOURCE)
> +		return HUSB320_STATE_SOURCE;
> +	if (type & (HUSB320_TYPE_AUDIO_VBUS | HUSB320_TYPE_AUDIO))
> +		return HUSB320_STATE_AUDIO;

[Severity: Medium]
Does mapping HUSB320_TYPE_AUDIO_VBUS to HUSB320_STATE_AUDIO cause
charge-through accessories to be treated exactly the same as standard audio
accessories?

Since husb320_state_power_role() forces TYPEC_SOURCE for HUSB320_STATE_AUDIO,
would this result in incorrectly reporting the host as providing power when
it is actually receiving it?

> +
> +	return HUSB320_STATE_UNATTACHED;
> +}

[ ... ]

> +static irqreturn_t husb320_irq_thread(int irq, void *data)
> +{
> +	struct husb320 *husb = data;
> +	unsigned int interrupt;
> +	unsigned int interrupt1;
> +	int disable_ret;
> +	int ret;
> +
> +	mutex_lock(&husb->lock);
> +	if (husb->shutting_down) {
> +		disable_irq_nosync(irq);
> +		mutex_unlock(&husb->lock);
> +		return IRQ_HANDLED;
> +	}
> +
> +	ret = regmap_read(husb->regmap, HUSB320_REG_INTERRUPT, &interrupt);
> +	if (ret)
> +		goto err;
> +
> +	ret = regmap_read(husb->regmap, HUSB320_REG_INTERRUPT1, &interrupt1);
> +	if (ret)
> +		goto err;
> +
> +	interrupt &= HUSB320_INT_VALID;
> +	interrupt1 &= HUSB320_INT1_VALID;
> +	if (!interrupt && !interrupt1) {
> +		mutex_unlock(&husb->lock);
> +		return IRQ_NONE;
> +	}
> +
> +	if (interrupt) {
> +		ret = regmap_write(husb->regmap, HUSB320_REG_INTERRUPT,
> +				   interrupt);

[Severity: High]
If the hardware asserts a reserved or undocumented interrupt bit, will
masking the interrupt variable against HUSB320_INT_VALID prevent that bit
from being cleared when written back?

Could this cause an unhandled hardware interrupt to remain asserted,
leading to an interrupt storm that eventually forces the kernel to disable
the IRQ line?

> +		if (ret)
> +			goto err;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-husb320-v1-0-4a1e259d7dc8@163.com?part=2