From: Joe Perches <joe@perches.com> Date: 2016-03-17 20:22:43
On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
2016-03-17 Gustavo Padovan [off-list ref]:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
quoted
This function had copies in 3 different files. Unify them in
kernel.h.
This is only used by gpu/drm.
I think this is a poor name for a generic function
that would be in kernel.h.
Isn't there an include file in linux/drm that's
appropriate for this. Maybe drmP.h
Maybe prefix this function name with drm_ too.
No, the next patch adds a user to drivers/staging (which will be moved
to drivers/dma-buf) soon. Maybe move to a different header in
include/linux/? not sure which one.
quoted
Also, there's this that might conflict:
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ptr_to_compat(p)
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ((unsigned long)(p))
Right, I'll figure out how to replace these two too.
The powerpc to_user_ptr has a different meaning from the one I'm adding
in this patch. I propose we just rename powerpc's to_user_ptr to
__to_user_ptr and leave the rest as is.
I think that's not a good idea, and you should really check
this concept with the powerpc folk (added to to:s and cc:ed)
If it were really added, then the function meaning is incorrect.
This is taking a u64, casting that to (unsigned long/uint_ptr_t),
then converting that to a user pointer.
Does that naming and use make sense on x86-32 or arm32?
From: Rob Clark <hidden> Date: 2016-03-17 20:33:49
On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches [off-list ref] wrote:
On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
quoted
2016-03-17 Gustavo Padovan [off-list ref]:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
quoted
This function had copies in 3 different files. Unify them in
kernel.h.
This is only used by gpu/drm.
I think this is a poor name for a generic function
that would be in kernel.h.
Isn't there an include file in linux/drm that's
appropriate for this. Maybe drmP.h
Maybe prefix this function name with drm_ too.
No, the next patch adds a user to drivers/staging (which will be moved
to drivers/dma-buf) soon. Maybe move to a different header in
include/linux/? not sure which one.
quoted
Also, there's this that might conflict:
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ptr_to_compat(p)
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ((unsigned long)(p))
Right, I'll figure out how to replace these two too.
The powerpc to_user_ptr has a different meaning from the one I'm adding
in this patch. I propose we just rename powerpc's to_user_ptr to
__to_user_ptr and leave the rest as is.
I think that's not a good idea, and you should really check
this concept with the powerpc folk (added to to:s and cc:ed)
If it were really added, then the function meaning is incorrect.
This is taking a u64, casting that to (unsigned long/uint_ptr_t),
then converting that to a user pointer.
Does that naming and use make sense on x86-32 or arm32?
fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
Not entirely sure what doesn't make sense about it
BR,
-R
From: Joe Perches <joe@perches.com> Date: 2016-03-17 20:40:18
On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches [off-list ref] wrote:
quoted
On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
quoted
2016-03-17 Gustavo Padovan [off-list ref]:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
quoted
This function had copies in 3 different files. Unify them in
kernel.h.
This is only used by gpu/drm.
I think this is a poor name for a generic function
that would be in kernel.h.
Isn't there an include file in linux/drm that's
appropriate for this. Maybe drmP.h
Maybe prefix this function name with drm_ too.
No, the next patch adds a user to drivers/staging (which will be moved
to drivers/dma-buf) soon. Maybe move to a different header in
include/linux/? not sure which one.
quoted
Also, there's this that might conflict:
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ptr_to_compat(p)
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ((unsigned long)(p))
Right, I'll figure out how to replace these two too.
The powerpc to_user_ptr has a different meaning from the one I'm adding
in this patch. I propose we just rename powerpc's to_user_ptr to
__to_user_ptr and leave the rest as is.
I think that's not a good idea, and you should really check
this concept with the powerpc folk (added to to:s and cc:ed)
If it were really added, then the function meaning is incorrect.
This is taking a u64, casting that to (unsigned long/uint_ptr_t),
then converting that to a user pointer.
Does that naming and use make sense on x86-32 or arm32?
fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
Not entirely sure what doesn't make sense about it
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
As a static function in a single file, it's not
great, but OK, fine, it's static.
As a global function in kernel.h, it's misleading.
From: Rob Clark <hidden> Date: 2016-03-17 20:50:21
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches [off-list ref] wrote:
quoted
On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
quoted
2016-03-17 Gustavo Padovan [off-list ref]:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
quoted
This function had copies in 3 different files. Unify them in
kernel.h.
This is only used by gpu/drm.
I think this is a poor name for a generic function
that would be in kernel.h.
Isn't there an include file in linux/drm that's
appropriate for this. Maybe drmP.h
Maybe prefix this function name with drm_ too.
No, the next patch adds a user to drivers/staging (which will be moved
to drivers/dma-buf) soon. Maybe move to a different header in
include/linux/? not sure which one.
quoted
Also, there's this that might conflict:
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ptr_to_compat(p)
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p) ((unsigned long)(p))
Right, I'll figure out how to replace these two too.
The powerpc to_user_ptr has a different meaning from the one I'm adding
in this patch. I propose we just rename powerpc's to_user_ptr to
__to_user_ptr and leave the rest as is.
I think that's not a good idea, and you should really check
this concept with the powerpc folk (added to to:s and cc:ed)
If it were really added, then the function meaning is incorrect.
This is taking a u64, casting that to (unsigned long/uint_ptr_t),
then converting that to a user pointer.
Does that naming and use make sense on x86-32 or arm32?
fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
Not entirely sure what doesn't make sense about it
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
ahh, ok. I guess I was used to using it in the context of ioctl
structs.. in that context u64 -> (void __user *) made more sense.
Maybe uapi_to_ptr()? (ok, not super-creative.. maybe someone has a better idea)
BR,
-R
As a static function in a single file, it's not
great, but OK, fine, it's static.
As a global function in kernel.h, it's misleading.
From: Joe Perches <joe@perches.com> Date: 2016-03-17 21:10:20
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
[]
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
ahh, ok. I guess I was used to using it in the context of ioctl
structs.. in that context u64 -> (void __user *) made more sense.
Maybe uapi_to_ptr()? (ok, not super-creative.. maybe someone has a
better idea)
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
[]
quoted
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
=20
static inline void __user *to_user_ptr(void *p)
{
=A0=A0=A0=A0=A0=A0=A0=A0return (void __user *)p;
}
ahh, ok.=A0=A0I guess I was used to using it in the context of ioctl
structs..=A0=A0in that context u64 -> (void __user *) made more sense.
=20
Maybe uapi_to_ptr()?=A0=A0(ok, not super-creative.. maybe someone has a
better idea)
=20
Maybe u64_to_user_ptr?
That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?
Gustavo
From: Rob Clark <hidden> Date: 2016-03-17 21:25:53
On Thu, Mar 17, 2016 at 5:19 PM, Gustavo Padovan [off-list ref] wrote:
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
[]
quoted
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
ahh, ok. I guess I was used to using it in the context of ioctl
structs.. in that context u64 -> (void __user *) made more sense.
Maybe uapi_to_ptr()? (ok, not super-creative.. maybe someone has a
better idea)
Maybe u64_to_user_ptr?
That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?
From: Joe Perches <joe@perches.com> Date: 2016-03-17 21:33:59
On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
[]
quoted
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
ahh, ok. I guess I was used to using it in the context of ioctl
structs.. in that context u64 -> (void __user *) made more sense.
Maybe uapi_to_ptr()? (ok, not super-creative.. maybe someone has a
better idea)
Maybe u64_to_user_ptr?
That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?
I've no particular opinion about location,
but maybe compat.h might be appropriate.
Maybe add all variants:
void __user *u32_to_user_ptr(u32 val)
void __user *u64_to_user_ptr(u64 val)
u32 user_ptr_to_u32(void __user *p)
u64 user_ptr_to_u64(void __user *p)
Maybe there's something about 32 bit userspace on
64 OS that should be done too.
On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrot=
e:
quoted
quoted
[]
quoted
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
=20
static inline void __user *to_user_ptr(void *p)
{
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return (void __us=
er *)p;
quoted
quoted
quoted
quoted
}
ahh, ok.=C2=A0=C2=A0I guess I was used to using it in the context o=
f ioctl
quoted
quoted
quoted
structs..=C2=A0=C2=A0in that context u64 -> (void __user *) made mo=
re sense.
quoted
quoted
quoted
=20
Maybe uapi_to_ptr()?=C2=A0=C2=A0(ok, not super-creative.. maybe som=
eone has a
quoted
quoted
quoted
better idea)
Maybe u64_to_user_ptr?
That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?
=20
I've no particular opinion about location,
but maybe compat.h might be appropriate.
I don't think this is really related to compat. I'd keep kernel.h.
The problem I'm trying to solve here is:
CC drivers/dma-buf/sync_file.o
drivers/dma-buf/sync_file.c: In function =E2=80=98sync_file_ioctl_fence_inf=
o=E2=80=99:
drivers/dma-buf/sync_file.c:341:19: warning: cast to pointer from
integer of different size [-Wint-to-pointer-cast]
if (copy_to_user((void __user *)info.sync_fence_info, fence_info,
where info.sync_fence_info is __u64.
Gustavo
From: Daniel Vetter <hidden> Date: 2016-03-18 08:23:19
On Thu, Mar 17, 2016 at 02:33:50PM -0700, Joe Perches wrote:
On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
quoted
2016-03-17 Joe Perches [off-list ref]:
quoted
On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
quoted
On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches [off-list ref] wrote:
[]
quoted
quoted
It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:
static inline void __user *to_user_ptr(void *p)
{
return (void __user *)p;
}
ahh, ok. I guess I was used to using it in the context of ioctl
structs.. in that context u64 -> (void __user *) made more sense.
Maybe uapi_to_ptr()? (ok, not super-creative.. maybe someone has a
better idea)
Maybe u64_to_user_ptr?
That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?
I've no particular opinion about location,
but maybe compat.h might be appropriate.
Maybe add all variants:
void __user *u32_to_user_ptr(u32 val)
void __user *u64_to_user_ptr(u64 val)
u32 user_ptr_to_u32(void __user *p)
u64 user_ptr_to_u64(void __user *p)
Maybe there's something about 32 bit userspace on
64 OS that should be done too.
Tbh I really don't think we should add 32bit variants and encourage the
mispractice of having 32bit user ptrs in ioctl structs and stuff. Anyway,
just my bikeshed on top ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch