[PATCH 0/3] panic: improvements

[PATCH 0/3] panic: improvements

[Felipe Contreras]

Hi,

While debugging a hang after panic in my laptop (I cannot hard-reboot, or
unplug the battery), I noticed the panic code has some areas of improvment.

Only the first patch is really needed, the rest seem like good to have.

Felipe Contreras (3):
  panic: setup panic_timeout early
  panic: improve panic_timeout calculation
  panic: enable local IRQs for restart timeout too

 kernel/panic.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
1.8.4.2+fc1

[PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 kernel/panic.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..46e37ff 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *str)
+{
+	int timeout;
+
+	if (!get_option(&str, &timeout))
+		return -EINVAL;
+
+	panic_timeout = timeout;
+	return 0;
+}
+
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
 	if (!s)
-- 
1.8.4.2+fc1

[PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

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.

Signed-off-by: Felipe Contreras <felipe.contreras@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
 
 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;
 			}
 			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;
 		}
 		mdelay(PANIC_TIMER_STEP);
 	}
-- 
1.8.4.2+fc1

[PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 kernel/panic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 5a0bdaa..e32919f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -133,6 +133,8 @@ void panic(const char *fmt, ...)
 
 	bust_spinlocks(0);
 
+	local_irq_enable();
+
 	if (!panic_blink)
 		panic_blink = no_blink;
 
@@ -176,7 +178,6 @@ void panic(const char *fmt, ...)
 		disabled_wait(caller);
 	}
 #endif
