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

[Dmitry Baryshkov]

On Sun, 7 Apr 2024 at 01:03, Bryan O'Donoghue
<bryan.odonoghue@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
>
> ---
> bod



-- 
With best wishes
Dmitry