Re: [PATCH 1/3] phy: apple: atc: Support DUMMY PIPEHANDLER state in configure_pipehandler

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

Pre-existing issues:
- [High] `atcphy->pipehandler_up` is unconditionally set to true even if `atcphy_configure_pipehandler_usb3()` fails, masking errors on subsequent configuration attempts.
--

--- Patch [1]: [PATCH 1/3] phy: apple: atc: Support DUMMY PIPEHANDLER state in configure_pipehandler ---
commit 969f2e11a2c1dceb64c1ff0f1f1da36afa041518
Author: Sven Peter <[email protected]>

phy: apple: atc: Support DUMMY PIPEHANDLER state in configure_pipehandler

For both Thunderbolt and DisplayPort atcphy_configure_pipehandler is
reached with a request to switch to the DUMMY state (i.e. usb2 only).
This actually handles that case correctly and drops the default from
the switch to enable compiler warnings for missing pipehandler states.

Closes: https://github.com/AsahiLinux/linux/pull/515
Fixes: 8e98ca1e74db ("phy: apple: Add Apple Type-C PHY")

> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index 4156fabad742c..a3162f25e6cd6 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c
> @@ -1121,7 +1121,7 @@ static int atcphy_configure_pipehandler_dummy(struct apple_atcphy *atcphy)
>  
>  static int atcphy_configure_pipehandler(struct apple_atcphy *atcphy, bool host)
>  {
> -	int ret;
> +	int ret = -EINVAL;
>  
>  	lockdep_assert_held(&atcphy->lock);
>  

[Severity: High]
This isn't a bug introduced by this patch, but looking at the USB3 state
handling in atcphy_configure_pipehandler() just below this:

drivers/phy/apple/atc.c:atcphy_configure_pipehandler() {
    ...
	case ATCPHY_PIPEHANDLER_STATE_USB3:
		ret = atcphy_configure_pipehandler_usb3(atcphy, host);
		atcphy->pipehandler_up = true;
		break;
    ...
}

Is it safe to unconditionally set atcphy->pipehandler_up to true even if
atcphy_configure_pipehandler_usb3() fails?

If the USB3 PHY initialization fails (for example, due to a lock timeout or
BIST failure), the PHY is not correctly configured but the state is still
marked as up.

Could this cause subsequent mode setting attempts (like from the generic
xHCI code) to falsely return success without re-attempting configuration,
leaving the USB port permanently broken?

[ ... ]

> @@ -1136,8 +1136,10 @@ static int atcphy_configure_pipehandler(struct apple_atcphy *atcphy, bool host)
>  		ret = atcphy_configure_pipehandler_dummy(atcphy);
>  		atcphy->pipehandler_up = false;
>  		break;
> -	default:
> -		ret = -EINVAL;
> +	case ATCPHY_PIPEHANDLER_STATE_DUMMY:
> +		ret = atcphy_configure_pipehandler_dummy(atcphy);
> +		atcphy->pipehandler_up = false;
> +		break;
>  	}
>  
>  	return ret;

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

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