Re: [PATCH v2 02/14] phy: qcom: Introduce Qualcomm IPQ5332 Super-Speed USB UNIPHY driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `platform_set_drvdata()` in the probe function causes a NULL pointer dereference in the remove callback.
- [Medium] Probe failure paths fail to restore the TCSR multiplexer to its default hardware state.
- [Medium] The driver fails to `select MFD_SYSCON` in Kconfig, risking a silent probe failure.
--

--- Patch [2]: [PATCH v2 02/14] phy: qcom: Introduce Qualcomm IPQ5332 Super-Speed USB UNIPHY driver ---
commit 2ff7b4ffaf9a300ab45c516bb3b192d9f0a5c88e
Author: Praveenkumar I <[email protected]>

phy: qcom: Introduce Qualcomm IPQ5332 Super-Speed USB UNIPHY driver

Adds Qualcomm 22ull Super-Speed USB UNIPHY driver support which is
present in Qualcomm IPQ5018 & IPQ5332 SoCs.

This PHY is interfaced with SNPS DWC3 USB and SNPS DWC PCIe. Either one
of the interface can use the it and selection is done via mux present in
TCSR register. By default, the PHY is configured for PCIe interface and
needs this driver to configure the mux for USB.

> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index 60a0ead127fa9..1ceb04afe1e3a 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -158,6 +158,16 @@ config PHY_QCOM_UNIPHY_PCIE_28LP
>  	  handles PHY initialization, clock management required after
>  	  resetting the hardware and power management.
>  
> +config PHY_QCOM_UNIPHY_USB_22ULL
> +	tristate "Qualcomm IPQ5332 UNIPHY USB Super-Speed 22ull driver"
> +	depends on USB && (ARCH_QCOM || COMPILE_TEST)
> +	select GENERIC_PHY

[Severity: Medium]
Since this driver uses syscon_regmap_lookup_by_phandle_args(), should it
select MFD_SYSCON to ensure the required syscon utilities are compiled in
and avoid silent probe failures?

> +	help
> +	  Enable this to support the Qualcomm USB Super-Speed UNIPHY transceiver

[ ... ]

> diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-usb-ss-22ull.c b/drivers/phy/qualcomm/phy-qcom-uniphy-usb-ss-22ull.c
> new file mode 100644
> index 0000000000000..089de7cebef03
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-uniphy-usb-ss-22ull.c

[ ... ]

> +static int qcom_uniphy_usb_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	/*
> +	 * Select the USB position of the phy mux before anything comes out
> +	 * of reset. Selecting it later leaves the SuperSpeed pads muxed away
> +	 * while the controller resets, and the SS link doesn't train and is
> +	 * stuck in Rx.Detect (USB2 is unaffected as it isn't muxed).
> +	 */
> +	ret = qcom_uniphy_usb_mux_select(uniphy, true);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to select USB mux\n");
> +
> +	uniphy->phy = devm_phy_create(dev, NULL, &uniphy_usb_ops);
> +	if (IS_ERR(uniphy->phy)) {
> +		ret = PTR_ERR(uniphy->phy);
> +		return dev_err_probe(dev, ret, "failed to create PHY\n");
> +	}

[Severity: Medium]
If devm_phy_create() or phy_pipe_clk_register() fail after the mux has been
selected, the function returns directly without restoring the hardware state.
Should there be a cleanup action (perhaps via devm_add_action_or_reset())
to restore the multiplexer to its default PCIe state on probe failure?

> +
> +	ret = phy_pipe_clk_register(uniphy, uniphy->phy->id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register pipe clk\n");
> +
> +	phy_set_drvdata(uniphy->phy, uniphy);

[Severity: High]
Does this function also need to call platform_set_drvdata(pdev, uniphy)?
Currently qcom_uniphy_usb_remove() relies on platform_get_drvdata(pdev),
which would return NULL if not set here, and cause a NULL pointer dereference
when passed to qcom_uniphy_usb_mux_select().

> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	ret = PTR_ERR_OR_ZERO(phy_provider);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy