Re: [RFC PATCH 1/1] cxl/feature: Using Full Data Transfer only when offset is 0

[Dave Jiang]

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@xxxxxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>

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;