[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.c...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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(-)

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.c...@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) {

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.c...@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)

[Ingo Molnar]

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

[Felipe Contreras]

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

[Felipe Contreras]

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

[Ingo Molnar]

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

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

[Ingo Molnar]

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

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

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

Thanks,

Ingo

[Ingo Molnar]

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

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

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mi...@kernel.org> wrote:
>
> * Felipe Contreras <felipe.c...@gmail.com> wrote:
>
>> Signed-off-by: Felipe Contreras <felipe.c...@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

[Ingo Molnar]

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

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

Thanks,

Ingo

[Felipe Contreras]

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

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

This is exactly the same as in the previous try, but now alone since it was
mudded by largely irrelevant patches in the series.

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

[Felipe Contreras]

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

[Ingo Molnar]

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

> On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mi...@kernel.org> wrote:
> >
> > * Felipe Contreras <felipe.c...@gmail.com> wrote:
> >
> >> Signed-off-by: Felipe Contreras <felipe.c...@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

[Ingo Molnar]

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

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

[Felipe Contreras]

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

--
Felipe Contreras

[Ingo Molnar]

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

This still has the problem I pointed out for your v1 patch: for simple

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

Thanks,

Ingo

[Ingo Molnar]

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

[Felipe Contreras]

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

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

This time using kstrtol() instead of get_option().

Interdiff:

diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -470,12 +470,14 @@ EXPORT_SYMBOL(__stack_chk_fail);

core_param(pause_on_oops, pause_on_oops, int, 0644);

-static int __init set_panic_timeout(char *str)
+static int __init set_panic_timeout(char *val)
{
- int timeout;
+ long timeout;
+ int ret;

- if (!get_option(&str, &timeout))
- return -EINVAL;
+ ret = kstrtol(val, 0, &timeout);
+ if (ret < 0)
+ return ret;

panic_timeout = timeout;
return 0;

kernel/panic.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,23 @@ 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 *val)
+{
+ long timeout;
+ int ret;
+
+ ret = kstrtol(val, 0, &timeout);
+ if (ret < 0)
+ return ret;

+
+ 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

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:54 AM, Ingo Molnar <mi...@kernel.org> wrote:
>
> * Felipe Contreras <felipe.c...@gmail.com> wrote:
>
>> On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar <mi...@kernel.org> wrote:
>> >
>> > * Felipe Contreras <felipe.c...@gmail.com> wrote:
>> >
>> >> Signed-off-by: Felipe Contreras <felipe.c...@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

[Felipe Contreras]

Then somebody made the wrong choice for 'loglevel'.

--
Felipe Contreras

[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

[Felipe Contreras]

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

[Ingo Molnar]

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

> Otherwise we might not reboot when the user needs it the most (early
> on).
>
> Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
> ---
>

[...]

>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..d865263 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -468,9 +468,23 @@ 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 *val)
> +{
> + long timeout;
> + int ret;
> +
> + ret = kstrtol(val, 0, &timeout);
> + if (ret < 0)
> + return ret;
> +
> + panic_timeout = timeout;
> + return 0;
> +}

I think the type of the 'timeout' local variable should match the type of
'panic_timeout' (which is 'int', not 'long').

Thanks,

Ingo

[Ingo Molnar]

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

[Felipe Contreras]

So you would rather have this?

kstrtol(val, 0, (long *)&timeout);

Couldn't that potentially write the value beyond the memory allocated
to 'timeout'?

--
Felipe Contreras

[Levente Kurusa]

No, 'panic_timeout' is a variable of type 'int'.
Your 'long timeout;' line is wrong and should say 'int timeout;'


--
Regards,
Levente Kurusa

[Felipe Contreras]

Hi,

Sending again since the previous one was rejected by the server.

On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
<felipe.c...@gmail.com> wrote:

> Oh really? Something like this?
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);

>
> static int __init set_panic_timeout(char *val)
> {

> - long timeout;
> + int timeout;
> int ret;

>
> ret = kstrtol(val, 0, &timeout);
>

> And then what happens?
>
> kernel/panic.c: In function ‘set_panic_timeout’:
> kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from incompatible pointer type [enabled by default]

> ret = kstrtol(val, 0, &timeout);

> ^
> In file included from include/linux/debug_locks.h:4:0,
> from kernel/panic.c:11:
> include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is of type ‘int *’
> static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>
> To fix that warning you need this:
> ^
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);

>
> static int __init set_panic_timeout(char *val)
> {

> - long timeout;
> + int timeout;
> int ret;
>
> - ret = kstrtol(val, 0, &timeout);
> + ret = kstrtol(val, 0, (long *)&timeout);
> if (ret < 0)
> return ret;
>
>
> Which is the only logical conclusion I arrived to. What else do you suggest to
> fix the problem that kstrtol() expects a long? And since this fix is not what
> we want because we would be writing to the wrong memory, we don't want 'timeout'
> to be int.
>
> Therefore 'timeout' should be a long. How is that not clear?
>
> You can even see that it's already done this way for parameters:
>
> STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>
> #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn) \
> int param_set_##name(const char *val, const struct kernel_param *kp) \
> { \
> tmptype l; \
> int ret; \
> \
> ret = strtolfn(val, 0, &l); \
> if (ret < 0 || ((type)l != l)) \
> return ret < 0 ? ret : -EINVAL; \
> *((type *)kp->arg) = l; \
> return 0; \
> } \
>
> So yes, we obviously want the temporary variable 'timeout' to be a long, even
> though the final destination is an int.

--
Felipe Contreras

[Ingo Molnar]

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

> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mi...@kernel.org> wrote:
> >
> >> +static int __init set_panic_timeout(char *val)
> >> +{
> >> + long timeout;
> >> + int ret;
> >> +
> >> + ret = kstrtol(val, 0, &timeout);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + panic_timeout = timeout;
> >> + return 0;
> >> +}
> >
> > I think the type of the 'timeout' local variable should match the type of
> > 'panic_timeout' (which is 'int', not 'long').
>
> So you would rather have this?
>
> kstrtol(val, 0, (long *)&timeout);
>
> Couldn't that potentially write the value beyond the memory
> allocated to 'timeout'?

No, casting that to 'long *' is actively wrong, I'd use a
string -> integer conversion method that deals with ints,
not longs, such as kstrtoint().

Thanks,

Ingo

[Levente Kurusa]

Hi,

No, you don't want timeout to be a long, and instead of kstrtol, you should use
kstrtoint(char *s, int base, int *res)
which is defined in lib/ktstrtox.c

Also, a bit of advice: Calm down. If you continue to act childish nobody will take
you seriously. Before acting like you are the king here, please at least take the time
to research other options.

--
Regards,
Levente Kurusa

[Felipe Contreras]

On Fri, Nov 15, 2013 at 7:12 AM, Levente Kurusa <le...@linux.com> wrote:

> No, you don't want timeout to be a long, and instead of kstrtol, you should use
> kstrtoint(char *s, int base, int *res)
> which is defined in lib/ktstrtox.c

And if you look into kstrtoint() you would see that it's doing
*exactly* the same I'm doing; using a temporary bigger integer, same
as STANDARD_PARAM_DEF.

> Also, a bit of advice: Calm down. If you continue to act childish nobody will take
> you seriously. Before acting like you are the king here, please at least take the time
> to research other options.

I am irrelevant, the code is what matters, and panic_timeout should be
set early on, regardless of who is sending the patch.

Moreover, if you are going to claim that I didn't research other
options, then the guy that did this:

STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);

Didn't research other options either, because it should be:

STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Presumably it doesn't matter much, but it seems it does matter when
*I* am the one sending the patch.

And what exactly is childish about asking this question?

> So you would rather have this?
>
> kstrtol(val, 0, (long *)&timeout);

The answer should be: no, use kstrtoint(), but that wasn't your
answer, was it? Your answer basically repeated what Ingo said, so I
had to go step by step to the conclusion of 'kstrtol(val, 0, (long
*)&timeout);'. And then I asked you a question: "What else do you
suggest to fix the problem that kstrtol() expects a long?" *Now* your
answer is useful. Why is that childish?

