* [PATCH v2] bitops: Fix incorrect value in comment
@ 2021-11-30 18:12 Ayan Kumar Halder
2021-12-01 7:21 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ayan Kumar Halder @ 2021-11-30 18:12 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, jbeulich, Ayan Kumar Halder, andre.przywara
GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
in bitops.h.
Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog :-
v2 :- 1. Replaced the word "vector" with "value" in comment.
2. Changed 0x07fe00000 to 0x7fe00000.
3. Updated the commit message to make it meaningful.
(All suggested by Jan Beulich)
xen/include/xen/bitops.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index a64595f68e..dad4b5aa1e 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -5,7 +5,7 @@
/*
* Create a contiguous bitmask starting at bit position @l and ending at
* position @h. For example
- * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
+ * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
*/
#define GENMASK(h, l) \
(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-11-30 18:12 [PATCH v2] bitops: Fix incorrect value in comment Ayan Kumar Halder
@ 2021-12-01 7:21 ` Jan Beulich
2021-12-01 8:40 ` Bertrand Marquis
2021-12-01 9:33 ` Julien Grall
2 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2021-12-01 7:21 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, Ayan Kumar Halder, andre.przywara, xen-devel
On 30.11.2021 19:12, Ayan Kumar Halder wrote:
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-11-30 18:12 [PATCH v2] bitops: Fix incorrect value in comment Ayan Kumar Halder
2021-12-01 7:21 ` Jan Beulich
@ 2021-12-01 8:40 ` Bertrand Marquis
2021-12-01 9:33 ` Julien Grall
2 siblings, 0 replies; 8+ messages in thread
From: Bertrand Marquis @ 2021-12-01 8:40 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: Xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk,
jbeulich, Ayan Kumar Halder, Andre Przywara
Hi Ayan,
> On 30 Nov 2021, at 18:12, Ayan Kumar Halder <ayan.kumar.halder@xilinx.com> wrote:
>
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> Changelog :-
> v2 :- 1. Replaced the word "vector" with "value" in comment.
> 2. Changed 0x07fe00000 to 0x7fe00000.
> 3. Updated the commit message to make it meaningful.
> (All suggested by Jan Beulich)
>
> xen/include/xen/bitops.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index a64595f68e..dad4b5aa1e 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -5,7 +5,7 @@
> /*
> * Create a contiguous bitmask starting at bit position @l and ending at
> * position @h. For example
> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
> */
> #define GENMASK(h, l) \
> (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-11-30 18:12 [PATCH v2] bitops: Fix incorrect value in comment Ayan Kumar Halder
2021-12-01 7:21 ` Jan Beulich
2021-12-01 8:40 ` Bertrand Marquis
@ 2021-12-01 9:33 ` Julien Grall
2021-12-01 9:38 ` Jan Beulich
2 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2021-12-01 9:33 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
jbeulich, Ayan Kumar Halder, andre.przywara
Hi,
On 30/11/2021 18:12, Ayan Kumar Halder wrote:
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.
I am afraid this commit message is incomplete. You say you just
corrected the bitmask returned but...
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
> Changelog :-
> v2 :- 1. Replaced the word "vector" with "value" in comment.
> 2. Changed 0x07fe00000 to 0x7fe00000.
> 3. Updated the commit message to make it meaningful.
> (All suggested by Jan Beulich)
>
> xen/include/xen/bitops.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index a64595f68e..dad4b5aa1e 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -5,7 +5,7 @@
> /*
> * Create a contiguous bitmask starting at bit position @l and ending at
> * position @h. For example
> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
... there are two extra changes here:
1) The bitmask is now described with 8-characters (rather than 9)
2) 'vector' is replaced with 'value'
The former makes sense to me, but it is not clear to me why the latter
should be changed.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-12-01 9:33 ` Julien Grall
@ 2021-12-01 9:38 ` Jan Beulich
2021-12-01 9:56 ` Julien Grall
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2021-12-01 9:38 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
Ayan Kumar Halder, andre.przywara, Ayan Kumar Halder, xen-devel
On 01.12.2021 10:33, Julien Grall wrote:
> On 30/11/2021 18:12, Ayan Kumar Halder wrote:
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -5,7 +5,7 @@
>> /*
>> * Create a contiguous bitmask starting at bit position @l and ending at
>> * position @h. For example
>> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
>> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
>
> ... there are two extra changes here:
> 1) The bitmask is now described with 8-characters (rather than 9)
> 2) 'vector' is replaced with 'value'
>
> The former makes sense to me, but it is not clear to me why the latter
> should be changed.
Would you mind explaining to me in which way you see "vector" accurately
describe the entity talked about?
I also think the commit message is quite fine as is.
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-12-01 9:38 ` Jan Beulich
@ 2021-12-01 9:56 ` Julien Grall
2021-12-01 21:38 ` Andrew Cooper
0 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2021-12-01 9:56 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
Ayan Kumar Halder, andre.przywara, Ayan Kumar Halder, xen-devel
Hi,
On 01/12/2021 09:38, Jan Beulich wrote:
> On 01.12.2021 10:33, Julien Grall wrote:
>> On 30/11/2021 18:12, Ayan Kumar Halder wrote:
>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -5,7 +5,7 @@
>>> /*
>>> * Create a contiguous bitmask starting at bit position @l and ending at
>>> * position @h. For example
>>> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
>>> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
>>
>> ... there are two extra changes here:
>> 1) The bitmask is now described with 8-characters (rather than 9)
>> 2) 'vector' is replaced with 'value'
>>
>> The former makes sense to me, but it is not clear to me why the latter
>> should be changed.
>
> Would you mind explaining to me in which way you see "vector" accurately
> describe the entity talked about?
This can be seen as a vector of bit. I can see why people may think
otherwise. However... if you think it doesn't describe it accurately,
then I think this ought to be changed in Linux first (where the code and
comment comes from).
>
> I also think the commit message is quite fine as is.
IMHO, this is similar to when one do coding style change in a patch.
They are unrelated but would be acceptable so long they are explained in
the commit message.
What I request is something like:
"GENMASK(30, 21) should be 0x7fe00000 and only use 8-characters (it is a
32-bit comment). Fixed this in the comment.
Take the opportunity to replace 'vector' with 'value' because..."
This is simple enough and clarify what is the intent of the patch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-12-01 9:56 ` Julien Grall
@ 2021-12-01 21:38 ` Andrew Cooper
2021-12-02 9:05 ` Julien Grall
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2021-12-01 21:38 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
Ayan Kumar Halder, andre.przywara, Ayan Kumar Halder, xen-devel
On 01/12/2021 09:56, Julien Grall wrote:
> Hi,
>
> On 01/12/2021 09:38, Jan Beulich wrote:
>> On 01.12.2021 10:33, Julien Grall wrote:
>>> On 30/11/2021 18:12, Ayan Kumar Halder wrote:
>>>> --- a/xen/include/xen/bitops.h
>>>> +++ b/xen/include/xen/bitops.h
>>>> @@ -5,7 +5,7 @@
>>>> /*
>>>> * Create a contiguous bitmask starting at bit position @l and
>>>> ending at
>>>> * position @h. For example
>>>> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
>>>> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
>>>
>>> ... there are two extra changes here:
>>> 1) The bitmask is now described with 8-characters (rather than 9)
>>> 2) 'vector' is replaced with 'value'
>>>
>>> The former makes sense to me, but it is not clear to me why the latter
>>> should be changed.
>>
>> Would you mind explaining to me in which way you see "vector" accurately
>> describe the entity talked about?
>
> This can be seen as a vector of bit. I can see why people may think
> otherwise. However... if you think it doesn't describe it accurately,
> then I think this ought to be changed in Linux first (where the code
> and comment comes from).
>
>>
>> I also think the commit message is quite fine as is.
> IMHO, this is similar to when one do coding style change in a patch.
> They are unrelated but would be acceptable so long they are explained
> in the commit message.
>
> What I request is something like:
>
> "GENMASK(30, 21) should be 0x7fe00000 and only use 8-characters (it is
> a 32-bit comment). Fixed this in the comment.
>
> Take the opportunity to replace 'vector' with 'value' because..."
>
> This is simple enough and clarify what is the intent of the patch.
This is an unreasonable quantity of bikeshedding. It's just a comment,
and a commit message of "fix the comment" is perfectly fine.
Furthermore, the intent of the text is clear.
However, "32bit $WHATEVER" is also wrong because GENMASK() yields a
unsigned long constant. Importantly, not 32 bits in an ARM64 build.
This trivial patch has lingered far too long. I have committed it,
along with an adjustment. Further bikeshedding will be redirected to
/dev/null.
~Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] bitops: Fix incorrect value in comment
2021-12-01 21:38 ` Andrew Cooper
@ 2021-12-02 9:05 ` Julien Grall
0 siblings, 0 replies; 8+ messages in thread
From: Julien Grall @ 2021-12-02 9:05 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
Ayan Kumar Halder, andre.przywara, Ayan Kumar Halder, xen-devel,
Ian Jackson
Hi Andrew,
On 01/12/2021 21:38, Andrew Cooper wrote:
> On 01/12/2021 09:56, Julien Grall wrote:
>> Hi,
>>
>> On 01/12/2021 09:38, Jan Beulich wrote:
>>> On 01.12.2021 10:33, Julien Grall wrote:
>>>> On 30/11/2021 18:12, Ayan Kumar Halder wrote:
>>>>> --- a/xen/include/xen/bitops.h
>>>>> +++ b/xen/include/xen/bitops.h
>>>>> @@ -5,7 +5,7 @@
>>>>> /*
>>>>> * Create a contiguous bitmask starting at bit position @l and
>>>>> ending at
>>>>> * position @h. For example
>>>>> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
>>>>> + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
>>>>
>>>> ... there are two extra changes here:
>>>> 1) The bitmask is now described with 8-characters (rather than 9)
>>>> 2) 'vector' is replaced with 'value'
>>>>
>>>> The former makes sense to me, but it is not clear to me why the latter
>>>> should be changed.
>>>
>>> Would you mind explaining to me in which way you see "vector" accurately
>>> describe the entity talked about?
>>
>> This can be seen as a vector of bit. I can see why people may think
>> otherwise. However... if you think it doesn't describe it accurately,
>> then I think this ought to be changed in Linux first (where the code
>> and comment comes from).
>>
>>>
>>> I also think the commit message is quite fine as is.
>> IMHO, this is similar to when one do coding style change in a patch.
>> They are unrelated but would be acceptable so long they are explained
>> in the commit message.
>>
>> What I request is something like:
>>
>> "GENMASK(30, 21) should be 0x7fe00000 and only use 8-characters (it is
>> a 32-bit comment). Fixed this in the comment.
>>
>> Take the opportunity to replace 'vector' with 'value' because..."
>>
>> This is simple enough and clarify what is the intent of the patch.
>
> This is an unreasonable quantity of bikeshedding.
I didn't realize that two emails were considered bikeshedding.
I actually provided and worked towards a solution rather than
unhelpfully saying just no.
> It's just a comment,
> and a commit message of "fix the comment" is perfectly fine.
> Furthermore, the intent of the text is clear.
>
> However, "32bit $WHATEVER" is also wrong because GENMASK() yields a
> unsigned long constant. Importantly, not 32 bits in an ARM64 build.
>
>
> This trivial patch has lingered far too long. I have committed it,
> along with an adjustment. Further bikeshedding will be redirected to
> /dev/null.
It is an interesting approach. I could have committed this patch
after updating the commit message like you did ;).
But, so far, I have refrained from blatantly ignoring comments and going
ahead with committing ([1] is an example where this could be used)
because I think we should try to have a consensus first.
Anyway, I am happy to accept that two maintainers have an opposite view
from me and go with the tide. That said, there are probably better a way
to express your view...
Cheers,
[1]
https://lore.kernel.org/xen-devel/062bcbd3-420e-e1c0-3aa0-0dfb229e6ae9@suse.com/
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-02 9:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-30 18:12 [PATCH v2] bitops: Fix incorrect value in comment Ayan Kumar Halder
2021-12-01 7:21 ` Jan Beulich
2021-12-01 8:40 ` Bertrand Marquis
2021-12-01 9:33 ` Julien Grall
2021-12-01 9:38 ` Jan Beulich
2021-12-01 9:56 ` Julien Grall
2021-12-01 21:38 ` Andrew Cooper
2021-12-02 9:05 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.