Message-ID: <2025020844-stank-catlike-2661@gregkh>
Date: Sat, 8 Feb 2025 09:49:50 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, Lyude Paul <lyude@...hat.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Liam Girdwood <lgirdwood@...il.com>, Lukas Wunner <lukas@...ner.de>,
	Mark Brown <broonie@...nel.org>,
	Maíra Canal <mairacanal@...eup.net>,
	Robin Murphy <robin.murphy@....com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	Zijun Hu <quic_zijuhu@...cinc.com>, linux-usb@...r.kernel.org,
	rust-for-linux@...r.kernel.org,
	Haneen Mohammed <hamohammed.sa@...il.com>,
	Simona Vetter <simona@...ll.ch>,
	Melissa Wen <melissa.srw@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3 8/8] drm/vkms: convert to use faux_device

On Sat, Feb 08, 2025 at 09:37:56AM +0100, Louis Chauvet wrote:
> On 08/02/25 - 08:12, Greg Kroah-Hartman wrote:
> > On Fri, Feb 07, 2025 at 05:59:04PM +0100, Louis Chauvet wrote:
> > > On 06/02/25 - 18:38, Greg Kroah-Hartman wrote:
> > > > The vkms driver does not need to create a platform device, as there is
> > > > no real platform resources associated it,  it only did so because it was
> > > > simple to do that in order to get a device to use for resource
> > > > management of drm resources.  Change the driver to use the faux device
> > > > instead as this is NOT a real platform device.
> > > > 
> > > > Cc: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > Cc: Haneen Mohammed <hamohammed.sa@...il.com>
> > > > Cc: Simona Vetter <simona@...ll.ch>
> > > > Cc: Melissa Wen <melissa.srw@...il.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> > > > Cc: Maxime Ripard <mripard@...nel.org>
> > > > Cc: Thomas Zimmermann <tzimmermann@...e.de>
> > > > Cc: David Airlie <airlied@...il.com>
> > > > Cc: dri-devel@...ts.freedesktop.org
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > > ---
> > > >  v3: new patch in the series.  For an example of the api working, does
> > > >      not have to be merged at this time, but I can take it if the
> > > >      maintainers give an ack.
> > > 
> > > Hi,
> > > 
> > > This patch cannot be merged into drm-misc-next because we modified the 
> > > vkms_device structure in commit 49a167c393b0 ("drm/vkms: Switch to dynamic 
> > > allocation for CRTC"), which is present in linux-next.
> > > 
> > > Once this conflict is resolved, I agree with changing from platform_device 
> > > to faux_device.
> > > 
> > > Apart from this minor conflict, I believe your patch has revealed an issue 
> > > in VKMS:
> > > 
> > > Without your patch:
> > > 
> > > 	bash-5.2# modprobe vkms
> > > 	[drm] Initialized vkms 1.0.0 for vkms on minor 0
> > > 	bash-5.2#
> > > 
> > > With your patch:
> > > 
> > > 	bash-5.2# modprobe vkms
> > > 	faux vkms: Resources present before probing
> > > 	[drm] Initialized vkms 1.0.0 for vkms on minor 0
> > > 	bash-5.2#
> > > 
> > > After some investigation, I found that the issue is not caused by your 
> > > patch but by VKMS itself:
> > > 
> > > During faux_device_create, the device core postpones the device probe to 
> > > run it later [0]. This probe checks if the devres list is empty [1] and 
> > > fails if it is not.
> > > 
> > > [0]:https://elixir.bootlin.com/linux/v6.13.1/source/drivers/base/bus.c#L534
> > > [1]:https://elixir.bootlin.com/linux/v6.13.1/source/drivers/base/dd.c#L626
> > > 
> > > With a platform driver, the order of execution was:
> > > 
> > > 	platform_device_register_simple();
> > > 		device_add();
> > > 	*async* device_probe(); /* no issue, the devres is untouched */
> > > 	devres_open_group();
> > > 
> > > But with faux-device, the order is:
> > > 
> > > 	faux_device_create();
> > > 		device_add();
> > > 	devres_open_group();
> > > 	*async* device_probe(); /* issue here, because of the previous 
> > > 				   devres_open_group */
> > 
> > Wait, what?  It shouuld be the exact same codepath, as faux_device() is
> > not doing anything different from platform here.  You might just be
> > hitting a race condition as the async probing is the same here.
> 
> Yes, this is the same codepath, and this is a race condition. VKMS was 
> just lucky it never happend before. 
> 
> > > How do you think this should be solved? I would like to keep a simple 
> > > solution, given that:
> > > - we want to have multiple vkms devices (configfs [2])
> > > - we need to ensure that device_probe is called before devres_open_group 
> > >   and devm_drm_dev_alloc to avoid this error
> > 
> > How about we take out the async probe?  You are getting lucky that it's
> > not hit on the platform device code today.  Faux really doesn't need
> > async, I was just trying to make the system work the same way that
> > platform devices did.
> 
> I think this should be sufficient, and allows for a very simple interface: 
> once faux_device_create returns, you can use the device "as-is", no 
> need to wait for the probe.
> 
> What change can I do to disable async probe and test?

Try this patch:

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 27879ae78f53..0b9d17cd41f2 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -73,7 +73,7 @@ static const struct bus_type faux_bus_type = {
 static struct device_driver faux_driver = {
 	.name		= "faux_driver",
 	.bus		= &faux_bus_type,
-	.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
+	.probe_type	= PROBE_FORCE_SYNCHRONOUS,
 };
 
 static void faux_device_release(struct device *dev)

Powered by blists - more mailing lists