| Message ID | 20231016092051.2306831-1-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] octeon_ep: pack hardware structure | expand |
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>
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.
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?
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.
Ok, I understand. Thanks for your input Jakub. I will submit another patch correcting it.
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_ */
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(-)