Re: [PATCH 3/3] phy: apple: atc: Implement the USB4 pipehandler state

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

New issues:
- [High] Missing cleanup of `PIPEHANDLER_OVERRIDE` registers on lock failure leaves link detection permanently disabled.

Pre-existing issues:
- [High] Unconditional state update of `atcphy->pipehandler_up = true` on the error path masks hardware configuration failures.
--

--- Patch [3]: [PATCH 3/3] phy: apple: atc: Implement the USB4 pipehandler state ---
commit 6b90c04f8cd4e7a3f818a4bc0eb1057996bb1845
Author: Sven Peter <[email protected]>

phy: apple: atc: Implement the USB4 pipehandler state

This commit implements the USB4 pipehandler state for the Apple ATC PHY driver.
It switches the dwc3's PIPE interface to the USB4 NHI which takes care of
tunneling USB3 over USB4.

> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index 96158d3486560..613bcd0ae8865 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c
> @@ -1084,6 +1084,41 @@ static int atcphy_configure_pipehandler_usb3(struct apple_atcphy *atcphy, bool h
>  	return 0;
>  }
>  
> +static int atcphy_configure_pipehandler_usb4(struct apple_atcphy *atcphy)
> +{
> +	int ret;
> +
> +	ret = atcphy_pipehandler_check(atcphy);
> +	if (ret)
> +		return ret;
> +
> +	/* Force disable link detection */
> +	clear32(atcphy->regs.pipehandler + PIPEHANDLER_OVERRIDE_VALUES,
> +		PIPEHANDLER_OVERRIDE_VAL_RXDETECT0 | PIPEHANDLER_OVERRIDE_VAL_RXDETECT1);
> +	set32(atcphy->regs.pipehandler + PIPEHANDLER_OVERRIDE, PIPEHANDLER_OVERRIDE_RXVALID);
> +	set32(atcphy->regs.pipehandler + PIPEHANDLER_OVERRIDE, PIPEHANDLER_OVERRIDE_RXDETECT);
> +
> +	ret = atcphy_pipehandler_lock(atcphy);
> +	if (ret) {
> +		dev_err(atcphy->dev, "Failed to lock pipehandler\n");
> +		return ret;
> +	}

[Severity: High]
If atcphy_pipehandler_lock() fails and returns early, the link detection
override bits (PIPEHANDLER_OVERRIDE_RXVALID and
PIPEHANDLER_OVERRIDE_RXDETECT) are left set.

Does this permanently disable link detection on the PHY hardware for
subsequent retries until a hard reset occurs?

[ ... ]

> @@ -1130,10 +1165,8 @@ static int atcphy_configure_pipehandler(struct apple_atcphy *atcphy, bool host)
>  		atcphy->pipehandler_up = true;
>  		break;
>  	case ATCPHY_PIPEHANDLER_STATE_USB4:
> -		dev_warn(atcphy->dev,
> -			 "ATCPHY_PIPEHANDLER_STATE_USB4 not implemented; falling back to USB2\n");
> -		ret = atcphy_configure_pipehandler_dummy(atcphy);
> -		atcphy->pipehandler_up = false;
> +		ret = atcphy_configure_pipehandler_usb4(atcphy);
> +		atcphy->pipehandler_up = true;

[Severity: High]
This is a pre-existing issue, but if atcphy_configure_pipehandler_usb4()
returns an error, the software state atcphy->pipehandler_up is still
unconditionally set to true.

When a subsequent caller (such as the generic xhci code) invokes
atcphy_usb3_set_mode(), it checks atcphy->pipehandler_up, assumes the PHY
is successfully configured, and returns 0 without actually doing anything.

Could this state desynchronization mask hardware configuration failures and
prevent subsequent valid retries from successfully initializing the PHY?

>  		break;
>  	case ATCPHY_PIPEHANDLER_STATE_DUMMY:
>  		ret = atcphy_configure_pipehandler_dummy(atcphy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy