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@xxxxxxxxxxx>
> > > Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> > > Cc: Simona Vetter <simona@xxxxxxxx>
> > > Cc: Melissa Wen <melissa.srw@xxxxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > Cc: David Airlie <airlied@xxxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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?
Thanks,
Louis Chauvet
> 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
