linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] panic: setup panic_timeout early
@ 2013-11-11 14:40 Felipe Contreras
  2013-11-13  0:03 ` Ingo Molnar
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-11-11 14:40 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Linus Torvalds, Andrew Morton, 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>
---

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-11 14:40 [PATCH v2] panic: setup panic_timeout early Felipe Contreras
@ 2013-11-13  0:03 ` Ingo Molnar
  2013-11-14 11:16   ` Felipe Contreras
  0 siblings, 1 reply; 16+ messages in thread
From: Ingo Molnar @ 2013-11-13  0:03 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: linux-kernel, Ingo Molnar, Linus Torvalds, Andrew Morton


* 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>
> ---
> 
[...]
>
> 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-13  0:03 ` Ingo Molnar
@ 2013-11-14 11:16   ` Felipe Contreras
  2013-11-14 17:42     ` Levente Kurusa
  2013-11-15 11:27     ` Ingo Molnar
  0 siblings, 2 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-11-14 11:16 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linux Kernel Mailing List, Ingo Molnar, Linus Torvalds, Andrew Morton

On Tue, Nov 12, 2013 at 6:03 PM, 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>
>> ---
>>
> [...]
>>
>> 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').

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-14 11:16   ` Felipe Contreras
@ 2013-11-14 17:42     ` Levente Kurusa
       [not found]       ` <52853fe6b0fd9_bf6d31e7c1a@nysa.notmuch>
  2013-11-15 11:27     ` Ingo Molnar
  1 sibling, 1 reply; 16+ messages in thread
From: Levente Kurusa @ 2013-11-14 17:42 UTC (permalink / raw)
  To: Felipe Contreras, Ingo Molnar
  Cc: Linux Kernel Mailing List, Ingo Molnar, Linus Torvalds, Andrew Morton

2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
> On Tue, Nov 12, 2013 at 6:03 PM, 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>
>>> ---
>>>
>> [...]
>>>
>>> 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').
> 
> 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, 'panic_timeout' is a variable of type 'int'.
Your 'long timeout;' line is wrong and should say 'int timeout;'


-- 
Regards,
Levente Kurusa

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
       [not found]       ` <52853fe6b0fd9_bf6d31e7c1a@nysa.notmuch>
@ 2013-11-14 22:32         ` Felipe Contreras
  2013-11-15 13:12           ` Levente Kurusa
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-11-14 22:32 UTC (permalink / raw)
  To: Levente Kurusa, Felipe Contreras, Ingo Molnar; +Cc: Linux Kernel Mailing List

Hi,

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

On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Levente Kurusa wrote:
>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>> > On Tue, Nov 12, 2013 at 6:03 PM, 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>
>> >>> ---
>> >>>
>> >> [...]
>> >>>
>> >>> 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').
>> >
>> > 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, 'panic_timeout' is a variable of type 'int'.
>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>
> 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-14 11:16   ` Felipe Contreras
  2013-11-14 17:42     ` Levente Kurusa
@ 2013-11-15 11:27     ` Ingo Molnar
  1 sibling, 0 replies; 16+ messages in thread
From: Ingo Molnar @ 2013-11-15 11:27 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Linux Kernel Mailing List, Ingo Molnar, Linus Torvalds, Andrew Morton


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

> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-14 22:32         ` Felipe Contreras
@ 2013-11-15 13:12           ` Levente Kurusa
  2013-11-15 19:33             ` Felipe Contreras
  0 siblings, 1 reply; 16+ messages in thread
From: Levente Kurusa @ 2013-11-15 13:12 UTC (permalink / raw)
  To: Felipe Contreras, Ingo Molnar; +Cc: Linux Kernel Mailing List

2013-11-14 23:32 keltezéssel, Felipe Contreras írta:
> Hi,
> 
> Sending again since the previous one was rejected by the server.
> 
> On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Levente Kurusa wrote:
>>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>>>> On Tue, Nov 12, 2013 at 6:03 PM, 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>
>>>>>> ---
>>>>>>
>>>>> [...]
>>>>>>
>>>>>> 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').
>>>>
>>>> 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, 'panic_timeout' is a variable of type 'int'.
>>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>>
>> 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.
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 13:12           ` Levente Kurusa
@ 2013-11-15 19:33             ` Felipe Contreras
  2013-11-15 19:55               ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-11-15 19:33 UTC (permalink / raw)
  To: Levente Kurusa; +Cc: Ingo Molnar, Linux Kernel Mailing List

