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

[Li Ming <ming.li@zohomail.com>]

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]