[PATCH v2] dmaengine: idxd: Remove __packed from structures

[PATCH v2] dmaengine: idxd: Remove __packed from structures

[Yi Sun]

The __packed attribute introduces potential unaligned memory accesses
and endianness portability issues. Instead of relying on compiler-specific
packing, it's much better to explicitly fill structure gaps using padding
fields, ensuring natural alignment.

Since all previously __packed structures already enforce proper alignment
through manual padding, the __packed qualifiers are unnecessary and can be
safely removed.

Signed-off-by: Yi Sun <yi.sun@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>

---
Changelog:
- v2: Change the subject to match idxd driver patch convention (Fenghua)

diff --git a/drivers/dma/idxd/registers.h b/drivers/dma/idxd/registers.h
index 006ba206ab1b..9c1c546fe443 100644
--- a/drivers/dma/idxd/registers.h
+++ b/drivers/dma/idxd/registers.h
@@ -45,7 +45,7 @@ union gen_cap_reg {
 		u64 rsvd3:32;
 	};
 	u64 bits;
-} __packed;
+};
 #define IDXD_GENCAP_OFFSET		0x10
 
 union wq_cap_reg {
@@ -65,7 +65,7 @@ union wq_cap_reg {
 		u64 rsvd4:8;
 	};
 	u64 bits;
-} __packed;
+};
 #define IDXD_WQCAP_OFFSET		0x20
 #define IDXD_WQCFG_MIN			5
 
@@ -79,7 +79,7 @@ union group_cap_reg {
 		u64 rsvd:45;
 	};
 	u64 bits;
-} __packed;
+};
 #define IDXD_GRPCAP_OFFSET		0x30
 
 union engine_cap_reg {
@@ -88,7 +88,7 @@ union engine_cap_reg {
 		u64 rsvd:56;
 	};
 	u64 bits;
-} __packed;
+};
 
 #define IDXD_ENGCAP_OFFSET		0x38
 
@@ -114,7 +114,7 @@ union offsets_reg {
 		u64 rsvd:48;
 	};
 	u64 bits[2];
-} __packed;
+};
 
 #define IDXD_TABLE_MULT			0x100
 
@@ -128,7 +128,7 @@ union gencfg_reg {
 		u32 rsvd2:18;
 	};
 	u32 bits;
-} __packed;
+};
 
 #define IDXD_GENCTRL_OFFSET		0x88
 union genctrl_reg {
@@ -139,7 +139,7 @@ union genctrl_reg {
 		u32 rsvd:29;
 	};
 	u32 bits;
-} __packed;
+};
 
 #define IDXD_GENSTATS_OFFSET		0x90
 union gensts_reg {
@@ -149,7 +149,7 @@ union gensts_reg {
 		u32 rsvd:28;
 	};
 	u32 bits;
-} __packed;
+};
 
 enum idxd_device_status_state {
 	IDXD_DEVICE_STATE_DISABLED = 0,
@@ -183,7 +183,7 @@ union idxd_command_reg {
 		u32 int_req:1;
 	};
 	u32 bits;
-} __packed;
+};
 
 enum idxd_cmd {
 	IDXD_CMD_ENABLE_DEVICE = 1,
@@ -213,7 +213,7 @@ union cmdsts_reg {
 		u8 active:1;
 	};
 	u32 bits;
-} __packed;
+};
 #define IDXD_CMDSTS_ACTIVE		0x80000000
 #define IDXD_CMDSTS_ERR_MASK		0xff
 #define IDXD_CMDSTS_RES_SHIFT		8
@@ -284,7 +284,7 @@ union sw_err_reg {
 		u64 rsvd5;
 	};
 	u64 bits[4];
-} __packed;
+};
 
 union iaa_cap_reg {
 	struct {
@@ -303,7 +303,7 @@ union iaa_cap_reg {
 		u64 rsvd:52;
 	};
 	u64 bits;
-} __packed;
+};
 
 #define IDXD_IAACAP_OFFSET	0x180
 
