| Message ID | 20240424-qc-pmic-typec-hpd-split-v4-1-f7e10d147443@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration | expand |
On Wed, Apr 24, 2024 at 05:16:57AM +0300, Dmitry Baryshkov wrote: > If a probe function returns -EPROBE_DEFER after creating another device > there is a change of ending up in a probe deferral loop, (see commit > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > external resources (USB role switch and inherently via called > typec_register_port() USB-C muxes, switches and retimers). > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > driver, use the API added by Johan Hovold and move HPD bridge > registration to the end of the probe function. > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > function after all TCPM start functions. This is done as a way to > overcome a different problem, the DRM subsystem can not properly cope > with the DRM bridges being destroyed once the bridge is attached. Having > this function call at the end of the probe function prevents possible > DRM bridge device creation followed by destruction in case one of the > TCPM start functions returns an error. > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > --- > Changes in v4: > - Rebased on top of Johan's patches > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > Changes in v3: > - Updated commit message to explain my decisions (Johan). > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > Changes in v2: > - Fix commit message (Bryan) > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > --- > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > 1 file changed, 8 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 d3958c061a97..501eddb294e4 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,8 +110,14 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) > if (ret) > goto port_stop; > > + ret = devm_drm_dp_hpd_bridge_add(tcpm->dev, bridge_dev); > + if (ret) > + goto pdphy_stop; > + > return 0; > > +pdphy_stop: > + tcpm->pdphy_stop(tcpm); > port_stop: > tcpm->port_stop(tcpm); > port_unregister: > > --- > base-commit: 15d419fa23ecc51e388a369bbeaf3b47b0b5c76a > change-id: 20240405-qc-pmic-typec-hpd-split-22804201902b > > Best regards, > -- > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Wed, Apr 24, 2024 at 05:16:57AM +0300, Dmitry Baryshkov wrote: > If a probe function returns -EPROBE_DEFER after creating another device > there is a change of ending up in a probe deferral loop, (see commit > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > external resources (USB role switch and inherently via called > typec_register_port() USB-C muxes, switches and retimers). > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > driver, use the API added by Johan Hovold and move HPD bridge > registration to the end of the probe function. > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > function after all TCPM start functions. This is done as a way to > overcome a different problem, the DRM subsystem can not properly cope > with the DRM bridges being destroyed once the bridge is attached. Having > this function call at the end of the probe function prevents possible > DRM bridge device creation followed by destruction in case one of the > TCPM start functions returns an error. As I mentioned again off-list yesterday, I would have preferred to see an explanation why registering the bridge after starting the port and phy is ok as it's not obvious to someone not familiar with the driver. But given that bridge registration only registers an aux device which may not have been probed by the time the function returns I guess that implies that the old driver did not actually rely on it being registered before starting either. So I'm fine with this as-is. > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > --- > Changes in v4: > - Rebased on top of Johan's patches > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Johan
On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > If a probe function returns -EPROBE_DEFER after creating another device > there is a change of ending up in a probe deferral loop, (see commit > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > external resources (USB role switch and inherently via called > typec_register_port() USB-C muxes, switches and retimers). > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > driver, use the API added by Johan Hovold and move HPD bridge > registration to the end of the probe function. > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > function after all TCPM start functions. This is done as a way to > overcome a different problem, the DRM subsystem can not properly cope > with the DRM bridges being destroyed once the bridge is attached. Having > this function call at the end of the probe function prevents possible > DRM bridge device creation followed by destruction in case one of the > TCPM start functions returns an error. > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > --- > Changes in v4: > - Rebased on top of Johan's patches > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > Changes in v3: > - Updated commit message to explain my decisions (Johan). > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > Changes in v2: > - Fix commit message (Bryan) > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > --- > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) A stupid gracious ping. It would be nice to fix the issue in 6.10
On Sat, May 04, 2024 at 05:23:20PM +0300, Dmitry Baryshkov wrote: > On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > If a probe function returns -EPROBE_DEFER after creating another device > > there is a change of ending up in a probe deferral loop, (see commit > > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > > external resources (USB role switch and inherently via called > > typec_register_port() USB-C muxes, switches and retimers). > > > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > > driver, use the API added by Johan Hovold and move HPD bridge > > registration to the end of the probe function. > > > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > > function after all TCPM start functions. This is done as a way to > > overcome a different problem, the DRM subsystem can not properly cope > > with the DRM bridges being destroyed once the bridge is attached. Having > > this function call at the end of the probe function prevents possible > > DRM bridge device creation followed by destruction in case one of the > > TCPM start functions returns an error. > > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > > --- > > Changes in v4: > > - Rebased on top of Johan's patches > > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > > > Changes in v3: > > - Updated commit message to explain my decisions (Johan). > > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > > > Changes in v2: > > - Fix commit message (Bryan) > > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > > --- > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > A stupid gracious ping. It would be nice to fix the issue in 6.10 Is this a regression? If so, what commit does it fix? Or has it always just not worked? thanks, greg k-h
On Sat, May 04, 2024 at 05:15:45PM +0200, Greg Kroah-Hartman wrote: > On Sat, May 04, 2024 at 05:23:20PM +0300, Dmitry Baryshkov wrote: > > On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > If a probe function returns -EPROBE_DEFER after creating another device > > > there is a change of ending up in a probe deferral loop, (see commit > > > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > > > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > > > external resources (USB role switch and inherently via called > > > typec_register_port() USB-C muxes, switches and retimers). > > > > > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > > > driver, use the API added by Johan Hovold and move HPD bridge > > > registration to the end of the probe function. > > > > > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > > > function after all TCPM start functions. This is done as a way to > > > overcome a different problem, the DRM subsystem can not properly cope > > > with the DRM bridges being destroyed once the bridge is attached. Having > > > this function call at the end of the probe function prevents possible > > > DRM bridge device creation followed by destruction in case one of the > > > TCPM start functions returns an error. > > > > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > > > --- > > > Changes in v4: > > > - Rebased on top of Johan's patches > > > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > > > > > Changes in v3: > > > - Updated commit message to explain my decisions (Johan). > > > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > > > > > Changes in v2: > > > - Fix commit message (Bryan) > > > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > > > --- > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > A stupid gracious ping. It would be nice to fix the issue in 6.10 > > Is this a regression? If so, what commit does it fix? Or has it always > just not worked? Nevermind, you meant 6.10, not 6.9, my fault, queueing it up now... greg k-h
On Sat, May 04, 2024 at 05:15:45PM +0200, Greg Kroah-Hartman wrote: > On Sat, May 04, 2024 at 05:23:20PM +0300, Dmitry Baryshkov wrote: > > On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > If a probe function returns -EPROBE_DEFER after creating another device > > > there is a change of ending up in a probe deferral loop, (see commit > > > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > > > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > > > external resources (USB role switch and inherently via called > > > typec_register_port() USB-C muxes, switches and retimers). > > > > > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > > > driver, use the API added by Johan Hovold and move HPD bridge > > > registration to the end of the probe function. > > > > > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > > > function after all TCPM start functions. This is done as a way to > > > overcome a different problem, the DRM subsystem can not properly cope > > > with the DRM bridges being destroyed once the bridge is attached. Having > > > this function call at the end of the probe function prevents possible > > > DRM bridge device creation followed by destruction in case one of the > > > TCPM start functions returns an error. > > > > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > > > --- > > > Changes in v4: > > > - Rebased on top of Johan's patches > > > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > > > > > Changes in v3: > > > - Updated commit message to explain my decisions (Johan). > > > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > > > > > Changes in v2: > > > - Fix commit message (Bryan) > > > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > > > --- > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > A stupid gracious ping. It would be nice to fix the issue in 6.10 > > Is this a regression? If so, what commit does it fix? Or has it always > just not worked? Oh wait, I need Johan's patches applied first, I was waiting for that to happen, so I'll take this next week when that gets into Linus's tree, sorry for the delay. greg k-h
On Sat, 4 May 2024 at 19:22, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sat, May 04, 2024 at 05:15:45PM +0200, Greg Kroah-Hartman wrote: > > On Sat, May 04, 2024 at 05:23:20PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > If a probe function returns -EPROBE_DEFER after creating another device > > > > there is a change of ending up in a probe deferral loop, (see commit > > > > fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case > > > > of the qcom-pmic-typec driver the tcpm_register_port() function looks up > > > > external resources (USB role switch and inherently via called > > > > typec_register_port() USB-C muxes, switches and retimers). > > > > > > > > In order to prevent such probe-defer loops caused by qcom-pmic-typec > > > > driver, use the API added by Johan Hovold and move HPD bridge > > > > registration to the end of the probe function. > > > > > > > > The devm_drm_dp_hpd_bridge_add() is called at the end of the probe > > > > function after all TCPM start functions. This is done as a way to > > > > overcome a different problem, the DRM subsystem can not properly cope > > > > with the DRM bridges being destroyed once the bridge is attached. Having > > > > this function call at the end of the probe function prevents possible > > > > DRM bridge device creation followed by destruction in case one of the > > > > TCPM start functions returns an error. > > > > > > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ > > > > --- > > > > Changes in v4: > > > > - Rebased on top of Johan's patches > > > > - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org > > > > > > > > Changes in v3: > > > > - Updated commit message to explain my decisions (Johan). > > > > - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org > > > > > > > > Changes in v2: > > > > - Fix commit message (Bryan) > > > > - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org > > > > --- > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > A stupid gracious ping. It would be nice to fix the issue in 6.10 > > > > Is this a regression? If so, what commit does it fix? Or has it always > > just not worked? > > Oh wait, I need Johan's patches applied first, I was waiting for that to > happen, so I'll take this next week when that gets into Linus's tree, > sorry for the delay. No problem, as long as it has a chance to land at 6.10. Thank you!
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c index d3958c061a97..501eddb294e4 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,8 +110,14 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev) if (ret) goto port_stop; + ret = devm_drm_dp_hpd_bridge_add(tcpm->dev, bridge_dev); + if (ret) + goto pdphy_stop; + return 0; +pdphy_stop: + tcpm->pdphy_stop(tcpm); port_stop: tcpm->port_stop(tcpm); port_unregister:
If a probe function returns -EPROBE_DEFER after creating another device there is a change of ending up in a probe deferral loop, (see commit fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER"). In case of the qcom-pmic-typec driver the tcpm_register_port() function looks up external resources (USB role switch and inherently via called typec_register_port() USB-C muxes, switches and retimers). In order to prevent such probe-defer loops caused by qcom-pmic-typec driver, use the API added by Johan Hovold and move HPD bridge registration to the end of the probe function. The devm_drm_dp_hpd_bridge_add() is called at the end of the probe function after all TCPM start functions. This is done as a way to overcome a different problem, the DRM subsystem can not properly cope with the DRM bridges being destroyed once the bridge is attached. Having this function call at the end of the probe function prevents possible DRM bridge device creation followed by destruction in case one of the TCPM start functions returns an error. Reported-by: Caleb Connolly <caleb.connolly@linaro.org> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dependency: https://lore.kernel.org/lkml/20240418145730.4605-2-johan+linaro@kernel.org/ --- Changes in v4: - Rebased on top of Johan's patches - Link to v3: https://lore.kernel.org/r/20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org Changes in v3: - Updated commit message to explain my decisions (Johan). - Link to v2: https://lore.kernel.org/r/20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org Changes in v2: - Fix commit message (Bryan) - Link to v1: https://lore.kernel.org/r/20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org --- drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) --- base-commit: 15d419fa23ecc51e388a369bbeaf3b47b0b5c76a change-id: 20240405-qc-pmic-typec-hpd-split-22804201902b Best regards,