Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures

2018-03-13 Thread Kieran Bingham
Hi David,

Just for reference here, I've posted a v2 of this patch now titled:
[PATCH v2 02/11] media: vsp1: Remove packed attributes from aligned structures

which removes the attributes instead of modifying them.

Thanks for the pointers!

Regards

Kieran

On 13/03/18 15:03, Kieran Bingham wrote:
> Hi David,
> 
> On 13/03/18 13:38, David Laight wrote:
>> From: Kieran Bingham [mailto:kieran.bingham+rene...@ideasonboard.com]
>>> On 13/03/18 11:20, David Laight wrote:
 From: Kieran Bingham
> Sent: 09 March 2018 22:04
> The kernel provides a __packed definition to abstract away from the
> compiler specific attributes tag.
>
> Convert all packed structures in VSP1 to use it.
>
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 37e2c984fbf3..382e45c2054e 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -29,19 +29,19 @@
>  struct vsp1_dl_header_list {
>   u32 num_bytes;
>   u32 addr;
> -} __attribute__((__packed__));
> +} __packed;
>
>  struct vsp1_dl_header {
>   u32 num_lists;
>   struct vsp1_dl_header_list lists[8];
>   u32 next_header;
>   u32 flags;
> -} __attribute__((__packed__));
> +} __packed;
>
>  struct vsp1_dl_entry {
>   u32 addr;
>   u32 data;
> -} __attribute__((__packed__));
> +} __packed;

 Do these structures ever actually appear in misaligned memory?
 If they don't then they shouldn't be marked 'packed'.
>>>
>>> I believe the declaration is to ensure that the struct definition is not 
>>> altered
>>> by the compiler as these structures specifically define the layout of how 
>>> the
>>> memory is read by the VSP1 hardware.
>>
>> The C language and ABI define structure layouts.
>>
>>> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved 
>>> or
>>> rearranged by the compiler (though I believe it would be free to do so if it
>>> chose without this attribute), but I think the declaration shows the intent 
>>> of
>>> the memory structure.
>>
>> The language requires the fields be in order and the ABI stops the compiler
>> adding 'random' padding.
>>
>>> Isn't this a common approach throughout the kernel when dealing with 
>>> hardware
>>> defined memory structures ?
>>
>> Absolutely not.
>> __packed tells the compiler that the structure might be on any address 
>> boundary.
>> On many architectures this means the compiler must use byte accesses with 
>> shifts
>> and ors for every access.
>> The most a hardware defined structure will have is a compile-time assert
>> that it is the correct size (to avoid silly errors from changes).
> 
> 
> 
> Ok - interesting, I see what you're saying - and certainly don't want the
> compiler to be performing byte accesses on the structures unnecessarily.
> 
> I'm trying to distinguish the difference here. Is the single point that
>  __packed
> 
> causes byte-access, where as
> 
> __attribute__((__packed__));
> 
> does not?
> 
> Looking at the GCC docs [0]: I see that  __attribute__((__packed__)) tells the
> compiler that the "structure or union is placed to minimize the memory 
> required".
> 
> However, the keil compiler docs[1] do certainly declare that __packed causes
> byte alignment.
> 
> I was confused/thrown off here by the Kernel defining __packed as
> __attribute__((packed)) at [2].
> 
> Do __attribute__((packed)) and __attribute__((__packed__)) differ ?
> 
> In which case, from what I've read so far I wish "__packed" was 
> "__unaligned"...
> 
> 
> [0]
> https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute
> 
> [1] http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm
> 
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.16-rc5#n92
> 
> 
> Regards
> 
> Kieran
> 
> 
>>  David
>>


Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures

2018-03-13 Thread Kieran Bingham
Hi David,