@@ -320,7 +320,7 @@ union evlcfg_reg {
 		u64 rsvd2:28;
 	};
 	u64 bits[2];
-} __packed;
+};
 
 #define IDXD_EVL_SIZE_MIN	0x0040
 #define IDXD_EVL_SIZE_MAX	0xffff
@@ -334,7 +334,7 @@ union msix_perm {
 		u32 pasid:20;
 	};
 	u32 bits;
-} __packed;
+};
 
 union group_flags {
 	struct {
@@ -352,13 +352,13 @@ union group_flags {
 		u64 rsvd5:26;
 	};
 	u64 bits;
-} __packed;
+};
 
 struct grpcfg {
 	u64 wqs[4];
 	u64 engines;
 	union group_flags flags;
-} __packed;
+};
 
 union wqcfg {
 	struct {
@@ -410,7 +410,7 @@ union wqcfg {
 		u64 op_config[4];
 	};
 	u32 bits[16];
-} __packed;
+};
 
 #define WQCFG_PASID_IDX                2
 #define WQCFG_PRIVL_IDX		2
@@ -474,7 +474,7 @@ union idxd_perfcap {
 		u64 rsvd3:8;
 	};
 	u64 bits;
-} __packed;
+};
 
 #define IDXD_EVNTCAP_OFFSET		0x80
 union idxd_evntcap {
@@ -483,7 +483,7 @@ union idxd_evntcap {
 		u64 rsvd:36;
 	};
 	u64 bits;
-} __packed;
+};
 
 struct idxd_event {
 	union {
@@ -493,7 +493,7 @@ struct idxd_event {
 		};
 		u32 val;
 	};
-} __packed;
+};
 
 #define IDXD_CNTRCAP_OFFSET		0x800
 struct idxd_cntrcap {
@@ -506,7 +506,7 @@ struct idxd_cntrcap {
 		u32 val;
 	};
 	struct idxd_event events[];
-} __packed;
+};
 
 #define IDXD_PERFRST_OFFSET		0x10
 union idxd_perfrst {
@@ -516,7 +516,7 @@ union idxd_perfrst {
 		u32 rsvd:30;
 	};
 	u32 val;
-} __packed;
+};
 
 #define IDXD_OVFSTATUS_OFFSET		0x30
 #define IDXD_PERFFRZ_OFFSET		0x20
@@ -533,7 +533,7 @@ union idxd_cntrcfg {
 		u64 rsvd3:4;
 	};
 	u64 val;
-} __packed;
+};
 
 #define IDXD_FLTCFG_OFFSET		0x300
 
@@ -543,7 +543,7 @@ union idxd_cntrdata {
 		u64 event_count_value;
 	};
 	u64 val;
-} __packed;
+};
 
 union event_cfg {
 	struct {
@@ -551,7 +551,7 @@ union event_cfg {
 		u64 event_enc:28;
 	};
 	u64 val;
-} __packed;
+};
 
 union filter_cfg {
 	struct {
@@ -562,7 +562,7 @@ union filter_cfg {
 		u64 eng:8;
 	};
 	u64 val;
-} __packed;
+};
 
 #define IDXD_EVLSTATUS_OFFSET		0xf0
 
@@ -580,7 +580,7 @@ union evl_status_reg {
 		u32 bits_upper32;
 	};
 	u64 bits;
-} __packed;
+};
 
 #define IDXD_MAX_BATCH_IDENT	256
 
@@ -620,17 +620,17 @@ struct __evl_entry {
 	};
 	u64 fault_addr;
 	u64 rsvd5;
-} __packed;
+};
 
 struct dsa_evl_entry {
 	struct __evl_entry e;
 	struct dsa_completion_record cr;
-} __packed;
+};
 
 struct iax_evl_entry {
 	struct __evl_entry e;
 	u64 rsvd[4];
 	struct iax_completion_record cr;
-} __packed;
+};
 
 #endif