On Fri, Nov 15, 2013 at 7:12 AM, Levente Kurusa <levex@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 19:33             ` Felipe Contreras
@ 2013-11-15 19:55               ` Linus Torvalds
  2013-11-15 20:10                 ` Felipe Contreras
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2013-11-15 19:55 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Levente Kurusa, Ingo Molnar, Linux Kernel Mailing List

On Fri, Nov 15, 2013 at 11:33 AM, Felipe Contreras
<felipe.contreras@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 19:55               ` Linus Torvalds
@ 2013-11-15 20:10                 ` Felipe Contreras
  2013-11-15 20:15                   ` Linus Torvalds
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-11-15 20:10 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Levente Kurusa, Ingo Molnar, Linux Kernel Mailing List

On Fri, Nov 15, 2013 at 1:55 PM, Linus Torvalds
<torvalds@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 20:10                 ` Felipe Contreras
@ 2013-11-15 20:15                   ` Linus Torvalds
  2013-11-15 20:45                     ` Felipe Contreras
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2013-11-15 20:15 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Levente Kurusa, Ingo Molnar, Linux Kernel Mailing List

On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
<felipe.contreras@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.

              Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 20:15                   ` Linus Torvalds
@ 2013-11-15 20:45                     ` Felipe Contreras
  2013-11-16 17:45                       ` Ingo Molnar
  0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-11-15 20:45 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Levente Kurusa, Ingo Molnar, Linux Kernel Mailing List

On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> <felipe.contreras@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-15 20:45                     ` Felipe Contreras
@ 2013-11-16 17:45                       ` Ingo Molnar
  2013-11-16 18:46                         ` Felipe Contreras
  0 siblings, 1 reply; 16+ messages in thread
From: Ingo Molnar @ 2013-11-16 17:45 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Linus Torvalds, Levente Kurusa, Linux Kernel Mailing List


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

> On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
> > <felipe.contreras@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-16 17:45                       ` Ingo Molnar
@ 2013-11-16 18:46                         ` Felipe Contreras
  2013-11-16 20:15                           ` Levente Kurusa
  2013-11-16 20:24                           ` Levente Kurusa
  0 siblings, 2 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-11-16 18:46 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Linus Torvalds, Levente Kurusa, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-16 18:46                         ` Felipe Contreras
@ 2013-11-16 20:15                           ` Levente Kurusa
  2013-11-16 20:24                           ` Levente Kurusa
  1 sibling, 0 replies; 16+ messages in thread
From: Levente Kurusa @ 2013-11-16 20:15 UTC (permalink / raw)
  To: Felipe Contreras, Ingo Molnar; +Cc: Linus Torvalds, Linux Kernel Mailing List

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:
> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> * Felipe Contreras <felipe.contreras@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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] panic: setup panic_timeout early
  2013-11-16 18:46                         ` Felipe Contreras
  2013-11-16 20:15                           ` Levente Kurusa
@ 2013-11-16 20:24                           ` Levente Kurusa
  1 sibling, 0 replies; 16+ messages in thread
From: Levente Kurusa @ 2013-11-16 20:24 UTC (permalink / raw)
  To: Felipe Contreras, Ingo Molnar; +Cc: Linux Kernel Mailing List

2013-11-16 19:46 keltezéssel, Felipe Contreras írta:
> On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> * Felipe Contreras <felipe.contreras@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 <levex@linux.com>

-- 
Regards,
Levente Kurusa

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-11-16 20:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-11 14:40 [PATCH v2] panic: setup panic_timeout early Felipe Contreras
2013-11-13  0:03 ` Ingo Molnar
2013-11-14 11:16   ` Felipe Contreras
2013-11-14 17:42     ` Levente Kurusa
     [not found]       ` <52853fe6b0fd9_bf6d31e7c1a@nysa.notmuch>
2013-11-14 22:32         ` Felipe Contreras
2013-11-15 13:12           ` Levente Kurusa
2013-11-15 19:33             ` Felipe Contreras
2013-11-15 19:55               ` Linus Torvalds
2013-11-15 20:10                 ` Felipe Contreras
2013-11-15 20:15                   ` Linus Torvalds
2013-11-15 20:45                     ` Felipe Contreras
2013-11-16 17:45                       ` Ingo Molnar
2013-11-16 18:46                         ` Felipe Contreras
2013-11-16 20:15                           ` Levente Kurusa
2013-11-16 20:24                           ` Levente Kurusa
2013-11-15 11:27     ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).