Re: [PATCH 2/4] usb: misc: qcom_eud: set the TCSR check register before enabling on QCM2290

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: misc: qcom_eud: tcsr_check left set when EUD_EN2 write
  fails
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: misc: qcom_eud: missing state update in enable_store()
  [1]
--

commit c35f742b781c2fbb7146624bfd77bf246348e228
Author: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>

usb: misc: qcom_eud: set the TCSR check register before enabling on QCM2290

This commit adds support for an optional third memory resource for the
TCSR check register on QCM2290 and writes to it via qcom_scm_io_writel()
when enabling and disabling the EUD.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 73f9fac22e19a..217f6c1791f01 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c

[ ... ]

> @@ -46,6 +47,16 @@ static int enable_eud(struct eud_chip *priv)
>  {
>  	int ret;
>  
> +	/*
> +	 * On QCM2290 the boot firmware sets the TCSR check register before
> +	 * EUD_EN2; without it the mode manager ignores the enable.
> +	 */
> +	if (priv->tcsr_check) {
> +		ret = qcom_scm_io_writel(priv->tcsr_check, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = qcom_scm_io_writel(priv->mode_mgr + EUD_REG_EUD_EN2, 1);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does this leave priv->tcsr_check set to 1 when the write to
EUD_REG_EUD_EN2 fails?

On non-debug TF-A builds, the write to priv->tcsr_check succeeds while the
write to EUD_REG_EUD_EN2 is rejected.  When enable_store() calls
disable_eud() to roll back after enable_eud() fails, disable_eud() also
writes to EUD_REG_EUD_EN2 first and returns on error before clearing
priv->tcsr_check:

drivers/usb/misc/qcom_eud.c:disable_eud() {
	ret = qcom_scm_io_writel(priv->mode_mgr + EUD_REG_EUD_EN2, 0);
	if (ret)
		return ret;

	if (priv->tcsr_check)
		qcom_scm_io_writel(priv->tcsr_check, 0);
    ...
}

As a result, the TCSR check register remains set to 1 even though enabling
the EUD failed.

> @@ -65,6 +76,9 @@ static int disable_eud(struct eud_chip *priv)
>  	if (ret)
>  		return ret;
>  
> +	if (priv->tcsr_check)
> +		qcom_scm_io_writel(priv->tcsr_check, 0);
> +
>  	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
>  	return 0;
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but does enable_store()
leave chip->enabled stuck as true [1] after disable_eud() succeeds?

drivers/usb/misc/qcom_eud.c:enable_store() {
    ...
	if (enable) {
		ret = enable_eud(chip);
		if (!ret)
			chip->enabled = enable;
		else
			disable_eud(chip);

	} else {
		ret = disable_eud(chip);
	}
    ...
}

Because the disable branch never clears chip->enabled, subsequent reads of
the enable sysfs attribute still report 1 after the EUD has been disabled.

[1] https://sashiko.dev/#/bug/linux-99d39974-0e5d-4e78-9df1-4c5cd22f0990

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-qcm2290-eud-v1-0-b6157b817377@oss.qualcomm.com?part=2