[RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Ryan Mallon]

The assignment of handle in vmw_framebuffer_create_handle doesn't
actually do anything useful and is incorrectly assigning an integer
value to a pointer argument. It appears that this is a typo and should
be dereferencing handle rather than assigning to it directly.
    
Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 0af6ebd..b66ef0e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct drm_framebuffer *fb,
 				  unsigned int *handle)
 {
 	if (handle)
-		handle = 0;
+		*handle = 0;
 
 	return 0;
 }

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Thomas Hellstrom]

Ryan,

Thanks for pointing this out. Unfortunately there  seems to be two bugs 
here, the one you pointed out and the fact that we set the handle to zero,
when it probably should be set to struct vmw_framebuffer::user_handle.

Jakob, can you comment on this?

/Thomas

On 01/27/2012 07:25 AM, Ryan Mallon wrote:
> The assignment of handle in vmw_framebuffer_create_handle doesn't
> actually do anything useful and is incorrectly assigning an integer
> value to a pointer argument. It appears that this is a typo and should
> be dereferencing handle rather than assigning to it directly.
>
> Signed-off-by: Ryan Mallon<rmallon@gmail.com>
> ---
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 0af6ebd..b66ef0e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct drm_framebuffer *fb,
>   				  unsigned int *handle)
>   {
>   	if (handle)
> -		handle = 0;
> +		*handle = 0;
>
>   	return 0;
>   }
>

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Jakob Bornecrantz]

I was asking around and this seems to only be used by X when it
starts and we want to preserve the contents of the screen. That
feature is implemented by the X driver. So we need to figure how we
want to solve it.

Either way this fix should probably go into this RC series, not
sure if we need to send this to stable, since we are not leaking
data to userspace (check drm_mode_getfb), but we might as well.

Reviewed-by: Jakob Bornecrantz <jakob@vmware.com>

Cheers, Jakob.

----- Original Message -----
> Ryan,
> 
> Thanks for pointing this out. Unfortunately there  seems to be two
> bugs here, the one you pointed out and the fact that we set the handle
> to zero, when it probably should be set to struct
> vmw_framebuffer::user_handle.
> 
> Jakob, can you comment on this?
> 
> /Thomas
> 
> On 01/27/2012 07:25 AM, Ryan Mallon wrote:
> > The assignment of handle in vmw_framebuffer_create_handle doesn't
> > actually do anything useful and is incorrectly assigning an integer
> > value to a pointer argument. It appears that this is a typo and
> > should
> > be dereferencing handle rather than assigning to it directly.
> >
> > Signed-off-by: Ryan Mallon<rmallon@gmail.com>
> > ---
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > index 0af6ebd..b66ef0e 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct
> > drm_framebuffer *fb,
> >   				  unsigned int *handle)
> >   {
> >   	if (handle)
> > -		handle = 0;
> > +		*handle = 0;
> >
> >   	return 0;
> >   }
> >
> 
> 

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Thomas Hellstrom]

On 01/27/2012 03:24 PM, Jakob Bornecrantz wrote:
> I was asking around and this seems to only be used by X when it
> starts and we want to preserve the contents of the screen. That
> feature is implemented by the X driver. So we need to figure how we
> want to solve it.
>
> Either way this fix should probably go into this RC series, not
> sure if we need to send this to stable, since we are not leaking
> data to userspace (check drm_mode_getfb), but we might as well.
>
> Reviewed-by: Jakob Bornecrantz<jakob@vmware.com>

But shouldn't we return the *real* handle. Not 0??

/Thomas




>
> Cheers, Jakob.
>
> ----- Original Message -----
>> Ryan,
>>
>> Thanks for pointing this out. Unfortunately there  seems to be two
>> bugs here, the one you pointed out and the fact that we set the handle
>> to zero, when it probably should be set to struct
>> vmw_framebuffer::user_handle.
>>
>> Jakob, can you comment on this?
>>
>> /Thomas
>>
>> On 01/27/2012 07:25 AM, Ryan Mallon wrote:
>>> The assignment of handle in vmw_framebuffer_create_handle doesn't
>>> actually do anything useful and is incorrectly assigning an integer
>>> value to a pointer argument. It appears that this is a typo and
>>> should
>>> be dereferencing handle rather than assigning to it directly.
>>>
>>> Signed-off-by: Ryan Mallon<rmallon@gmail.com>
>>> ---
>>>
>>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>>> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>>> index 0af6ebd..b66ef0e 100644
>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>>> @@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct
>>> drm_framebuffer *fb,
>>>    				  unsigned int *handle)
>>>    {
>>>    	if (handle)
>>> -		handle = 0;
>>> +		*handle = 0;
>>>
>>>    	return 0;
>>>    }
>>>
>>

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Jakob Bornecrantz]

