Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 02/03/2014 04:58 PM, Dan Carpenter wrote:
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
Yeah, just like your original discussion. :-)
Hmm... can we say: for metag compiler, in a pack region, it considers
variables alignment, but does not consider about struct/union alignment
(except append packed to each related struct/union).
For compatible (consider about its ABI), it has to keep this features,
but for kernel, it needs be changed.
So, I suggest to add one parameter to compiler to switch this feature,
and append this parameter to KBUILD_CFLAGS in arch/metag/Makefile
which can satisfy both ABI and kernel.
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
From: Dan Carpenter
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
The same is probably be true of: struct foo { _u16 bar; };
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 02/03/2014 06:22 PM, James Hogan wrote:
On 03/02/14 10:05, David Laight wrote:
From: Dan Carpenter
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
The same is probably be true of: struct foo { _u16 bar; };
Yes indeed.
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).
Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).
Oh, thank you for your explanation.
And hope this feature issue can be fixed, and satisfy both kernel and
ABI. :-)
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
From: James Hogan
On 03/02/14 10:05, David Laight wrote:
From: Dan Carpenter
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
The same is probably be true of: struct foo { _u16 bar; };
Yes indeed.
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).
Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).
Compile some code for a cpu that doesn't support misaligned transfers
(probably one of sparc, arm, ppc) and see if the compiler generates a
single 16bit request or two 8 bits ones.
You don't want the compiler generating multiple byte-sized memory transfers.
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 03/02/14 10:35, David Laight wrote:
From: James Hogan
On 03/02/14 10:05, David Laight wrote:
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).
Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).
Compile some code for a cpu that doesn't support misaligned transfers
(probably one of sparc, arm, ppc) and see if the compiler generates a
single 16bit request or two 8 bits ones.
You don't want the compiler generating multiple byte-sized memory transfers.
Meta is also one of those arches, and according to my quick tests,
__packed alone does correctly make it fall back to byte loads/stores,
but with __packed __aligned(2) it uses 16bit loads/stores. I've also
confirmed that with an ARM toolchain (see below for example).
Cheers
James
input:
#define __aligned(x)__attribute__((aligned(x)))
#define __packed__attribute__((packed))
union a {
short x, y;
} __aligned(2) __packed;
struct b {
short x;
} __aligned(2) __packed;
unsigned int soa = sizeof(union a);
unsigned int aoa = __alignof__(union a);
unsigned int sob = sizeof(struct b);
unsigned int aob = __alignof__(struct b);
void t(struct b *x, union a *y)
{
++x-x;
++y-x;
}
ARM output (-O2):
.cpu arm10tdmi
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 2
.eabi_attribute 30, 2
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file alignment4.c
.text
.align 2
.global t
.type t, %function
t:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
ldrhr3, [r0, #0]
add r3, r3, #1
strhr3, [r0, #0]@ movhi
ldrhr3, [r1, #0]
add r3, r3, #1
strhr3, [r1, #0]@ movhi
bx lr
.size t, .-t
.global aob
.global sob
.global aoa
.global soa
.data
.align 2
.type aob, %object
.size aob, 4
aob:
.word 2
.type sob, %object
.size sob, 4
sob:
.word 2
.type aoa, %object
.size aoa, 4
aoa:
.word 2
.type soa, %object
.size soa, 4
soa:
.word 2
.ident GCC: (GNU) 4.7.1 20120606 (Red Hat 4.7.1-0.1.20120606)
.section.note.GNU-stack,,%progbits
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 02/03/2014 06:03 PM, Chen Gang wrote:
On 02/03/2014 04:58 PM, Dan Carpenter wrote:
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
Yeah, just like your original discussion. :-)
Hmm... can we say: for metag compiler, in a pack region, it considers
variables alignment, but does not consider about struct/union alignment
(except append packed to each related struct/union).
For compatible (consider about its ABI), it has to keep this features,
but for kernel, it needs be changed.
So, I suggest to add one parameter to compiler to switch this feature,
and append this parameter to KBUILD_CFLAGS in arch/metag/Makefile
which can satisfy both ABI and kernel.
After append the parameter to KBUILD_CFLAGS in arch/metag/Makefile,
- I guess/assume include/uapi/* should/will not need be modified.
- but need check all files in arch/metag/include/uapi/*.
(add padding data for packed struct/union when __KERNEL__ defined)
- maybe we have to process metag related ABI which not in */uapi/*
(add padding data for packed struct/union when __KERNEL__ defined)
and when we find them, recommend to move all of them to */uapi/*.
Sorry, I don't know whether this way is the best way or not, but for me
it is an executable way to solve this feature issue and satisfy both
kernel and ABI.
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
From: James Hogan On 03/02/14 10:35, David Laight wrote: From: James Hogan Combining __packed with __aligned(2) does the trick though (__packed alone sets __aligned(1) which is obviously going to be suboptimal). ... Meta is also one of those arches, and according to my quick tests, __packed alone does correctly make it fall back to byte loads/stores, but with __packed __aligned(2) it uses 16bit loads/stores. I've also confirmed that with an ARM toolchain (see below for example). I would either: 1a) Add explicit padding to the relevant structures so that they are multiple of 4 bytes. or: 1b) #define some token to __packed __aligned(2) before all the structures that require changing, and use that in there definitions. This lets you comment on WHY you are doing it. and: 2) Add a compile-time assert that the structures are the correct size. Clearly you don't want to mark anything that contains a 32bit value with __packed __aligned(2). I'm not at all clear whether you are sometimes using a different compiler. David ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 01/21/2014 06:36 PM, James Hogan wrote: Hi Dan, On 20/01/14 21:13, Dan Carpenter wrote: I made a quick and dirty sparse patch to check for this. I don't think I will bother trying to send it to sparse upstream, but you can if you want to. It found 289 unions which might need a __packed added. The lustre unions were not in my allmodconfig so they're not listed. Thanks a lot for this, it seems to be useful. I'm adapting it to reduce false negatives (e.g. omitting the check if the struct/union is already packed), and I imagine it could be made to only warn about padded unpacked structs/unions which are used as nested members of packed structs/unions. It wouldn't catch everything but would probably catch a lot of cases that are most likely to be genuine since they would have been packed at the outer level for a reason. Perhaps there could be a command line option or a pragma so that unions will work in the kernel. We don't care about linking to outside libraries. We still interact with userland via structs and unions, so it would probably have to exclude anything in uapi/. Thank all of you firstly. But excuse me, I am still not quit clear that: what need we do enough to solve this feature issue? So I guess our current result is: - It is not a good idea to only let kernel to fit with compiler. - It is not a good idea to only let compiler to fit with kernel. - Need let compiler and kernel to fit with each other: - compiler will print related warning, but not break compiling. so metag compiler need be improvement (check and warn for it). - if check alignment explicitly in kernel source code, it need be fixed within kernel: apply related patches (pack each struct or union), but the related patch comments need be improved. Is what I guess correct? Thanks. -- Chen Gang Open, share and attitude like air, water and life which God blessed ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Hi Dan, On 20/01/14 21:13, Dan Carpenter wrote: I made a quick and dirty sparse patch to check for this. I don't think I will bother trying to send it to sparse upstream, but you can if you want to. It found 289 unions which might need a __packed added. The lustre unions were not in my allmodconfig so they're not listed. Thanks a lot for this, it seems to be useful. I'm adapting it to reduce false negatives (e.g. omitting the check if the struct/union is already packed), and I imagine it could be made to only warn about padded unpacked structs/unions which are used as nested members of packed structs/unions. It wouldn't catch everything but would probably catch a lot of cases that are most likely to be genuine since they would have been packed at the outer level for a reason. Perhaps there could be a command line option or a pragma so that unions will work in the kernel. We don't care about linking to outside libraries. We still interact with userland via structs and unions, so it would probably have to exclude anything in uapi/. Cheers James signature.asc Description: OpenPGP digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Hi Chen,
On 19/01/14 10:07, Chen Gang wrote:
BTW: this patch is related with another patch which is discussing (so I
have cc that patch to you and Greg too): if we could sure that it is a
compiler's feature issue, we will skip this patch.
If you're referring to the #pragma pack portability issue then this
issue is unrelated since it doesn't use #pragma pack.
The issue is *not* that the compiler is defectively failing to pack
nested structs. Doing that would be utterly broken since it would change
the layout of the same struct depending on where it is placed in the
program, Consider this example (which uses a nested struct rather than
union to demonstrate the point):
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
Both ABIs behave the same here:
Archsizeof(struct b)sizeof(struct a)
x86_64 8 10
metag 8 10
If struct b is made __packed, again both ABIs behave differently in the
same way:
Archsizeof(struct b)sizeof(struct a)
x86_64 6 8
metag 6 8
The issue is that C compiler ABIs may (and unfortunately metag ABI does)
pack structs and unions to at least 4 bytes, even if no members of the
struct or union are that large, which means that the nested struct/union
should be __packed too to portably ensure it's the expected size.
Cheers
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Ah. From so metag is a new arch and not a compiler like the changelog
says.
On Mon, Jan 20, 2014 at 11:56:47AM +, James Hogan wrote:
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
This is not the code we are discussing. It should look like:
struct a {
union {
short x;
short y;
}
short z;
};
Any normal person would assume that sizeof(struct a) would be 4 but
apparently on metag it is 8. That totally defeats the point of using
a union in the first place. It's easy enough to add a __packed to the
lustre declaration but I expect this to cause an endless stream of bugs.
It it is really stupid.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 20/01/14 12:30, Dan Carpenter wrote:
Ah. From so metag is a new arch and not a compiler like the changelog
says.
On Mon, Jan 20, 2014 at 11:56:47AM +, James Hogan wrote:
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
This is not the code we are discussing. It should look like:
struct a {
union {
short x;
short y;
}
short z;
};
Any normal person would assume that sizeof(struct a) would be 4 but
apparently on metag it is 8. That totally defeats the point of using
a union in the first place. It's easy enough to add a __packed to the
lustre declaration but I expect this to cause an endless stream of bugs.
It it is really stupid.
I agree completely (and did request this be changed when I first found
out about it, but since it's an ABI issue it was really too late).
That's why I'm not actively pushing for every case to be fixed unless
it's in generic code that actually affects metag.
Cheers
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On Mon, Jan 20, 2014 at 12:37:57PM +, James Hogan wrote:
On 20/01/14 12:30, Dan Carpenter wrote:
Ah. From so metag is a new arch and not a compiler like the changelog
says.
On Mon, Jan 20, 2014 at 11:56:47AM +, James Hogan wrote:
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
This is not the code we are discussing. It should look like:
struct a {
union {
short x;
short y;
}
short z;
};
Any normal person would assume that sizeof(struct a) would be 4 but
apparently on metag it is 8. That totally defeats the point of using
a union in the first place. It's easy enough to add a __packed to the
lustre declaration but I expect this to cause an endless stream of bugs.
It it is really stupid.
I agree completely (and did request this be changed when I first found
out about it, but since it's an ABI issue it was really too late).
That's why I'm not actively pushing for every case to be fixed unless
it's in generic code that actually affects metag.
It would be easy enough to make the compiler complain about any union
which would normally have size which is not a multiple of 4.
Warning: union will be padded with 2 bytes unless __attribute__((packed)).
Otherwise you will be fighting this for ever.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Are you sure it's padding the unions, and not just treating the unions
as structs? What is the size of this union?
union a {
int x;
short y;
};
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
I made a quick and dirty sparse patch to check for this. I don't think
I will bother trying to send it to sparse upstream, but you can if you
want to.
It found 289 unions which might need a __packed added. The lustre
unions were not in my allmodconfig so they're not listed.
Perhaps there could be a command line option or a pragma so that unions
will work in the kernel. We don't care about linking to outside
libraries.
regards,
dan carpenter
diff --git a/symbol.c b/symbol.c
index ebba56deaf94..596e47883aad 100644
--- a/symbol.c
+++ b/symbol.c
@@ -187,6 +187,12 @@ static struct symbol * examine_struct_union_type(struct
symbol *sym, int advance
bit_size = (bit_size + bit_align) ~bit_align;
}
sym-bit_size = bit_size;
+
+ if (!advance (info.bit_size / 8) % 4) {
+ int pad = 4 - (info.bit_size / 8) % 4;
+ warning(sym-pos, '%s' union will be padded with %d bytes
unless __attribute__((packed))., sym-ident ? sym-ident-name: null,
pad);
+ }
+
return sym;
}
drivers/acpi/sbshc.c:55:7: warning: 'acpi_smb_status' union will be padded with
3 bytes unless __attribute__((packed)).
drivers/atm/horizon.h:317:9: warning: 'null' union will be padded with 3
bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2173:15: warning: 'DAC960_V1_StatusMailbox' union will
be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2580:15: warning: 'DAC960_GEM_OutboundDoorBellRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2618:15: warning: 'DAC960_GEM_ErrorStatusRegister' union
will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2867:15: warning: 'DAC960_BA_InboundDoorBellRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2891:15: warning: 'DAC960_BA_OutboundDoorBellRegister'
union will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2912:15: warning: 'DAC960_BA_InterruptMaskRegister'
union will be padded with 1 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:2929:15: warning: 'DAC960_BA_ErrorStatusRegister' union
will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3173:15: warning: 'DAC960_LP_InboundDoorBellRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3197:15: warning: 'DAC960_LP_OutboundDoorBellRegister'
union will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3218:15: warning: 'DAC960_LP_InterruptMaskRegister'
union will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3234:15: warning: 'DAC960_LP_ErrorStatusRegister' union
will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3487:15: warning: 'DAC960_LA_InboundDoorBellRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3511:15: warning: 'DAC960_LA_OutboundDoorBellRegister'
union will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3532:15: warning: 'DAC960_LA_InterruptMaskRegister'
union will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3548:15: warning: 'DAC960_LA_ErrorStatusRegister' union
will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:3869:15: warning: 'DAC960_PG_ErrorStatusRegister' union
will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:4155:15: warning: 'DAC960_PD_OutboundDoorBellRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:4174:15: warning: 'DAC960_PD_InterruptEnableRegister'
union will be padded with 3 bytes unless __attribute__((packed)).
drivers/block/DAC960.h:4189:15: warning: 'DAC960_PD_ErrorStatusRegister' union
will be padded with 2 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:221:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:225:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:229:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:233:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:238:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:242:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/mtip32xx/mtip32xx.h:246:15: warning: 'null' union will be
padded with 3 bytes unless __attribute__((packed)).
drivers/block/paride/paride.h:120:15: warning: 'null' union will be padded
with 2 bytes unless __attribute__((packed)).
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 01/18/2014 10:24 PM, Dan Carpenter wrote: On Sat, Jan 18, 2014 at 06:26:10PM +0800, Chen Gang wrote: On 01/18/2014 06:05 PM, Dan Carpenter wrote: On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote: Unfortunately, not all compilers assumes the structures within a pack region also need be packed (e.g. metag), so need add a pack explicitly to satisfy all compilers. The related error (under metag with allmodconfig): CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe': drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here And originally, all related code used __attribute__((packed)), so still use it instead of '__packed'. Use __packed. Then at least one line will be correct which is better than nothing. Hmm... but that will break the 'consistency' (which is not quite good for readers). For me, it will be better to provide another patch to change all __attribute__((packed)) to __packed within this file. What about your ideas? In the end, it's not something we care about enough to ask you to redo the patch. But what I'm saying is that you should prefer kernel style over local style. We'll fix the surrounding lines later. OK, thanks, and also thank James and ./scripts/checkpatch.pl who/which also mentioned about it to me. BTW: this patch is related with another patch which is discussing (so I have cc that patch to you and Greg too): if we could sure that it is a compiler's feature issue, we will skip this patch. Thanks -- Chen Gang Open, share and attitude like air, water and life which God blessed ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Unfortunately, not all compilers assumes the structures within a pack
region also need be packed (e.g. metag), so need add a pack explicitly
to satisfy all compilers.
The related error (under metag with allmodconfig):
CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
And originally, all related code used __attribute__((packed)), so
still use it instead of '__packed'.
Signed-off-by: Chen Gang gang.chen.5...@gmail.com
---
drivers/staging/lustre/lustre/include/lustre/lustre_user.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
index 6b6c0240..0828b31 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
@@ -345,7 +345,7 @@ struct lov_user_md_v3 {/* LOV EA user data
(host-endian) */
* lmm_objects, use when writing */
__u16 lmm_layout_gen; /* layout generation number
* used when reading */
- };
+ } __attribute__((packed));
char lmm_pool_name[LOV_MAXPOOLNAME]; /* pool name */
struct lov_user_ost_data_v1 lmm_objects[0]; /* per-stripe data */
} __attribute__((packed));
--
1.7.11.7
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote: Unfortunately, not all compilers assumes the structures within a pack region also need be packed (e.g. metag), so need add a pack explicitly to satisfy all compilers. The related error (under metag with allmodconfig): CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe': drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here And originally, all related code used __attribute__((packed)), so still use it instead of '__packed'. Use __packed. Then at least one line will be correct which is better than nothing. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 01/18/2014 06:05 PM, Dan Carpenter wrote: On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote: Unfortunately, not all compilers assumes the structures within a pack region also need be packed (e.g. metag), so need add a pack explicitly to satisfy all compilers. The related error (under metag with allmodconfig): CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe': drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here And originally, all related code used __attribute__((packed)), so still use it instead of '__packed'. Use __packed. Then at least one line will be correct which is better than nothing. Hmm... but that will break the 'consistency' (which is not quite good for readers). For me, it will be better to provide another patch to change all __attribute__((packed)) to __packed within this file. What about your ideas? Thanks. -- Chen Gang Open, share and attitude like air, water and life which God blessed ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On Sat, Jan 18, 2014 at 06:26:10PM +0800, Chen Gang wrote: On 01/18/2014 06:05 PM, Dan Carpenter wrote: On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote: Unfortunately, not all compilers assumes the structures within a pack region also need be packed (e.g. metag), so need add a pack explicitly to satisfy all compilers. The related error (under metag with allmodconfig): CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe': drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here And originally, all related code used __attribute__((packed)), so still use it instead of '__packed'. Use __packed. Then at least one line will be correct which is better than nothing. Hmm... but that will break the 'consistency' (which is not quite good for readers). For me, it will be better to provide another patch to change all __attribute__((packed)) to __packed within this file. What about your ideas? In the end, it's not something we care about enough to ask you to redo the patch. But what I'm saying is that you should prefer kernel style over local style. We'll fix the surrounding lines later. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