On 13/03/18 13:38, David Laight wrote:
> From: Kieran Bingham [mailto:kieran.bingham+rene...@ideasonboard.com]
>> On 13/03/18 11:20, David Laight wrote:
>>> From: Kieran Bingham
 Sent: 09 March 2018 22:04
 The kernel provides a __packed definition to abstract away from the
 compiler specific attributes tag.

 Convert all packed structures in VSP1 to use it.

 Signed-off-by: Kieran Bingham 
 ---
  drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
 b/drivers/media/platform/vsp1/vsp1_dl.c
 index 37e2c984fbf3..382e45c2054e 100644
 --- a/drivers/media/platform/vsp1/vsp1_dl.c
 +++ b/drivers/media/platform/vsp1/vsp1_dl.c
 @@ -29,19 +29,19 @@
  struct vsp1_dl_header_list {
u32 num_bytes;
u32 addr;
 -} __attribute__((__packed__));
 +} __packed;

  struct vsp1_dl_header {
u32 num_lists;
struct vsp1_dl_header_list lists[8];
u32 next_header;
u32 flags;
 -} __attribute__((__packed__));
 +} __packed;

  struct vsp1_dl_entry {
u32 addr;
u32 data;
 -} __attribute__((__packed__));
 +} __packed;
>>>
>>> Do these structures ever actually appear in misaligned memory?
>>> If they don't then they shouldn't be marked 'packed'.
>>
>> I believe the declaration is to ensure that the struct definition is not 
>> altered
>> by the compiler as these structures specifically define the layout of how the
>> memory is read by the VSP1 hardware.
> 
> The C language and ABI define structure layouts.
> 
>> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or
>> rearranged by the compiler (though I believe it would be free to do so if it
>> chose without this attribute), but I think the declaration shows the intent 
>> of
>> the memory structure.
> 
> The language requires the fields be in order and the ABI stops the compiler
> adding 'random' padding.
> 
>> Isn't this a common approach throughout the kernel when dealing with hardware
>> defined memory structures ?
> 
> Absolutely not.
> __packed tells the compiler that the structure might be on any address 
> boundary.
> On many architectures this means the compiler must use byte accesses with 
> shifts
> and ors for every access.
> The most a hardware defined structure will have is a compile-time assert
> that it is the correct size (to avoid silly errors from changes).



Ok - interesting, I see what you're saying - and certainly don't want the
compiler to be performing byte accesses on the structures unnecessarily.

I'm trying to distinguish the difference here. Is the single point that
 __packed

causes byte-access, where as

__attribute__((__packed__));

does not?

Looking at the GCC docs [0]: I see that  __attribute__((__packed__)) tells the
compiler that the "structure or union is placed to minimize the memory 
required".

However, the keil compiler docs[1] do certainly declare that __packed causes
byte alignment.

I was confused/thrown off here by the Kernel defining __packed as
__attribute__((packed)) at [2].

Do __attribute__((packed)) and __attribute__((__packed__)) differ ?

In which case, from what I've read so far I wish "__packed" was "__unaligned"...


[0]
https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute

[1] http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.16-rc5#n92


Regards

Kieran


>   David
> 


RE: [PATCH 02/11] media: vsp1: use kernel __packed for structures

2018-03-13 Thread David Laight
From: Kieran Bingham [mailto:kieran.bingham+rene...@ideasonboard.com]
> On 13/03/18 11:20, David Laight wrote:
> > From: Kieran Bingham
> >> Sent: 09 March 2018 22:04
> >> The kernel provides a __packed definition to abstract away from the
> >> compiler specific attributes tag.
> >>
> >> Convert all packed structures in VSP1 to use it.
> >>
> >> Signed-off-by: Kieran Bingham 
> >> ---
> >>  drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> >> b/drivers/media/platform/vsp1/vsp1_dl.c
> >> index 37e2c984fbf3..382e45c2054e 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> >> @@ -29,19 +29,19 @@
> >>  struct vsp1_dl_header_list {
> >>u32 num_bytes;
> >>u32 addr;
> >> -} __attribute__((__packed__));
> >> +} __packed;
> >>
> >>  struct vsp1_dl_header {
> >>u32 num_lists;
> >>struct vsp1_dl_header_list lists[8];
> >>u32 next_header;
> >>u32 flags;
> >> -} __attribute__((__packed__));
> >> +} __packed;
> >>
> >>  struct vsp1_dl_entry {
> >>u32 addr;
> >>u32 data;
> >> -} __attribute__((__packed__));
> >> +} __packed;
> >
> > Do these structures ever actually appear in misaligned memory?
> > If they don't then they shouldn't be marked 'packed'.
> 
> I believe the declaration is to ensure that the struct definition is not 
> altered
> by the compiler as these structures specifically define the layout of how the
> memory is read by the VSP1 hardware.