-- 
2.43.0

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Yi Sun]

On 04.04.2025 13:36, Yi Sun wrote:
>The __packed attribute introduces potential unaligned memory accesses
>and endianness portability issues. Instead of relying on compiler-specific
>packing, it's much better to explicitly fill structure gaps using padding
>fields, ensuring natural alignment.
>
>Since all previously __packed structures already enforce proper alignment
>through manual padding, the __packed qualifiers are unnecessary and can be
>safely removed.
>
>Signed-off-by: Yi Sun <yi.sun@intel.com>
>Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>
>
Hi Vinod,

Gentle ping.

Thanks
    --Sun, Yi

>---
>Changelog:
>- v2: Change the subject to match idxd driver patch convention (Fenghua)
>
>diff --git a/drivers/dma/idxd/registers.h b/drivers/dma/idxd/registers.h
>index 006ba206ab1b..9c1c546fe443 100644
>--- a/drivers/dma/idxd/registers.h
>+++ b/drivers/dma/idxd/registers.h
>@@ -45,7 +45,7 @@ union gen_cap_reg {
> 		u64 rsvd3:32;
> 	};
> 	u64 bits;
>-} __packed;
>+};
> #define IDXD_GENCAP_OFFSET		0x10
>
> union wq_cap_reg {
>@@ -65,7 +65,7 @@ union wq_cap_reg {
> 		u64 rsvd4:8;
> 	};
> 	u64 bits;
>-} __packed;
>+};
> #define IDXD_WQCAP_OFFSET		0x20
> #define IDXD_WQCFG_MIN			5
>
>@@ -79,7 +79,7 @@ union group_cap_reg {
> 		u64 rsvd:45;
> 	};
> 	u64 bits;
>-} __packed;
>+};
> #define IDXD_GRPCAP_OFFSET		0x30
>
> union engine_cap_reg {
>@@ -88,7 +88,7 @@ union engine_cap_reg {
> 		u64 rsvd:56;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> #define IDXD_ENGCAP_OFFSET		0x38
>
>@@ -114,7 +114,7 @@ union offsets_reg {
> 		u64 rsvd:48;
> 	};
> 	u64 bits[2];
>-} __packed;
>+};
>
> #define IDXD_TABLE_MULT			0x100
>
>@@ -128,7 +128,7 @@ union gencfg_reg {
> 		u32 rsvd2:18;
> 	};
> 	u32 bits;
>-} __packed;
>+};
>
> #define IDXD_GENCTRL_OFFSET		0x88
> union genctrl_reg {
>@@ -139,7 +139,7 @@ union genctrl_reg {
> 		u32 rsvd:29;
> 	};
> 	u32 bits;
>-} __packed;
>+};
>
> #define IDXD_GENSTATS_OFFSET		0x90
> union gensts_reg {
>@@ -149,7 +149,7 @@ union gensts_reg {
> 		u32 rsvd:28;
> 	};
> 	u32 bits;
>-} __packed;
>+};
>
> enum idxd_device_status_state {
> 	IDXD_DEVICE_STATE_DISABLED = 0,
>@@ -183,7 +183,7 @@ union idxd_command_reg {
> 		u32 int_req:1;
> 	};
> 	u32 bits;
>-} __packed;
>+};
>
> enum idxd_cmd {
> 	IDXD_CMD_ENABLE_DEVICE = 1,
>@@ -213,7 +213,7 @@ union cmdsts_reg {
> 		u8 active:1;
> 	};
> 	u32 bits;
>-} __packed;
>+};
> #define IDXD_CMDSTS_ACTIVE		0x80000000
> #define IDXD_CMDSTS_ERR_MASK		0xff
> #define IDXD_CMDSTS_RES_SHIFT		8
>@@ -284,7 +284,7 @@ union sw_err_reg {
> 		u64 rsvd5;
> 	};
> 	u64 bits[4];
>-} __packed;
>+};
>
> union iaa_cap_reg {
> 	struct {
>@@ -303,7 +303,7 @@ union iaa_cap_reg {
> 		u64 rsvd:52;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> #define IDXD_IAACAP_OFFSET	0x180
>
>@@ -320,7 +320,7 @@ union evlcfg_reg {
> 		u64 rsvd2:28;
> 	};
> 	u64 bits[2];
>-} __packed;
>+};
>
> #define IDXD_EVL_SIZE_MIN	0x0040
> #define IDXD_EVL_SIZE_MAX	0xffff
>@@ -334,7 +334,7 @@ union msix_perm {
> 		u32 pasid:20;
> 	};
> 	u32 bits;
>-} __packed;
>+};
>
> union group_flags {
> 	struct {
>@@ -352,13 +352,13 @@ union group_flags {
> 		u64 rsvd5:26;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> struct grpcfg {
> 	u64 wqs[4];
> 	u64 engines;
> 	union group_flags flags;
>-} __packed;
>+};
>
> union wqcfg {
> 	struct {
>@@ -410,7 +410,7 @@ union wqcfg {
> 		u64 op_config[4];
> 	};
> 	u32 bits[16];
>-} __packed;
>+};
>
> #define WQCFG_PASID_IDX                2
> #define WQCFG_PRIVL_IDX		2
>@@ -474,7 +474,7 @@ union idxd_perfcap {
> 		u64 rsvd3:8;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> #define IDXD_EVNTCAP_OFFSET		0x80
> union idxd_evntcap {
>@@ -483,7 +483,7 @@ union idxd_evntcap {
> 		u64 rsvd:36;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> struct idxd_event {
> 	union {
>@@ -493,7 +493,7 @@ struct idxd_event {
> 		};
> 		u32 val;
> 	};
>-} __packed;
>+};
>
> #define IDXD_CNTRCAP_OFFSET		0x800
> struct idxd_cntrcap {
>@@ -506,7 +506,7 @@ struct idxd_cntrcap {
> 		u32 val;
> 	};
> 	struct idxd_event events[];
>-} __packed;
>+};
>
> #define IDXD_PERFRST_OFFSET		0x10
> union idxd_perfrst {
>@@ -516,7 +516,7 @@ union idxd_perfrst {
> 		u32 rsvd:30;
> 	};
> 	u32 val;
>-} __packed;
>+};
>
> #define IDXD_OVFSTATUS_OFFSET		0x30
> #define IDXD_PERFFRZ_OFFSET		0x20
>@@ -533,7 +533,7 @@ union idxd_cntrcfg {
> 		u64 rsvd3:4;
> 	};
> 	u64 val;
>-} __packed;
>+};
>
> #define IDXD_FLTCFG_OFFSET		0x300
>
>@@ -543,7 +543,7 @@ union idxd_cntrdata {
> 		u64 event_count_value;
> 	};
> 	u64 val;
>-} __packed;
>+};
>
> union event_cfg {
> 	struct {
>@@ -551,7 +551,7 @@ union event_cfg {
> 		u64 event_enc:28;
> 	};
> 	u64 val;
>-} __packed;
>+};
>
> union filter_cfg {
> 	struct {
>@@ -562,7 +562,7 @@ union filter_cfg {
> 		u64 eng:8;
> 	};
> 	u64 val;
>-} __packed;
>+};
>
> #define IDXD_EVLSTATUS_OFFSET		0xf0
>
>@@ -580,7 +580,7 @@ union evl_status_reg {
> 		u32 bits_upper32;
> 	};
> 	u64 bits;
>-} __packed;
>+};
>
> #define IDXD_MAX_BATCH_IDENT	256
>
>@@ -620,17 +620,17 @@ struct __evl_entry {
> 	};
> 	u64 fault_addr;
> 	u64 rsvd5;
>-} __packed;
>+};
>
> struct dsa_evl_entry {
> 	struct __evl_entry e;
> 	struct dsa_completion_record cr;
>-} __packed;
>+};
>
> struct iax_evl_entry {
> 	struct __evl_entry e;
> 	u64 rsvd[4];
> 	struct iax_completion_record cr;
>-} __packed;
>+};
>
> #endif
>-- 
>2.43.0
>

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Vinod Koul]

