linux-kernel - Re: [PATCH v4 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

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: <47f5ab83-d25f-4132-83a6-38236575510c@quicinc.com>
Date: Wed, 26 Feb 2025 11:12:18 +0800
From: Qiang Yu <quic_qianyu@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        "Wenbin Yao
 (Consultant)" <quic_wenbyao@...cinc.com>,
        Konrad Dybcio
	<konrad.dybcio@....qualcomm.com>, <vkoul@...nel.org>,
        <kishon@...nel.org>, <p.zabel@...gutronix.de>, <abel.vesa@...aro.org>,
        <neil.armstrong@...aro.org>, <quic_devipriy@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] phy: qcom: qmp-pcie: Add PHY register retention
 support


On 2/25/2025 7:46 PM, Dmitry Baryshkov wrote:
> On Tue, Feb 25, 2025 at 06:08:03PM +0800, Qiang Yu wrote:
>> On 2/25/2025 4:17 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Feb 25, 2025 at 04:06:16PM +0800, Wenbin Yao (Consultant) wrote:
>>>> On 2/24/2025 8:24 PM, Manivannan Sadhasivam wrote:
>>>>> On Mon, Feb 24, 2025 at 12:46:44PM +0100, Konrad Dybcio wrote:
>>>>>> On 24.02.2025 9:46 AM, Wenbin Yao (Consultant) wrote:
>>>>>>> On 2/24/2025 3:33 PM, Manivannan Sadhasivam wrote:
>>>>>>>> On Thu, Feb 20, 2025 at 06:22:53PM +0800, Wenbin Yao wrote:
>>>>>>>>> From: Qiang Yu <quic_qianyu@...cinc.com>
>>>>>>>>>
>>>>>>>>> Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the
>>>>>>>>> whole PHY (hardware and register), no_csr reset only resets PHY hardware
>>>>>>>>> but retains register values, which means PHY setting can be skipped during
>>>>>>>>> PHY init if PCIe link is enabled in booltloader and only no_csr is toggled
>>>>>>>>> after that.
>>>>>>>>>
>>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>>> verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is
>>>>>>>>> available, skip BCR reset and PHY register setting to establish the PCIe
>>>>>>>>> link with bootloader - programmed PHY settings.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Qiang Yu <quic_qianyu@...cinc.com>
>>>>>>>>> Signed-off-by: Wenbin Yao <quic_wenbyao@...cinc.com>
>>>>>>>> Some nitpicks below.
>>>>>>>>
>>>>>>>>> ---
>>>>>> [...]
>>>>>>
>>>>>>>>> +     * In this way, no matter whether the PHY settings were initially
>>>>>>>>> +     * programmed by bootloader or PHY driver itself, we can reuse them
>>>>>>>> It is really possible to have bootloader not programming the init sequence for
>>>>>>>> no_csr reset platforms? The comment sounds like it is possible. But I heard the
>>>>>>>> opposite.
>>>>>>> PCIe3 on X1E80100 QCP is disabled by default in UEFI. We need to enable it
>>>>>>> manually in UEFI shell if we want.
>>>>>> IIUC this will not be a concern going forward, and this is a special case
>>>>>>
>>>>> I'm wondering how many *special* cases we may have to deal with going forward.
>>>>> Anyhow, I would propose to atleast throw an error and fail probe() if:
>>>>>
>>>>> * the platform has no_csr reset AND
>>>>> * bootloader has not initialized the PHY AND
>>>>> * there are no init sequences in the kernel
>>>>>
>>>>> - Mani
>>>> Hmmm, regardless of whether it's a special case, we can't assume that UEFI
>>>> will enable the PHY supporting no_csr reset on all platforms. It's a bit
>>>> risky. If we make such an assumption, we also won't need to check whether
>>>> the PHY is enabled by UEFI during powering on. We just need to check
>>>> whether no_csr reset is available.
>>>>
>>> I am not supportive of this assumption to be clear. While I am OK with relying
>>> on no_csr reset and bootloader programming the PHY, we should also make sure to
>>> catch if the PHY doesn't initialize it. Otherwise, the driver would assume that
>>> the PHY is working, but the users won't see any PCIe devices.
>>>
>>>> But it makes sense to check the exsitence of PHY senquence. How about
>>>> adding the check in qmp_pcie_init, if a PHY supports no_csr reset and isn't
>>>> initialized in UEFI and there is no cfg->tbls, return error and print some
>>>> error log so that the PCIe controller will fail to probe.
>>>>
>>> Sounds good to me.
>> I'm wondering is it necessary to add this check? In current PHY driver,
>> for PHY that doesn't suppot no_csr reset there is also no such check.
>> If a PHY supports no_csr reset and isn't init in UEFI and there is no
>> cfg->tbls, the worst issue is link training fail and PCIe controller will
>> also fail to probe. Adding sucj check seems not change the result.
> Failing the training is a random error which can mean a lot, e.g. the
> missing voltage rail. An explicit check is beneficial, it helps
> developers (and users) to better understand the reason of the failure.
In the struct qmp_phy_cfg, tbls is not a pointer, we can't directly check
if tbls == NULL to determine if the PHY init sequence is available. Can we
add a separate patch to change the definition from
"const struct qmp_phy_cfg_tbls tbls" to
"const struct qmp_phy_cfg_tbls *tbls" like tlbs_rc and tbls_ep, even though
this will affect all platforms?