| Message ID | 2025020625-unlaced-vagueness-ae34@gregkh (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Lol, I wrote up a patch for this last night but it looks like you got here first :P On Thu, 2025-02-06 at 18:38 +0100, 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. > drivers/gpu/drm/vkms/vkms_drv.c | 28 ++++++++++++++-------------- > drivers/gpu/drm/vkms/vkms_drv.h | 4 ++-- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > index e0409aba9349..b1269f984886 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.c > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > @@ -10,7 +10,7 @@ > */ > > #include <linux/module.h> > -#include <linux/platform_device.h> > +#include <linux/device/faux.h> > #include <linux/dma-mapping.h> > > #include <drm/clients/drm_client_setup.h> > @@ -177,25 +177,25 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) > static int vkms_create(struct vkms_config *config) > { > int ret; > - struct platform_device *pdev; > + struct faux_device *fdev; > struct vkms_device *vkms_device; > > - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0); > - if (IS_ERR(pdev)) > - return PTR_ERR(pdev); > + fdev = faux_device_create(DRIVER_NAME, NULL); > + if (!fdev) > + return -ENODEV; > > - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > + if (!devres_open_group(&fdev->dev, NULL, GFP_KERNEL)) { > ret = -ENOMEM; > goto out_unregister; > } > > - vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, > + vkms_device = devm_drm_dev_alloc(&fdev->dev, &vkms_driver, > struct vkms_device, drm); > if (IS_ERR(vkms_device)) { > ret = PTR_ERR(vkms_device); > goto out_devres; > } > - vkms_device->platform = pdev; > + vkms_device->faux_dev = fdev; > vkms_device->config = config; > config->dev = vkms_device; > > @@ -229,9 +229,9 @@ static int vkms_create(struct vkms_config *config) > return 0; > > out_devres: > - devres_release_group(&pdev->dev, NULL); > + devres_release_group(&fdev->dev, NULL); > out_unregister: > - platform_device_unregister(pdev); > + faux_device_destroy(fdev); > return ret; > } > > @@ -259,19 +259,19 @@ static int __init vkms_init(void) > > static void vkms_destroy(struct vkms_config *config) > { > - struct platform_device *pdev; > + struct faux_device *fdev; > > if (!config->dev) { > DRM_INFO("vkms_device is NULL.\n"); > return; > } > > - pdev = config->dev->platform; > + fdev = config->dev->faux_dev; > > drm_dev_unregister(&config->dev->drm); > drm_atomic_helper_shutdown(&config->dev->drm); > - devres_release_group(&pdev->dev, NULL); > - platform_device_unregister(pdev); > + devres_release_group(&fdev->dev, NULL); > + faux_device_destroy(fdev); > > config->dev = NULL; > } > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 00541eff3d1b..4668b0e29a84 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -209,13 +209,13 @@ struct vkms_config { > * struct vkms_device - Description of a VKMS device > * > * @drm - Base device in DRM > - * @platform - Associated platform device > + * @faux_dev- Associated faux device Small nitpick - you dropped the space on the - here by mistake With that fixed: Reviewed-by: Lyude Paul <lyude@redhat.com> > * @output - Configuration and sub-components of the VKMS device > * @config: Configuration used in this VKMS device > */ > struct vkms_device { > struct drm_device drm; > - struct platform_device *platform; > + struct faux_device *faux_dev; > struct vkms_output output; > const struct vkms_config *config; > };
On Thu, Feb 06, 2025 at 03:03:41PM -0500, Lyude Paul wrote: > Lol, I wrote up a patch for this last night but it looks like you got here > first :P > > On Thu, 2025-02-06 at 18:38 +0100, 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. > > drivers/gpu/drm/vkms/vkms_drv.c | 28 ++++++++++++++-------------- > > drivers/gpu/drm/vkms/vkms_drv.h | 4 ++-- > > 2 files changed, 16 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > > index e0409aba9349..b1269f984886 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > @@ -10,7 +10,7 @@ > > */ > > > > #include <linux/module.h> > > -#include <linux/platform_device.h> > > +#include <linux/device/faux.h> > > #include <linux/dma-mapping.h> > > > > #include <drm/clients/drm_client_setup.h> > > @@ -177,25 +177,25 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) > > static int vkms_create(struct vkms_config *config) > > { > > int ret; > > - struct platform_device *pdev; > > + struct faux_device *fdev; > > struct vkms_device *vkms_device; > > > > - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0); > > - if (IS_ERR(pdev)) > > - return PTR_ERR(pdev); > > + fdev = faux_device_create(DRIVER_NAME, NULL); > > + if (!fdev) > > + return -ENODEV; > > > > - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > > + if (!devres_open_group(&fdev->dev, NULL, GFP_KERNEL)) { > > ret = -ENOMEM; > > goto out_unregister; > > } > > > > - vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, > > + vkms_device = devm_drm_dev_alloc(&fdev->dev, &vkms_driver, > > struct vkms_device, drm); > > if (IS_ERR(vkms_device)) { > > ret = PTR_ERR(vkms_device); > > goto out_devres; > > } > > - vkms_device->platform = pdev; > > + vkms_device->faux_dev = fdev; > > vkms_device->config = config; > > config->dev = vkms_device; > > > > @@ -229,9 +229,9 @@ static int vkms_create(struct vkms_config *config) > > return 0; > > > > out_devres: > > - devres_release_group(&pdev->dev, NULL); > > + devres_release_group(&fdev->dev, NULL); > > out_unregister: > > - platform_device_unregister(pdev); > > + faux_device_destroy(fdev); > > return ret; > > } > > > > @@ -259,19 +259,19 @@ static int __init vkms_init(void) > > > > static void vkms_destroy(struct vkms_config *config) > > { > > - struct platform_device *pdev; > > + struct faux_device *fdev; > > > > if (!config->dev) { > > DRM_INFO("vkms_device is NULL.\n"); > > return; > > } > > > > - pdev = config->dev->platform; > > + fdev = config->dev->faux_dev; > > > > drm_dev_unregister(&config->dev->drm); > > drm_atomic_helper_shutdown(&config->dev->drm); > > - devres_release_group(&pdev->dev, NULL); > > - platform_device_unregister(pdev); > > + devres_release_group(&fdev->dev, NULL); > > + faux_device_destroy(fdev); > > > > config->dev = NULL; > > } > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 00541eff3d1b..4668b0e29a84 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -209,13 +209,13 @@ struct vkms_config { > > * struct vkms_device - Description of a VKMS device > > * > > * @drm - Base device in DRM > > - * @platform - Associated platform device > > + * @faux_dev- Associated faux device > > Small nitpick - you dropped the space on the - here by mistake Ick, good catch. > With that fixed: > > Reviewed-by: Lyude Paul <lyude@redhat.com> Thanks for the review! greg k-h
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 */ 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 [2]:https://lore.kernel.org/all/20250121-google-config-fs-v3-0-8154a6945142@bootlin.com/ I found two other drm driver that may be broken in the same way (very similar code pattern): https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c#L64 https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpu/drm/vgem/vgem_drv.c#L138 Thanks a lot, Louis Chauvet Change to hide the issue: diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 89ccf0d6419a..84777d6ba889 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -174,7 +174,7 @@ static int vkms_create(struct vkms_config *config) fdev = faux_device_create(DRIVER_NAME, NULL); if (!fdev) return -ENODEV; - + pr_err("%s:%d\n", __FILE__, __LINE__); if (!devres_open_group(&fdev->dev, NULL, GFP_KERNEL)) { ret = -ENOMEM; goto out_unregister; > drivers/gpu/drm/vkms/vkms_drv.c | 28 ++++++++++++++-------------- > drivers/gpu/drm/vkms/vkms_drv.h | 4 ++-- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > index e0409aba9349..b1269f984886 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.c > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > @@ -10,7 +10,7 @@ > */ > > #include <linux/module.h> > -#include <linux/platform_device.h> > +#include <linux/device/faux.h> > #include <linux/dma-mapping.h> > > #include <drm/clients/drm_client_setup.h> > @@ -177,25 +177,25 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) > static int vkms_create(struct vkms_config *config) > { > int ret; > - struct platform_device *pdev; > + struct faux_device *fdev; > struct vkms_device *vkms_device; > > - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0); > - if (IS_ERR(pdev)) > - return PTR_ERR(pdev); > + fdev = faux_device_create(DRIVER_NAME, NULL); > + if (!fdev) > + return -ENODEV; > > - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > + if (!devres_open_group(&fdev->dev, NULL, GFP_KERNEL)) { > ret = -ENOMEM; > goto out_unregister; > } > > - vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, > + vkms_device = devm_drm_dev_alloc(&fdev->dev, &vkms_driver, > struct vkms_device, drm); > if (IS_ERR(vkms_device)) { > ret = PTR_ERR(vkms_device); > goto out_devres; > } > - vkms_device->platform = pdev; > + vkms_device->faux_dev = fdev; > vkms_device->config = config; > config->dev = vkms_device; > > @@ -229,9 +229,9 @@ static int vkms_create(struct vkms_config *config) > return 0; > > out_devres: > - devres_release_group(&pdev->dev, NULL); > + devres_release_group(&fdev->dev, NULL); > out_unregister: > - platform_device_unregister(pdev); > + faux_device_destroy(fdev); > return ret; > } > > @@ -259,19 +259,19 @@ static int __init vkms_init(void) > > static void vkms_destroy(struct vkms_config *config) > { > - struct platform_device *pdev; > + struct faux_device *fdev; > > if (!config->dev) { > DRM_INFO("vkms_device is NULL.\n"); > return; > } > > - pdev = config->dev->platform; > + fdev = config->dev->faux_dev; > > drm_dev_unregister(&config->dev->drm); > drm_atomic_helper_shutdown(&config->dev->drm); > - devres_release_group(&pdev->dev, NULL); > - platform_device_unregister(pdev); > + devres_release_group(&fdev->dev, NULL); > + faux_device_destroy(fdev); > > config->dev = NULL; > } > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 00541eff3d1b..4668b0e29a84 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -209,13 +209,13 @@ struct vkms_config { > * struct vkms_device - Description of a VKMS device > * > * @drm - Base device in DRM > - * @platform - Associated platform device > + * @faux_dev- Associated faux device > * @output - Configuration and sub-components of the VKMS device > * @config: Configuration used in this VKMS device > */ > struct vkms_device { > struct drm_device drm; > - struct platform_device *platform; > + struct faux_device *faux_dev; > struct vkms_output output; > const struct vkms_config *config; > }; > -- > 2.48.1 >
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
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@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. 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
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@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. > > 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)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index e0409aba9349..b1269f984886 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -10,7 +10,7 @@ */ #include <linux/module.h> -#include <linux/platform_device.h> +#include <linux/device/faux.h> #include <linux/dma-mapping.h> #include <drm/clients/drm_client_setup.h> @@ -177,25 +177,25 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) static int vkms_create(struct vkms_config *config) { int ret; - struct platform_device *pdev; + struct faux_device *fdev; struct vkms_device *vkms_device; - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0); - if (IS_ERR(pdev)) - return PTR_ERR(pdev); + fdev = faux_device_create(DRIVER_NAME, NULL); + if (!fdev) + return -ENODEV; - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { + if (!devres_open_group(&fdev->dev, NULL, GFP_KERNEL)) { ret = -ENOMEM; goto out_unregister; } - vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, + vkms_device = devm_drm_dev_alloc(&fdev->dev, &vkms_driver, struct vkms_device, drm); if (IS_ERR(vkms_device)) { ret = PTR_ERR(vkms_device); goto out_devres; } - vkms_device->platform = pdev; + vkms_device->faux_dev = fdev; vkms_device->config = config; config->dev = vkms_device; @@ -229,9 +229,9 @@ static int vkms_create(struct vkms_config *config) return 0; out_devres: - devres_release_group(&pdev->dev, NULL); + devres_release_group(&fdev->dev, NULL); out_unregister: - platform_device_unregister(pdev); + faux_device_destroy(fdev); return ret; } @@ -259,19 +259,19 @@ static int __init vkms_init(void) static void vkms_destroy(struct vkms_config *config) { - struct platform_device *pdev; + struct faux_device *fdev; if (!config->dev) { DRM_INFO("vkms_device is NULL.\n"); return; } - pdev = config->dev->platform; + fdev = config->dev->faux_dev; drm_dev_unregister(&config->dev->drm); drm_atomic_helper_shutdown(&config->dev->drm); - devres_release_group(&pdev->dev, NULL); - platform_device_unregister(pdev); + devres_release_group(&fdev->dev, NULL); + faux_device_destroy(fdev); config->dev = NULL; } diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 00541eff3d1b..4668b0e29a84 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -209,13 +209,13 @@ struct vkms_config { * struct vkms_device - Description of a VKMS device * * @drm - Base device in DRM - * @platform - Associated platform device + * @faux_dev- Associated faux device * @output - Configuration and sub-components of the VKMS device * @config: Configuration used in this VKMS device */ struct vkms_device { struct drm_device drm; - struct platform_device *platform; + struct faux_device *faux_dev; struct vkms_output output; const struct vkms_config *config; };
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. drivers/gpu/drm/vkms/vkms_drv.c | 28 ++++++++++++++-------------- drivers/gpu/drm/vkms/vkms_drv.h | 4 ++-- 2 files changed, 16 insertions(+), 16 deletions(-)