Received-SPF: pass (google.com: domain of linux-kernel+bounces-135273-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 8 Apr 2024 13:44:13 +0200
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>,
	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 v2] usb: typec: qcom-pmic-typec: split HPD bridge alloc
 and registration
Message-ID: <ZhPYjXjX3LcCMhyh@hovoldconsulting.com>
References: <20240408-qc-pmic-typec-hpd-split-v2-1-1704f5321b73@linaro.org>
 <ZhOYpHXz6t0fkzZ2@hovoldconsulting.com>
 <2ejpom6ykci6o7d7luwa2ao4stpm24aoyi66aoncxcqcwgidxz@gcsqvpb3s7nr>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2ejpom6ykci6o7d7luwa2ao4stpm24aoyi66aoncxcqcwgidxz@gcsqvpb3s7nr>

On Mon, Apr 08, 2024 at 01:49:48PM +0300, Dmitry Baryshkov wrote:
> On Mon, Apr 08, 2024 at 09:11:32AM +0200, Johan Hovold wrote:
> > On Mon, Apr 08, 2024 at 04:06:40AM +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 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.
> > 
> > You should be more specific here: which function called after
> > qcom_pmic_typec_probe() can trigger a probe deferral?
> > 
> > I doubt that applies to tcpm->port_start() and tcpm->pdphy_start() in
> > which case the bridge should be added before those calls unless there
> > are other reasons for not doing so, which then also should be mentioned.
> > 
> > I suspect the trouble is with tcpm_register_port(), but please spell
> > that out and mention in which scenarios that function may return
> > -EPROBE_DEFER.
> 
> The probe loop comes from from tcpm_register_port(), you are right.
> However then putting bridge registration before the _start() functions
> is also incorrect as this will be prone to use-after-free errors that
> you have fixed in pmic-glink.

You obviously have to mention that in the commit message as that is a
separate change and also one that looks broken as you're now registering
resources after the device has gone "live".

So you also need to explain why you think that is safe, if it should be
done at all. You're essentially just papering over a DRM bug in the
unlikely event that probe fails.

Johan