----- Original Message -----
> On 01/27/2012 03:24 PM, Jakob Bornecrantz wrote:
> > I was asking around and this seems to only be used by X when it
> > starts and we want to preserve the contents of the screen. That
> > feature is implemented by the X driver. So we need to figure how we
> > want to solve it.
> >
> > Either way this fix should probably go into this RC series, not
> > sure if we need to send this to stable, since we are not leaking
> > data to userspace (check drm_mode_getfb), but we might as well.
> >
> > Reviewed-by: Jakob Bornecrantz<jakob@vmware.com>
> 
> But shouldn't we return the *real* handle. Not 0??

Yeah, you are right. Not sure we have user_handle in the stable
kernels tho.

We need to change not only the X driver but also all the other
userspace components since at least libkms doesn't create shared
buffers. And if the thing that is running before X is using fbdev
this wont do much good either since fbdev is not backed by a
fb in the way other drivers do it.

Cheers, Jakob.

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Thomas Hellstrom]

On 01/27/2012 03:41 PM, Jakob Bornecrantz wrote:
> ----- Original Message -----
>> On 01/27/2012 03:24 PM, Jakob Bornecrantz wrote:
>>> I was asking around and this seems to only be used by X when it
>>> starts and we want to preserve the contents of the screen. That
>>> feature is implemented by the X driver. So we need to figure how we
>>> want to solve it.
>>>
>>> Either way this fix should probably go into this RC series, not
>>> sure if we need to send this to stable, since we are not leaking
>>> data to userspace (check drm_mode_getfb), but we might as well.
>>>
>>> Reviewed-by: Jakob Bornecrantz<jakob@vmware.com>
>> But shouldn't we return the *real* handle. Not 0??
> Yeah, you are right. Not sure we have user_handle in the stable
> kernels tho.
We do AFAICT.

>
> We need to change not only the X driver but also all the other
> userspace components since at least libkms doesn't create shared
> buffers.
Libkms is not used anymore, at least not in the X server driver.

>   And if the thing that is running before X is using fbdev
> this wont do much good either since fbdev is not backed by a
> fb in the way other drivers do it.
OK. Then I suggest we just return 0 here, and go ahead with the current 
patch, and
when we've figured out how to do the handover, we bump kernel minor and
return a proper handle. Does that sound OK?

/Thomas


>
> Cheers, Jakob.

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Jakob Bornecrantz]

----- Original Message -----
> On 01/27/2012 03:41 PM, Jakob Bornecrantz wrote:
> > ----- Original Message -----
> >> On 01/27/2012 03:24 PM, Jakob Bornecrantz wrote:
> >>> I was asking around and this seems to only be used by X when it
> >>> starts and we want to preserve the contents of the screen. That
> >>> feature is implemented by the X driver. So we need to figure how
> >>> we
> >>> want to solve it.
> >>>
> >>> Either way this fix should probably go into this RC series, not
> >>> sure if we need to send this to stable, since we are not leaking
> >>> data to userspace (check drm_mode_getfb), but we might as well.
> >>>
> >>> Reviewed-by: Jakob Bornecrantz<jakob@vmware.com>
> >> But shouldn't we return the *real* handle. Not 0??
> >
> > Yeah, you are right. Not sure we have user_handle in the stable
> > kernels tho.
>
> We do AFAICT.
> 
> >
> > We need to change not only the X driver but also all the other
> > userspace components since at least libkms doesn't create shared
> > buffers.
>
> Libkms is not used anymore, at least not in the X server driver.

I was more referring to Plymouth and the like.

> >   And if the thing that is running before X is using fbdev
> > this wont do much good either since fbdev is not backed by a
> > fb in the way other drivers do it.
>
> OK. Then I suggest we just return 0 here, and go ahead with the
> current patch, and when we've figured out how to do the handover,
> we bump kernel minor and return a proper handle. Does that sound
> OK?

Sounds good to me.

Cheers, Jakob.

Re: [RFC PATCH] vmwgfx: Fix assignment in vmw_framebuffer_create_handle

[Thomas Hellstrom]

On 01/27/2012 07:25 AM, Ryan Mallon wrote:
> The assignment of handle in vmw_framebuffer_create_handle doesn't
> actually do anything useful and is incorrectly assigning an integer
> value to a pointer argument. It appears that this is a typo and should
> be dereferencing handle rather than assigning to it directly.
>
> Signed-off-by: Ryan Mallon<rmallon@gmail.com>
> ---
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 0af6ebd..b66ef0e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -378,7 +378,7 @@ int vmw_framebuffer_create_handle(struct drm_framebuffer *fb,
>   				  unsigned int *handle)
>   {
>   	if (handle)
> -		handle = 0;
> +		*handle = 0;
>
>   	return 0;
>   }
>

Ryan,
Please repost this patch with Jakobs Reviewed-by: and below that a
Cc: stable@vger.kernel.org

Thanks,
Thomas