On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
> The __packed attribute introduces potential unaligned memory accesses
> and endianness portability issues. Instead of relying on compiler-specific
> packing, it's much better to explicitly fill structure gaps using padding
> fields, ensuring natural alignment.
> 
> Since all previously __packed structures already enforce proper alignment
> through manual padding, the __packed qualifiers are unnecessary and can be
> safely removed.
> 
> [...]

Applied, thanks!

[1/1] dmaengine: idxd: Remove __packed from structures
      commit: 671a654aecc710a278bdd30cfd2afef2d4e0828f

Best regards,
-- 
~Vinod

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Andy Shevchenko]

Thu, Jun 26, 2025 at 03:48:11PM -0700, Vinod Koul kirjoitti:
> 
> On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
> > The __packed attribute introduces potential unaligned memory accesses
> > and endianness portability issues. Instead of relying on compiler-specific
> > packing, it's much better to explicitly fill structure gaps using padding
> > fields, ensuring natural alignment.
> > 
> > Since all previously __packed structures already enforce proper alignment
> > through manual padding, the __packed qualifiers are unnecessary and can be
> > safely removed.

[...]

> Applied, thanks!

Please, don't or fix it ASAP. This patch is broken in the formal things,
i.e. changelog entry must not disrupt SoB chain. I'm not sure if Stephen's
scripts will catch this up on Linux Next integration, though.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Yi Sun]

