Re: [PATCH v3 4/6] xen: Switch to byteswap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.05.2022 16:21, Bertrand Marquis wrote:
>> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xxxxxxx> wrote:
>>> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>>> {
>>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>>> Why did you drop the __force?
>>>> Google told me __force is used in linux kernel to suppress warning in 
>>>> sparse,
>>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>>> Is sparse also used in xen?
>>>
>>> I am not aware of any use of Sparse in Xen, but it would technically be 
>>> possible.
>>>
>>> However, my point here is more that this change seems to be unrelated to 
>>> what the patch is meant to do (i.e. switching to byteswap). So if it is 
>>> unnecessary, then it should be dropped from this patch.
>>
>> I think making people pull little changes like this out into separate 
>> patches is asking too much. It’s a lot of extra effort on the part of the 
>> submitter for basically no value. We commonly do little clean-ups like this 
>> in patches, and just require a comment at the bottom, like this:
>>
>> 8<—
>>
>> While here:
>> - Drop ‘_force’ keyword, which is only needed when running the Sparse 
>> analysis tool
>>
>> —>8
>>
>> I do agree that minor changes like this need to be described, so that people 
>> 5 years from now have some hope of figuring out what’s going on.
> 
> I fully agree here. The effort involved by splitting a patch in several ones 
> (both for the
> contributor and the maintainers) means it should be prevented unless the 
> original patch
> could not be reviewed as is (patch to long or to complex to test for example 
> but there
> might be other valid cases).

This is fine for uncontroversial changes, but not in cases like this
one. Like Julien, I think we should rather strive towards completing
the Linux-inherited annotations like __force. Even without the actual
use of sparse they serve a documentation purpose.

Jan