linux-kernel - Re: [RFC PATCH 1/3] phy: don't let controllers change vbus reg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2261426.ZfL8zNpBrT@jernej-laptop>
Date: Thu, 20 Feb 2025 21:27:27 +0100
From: Jernej Škrabec <jernej.skrabec@...il.com>
To: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Chen-Yu Tsai <wens@...e.org>, Samuel Holland <samuel@...lland.org>,
 Philippe Simons <simons.philippe@...il.com>,
 "open list:GENERIC PHY FRAMEWORK" <linux-phy@...ts.infradead.org>,
 "moderated list:ARM/Allwinner sunXi SoC support"
 <linux-arm-kernel@...ts.infradead.org>,
 "open list:ARM/Allwinner sunXi SoC support" <linux-sunxi@...ts.linux.dev>,
 open list <linux-kernel@...r.kernel.org>,
 Philippe Simons <simons.philippe@...il.com>
Subject: Re: [RFC PATCH 1/3] phy: don't let controllers change vbus reg

Dne sobota, 18. januar 2025 ob 11:22:04 Srednjeevropski standardni čas je Philippe Simons napisal(a):
> Allwinners SoCs share phy0 between the MUSB controller and HCI controller.
> If we let these controllers independently power on the vbus on that phy,
> peripheral mode is dangerous because HCI never power down the phy, resulting
> in 5v being applied against the host 5v.
> 
> Override power_on/off for phy0 in that case, and let regulator be enabled/disabled
> based on id_det only.
> 
> Signed-off-by: Philippe Simons <simons.philippe@...il.com>

Is this fix for some observed issue? If so, Fixes tag would be appropriate.

> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index b0f19e950..24fbabe7a 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -465,6 +465,10 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
>  	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
>  	int ret;
>  
> +	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
> +	if (phy->index == 0 && data->cfg->phy0_dual_route)
> +		return 0;
> +
>  	if (!phy->vbus || phy->regulator_on)
>  		return 0;
>  
> @@ -493,6 +497,10 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
>  	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>  	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
>  
> +	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
> +	if (phy->index == 0 && data->cfg->phy0_dual_route)
> +		return 0;
> +
>  	if (!phy->vbus || !phy->regulator_on)
>  		return 0;
>  
> @@ -573,9 +581,21 @@ static void sun4i_usb_phy0_reroute(struct sun4i_usb_phy_data *data, int id_det)
>  	if (id_det == 0) {
>  		/* Host mode. Route phy0 to EHCI/OHCI */
>  		regval &= ~OTGCTL_ROUTE_MUSB;
> +
> +		/* Enable VBUS reg */
> +		if (phy->vbus && !phy->regulator_on) {

I don't see any "phy" variable in this function. Also, you should add same
protection as it is implemented in sun4i_usb_phy_power_on(). Skip enabling
regulator if:

sun4i_usb_phy0_have_vbus_det(data) && data->vbus_det

> +			regulator_enable(phy->vbus);
> +			phy->regulator_on = true;
> +		}
>  	} else {
>  		/* Peripheral mode. Route phy0 to MUSB */
>  		regval |= OTGCTL_ROUTE_MUSB;
> +		
> +		/* Disable VBUS reg */
> +		if (phy->vbus && phy->regulator_on) {
> +			regulator_disable(phy->vbus);
> +			phy->regulator_on = false;
> +		}
>  	}
>  	writel(regval, data->base + REG_PHY_OTGCTL);
>  }
> @@ -603,6 +623,12 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
>  		return;
>  	}
>  
> +	/* Disable internal VBUS reg if there is an external VBUS */
> +	if (vbus_det != 0 && phy->vbus && phy->regulator_on) {

Ideally, this should never happen.

Best regards,
Jernej

> +		regulator_disable(phy->vbus);
> +		phy->regulator_on = false;
> +	}
> +
>  	force_session_end = data->force_session_end;
>  	data->force_session_end = false;
>  
>