Re: [PATCH v3 4/4] phy: qualcomm: qmp-combo: Add support for Hawi SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing DP PHY function pointers in `hawi_usb3dpphy_cfg` lead to NULL pointer dereferences when the DP PHY is initialized.
- [High] Valid register offset `0x00` causes the AON toggle hardware reset override to be silently skipped.
--

commit 39c99ec3129f1da7348c282db12eb565e967941d
Author: Ronak Raheja <ronak.raheja@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Add support for Hawi SoC

This commit adds support for the USB3-DP combo PHY found on the Hawi
platform, introducing new V10 register layouts and the COM AON register
module.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 93f1aa10d4008..53b709ea93d52 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -2705,6 +2895,27 @@ static const struct qmp_phy_cfg x1e80100_usb3dpphy_cfg = {
>  	.regs			= qmp_v6_n4_usb3phy_regs_layout,
>  };
>  
> +static const struct qmp_phy_cfg hawi_usb3dpphy_cfg = {
> +	.offsets		= &qmp_combo_offsets_v10,
> +
> +	.serdes_tbl		= hawi_usb3_serdes_tbl,
> +	.serdes_tbl_num		= ARRAY_SIZE(hawi_usb3_serdes_tbl),
> +	.tx_tbl			= hawi_usb3_tx_tbl,
> +	.tx_tbl_num		= ARRAY_SIZE(hawi_usb3_tx_tbl),
> +	.rx_tbl			= hawi_usb3_rx_tbl,
> +	.rx_tbl_num		= ARRAY_SIZE(hawi_usb3_rx_tbl),
> +	.pcs_tbl		= hawi_usb3_pcs_tbl,
> +	.pcs_tbl_num		= ARRAY_SIZE(hawi_usb3_pcs_tbl),
> +	.pcs_usb_tbl		= hawi_usb3_pcs_usb_tbl,
> +	.pcs_usb_tbl_num	= ARRAY_SIZE(hawi_usb3_pcs_usb_tbl),
> +
> +	.regs			= qmp_v10_usb3phy_regs_layout,
> +	.reset_list		= msm8996_usb3phy_reset_l,
> +	.num_resets		= ARRAY_SIZE(msm8996_usb3phy_reset_l),
> +	.vreg_list		= qmp_phy_vreg_l,
> +	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> +};

Does this configuration miss the DP PHY function pointers? 

Because this is a USB3-DP combo PHY, the driver unconditionally creates the DP
PHY. When a DisplayPort controller tries to initialize it, functions like
qmp_combo_dp_init() and qmp_combo_dp_power_on() will unconditionally call
cfg->dp_aux_init(qmp) and cfg->configure_dp_tx(qmp). 

Without these pointers defined in hawi_usb3dpphy_cfg, will this lead to a NULL
pointer dereference and a kernel panic upon DP PHY initialization?

[ ... ]
> @@ -3699,9 +3915,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  			SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>  
>  	/* override hardware control for reset of qmp phy */
> -	if (pcs_aon && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
> -		qphy_clrbits(pcs_aon, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
> -		qphy_clrbits(pcs_aon, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
> +	if (aon_toggle && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
> +		qphy_clrbits(aon_toggle, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
> +		qphy_clrbits(aon_toggle, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
>  	}

Is it possible for the hardware reset override to be silently skipped here?

The V10 register layout maps QPHY_AON_TOGGLE_ENABLE to
QPHY_V10_COM_AON_USB3_AON_TOGGLE_ENABLE, which is defined as 0x00 in
phy-qcom-qmp-com-aon-v10.h.

Because 0x00 is a valid register offset but evaluates to false in C, the
check for cfg->regs[QPHY_AON_TOGGLE_ENABLE] will fail. This would skip the
qphy_clrbits() calls entirely, bypassing the necessary hardware reset override
sequence and leaving the PHY in a broken state. 

Could we use a different way to check if this register is supported by the
layout without relying on the truthiness of the offset value?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508213234.4643-1-ronak.raheja@oss.qualcomm.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy