Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/21/2018 10:58 AM, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Add support for Xen para-virtualized frontend display driver. Accompanying backend [1] is implemented as a user-space application and its helper library [2], capable of running as a Weston client or DRM master. Configuration of both backend and frontend is done via Xen guest domain configuration options [3]. I won't claim that I really understand what's going on here as far as DRM stuff is concerned but I didn't see any obvious issues with Xen bits. So for that you can tack on my Reviewed-by: Boris Ostrovsky ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/22/2018 03:14 AM, Boris Ostrovsky wrote: On 03/21/2018 10:58 AM, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Add support for Xen para-virtualized frontend display driver. Accompanying backend [1] is implemented as a user-space application and its helper library [2], capable of running as a Weston client or DRM master. Configuration of both backend and frontend is done via Xen guest domain configuration options [3]. I won't claim that I really understand what's going on here as far as DRM stuff is concerned but I didn't see any obvious issues with Xen bits. So for that you can tack on my Reviewed-by: Boris Ostrovsky Thank you ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
My apologies, but I found a few more things that look strange and should be cleaned up. Sorry for this iterative review approach, but I think we're slowly getting there. Thank you for reviewing! Cheers, Daniel --- +static int xen_drm_drv_dumb_create(struct drm_file *filp, + struct drm_device *dev, struct drm_mode_create_dumb *args) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct drm_gem_object *obj; + int ret; + + ret = xen_drm_front_gem_dumb_create(filp, dev, args); + if (ret) + goto fail; + + obj = drm_gem_object_lookup(filp, args->handle); + if (!obj) { + ret = -ENOENT; + goto fail_destroy; + } + + drm_gem_object_unreference_unlocked(obj); You can't drop the reference while you keep using the object, someone else might sneak in and destroy your object. The unreference always must be last. Will fix, thank you + + /* +* In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed +* via DRM CMA helpers and doesn't have ->pages allocated +* (xendrm_gem_get_pages will return NULL), but instead can provide +* sg table +*/ + if (xen_drm_front_gem_get_pages(obj)) + ret = xen_drm_front_dbuf_create_from_pages( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_pages(obj)); + else + ret = xen_drm_front_dbuf_create_from_sgt( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_sg_table(obj)); + if (ret) + goto fail_destroy; + The above also has another race: If you construct an object, then it must be fully constructed by the time you publish it to the wider world. In gem this is done by calling drm_gem_handle_create() - after that userspace can get at your object and do nasty things with it in a separate thread, forcing your driver to Oops if the object isn't fully constructed yet. That means you need to redo this code here to make sure that the gem object is fully set up (including pages and sg tables) _before_ anything calls drm_gem_handle_create(). You are correct, I need to rework this code This probably means you also need to open-code the cma side, by first calling drm_gem_cma_create(), then doing any additional setup, and finally doing the registration to userspace with drm_gem_handle_create as the very last thing. Although I tend to avoid open-coding, but this seems the necessary measure here Alternativet is to do the pages/sg setup only when you create an fb (and drop the pages again when the fb is destroyed), but that requires some refcounting/locking in the driver. Not sure this will work: nothing prevents you from attaching multiple FBs to a single dumb handle So, not only ref-counting should be done here, but I also need to check if the dumb buffer, we are attaching to, has been created already So, I will rework with open-coding some stuff from CMA helpers Aside: There's still a lot of indirection and jumping around which makes the code a bit hard to follow. Probably I am not sure of which indirection we are talking about, could you please specifically mark those annoying you? + +static void xen_drm_drv_release(struct drm_device *dev) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct xen_drm_front_info *front_info = drm_info->front_info; + + drm_atomic_helper_shutdown(dev); + drm_mode_config_cleanup(dev); + + xen_drm_front_evtchnl_free_all(front_info); + dbuf_free_all(&front_info->dbuf_list); + + drm_dev_fini(dev); + kfree(dev); + + /* +* Free now, as this release could be not due to rmmod, but +* due to the backend disconnect, making drm_info hang in +* memory until rmmod +*/ + devm_kfree(&front_info->xb_dev->dev, front_info->drm_info); + front_info->drm_info = NULL; + + /* Tell the backend we are ready to (re)initialize */ + xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising); This needs to be in the unplug code. Yes that means you'll have multiple drm_devices floating around, but that's how hotplug works. That would also mean that you need to drop the front_info pointer from the backend at unplug time. If you don't like those semantics then the only other option is to never destroy the drm_device, but only mark the drm_connector as disconnected when the xenbus backend is gone. But this half-half solution here wher
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote: > > > My apologies, but I found a few more things that look strange and should > > be cleaned up. Sorry for this iterative review approach, but I think we're > > slowly getting there. > Thank you for reviewing! > > Cheers, Daniel > > > > > --- > > > +static int xen_drm_drv_dumb_create(struct drm_file *filp, > > > + struct drm_device *dev, struct drm_mode_create_dumb *args) > > > +{ > > > + struct xen_drm_front_drm_info *drm_info = dev->dev_private; > > > + struct drm_gem_object *obj; > > > + int ret; > > > + > > > + ret = xen_drm_front_gem_dumb_create(filp, dev, args); > > > + if (ret) > > > + goto fail; > > > + > > > + obj = drm_gem_object_lookup(filp, args->handle); > > > + if (!obj) { > > > + ret = -ENOENT; > > > + goto fail_destroy; > > > + } > > > + > > > + drm_gem_object_unreference_unlocked(obj); > > You can't drop the reference while you keep using the object, someone else > > might sneak in and destroy your object. The unreference always must be > > last. > Will fix, thank you > > > + > > > + /* > > > + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed > > > + * via DRM CMA helpers and doesn't have ->pages allocated > > > + * (xendrm_gem_get_pages will return NULL), but instead can provide > > > + * sg table > > > + */ > > > + if (xen_drm_front_gem_get_pages(obj)) > > > + ret = xen_drm_front_dbuf_create_from_pages( > > > + drm_info->front_info, > > > + xen_drm_front_dbuf_to_cookie(obj), > > > + args->width, args->height, args->bpp, > > > + args->size, > > > + xen_drm_front_gem_get_pages(obj)); > > > + else > > > + ret = xen_drm_front_dbuf_create_from_sgt( > > > + drm_info->front_info, > > > + xen_drm_front_dbuf_to_cookie(obj), > > > + args->width, args->height, args->bpp, > > > + args->size, > > > + xen_drm_front_gem_get_sg_table(obj)); > > > + if (ret) > > > + goto fail_destroy; > > > + > > The above also has another race: If you construct an object, then it must > > be fully constructed by the time you publish it to the wider world. In gem > > this is done by calling drm_gem_handle_create() - after that userspace can > > get at your object and do nasty things with it in a separate thread, > > forcing your driver to Oops if the object isn't fully constructed yet. > > > > That means you need to redo this code here to make sure that the gem > > object is fully set up (including pages and sg tables) _before_ anything > > calls drm_gem_handle_create(). > You are correct, I need to rework this code > > > > This probably means you also need to open-code the cma side, by first > > calling drm_gem_cma_create(), then doing any additional setup, and finally > > doing the registration to userspace with drm_gem_handle_create as the very > > last thing. > Although I tend to avoid open-coding, but this seems the necessary measure > here > > > > Alternativet is to do the pages/sg setup only when you create an fb (and > > drop the pages again when the fb is destroyed), but that requires some > > refcounting/locking in the driver. > Not sure this will work: nothing prevents you from attaching multiple FBs to > a single dumb handle > So, not only ref-counting should be done here, but I also need to check if > the dumb buffer, > we are attaching to, has been created already No, you must make sure that no dumb buffer can be seen by anyone else before it's fully created. If you don't register it in the file_priv idr using drm_gem_handle_create, no one else can get at your buffer. Trying to paper over this race from all the other places breaks the gem core code design, and is also much more fragile. > So, I will rework with open-coding some stuff from CMA helpers > > > > > Aside: There's still a lot of indirection and jumping around which makes > > the code a bit hard to follow. > Probably I am not sure of which indirection we are talking about, could you > please > specifically mark those annoying you? I think it's the same indirection we talked about last time, it still annoys me. But it's still ok if you prefer this way I think :-) > > > > > > + > > > +static void xen_drm_drv_release(struct drm_device *dev) > > > +{ > > > + struct xen_drm_front_drm_info *drm_info = dev->dev_private; > > > + struct xen_drm_front_info *front_info = drm_info->front_info; > > > + > > > + drm_atomic_helper_shutdown(dev); > > > + drm_mode_config_cleanup(dev); > > > + > > > + xen_drm_front_evtchnl_free_all(front_info); > > > + dbuf_free_all(&front_info->dbuf_list); > > > + > > > + drm_dev_fini(dev); > > > + kfree(dev); > > > + > > > + /* > > > + * Free now, as this release could be not due to rmmod, but > > > + * due to the backend disconnect,
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/26/2018 11:18 AM, Daniel Vetter wrote: On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote: My apologies, but I found a few more things that look strange and should be cleaned up. Sorry for this iterative review approach, but I think we're slowly getting there. Thank you for reviewing! Cheers, Daniel --- +static int xen_drm_drv_dumb_create(struct drm_file *filp, + struct drm_device *dev, struct drm_mode_create_dumb *args) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct drm_gem_object *obj; + int ret; + + ret = xen_drm_front_gem_dumb_create(filp, dev, args); + if (ret) + goto fail; + + obj = drm_gem_object_lookup(filp, args->handle); + if (!obj) { + ret = -ENOENT; + goto fail_destroy; + } + + drm_gem_object_unreference_unlocked(obj); You can't drop the reference while you keep using the object, someone else might sneak in and destroy your object. The unreference always must be last. Will fix, thank you + + /* +* In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed +* via DRM CMA helpers and doesn't have ->pages allocated +* (xendrm_gem_get_pages will return NULL), but instead can provide +* sg table +*/ + if (xen_drm_front_gem_get_pages(obj)) + ret = xen_drm_front_dbuf_create_from_pages( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_pages(obj)); + else + ret = xen_drm_front_dbuf_create_from_sgt( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_sg_table(obj)); + if (ret) + goto fail_destroy; + The above also has another race: If you construct an object, then it must be fully constructed by the time you publish it to the wider world. In gem this is done by calling drm_gem_handle_create() - after that userspace can get at your object and do nasty things with it in a separate thread, forcing your driver to Oops if the object isn't fully constructed yet. That means you need to redo this code here to make sure that the gem object is fully set up (including pages and sg tables) _before_ anything calls drm_gem_handle_create(). You are correct, I need to rework this code This probably means you also need to open-code the cma side, by first calling drm_gem_cma_create(), then doing any additional setup, and finally doing the registration to userspace with drm_gem_handle_create as the very last thing. Although I tend to avoid open-coding, but this seems the necessary measure here Alternativet is to do the pages/sg setup only when you create an fb (and drop the pages again when the fb is destroyed), but that requires some refcounting/locking in the driver. Not sure this will work: nothing prevents you from attaching multiple FBs to a single dumb handle So, not only ref-counting should be done here, but I also need to check if the dumb buffer, we are attaching to, has been created already No, you must make sure that no dumb buffer can be seen by anyone else before it's fully created. If you don't register it in the file_priv idr using drm_gem_handle_create, no one else can get at your buffer. Trying to paper over this race from all the other places breaks the gem core code design, and is also much more fragile. Yes, this is what I implement now, e.g. I do not create any dumb handle until GEM is fully created. I was just saying that alternative way when we do pages/sgt on FB attach will not work in my case So, I will rework with open-coding some stuff from CMA helpers Aside: There's still a lot of indirection and jumping around which makes the code a bit hard to follow. Probably I am not sure of which indirection we are talking about, could you please specifically mark those annoying you? I think it's the same indirection we talked about last time, it still annoys me. But it's still ok if you prefer this way I think :-) Ok, probably this is because I'm looking at the driver from an editor, but you are from your mail client ;) + +static void xen_drm_drv_release(struct drm_device *dev) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct xen_drm_front_info *front_info = drm_info->front_info; + + drm_atomic_helper_shutdown(dev); + drm_mode_config_cleanup(dev); + + xen_drm_front_evtchnl_free_all(front_info); + dbuf_free_all(&front_info->dbuf_list); + + drm_dev_fini(dev); + kfree(dev); +
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
Hi, Daniel! On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote: On 03/26/2018 11:18 AM, Daniel Vetter wrote: On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote: My apologies, but I found a few more things that look strange and should be cleaned up. Sorry for this iterative review approach, but I think we're slowly getting there. Thank you for reviewing! Cheers, Daniel --- +static int xen_drm_drv_dumb_create(struct drm_file *filp, + struct drm_device *dev, struct drm_mode_create_dumb *args) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct drm_gem_object *obj; + int ret; + + ret = xen_drm_front_gem_dumb_create(filp, dev, args); + if (ret) + goto fail; + + obj = drm_gem_object_lookup(filp, args->handle); + if (!obj) { + ret = -ENOENT; + goto fail_destroy; + } + + drm_gem_object_unreference_unlocked(obj); You can't drop the reference while you keep using the object, someone else might sneak in and destroy your object. The unreference always must be last. Will fix, thank you + + /* + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed + * via DRM CMA helpers and doesn't have ->pages allocated + * (xendrm_gem_get_pages will return NULL), but instead can provide + * sg table + */ + if (xen_drm_front_gem_get_pages(obj)) + ret = xen_drm_front_dbuf_create_from_pages( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_pages(obj)); + else + ret = xen_drm_front_dbuf_create_from_sgt( + drm_info->front_info, + xen_drm_front_dbuf_to_cookie(obj), + args->width, args->height, args->bpp, + args->size, + xen_drm_front_gem_get_sg_table(obj)); + if (ret) + goto fail_destroy; + The above also has another race: If you construct an object, then it must be fully constructed by the time you publish it to the wider world. In gem this is done by calling drm_gem_handle_create() - after that userspace can get at your object and do nasty things with it in a separate thread, forcing your driver to Oops if the object isn't fully constructed yet. That means you need to redo this code here to make sure that the gem object is fully set up (including pages and sg tables) _before_ anything calls drm_gem_handle_create(). You are correct, I need to rework this code This probably means you also need to open-code the cma side, by first calling drm_gem_cma_create(), then doing any additional setup, and finally doing the registration to userspace with drm_gem_handle_create as the very last thing. Although I tend to avoid open-coding, but this seems the necessary measure here Alternativet is to do the pages/sg setup only when you create an fb (and drop the pages again when the fb is destroyed), but that requires some refcounting/locking in the driver. Not sure this will work: nothing prevents you from attaching multiple FBs to a single dumb handle So, not only ref-counting should be done here, but I also need to check if the dumb buffer, we are attaching to, has been created already No, you must make sure that no dumb buffer can be seen by anyone else before it's fully created. If you don't register it in the file_priv idr using drm_gem_handle_create, no one else can get at your buffer. Trying to paper over this race from all the other places breaks the gem core code design, and is also much more fragile. Yes, this is what I implement now, e.g. I do not create any dumb handle until GEM is fully created. I was just saying that alternative way when we do pages/sgt on FB attach will not work in my case So, I will rework with open-coding some stuff from CMA helpers Aside: There's still a lot of indirection and jumping around which makes the code a bit hard to follow. Probably I am not sure of which indirection we are talking about, could you please specifically mark those annoying you? I think it's the same indirection we talked about last time, it still annoys me. But it's still ok if you prefer this way I think :-) Ok, probably this is because I'm looking at the driver from an editor, but you are from your mail client ;) + +static void xen_drm_drv_release(struct drm_device *dev) +{ + struct xen_drm_front_drm_info *drm_info = dev->dev_private; + struct xen_drm_front_info *front_info = drm_info->front_info; + + drm_atomic_helper_shutdown(dev); + drm_mode_config_cleanup(dev); + + xen_drm_front_evtchnl_free_all(front_info); + dbuf_free_all(&front_info->dbuf_list); + + drm_dev_fini(dev); + kfree(dev); + + /* + * Free now, as this release could be not due to rmmod, but + * due to the backend disconnect, making drm_info hang in + * memory until rmmod + */ + devm_kfre
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On Tue, Mar 27, 2018 at 11:34 AM, Oleksandr Andrushchenko wrote: > Hi, Daniel! > > > On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote: >> >> On 03/26/2018 11:18 AM, Daniel Vetter wrote: >>> >>> On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote: > > My apologies, but I found a few more things that look strange and > should > be cleaned up. Sorry for this iterative review approach, but I think > we're > slowly getting there. Thank you for reviewing! > > Cheers, Daniel > >> --- >> +static int xen_drm_drv_dumb_create(struct drm_file *filp, >> +struct drm_device *dev, struct drm_mode_create_dumb *args) >> +{ >> +struct xen_drm_front_drm_info *drm_info = dev->dev_private; >> +struct drm_gem_object *obj; >> +int ret; >> + >> +ret = xen_drm_front_gem_dumb_create(filp, dev, args); >> +if (ret) >> +goto fail; >> + >> +obj = drm_gem_object_lookup(filp, args->handle); >> +if (!obj) { >> +ret = -ENOENT; >> +goto fail_destroy; >> +} >> + >> +drm_gem_object_unreference_unlocked(obj); > > You can't drop the reference while you keep using the object, someone > else > might sneak in and destroy your object. The unreference always must be > last. Will fix, thank you >> >> + >> +/* >> + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed >> + * via DRM CMA helpers and doesn't have ->pages allocated >> + * (xendrm_gem_get_pages will return NULL), but instead can >> provide >> + * sg table >> + */ >> +if (xen_drm_front_gem_get_pages(obj)) >> +ret = xen_drm_front_dbuf_create_from_pages( >> +drm_info->front_info, >> +xen_drm_front_dbuf_to_cookie(obj), >> +args->width, args->height, args->bpp, >> +args->size, >> +xen_drm_front_gem_get_pages(obj)); >> +else >> +ret = xen_drm_front_dbuf_create_from_sgt( >> +drm_info->front_info, >> +xen_drm_front_dbuf_to_cookie(obj), >> +args->width, args->height, args->bpp, >> +args->size, >> +xen_drm_front_gem_get_sg_table(obj)); >> +if (ret) >> +goto fail_destroy; >> + > > The above also has another race: If you construct an object, then it > must > be fully constructed by the time you publish it to the wider world. In > gem > this is done by calling drm_gem_handle_create() - after that userspace > can > get at your object and do nasty things with it in a separate thread, > forcing your driver to Oops if the object isn't fully constructed yet. > > That means you need to redo this code here to make sure that the gem > object is fully set up (including pages and sg tables) _before_ > anything > calls drm_gem_handle_create(). You are correct, I need to rework this code > > This probably means you also need to open-code the cma side, by first > calling drm_gem_cma_create(), then doing any additional setup, and > finally > doing the registration to userspace with drm_gem_handle_create as the > very > last thing. Although I tend to avoid open-coding, but this seems the necessary measure here > > Alternativet is to do the pages/sg setup only when you create an fb > (and > drop the pages again when the fb is destroyed), but that requires some > refcounting/locking in the driver. Not sure this will work: nothing prevents you from attaching multiple FBs to a single dumb handle So, not only ref-counting should be done here, but I also need to check if the dumb buffer, we are attaching to, has been created already >>> >>> No, you must make sure that no dumb buffer can be seen by anyone else >>> before it's fully created. If you don't register it in the file_priv idr >>> using drm_gem_handle_create, no one else can get at your buffer. Trying >>> to >>> paper over this race from all the other places breaks the gem core code >>> design, and is also much more fragile. >> >> Yes, this is what I implement now, e.g. I do not create >> any dumb handle until GEM is fully created. I was just >> saying that alternative way when we do pages/sgt on FB >> attach will not work in my case So, I will rework with open-coding some stuff from CMA helpers > Aside: There's still a lot of indirection and jumping around which > makes > the code a bit hard to follow. Probably I am not sure of which indirection we are talking about, could you please specifically mark those annoying you? >>> >>> I think it's the same indirection we talked a
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/27/2018 12:50 PM, Daniel Vetter wrote: On Tue, Mar 27, 2018 at 11:34 AM, Oleksandr Andrushchenko wrote: Hi, Daniel! On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote: On 03/26/2018 11:18 AM, Daniel Vetter wrote: On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote: My apologies, but I found a few more things that look strange and should be cleaned up. Sorry for this iterative review approach, but I think we're slowly getting there. Thank you for reviewing! Cheers, Daniel --- +static int xen_drm_drv_dumb_create(struct drm_file *filp, +struct drm_device *dev, struct drm_mode_create_dumb *args) +{ +struct xen_drm_front_drm_info *drm_info = dev->dev_private; +struct drm_gem_object *obj; +int ret; + +ret = xen_drm_front_gem_dumb_create(filp, dev, args); +if (ret) +goto fail; + +obj = drm_gem_object_lookup(filp, args->handle); +if (!obj) { +ret = -ENOENT; +goto fail_destroy; +} + +drm_gem_object_unreference_unlocked(obj); You can't drop the reference while you keep using the object, someone else might sneak in and destroy your object. The unreference always must be last. Will fix, thank you + +/* + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed + * via DRM CMA helpers and doesn't have ->pages allocated + * (xendrm_gem_get_pages will return NULL), but instead can provide + * sg table + */ +if (xen_drm_front_gem_get_pages(obj)) +ret = xen_drm_front_dbuf_create_from_pages( +drm_info->front_info, +xen_drm_front_dbuf_to_cookie(obj), +args->width, args->height, args->bpp, +args->size, +xen_drm_front_gem_get_pages(obj)); +else +ret = xen_drm_front_dbuf_create_from_sgt( +drm_info->front_info, +xen_drm_front_dbuf_to_cookie(obj), +args->width, args->height, args->bpp, +args->size, +xen_drm_front_gem_get_sg_table(obj)); +if (ret) +goto fail_destroy; + The above also has another race: If you construct an object, then it must be fully constructed by the time you publish it to the wider world. In gem this is done by calling drm_gem_handle_create() - after that userspace can get at your object and do nasty things with it in a separate thread, forcing your driver to Oops if the object isn't fully constructed yet. That means you need to redo this code here to make sure that the gem object is fully set up (including pages and sg tables) _before_ anything calls drm_gem_handle_create(). You are correct, I need to rework this code This probably means you also need to open-code the cma side, by first calling drm_gem_cma_create(), then doing any additional setup, and finally doing the registration to userspace with drm_gem_handle_create as the very last thing. Although I tend to avoid open-coding, but this seems the necessary measure here Alternativet is to do the pages/sg setup only when you create an fb (and drop the pages again when the fb is destroyed), but that requires some refcounting/locking in the driver. Not sure this will work: nothing prevents you from attaching multiple FBs to a single dumb handle So, not only ref-counting should be done here, but I also need to check if the dumb buffer, we are attaching to, has been created already No, you must make sure that no dumb buffer can be seen by anyone else before it's fully created. If you don't register it in the file_priv idr using drm_gem_handle_create, no one else can get at your buffer. Trying to paper over this race from all the other places breaks the gem core code design, and is also much more fragile. Yes, this is what I implement now, e.g. I do not create any dumb handle until GEM is fully created. I was just saying that alternative way when we do pages/sgt on FB attach will not work in my case So, I will rework with open-coding some stuff from CMA helpers Aside: There's still a lot of indirection and jumping around which makes the code a bit hard to follow. Probably I am not sure of which indirection we are talking about, could you please specifically mark those annoying you? I think it's the same indirection we talked about last time, it still annoys me. But it's still ok if you prefer this way I think :-) Ok, probably this is because I'm looking at the driver from an editor, but you are from your mail client ;) + +static void xen_drm_drv_release(struct drm_device *dev) +{ +struct xen_drm_front_drm_info *drm_info = dev->dev_private; +struct xen_drm_front_info *front_info = drm_info->front_info; + +drm_atomic_helper_shutdown(dev); +drm_mode_config_cleanup(dev); + +xen_drm_front_evtchnl_free_all(front_info); +dbuf_free_all(&front_info->dbuf_list); + +drm_dev_fini(dev); +kfree(dev); + +/* + * Free now, as this release could be not due to rmmod, but + * due to the ba