On 27.06.2025 07:05, Andy Shevchenko wrote:
>Thu, Jun 26, 2025 at 03:48:11PM -0700, Vinod Koul kirjoitti:
>>
>> On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
>> > The __packed attribute introduces potential unaligned memory accesses
>> > and endianness portability issues. Instead of relying on compiler-specific
>> > packing, it's much better to explicitly fill structure gaps using padding
>> > fields, ensuring natural alignment.
>> >
>> > Since all previously __packed structures already enforce proper alignment
>> > through manual padding, the __packed qualifiers are unnecessary and can be
>> > safely removed.
>
>[...]
>
>> Applied, thanks!
>
>Please, don't or fix it ASAP. This patch is broken in the formal things,
>i.e. changelog entry must not disrupt SoB chain. I'm not sure if Stephen's
>scripts will catch this up on Linux Next integration, though.
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>

Hi Andy

 From what I understand, changelog comments are ignored by git am and do not
interfere with the SoB chain. They appear after the "---" separator and
aren't part of the actual commit message. So it should be safe and won't
break anything during the integration.

Let me know if there's something I might have missed.

Thanks
    --Sun, Yi

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Andy Shevchenko]

On Fri, Jun 27, 2025 at 9:30 AM Yi Sun <yi.sun@intel.com> wrote:
>
> On 27.06.2025 07:05, Andy Shevchenko wrote:
> >Thu, Jun 26, 2025 at 03:48:11PM -0700, Vinod Koul kirjoitti:
> >>
> >> On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
> >> > The __packed attribute introduces potential unaligned memory accesses
> >> > and endianness portability issues. Instead of relying on compiler-specific
> >> > packing, it's much better to explicitly fill structure gaps using padding
> >> > fields, ensuring natural alignment.
> >> >
> >> > Since all previously __packed structures already enforce proper alignment
> >> > through manual padding, the __packed qualifiers are unnecessary and can be
> >> > safely removed.
> >
> >[...]
> >
> >> Applied, thanks!
> >
> >Please, don't or fix it ASAP. This patch is broken in the formal things,
> >i.e. changelog entry must not disrupt SoB chain. I'm not sure if Stephen's
> >scripts will catch this up on Linux Next integration, though.
> >
> >--
> >With Best Regards,
> >Andy Shevchenko
> >
> >
>
> Hi Andy
>
>  From what I understand, changelog comments are ignored by git am and do not
> interfere with the SoB chain. They appear after the "---" separator and
> aren't part of the actual commit message. So it should be safe and won't
> break anything during the integration.
>
> Let me know if there's something I might have missed.

