diff mbox series

[v2] drm/gem-shmem: When drm_gem_object_init failed, should release object

Message ID 20221111033817.366-1-tangchunyou@163.com
State New
Headers
Series [v2] drm/gem-shmem: When drm_gem_object_init failed, should release object |

Commit Message

ChunyouTang Nov. 11, 2022, 3:38 a.m. UTC 
  when goto err_free, the object had init, so it should be release when fail.

Signed-off-by: ChunyouTang <tangchunyou@163.com>
---
 drivers/gpu/drm/drm_gem.c              | 19 ++++++++++++++++---
 drivers/gpu/drm/drm_gem_shmem_helper.c |  4 +++-
 include/drm/drm_gem.h                  |  1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

Comments

Thomas Zimmermann Nov. 17, 2022, 1:42 p.m. UTC | #1 
Hi

Am 11.11.22 um 04:38 schrieb ChunyouTang:
> when goto err_free, the object had init, so it should be release when fail.
> 
> Signed-off-by: ChunyouTang <tangchunyou@163.com>
> ---
>   drivers/gpu/drm/drm_gem.c              | 19 ++++++++++++++++---
>   drivers/gpu/drm/drm_gem_shmem_helper.c |  4 +++-
>   include/drm/drm_gem.h                  |  1 +
>   3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8b68a3c1e6ab..cba32c46bb05 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -169,6 +169,21 @@ void drm_gem_private_object_init(struct drm_device *dev,
>   }
>   EXPORT_SYMBOL(drm_gem_private_object_init);
>   
> +/**
> + * drm_gem_private_object_fini - Finalize a failed drm_gem_object
> + * @obj: drm_gem_object
> + *
> + * Uninitialize an already allocated GEM object when it initialized failed
> + */
> +void drm_gem_private_object_fini(struct drm_gem_object *obj)
> +{
> +	WARN_ON(obj->dma_buf);

Rather lease this in its original place.

> +
> +	dma_resv_fini(&obj->_resv);
> +	drm_gem_lru_remove(obj);

AFAICT drm_gem_lru_remove() doesn't belong into this function.

> +}
> +EXPORT_SYMBOL(drm_gem_private_object_fini);
> +
>   /**
>    * drm_gem_object_handle_free - release resources bound to userspace handles
>    * @obj: GEM object to clean up.
> @@ -930,14 +945,12 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
>   void
>   drm_gem_object_release(struct drm_gem_object *obj)
>   {
> -	WARN_ON(obj->dma_buf);
> +	drm_gem_private_object_fini(obj);
>   
>   	if (obj->filp)
>   		fput(obj->filp);
>   
> -	dma_resv_fini(&obj->_resv);

Please call drm_gem_private_object_fini() here.

>   	drm_gem_free_mmap_offset(obj);
> -	drm_gem_lru_remove(obj);

Please keep this line here.

Best regards
Thomas

>   }
>   EXPORT_SYMBOL(drm_gem_object_release);
>   
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 35138f8a375c..845e3d5d71eb 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -79,8 +79,10 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
>   	} else {
>   		ret = drm_gem_object_init(dev, obj, size);
>   	}
> -	if (ret)
> +	if (ret) {
> +		drm_gem_private_object_fini(obj)
>   		goto err_free;
> +	}
>   
>   	ret = drm_gem_create_mmap_offset(obj);
>   	if (ret)
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index bd42f25e449c..9b1feb03069d 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -405,6 +405,7 @@ int drm_gem_object_init(struct drm_device *dev,
>   			struct drm_gem_object *obj, size_t size);
>   void drm_gem_private_object_init(struct drm_device *dev,
>   				 struct drm_gem_object *obj, size_t size);
> +void drm_gem_private_object_fini(struct drm_gem_object *obj);
>   void drm_gem_vm_open(struct vm_area_struct *vma);
>   void drm_gem_vm_close(struct vm_area_struct *vma);
>   int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
ChunyouTang Nov. 18, 2022, 10:32 a.m. UTC | #2 
Hi Thomas,
   Can I discard the first two patchs, and pull the new code, then
   modify and git send-email this patch?


于 Thu, 17 Nov 2022 14:42:36 +0100
Thomas Zimmermann <tzimmermann@suse.de> 写道:

> Hi
> 
> Am 11.11.22 um 04:38 schrieb ChunyouTang:
> > when goto err_free, the object had init, so it should be release
> > when fail.
> > 
> > Signed-off-by: ChunyouTang <tangchunyou@163.com>
> > ---
> >   drivers/gpu/drm/drm_gem.c              | 19 ++++++++++++++++---
> >   drivers/gpu/drm/drm_gem_shmem_helper.c |  4 +++-
> >   include/drm/drm_gem.h                  |  1 +
> >   3 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 8b68a3c1e6ab..cba32c46bb05 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -169,6 +169,21 @@ void drm_gem_private_object_init(struct
> > drm_device *dev, }
> >   EXPORT_SYMBOL(drm_gem_private_object_init);
> >   
> > +/**
> > + * drm_gem_private_object_fini - Finalize a failed drm_gem_object
> > + * @obj: drm_gem_object
> > + *
> > + * Uninitialize an already allocated GEM object when it
> > initialized failed
> > + */
> > +void drm_gem_private_object_fini(struct drm_gem_object *obj)
> > +{
> > +	WARN_ON(obj->dma_buf);
> 
> Rather lease this in its original place.
> 
> > +
> > +	dma_resv_fini(&obj->_resv);
> > +	drm_gem_lru_remove(obj);
> 
> AFAICT drm_gem_lru_remove() doesn't belong into this function.
> 
> > +}
> > +EXPORT_SYMBOL(drm_gem_private_object_fini);
> > +
> >   /**
> >    * drm_gem_object_handle_free - release resources bound to
> > userspace handles
> >    * @obj: GEM object to clean up.
> > @@ -930,14 +945,12 @@ drm_gem_release(struct drm_device *dev,
> > struct drm_file *file_private) void
> >   drm_gem_object_release(struct drm_gem_object *obj)
> >   {
> > -	WARN_ON(obj->dma_buf);
> > +	drm_gem_private_object_fini(obj);
> >   
> >   	if (obj->filp)
> >   		fput(obj->filp);
> >   
> > -	dma_resv_fini(&obj->_resv);
> 
> Please call drm_gem_private_object_fini() here.
> 
> >   	drm_gem_free_mmap_offset(obj);
> > -	drm_gem_lru_remove(obj);
> 
> Please keep this line here.
> 
> Best regards
> Thomas
> 
> >   }
> >   EXPORT_SYMBOL(drm_gem_object_release);
> >   
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c index
> > 35138f8a375c..845e3d5d71eb 100644 ---
> > a/drivers/gpu/drm/drm_gem_shmem_helper.c +++
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -79,8 +79,10 @@
> > __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool
> > private) } else { ret = drm_gem_object_init(dev, obj, size);
> >   	}
> > -	if (ret)
> > +	if (ret) {
> > +		drm_gem_private_object_fini(obj)
> >   		goto err_free;
> > +	}
> >   
> >   	ret = drm_gem_create_mmap_offset(obj);
> >   	if (ret)
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index bd42f25e449c..9b1feb03069d 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -405,6 +405,7 @@ int drm_gem_object_init(struct drm_device *dev,
> >   			struct drm_gem_object *obj, size_t size);
> >   void drm_gem_private_object_init(struct drm_device *dev,
> >   				 struct drm_gem_object *obj,
> > size_t size); +void drm_gem_private_object_fini(struct
> > drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct
> > *vma); void drm_gem_vm_close(struct vm_area_struct *vma);
> >   int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long
> > obj_size,
>
Thomas Zimmermann Nov. 18, 2022, 11:21 a.m. UTC | #3 
Hi

Am 18.11.22 um 11:32 schrieb Chunyou Tang:
> Hi Thomas,
>     Can I discard the first two patchs, and pull the new code, then
>     modify and git send-email this patch?

Yes, of course. Just make sure that the next version is marked as v3, so 
it's obvious what it belongs to.

Best regards
Thomas

> 
> 
> 于 Thu, 17 Nov 2022 14:42:36 +0100
> Thomas Zimmermann <tzimmermann@suse.de> 写道:
> 
>> Hi
>>
>> Am 11.11.22 um 04:38 schrieb ChunyouTang:
>>> when goto err_free, the object had init, so it should be release
>>> when fail.
>>>
>>> Signed-off-by: ChunyouTang <tangchunyou@163.com>
>>> ---
>>>    drivers/gpu/drm/drm_gem.c              | 19 ++++++++++++++++---
>>>    drivers/gpu/drm/drm_gem_shmem_helper.c |  4 +++-
>>>    include/drm/drm_gem.h                  |  1 +
>>>    3 files changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 8b68a3c1e6ab..cba32c46bb05 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -169,6 +169,21 @@ void drm_gem_private_object_init(struct
>>> drm_device *dev, }
>>>    EXPORT_SYMBOL(drm_gem_private_object_init);
>>>    
>>> +/**
>>> + * drm_gem_private_object_fini - Finalize a failed drm_gem_object
>>> + * @obj: drm_gem_object
>>> + *
>>> + * Uninitialize an already allocated GEM object when it
>>> initialized failed
>>> + */
>>> +void drm_gem_private_object_fini(struct drm_gem_object *obj)
>>> +{
>>> +	WARN_ON(obj->dma_buf);
>>
>> Rather lease this in its original place.
>>
>>> +
>>> +	dma_resv_fini(&obj->_resv);
>>> +	drm_gem_lru_remove(obj);
>>
>> AFAICT drm_gem_lru_remove() doesn't belong into this function.
>>
>>> +}
>>> +EXPORT_SYMBOL(drm_gem_private_object_fini);
>>> +
>>>    /**
>>>     * drm_gem_object_handle_free - release resources bound to
>>> userspace handles
>>>     * @obj: GEM object to clean up.
>>> @@ -930,14 +945,12 @@ drm_gem_release(struct drm_device *dev,
>>> struct drm_file *file_private) void
>>>    drm_gem_object_release(struct drm_gem_object *obj)
>>>    {
>>> -	WARN_ON(obj->dma_buf);
>>> +	drm_gem_private_object_fini(obj);
>>>    
>>>    	if (obj->filp)
>>>    		fput(obj->filp);
>>>    
>>> -	dma_resv_fini(&obj->_resv);
>>
>> Please call drm_gem_private_object_fini() here.
>>
>>>    	drm_gem_free_mmap_offset(obj);
>>> -	drm_gem_lru_remove(obj);
>>
>> Please keep this line here.
>>
>> Best regards
>> Thomas
>>
>>>    }
>>>    EXPORT_SYMBOL(drm_gem_object_release);
>>>    
>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> b/drivers/gpu/drm/drm_gem_shmem_helper.c index
>>> 35138f8a375c..845e3d5d71eb 100644 ---
>>> a/drivers/gpu/drm/drm_gem_shmem_helper.c +++
>>> b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -79,8 +79,10 @@
>>> __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool
>>> private) } else { ret = drm_gem_object_init(dev, obj, size);
>>>    	}
>>> -	if (ret)
>>> +	if (ret) {
>>> +		drm_gem_private_object_fini(obj)
>>>    		goto err_free;
>>> +	}
>>>    
>>>    	ret = drm_gem_create_mmap_offset(obj);
>>>    	if (ret)
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index bd42f25e449c..9b1feb03069d 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -405,6 +405,7 @@ int drm_gem_object_init(struct drm_device *dev,
>>>    			struct drm_gem_object *obj, size_t size);
>>>    void drm_gem_private_object_init(struct drm_device *dev,
>>>    				 struct drm_gem_object *obj,
>>> size_t size); +void drm_gem_private_object_fini(struct
>>> drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct
>>> *vma); void drm_gem_vm_close(struct vm_area_struct *vma);
>>>    int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long
>>> obj_size,
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 8b68a3c1e6ab..cba32c46bb05 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -169,6 +169,21 @@  void drm_gem_private_object_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_gem_private_object_init);
 
+/**
+ * drm_gem_private_object_fini - Finalize a failed drm_gem_object
+ * @obj: drm_gem_object
+ *
+ * Uninitialize an already allocated GEM object when it initialized failed
+ */
+void drm_gem_private_object_fini(struct drm_gem_object *obj)
+{
+	WARN_ON(obj->dma_buf);
+
+	dma_resv_fini(&obj->_resv);
+	drm_gem_lru_remove(obj);
+}
+EXPORT_SYMBOL(drm_gem_private_object_fini);
+
 /**
  * drm_gem_object_handle_free - release resources bound to userspace handles
  * @obj: GEM object to clean up.
@@ -930,14 +945,12 @@  drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 void
 drm_gem_object_release(struct drm_gem_object *obj)
 {
-	WARN_ON(obj->dma_buf);
+	drm_gem_private_object_fini(obj);
 
 	if (obj->filp)
 		fput(obj->filp);
 
-	dma_resv_fini(&obj->_resv);
 	drm_gem_free_mmap_offset(obj);
-	drm_gem_lru_remove(obj);
 }
 EXPORT_SYMBOL(drm_gem_object_release);
 
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 35138f8a375c..845e3d5d71eb 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -79,8 +79,10 @@  __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
 	} else {
 		ret = drm_gem_object_init(dev, obj, size);
 	}
-	if (ret)
+	if (ret) {
+		drm_gem_private_object_fini(obj)
 		goto err_free;
+	}
 
 	ret = drm_gem_create_mmap_offset(obj);
 	if (ret)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index bd42f25e449c..9b1feb03069d 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -405,6 +405,7 @@  int drm_gem_object_init(struct drm_device *dev,
 			struct drm_gem_object *obj, size_t size);
 void drm_gem_private_object_init(struct drm_device *dev,
 				 struct drm_gem_object *obj, size_t size);
+void drm_gem_private_object_fini(struct drm_gem_object *obj);
 void drm_gem_vm_open(struct vm_area_struct *vma);
 void drm_gem_vm_close(struct vm_area_struct *vma);
 int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,