[PATCH] can.h: make padding given by gcc explicit

[PATCH] can.h: make padding given by gcc explicit

[Shawn Landden]

The current definition of struct can_frame has a 16-byte size, with 8-byte
alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
of padding of struct canfd_frame. Make it explicit so it is easier to read.

Signed-off-by: Shawn Landden <shawn@churchofgit.com>
---
 include/uapi/linux/can.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 41892f7..bc1b937 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -100,6 +100,7 @@ typedef __u32 can_err_mask_t;
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
 	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
+        __u8    __res[3];/* reserved / padding */
 	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
 };
 
-- 
2.2.1.209.g41e5f3a

Re: [PATCH] can.h: make padding given by gcc explicit

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On 05/01/2015 09:54 PM, Shawn Landden wrote:
> The current definition of struct can_frame has a 16-byte size, with 8-byte
> alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
> of padding of struct canfd_frame. Make it explicit so it is easier to read.
> 
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>

As this is a userspace API header, I'd rather not change it for
readability reasons..... Oliver's on holidays currently, let's wait for
his oppinion.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] can.h: make padding given by gcc explicit

[Oliver Hartkopp]

Hi Shawn,

fortunately some camping sites have free WLAN :-)

Usually the user space API (== ABI) is written into stone but making the
implicite padding more explicit looks good from my side.

The problem was, that in the early days of SocketCAN people would have seen
this kind of 'free space' as an invitation to put bogus stuff like CAN
controller mailbox indices here %-(

To be somehow analogue to the struct canfd_frame I would suggest this layout:

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 41892f7..9692cda 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -95,11 +95,17 @@ typedef __u32 can_err_mask_t;
  * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
  *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
  *           mapping of the 'data length code' to the real payload length
+ * @__pad:   padding
+ * @__res0:  reserved / padding
+ * @__res1:  reserved / padding
  * @data:    CAN frame payload (up to 8 byte)
  */
 struct can_frame {
        canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
        __u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
+       __u8    __pad;   /* padding */
+       __u8    __res0;  /* reserved / padding */
+       __u8    __res1;  /* reserved / padding */
        __u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
 };
 

And when providing a an update of this central data structure the references
in Documentation/networking/can.txt have to be updated too :-)

@Marc: Do you have any objections to add this padding stuff?

Regards,
Oliver

ps. I don't know when I will be online next time. But net-next is not closing until
I'm back to work ... no hurry.

On 05/01/2015 09:54 PM, Shawn Landden wrote:
> The current definition of struct can_frame has a 16-byte size, with 8-byte
> alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
> of padding of struct canfd_frame. Make it explicit so it is easier to read.
> 
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
> ---
>  include/uapi/linux/can.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 41892f7..bc1b937 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -100,6 +100,7 @@ typedef __u32 can_err_mask_t;
>  struct can_frame {
>  	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>  	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
> +        __u8    __res[3];/* reserved / padding */
>  	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
>  };
>  
> 

[PATCH] can.h: make padding given by gcc explicit

[Shawn Landden]

The current definition of struct can_frame has a 16-byte size, with 8-byte
alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
of padding of struct canfd_frame. Make it explicit so it is easier to read.

v2: match analogous padding fields in canfd_frame
    update Documentation
---
 Documentation/networking/can.txt | 3 +++
 include/uapi/linux/can.h         | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt
index 5abad1e..b48d4a1 100644
--- a/Documentation/networking/can.txt
+++ b/Documentation/networking/can.txt
@@ -268,6 +268,9 @@ solution for a couple of reasons:
     struct can_frame {
             canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
             __u8    can_dlc; /* frame payload length in byte (0 .. 8) */
+            __u8    __pad;   /* padding */
+            __u8    __res0;  /* reserved / padding */
+            __u8    __res1;  /* reserved / padding */
             __u8    data[8] __attribute__((aligned(8)));
     };
 
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 41892f7..9692cda 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -95,11 +95,17 @@ typedef __u32 can_err_mask_t;
  * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
  *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
  *           mapping of the 'data length code' to the real payload length
+ * @__pad:   padding
+ * @__res0:  reserved / padding
+ * @__res1:  reserved / padding
  * @data:    CAN frame payload (up to 8 byte)
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
 	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
