[RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Hogan]

The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
used in struct osd_cdb_head, but it isn't marked as packed. Some
architectures will round the struct size up which triggers BUILD_BUG_ON
compile errors in osd_initiator.c when the outer structs are unexpected
sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 include/scsi/scsi.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 66216c1..3beaef3 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr {
 	__u8 additional_cdb_length;         /* total cdb length - 8 */
 	__be16 service_action;
 	/* service specific data follows */
-};
+} __packed;
 
 static inline unsigned
 scsi_varlen_cdb_length(const void *hdr)
-- 
1.7.7.6

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[Bart Van Assche]

On 10/11/12 11:15, James Hogan wrote:
> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
> used in struct osd_cdb_head, but it isn't marked as packed. Some
> architectures will round the struct size up which triggers BUILD_BUG_ON
> compile errors in osd_initiator.c when the outer structs are unexpected
> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>   include/scsi/scsi.h |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 66216c1..3beaef3 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr {
>   	__u8 additional_cdb_length;         /* total cdb length - 8 */
>   	__be16 service_action;
>   	/* service specific data follows */
> -};
> +} __packed;
>
>   static inline unsigned
>   scsi_varlen_cdb_length(const void *hdr)

Hello James,

Are you aware that __packed can also be used on individual struct 
members and that doing so has a lower performance penalty than using the 
__packed attribute on an entire struct ? See e.g. <linux/sysv_fs.h> for 
an example.

Bart.

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Bottomley]

On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote:
> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
> used in struct osd_cdb_head, but it isn't marked as packed. Some
> architectures will round the struct size up which triggers BUILD_BUG_ON
> compile errors in osd_initiator.c when the outer structs are unexpected
> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.

What actual problem have you encountered? The structure is {u8[8], u16}
which is naturally packed on every architecture I know about.  I've even
built osd_initiator without problem on parisc, which has some of the
most rigid alignment rules I've seen.

James

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Hogan]

On 11/10/12 11:01, Bart Van Assche wrote:
> On 10/11/12 11:15, James Hogan wrote:
>> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
>> used in struct osd_cdb_head, but it isn't marked as packed. Some
>> architectures will round the struct size up which triggers BUILD_BUG_ON
>> compile errors in osd_initiator.c when the outer structs are unexpected
>> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> ---
>>   include/scsi/scsi.h |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
>> index 66216c1..3beaef3 100644
>> --- a/include/scsi/scsi.h
>> +++ b/include/scsi/scsi.h
>> @@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr {
>>       __u8 additional_cdb_length;         /* total cdb length - 8 */
>>       __be16 service_action;
>>       /* service specific data follows */
>> -};
>> +} __packed;
>>
>>   static inline unsigned
>>   scsi_varlen_cdb_length(const void *hdr)
> 
> Hello James,
> 
> Are you aware that __packed can also be used on individual struct
> members and that doing so has a lower performance penalty than using the
> __packed attribute on an entire struct ? See e.g. <linux/sysv_fs.h> for
> an example.

Hi Bart,

Thanks, I wasn't aware of that, however the problem is the size of the
struct as a whole rather than the alignment or offsets of any individual
data members. E.g. adding __attribute__((packed,aligned(2))) to the
16bit member doesn't reduce the struct to 10 bytes, or change the offset
from 8 bytes.

Cheers
James

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Hogan]

On 11/10/12 11:24, James Bottomley wrote:
> On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote:
>> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
>> used in struct osd_cdb_head, but it isn't marked as packed. Some
>> architectures will round the struct size up which triggers BUILD_BUG_ON
>> compile errors in osd_initiator.c when the outer structs are unexpected
>> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
> 
> What actual problem have you encountered? The structure is {u8[8], u16}
> which is naturally packed on every architecture I know about.  I've even
> built osd_initiator without problem on parisc, which has some of the
> most rigid alignment rules I've seen.

Hi James,

The alignment is fine (the offset of the u16 is 8 bytes), but
unfortunately with the metag port of gcc, sizeof(struct
scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the
largest data member alignment is only 2 bytes), which is 12 bytes
instead of 10.

Cheers
James

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Bottomley]

On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote:
> On 11/10/12 11:24, James Bottomley wrote:
> > On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote:
> >> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
> >> used in struct osd_cdb_head, but it isn't marked as packed. Some
> >> architectures will round the struct size up which triggers BUILD_BUG_ON
> >> compile errors in osd_initiator.c when the outer structs are unexpected
> >> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
> > 
> > What actual problem have you encountered? The structure is {u8[8], u16}
> > which is naturally packed on every architecture I know about.  I've even
> > built osd_initiator without problem on parisc, which has some of the
> > most rigid alignment rules I've seen.
> 
> Hi James,
> 
> The alignment is fine (the offset of the u16 is 8 bytes), but
> unfortunately with the metag port of gcc, sizeof(struct
> scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the
> largest data member alignment is only 2 bytes), which is 12 bytes
> instead of 10.

