Received-SPF: pass (google.com: domain of linux-kernel+bounces-134533-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240405-qc-pmic-typec-hpd-split-v1-1-363daafb3c36@linaro.org> <c1899fae-8669-485a-95bd-0e76738a1187@linaro.org>
In-Reply-To: <c1899fae-8669-485a-95bd-0e76738a1187@linaro.org>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Sun, 7 Apr 2024 18:08:55 +0300
Message-ID: <CAA8EJppBE8xdmc6zQ2VLLPSzmtNq-+03H5skqbnLmLZ8AqSBfw@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration
To: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>, Bjorn Andersson <andersson@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Caleb Connolly <caleb.connolly@linaro.org>, 
	Johan Hovold <johan+linaro@kernel.org>
Content-Type: text/plain; charset="UTF-8"

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


-- 
With best wishes
Dmitry