Now, in this process a few things are clear to me:

1) loglevel should use kstrtoint() as well, not get_option
2) get_option() should use kstrtoint(); it's using simple_strtol() and
it's not even checking for overflows
3) It should be STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Don't you think it's little bit of a stretch to say that *I* didn't do
my research, when in fact I did, and none of the parameters code is
using kstrtoint() as it should, even Ingo himself told me I should use
simple_strtol(), which is actually deprecated.

My patch is simply doing what similar code is already doing. Why don't
you take a step back and accept the possibility that a) I did do my
research, and b) I wasn't being childish in asking how to make kstrtol
work (given that Ingo suggested to use simple_strtol()). I think you
should take your own advice and calm down, maybe even take back what
you said. Maybe I was combative, and maybe I made the wrong
assumptions, but at least I didn't throw insults, nor called anybody
names like "childish".

Anyway, I will update the patch to sue kstrtoint(), and I would gladly
fix the existing code for issues 1) 2) and 3) *if* you or anybody
agrees they should be using kstrtoint() as well, I'm not in the mood
of going through Ingo's review process again for something so trivial.

Cheers.

--
Felipe Contreras

[Felipe Contreras]

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

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

Since v2:

There's no need for a temporary variable because kstrtoint() is already taking
care of that (if there's an error panic_timeout is not updated).

diff --git a/kernel/panic.c b/kernel/panic.c
index d865263..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -472,15 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);

static int __init set_panic_timeout(char *val)
{
- long timeout;

- int ret;
-

- ret = kstrtol(val, 0, &timeout);

- if (ret < 0)
- return ret;
-
- panic_timeout = timeout;
- return 0;
+ return kstrtoint(val, 0, &panic_timeout);
}

early_param("panic_timeout", set_panic_timeout);

kernel/panic.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,15 @@ 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 *val)
+{

+ return kstrtoint(val, 0, &panic_timeout);

+}
+
+early_param("panic_timeout", set_panic_timeout);
+

