linux-kernel - Re: [PATCH 4/6] phy: qcom-qmp-combo: fix broken power on

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e4a423c6-e92d-1c40-2609-e8512bd9c03c@linaro.org>
Date:   Sat, 12 Nov 2022 09:15:43 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 4/6] phy: qcom-qmp-combo: fix broken power on

On 11/11/2022 11:42, Johan Hovold wrote:
> The PHY is powered on during phy-init by setting the SW_PRWDN bit in the

Nit: SW_PWRDN

> COM_POWER_DOWN_CTRL register and then setting the same bit in the in the
> PCS_POWER_DOWN_CONTROL register that belongs to the USB part of the
> PHY.
> 
> Currently, whether power on succeeds depends on probe order and having
> the USB part of the PHY be initialised first. In case the DP part of the
> PHY is instead initialised first, the intended power on of the USB block
> results in a corrupted DP_PHY register (e.g. DP_PHY_AUX_CFG8).
> 
> Add a pointer to the USB part of the PHY to the driver data and use that
> to power on the PHY also if the DP part of the PHY is initialised first.
> 
> Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
> Cc: stable@...r.kernel.org	# 5.10
> Signed-off-by: Johan Hovold <johan+linaro@...nel.org>

I can only hope that at some point in your cleanup this hack is going to 
be removed.
Nevertheless, I don't see a good way to do this at this moment. Thus:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 40c25a0ead23..17707f68d482 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -932,6 +932,7 @@ struct qcom_qmp {
>   	struct regulator_bulk_data *vregs;
>   
>   	struct qmp_phy **phys;
> +	struct qmp_phy *usb_phy;
>   
>   	struct mutex phy_mutex;
>   	int init_count;
> @@ -1911,7 +1912,7 @@ static int qmp_combo_com_init(struct qmp_phy *qphy)
>   {
>   	struct qcom_qmp *qmp = qphy->qmp;
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
> -	void __iomem *pcs = qphy->pcs;
> +	struct qmp_phy *usb_phy = qmp->usb_phy;
>   	void __iomem *dp_com = qmp->dp_com;
>   	int ret;
>   
> @@ -1963,7 +1964,8 @@ static int qmp_combo_com_init(struct qmp_phy *qphy)
>   	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
>   	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>   
> -	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> +	qphy_setbits(usb_phy->pcs, usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +			SW_PWRDN);
>   
>   	mutex_unlock(&qmp->phy_mutex);
>   
> @@ -2831,6 +2833,8 @@ static int qmp_combo_probe(struct platform_device *pdev)
>   				goto err_node_put;
>   			}
>   
> +			qmp->usb_phy = qmp->phys[id];
> +
>   			/*
>   			 * Register the pipe clock provided by phy.
>   			 * See function description to see details of this pipe clock.
> @@ -2846,6 +2850,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
>   		id++;
>   	}
>   
> +	if (!qmp->usb_phy)
> +		return -EINVAL;
> +
>   	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>   
>   	return PTR_ERR_OR_ZERO(phy_provider);

-- 
With best wishes
Dmitry