That sounds to be a bug in your compiler ... it shouldn't be rounding up
structure sizes if the structure can fit in 10 bytes.  This isn't
happening in any other architecture that I know of (otherwise we'd have
had a reported build break).

James

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[Alan Cox]

> > The alignment is fine (the offset of the u16 is 8 bytes), but
> > unfortunately with the metag port of gcc, sizeof(struct
> > scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the
> > largest data member alignment is only 2 bytes), which is 12 bytes
> > instead of 10.
> 
> That sounds to be a bug in your compiler ... it shouldn't be rounding up
> structure sizes if the structure can fit in 10 bytes.  This isn't
> happening in any other architecture that I know of (otherwise we'd have
> had a reported build break).

It's not a bug for the alignment rules of the processor as far as I can
see. The architectural definition is perfectly entitled to have tail
padding in this case.

The x86 equivalent would be

struct foo {
	double x;
	int a;
};


which is *NOT* 12 bytes long.

It is indeed a portability bug in the scsi layer.

Alan

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Hogan]

On 11/10/12 13:58, James Bottomley wrote:
> On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote:
>> On 11/10/12 11:24, James Bottomley wrote:
>>> On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote:
>>>> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
>>>> used in struct osd_cdb_head, but it isn't marked as packed. Some
>>>> architectures will round the struct size up which triggers BUILD_BUG_ON
>>>> compile errors in osd_initiator.c when the outer structs are unexpected
>>>> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
>>>
>>> What actual problem have you encountered? The structure is {u8[8], u16}
>>> which is naturally packed on every architecture I know about.  I've even
>>> built osd_initiator without problem on parisc, which has some of the
>>> most rigid alignment rules I've seen.
>>
>> Hi James,
>>
>> The alignment is fine (the offset of the u16 is 8 bytes), but
>> unfortunately with the metag port of gcc, sizeof(struct
>> scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the
>> largest data member alignment is only 2 bytes), which is 12 bytes
>> instead of 10.
> 
> That sounds to be a bug in your compiler ... it shouldn't be rounding up
> structure sizes if the structure can fit in 10 bytes.  This isn't
> happening in any other architecture that I know of (otherwise we'd have
> had a reported build break).

This was my initial thought, and I share your feeling that this isn't
ideal compiler behaviour, however it's pretty much set in stone as part
of our ABI now. Having talked to one of our compiler folk about it
here's what he had to say:
> In GCC 4.2 the following backends have STRUCTURE_SIZE_BOUNDARY set to >=32 which will trigger this behaviour:
> 
> Alpha+unicos
> Arm prior to AAPCS
> Mt
> 
> This is within standards to my knowledge.

Cheers
James

Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed

[James Hogan]

Hi James,

On 11/10/12 15:10, James Hogan wrote:
> On 11/10/12 13:58, James Bottomley wrote:
>> On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote:
>>> On 11/10/12 11:24, James Bottomley wrote:
>>>> On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote:
>>>>> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when
>>>>> used in struct osd_cdb_head, but it isn't marked as packed. Some
>>>>> architectures will round the struct size up which triggers BUILD_BUG_ON
>>>>> compile errors in osd_initiator.c when the outer structs are unexpected
>>>>> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed.
>>>>
>>>> What actual problem have you encountered? The structure is {u8[8], u16}
>>>> which is naturally packed on every architecture I know about.  I've even
>>>> built osd_initiator without problem on parisc, which has some of the
>>>> most rigid alignment rules I've seen.
>>>
>>> Hi James,
>>>
>>> The alignment is fine (the offset of the u16 is 8 bytes), but
>>> unfortunately with the metag port of gcc, sizeof(struct
>>> scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the
>>> largest data member alignment is only 2 bytes), which is 12 bytes
>>> instead of 10.
>>
>> That sounds to be a bug in your compiler ... it shouldn't be rounding up
>> structure sizes if the structure can fit in 10 bytes.  This isn't
>> happening in any other architecture that I know of (otherwise we'd have
>> had a reported build break).
> 
> This was my initial thought, and I share your feeling that this isn't
> ideal compiler behaviour, however it's pretty much set in stone as part
> of our ABI now. Having talked to one of our compiler folk about it
> here's what he had to say:
>> In GCC 4.2 the following backends have STRUCTURE_SIZE_BOUNDARY set to >=32 which will trigger this behaviour:
>>
>> Alpha+unicos
>> Arm prior to AAPCS
>> Mt
>>
>> This is within standards to my knowledge.

What are the chances of getting this patch accepted? I believe it is
correct and the compiler behaviour is within standards, and hopefully
arch/metag will be merged for v3.9 so without this patch the build of
osd_initiator.c will be broken on an in-tree architecture.

Thanks
James