static int __init oops_setup(char *s)
{
if (!s)
--
1.8.4.2+fc1

[Linus Torvalds]

On Fri, Nov 15, 2013 at 11:33 AM, Felipe Contreras
<felipe.c...@gmail.com> wrote:
>
> I am irrelevant, the code is what matter

Not so. You are relevant in the sense that you need to communicate why
people should take patches from you.

I know people who just put you in their kill-files because they can't
bother with the endless argumentative flames you generate. At which
point all the code you produce is kind of irrelevant, because people
won't see it.

Seriously, felipe. You pissed off Junio, who is *hard* to annoy. Just
learn from your mistakes, instead of repeating them over and over.

It's not just code. You need to also explain *why* people should apply
it, and stop the f*cking idiotic arguing every time somebody comments
about your patches.

Because it literally seems to be *EVERY* single time.

Linus

[Linus Torvalds]

On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
<felipe.c...@gmail.com> wrote:
>
> I haven't seen a single complaint about this commit message, so I
> don't see what is your point.

My point is that I have sixteen pointless messages in my mbox, half of
which are due to just your argumentative nature.

In other words, my point is that you are wasting my - and other
peoples - time just because you *always* have to turn every single
thing into a big argument when somebody comments on a patch.

[Felipe Contreras]

On Fri, Nov 15, 2013 at 1:55 PM, Linus Torvalds
<torv...@linux-foundation.org> wrote:

> It's not just code. You need to also explain *why* people should apply
> it, and stop the f*cking idiotic arguing every time somebody comments
> about your patches.

Doesn't this explain it?

------------------------------------------------------------------------------
panic: setup panic_timeout early

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

------------------------------------------------------------------------------

I haven't seen a single complaint about this commit message, so I
don't see what is your point.

You didn't address what I said at all. If the code is technically
correct *and* it is clear there's a reason why the patch should be
applied, who sent the patch should be irrelevant, because even if that
person is problematic, and there's something lacking in the patch,
somebody else can take it from there and fix the remaining issues,
because if there's a reason the patch should be applied, it should be
applied.

In this particular case it is clear we want panic_timeout to work
early on, therefore a patch should be applied to achieve that, and you
might as well simply apply the patch I sent, even though *I* sent it,
because it's technically correct, and the need is explained. Why
wouldn't you?

--
Felipe Contreras

[Felipe Contreras]

On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
<torv...@linux-foundation.org> wrote:
> On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> <felipe.c...@gmail.com> wrote:
>>
>> I haven't seen a single complaint about this commit message, so I
>> don't see what is your point.
>
> My point is that I have sixteen pointless messages in my mbox, half of
> which are due to just your argumentative nature.

This is clearly not the point you were making, but I won't argue why.

You don't want to argue? Then don't argue. Apply the patch. Unless you
see something wrong with the patch. Do you see something wrong with
the patch?

--
Felipe Contreras

[Ingo Molnar]

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

> On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
> <torv...@linux-foundation.org> wrote:
> > On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> > <felipe.c...@gmail.com> wrote:
> >>
> >> I haven't seen a single complaint about this commit message, so I
> >> don't see what is your point.
> >
> > My point is that I have sixteen pointless messages in my mbox,
> > half of which are due to just your argumentative nature.
>
> This is clearly not the point you were making, but I won't argue
> why.

That was exactly the point Linus was making.

Anyway, the fact that you are argumentative even with Linus gives me
little hope that you will improve your communication patterns with
_me_, so I'm personally done arguing with you.

> You don't want to argue? Then don't argue. Apply the patch. [...]

*Plonk*.

Ingo

[Felipe Contreras]

On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mi...@kernel.org> wrote:
> * Felipe Contreras <felipe.c...@gmail.com> wrote:

> Anyway, the fact that you are argumentative even with Linus gives me
> little hope that you will improve your communication patterns with
> _me_, so I'm personally done arguing with you.

How am I being argumentative? I avoided all arguments.

>> You don't want to argue? Then don't argue. Apply the patch. [...]
>
> *Plonk*.

This is exactly the opposite of what is conducive to less argumentation.

If you are not going to comment on the patch, then don't say anything.

The patch is good, and the fact that both you and Linus are avoiding
any comments in the patch itself doesn't speak well for your
intentions to avoid argumentation.

So I ask you again. Do you see something wrong with the patch?

--
Felipe Contreras

[Levente Kurusa]

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:
> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mi...@kernel.org> wrote:
>> * Felipe Contreras <felipe.c...@gmail.com> wrote:
>
>> Anyway, the fact that you are argumentative even with Linus gives me
>> little hope that you will improve your communication patterns with
>> _me_, so I'm personally done arguing with you.
>
> How am I being argumentative? I avoided all arguments.
>
>>> You don't want to argue? Then don't argue. Apply the patch. [...]
>>
>> *Plonk*.
>
> This is exactly the opposite of what is conducive to less argumentation.
>
> If you are not going to comment on the patch, then don't say anything.
>
> The patch is good, and the fact that both you and Linus are avoiding
> any comments in the patch itself doesn't speak well for your
> intentions to avoid argumentation.
>
> So I ask you again. Do you see something wrong with the patch?
>

Yes, you.

--
Regards,
Levente Kurusa

[Levente Kurusa]

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:

> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mi...@kernel.org> wrote:
>> * Felipe Contreras <felipe.c...@gmail.com> wrote:
>
>> Anyway, the fact that you are argumentative even with Linus gives me
>> little hope that you will improve your communication patterns with
>> _me_, so I'm personally done arguing with you.
>
> How am I being argumentative? I avoided all arguments.
>
>>> You don't want to argue? Then don't argue. Apply the patch. [...]
>>
>> *Plonk*.
>
> This is exactly the opposite of what is conducive to less argumentation.
>
> If you are not going to comment on the patch, then don't say anything.
>
> The patch is good, and the fact that both you and Linus are avoiding
> any comments in the patch itself doesn't speak well for your
> intentions to avoid argumentation.
>
> So I ask you again. Do you see something wrong with the patch?
>

Out-of-joke, the patch is now in an 'appliable' state.

You can add my:
Reviewed-by: Levente Kurusa <le...@linux.com>

--
Regards,
Levente Kurusa