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

Commit Message

Dmitry Baryshkov April 24, 2024, 2:16 a.m. UTC

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 <[e-mail]>
Acked-by: Bryan O'Donoghue <[e-mail]>
Signed-off-by: Dmitry Baryshkov <[e-mail]>
---
Dependency: https://lore.kernel.org/lkml/[e-mail]/
---
Changes in v4:
- Rebased on top of Johan's patches
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- Updated commit message to explain my decisions (Johan).
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- Fix commit message (Bryan)
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 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,

Comments

Heikki Krogerus April 25, 2024, 9:53 a.m. UTC | #1

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 <[e-mail]>
> Acked-by: Bryan O'Donoghue <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>

Reviewed-by: Heikki Krogerus <[e-mail]>

> ---
> Dependency: https://lore.kernel.org/lkml/[e-mail]/
> ---
> Changes in v4:
> - Rebased on top of Johan's patches
> - Link to v3: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v3:
> - Updated commit message to explain my decisions (Johan).
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v2:
> - Fix commit message (Bryan)
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  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 <[e-mail]>

Johan Hovold April 25, 2024, 12:17 p.m. UTC | #2

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 <[e-mail]>
> Acked-by: Bryan O'Donoghue <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
> Dependency: https://lore.kernel.org/lkml/[e-mail]/
> ---
> Changes in v4:
> - Rebased on top of Johan's patches
> - Link to v3: https://lore.kernel.org/r/[e-mail]

Reviewed-by: Johan Hovold <[e-mail]>

Johan

Dmitry Baryshkov May 4, 2024, 2:23 p.m. UTC | #3

On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov
<[e-mail]> 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 <[e-mail]>
> Acked-by: Bryan O'Donoghue <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
> Dependency: https://lore.kernel.org/lkml/[e-mail]/
> ---
> Changes in v4:
> - Rebased on top of Johan's patches
> - Link to v3: https://lore.kernel.org/r/[e-mail]
>
> Changes in v3:
> - Updated commit message to explain my decisions (Johan).
> - Link to v2: https://lore.kernel.org/r/[e-mail]
>
> Changes in v2:
> - Fix commit message (Bryan)
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  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

Greg Kroah-Hartman May 4, 2024, 3:15 p.m. UTC | #4

On Sat, May 04, 2024 at 05:23:20PM +0300, Dmitry Baryshkov wrote:
> On Wed, 24 Apr 2024 at 05:16, Dmitry Baryshkov
> <[e-mail]> 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 <[e-mail]>
> > Acked-by: Bryan O'Donoghue <[e-mail]>
> > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > ---
> > Dependency: https://lore.kernel.org/lkml/[e-mail]/
> > ---
> > Changes in v4:
> > - Rebased on top of Johan's patches
> > - Link to v3: https://lore.kernel.org/r/[e-mail]
> >
> > Changes in v3:
> > - Updated commit message to explain my decisions (Johan).
> > - Link to v2: https://lore.kernel.org/r/[e-mail]
> >
> > Changes in v2:
> > - Fix commit message (Bryan)
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  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

Greg Kroah-Hartman May 4, 2024, 4:21 p.m. UTC | #5

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
> > <[e-mail]> 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 <[e-mail]>
> > > Acked-by: Bryan O'Donoghue <[e-mail]>
> > > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > ---
> > > Dependency: https://lore.kernel.org/lkml/[e-mail]/
> > > ---
> > > Changes in v4:
> > > - Rebased on top of Johan's patches
> > > - Link to v3: https://lore.kernel.org/r/[e-mail]
> > >
> > > Changes in v3:
> > > - Updated commit message to explain my decisions (Johan).
> > > - Link to v2: https://lore.kernel.org/r/[e-mail]
> > >
> > > Changes in v2:
> > > - Fix commit message (Bryan)
> > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > ---
> > >  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

Greg Kroah-Hartman May 4, 2024, 4:22 p.m. UTC | #6

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
> > <[e-mail]> 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 <[e-mail]>
> > > Acked-by: Bryan O'Donoghue <[e-mail]>
> > > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > ---
> > > Dependency: https://lore.kernel.org/lkml/[e-mail]/
> > > ---
> > > Changes in v4:
> > > - Rebased on top of Johan's patches
> > > - Link to v3: https://lore.kernel.org/r/[e-mail]
> > >
> > > Changes in v3:
> > > - Updated commit message to explain my decisions (Johan).
> > > - Link to v2: https://lore.kernel.org/r/[e-mail]
> > >
> > > Changes in v2:
> > > - Fix commit message (Bryan)
> > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > ---
> > >  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

Dmitry Baryshkov May 4, 2024, 4:54 p.m. UTC | #7

On Sat, 4 May 2024 at 19:22, Greg Kroah-Hartman
<[e-mail]> 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
> > > <[e-mail]> 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 <[e-mail]>
> > > > Acked-by: Bryan O'Donoghue <[e-mail]>
> > > > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > > > ---
> > > > Dependency: https://lore.kernel.org/lkml/[e-mail]/
> > > > ---
> > > > Changes in v4:
> > > > - Rebased on top of Johan's patches
> > > > - Link to v3: https://lore.kernel.org/r/[e-mail]
> > > >
> > > > Changes in v3:
> > > > - Updated commit message to explain my decisions (Johan).
> > > > - Link to v2: https://lore.kernel.org/r/[e-mail]
> > > >
> > > > Changes in v2:
> > > > - Fix commit message (Bryan)
> > > > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > > > ---
> > > >  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!

Patch

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: