| Message ID | 20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration | expand |
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
On Sun, 7 Apr 2024 at 01:03, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > 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. The bridge-add just registers a device. Actual bridge is added by the aux-hpd-bridge device driver. I moved it to the end to prevent possible use-after-free issues like we had in the PMIC Glink case. 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. > > With those minor points addressed > > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > --- > bod
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
On Sun, 7 Apr 2024 at 22:32, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > 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. There is no actual race. In the worst case scenario, the TCPM will bring up the DP altmode and the altmode driver will send an OOB HPD event. However as this is an OOB event, the DRM subsystem correctly handles the case if there is no corresponding connector. > If you're happy you've reasoned about that and it won't happen, then > apply the ACK with the commit log fixed alone. Thanks!
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:
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,