| Message ID | 20250409024825.510632-1-ming.li@zohomail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/1] cxl/feature: Using Full Data Transfer only when offset is 0 | expand |
On 4/8/25 7:48 PM, Li Ming wrote: > Per CXL r3.2 section 8.2.10.6.3 Set Feature(Opcode 0502h) > > "If the Feature data is transferred in its entirely, the caller makes > one call to Set Feature with Action = Full Data Transfer. The Offset > field is not used and shall be ignored." > > It implies if using Full Data Transfer, the received data should be > updated from offset 0 on device side. > > Current driver implementation is if feature data size is less than mbox > payload - set feature header, driver will use Full Data Transfer even if > user provides an offset. Per above description, feature data will be > written from offset 0 rather than the offset value user provided, the > result will not be what user expects. > > The changes is checking if the offset caller provides is equal to 0, if > yes, using Full Data Transfer, if not, using Initiate Data Transfer - > Finish Data Transfer. > > After the changes, all Set Feature transfer scenarios are below: > > 1. data size + header is less than mbox payload and offset is 0 > Full Data Transfer > > 2. data size + header is less than mbox payload and offset is not 0 > Initiate Data Transfer(with all feature data) - Finish Data Transfer(with no feature data) > > 3. data size + header us greater than mbox payload with any value of offset > Initiate Data Transfer - Continue Data Transfer - Finish Data Transfer > > Fixes: 14d502cc2718e ("cxl/mbox: Add SET_FEATURE mailbox command") > Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Thanks for this. LGTM. You can drop the RFC. > --- > base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 v6.15-rc1 > --- > drivers/cxl/core/features.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c > index f4daefe3180e..fcc624cefe89 100644 > --- a/drivers/cxl/core/features.c > +++ b/drivers/cxl/core/features.c > @@ -259,6 +259,17 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid, > return data_rcvd_size; > } > > +static bool is_last_feat_transfer(struct cxl_mbox_set_feat_in *pi) > +{ > + switch (le32_to_cpu(pi->flags) & CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK) { > + case CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER: > + case CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER: > + return true; > + default: > + return false; > + } > +} > + > /* > * FEAT_DATA_MIN_PAYLOAD_SIZE - min extra number of bytes should be > * available in the mailbox for storing the actual feature data so that > @@ -294,14 +305,14 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, > if (hdr_size + FEAT_DATA_MIN_PAYLOAD_SIZE > cxl_mbox->payload_size) > return -ENOMEM; > > - if (hdr_size + feat_data_size <= cxl_mbox->payload_size) { > + if (hdr_size + feat_data_size <= cxl_mbox->payload_size && !offset) { > pi->flags = cpu_to_le32(feat_flag | > CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER); > data_in_size = feat_data_size; > } else { > pi->flags = cpu_to_le32(feat_flag | > CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER); > - data_in_size = cxl_mbox->payload_size - hdr_size; > + data_in_size = min(feat_data_size, cxl_mbox->payload_size - hdr_size); > } > > do { > @@ -322,7 +333,8 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, > } > > data_sent_size += data_in_size; > - if (data_sent_size >= feat_data_size) { > + if (data_sent_size >= feat_data_size && > + is_last_feat_transfer(pi)) { > if (return_code) > *return_code = CXL_MBOX_CMD_RC_SUCCESS; > return 0;
On Wed, 09 Apr 2025, Li Ming wrote: >Per CXL r3.2 section 8.2.10.6.3 Set Feature(Opcode 0502h) > >"If the Feature data is transferred in its entirely, the caller makes >one call to Set Feature with Action = Full Data Transfer. The Offset >field is not used and shall be ignored." > >It implies if using Full Data Transfer, the received data should be >updated from offset 0 on device side. > >Current driver implementation is if feature data size is less than mbox >payload - set feature header, driver will use Full Data Transfer even if >user provides an offset. Per above description, feature data will be >written from offset 0 rather than the offset value user provided, the >result will not be what user expects. > >The changes is checking if the offset caller provides is equal to 0, if >yes, using Full Data Transfer, if not, using Initiate Data Transfer - >Finish Data Transfer. > >After the changes, all Set Feature transfer scenarios are below: > >1. data size + header is less than mbox payload and offset is 0 > Full Data Transfer > >2. data size + header is less than mbox payload and offset is not 0 > Initiate Data Transfer(with all feature data) - Finish Data Transfer(with no feature data) > >3. data size + header us greater than mbox payload with any value of offset > Initiate Data Transfer - Continue Data Transfer - Finish Data Transfer > >Fixes: 14d502cc2718e ("cxl/mbox: Add SET_FEATURE mailbox command") >Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c index f4daefe3180e..fcc624cefe89 100644 --- a/drivers/cxl/core/features.c +++ b/drivers/cxl/core/features.c @@ -259,6 +259,17 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid, return data_rcvd_size; } +static bool is_last_feat_transfer(struct cxl_mbox_set_feat_in *pi) +{ + switch (le32_to_cpu(pi->flags) & CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK) { + case CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER: + case CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER: + return true; + default: + return false; + } +} + /* * FEAT_DATA_MIN_PAYLOAD_SIZE - min extra number of bytes should be * available in the mailbox for storing the actual feature data so that @@ -294,14 +305,14 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, if (hdr_size + FEAT_DATA_MIN_PAYLOAD_SIZE > cxl_mbox->payload_size) return -ENOMEM; - if (hdr_size + feat_data_size <= cxl_mbox->payload_size) { + if (hdr_size + feat_data_size <= cxl_mbox->payload_size && !offset) { pi->flags = cpu_to_le32(feat_flag | CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER); data_in_size = feat_data_size; } else { pi->flags = cpu_to_le32(feat_flag | CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER); - data_in_size = cxl_mbox->payload_size - hdr_size; + data_in_size = min(feat_data_size, cxl_mbox->payload_size - hdr_size); } do { @@ -322,7 +333,8 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, } data_sent_size += data_in_size; - if (data_sent_size >= feat_data_size) { + if (data_sent_size >= feat_data_size && + is_last_feat_transfer(pi)) { if (return_code) *return_code = CXL_MBOX_CMD_RC_SUCCESS; return 0;
Per CXL r3.2 section 8.2.10.6.3 Set Feature(Opcode 0502h) "If the Feature data is transferred in its entirely, the caller makes one call to Set Feature with Action = Full Data Transfer. The Offset field is not used and shall be ignored." It implies if using Full Data Transfer, the received data should be updated from offset 0 on device side. Current driver implementation is if feature data size is less than mbox payload - set feature header, driver will use Full Data Transfer even if user provides an offset. Per above description, feature data will be written from offset 0 rather than the offset value user provided, the result will not be what user expects. The changes is checking if the offset caller provides is equal to 0, if yes, using Full Data Transfer, if not, using Initiate Data Transfer - Finish Data Transfer. After the changes, all Set Feature transfer scenarios are below: 1. data size + header is less than mbox payload and offset is 0 Full Data Transfer 2. data size + header is less than mbox payload and offset is not 0 Initiate Data Transfer(with all feature data) - Finish Data Transfer(with no feature data) 3. data size + header us greater than mbox payload with any value of offset Initiate Data Transfer - Continue Data Transfer - Finish Data Transfer Fixes: 14d502cc2718e ("cxl/mbox: Add SET_FEATURE mailbox command") Signed-off-by: Li Ming <ming.li@zohomail.com> --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 v6.15-rc1 --- drivers/cxl/core/features.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)