-	local_irq_enable();
 	for (i = 0; ; i += PANIC_TIMER_STEP) {
 		touch_softlockup_watchdog();
 		if (i >= i_next) {
-- 
1.8.4.2+fc1

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@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.contreras@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

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@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.contreras@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

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.contreras@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 <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Felipe Contreras <felipe.contreras@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 <mingo@kernel.org>

Suit yourself, stay with your buggy code then.

Patch #1 is the important one anyway.

-- 
Felipe Contreras

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

The changelog is missing and the title is not self-explanatory.
Please fix.

Thanks,

	Ingo

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >
> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Felipe Contreras <felipe.contreras@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 <mingo@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

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> The changelog is missing and the title is not self-explanatory.

Either the local IRQs should be enabled for both the restart and halt
blinks, or it shouldn't be enabled for either. Why enable them for
halt, but not restart?

I think enabling them for restart too makes sense.

-- 
Felipe Contreras

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> >
>> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> > * Felipe Contreras <felipe.contreras@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 <mingo@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 <mingo@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

Re: [PATCH 1/3] panic: setup panic_timeout early

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Otherwise we might not reboot when the user needs it the most (early
> on).
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  kernel/panic.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..46e37ff 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
>  
>  #endif
>  
> -core_param(panic, panic_timeout, int, 0644);
>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>  
> +static int __init set_panic_timeout(char *str)
> +{
> +	int timeout;
> +
> +	if (!get_option(&str, &timeout))
> +		return -EINVAL;
> +
> +	panic_timeout = timeout;
> +	return 0;
> +}
> +
> +early_param("panic_timeout", set_panic_timeout);
> +

For simple integer parameters simple_strtol() is better and (a tiny bit) 
faster.

Thanks,

	Ingo

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >
> >> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >> >
> >> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >> >
> >> >> > * Felipe Contreras <felipe.contreras@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 <mingo@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 <mingo@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

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > The changelog is missing and the title is not self-explanatory.
> 
> Either the local IRQs should be enabled for both the restart and halt
> blinks, or it shouldn't be enabled for either. Why enable them for
> halt, but not restart?
> 
> I think enabling them for restart too makes sense.

Such arguments belong into the changelog, with a description of what was 
done before and what is done after - please use the customary (verbose) 
changelog style we use in the kernel.

Thanks,

	Ingo

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> Otherwise we might not reboot when the user needs it the most (early
>> on).
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  kernel/panic.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..46e37ff 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>
>>  #endif
>>
>> -core_param(panic, panic_timeout, int, 0644);
>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>> +static int __init set_panic_timeout(char *str)
>> +{
>> +     int timeout;
>> +
>> +     if (!get_option(&str, &timeout))
>> +             return -EINVAL;
>> +
>> +     panic_timeout = timeout;
>> +     return 0;
>> +}
>> +
>> +early_param("panic_timeout", set_panic_timeout);
>> +
>
> For simple integer parameters simple_strtol() is better and (a tiny bit)
> faster.

simple_strtol() is deprecated in favor of kstrtol().

-- 
Felipe Contreras

Re: [PATCH 1/3] panic: setup panic_timeout early

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >
> >> Otherwise we might not reboot when the user needs it the most (early
> >> on).
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  kernel/panic.c | 14 +++++++++++++-
> >>  1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/panic.c b/kernel/panic.c
> >> index b6c482c..46e37ff 100644
> >> --- a/kernel/panic.c
> >> +++ b/kernel/panic.c
> >> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
> >>
> >>  #endif
> >>
> >> -core_param(panic, panic_timeout, int, 0644);
> >>  core_param(pause_on_oops, pause_on_oops, int, 0644);
> >>
> >> +static int __init set_panic_timeout(char *str)
> >> +{
> >> +     int timeout;
> >> +
> >> +     if (!get_option(&str, &timeout))
> >> +             return -EINVAL;
> >> +
> >> +     panic_timeout = timeout;
> >> +     return 0;
> >> +}
> >> +
> >> +early_param("panic_timeout", set_panic_timeout);
> >> +
> >
> > For simple integer parameters simple_strtol() is better and (a tiny bit)
> > faster.
> 
> simple_strtol() is deprecated in favor of kstrtol().

Yes, true - my main point is that get_option() isn't the right choice 
here, you need to use a single-integer parsing function.

Thanks,

	Ingo

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:54 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> >
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >
>> > The changelog is missing and the title is not self-explanatory.
>>
>> Either the local IRQs should be enabled for both the restart and halt
>> blinks, or it shouldn't be enabled for either. Why enable them for
>> halt, but not restart?
>>
>> I think enabling them for restart too makes sense.
>
> Such arguments belong into the changelog, with a description of what was
> done before and what is done after - please use the customary (verbose)
> changelog style we use in the kernel.

I'm not going to re-roll with such description only so you can NAK it
again. If you agree that such an explanation is OK, say so.

-- 
Felipe Contreras

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 8:22 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> >
>> >> Otherwise we might not reboot when the user needs it the most (early
>> >> on).
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> ---
>> >>  kernel/panic.c | 14 +++++++++++++-
>> >>  1 file changed, 13 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/kernel/panic.c b/kernel/panic.c
>> >> index b6c482c..46e37ff 100644
>> >> --- a/kernel/panic.c
>> >> +++ b/kernel/panic.c
>> >> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
>> >>
>> >>  #endif
>> >>
>> >> -core_param(panic, panic_timeout, int, 0644);
>> >>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>> >>
>> >> +static int __init set_panic_timeout(char *str)
>> >> +{
>> >> +     int timeout;
>> >> +
>> >> +     if (!get_option(&str, &timeout))
>> >> +             return -EINVAL;
>> >> +
>> >> +     panic_timeout = timeout;
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +early_param("panic_timeout", set_panic_timeout);
>> >> +
>> >
>> > For simple integer parameters simple_strtol() is better and (a tiny bit)
>> > faster.
>>
>> simple_strtol() is deprecated in favor of kstrtol().
>
> Yes, true - my main point is that get_option() isn't the right choice
> here, you need to use a single-integer parsing function.

Then somebody made the wrong choice for 'loglevel'.

-- 
Felipe Contreras

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Theodore Ts'o]

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

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o <tytso@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

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Ingo Molnar]

* Felipe Contreras <felipe.contreras@gmail.com> wrote:

> On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o <tytso@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