Please, look how it looks in the repository:
https://web.git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?id=671a654aecc710a278bdd30cfd2afef2d4e0828f
This is wrong.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Yi Sun]

On 27.06.2025 09:57, Andy Shevchenko wrote:
>On Fri, Jun 27, 2025 at 9:30 AM Yi Sun <yi.sun@intel.com> wrote:
>>
>> On 27.06.2025 07:05, Andy Shevchenko wrote:
>> >Thu, Jun 26, 2025 at 03:48:11PM -0700, Vinod Koul kirjoitti:
>> >>
>> >> On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
>> >> > The __packed attribute introduces potential unaligned memory accesses
>> >> > and endianness portability issues. Instead of relying on compiler-specific
>> >> > packing, it's much better to explicitly fill structure gaps using padding
>> >> > fields, ensuring natural alignment.
>> >> >
>> >> > Since all previously __packed structures already enforce proper alignment
>> >> > through manual padding, the __packed qualifiers are unnecessary and can be
>> >> > safely removed.
>> >
>> >[...]
>> >
>> >> Applied, thanks!
>> >
>> >Please, don't or fix it ASAP. This patch is broken in the formal things,
>> >i.e. changelog entry must not disrupt SoB chain. I'm not sure if Stephen's
>> >scripts will catch this up on Linux Next integration, though.
>> >
>> >--
>> >With Best Regards,
>> >Andy Shevchenko
>> >
>> >
>>
>> Hi Andy
>>
>>  From what I understand, changelog comments are ignored by git am and do not
>> interfere with the SoB chain. They appear after the "---" separator and
>> aren't part of the actual commit message. So it should be safe and won't
>> break anything during the integration.
>>
>> Let me know if there's something I might have missed.
>
>Please, look how it looks in the repository:
>https://web.git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?id=671a654aecc710a278bdd30cfd2afef2d4e0828f
>This is wrong.
>
>
>-- 
>With Best Regards,
>Andy Shevchenko

Oops, I see. Something went wrong during the integration.
I didn't notice Vinod's repo.

Thanks
    --Sun, Yi

Re: [PATCH v2] dmaengine: idxd: Remove __packed from structures

[Vinod Koul]

On 27-06-25, 07:05, Andy Shevchenko wrote:
> Thu, Jun 26, 2025 at 03:48:11PM -0700, Vinod Koul kirjoitti:
> > 
> > On Fri, 04 Apr 2025 13:36:14 +0800, Yi Sun wrote:
> > > The __packed attribute introduces potential unaligned memory accesses
> > > and endianness portability issues. Instead of relying on compiler-specific
> > > packing, it's much better to explicitly fill structure gaps using padding
> > > fields, ensuring natural alignment.
> > > 
> > > Since all previously __packed structures already enforce proper alignment
> > > through manual padding, the __packed qualifiers are unnecessary and can be
> > > safely removed.
> 
> [...]
> 
> > Applied, thanks!
> 
> Please, don't or fix it ASAP. This patch is broken in the formal things,
> i.e. changelog entry must not disrupt SoB chain. I'm not sure if Stephen's
> scripts will catch this up on Linux Next integration, though.

Thanks for letting me know. My script didnt catch it, will check why.
I have fixed it up now

-- 
~Vinod