diff mbox series

[01/22] drm: Add drm_mode_init()

Message ID 20220218100403.7028-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm: Review of mode copies | expand

Commit Message

Ville Syrjälä Feb. 18, 2022, 10:03 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a variant of drm_mode_copy() that explicitly clears out
the list head of the destination mode. Helpful to guarantee
we don't have stack garbage left in there for on-stack modes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++
 include/drm/drm_modes.h     |  2 ++
 2 files changed, 19 insertions(+)

Comments

Andrzej Hajda Feb. 18, 2022, 11:22 a.m. UTC | #1 
On 18.02.2022 11:03, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add a variant of drm_mode_copy() that explicitly clears out
> the list head of the destination mode. Helpful to guarantee
> we don't have stack garbage left in there for on-stack modes.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++
>   include/drm/drm_modes.h     |  2 ++
>   2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 96b13e36293c..40d4ce4a1da4 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *
>   }
>   EXPORT_SYMBOL(drm_mode_copy);
>   
> +/**
> + * drm_mode_init - initialize the mode from another mode
> + * @dst: mode to overwrite
> + * @src: mode to copy
> + *
> + * Copy an existing mode into another mode, zeroing the
> + * list head of the destination mode. Typically used
> + * to guarantee the list head is not left with stack
> + * garbage in on-stack modes.
> + */
> +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src)
> +{
> +	memset(dst, 0, sizeof(*dst));

Why not just clear the list head? Or maybe poison it? It would be more 
cleaner.

I wonder why there is no such helper in list.h.


Regards
Andrzej

> +	drm_mode_copy(dst, src);
> +}
> +EXPORT_SYMBOL(drm_mode_init);
> +
>   /**
>    * drm_mode_duplicate - allocate and duplicate an existing mode
>    * @dev: drm_device to allocate the duplicated mode for
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 29ba4adf0c53..e6e5a588fab1 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p,
>   			   int adjust_flags);
>   void drm_mode_copy(struct drm_display_mode *dst,
>   		   const struct drm_display_mode *src);
> +void drm_mode_init(struct drm_display_mode *dst,
> +		   const struct drm_display_mode *src);
>   struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
>   					    const struct drm_display_mode *mode);
>   bool drm_mode_match(const struct drm_display_mode *mode1,
Ville Syrjälä Feb. 18, 2022, 11:56 a.m. UTC | #2 
On Fri, Feb 18, 2022 at 12:22:44PM +0100, Andrzej Hajda wrote:
> 
> 
> On 18.02.2022 11:03, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add a variant of drm_mode_copy() that explicitly clears out
> > the list head of the destination mode. Helpful to guarantee
> > we don't have stack garbage left in there for on-stack modes.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++
> >   include/drm/drm_modes.h     |  2 ++
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 96b13e36293c..40d4ce4a1da4 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *
> >   }
> >   EXPORT_SYMBOL(drm_mode_copy);
> >   
> > +/**
> > + * drm_mode_init - initialize the mode from another mode
> > + * @dst: mode to overwrite
> > + * @src: mode to copy
> > + *
> > + * Copy an existing mode into another mode, zeroing the
> > + * list head of the destination mode. Typically used
> > + * to guarantee the list head is not left with stack
> > + * garbage in on-stack modes.
> > + */
> > +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src)
> > +{
> > +	memset(dst, 0, sizeof(*dst));
> 
> Why not just clear the list head? Or maybe poison it? It would be more 
> cleaner.

Then we have two places that need to be updated if some other field
gets introduced that needs preserving. With a full memset() we only
have to care about drm_mode_copy(). Don't see much point in
micro-optimizing this thing.
Andrzej Hajda Feb. 18, 2022, 12:23 p.m. UTC | #3 
On 18.02.2022 12:56, Ville Syrjälä wrote:
> On Fri, Feb 18, 2022 at 12:22:44PM +0100, Andrzej Hajda wrote:
>>
>> On 18.02.2022 11:03, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Add a variant of drm_mode_copy() that explicitly clears out
>>> the list head of the destination mode. Helpful to guarantee
>>> we don't have stack garbage left in there for on-stack modes.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++
>>>    include/drm/drm_modes.h     |  2 ++
>>>    2 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>> index 96b13e36293c..40d4ce4a1da4 100644
>>> --- a/drivers/gpu/drm/drm_modes.c
>>> +++ b/drivers/gpu/drm/drm_modes.c
>>> @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *
>>>    }
>>>    EXPORT_SYMBOL(drm_mode_copy);
>>>    
>>> +/**
>>> + * drm_mode_init - initialize the mode from another mode
>>> + * @dst: mode to overwrite
>>> + * @src: mode to copy
>>> + *
>>> + * Copy an existing mode into another mode, zeroing the
>>> + * list head of the destination mode. Typically used
>>> + * to guarantee the list head is not left with stack
>>> + * garbage in on-stack modes.
>>> + */
>>> +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src)
>>> +{
>>> +	memset(dst, 0, sizeof(*dst));
>> Why not just clear the list head? Or maybe poison it? It would be more
>> cleaner.
> Then we have two places that need to be updated if some other field
> gets introduced that needs preserving. With a full memset() we only
> have to care about drm_mode_copy(). Don't see much point in
> micro-optimizing this thing.
>
In such case DOC should be modified to avoid updating it "if some other 
field..." :)

Anyway:
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej
Harry Wentland Feb. 18, 2022, 4:34 p.m. UTC | #4 
On 2022-02-18 05:03, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a variant of drm_mode_copy() that explicitly clears out
> the list head of the destination mode. Helpful to guarantee
> we don't have stack garbage left in there for on-stack modes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/drm_modes.c | 17 +++++++++++++++++
>  include/drm/drm_modes.h     |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 96b13e36293c..40d4ce4a1da4 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *
>  }
>  EXPORT_SYMBOL(drm_mode_copy);
>  
> +/**
> + * drm_mode_init - initialize the mode from another mode
> + * @dst: mode to overwrite
> + * @src: mode to copy
> + *
> + * Copy an existing mode into another mode, zeroing the
> + * list head of the destination mode. Typically used
> + * to guarantee the list head is not left with stack
> + * garbage in on-stack modes.
> + */
> +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src)
> +{
> +	memset(dst, 0, sizeof(*dst));
> +	drm_mode_copy(dst, src);
> +}
> +EXPORT_SYMBOL(drm_mode_init);
> +
>  /**
>   * drm_mode_duplicate - allocate and duplicate an existing mode
>   * @dev: drm_device to allocate the duplicated mode for
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 29ba4adf0c53..e6e5a588fab1 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p,
>  			   int adjust_flags);
>  void drm_mode_copy(struct drm_display_mode *dst,
>  		   const struct drm_display_mode *src);
> +void drm_mode_init(struct drm_display_mode *dst,
> +		   const struct drm_display_mode *src);
>  struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
>  					    const struct drm_display_mode *mode);
>  bool drm_mode_match(const struct drm_display_mode *mode1,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 96b13e36293c..40d4ce4a1da4 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -892,6 +892,23 @@  void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *
 }
 EXPORT_SYMBOL(drm_mode_copy);
 
+/**
+ * drm_mode_init - initialize the mode from another mode
+ * @dst: mode to overwrite
+ * @src: mode to copy
+ *
+ * Copy an existing mode into another mode, zeroing the
+ * list head of the destination mode. Typically used
+ * to guarantee the list head is not left with stack
+ * garbage in on-stack modes.
+ */
+void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src)
+{
+	memset(dst, 0, sizeof(*dst));
+	drm_mode_copy(dst, src);
+}
+EXPORT_SYMBOL(drm_mode_init);
+
 /**
  * drm_mode_duplicate - allocate and duplicate an existing mode
  * @dev: drm_device to allocate the duplicated mode for
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 29ba4adf0c53..e6e5a588fab1 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -484,6 +484,8 @@  void drm_mode_set_crtcinfo(struct drm_display_mode *p,
 			   int adjust_flags);
 void drm_mode_copy(struct drm_display_mode *dst,
 		   const struct drm_display_mode *src);
+void drm_mode_init(struct drm_display_mode *dst,
+		   const struct drm_display_mode *src);
 struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
 					    const struct drm_display_mode *mode);
 bool drm_mode_match(const struct drm_display_mode *mode1,