| Message ID | 20250410024726.514170-1-ming.li@zohomail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] cxl/feature: Using Full Data Transfer only when offset is 0 | expand |
On Thu, 10 Apr 2025 10:47:26 +0800 Li Ming <ming.li@zohomail.com> 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 entirety > 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 will 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 Is it valid to call this function with a none zero offset? I'm not sure. In my possibly entirely incorrect mental model the offset should be an internal detail of how we get the features that we should not expose to userspace (oops). The specification is confusing on this point so I think we should ask for a clarification. There is text that for example says "Once the _entire_ Feature data is fully transferred to the device (i.e. Action = Full Data Transfer or Action = Finish Data Transfer"), the device shall update the attribute(s) of the Feature." What does 'entire Feature data mean here?' The whole thing or just the bit from offset used in Initiate Data Transfer onwards? I'd read entire as meaning all of it so effectively the offset passed to this function should always be zero. I'll send a query and let you know the answer. Maybe I'm reading too much into that 'entire'. J > Initiate Data Transfer(with all feature data) - Finish Data Transfer(with no feature data) > > 3. data size + header is greater than mbox payload with any value of offset > Initiate Data Transfer - Continue Data Transfer(s) - Finish Data Transfer > > Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command") > Signed-off-by: Li Ming <ming.li@zohomail.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 v6.15-rc1 > > v2: > - Drop RFC tag. (Dave) > - Fix typo issue in changelog > --- > 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 4/15/2025 10:36 PM, Jonathan Cameron wrote: > On Thu, 10 Apr 2025 10:47:26 +0800 > Li Ming <ming.li@zohomail.com> 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 > entirety > >> 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 will 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 > Is it valid to call this function with a none zero offset? I'm not sure. > In my possibly entirely incorrect mental model the offset should be > an internal detail of how we get the features that we should not expose to > userspace (oops). The specification is confusing on this point so I think > we should ask for a clarification. > > There is text that for example says > "Once the _entire_ Feature data is fully transferred to the device (i.e. > Action = Full Data Transfer or Action = Finish Data Transfer"), the > device shall update the attribute(s) of the Feature." > > What does 'entire Feature data mean here?' The whole thing or just > the bit from offset used in Initiate Data Transfer onwards? > > I'd read entire as meaning all of it so effectively the offset passed > to this function should always be zero. > > I'll send a query and let you know the answer. Maybe I'm reading too > much into that 'entire'. > > J Thanks for that. I have the same doubt too. 'entire' can be whole feature data or just mean a part of feature data needed to be updated. If 'entire' means whole feature data, it will require Initiate Data Transfer to be with an zero offset. But I can't find any description of SPEC including this hint. And current set feature implementation does not have such limitation either, so I made current solution. I agree with you that we should have an accurate answer. Ming [snip]
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 will 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 is greater than mbox payload with any value of offset Initiate Data Transfer - Continue Data Transfer(s) - Finish Data Transfer Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command") Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 v6.15-rc1 v2: - Drop RFC tag. (Dave) - Fix typo issue in changelog --- drivers/cxl/core/features.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)