Re: [PATCH v3 8/8] drm/vkms: convert to use faux_device

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@bootlin.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Simona Vetter <simona@ffwll.ch>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.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.

> 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.

And as for the merge issue, not a problem, I just did this conversion
for people to see how this works and ideally test it, as you did, to
find issues!

thanks,

greg k-h