diff mbox series

usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration

Message ID 20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org
State Superseded
Headers show
Series usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration | expand

Commit Message

Dmitry Baryshkov April 5, 2024, 6:11 p.m. UTC
If a probe function returns -EPROBE_DEFER after creating another device
there is a change of ening up in a probe deferral loop, (see commit
fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER").

In order to prevent such probe-defer loops caused by qcom-pmic-typec
driver, use the API added by Johan Hoval and move HPD bridge
registeration to the end of the probe function.

Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


---
base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
change-id: 20240405-qc-pmic-typec-hpd-split-22804201902b

Best regards,

Comments

Bryan O'Donoghue April 6, 2024, 10:03 p.m. UTC | #1
On 05/04/2024 19:11, Dmitry Baryshkov wrote:
> If a probe function returns -EPROBE_DEFER after creating another device
> there is a change of ening up in a probe deferral loop, (see commit

*ending

> fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER").
> 
> In order to prevent such probe-defer loops caused by qcom-pmic-typec
> driver, use the API added by Johan Hoval and move HPD bridge

*Hovold

> registeration to the end of the probe function.

registration

> 
> Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> index e48412cdcb0f..96b41efae318 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> @@ -41,7 +41,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>   	struct device_node *np = dev->of_node;
>   	const struct pmic_typec_resources *res;
>   	struct regmap *regmap;
> -	struct device *bridge_dev;
> +	struct auxiliary_device *bridge_dev;
>   	u32 base;
>   	int ret;
>   
> @@ -92,7 +92,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>   	if (!tcpm->tcpc.fwnode)
>   		return -EINVAL;
>   
> -	bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode));
> +	bridge_dev = devm_drm_dp_hpd_bridge_alloc(tcpm->dev, to_of_node(tcpm->tcpc.fwnode));
>   	if (IS_ERR(bridge_dev))
>   		return PTR_ERR(bridge_dev);
>   
> @@ -110,6 +110,10 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto fwnode_remove;
>   
> +	ret = devm_drm_dp_hpd_bridge_add(tcpm->dev, bridge_dev);
> +	if (ret)
> +		goto fwnode_remove;
> +

Is there any reason this call comes after port_start/pdphy_start ?

I think the bridge add should go before starting the typec port and 
pdphy since after the start calls IRQs are enabled.

With those minor points addressed

Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod
Bryan O'Donoghue April 7, 2024, 7:32 p.m. UTC | #2
On 07/04/2024 16:08, Dmitry Baryshkov wrote:
> Basically, if for some reason (e.g. because the TCPM returns an error
> to one of the start functions) the drm_bridge is destroyed, the DRM
> driver isn't notified about the event. It still keeps the pointer to
> the bridge pointer and can access freed memory afterwards.

Hmm, my concern/question is about the TCPM code triggered by an IRQ 
firing here, racing with the bridge code.

If you're happy you've reasoned about that and it won't happen, then 
apply the ACK with the commit log fixed alone.

---
bod
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index e48412cdcb0f..96b41efae318 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -41,7 +41,7 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	const struct pmic_typec_resources *res;
 	struct regmap *regmap;
-	struct device *bridge_dev;
+	struct auxiliary_device *bridge_dev;
 	u32 base;
 	int ret;
 
@@ -92,7 +92,7 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	if (!tcpm->tcpc.fwnode)
 		return -EINVAL;
 
-	bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode));
+	bridge_dev = devm_drm_dp_hpd_bridge_alloc(tcpm->dev, to_of_node(tcpm->tcpc.fwnode));
 	if (IS_ERR(bridge_dev))
 		return PTR_ERR(bridge_dev);
 
@@ -110,6 +110,10 @@  static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	if (ret)
 		goto fwnode_remove;
 
+	ret = devm_drm_dp_hpd_bridge_add(tcpm->dev, bridge_dev);
+	if (ret)
+		goto fwnode_remove;
+
 	return 0;
 
 fwnode_remove: