| Message ID | 1454102426-20637-7-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gustavo, s/__64/__u64/ in the commit message. On 29 January 2016 at 23:20, Gustavo Padovan <gustavo@padovan.org> wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Making fence_info a pointer enables us to extend the struct in the future > without breaking the ABI. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > index ed281fc..9f07aa7 100644 > --- a/drivers/staging/android/uapi/sync.h > +++ b/drivers/staging/android/uapi/sync.h > @@ -54,7 +54,7 @@ struct sync_file_info { > char name[32]; > __s32 status; > > - __u8 fence_info[0]; > + __u64 *fence_info; I believe you misinterpreted what was meant with "_u64 pointer". As the storage use for of a pointer varies across 32/64 bit arch, we explicitly use a variable type that is large enough (and consistent) for both cases. Thus the above should be + __u64 fence_info; Hope this clarifies things. -Emil
Op 29-01-16 om 22:20 schreef Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Making fence_info a pointer enables us to extend the struct in the future > without breaking the ABI. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/staging/android/sync.c | 2 +- > drivers/staging/android/uapi/sync.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > index f7530f0..51d4f47 100644 > --- a/drivers/staging/android/sync.c > +++ b/drivers/staging/android/sync.c > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > if (info->status >= 0) > info->status = !info->status; > > - len = sizeof(struct sync_file_info); > + len = sizeof(struct sync_file_info) - sizeof(__u64 *); > > for (i = 0; i < sync_file->num_fences; ++i) { > struct fence *fence = sync_file->cbs[i].fence; > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > index ed281fc..9f07aa7 100644 > --- a/drivers/staging/android/uapi/sync.h > +++ b/drivers/staging/android/uapi/sync.h > @@ -54,7 +54,7 @@ struct sync_file_info { > char name[32]; > __s32 status; > > - __u8 fence_info[0]; > + __u64 *fence_info; > }; > Pointers are awful, it should be a __u64 since it's a pointer type. Userspace should cast it to a uintptr_t in userspace. This structure also won't work on 64-bits systems, there may be a hole between fence_info and status (or num_fences in next patch). It's probably best to move it to the top and ensure the struct is 64-bits aligned.
Hi Maarten, 2016-02-01 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Op 29-01-16 om 22:20 schreef Gustavo Padovan: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Making fence_info a pointer enables us to extend the struct in the future > > without breaking the ABI. > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > drivers/staging/android/sync.c | 2 +- > > drivers/staging/android/uapi/sync.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > > index f7530f0..51d4f47 100644 > > --- a/drivers/staging/android/sync.c > > +++ b/drivers/staging/android/sync.c > > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > > if (info->status >= 0) > > info->status = !info->status; > > > > - len = sizeof(struct sync_file_info); > > + len = sizeof(struct sync_file_info) - sizeof(__u64 *); > > > > for (i = 0; i < sync_file->num_fences; ++i) { > > struct fence *fence = sync_file->cbs[i].fence; > > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > > index ed281fc..9f07aa7 100644 > > --- a/drivers/staging/android/uapi/sync.h > > +++ b/drivers/staging/android/uapi/sync.h > > @@ -54,7 +54,7 @@ struct sync_file_info { > > char name[32]; > > __s32 status; > > > > - __u8 fence_info[0]; > > + __u64 *fence_info; > > }; > > > Pointers are awful, it should be a __u64 since it's a pointer type. Userspace should cast it to a uintptr_t in userspace. Oh, I made a mistake. I'll fix this. > This structure also won't work on 64-bits systems, there may be a hole between fence_info and status (or num_fences in next patch). > > It's probably best to move it to the top and ensure the struct is 64-bits aligned. That is not possible because we are not allocating only 64bits there but a array of struct fence_info, so it needs to be the last one. Maybe we can add some sort of padding? Gustavo
2016-02-01 Gustavo Padovan <gustavo@padovan.org>: > Hi Maarten, > > 2016-02-01 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > Op 29-01-16 om 22:20 schreef Gustavo Padovan: > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > Making fence_info a pointer enables us to extend the struct in the future > > > without breaking the ABI. > > > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > --- > > > drivers/staging/android/sync.c | 2 +- > > > drivers/staging/android/uapi/sync.h | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > > > index f7530f0..51d4f47 100644 > > > --- a/drivers/staging/android/sync.c > > > +++ b/drivers/staging/android/sync.c > > > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > > > if (info->status >= 0) > > > info->status = !info->status; > > > > > > - len = sizeof(struct sync_file_info); > > > + len = sizeof(struct sync_file_info) - sizeof(__u64 *); > > > > > > for (i = 0; i < sync_file->num_fences; ++i) { > > > struct fence *fence = sync_file->cbs[i].fence; > > > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > > > index ed281fc..9f07aa7 100644 > > > --- a/drivers/staging/android/uapi/sync.h > > > +++ b/drivers/staging/android/uapi/sync.h > > > @@ -54,7 +54,7 @@ struct sync_file_info { > > > char name[32]; > > > __s32 status; > > > > > > - __u8 fence_info[0]; > > > + __u64 *fence_info; > > > }; > > > > > Pointers are awful, it should be a __u64 since it's a pointer type. Userspace should cast it to a uintptr_t in userspace. > > Oh, I made a mistake. I'll fix this. > > > > This structure also won't work on 64-bits systems, there may be a hole between fence_info and status (or num_fences in next patch). > > > > It's probably best to move it to the top and ensure the struct is 64-bits aligned. > > That is not possible because we are not allocating only 64bits there but > a array of struct fence_info, so it needs to be the last one. Maybe we > can add some sort of padding? Actually it is 64bits aligned: struct sync_file_info { char name[32]; __s32 status; __u32 flags; __u32 num_fences; __u32 len; __u64 fence_info; }; So nothing to worry here. Gustavo
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index f7530f0..51d4f47 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, if (info->status >= 0) info->status = !info->status; - len = sizeof(struct sync_file_info); + len = sizeof(struct sync_file_info) - sizeof(__u64 *); for (i = 0; i < sync_file->num_fences; ++i) { struct fence *fence = sync_file->cbs[i].fence; diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h index ed281fc..9f07aa7 100644 --- a/drivers/staging/android/uapi/sync.h +++ b/drivers/staging/android/uapi/sync.h @@ -54,7 +54,7 @@ struct sync_file_info { char name[32]; __s32 status; - __u8 fence_info[0]; + __u64 *fence_info; }; #define SYNC_IOC_MAGIC '>'