The C language and ABI define structure layouts.

> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or
> rearranged by the compiler (though I believe it would be free to do so if it
> chose without this attribute), but I think the declaration shows the intent of
> the memory structure.

The language requires the fields be in order and the ABI stops the compiler
adding 'random' padding.

> Isn't this a common approach throughout the kernel when dealing with hardware
> defined memory structures ?

Absolutely not.
__packed tells the compiler that the structure might be on any address boundary.
On many architectures this means the compiler must use byte accesses with shifts
and ors for every access.
The most a hardware defined structure will have is a compile-time assert
that it is the correct size (to avoid silly errors from changes).

David



Re: [PATCH 02/11] media: vsp1: use kernel __packed for structures

2018-03-13 Thread Kieran Bingham
Hi David,

On 13/03/18 11:20, David Laight wrote:
> From: Kieran Bingham
>> Sent: 09 March 2018 22:04
>> The kernel provides a __packed definition to abstract away from the
>> compiler specific attributes tag.
>>
>> Convert all packed structures in VSP1 to use it.
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>  drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
>> b/drivers/media/platform/vsp1/vsp1_dl.c
>> index 37e2c984fbf3..382e45c2054e 100644
>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>> @@ -29,19 +29,19 @@
>>  struct vsp1_dl_header_list {
>>  u32 num_bytes;
>>  u32 addr;
>> -} __attribute__((__packed__));
>> +} __packed;
>>
>>  struct vsp1_dl_header {
>>  u32 num_lists;
>>  struct vsp1_dl_header_list lists[8];
>>  u32 next_header;
>>  u32 flags;
>> -} __attribute__((__packed__));
>> +} __packed;
>>
>>  struct vsp1_dl_entry {
>>  u32 addr;
>>  u32 data;
>> -} __attribute__((__packed__));
>> +} __packed;
> 
> Do these structures ever actually appear in misaligned memory?
> If they don't then they shouldn't be marked 'packed'.

I believe the declaration is to ensure that the struct definition is not altered
by the compiler as these structures specifically define the layout of how the
memory is read by the VSP1 hardware.

Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or
rearranged by the compiler (though I believe it would be free to do so if it
chose without this attribute), but I think the declaration shows the intent of
the memory structure.

Isn't this a common approach throughout the kernel when dealing with hardware
defined memory structures ?

Regards
--
Kieran


> 
>   David
> 


RE: [PATCH 02/11] media: vsp1: use kernel __packed for structures

2018-03-13 Thread David Laight
From: Kieran Bingham
> Sent: 09 March 2018 22:04
> The kernel provides a __packed definition to abstract away from the
> compiler specific attributes tag.
> 
> Convert all packed structures in VSP1 to use it.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 37e2c984fbf3..382e45c2054e 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -29,19 +29,19 @@
>  struct vsp1_dl_header_list {
>   u32 num_bytes;
>   u32 addr;
> -} __attribute__((__packed__));
> +} __packed;
> 
>  struct vsp1_dl_header {
>   u32 num_lists;
>   struct vsp1_dl_header_list lists[8];
>   u32 next_header;
>   u32 flags;
> -} __attribute__((__packed__));
> +} __packed;
> 
>  struct vsp1_dl_entry {
>   u32 addr;
>   u32 data;
> -} __attribute__((__packed__));
> +} __packed;

Do these structures ever actually appear in misaligned memory?
If they don't then they shouldn't be marked 'packed'.

David