Commit [1] introduced DP support to QMP driver. While doing so, the dp and usb configuration structures were added to a combo_phy_cfg structure. During probe, the match data is used to parse and identify the dp and usb configs separately. While doing so, the usb_cfg variable represents the configuration parameters for USB part of the phy (whether it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg for Uni PHYs is left incomplete and it is left as NULL. This NULL variable further percolates down to qmp_phy_create() call essentially getting de-referenced and causing a crash.
Subsequently, commit [2] split the driver into multiple files, each handling a specific PHY type (USB, DP-Combo, UFS, PCIe). During this refactoring, the probing process was modified, and the NULL pointer dereference issue no longer showed up.
[1]: https://lore.kernel.org/all/20200916231202.3637932-8-swboyd@chromium.org/ [2]: https://lore.kernel.org/all/20220607213203.2819885-1-dmitry.baryshkov@linaro...
Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy") Cc: stable@vger.kernel.org # 5.15.y Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com --- drivers/phy/qualcomm/phy-qcom-qmp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index eef863108bfe..e22ee71aa060 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -5714,6 +5714,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
usb_cfg = combo_cfg->usb_cfg; cfg = usb_cfg; /* Setup clks and regulators */ + } else { + usb_cfg = cfg; }
/* per PHY serdes; usually located at base address */
On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
Commit [1] introduced DP support to QMP driver. While doing so, the dp and usb configuration structures were added to a combo_phy_cfg structure. During probe, the match data is used to parse and identify the dp and usb configs separately. While doing so, the usb_cfg variable represents the configuration parameters for USB part of the phy (whether it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg for Uni PHYs is left incomplete and it is left as NULL. This NULL variable further percolates down to qmp_phy_create() call essentially getting de-referenced and causing a crash.
The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg variable should not be used in the for_each_available_child_of_node() loop.
Please provide details for the platform on which you observe the crash and the backtrace.
Subsequently, commit [2] split the driver into multiple files, each handling a specific PHY type (USB, DP-Combo, UFS, PCIe). During this refactoring, the probing process was modified, and the NULL pointer dereference issue no longer showed up.
Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy") Cc: stable@vger.kernel.org # 5.15.y Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com
drivers/phy/qualcomm/phy-qcom-qmp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index eef863108bfe..e22ee71aa060 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -5714,6 +5714,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) usb_cfg = combo_cfg->usb_cfg; cfg = usb_cfg; /* Setup clks and regulators */
- } else {
}usb_cfg = cfg;
/* per PHY serdes; usually located at base address */ -- 2.34.1
On 11/15/2024 9:29 PM, Dmitry Baryshkov wrote:
On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
Commit [1] introduced DP support to QMP driver. While doing so, the dp and usb configuration structures were added to a combo_phy_cfg structure. During probe, the match data is used to parse and identify the dp and usb configs separately. While doing so, the usb_cfg variable represents the configuration parameters for USB part of the phy (whether it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg for Uni PHYs is left incomplete and it is left as NULL. This NULL variable further percolates down to qmp_phy_create() call essentially getting de-referenced and causing a crash.
The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg variable should not be used in the for_each_available_child_of_node() loop.
Please provide details for the platform on which you observe the crash and the backtrace.
I got this error when I started working on multiport support (begining of 2023). Initially I tried testing on my code on 5.15, hence this patch was raised on the same.
The 2 qmp phys on sa8195 and sa8295 (based on sc8280xp) are uni phy and the following was the DT node that worked out for me on 5.15 codebase:
usb_1_qmpphy: ssphy@88eb000 { compatible = "qcom,sm8150-qmp-usb3-uni-phy"; reg = <0x088eb000 0x200>; #address-cells = <1>; #size-cells = <1>; ranges; //status = "disabled"; clocks = <&gcc GCC_USB3_MP_PHY_AUX_CLK>, <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_USB3_SEC_CLKREF_CLK>, <&gcc GCC_USB3_MP_PHY_COM_AUX_CLK>; clock-names = "aux", "ref_clk_src", "ref", "com_aux";
resets = <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>, <&gcc GCC_USB3_UNIPHY_MP0_BCR>; reset-names = "phy", "common";
//vdda-phy-supply = <&L3C>; vdda-pll-supply = <&L5E>;
usb_1_ssphy: usb3-phy@88eb200 { reg = <0x088eb200 0x200>, <0x088eb400 0x200>, <0x088eb800 0x800>, <0x088eb600 0x200>; #clock-cells = <0>; #phy-cells = <0>; clocks = <&gcc GCC_USB3_MP_PHY_PIPE_0_CLK>; clock-names = "pipe0"; clock-output-names = "usb3_uni_phy_pipe_clk_src"; }; };
I was hitting the bug when I write the DT above way on top of 5.15 baseline.
In 5.15.y, the SM8150 usb_2_qmpphy dT is as follows:
usb_2_qmpphy: phy@88eb000 { compatible = "qcom,sm8150-qmp-usb3-uni-phy"; reg = <0 0x088eb000 0 0x200>; status = "disabled"; #address-cells = <2>; #size-cells = <2>; ranges;
clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>, <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_USB3_SEC_CLKREF_CLK>, <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>; clock-names = "aux", "ref_clk_src", "ref", "com_aux";
resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>, <&gcc GCC_USB3_PHY_SEC_BCR>; reset-names = "phy", "common";
usb_2_ssphy: phy@88eb200 { reg = <0 0x088eb200 0 0x200>, <0 0x088eb400 0 0x200>, <0 0x088eb800 0 0x800>, <0 0x088eb600 0 0x200>; #clock-cells = <0>; #phy-cells = <0>; clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>; clock-names = "pipe0"; clock-output-names = "usb3_uni_phy_pipe_clk_src"; }; };
IIRC, when I tried using the above sm8150 dt on 5.15.y, the phy_create was (either not getting called) or crashing. Probably because "of_node_name_eq()" didn't find either "dp-phy" or "usb3-phy" and cfg variable was NULL.
I can try reproducing the issue and get back again in a week.
Apologies if I have misunderstood something and this patch doesn't make sense. Let me know if I have made any mistake anywhere (either in my DT) or in understanding.
Regards, Krishna,
On Sun, 17 Nov 2024 at 13:51, Krishna Kurapati quic_kriskura@quicinc.com wrote:
On 11/15/2024 9:29 PM, Dmitry Baryshkov wrote:
On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
Commit [1] introduced DP support to QMP driver. While doing so, the dp and usb configuration structures were added to a combo_phy_cfg structure. During probe, the match data is used to parse and identify the dp and usb configs separately. While doing so, the usb_cfg variable represents the configuration parameters for USB part of the phy (whether it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg for Uni PHYs is left incomplete and it is left as NULL. This NULL variable further percolates down to qmp_phy_create() call essentially getting de-referenced and causing a crash.
The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg variable should not be used in the for_each_available_child_of_node() loop.
Please provide details for the platform on which you observe the crash and the backtrace.
I got this error when I started working on multiport support (begining of 2023). Initially I tried testing on my code on 5.15, hence this patch was raised on the same.
The 2 qmp phys on sa8195 and sa8295 (based on sc8280xp) are uni phy and the following was the DT node that worked out for me on 5.15 codebase:
usb_1_qmpphy: ssphy@88eb000 { compatible = "qcom,sm8150-qmp-usb3-uni-phy"; reg = <0x088eb000 0x200>; #address-cells = <1>; #size-cells = <1>; ranges; //status = "disabled"; clocks = <&gcc GCC_USB3_MP_PHY_AUX_CLK>, <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_USB3_SEC_CLKREF_CLK>, <&gcc GCC_USB3_MP_PHY_COM_AUX_CLK>; clock-names = "aux", "ref_clk_src", "ref", "com_aux"; resets = <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>, <&gcc GCC_USB3_UNIPHY_MP0_BCR>; reset-names = "phy", "common"; //vdda-phy-supply = <&L3C>; vdda-pll-supply = <&L5E>; usb_1_ssphy: usb3-phy@88eb200 {
As this is a UNI PHY and not a combo PHY, the child node should be just phy@, not usb3-phy@. See Documentation/devicetree/bindings/phy/qcom,qmp-phy.yaml
reg = <0x088eb200 0x200>, <0x088eb400 0x200>, <0x088eb800 0x800>, <0x088eb600 0x200>; #clock-cells = <0>; #phy-cells = <0>; clocks = <&gcc GCC_USB3_MP_PHY_PIPE_0_CLK>; clock-names = "pipe0"; clock-output-names = "usb3_uni_phy_pipe_clk_src"; }; };
I was hitting the bug when I write the DT above way on top of 5.15 baseline.
In 5.15.y, the SM8150 usb_2_qmpphy dT is as follows:
usb_2_qmpphy: phy@88eb000 { compatible = "qcom,sm8150-qmp-usb3-uni-phy"; reg = <0 0x088eb000 0 0x200>; status = "disabled"; #address-cells = <2>; #size-cells = <2>; ranges; clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>, <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_USB3_SEC_CLKREF_CLK>, <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>; clock-names = "aux", "ref_clk_src", "ref",
"com_aux";
resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>, <&gcc GCC_USB3_PHY_SEC_BCR>; reset-names = "phy", "common"; usb_2_ssphy: phy@88eb200 {
Just as I wrote, this one correctly uses phy@
reg = <0 0x088eb200 0 0x200>, <0 0x088eb400 0 0x200>, <0 0x088eb800 0 0x800>, <0 0x088eb600 0 0x200>; #clock-cells = <0>; #phy-cells = <0>; clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>; clock-names = "pipe0"; clock-output-names =
"usb3_uni_phy_pipe_clk_src"; }; };
IIRC, when I tried using the above sm8150 dt on 5.15.y, the phy_create was (either not getting called) or crashing. Probably because "of_node_name_eq()" didn't find either "dp-phy" or "usb3-phy" and cfg variable was NULL.
Unless somebody backported some patch in an incorrect way, the SM8150 DT entry is correct, while SA8xxx is not,
I can try reproducing the issue and get back again in a week.
Yes, please.
Apologies if I have misunderstood something and this patch doesn't make sense. Let me know if I have made any mistake anywhere (either in my DT) or in understanding.
Regards, Krishna,
linux-stable-mirror@lists.linaro.org