Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras wrote: > On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o wrote: > > On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: > >> > That's exactly what I did. Addressing feedback constructively doesn't > >> > mean do exactly what you say without arguing. > >> > >> Your reply to my routine feedback was obtuse, argumentative and needlessly > >> confrontative - that's not 'constructive'. > > > > Felipe, remember when on the Git list Junio said he would stop trying > > to respond to any patches that had problems because you couldn't > > respond constructively to feedback, and you claimed that you had no > > problems working with other folks, including on the Linux Kernel > > mailing list? > > Ingo Molnar != kernel folks, and I don't see any hints of kernel folks > suggesting to drop patch #1 because of non-technical issues. > > If the patch is technically correct, conforms to standard practices, and > solves a problem; it gets applied. Isn't that how it works in Linux? I simply described to you what is standing Linux kernel maintenance policy. It is not new nor unusual that kernel patch changelog quality matters: defective changelogs are routinely pointed out during review and are required to be fixed before a patch can progress. Linux kernel maintainers frequently push back against deficient changelogs - in fact they are expected to push back against them. Your claim that a changelog defect that got pointed out during review is a 'non-technical', 'administrative' problem in Linux kernel development is simply wrong and your continued stubborn refusal to address such review feedback constructively is unnecessarily complicating the efficient processing of these patches. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o ty...@mit.edu wrote: On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. Your reply to my routine feedback was obtuse, argumentative and needlessly confrontative - that's not 'constructive'. Felipe, remember when on the Git list Junio said he would stop trying to respond to any patches that had problems because you couldn't respond constructively to feedback, and you claimed that you had no problems working with other folks, including on the Linux Kernel mailing list? Ingo Molnar != kernel folks, and I don't see any hints of kernel folks suggesting to drop patch #1 because of non-technical issues. If the patch is technically correct, conforms to standard practices, and solves a problem; it gets applied. Isn't that how it works in Linux? I simply described to you what is standing Linux kernel maintenance policy. It is not new nor unusual that kernel patch changelog quality matters: defective changelogs are routinely pointed out during review and are required to be fixed before a patch can progress. Linux kernel maintainers frequently push back against deficient changelogs - in fact they are expected to push back against them. Your claim that a changelog defect that got pointed out during review is a 'non-technical', 'administrative' problem in Linux kernel development is simply wrong and your continued stubborn refusal to address such review feedback constructively is unnecessarily complicating the efficient processing of these patches. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o wrote: > On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: >> > That's exactly what I did. Addressing feedback constructively doesn't >> > mean do exactly what you say without arguing. >> >> Your reply to my routine feedback was obtuse, argumentative and needlessly >> confrontative - that's not 'constructive'. > > Felipe, remember when on the Git list Junio said he would stop trying > to respond to any patches that had problems because you couldn't > respond constructively to feedback, and you claimed that you had no > problems working with other folks, including on the Linux Kernel > mailing list? Ingo Molnar != kernel folks, and I don't see any hints of kernel folks suggesting to drop patch #1 because of non-technical issues. If the patch is technically correct, conforms to standard practices, and solves a problem; it gets applied. Isn't that how it works in Linux? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: > > That's exactly what I did. Addressing feedback constructively doesn't > > mean do exactly what you say without arguing. > > Your reply to my routine feedback was obtuse, argumentative and needlessly > confrontative - that's not 'constructive'. Felipe, remember when on the Git list Junio said he would stop trying to respond to any patches that had problems because you couldn't respond constructively to feedback, and you claimed that you had no problems working with other folks, including on the Linux Kernel mailing list? You might want to take this feedback and consider whether the problem may be with *you*, and your user interface, and not with other folks like Ingo and Junio. You clearly want to contribute to both projects, and we can always use more skilled hackers. But part of being a good hacker is being able to play well with others. Best regards, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras wrote: > On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar wrote: > > > > * Felipe Contreras wrote: > > > >> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar wrote: > >> > > >> > * Felipe Contreras wrote: > >> > > >> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote: > >> >> > > >> >> > * Felipe Contreras wrote: > >> >> > > >> >> >> We want to calculate the blinks per second, and instead of making it > >> >> >> 5 > >> >> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks > >> >> >> per second. > >> >> > > >> >> > Please use the customary changelog style we use in the kernel: > >> >> > > >> >> > " Current code does (A), this has a problem when (B). > >> >> > We can improve this doing (C), because (D)." > >> >> > >> >> A is explained, B is empty, C is explained, D is because it makes sense. > > > > So one problem with your changelog is that you describe the change but > > don't explain a couple of things - for example why you changed '3600' to > > '1000'. > > Yes, I am aware of that, and it probably should, but that has nothing > to do with (A)(B)(C) or (D). So you knew that your changelog was defective, but you elected to interpret reviewer feedback narrowly and chose to be intentionally obtuse and difficult to waste even more reviewer time? That's rather destructive behavior, which you further demonstrate in the rest of your reply: > >> > NAKed-by: Ingo Molnar > >> > >> Suit yourself, stay with your buggy code then. > > > > I NAK-ed your patch because your patch has several technical problems. > > No, this is why you NAK-ed the patch: I very much know why I NAK-ed your patch. > > > A is explained, B is empty, C is explained, D is because it makes > > > sense. > > > > NAKed-by: Ingo Molnar > > That is not a technical problem, that's an allegedly administrative one. > I said I would fix the technical issues. You are mistaken: kernel changelogs are part of the change, a problem with a changelog is generally considered a technical problem as well. > > To lift the NAK you'll need to address my review feedback > > constructively. > > That's exactly what I did. Addressing feedback constructively doesn't > mean do exactly what you say without arguing. Your reply to my routine feedback was obtuse, argumentative and needlessly confrontative - that's not 'constructive'. > I will resend the patches separately since you are focusing on the > irrelevant patches and not paying attention to the one I made clear was > the important one, muddying it. [...] That first patch is faulty as well. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar wrote: > > * Felipe Contreras wrote: > >> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar wrote: >> > >> > * Felipe Contreras wrote: >> > >> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote: >> >> > >> >> > * Felipe Contreras wrote: >> >> > >> >> >> We want to calculate the blinks per second, and instead of making it 5 >> >> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks >> >> >> per second. >> >> > >> >> > Please use the customary changelog style we use in the kernel: >> >> > >> >> > " Current code does (A), this has a problem when (B). >> >> > We can improve this doing (C), because (D)." >> >> >> >> A is explained, B is empty, C is explained, D is because it makes sense. > > So one problem with your changelog is that you describe the change but > don't explain a couple of things - for example why you changed '3600' to > '1000'. Yes, I am aware of that, and it probably should, but that has nothing to do with (A)(B)(C) or (D). >> > NAKed-by: Ingo Molnar >> >> Suit yourself, stay with your buggy code then. > > I NAK-ed your patch because your patch has several technical problems. No, this is why you NAK-ed the patch: > > A is explained, B is empty, C is explained, D is because it makes sense. > > NAKed-by: Ingo Molnar That is not a technical problem, that's an allegedly administrative one. I said I would fix the technical issues. > To lift the NAK you'll need to address my review feedback constructively. That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. I will resend the patches separately since you are focusing on the irrelevant patches and not paying attention to the one I made clear was the important one, muddying it. I will address the technical and administrative issues in the 2nd and 3rd patches in the way I think is best. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras wrote: > On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar wrote: > > > > * Felipe Contreras wrote: > > > >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote: > >> > > >> > * Felipe Contreras wrote: > >> > > >> >> We want to calculate the blinks per second, and instead of making it 5 > >> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks > >> >> per second. > >> > > >> > Please use the customary changelog style we use in the kernel: > >> > > >> > " Current code does (A), this has a problem when (B). > >> > We can improve this doing (C), because (D)." > >> > >> A is explained, B is empty, C is explained, D is because it makes sense. So one problem with your changelog is that you describe the change but don't explain a couple of things - for example why you changed '3600' to '1000'. I know why you did it because I've read the diff and the related code, but such kind of information is best put into changelogs. This is standard review feedback. > > > > NAKed-by: Ingo Molnar > > Suit yourself, stay with your buggy code then. I NAK-ed your patch because your patch has several technical problems. To lift the NAK you'll need to address my review feedback constructively. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar wrote: > > * Felipe Contreras wrote: > >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote: >> > >> > * Felipe Contreras wrote: >> > >> >> We want to calculate the blinks per second, and instead of making it 5 >> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks >> >> per second. >> > >> > Please use the customary changelog style we use in the kernel: >> > >> > " Current code does (A), this has a problem when (B). >> > We can improve this doing (C), because (D)." >> >> A is explained, B is empty, C is explained, D is because it makes sense. > > NAKed-by: Ingo Molnar Suit yourself, stay with your buggy code then. Patch #1 is the important one anyway. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras wrote: > On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote: > > > > * Felipe Contreras wrote: > > > >> We want to calculate the blinks per second, and instead of making it 5 > >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks > >> per second. > > > > Please use the customary changelog style we use in the kernel: > > > > " Current code does (A), this has a problem when (B). > > We can improve this doing (C), because (D)." > > A is explained, B is empty, C is explained, D is because it makes sense. NAKed-by: Ingo Molnar Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar wrote:
>
> * Felipe Contreras wrote:
>
>> We want to calculate the blinks per second, and instead of making it 5
>> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
>> per second.
>
> Please use the customary changelog style we use in the kernel:
>
> " Current code does (A), this has a problem when (B).
> We can improve this doing (C), because (D)."
A is explained, B is empty, C is explained, D is because it makes sense.
>> Signed-off-by: Felipe Contreras
>> ---
>> kernel/panic.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index 46e37ff..5a0bdaa 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -25,7 +25,7 @@
>> #include
>>
>> #define PANIC_TIMER_STEP 100
>> -#define PANIC_BLINK_SPD 18
>> +#define PANIC_BLINK_SPD 4
>
> Please while at it also do another patch that renames it to a sane name,
> PANIC_BLINK_SPEED or so.
If I can do that, I would rather use PANIC_BLINK_PER_SECOND.
>> int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
>> static unsigned long tainted_mask;
>> @@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
>> touch_nmi_watchdog();
>> if (i >= i_next) {
>> i += panic_blink(state ^= 1);
>> - i_next = i + 3600 / PANIC_BLINK_SPD;
>> + i_next = i + 1000 / PANIC_BLINK_SPD;
>
> This changes a magic value to another magic value.
A magic value that is used all over the kernel, including
kernel/time.c and include/linux/delay.h. I'll change it to
MSEC_PER_SEC if that makes you happy.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras wrote:
> We want to calculate the blinks per second, and instead of making it 5
> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
> per second.
Please use the customary changelog style we use in the kernel:
" Current code does (A), this has a problem when (B).
We can improve this doing (C), because (D)."
> Signed-off-by: Felipe Contreras
> ---
> kernel/panic.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 46e37ff..5a0bdaa 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -25,7 +25,7 @@
> #include
>
> #define PANIC_TIMER_STEP 100
> -#define PANIC_BLINK_SPD 18
> +#define PANIC_BLINK_SPD 4
Please while at it also do another patch that renames it to a sane name,
PANIC_BLINK_SPEED or so.
>
> int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
> static unsigned long tainted_mask;
> @@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
> touch_nmi_watchdog();
> if (i >= i_next) {
> i += panic_blink(state ^= 1);
> - i_next = i + 3600 / PANIC_BLINK_SPD;
> + i_next = i + 1000 / PANIC_BLINK_SPD;
This changes a magic value to another magic value.
> }
> mdelay(PANIC_TIMER_STEP);
> }
> @@ -181,7 +181,7 @@ void panic(const char *fmt, ...)
> touch_softlockup_watchdog();
> if (i >= i_next) {
> i += panic_blink(state ^= 1);
> - i_next = i + 3600 / PANIC_BLINK_SPD;
> + i_next = i + 1000 / PANIC_BLINK_SPD;
Ditto.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras felipe.contre...@gmail.com wrote:
We want to calculate the blinks per second, and instead of making it 5
(1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
per second.
Please use the customary changelog style we use in the kernel:
Current code does (A), this has a problem when (B).
We can improve this doing (C), because (D).
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
kernel/panic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..5a0bdaa 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -25,7 +25,7 @@
#include linux/nmi.h
#define PANIC_TIMER_STEP 100
-#define PANIC_BLINK_SPD 18
+#define PANIC_BLINK_SPD 4
Please while at it also do another patch that renames it to a sane name,
PANIC_BLINK_SPEED or so.
int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
static unsigned long tainted_mask;
@@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
touch_nmi_watchdog();
if (i = i_next) {
i += panic_blink(state ^= 1);
- i_next = i + 3600 / PANIC_BLINK_SPD;
+ i_next = i + 1000 / PANIC_BLINK_SPD;
This changes a magic value to another magic value.
}
mdelay(PANIC_TIMER_STEP);
}
@@ -181,7 +181,7 @@ void panic(const char *fmt, ...)
touch_softlockup_watchdog();
if (i = i_next) {
i += panic_blink(state ^= 1);
- i_next = i + 3600 / PANIC_BLINK_SPD;
+ i_next = i + 1000 / PANIC_BLINK_SPD;
Ditto.
Thanks,
Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote:
* Felipe Contreras felipe.contre...@gmail.com wrote:
We want to calculate the blinks per second, and instead of making it 5
(1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
per second.
Please use the customary changelog style we use in the kernel:
Current code does (A), this has a problem when (B).
We can improve this doing (C), because (D).
A is explained, B is empty, C is explained, D is because it makes sense.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
kernel/panic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..5a0bdaa 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -25,7 +25,7 @@
#include linux/nmi.h
#define PANIC_TIMER_STEP 100
-#define PANIC_BLINK_SPD 18
+#define PANIC_BLINK_SPD 4
Please while at it also do another patch that renames it to a sane name,
PANIC_BLINK_SPEED or so.
If I can do that, I would rather use PANIC_BLINK_PER_SECOND.
int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
static unsigned long tainted_mask;
@@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
touch_nmi_watchdog();
if (i = i_next) {
i += panic_blink(state ^= 1);
- i_next = i + 3600 / PANIC_BLINK_SPD;
+ i_next = i + 1000 / PANIC_BLINK_SPD;
This changes a magic value to another magic value.
A magic value that is used all over the kernel, including
kernel/time.c and include/linux/delay.h. I'll change it to
MSEC_PER_SEC if that makes you happy.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: We want to calculate the blinks per second, and instead of making it 5 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks per second. Please use the customary changelog style we use in the kernel: Current code does (A), this has a problem when (B). We can improve this doing (C), because (D). A is explained, B is empty, C is explained, D is because it makes sense. NAKed-by: Ingo Molnar mi...@kernel.org Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: We want to calculate the blinks per second, and instead of making it 5 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks per second. Please use the customary changelog style we use in the kernel: Current code does (A), this has a problem when (B). We can improve this doing (C), because (D). A is explained, B is empty, C is explained, D is because it makes sense. NAKed-by: Ingo Molnar mi...@kernel.org Suit yourself, stay with your buggy code then. Patch #1 is the important one anyway. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: We want to calculate the blinks per second, and instead of making it 5 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks per second. Please use the customary changelog style we use in the kernel: Current code does (A), this has a problem when (B). We can improve this doing (C), because (D). A is explained, B is empty, C is explained, D is because it makes sense. So one problem with your changelog is that you describe the change but don't explain a couple of things - for example why you changed '3600' to '1000'. I know why you did it because I've read the diff and the related code, but such kind of information is best put into changelogs. This is standard review feedback. NAKed-by: Ingo Molnar mi...@kernel.org Suit yourself, stay with your buggy code then. I NAK-ed your patch because your patch has several technical problems. To lift the NAK you'll need to address my review feedback constructively. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: We want to calculate the blinks per second, and instead of making it 5 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks per second. Please use the customary changelog style we use in the kernel: Current code does (A), this has a problem when (B). We can improve this doing (C), because (D). A is explained, B is empty, C is explained, D is because it makes sense. So one problem with your changelog is that you describe the change but don't explain a couple of things - for example why you changed '3600' to '1000'. Yes, I am aware of that, and it probably should, but that has nothing to do with (A)(B)(C) or (D). NAKed-by: Ingo Molnar mi...@kernel.org Suit yourself, stay with your buggy code then. I NAK-ed your patch because your patch has several technical problems. No, this is why you NAK-ed the patch: A is explained, B is empty, C is explained, D is because it makes sense. NAKed-by: Ingo Molnar mi...@kernel.org That is not a technical problem, that's an allegedly administrative one. I said I would fix the technical issues. To lift the NAK you'll need to address my review feedback constructively. That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. I will resend the patches separately since you are focusing on the irrelevant patches and not paying attention to the one I made clear was the important one, muddying it. I will address the technical and administrative issues in the 2nd and 3rd patches in the way I think is best. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
* Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote: * Felipe Contreras felipe.contre...@gmail.com wrote: We want to calculate the blinks per second, and instead of making it 5 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks per second. Please use the customary changelog style we use in the kernel: Current code does (A), this has a problem when (B). We can improve this doing (C), because (D). A is explained, B is empty, C is explained, D is because it makes sense. So one problem with your changelog is that you describe the change but don't explain a couple of things - for example why you changed '3600' to '1000'. Yes, I am aware of that, and it probably should, but that has nothing to do with (A)(B)(C) or (D). So you knew that your changelog was defective, but you elected to interpret reviewer feedback narrowly and chose to be intentionally obtuse and difficult to waste even more reviewer time? That's rather destructive behavior, which you further demonstrate in the rest of your reply: NAKed-by: Ingo Molnar mi...@kernel.org Suit yourself, stay with your buggy code then. I NAK-ed your patch because your patch has several technical problems. No, this is why you NAK-ed the patch: I very much know why I NAK-ed your patch. A is explained, B is empty, C is explained, D is because it makes sense. NAKed-by: Ingo Molnar mi...@kernel.org That is not a technical problem, that's an allegedly administrative one. I said I would fix the technical issues. You are mistaken: kernel changelogs are part of the change, a problem with a changelog is generally considered a technical problem as well. To lift the NAK you'll need to address my review feedback constructively. That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. Your reply to my routine feedback was obtuse, argumentative and needlessly confrontative - that's not 'constructive'. I will resend the patches separately since you are focusing on the irrelevant patches and not paying attention to the one I made clear was the important one, muddying it. [...] That first patch is faulty as well. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. Your reply to my routine feedback was obtuse, argumentative and needlessly confrontative - that's not 'constructive'. Felipe, remember when on the Git list Junio said he would stop trying to respond to any patches that had problems because you couldn't respond constructively to feedback, and you claimed that you had no problems working with other folks, including on the Linux Kernel mailing list? You might want to take this feedback and consider whether the problem may be with *you*, and your user interface, and not with other folks like Ingo and Junio. You clearly want to contribute to both projects, and we can always use more skilled hackers. But part of being a good hacker is being able to play well with others. Best regards, - Ted -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] panic: improve panic_timeout calculation
On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o ty...@mit.edu wrote: On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote: That's exactly what I did. Addressing feedback constructively doesn't mean do exactly what you say without arguing. Your reply to my routine feedback was obtuse, argumentative and needlessly confrontative - that's not 'constructive'. Felipe, remember when on the Git list Junio said he would stop trying to respond to any patches that had problems because you couldn't respond constructively to feedback, and you claimed that you had no problems working with other folks, including on the Linux Kernel mailing list? Ingo Molnar != kernel folks, and I don't see any hints of kernel folks suggesting to drop patch #1 because of non-technical issues. If the patch is technically correct, conforms to standard practices, and solves a problem; it gets applied. Isn't that how it works in Linux? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