+	__u8    __pad;   /* padding */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
 	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
 };
 
-- 
2.2.1.209.g41e5f3a

Re: [PATCH] can.h: make padding given by gcc explicit

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On 05/05/2015 09:03 AM, Shawn Landden wrote:
> The current definition of struct can_frame has a 16-byte size, with 8-byte
> alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
> of padding of struct canfd_frame. Make it explicit so it is easier to read.
> 
> v2: match analogous padding fields in canfd_frame
>     update Documentation

Can you add your S-o-b?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH] can.h: make padding given by gcc explicit

[Shawn Landden]

The current definition of struct can_frame has a 16-byte size, with 8-byte
alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
of padding of struct canfd_frame. Make it explicit so it is easier to read.

v2: match analogous padding fields in canfd_frame
    update Documentation

Signed-off-by: Shawn Landden <shawn@churchofgit.com>
---
 Documentation/networking/can.txt | 3 +++
 include/uapi/linux/can.h         | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt
index 5abad1e..b48d4a1 100644
--- a/Documentation/networking/can.txt
+++ b/Documentation/networking/can.txt
@@ -268,6 +268,9 @@ solution for a couple of reasons:
     struct can_frame {
             canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
             __u8    can_dlc; /* frame payload length in byte (0 .. 8) */
+            __u8    __pad;   /* padding */
+            __u8    __res0;  /* reserved / padding */
+            __u8    __res1;  /* reserved / padding */
             __u8    data[8] __attribute__((aligned(8)));
     };
 
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 41892f7..9692cda 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -95,11 +95,17 @@ typedef __u32 can_err_mask_t;
  * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
  *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
  *           mapping of the 'data length code' to the real payload length
+ * @__pad:   padding
+ * @__res0:  reserved / padding
+ * @__res1:  reserved / padding
  * @data:    CAN frame payload (up to 8 byte)
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
 	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
+	__u8    __pad;   /* padding */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
 	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
 };
 
-- 
2.2.1.209.g41e5f3a

Re: [PATCH] can.h: make padding given by gcc explicit

[Oliver Hartkopp]

On 05/05/2015 06:07 PM, Shawn Landden wrote:
> The current definition of struct can_frame has a 16-byte size, with 8-byte
> alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
> of padding of struct canfd_frame. Make it explicit so it is easier to read.
> 
> v2: match analogous padding fields in canfd_frame
>     update Documentation
> 
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Shawn!

> ---
>  Documentation/networking/can.txt | 3 +++
>  include/uapi/linux/can.h         | 6 ++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt
> index 5abad1e..b48d4a1 100644
> --- a/Documentation/networking/can.txt
> +++ b/Documentation/networking/can.txt
> @@ -268,6 +268,9 @@ solution for a couple of reasons:
>      struct can_frame {
>              canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>              __u8    can_dlc; /* frame payload length in byte (0 .. 8) */
> +            __u8    __pad;   /* padding */
> +            __u8    __res0;  /* reserved / padding */
> +            __u8    __res1;  /* reserved / padding */
>              __u8    data[8] __attribute__((aligned(8)));
>      };
>  
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 41892f7..9692cda 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -95,11 +95,17 @@ typedef __u32 can_err_mask_t;
>   * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
>   *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
>   *           mapping of the 'data length code' to the real payload length
> + * @__pad:   padding
> + * @__res0:  reserved / padding
> + * @__res1:  reserved / padding
>   * @data:    CAN frame payload (up to 8 byte)
>   */
>  struct can_frame {
>  	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>  	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
> +	__u8    __pad;   /* padding */
> +	__u8    __res0;  /* reserved / padding */
> +	__u8    __res1;  /* reserved / padding */
>  	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
>  };
>  
> 

Re: [PATCH] can.h: make padding given by gcc explicit

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On 05/05/2015 06:07 PM, Shawn Landden wrote:
> The current definition of struct can_frame has a 16-byte size, with 8-byte
> alignment, but the 3 bytes of padding are not explicit like the similar 2 bytes
> of padding of struct canfd_frame. Make it explicit so it is easier to read.
> 
> v2: match analogous padding fields in canfd_frame
>     update Documentation

Applied to can-next.

Thanks,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]