On 18.12.23 13:21, Wen Gu wrote: > The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common > seem to have not changed since SMCDv1. So I guess there is no v2-only fields > in this two struct. I tried to confirm this in some documents but didn't find > the message format for v1. V1 is documented in https://datatracker.ietf.org/doc/html/draft-fox-tcpm-shared-memory-rdma-03 > > If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common > is inherited from v1, should we still put the fields of v2 into these two structures? You are right, they do not contain v2 fields, I guess I was confused. I still think, it would be better for readability and maintainability to avoid +#define r0 r1._r0 +#define d0 d1._d0 I guess you and previous editors wanted to avoid changing all the instances that use r0 and d0. But then.. it is a rather simple search/replace.. > > If still, I will change these structures as > > diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h > index 614fa2f298f5..18157aeb14ec 100644 > --- a/net/smc/smc_clc.h > +++ b/net/smc/smc_clc.h > @@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm { /* SMCR accept/confirm */ > __be64 rmb_dma_addr; /* RMB virtual address */ > u8 reserved2; > u8 psn[3]; /* packet sequence number */ > + /* v2 only, reserved and ignored in v1: */ > + u8 eid[SMC_MAX_EID_LEN]; > + u8 reserved6[8]; > } __packed; > > -struct smcd_clc_msg_accept_confirm_common { /* SMCD accept/confirm */ > +struct smcd_clc_msg_accept_confirm { /* SMCD accept/confirm */ > __be64 gid; /* Sender GID */ > __be64 token; /* DMB token */ > u8 dmbe_idx; /* DMBE index */ > @@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common { /* SMCD accept/confirm */ > #endif > u16 reserved4; > __be32 linkid; /* Link identifier */ > + /* v2 only, reserved and ignored in v1: */ > + __be16 chid; > + u8 eid[SMC_MAX_EID_LEN]; > + u8 reserved5[8]; > } __packed; > > #define SMC_CLC_OS_ZOS 1 > @@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext { > struct smc_clc_msg_accept_confirm { /* clc accept / confirm message */ > struct smc_clc_msg_hdr hdr; > union { > - struct { /* SMC-R */ > - struct smcr_clc_msg_accept_confirm _r0; > - /* v2 only, reserved and ignored in v1: */ ^^ Actually these commetns are not fully correct. The fields are not reserved in V1. (my bad) The message length is shorter in V1. So /* v2 only: */ would be more correct. > - u8 eid[SMC_MAX_EID_LEN]; > - u8 reserved6[8]; > - } r1; > - struct { /* SMC-D */ > - struct smcd_clc_msg_accept_confirm_common _d0; > - /* v2 only, reserved and ignored in v1: */ same here: /* v2 only: */ > - __be16 chid; > - u8 eid[SMC_MAX_EID_LEN]; > - u8 reserved5[8]; > - } d1; > + struct smcr_clc_msg_accept_confirm r0; /* SMC-R */ > + struct smcd_clc_msg_accept_confirm d0; /* SMC-D */ > }; > -#define r0 r1._r0 > -#define d0 d1._d0 > }; > >> >>> }; Yes, I like that solution better. But I have no strong feelings. At least the duplicate declarations are gone. So, if you prefer the #defines , it's ok with me. >> >> You have removed the __packed attribute. >> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well. >> > > r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in > this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext, > thus making a hole before gid_ext, so I added __packed attribute to SMC-D. > > If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R. > Yes, __packed is not only about preventing misalignement today. IMU, without __packed, there is no guarantee that a future compile run will not insert unused bytes. (highly unlikely, I admit). But __packed makes it visible that this needs to go to hardware in exactly this layout. > Thanks.
