Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1740385 > unrolled thread

Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain  properties David Miller <davem@davemloft.net> - 2017-09-27 06:40 +0200
    Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-09-27 13:40 +0200
      RE: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties David Laight <David.Laight@ACULAB.COM> - 2017-09-27 18:20 +0200
      Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain  properties David Miller <davem@davemloft.net> - 2017-09-27 18:30 +0200

#1740385 — Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties

From: Mika Westerberg <mika.westerberg@linux.intel.com>
Date: Mon, 25 Sep 2017 14:07:24 +0300

> +struct tb_property_entry {
> +	u32 key_hi;
> +	u32 key_lo;
> +	u16 length;
> +	u8 reserved;
> +	u8 type;
> +	u32 value;
> +} __packed;
> +
> +struct tb_property_rootdir_entry {
> +	u32 magic;
> +	u32 length;
> +	struct tb_property_entry entries[];
> +} __packed;
> +
> +struct tb_property_dir_entry {
> +	u32 uuid[4];
> +	struct tb_property_entry entries[];
> +} __packed;

There is no apparent need for __packed here, and __packed should be
avoided unless absolutely necessary as it pessimizes the code
significantly on some architectures.

Please remove __packed from these datastructures unless you can
prove it is absolutely needed and, in such case, please document
in a comment why that requirement exists.  Because from the layout
of these types, everything will be packed in just fine without
__packed.

Thank you.

[toc] | [next] | [standalone]

#1740630 — Re: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties

On Tue, Sep 26, 2017 at 09:33:54PM -0700, David Miller wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Date: Mon, 25 Sep 2017 14:07:24 +0300
> 
> > +struct tb_property_entry {
> > +	u32 key_hi;
> > +	u32 key_lo;
> > +	u16 length;
> > +	u8 reserved;
> > +	u8 type;
> > +	u32 value;
> > +} __packed;
> > +
> > +struct tb_property_rootdir_entry {
> > +	u32 magic;
> > +	u32 length;
> > +	struct tb_property_entry entries[];
> > +} __packed;
> > +
> > +struct tb_property_dir_entry {
> > +	u32 uuid[4];
> > +	struct tb_property_entry entries[];
> > +} __packed;
> 
> There is no apparent need for __packed here, and __packed should be
> avoided unless absolutely necessary as it pessimizes the code
> significantly on some architectures.
> 
> Please remove __packed from these datastructures unless you can
> prove it is absolutely needed and, in such case, please document
> in a comment why that requirement exists.  Because from the layout
> of these types, everything will be packed in just fine without
> __packed.

I will thanks.

Just for my education, is there some rule which tells when __packed is
to be used? For example the above structures are all 32-bit aligned but
how about something like:

struct foo {
	u32 value1;
	u8 value2;
};

If the on-wire format requires such structures I assume __packed
is needed here?

Thanks!

[toc] | [prev] | [next] | [standalone]

#1740862 — RE: [PATCH v2 02/16] thunderbolt: Add support for XDomain properties

From: Mika Westerberg
> Sent: 27 September 2017 12:33
...
> Just for my education, is there some rule which tells when __packed is
> to be used? For example the above structures are all 32-bit aligned but
> how about something like:
> 
> struct foo {
> 	u32 value1;
> 	u8 value2;
> };
> 
> If the on-wire format requires such structures I assume __packed
> is needed here?

You've endianness considerations as well with on-wire formats.

__packed indicates two things:
1) There will be no padding bytes between fields.
2) The structure itself might appear on any byte boundary.

The latter causes the compiler to do byte memory accesses and
shifts to load/store the data on some architectures.
So only mark things __packed when they might be misaligned in
memory.

	David

[toc] | [prev] | [next] | [standalone]

#1740875

From: Mika Westerberg <mika.westerberg@linux.intel.com>
Date: Wed, 27 Sep 2017 14:32:41 +0300

> Just for my education, is there some rule which tells when __packed is
> to be used? For example the above structures are all 32-bit aligned but
> how about something like:
> 
> struct foo {
> 	u32 value1;
> 	u8 value2;
> };
> 
> If the on-wire format requires such structures I assume __packed
> is needed here?

Usually header elements are 32-bit aligned in a protocol, so it wouldn't
be specified like that.

The only legitimate case I've seen is where things are purposefully
misaligned within the header, like this:

struct foo {
	u16	x;
	u64	y;
	u16	z;
};

Where the 'y' element is 2-byte aligned.

Fortunately, those situations are extremely rare.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web