[net-next PATCH v3] octeon_ep: pack hardware structure

[net-next PATCH v3] octeon_ep: pack hardware structure

[Shinas Rasheed]

Clean up structure defines related to hardware data to be
attributed 'packed' in the code, as padding is not allowed
by hardware.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V3:
  - Updated changelog to indicate this is a cleanup
V2: https://lore.kernel.org/all/20231010194026.2284786-1-srasheed@marvell.com/
  - Updated changelog to provide more information
V1: https://lore.kernel.org/all/20231006120225.2259533-1-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_rx.h | 6 +++---
 drivers/net/ethernet/marvell/octeon_ep/octep_tx.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
index 782a24f27f3e..ca42ddb77491 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
@@ -19,7 +19,7 @@
 struct octep_oq_desc_hw {
 	dma_addr_t buffer_ptr;
 	u64 info_ptr;
-};
+} __packed;
 
 #define OCTEP_OQ_DESC_SIZE    (sizeof(struct octep_oq_desc_hw))
 
@@ -38,7 +38,7 @@ struct octep_oq_resp_hw_ext {
 
 	/* checksum verified. */
 	u64 csum_verified:2;
-};
+} __packed;
 
 #define  OCTEP_OQ_RESP_HW_EXT_SIZE   (sizeof(struct octep_oq_resp_hw_ext))
 
@@ -49,7 +49,7 @@ struct octep_oq_resp_hw_ext {
 struct octep_oq_resp_hw {
 	/* The Length of the packet. */
 	__be64 length;
-};
+} __packed;
 
 #define OCTEP_OQ_RESP_HW_SIZE   (sizeof(struct octep_oq_resp_hw))
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
index 21e75ff9f5e7..74189e5a7d33 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
@@ -35,7 +35,7 @@
 struct octep_tx_sglist_desc {
 	u16 len[4];
 	dma_addr_t dma_ptr[4];
-};
+} __packed;
 
 /* Each Scatter/Gather entry sent to hardwar hold four pointers.
  * So, number of entries required is (MAX_SKB_FRAGS + 1)/4, where '+1'
@@ -238,7 +238,7 @@ struct octep_instr_hdr {
 
 	/* Reserved3 */
 	u64 reserved3:1;
-};
+} __packed;
 
 /* Hardware Tx completion response header */
 struct octep_instr_resp_hdr {
@@ -262,7 +262,7 @@ struct octep_instr_resp_hdr {
 
 	/* Opcode for the return packet  */
 	u64 opcode:16;
-};
+} __packed;
 
 /* 64-byte Tx instruction format.
  * Format of instruction for a 64-byte mode input queue.
@@ -292,7 +292,7 @@ struct octep_tx_desc_hw {
 
 	/* Additional headers available in a 64-byte instruction. */
 	u64 exhdr[4];
-};
+} __packed;
 
 #define OCTEP_IQ_DESC_SIZE (sizeof(struct octep_tx_desc_hw))
 #endif /* _OCTEP_TX_H_ */
-- 
2.25.1

Re: [net-next PATCH v3] octeon_ep: pack hardware structure

[Simon Horman]

On Mon, Oct 16, 2023 at 02:20:51AM -0700, Shinas Rasheed wrote:
> Clean up structure defines related to hardware data to be
> attributed 'packed' in the code, as padding is not allowed
> by hardware.
> 
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V3:
>   - Updated changelog to indicate this is a cleanup

Thanks, the patch description now matches my understanding
of the intent of the patch.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net-next PATCH v3] octeon_ep: pack hardware structure

[Jakub Kicinski]

On Mon, 16 Oct 2023 02:20:51 -0700 Shinas Rasheed wrote:
> Clean up structure defines related to hardware data to be
> attributed 'packed' in the code, as padding is not allowed
> by hardware.

Looks like the patch was marked as Changes Requested in pw.
I'm guessing it's because we generally discourage __packed.
It's better to add size asserts, e.g.:

static_assert(sizeof(struct octep_oq_desc_hw) == 16);

__packed also implies lack of alignment, which may force compiler 
to generate worse code.

Re: [EXT] Re: [net-next PATCH v3] octeon_ep: pack hardware structure

[Shinas Rasheed]

Hi Jakub,

Since these structures represent how hardware expects data, there can be a lack of alignment. I'm afraid static asserting all the hardware data structures might force some compilers to fail? 

Re: [EXT] Re: [net-next PATCH v3] octeon_ep: pack hardware structure

[Jakub Kicinski]

On Thu, 19 Oct 2023 18:46:09 +0000 Shinas Rasheed wrote:
> Since these structures represent how hardware expects data, there can
> be a lack of alignment.

Doesn't the host allocate at least most of those?
Therefore controlling the alignment?

> I'm afraid static asserting all the hardware data structures might
> force some compilers to fail?

C has structure packing rules which mean that in 99.9999% of the cases
none of your structs need explicit packing. For the 0.0001% of arches
breaking build is fine.

At least that's my guess, again, I wasn't the one who rejected the
patch. I just noticed it was dropped in patchwork and made a guess
based on past experience.

Re: [EXT] Re: [net-next PATCH v3] octeon_ep: pack hardware structure

[Shinas Rasheed]

Ok, I understand. Thanks for your input Jakub. I will submit another patch correcting it.