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

[Jan Beulich <jbeulich@suse.com>]

On 11.05.2022 16:21, Bertrand Marquis wrote:
>> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@citrix.com> wrote:
>>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xen.org> 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