From: Johan Hovold <johan@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Johan Hovold <johan+linaro@kernel.org>,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org,
Caleb Connolly <caleb.connolly@linaro.org>
Subject: Re: [PATCH v3] usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration
Date: Thu, 18 Apr 2024 17:12:30 +0200 [thread overview]
Message-ID: <ZiE4Xpjhv2llY4pG@hovoldconsulting.com> (raw)
In-Reply-To: <20240416-qc-pmic-typec-hpd-split-v3-1-fd071e3191a1@linaro.org>
On Tue, Apr 16, 2024 at 05:18:56AM +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.
You're still not explaining why it is ok to move registration of the
bridge to after starting the port and pdphy.
Perhaps it's obvious to you but it should still go in the commit message
as such a change is potentially something that could end up causing
trouble (e.g. enabling interrupts before all resources have been setup
and registered).
As I've mentioned before, I'm also sceptical to papering over the DRM
issue in each and every driver registering a bridge. These late error
paths would normally not be taken, unlike the earlier ones which can be
triggered by probe deferrals and which we have to fix also for the probe
deferral loops.
> @@ -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;
This is leaking resources and can lead to a use-after-free.
When looking at the driver, I noticed that the existing error handling
is already broken so I just sent a fix here:
https://lore.kernel.org/lkml/20240418145730.4605-1-johan+linaro@kernel.org/
You should rebase on that series and not introduce further issues with
the new bridge-add error path.
> +
> return 0;
>
> fwnode_remove:
Johan
prev parent reply other threads:[~2024-04-18 15:12 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 2:18 [PATCH v3] usb: typec: qcom-pmic-typec: split HPD bridge alloc and registration Dmitry Baryshkov
2024-04-18 15:12 ` Johan Hovold [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZiE4Xpjhv2llY4pG@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=andersson@kernel.org \
--cc=bryan.odonoghue@linaro.org \
--cc=caleb.connolly@linaro.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=johan+linaro@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).