Re: Disabling gcc inline operation

[anish singh]

On Mon, Mar 24, 2014 at 2:33 AM, Arend van Spriel  wrote:
> On 23/03/14 23:31, anish singh wrote:
>>
>> Many a time i have got a crash and it is difficult
>> to find out the exact function which crashed
>> because the crash stack doesn't show the "real"
>> function because gcc inlines many functions when
>> ever it desires or when it optimizes for speed.
>>
>> So i don't want gcc to inline any function instead
>> just call the function so that i can see the crash
>> stack of each function called. I just want to do
>> this for debugging. Please let me know how can
>> i do that?
>>
>> What switch command to pass to gcc in the make
>> of linux kernel?
>
>
> You tried to look at gcc.gnu.org, right? Your best options are -O0 and/or
> -Og for debugging purposes.
It worked with -fno-inline
>
> Regards,
> Arend
>
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Disabling gcc inline operation

[anish singh]

On Mon, Mar 24, 2014 at 2:33 AM, Arend van Spriel ar...@broadcom.com wrote:
 On 23/03/14 23:31, anish singh wrote:

 Many a time i have got a crash and it is difficult
 to find out the exact function which crashed
 because the crash stack doesn't show the real
 function because gcc inlines many functions when
 ever it desires or when it optimizes for speed.

 So i don't want gcc to inline any function instead
 just call the function so that i can see the crash
 stack of each function called. I just want to do
 this for debugging. Please let me know how can
 i do that?

 What switch command to pass to gcc in the make
 of linux kernel?


 You tried to look at gcc.gnu.org, right? Your best options are -O0 and/or
 -Og for debugging purposes.
It worked with -fno-inline

 Regards,
 Arend


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


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

Disabling gcc inline operation

[anish singh]

Many a time i have got a crash and it is difficult
to find out the exact function which crashed
because the crash stack doesn't show the "real"
function because gcc inlines many functions when
ever it desires or when it optimizes for speed.

So i don't want gcc to inline any function instead
just call the function so that i can see the crash
stack of each function called. I just want to do
this for debugging. Please let me know how can
i do that?

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

Disabling gcc inline operation

[anish singh]

Many a time i have got a crash and it is difficult
to find out the exact function which crashed
because the crash stack doesn't show the real
function because gcc inlines many functions when
ever it desires or when it optimizes for speed.

So i don't want gcc to inline any function instead
just call the function so that i can see the crash
stack of each function called. I just want to do
this for debugging. Please let me know how can
i do that?

What switch command to pass to gcc in the make
of linux kernel?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: list_empty atomic?

[anish singh]

On Thu, Dec 12, 2013 at 12:21 PM, Filipe David Manana
 wrote:
> Hello,
>
> I have a list that is manipulated by several threads. Insert, remove
> and iteration are protected by a lock. Is the locking necessary too
> just for checking if the list is empty, i.e., is list_empty()
> atomic/safe to call without the lock held or not?
Which lock are you taking here?If mutex is used then
you still need to take a lock even when you are checking
the list.Incase of spinlock it depends on the code i.e.
if you are writing interrupt handler and if the interrupt
handler can run on all the cores simultaneously then
you still need to protect it with the spinlock AFICT.

best advice is to just take the damn lock.
>
> thanks
>
> --
> Filipe David Manana,
>
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."
>
> ___
> Kernelnewbies mailing list
> kernelnewb...@kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: list_empty atomic?

[anish singh]

On Thu, Dec 12, 2013 at 12:21 PM, Filipe David Manana
fdman...@gmail.com wrote:
 Hello,

 I have a list that is manipulated by several threads. Insert, remove
 and iteration are protected by a lock. Is the locking necessary too
 just for checking if the list is empty, i.e., is list_empty()
 atomic/safe to call without the lock held or not?
Which lock are you taking here?If mutex is used then
you still need to take a lock even when you are checking
the list.Incase of spinlock it depends on the code i.e.
if you are writing interrupt handler and if the interrupt
handler can run on all the cores simultaneously then
you still need to protect it with the spinlock AFICT.

best advice is to just take the damn lock.

 thanks

 --
 Filipe David Manana,

 Reasonable men adapt themselves to the world.
  Unreasonable men adapt the world to themselves.
  That's why all progress depends on unreasonable men.

 ___
 Kernelnewbies mailing list
 kernelnewb...@kernelnewbies.org
 http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: questions of cpuidle

[anish singh]

On Mon, Dec 9, 2013 at 11:27 PM, Daniel Lezcano
 wrote:
> On 12/10/2013 07:33 AM, Alex Shi wrote:
>>
>> On 12/09/2013 10:17 PM, Daniel Lezcano wrote:
>>>
>>>
>>> Concerning the wake up of the cpu: the cpu disabled the irq and
>>> goes to sleep, it is up to the firmware to wake up the cpu when an
>>> interrupt occurs. It will exits its sleep state, call
>>> clock_events_notify(EXIT), by this way re-switching to the local
>>> timer, and then re-enabling the local interrupt which leads to the
>>> interrupt handler.
>>
>>
>> Thanks a lots for excellent article and detailed explains!
>>
>> So, if the firmware is in response to wake up cpu. that means there
>> is a unit which control the firmware and it can not be power down.
>
>
> Correct.
>
>
>> Do you know which unit running the firmware to wake up deep idle
>> CPU.
>
>
> That depends on the SoC implementation.
and which is intentionally kept hidden away.
>
> Some SoC have a "Power Management Unit". The PMU has several idle states
> defined, each of them are described in the technical reference manual
> (TRM) with the wake up sources.
PMU is intentionally kept hidden by OEM companies as this way
they protect there hardware IP.
>
> Some SoC don't have any PMU and the idle states are very few, keeping
> most of the logic on.
>
> Some other SoC hide the PMU behind PSCI calls.
which is intentional.
>
>
>> And does the wake up pass via GIC to CPU? If so, does the GIC need
>> keep awake when all cpu idle? If not, how the firmware give the
>> interrupt to CPU? And I am wondering if the deep idle cpu voltage get
>> to near 0. How the cpu get the interrupt signal?
>
>
> If a deep idle state powers down the GIC, it is up to the PMU to proxy
> the interrupts. When an interrupt occurs, the PMU powers up the logic,
> including the GIC. The notifier call chain with cpu_suspend / cpu_resume
> will save and restore the GIC registers.
>
> But this is hardware specific and will depend on how the PMU is
> implemented and how far it goes in the power management.
>
> You have a good example in the drivers/cpuidle/cpuidle-ux500.c to
> understand with the comments how the interrupts are handled through the
> power management unit.
>
> In the Xillinx documentation available on the web [1], the chapter 24.4
> gives the information about one kind of PMU.
>
> I believe the mechanism is pretty similar on all the hardware but it is
> obfuscated by a generic power instruction like mwait.
>
>   -- Daniel
>
> [1]
> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
>
>
>
>>> There are some more informations in the wiki page [1].
>>>
>>> -- Daniel
>>>
>>> [1]
>>> https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/WakeUpSources
>>
>>
>>>
>>
>
>
> --
>   Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: questions of cpuidle

[anish singh]

On Mon, Dec 9, 2013 at 11:27 PM, Daniel Lezcano
daniel.lezc...@linaro.org wrote:
 On 12/10/2013 07:33 AM, Alex Shi wrote:

 On 12/09/2013 10:17 PM, Daniel Lezcano wrote:


 Concerning the wake up of the cpu: the cpu disabled the irq and
 goes to sleep, it is up to the firmware to wake up the cpu when an
 interrupt occurs. It will exits its sleep state, call
 clock_events_notify(EXIT), by this way re-switching to the local
 timer, and then re-enabling the local interrupt which leads to the
 interrupt handler.


 Thanks a lots for excellent article and detailed explains!

 So, if the firmware is in response to wake up cpu. that means there
 is a unit which control the firmware and it can not be power down.


 Correct.


 Do you know which unit running the firmware to wake up deep idle
 CPU.


 That depends on the SoC implementation.
and which is intentionally kept hidden away.

 Some SoC have a Power Management Unit. The PMU has several idle states
 defined, each of them are described in the technical reference manual
 (TRM) with the wake up sources.
PMU is intentionally kept hidden by OEM companies as this way
they protect there hardware IP.

 Some SoC don't have any PMU and the idle states are very few, keeping
 most of the logic on.

 Some other SoC hide the PMU behind PSCI calls.
which is intentional.


 And does the wake up pass via GIC to CPU? If so, does the GIC need
 keep awake when all cpu idle? If not, how the firmware give the
 interrupt to CPU? And I am wondering if the deep idle cpu voltage get
 to near 0. How the cpu get the interrupt signal?


 If a deep idle state powers down the GIC, it is up to the PMU to proxy
 the interrupts. When an interrupt occurs, the PMU powers up the logic,
 including the GIC. The notifier call chain with cpu_suspend / cpu_resume
 will save and restore the GIC registers.

 But this is hardware specific and will depend on how the PMU is
 implemented and how far it goes in the power management.

 You have a good example in the drivers/cpuidle/cpuidle-ux500.c to
 understand with the comments how the interrupts are handled through the
 power management unit.

 In the Xillinx documentation available on the web [1], the chapter 24.4
 gives the information about one kind of PMU.

 I believe the mechanism is pretty similar on all the hardware but it is
 obfuscated by a generic power instruction like mwait.

   -- Daniel

 [1]
 http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf



 There are some more informations in the wiki page [1].

 -- Daniel

 [1]
 https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/WakeUpSources






 --
  http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs

 Follow Linaro:  http://www.facebook.com/pages/Linaro Facebook |
 http://twitter.com/#!/linaroorg Twitter |
 http://www.linaro.org/linaro-blog/ Blog

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

Re: list_head and lock?

[anish singh]

On Sun, Nov 17, 2013 at 5:19 PM, 韩磊  wrote:
> when we delete,add,search,amend the list_head,should we use spinlock
> or rcu in case of conflicit to list_head???
There is no implicit locking when we use 'list' api's.You should explicitly
do that AFAIK.
>
> Thank you!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: list_head and lock?

[anish singh]

On Sun, Nov 17, 2013 at 5:19 PM, 韩磊 bonben1...@gmail.com wrote:
 when we delete,add,search,amend the list_head,should we use spinlock
 or rcu in case of conflicit to list_head???
There is no implicit locking when we use 'list' api's.You should explicitly
do that AFAIK.

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

Re: [RFC][PATCH] x86: Lazy disabling of interrupts

[anish singh]

On Thu, Oct 10, 2013 at 5:57 PM, Steven Rostedt 
(by way of Steven Rostedt ) (by way of Steven
Rostedt  wrote:
>
> [ Resending, as somehow Claws email, removed the quotes from "H. Peter
>   Anvin", and that prevented LKML from receiving this ]
>
> *** NOT FOR INCLUSION ***
>
> What this does
> --
>
> There's several locations in the kernel that disable interrupts and
> enable them rather quickly. Most likely an interrupt will not happen
> during this time frame. Instead of actually disabling interrupts, set
> a flag instead, and if an interrupt were to come in, it would see
> the flag set and return (keeping interrupts disabled for real). When
> the flag is cleared, it checks if an interrupt came in and if it did
> it simulates that interrupt.
I think the concept is similar to the linux core interrupt code handling
where it does the lazy disabling of interrupt.

I was just wondering if we can do the same concept for ARM arch
and if some part of your code can be shared.It would be nice academic
exercise.
>
> Rational
> 
> I noticed in function tracing that disabling interrupts is quite
> expensive. To measure this, I ran the stack tracer and several runs of
> hackbench:
>
>   trace-cmd stack --start
>   for i in `seq 10` ; do time ./hackbench 100; done &> output
>
> The stack tracer uses function tracing to examine every function's stack
> as the function is executed. If it finds a stack larger than the last
> max stack, it records it. But most of the time it just does the check
> and returns. To do this safely (using per cpu variables), it disables
> preemption:
>
> kernel/trace/trace_stack.c: stack_trace_call()
>
> preempt_disable_notrace();
> [...]
> check_stack(ip, );
> [...]
> preempt_enable_notrace();
>
> Most of the time, check_stack() just returns without doing anything
> as it is unlikely to hit a new max (it does very seldom), and shouldn't
> be an issue in the benchmarks.
>
> Then I changed this code do be:
>
>
> kernel/trace/trace_stack.c: stack_trace_call()
>
> local_irq_save(flags);
> [...]
> check_stack(ip, );
> [...]
> local_irq_restore(flags);
>
> And ran the test again. This caused a very large performance hit.
>
> Running on: Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz
>   (4 cores HT enabled)
>
> Here's the differences:
>
> With preempt disable (10 runs):
>
> Time from hackbench:
> avg=2.0462
> std=0.181487189630563
>
> System time (from time):
> avg=10.5879
> std=0.862181477416443
>
> With irq disable (10 runs):
>
> Time from hackbench:
> avg=2.7082
> std=0.12304308188598
>
> System time (from time):
> avg=14.6807
> std=0.313856814487116
>
> A 32% performance hit when using irq disabling told me that this is
> something we could improve on in normal activities. That is, avoid
> disabling interrupts when possible. For the last couple of weeks I
> decided to implement a "lazy irq disable" to do this.
>
>
> The Setup
> -
>
> I only had to touch four functions that deal with interrupts:
>
>o native_irq_enable()
>o native_irq_disable()
>o native_save_fl()
>o native_restore_fl()
>
> As these are the basis for all other C functions that disable interrupts
>  (ie. local_irq_save(), local_irq_disable(), spin_lock_irq(), etc)
> just modifying them made it much easier to implement.
>
> I added raw_* versions of each that do the real enabling and disabling.
> Basically, the raw_* versions are what they currently do today.
>
> Per CPU
> ---
>
> I added a couple of per cpu variables:
>
>o lazy_irq_disabled_flags
>o lazy_irq_func
>o lazy_irq_vector
>o lazy_irq_on
>
> The lazy_irq_disabled_flags holds the state of the system. The flags
> are:
>
>  DISABLED - When set, irqs are considered disabled (whether they are for
> real or not).
>
>  TEMP_DISABLE - Set when coming from a trap or other assembly that
>  disables interrupts to let the native_irq_enable() know that interrupts
> are really disabled, and enable them as well.
>
>  IDLE - Used to tell the native_* functions that we are going idle and
>  to continue to do real interrupt disabling/enabling.
>
>  REAL_DISABLE - Set by interrupts themselves. When interrupts are
>  running, (this includes softirqs), we enable and disable interrupts
>  normally. No lazy disabling is done from interrupt context.
>
> The lazy_irq_func holds the interrupt function that was to trigger when
> we were in lazy irq disabled mode with interrupts enabled. Explained
> below.
>
> The lazy_irq_vector holds the orig_rax, which is the vector that the
> interrupt handler needs to know what interrupt vector was triggered.
> Saved for the same use as 

Re: [RFC][PATCH] x86: Lazy disabling of interrupts

[anish singh]

On Thu, Oct 10, 2013 at 5:57 PM, Steven Rostedt rost...@goodmis.org
(by way of Steven Rostedt rost...@goodmis.org) (by way of Steven
Rostedt rost...@goodmis.org wrote:

 [ Resending, as somehow Claws email, removed the quotes from H. Peter
   Anvin, and that prevented LKML from receiving this ]

 *** NOT FOR INCLUSION ***

 What this does
 --

 There's several locations in the kernel that disable interrupts and
 enable them rather quickly. Most likely an interrupt will not happen
 during this time frame. Instead of actually disabling interrupts, set
 a flag instead, and if an interrupt were to come in, it would see
 the flag set and return (keeping interrupts disabled for real). When
 the flag is cleared, it checks if an interrupt came in and if it did
 it simulates that interrupt.
I think the concept is similar to the linux core interrupt code handling
where it does the lazy disabling of interrupt.

I was just wondering if we can do the same concept for ARM arch
and if some part of your code can be shared.It would be nice academic
exercise.

 Rational
 
 I noticed in function tracing that disabling interrupts is quite
 expensive. To measure this, I ran the stack tracer and several runs of
 hackbench:

   trace-cmd stack --start
   for i in `seq 10` ; do time ./hackbench 100; done  output

 The stack tracer uses function tracing to examine every function's stack
 as the function is executed. If it finds a stack larger than the last
 max stack, it records it. But most of the time it just does the check
 and returns. To do this safely (using per cpu variables), it disables
 preemption:

 kernel/trace/trace_stack.c: stack_trace_call()

 preempt_disable_notrace();
 [...]
 check_stack(ip, stack);
 [...]
 preempt_enable_notrace();

 Most of the time, check_stack() just returns without doing anything
 as it is unlikely to hit a new max (it does very seldom), and shouldn't
 be an issue in the benchmarks.

 Then I changed this code do be:


 kernel/trace/trace_stack.c: stack_trace_call()

 local_irq_save(flags);
 [...]
 check_stack(ip, stack);
 [...]
 local_irq_restore(flags);

 And ran the test again. This caused a very large performance hit.

 Running on: Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz
   (4 cores HT enabled)

 Here's the differences:

 With preempt disable (10 runs):

 Time from hackbench:
 avg=2.0462
 std=0.181487189630563

 System time (from time):
 avg=10.5879
 std=0.862181477416443

 With irq disable (10 runs):

 Time from hackbench:
 avg=2.7082
 std=0.12304308188598

 System time (from time):
 avg=14.6807
 std=0.313856814487116

 A 32% performance hit when using irq disabling told me that this is
 something we could improve on in normal activities. That is, avoid
 disabling interrupts when possible. For the last couple of weeks I
 decided to implement a lazy irq disable to do this.


 The Setup
 -

 I only had to touch four functions that deal with interrupts:

o native_irq_enable()
o native_irq_disable()
o native_save_fl()
o native_restore_fl()

 As these are the basis for all other C functions that disable interrupts
  (ie. local_irq_save(), local_irq_disable(), spin_lock_irq(), etc)
 just modifying them made it much easier to implement.

 I added raw_* versions of each that do the real enabling and disabling.
 Basically, the raw_* versions are what they currently do today.

 Per CPU
 ---

 I added a couple of per cpu variables:

o lazy_irq_disabled_flags
o lazy_irq_func
o lazy_irq_vector
o lazy_irq_on

 The lazy_irq_disabled_flags holds the state of the system. The flags
 are:

  DISABLED - When set, irqs are considered disabled (whether they are for
 real or not).

  TEMP_DISABLE - Set when coming from a trap or other assembly that
  disables interrupts to let the native_irq_enable() know that interrupts
 are really disabled, and enable them as well.

  IDLE - Used to tell the native_* functions that we are going idle and
  to continue to do real interrupt disabling/enabling.

  REAL_DISABLE - Set by interrupts themselves. When interrupts are
  running, (this includes softirqs), we enable and disable interrupts
  normally. No lazy disabling is done from interrupt context.

 The lazy_irq_func holds the interrupt function that was to trigger when
 we were in lazy irq disabled mode with interrupts enabled. Explained
 below.

 The lazy_irq_vector holds the orig_rax, which is the vector that the
 interrupt handler needs to know what interrupt vector was triggered.
 Saved for the same use as lazy_irq_func is.

 Because preempt_disable is currently a task flag, we need a 

Re: [PATCH] fix a Sparse warning in the arch/x86/kernel/irq_work.c file

[anish singh]

On Tue, Aug 13, 2013 at 5:59 AM, Steven Rostedt  wrote:
> On Mon, 12 Aug 2013 15:18:16 -0700
> Shailaja Neelam  wrote:
>
>> I am a high school student trying to become familiar with the
>> opensource process and linux kernel. This is my first submission to
>> the ITC mailing list.
>
> Hi Shailaja,
>
> Welcome to Linux kernel development :-)
>
>>
>> My patch is for the file arch/x86/kernel/irq_work.c in the vesion
>> linux-3.10 kernel. When I ran the kernel with Sparse, the error read:
>> arch/x86/kernel/irq_work.c:21:
>> 6: warning: symbol 'arch_irq_work_raise'
>> was not declared. Should it be static?
>>
>> To fix this (rather than add static) I declared the symbol in the
>> header file linux/irq_work.h.
>
> Correct, because adding static would have been a bug.
>
>
>> Afterwards, my error did not show up
>> when I ran the kernel with Sparse again. I also ran the command "make
>> menuconfig" to change the kernel version so that I could assure the
>> correct kernel was running when I tested it, and it was. Then I test
>> built the kernel. It built and rebooted correctly.
>
> The patch looks good. Let me give you a bit more information and
> background on that function. Just your FYI.
>
> The purpose of irq_work() in general, is to trigger some event in a
> critical location that is not safe to do the event you want to trigger.
> For example, in perf, it may be executing within a Non-Maskable
> Interrupt (NMI) or within the scheduler with the runqueue (rq) lock
> held. If it would like to wake up a task that is monitoring the perf
> output, it can't because to do so would require taking the rq lock and
> possibly causing a deadlock.
>
> Instead, it calls irq_work() to do the event within a normal interrupt
> context. Some architectures have the ability to trigger an interrupt on

irq_work processes event in normal interrupt context as you said but
why interrupt context ? Is it because of the fast processing which is
needed? Can we use softirq as anyways we have interrupt
disabled(functions which calls irq_work makes sure of that right?).
Hope I am not asking something very obvious.
> the current CPU that it is running on. This way, if we are in an NMI,
> we trigger the self interrupt to the CPU, but because NMIs run with
> interrupts disable, and the rq lock is held with interrupts disabled,
> the interrupt will stay pending until interrupts are enabled again (out
> of NMI and out from holding the rq lock). When interrupts are enabled,
> the interrupt that we sent will trigger and run our event in a safe
> location (someplace that allows for interrupts to be enabled).
>
> But to do this self triggering of an interrupt requires specific
> architecture knowledge. As Linux supports 30 architectures, and few
> of us have hardware to test on each of these or even know how to write
> code for all of them, we have ways to do things for just the
> architectures we are familiar with. Some architectures may not even
> have the ability to do what we want, even if we knew the architecture
> well.
>
> The "arch_irq_work_raise()" function is one of these cases. For
> architectures that do not support a self triggering interrupt, or one
> that we just didn't get to yet, we create a generic
> arch_irq_work_raise() function that just does our work at the next
> timer interrupt. This isn't the most efficient way, because that next
> timer interrupt may be 10 milliseconds away. But we annotate that
> function with the gcc "__attribute__((weak))" attribute (defined in
> include/linux/compiler-gcc.h as "__weak").
>
> What the __weak attribute does, is to tell the compiler (linker really)
> that this function is to be used if it is not defined someplace else.
> Then, in an architecture that has this specific optimization, we write
> an arch_irq_work_raise() function without the "__weak" attribute, and
> the linker will use that function instead at all of the call sites that
> reference it. This way, the generic code can support architectures that
> does the optimization as well as those that don't.
>
>
>>
>> Signed-off-by: Shailaja Neelam 
>
> Reviewed-by: Steven Rostedt 

Nice explanation.
>
> -- Steve
>
>
>> ---
>> --- linux-3.10/include/linux/irq_
>> work.h2013-06-30 15:13:29.0 -0700
>> +++ linux-3.10.change/include/linux/irq_work.h2013-07-24
>> 12:06:15.521140635 -0700
>> @@ -33,6 +33,7 @@ void init_irq_work(struct irq_work *work
>>  void irq_work_queue(struct irq_work *work);
>>  void irq_work_run(void);
>>  void irq_work_sync(struct irq_work *work);
>> +void arch_irq_work_raise(void);
>>
>>  #ifdef CONFIG_IRQ_WORK
>>  bool irq_work_needs_cpu(void);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fix a Sparse warning in the arch/x86/kernel/irq_work.c file

[anish singh]

On Tue, Aug 13, 2013 at 5:59 AM, Steven Rostedt rost...@goodmis.org wrote:
 On Mon, 12 Aug 2013 15:18:16 -0700
 Shailaja Neelam neelamsha...@gmail.com wrote:

 I am a high school student trying to become familiar with the
 opensource process and linux kernel. This is my first submission to
 the ITC mailing list.

 Hi Shailaja,

 Welcome to Linux kernel development :-)


 My patch is for the file arch/x86/kernel/irq_work.c in the vesion
 linux-3.10 kernel. When I ran the kernel with Sparse, the error read:
 arch/x86/kernel/irq_work.c:21:
 6: warning: symbol 'arch_irq_work_raise'
 was not declared. Should it be static?

 To fix this (rather than add static) I declared the symbol in the
 header file linux/irq_work.h.

 Correct, because adding static would have been a bug.


 Afterwards, my error did not show up
 when I ran the kernel with Sparse again. I also ran the command make
 menuconfig to change the kernel version so that I could assure the
 correct kernel was running when I tested it, and it was. Then I test
 built the kernel. It built and rebooted correctly.

 The patch looks good. Let me give you a bit more information and
 background on that function. Just your FYI.

 The purpose of irq_work() in general, is to trigger some event in a
 critical location that is not safe to do the event you want to trigger.
 For example, in perf, it may be executing within a Non-Maskable
 Interrupt (NMI) or within the scheduler with the runqueue (rq) lock
 held. If it would like to wake up a task that is monitoring the perf
 output, it can't because to do so would require taking the rq lock and
 possibly causing a deadlock.

 Instead, it calls irq_work() to do the event within a normal interrupt
 context. Some architectures have the ability to trigger an interrupt on

irq_work processes event in normal interrupt context as you said but
why interrupt context ? Is it because of the fast processing which is
needed? Can we use softirq as anyways we have interrupt
disabled(functions which calls irq_work makes sure of that right?).
Hope I am not asking something very obvious.
 the current CPU that it is running on. This way, if we are in an NMI,
 we trigger the self interrupt to the CPU, but because NMIs run with
 interrupts disable, and the rq lock is held with interrupts disabled,
 the interrupt will stay pending until interrupts are enabled again (out
 of NMI and out from holding the rq lock). When interrupts are enabled,
 the interrupt that we sent will trigger and run our event in a safe
 location (someplace that allows for interrupts to be enabled).

 But to do this self triggering of an interrupt requires specific
 architecture knowledge. As Linux supports 30 architectures, and few
 of us have hardware to test on each of these or even know how to write
 code for all of them, we have ways to do things for just the
 architectures we are familiar with. Some architectures may not even
 have the ability to do what we want, even if we knew the architecture
 well.

 The arch_irq_work_raise() function is one of these cases. For
 architectures that do not support a self triggering interrupt, or one
 that we just didn't get to yet, we create a generic
 arch_irq_work_raise() function that just does our work at the next
 timer interrupt. This isn't the most efficient way, because that next
 timer interrupt may be 10 milliseconds away. But we annotate that
 function with the gcc __attribute__((weak)) attribute (defined in
 include/linux/compiler-gcc.h as __weak).

 What the __weak attribute does, is to tell the compiler (linker really)
 that this function is to be used if it is not defined someplace else.
 Then, in an architecture that has this specific optimization, we write
 an arch_irq_work_raise() function without the __weak attribute, and
 the linker will use that function instead at all of the call sites that
 reference it. This way, the generic code can support architectures that
 does the optimization as well as those that don't.



 Signed-off-by: Shailaja Neelam neelamsha...@gmail.com

 Reviewed-by: Steven Rostedt rost...@goodmis.org

Nice explanation.

 -- Steve


 ---
 --- linux-3.10/include/linux/irq_
 work.h2013-06-30 15:13:29.0 -0700
 +++ linux-3.10.change/include/linux/irq_work.h2013-07-24
 12:06:15.521140635 -0700
 @@ -33,6 +33,7 @@ void init_irq_work(struct irq_work *work
  void irq_work_queue(struct irq_work *work);
  void irq_work_run(void);
  void irq_work_sync(struct irq_work *work);
 +void arch_irq_work_raise(void);

  #ifdef CONFIG_IRQ_WORK
  bool irq_work_needs_cpu(void);

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

Re: [PATCH] bitops: move BITS() macro to the bitops file

[anish singh]

On Wed, Aug 7, 2013 at 2:36 PM, Linus Walleij  wrote:
> This macro was invented by Mattias Nilsson for the usecase
> where you want to set a sequence of bits inside a n-bit
> word, while leaving the head and tail of the sequence all
> zeroes. For example:
>
>   #include 
>
>   u16 mask = BITS(4, 12);

nice.I don't have anything against this patch but couldn't understand
how this works.So i wrote what i knew as below:

#include 

#define BIT(nr)(1UL << (nr))
#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
#define BITS_MY(_start, _end) (((BIT(_end-_start+1))-1)<<_start)
int main()
{
printf("%x\n", BITS(4,12));
printf("%x\n", BITS_MY(4,12));
return 0;
}

>
> Yields a mask like this:
>
>   0001
>
> This patch moves the construct out of the MFD PRCMU driver
> and make it available for common use, after noticing in a
> review or two that it would be useful for others.
>
> Cc: Akinobu Mita 
> Signed-off-by: Linus Walleij 
> ---
>  drivers/mfd/dbx500-prcmu-regs.h | 2 --
>  include/linux/bitops.h  | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/dbx500-prcmu-regs.h b/drivers/mfd/dbx500-prcmu-regs.h
> index 4f6f0fa..906e75e 100644
> --- a/drivers/mfd/dbx500-prcmu-regs.h
> +++ b/drivers/mfd/dbx500-prcmu-regs.h
> @@ -13,8 +13,6 @@
>  #ifndef __DB8500_PRCMU_REGS_H
>  #define __DB8500_PRCMU_REGS_H
>
> -#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
> -
>  #define PRCM_ACLK_MGT  (0x004)
>  #define PRCM_SVAMMCSPCLK_MGT   (0x008)
>  #define PRCM_SIAMMDSPCLK_MGT   (0x00C)
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index a3b6b82..1f9d78b 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -6,6 +6,7 @@
>  #define BIT(nr)(1UL << (nr))
>  #define BIT_MASK(nr)   (1UL << ((nr) % BITS_PER_LONG))
>  #define BIT_WORD(nr)   ((nr) / BITS_PER_LONG)
> +#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))

Shouldn't there be compile time check here for checking _end > _start?
BUILD_BUG_ON(_end<_start);

>  #define BITS_PER_BYTE  8
>  #define BITS_TO_LONGS(nr)  DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>  #endif
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] bitops: move BITS() macro to the bitops file

[anish singh]

On Wed, Aug 7, 2013 at 2:36 PM, Linus Walleij linus.wall...@linaro.org wrote:
 This macro was invented by Mattias Nilsson for the usecase
 where you want to set a sequence of bits inside a n-bit
 word, while leaving the head and tail of the sequence all
 zeroes. For example:

   #include linux/bitops.h

   u16 mask = BITS(4, 12);

nice.I don't have anything against this patch but couldn't understand
how this works.So i wrote what i knew as below:

#include stdio.h

#define BIT(nr)(1UL  (nr))
#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
#define BITS_MY(_start, _end) (((BIT(_end-_start+1))-1)_start)
int main()
{
printf(%x\n, BITS(4,12));
printf(%x\n, BITS_MY(4,12));
return 0;
}


 Yields a mask like this:

   0001

 This patch moves the construct out of the MFD PRCMU driver
 and make it available for common use, after noticing in a
 review or two that it would be useful for others.

 Cc: Akinobu Mita m...@miraclelinux.com
 Signed-off-by: Linus Walleij linus.wall...@linaro.org
 ---
  drivers/mfd/dbx500-prcmu-regs.h | 2 --
  include/linux/bitops.h  | 1 +
  2 files changed, 1 insertion(+), 2 deletions(-)

 diff --git a/drivers/mfd/dbx500-prcmu-regs.h b/drivers/mfd/dbx500-prcmu-regs.h
 index 4f6f0fa..906e75e 100644
 --- a/drivers/mfd/dbx500-prcmu-regs.h
 +++ b/drivers/mfd/dbx500-prcmu-regs.h
 @@ -13,8 +13,6 @@
  #ifndef __DB8500_PRCMU_REGS_H
  #define __DB8500_PRCMU_REGS_H

 -#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
 -
  #define PRCM_ACLK_MGT  (0x004)
  #define PRCM_SVAMMCSPCLK_MGT   (0x008)
  #define PRCM_SIAMMDSPCLK_MGT   (0x00C)
 diff --git a/include/linux/bitops.h b/include/linux/bitops.h
 index a3b6b82..1f9d78b 100644
 --- a/include/linux/bitops.h
 +++ b/include/linux/bitops.h
 @@ -6,6 +6,7 @@
  #define BIT(nr)(1UL  (nr))
  #define BIT_MASK(nr)   (1UL  ((nr) % BITS_PER_LONG))
  #define BIT_WORD(nr)   ((nr) / BITS_PER_LONG)
 +#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))

Shouldn't there be compile time check here for checking _end  _start?
BUILD_BUG_ON(_end_start);

  #define BITS_PER_BYTE  8
  #define BITS_TO_LONGS(nr)  DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
  #endif
 --
 1.8.1.4

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

Re: Question:Bsearch replace the search_extable implementations

[anish singh]

On Tue, Jul 16, 2013 at 10:29 AM, Rusty Russell  wrote:
> anish singh  writes:
>> Hello Rusty,
>>
>> Right now I see so many places in the kernel
>> where we open code binary search implementations
>> such as search_extable implementations.
>>
>> http://lxr.free-electrons.com/ident?a=powerpc;i=search_extable
>>
>> Would it be ok to replace this with bsearch lib code?
>>
>> I am not trying to fix any problem nor I see
>> any problem in current implementations but
>> if we can replace the large amount of code
>> would it not be a good reason to send a patch
>> for this?
>
> It should be fine, yes.
great.Will be sending patch for this soon.
>
> Thanks!
> Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Question:Bsearch replace the search_extable implementations

[anish singh]

Hello Rusty,

Right now I see so many places in the kernel
where we open code binary search implementations
such as search_extable implementations.

http://lxr.free-electrons.com/ident?a=powerpc;i=search_extable

Would it be ok to replace this with bsearch lib code?

I am not trying to fix any problem nor I see
any problem in current implementations but
if we can replace the large amount of code
would it not be a good reason to send a patch
for this?

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

Question:Bsearch replace the search_extable implementations

[anish singh]

Hello Rusty,

Right now I see so many places in the kernel
where we open code binary search implementations
such as search_extable implementations.

http://lxr.free-electrons.com/ident?a=powerpc;i=search_extable

Would it be ok to replace this with bsearch lib code?

I am not trying to fix any problem nor I see
any problem in current implementations but
if we can replace the large amount of code
would it not be a good reason to send a patch
for this?

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

Re: Question:Bsearch replace the search_extable implementations

[anish singh]

On Tue, Jul 16, 2013 at 10:29 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 anish singh anish198519851...@gmail.com writes:
 Hello Rusty,

 Right now I see so many places in the kernel
 where we open code binary search implementations
 such as search_extable implementations.

 http://lxr.free-electrons.com/ident?a=powerpc;i=search_extable

 Would it be ok to replace this with bsearch lib code?

 I am not trying to fix any problem nor I see
 any problem in current implementations but
 if we can replace the large amount of code
 would it not be a good reason to send a patch
 for this?

 It should be fine, yes.
great.Will be sending patch for this soon.

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Fri, Jun 28, 2013 at 1:01 AM, Wim Van Sebroeck  wrote:
> Hi Anish,
>
>> Certain watchdog drivers use a timer to keep kicking the watchdog at
>> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>> we can't guarantee that watchdog will be pinged fast enough
>> for all system loads, especially if timeout is configured for
>> less than or equal to 1 second(basically small values).
>>
>> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>> add this functionality of individual watchdog drivers in the core
>> watchdog core.
>
> Have you considered the effect this change has on all watchdog drivers
> that don't need a timer?
No,I will drop this patch.
>
> Kind regards,
> Wim.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Fri, Jun 28, 2013 at 1:01 AM, Wim Van Sebroeck w...@iguana.be wrote:
 Hi Anish,

 Certain watchdog drivers use a timer to keep kicking the watchdog at
 a rate of 0.5s (HZ/2) untill userspace times out.They do this as
 we can't guarantee that watchdog will be pinged fast enough
 for all system loads, especially if timeout is configured for
 less than or equal to 1 second(basically small values).

 As suggested by Wim Van Sebroeck  Guenter Roeck we should
 add this functionality of individual watchdog drivers in the core
 watchdog core.

 Have you considered the effect this change has on all watchdog drivers
 that don't need a timer?
No,I will drop this patch.

 Kind regards,
 Wim.

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

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Fri, Jun 14, 2013 at 9:42 PM, Steven Rostedt  wrote:
> On Fri, 2013-06-14 at 21:33 +0530, anish singh wrote:
>
>> May I know why we zeroed in on 1Hz? Is there any logical reason
>> or just because it is above 0Hz?
>> >
>
> We had to keep a tick. What number would you have picked?

Great.I will just confirming my understanding.Thanks.
>
> -- Steve
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

Thanks Paul & Steve for replying.
On Fri, Jun 14, 2013 at 5:56 PM, Paul E. McKenney
 wrote:
> On Fri, Jun 14, 2013 at 09:47:31AM +0530, anish singh wrote:
>> On Thu, Jun 13, 2013 at 10:46 PM, Steven Rostedt  wrote:
>> > On Thu, 2013-06-13 at 21:51 +0530, anish singh wrote:
>> >
>> >> > The concept behind full dynamic ticks is very easy. When you set a given
>> >> > CPU(s) to dynamic tick, when it only has a single task scheduled on that
>> >> > CPU, it disables the periodic tick. This removes essentially *all*
>> >>
>> >> Documentation/timers/highres.txt states that
>> >> hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
>> >> you mind elaborating "single task scheduled on that CPU"?
>> >> I am bit new to this so please excuse me if the question is too basic.
>> >
>> > That's the old CONFIG_NO_HZ, which only disables the tick on idle. What
>> > we are working on is to also disable the tick when there's only one task
>> > running on a given CPU. Why have as schedule tick when there's nothing
>> > to schedule?
>> >
>> > 3.10 now has new config options:
>> >
>> > CONFIG_NO_HZ_PERIODIC - which is NO_HZ disabled
>> >(the old # CONFIG_NO_HZ not set)
>> >
>> > CONFIG_NO_HZ_IDLE - which is what CONFIG_NO_HZ use to be.
>> >
>> > Note, CONFIG_NO_HZ still exists and if set, will make CONFIG_NO_HZ_IDLE
>> > the default. This was to keep the same configuration for people who
>> > update their kernel and run make oldconfig.
>> >
>> > Then there's the new:
>> >
>> > CONFIG_NO_HZ_FULL - this enables CONFIG_NO_HZ_IDLE plus adds the new
>> > feature with disabling the tick when only one task is running on a given
>> > CPU.
>>
>> Thanks and some more explanation in below documents.
>> Documentation/timers/NO_HZ.txt
>> Documentation/timers/highres.txt
>> >
>> >
>> >> > latency from the kernel! That is, if the task is doing some complex
>> >> > calculations, it wont be interrupted for kernel maintenance. A lot of
>> >> > Red Hat customers would love to have this feature. It allows for
>> >> > extremely low latency actions even without a real-time kernel. Heck, it
>> >> > works without even kernel preemption.
>> >> >
>> >> > Now removing the periodic tick is not a trivial task, and this is where
>> >>
>> >> You mean getting rid of period ticks in the kernel code is not easy as 
>> >> there
>> >> are many features such as perf is dependent on it right and that is why
>> >> instead of completely removing periodic ticks we just set the periodic 
>> >> tick
>> >> to happen at 1HZ instead of CONFIG_HZ value?
>> >
>> > IIRC, the reason for moving to 1 HZ is so that the scheduler doesn't get
>> > confused with overflows. It still needs to handle time keeping for
>> "overflows" meaning the tick happening at 1HZ?
>> However as I see here in this patch http://lwn.net/Articles/549592/ -
>> you have plans to move it to 0Hz then how does scheduler cope
>> with this?Does it not need this information to schedule a different
>> task when the current task on "adaptive-ticks CPU" is done?
>
> When the current task completed, it will enter the kernel, allowing
> the scheduler to take over.
>
> If a second task awakens or is created, there will either be some sort
> of interrupt to the CPU itself, or to some other CPU, and this other
> CPU will IPI the first CPU.  Either way, the scheduler gains control
> when it needs it -- but avoids continually interrupting the task.
>
>> Anyway doesn't "future works" should be part of No-hz.txt document?
>
> It does, please see the very last bullet of the document:
>
> o   Some process-handling operations still require the occasional
> scheduling-clock tick.  These operations include calculating CPU
> load, maintaining sched average, computing CFS entity vruntime,
> computing avenrun, and carrying out load balancing.  They are
> currently accommodated by scheduling-clock tick every second
> or so.  On-going work will eliminate the need even for these
> infrequent scheduling-clock ticks.
>
> Here, "the occasional scheduling-clock tick" is the 1Hz tick that

May I know why we zeroed in on 1Hz? Is there any logical reason
or just because it is above 0Hz?
>
> Thanx, Paul
>
>> > managing how to schedule tasks according to CFS.
>> >
>> > Everything else shouldn't depend on the tick... period.
>> >
>> > -- Steve
>> >
>> >> > all our issues come from. In fact, we can not even completely remove the
>> >> > tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
>> >> > set to. We have to handle everything that depends on that tick, which
>> >> > includes perf, among other things.
>> >> >
>> >
>> >
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

Thanks Paul  Steve for replying.
On Fri, Jun 14, 2013 at 5:56 PM, Paul E. McKenney
paul...@linux.vnet.ibm.com wrote:
 On Fri, Jun 14, 2013 at 09:47:31AM +0530, anish singh wrote:
 On Thu, Jun 13, 2013 at 10:46 PM, Steven Rostedt rost...@goodmis.org wrote:
  On Thu, 2013-06-13 at 21:51 +0530, anish singh wrote:
 
   The concept behind full dynamic ticks is very easy. When you set a given
   CPU(s) to dynamic tick, when it only has a single task scheduled on that
   CPU, it disables the periodic tick. This removes essentially *all*
 
  Documentation/timers/highres.txt states that
  hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
  you mind elaborating single task scheduled on that CPU?
  I am bit new to this so please excuse me if the question is too basic.
 
  That's the old CONFIG_NO_HZ, which only disables the tick on idle. What
  we are working on is to also disable the tick when there's only one task
  running on a given CPU. Why have as schedule tick when there's nothing
  to schedule?
 
  3.10 now has new config options:
 
  CONFIG_NO_HZ_PERIODIC - which is NO_HZ disabled
 (the old # CONFIG_NO_HZ not set)
 
  CONFIG_NO_HZ_IDLE - which is what CONFIG_NO_HZ use to be.
 
  Note, CONFIG_NO_HZ still exists and if set, will make CONFIG_NO_HZ_IDLE
  the default. This was to keep the same configuration for people who
  update their kernel and run make oldconfig.
 
  Then there's the new:
 
  CONFIG_NO_HZ_FULL - this enables CONFIG_NO_HZ_IDLE plus adds the new
  feature with disabling the tick when only one task is running on a given
  CPU.

 Thanks and some more explanation in below documents.
 Documentation/timers/NO_HZ.txt
 Documentation/timers/highres.txt
 
 
   latency from the kernel! That is, if the task is doing some complex
   calculations, it wont be interrupted for kernel maintenance. A lot of
   Red Hat customers would love to have this feature. It allows for
   extremely low latency actions even without a real-time kernel. Heck, it
   works without even kernel preemption.
  
   Now removing the periodic tick is not a trivial task, and this is where
 
  You mean getting rid of period ticks in the kernel code is not easy as 
  there
  are many features such as perf is dependent on it right and that is why
  instead of completely removing periodic ticks we just set the periodic 
  tick
  to happen at 1HZ instead of CONFIG_HZ value?
 
  IIRC, the reason for moving to 1 HZ is so that the scheduler doesn't get
  confused with overflows. It still needs to handle time keeping for
 overflows meaning the tick happening at 1HZ?
 However as I see here in this patch http://lwn.net/Articles/549592/ -
 you have plans to move it to 0Hz then how does scheduler cope
 with this?Does it not need this information to schedule a different
 task when the current task on adaptive-ticks CPU is done?

 When the current task completed, it will enter the kernel, allowing
 the scheduler to take over.

 If a second task awakens or is created, there will either be some sort
 of interrupt to the CPU itself, or to some other CPU, and this other
 CPU will IPI the first CPU.  Either way, the scheduler gains control
 when it needs it -- but avoids continually interrupting the task.

 Anyway doesn't future works should be part of No-hz.txt document?

 It does, please see the very last bullet of the document:

 o   Some process-handling operations still require the occasional
 scheduling-clock tick.  These operations include calculating CPU
 load, maintaining sched average, computing CFS entity vruntime,
 computing avenrun, and carrying out load balancing.  They are
 currently accommodated by scheduling-clock tick every second
 or so.  On-going work will eliminate the need even for these
 infrequent scheduling-clock ticks.

 Here, the occasional scheduling-clock tick is the 1Hz tick that

May I know why we zeroed in on 1Hz? Is there any logical reason
or just because it is above 0Hz?

 Thanx, Paul

  managing how to schedule tasks according to CFS.
 
  Everything else shouldn't depend on the tick... period.
 
  -- Steve
 
   all our issues come from. In fact, we can not even completely remove the
   tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
   set to. We have to handle everything that depends on that tick, which
   includes perf, among other things.
  
 
 


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

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Fri, Jun 14, 2013 at 9:42 PM, Steven Rostedt rost...@goodmis.org wrote:
 On Fri, 2013-06-14 at 21:33 +0530, anish singh wrote:

 May I know why we zeroed in on 1Hz? Is there any logical reason
 or just because it is above 0Hz?
 

 We had to keep a tick. What number would you have picked?

Great.I will just confirming my understanding.Thanks.

 -- Steve


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

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Thu, Jun 13, 2013 at 10:46 PM, Steven Rostedt  wrote:
> On Thu, 2013-06-13 at 21:51 +0530, anish singh wrote:
>
>> > The concept behind full dynamic ticks is very easy. When you set a given
>> > CPU(s) to dynamic tick, when it only has a single task scheduled on that
>> > CPU, it disables the periodic tick. This removes essentially *all*
>>
>> Documentation/timers/highres.txt states that
>> hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
>> you mind elaborating "single task scheduled on that CPU"?
>> I am bit new to this so please excuse me if the question is too basic.
>
> That's the old CONFIG_NO_HZ, which only disables the tick on idle. What
> we are working on is to also disable the tick when there's only one task
> running on a given CPU. Why have as schedule tick when there's nothing
> to schedule?
>
> 3.10 now has new config options:
>
> CONFIG_NO_HZ_PERIODIC - which is NO_HZ disabled
>(the old # CONFIG_NO_HZ not set)
>
> CONFIG_NO_HZ_IDLE - which is what CONFIG_NO_HZ use to be.
>
> Note, CONFIG_NO_HZ still exists and if set, will make CONFIG_NO_HZ_IDLE
> the default. This was to keep the same configuration for people who
> update their kernel and run make oldconfig.
>
> Then there's the new:
>
> CONFIG_NO_HZ_FULL - this enables CONFIG_NO_HZ_IDLE plus adds the new
> feature with disabling the tick when only one task is running on a given
> CPU.

Thanks and some more explanation in below documents.
Documentation/timers/NO_HZ.txt
Documentation/timers/highres.txt
>
>
>> > latency from the kernel! That is, if the task is doing some complex
>> > calculations, it wont be interrupted for kernel maintenance. A lot of
>> > Red Hat customers would love to have this feature. It allows for
>> > extremely low latency actions even without a real-time kernel. Heck, it
>> > works without even kernel preemption.
>> >
>> > Now removing the periodic tick is not a trivial task, and this is where
>>
>> You mean getting rid of period ticks in the kernel code is not easy as there
>> are many features such as perf is dependent on it right and that is why
>> instead of completely removing periodic ticks we just set the periodic tick
>> to happen at 1HZ instead of CONFIG_HZ value?
>
> IIRC, the reason for moving to 1 HZ is so that the scheduler doesn't get
> confused with overflows. It still needs to handle time keeping for
"overflows" meaning the tick happening at 1HZ?
However as I see here in this patch http://lwn.net/Articles/549592/ -
you have plans to move it to 0Hz then how does scheduler cope
with this?Does it not need this information to schedule a different
task when the current task on "adaptive-ticks CPU" is done?

Anyway doesn't "future works" should be part of No-hz.txt document?
> managing how to schedule tasks according to CFS.
>
> Everything else shouldn't depend on the tick... period.
>
> -- Steve
>
>> > all our issues come from. In fact, we can not even completely remove the
>> > tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
>> > set to. We have to handle everything that depends on that tick, which
>> > includes perf, among other things.
>> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Thu, Jun 13, 2013 at 9:18 PM, Steven Rostedt  wrote:
> On Thu, 2013-06-13 at 11:20 -0400, Don Zickus wrote:
>
>> I don't know enough about how full dynticks work to even present a
>> solution.  But currently I was working with the Red Hat performance team
>> to enhance perf to help our customers diagnose performance problems
>> easier.
>>
>> My fear is anyone who uses full dynticks and has issues, can't use perf to
>> help diagnose their problems because it will change the dynamics of the
>> problem.  And with the current huge drop in performance in cpu_idle (as
>> compared to RHEL-6's 2.6.32 kernel) due to what seems to be miscalculated
>> c-states, one might have a hard time evaluating if full dynticks is doing
>> the right thing or not.
>
> This needs to be fixed, but not for 3.11. Although, you can still use
> ftrace to diagnose it.
>
>>
>> Then again perhaps full dynticks isn't useful for distros like RHEL.
>
> It will be very useful for RHEL. But its still very new, and I wouldn't
> recommend using it in a production environment yet. There's still a few
> issues that need to be worked out, including this one. When the issues
> are fixed, then RHEL and other distributions will definitely want to
> enable this.
>
>>
>> That's why I was hoping to solve the underlying problem as opposed to
>> accepting patches like this which work around the symptoms.
>
> For now it's just to get things working as people expect it to. First
> impressions are very important, and if someone enables it and sees it
> makes no difference, they may from then on never trust it. The way to
> handle that is to make sure it works when enabled, even if it disables
> some other cool features. But as I said, it shouldn't be used in
> production quite yet.
>
>>
>> Again, my knowledge of full dynticks is poor, so I have almost no idea of
>> the complexities surrounding the problem and how hard it is to even solve
>> it.
>
> The concept behind full dynamic ticks is very easy. When you set a given
> CPU(s) to dynamic tick, when it only has a single task scheduled on that
> CPU, it disables the periodic tick. This removes essentially *all*

Documentation/timers/highres.txt states that
hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
you mind elaborating "single task scheduled on that CPU"?
I am bit new to this so please excuse me if the question is too basic.
> latency from the kernel! That is, if the task is doing some complex
> calculations, it wont be interrupted for kernel maintenance. A lot of
> Red Hat customers would love to have this feature. It allows for
> extremely low latency actions even without a real-time kernel. Heck, it
> works without even kernel preemption.
>
> Now removing the periodic tick is not a trivial task, and this is where

You mean getting rid of period ticks in the kernel code is not easy as there
are many features such as perf is dependent on it right and that is why
instead of completely removing periodic ticks we just set the periodic tick
to happen at 1HZ instead of CONFIG_HZ value?
> all our issues come from. In fact, we can not even completely remove the
> tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
> set to. We have to handle everything that depends on that tick, which
> includes perf, among other things.
>
> -- Steve
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Thu, Jun 13, 2013 at 9:18 PM, Steven Rostedt rost...@goodmis.org wrote:
 On Thu, 2013-06-13 at 11:20 -0400, Don Zickus wrote:

 I don't know enough about how full dynticks work to even present a
 solution.  But currently I was working with the Red Hat performance team
 to enhance perf to help our customers diagnose performance problems
 easier.

 My fear is anyone who uses full dynticks and has issues, can't use perf to
 help diagnose their problems because it will change the dynamics of the
 problem.  And with the current huge drop in performance in cpu_idle (as
 compared to RHEL-6's 2.6.32 kernel) due to what seems to be miscalculated
 c-states, one might have a hard time evaluating if full dynticks is doing
 the right thing or not.

 This needs to be fixed, but not for 3.11. Although, you can still use
 ftrace to diagnose it.


 Then again perhaps full dynticks isn't useful for distros like RHEL.

 It will be very useful for RHEL. But its still very new, and I wouldn't
 recommend using it in a production environment yet. There's still a few
 issues that need to be worked out, including this one. When the issues
 are fixed, then RHEL and other distributions will definitely want to
 enable this.


 That's why I was hoping to solve the underlying problem as opposed to
 accepting patches like this which work around the symptoms.

 For now it's just to get things working as people expect it to. First
 impressions are very important, and if someone enables it and sees it
 makes no difference, they may from then on never trust it. The way to
 handle that is to make sure it works when enabled, even if it disables
 some other cool features. But as I said, it shouldn't be used in
 production quite yet.


 Again, my knowledge of full dynticks is poor, so I have almost no idea of
 the complexities surrounding the problem and how hard it is to even solve
 it.

 The concept behind full dynamic ticks is very easy. When you set a given
 CPU(s) to dynamic tick, when it only has a single task scheduled on that
 CPU, it disables the periodic tick. This removes essentially *all*

Documentation/timers/highres.txt states that
hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
you mind elaborating single task scheduled on that CPU?
I am bit new to this so please excuse me if the question is too basic.
 latency from the kernel! That is, if the task is doing some complex
 calculations, it wont be interrupted for kernel maintenance. A lot of
 Red Hat customers would love to have this feature. It allows for
 extremely low latency actions even without a real-time kernel. Heck, it
 works without even kernel preemption.

 Now removing the periodic tick is not a trivial task, and this is where

You mean getting rid of period ticks in the kernel code is not easy as there
are many features such as perf is dependent on it right and that is why
instead of completely removing periodic ticks we just set the periodic tick
to happen at 1HZ instead of CONFIG_HZ value?
 all our issues come from. In fact, we can not even completely remove the
 tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
 set to. We have to handle everything that depends on that tick, which
 includes perf, among other things.

 -- Steve


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

Re: [PATCH 4/6] watchdog: Boot-disable by default on full dynticks

[anish singh]

On Thu, Jun 13, 2013 at 10:46 PM, Steven Rostedt rost...@goodmis.org wrote:
 On Thu, 2013-06-13 at 21:51 +0530, anish singh wrote:

  The concept behind full dynamic ticks is very easy. When you set a given
  CPU(s) to dynamic tick, when it only has a single task scheduled on that
  CPU, it disables the periodic tick. This removes essentially *all*

 Documentation/timers/highres.txt states that
 hrtimer_stop_sched_tick() is called when a CPU goes into idle state.Would
 you mind elaborating single task scheduled on that CPU?
 I am bit new to this so please excuse me if the question is too basic.

 That's the old CONFIG_NO_HZ, which only disables the tick on idle. What
 we are working on is to also disable the tick when there's only one task
 running on a given CPU. Why have as schedule tick when there's nothing
 to schedule?

 3.10 now has new config options:

 CONFIG_NO_HZ_PERIODIC - which is NO_HZ disabled
(the old # CONFIG_NO_HZ not set)

 CONFIG_NO_HZ_IDLE - which is what CONFIG_NO_HZ use to be.

 Note, CONFIG_NO_HZ still exists and if set, will make CONFIG_NO_HZ_IDLE
 the default. This was to keep the same configuration for people who
 update their kernel and run make oldconfig.

 Then there's the new:

 CONFIG_NO_HZ_FULL - this enables CONFIG_NO_HZ_IDLE plus adds the new
 feature with disabling the tick when only one task is running on a given
 CPU.

Thanks and some more explanation in below documents.
Documentation/timers/NO_HZ.txt
Documentation/timers/highres.txt


  latency from the kernel! That is, if the task is doing some complex
  calculations, it wont be interrupted for kernel maintenance. A lot of
  Red Hat customers would love to have this feature. It allows for
  extremely low latency actions even without a real-time kernel. Heck, it
  works without even kernel preemption.
 
  Now removing the periodic tick is not a trivial task, and this is where

 You mean getting rid of period ticks in the kernel code is not easy as there
 are many features such as perf is dependent on it right and that is why
 instead of completely removing periodic ticks we just set the periodic tick
 to happen at 1HZ instead of CONFIG_HZ value?

 IIRC, the reason for moving to 1 HZ is so that the scheduler doesn't get
 confused with overflows. It still needs to handle time keeping for
overflows meaning the tick happening at 1HZ?
However as I see here in this patch http://lwn.net/Articles/549592/ -
you have plans to move it to 0Hz then how does scheduler cope
with this?Does it not need this information to schedule a different
task when the current task on adaptive-ticks CPU is done?

Anyway doesn't future works should be part of No-hz.txt document?
 managing how to schedule tasks according to CFS.

 Everything else shouldn't depend on the tick... period.

 -- Steve

  all our issues come from. In fact, we can not even completely remove the
  tick yet, we just move it to 1 HZ instead of whatever the CONFIG_HZ is
  set to. We have to handle everything that depends on that tick, which
  includes perf, among other things.
 


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

Re: [PATCH] extcon: Add an API to get extcon device from dt node

[anish singh]

On Wed, Jun 12, 2013 at 7:09 AM, Chanwoo Choi  wrote:
> From: Kishon Vijay Abraham I 
>
> Added an API of_extcon_get_extcon_dev() to be used by drivers to get
> extcon device in the case of dt boot (this can be used instead of
> extcon_get_extcon_dev()).
>
> Signed-off-by: Kishon Vijay Abraham I 
> Signed-off-by: Chanwoo Choi 
> Signed-off-by: Myungjoo Ham 
> ---
>  drivers/extcon/Makefile  |  3 +++
>  drivers/extcon/of-extcon.c   | 56 
> 
>  include/linux/extcon/of-extcon.h | 30 +
>  3 files changed, 89 insertions(+)
>  create mode 100644 drivers/extcon/of-extcon.c
>  create mode 100644 include/linux/extcon/of-extcon.h
>
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 540e2c3..39cdf95 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -2,9 +2,12 @@
>  # Makefile for external connector class (extcon) devices
>  #
>
> +obj-$(CONFIG_OF)   += of-extcon.o
> +
>  obj-$(CONFIG_EXTCON)   += extcon-class.o
>  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
>  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> +
>  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX8997)   += extcon-max8997.o
>  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> diff --git a/drivers/extcon/of-extcon.c b/drivers/extcon/of-extcon.c
> new file mode 100644
> index 000..29f82b4
> --- /dev/null
> +++ b/drivers/extcon/of-extcon.c
> @@ -0,0 +1,56 @@
> +/*
> + * OF helpers for External connector (extcon) framework
> + *
> + * Copyright (C) 2013 Texas Instruments, Inc.
> + * Kishon Vijay Abraham I 
> + *
> + * Copyright (C) 2013 Samsung Electronics
> + * Chanwoo Choi 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct extcon_dev *of_extcon_get_extcon_dev(struct device *dev, int index)
Can this be simpler name? of_extcon_get_dev or something like that?
> +{
> +   struct device_node *node;
> +   struct extcon_dev *edev;
> +   struct platform_device *extcon_parent_dev;
> +
> +   if (!dev->of_node) {
> +   dev_dbg(dev, "device does not have a device node entry\n");
> +   return ERR_PTR(-EINVAL);
> +   }
> +
> +   node = of_parse_phandle(dev->of_node, "extcon", index);
> +   if (!node) {
> +   dev_dbg(dev, "failed to get phandle in %s node\n",
> +   dev->of_node->full_name);
> +   return ERR_PTR(-ENODEV);
> +   }
> +
> +   extcon_parent_dev = of_find_device_by_node(node);
> +   if (!extcon_parent_dev) {
> +   dev_dbg(dev, "unable to find device by node\n");
> +   return ERR_PTR(-EPROBE_DEFER);
> +   }
> +
> +   edev = extcon_get_extcon_dev(dev_name(_parent_dev->dev));
> +   if (!edev) {
> +   dev_dbg(dev, "unable to get extcon device : %s\n",
> +   dev_name(_parent_dev->dev));
> +   return ERR_PTR(-ENODEV);
> +   }
> +
> +   return edev;
> +}
> +EXPORT_SYMBOL_GPL(of_extcon_get_extcon_dev);
> diff --git a/include/linux/extcon/of-extcon.h 
> b/include/linux/extcon/of-extcon.h
> new file mode 100644
> index 000..77d01ba
> --- /dev/null
> +++ b/include/linux/extcon/of-extcon.h
> @@ -0,0 +1,30 @@
> +/*
> + * OF helpers for External connector (extcon) framework
> + *
> + * Copyright (C) 2013 Texas Instruments, Inc.
> + * Kishon Vijay Abraham I 
> + *
> + * Copyright (C) 2013 Samsung Electronics
> + * Chanwoo Choi 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __LINUX_OF_EXTCON_H
> +#define __LINUX_OF_EXTCON_H
> +
> +#if defined(CONFIG_OF)
> +extern struct extcon_dev
> +   *of_extcon_get_extcon_dev(struct device *dev, int index);
> +#else
> +static inline struct extcon_dev
> +   *of_extcon_get_extcon_dev(struct device *dev, int index);
> +{
> +   return NULL;
> +}
> +#endif /* CONFIG_OF */
> +
> +#endif /* __LINUX_OF_EXTCON_H */
> --
> 1.8.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] extcon: Add an API to get extcon device from dt node

[anish singh]

On Wed, Jun 12, 2013 at 7:09 AM, Chanwoo Choi cw00.c...@samsung.com wrote:
 From: Kishon Vijay Abraham I kis...@ti.com

 Added an API of_extcon_get_extcon_dev() to be used by drivers to get
 extcon device in the case of dt boot (this can be used instead of
 extcon_get_extcon_dev()).

 Signed-off-by: Kishon Vijay Abraham I kis...@ti.com
 Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
 Signed-off-by: Myungjoo Ham myungjoo@samsung.com
 ---
  drivers/extcon/Makefile  |  3 +++
  drivers/extcon/of-extcon.c   | 56 
 
  include/linux/extcon/of-extcon.h | 30 +
  3 files changed, 89 insertions(+)
  create mode 100644 drivers/extcon/of-extcon.c
  create mode 100644 include/linux/extcon/of-extcon.h

 diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
 index 540e2c3..39cdf95 100644
 --- a/drivers/extcon/Makefile
 +++ b/drivers/extcon/Makefile
 @@ -2,9 +2,12 @@
  # Makefile for external connector class (extcon) devices
  #

 +obj-$(CONFIG_OF)   += of-extcon.o
 +
  obj-$(CONFIG_EXTCON)   += extcon-class.o
  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
 +
  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
  obj-$(CONFIG_EXTCON_MAX8997)   += extcon-max8997.o
  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
 diff --git a/drivers/extcon/of-extcon.c b/drivers/extcon/of-extcon.c
 new file mode 100644
 index 000..29f82b4
 --- /dev/null
 +++ b/drivers/extcon/of-extcon.c
 @@ -0,0 +1,56 @@
 +/*
 + * OF helpers for External connector (extcon) framework
 + *
 + * Copyright (C) 2013 Texas Instruments, Inc.
 + * Kishon Vijay Abraham I kis...@ti.com
 + *
 + * Copyright (C) 2013 Samsung Electronics
 + * Chanwoo Choi cw00.c...@samsung.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + */
 +
 +#include linux/module.h
 +#include linux/slab.h
 +#include linux/extcon.h
 +#include linux/of.h
 +#include linux/of_platform.h
 +#include linux/extcon/of-extcon.h
 +
 +struct extcon_dev *of_extcon_get_extcon_dev(struct device *dev, int index)
Can this be simpler name? of_extcon_get_dev or something like that?
 +{
 +   struct device_node *node;
 +   struct extcon_dev *edev;
 +   struct platform_device *extcon_parent_dev;
 +
 +   if (!dev-of_node) {
 +   dev_dbg(dev, device does not have a device node entry\n);
 +   return ERR_PTR(-EINVAL);
 +   }
 +
 +   node = of_parse_phandle(dev-of_node, extcon, index);
 +   if (!node) {
 +   dev_dbg(dev, failed to get phandle in %s node\n,
 +   dev-of_node-full_name);
 +   return ERR_PTR(-ENODEV);
 +   }
 +
 +   extcon_parent_dev = of_find_device_by_node(node);
 +   if (!extcon_parent_dev) {
 +   dev_dbg(dev, unable to find device by node\n);
 +   return ERR_PTR(-EPROBE_DEFER);
 +   }
 +
 +   edev = extcon_get_extcon_dev(dev_name(extcon_parent_dev-dev));
 +   if (!edev) {
 +   dev_dbg(dev, unable to get extcon device : %s\n,
 +   dev_name(extcon_parent_dev-dev));
 +   return ERR_PTR(-ENODEV);
 +   }
 +
 +   return edev;
 +}
 +EXPORT_SYMBOL_GPL(of_extcon_get_extcon_dev);
 diff --git a/include/linux/extcon/of-extcon.h 
 b/include/linux/extcon/of-extcon.h
 new file mode 100644
 index 000..77d01ba
 --- /dev/null
 +++ b/include/linux/extcon/of-extcon.h
 @@ -0,0 +1,30 @@
 +/*
 + * OF helpers for External connector (extcon) framework
 + *
 + * Copyright (C) 2013 Texas Instruments, Inc.
 + * Kishon Vijay Abraham I kis...@ti.com
 + *
 + * Copyright (C) 2013 Samsung Electronics
 + * Chanwoo Choi cw00.c...@samsung.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + */
 +
 +#ifndef __LINUX_OF_EXTCON_H
 +#define __LINUX_OF_EXTCON_H
 +
 +#if defined(CONFIG_OF)
 +extern struct extcon_dev
 +   *of_extcon_get_extcon_dev(struct device *dev, int index);
 +#else
 +static inline struct extcon_dev
 +   *of_extcon_get_extcon_dev(struct device *dev, int index);
 +{
 +   return NULL;
 +}
 +#endif /* CONFIG_OF */
 +
 +#endif /* __LINUX_OF_EXTCON_H */
 --
 1.8.0

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

Re: [PATCHv2 4/6] sched_clock: Add support for >32 bit sched_clock

[anish singh]

On Mon, Jun 10, 2013 at 9:08 PM, Russell King - ARM Linux
 wrote:

Least I can do is to say "Thanks".
> On Mon, Jun 10, 2013 at 08:46:36PM +0530, anish singh wrote:
>> Probably a trivial question.I was wondering why this particular requirement
>> exists in the first place.I looked into this commit 112f38a4a3 but couldn't
>> gather the reason.
>
> You're looking at a commit introducing an implementation.  The requirement
> isn't driven by the implementation.  It's driven by the code and the maths
> in the core scheduler, and its been a requirement for years.
>
> sched_clock() needs to be monotonic, and needs to wrap at 64-bit, because
> calculations are done by comparing the difference of two 64-bit values
> returned from this function.

Yes, and this is the question.If it is 32 bit then also it can overflow but
it will happen relatively fast.So I guess that is the reason why we use 64 bit
and this will avoid recalculations for recalibration.
>
> Let's take a trivial example - if you have a 16 bit counter, and you have
> a value of 0xc000 ns, and next time you read it, it has value 0x0001 ns,
> then what value do you end up with when you calculate the time passed
> using 64-bit maths.
>
> That's 0x0001 - 0xc000.  The answer is a very big
> number which is not the correct 16385.  This means that things like process
> timeslice counting and scheduler fairness is compromised - I'd expect even

So you mean when counter overflows the scheduler doesn't handle it?
> more so if you're running RT and this is being used to provide guarantees.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 4/6] sched_clock: Add support for >32 bit sched_clock

[anish singh]

On Tue, Jun 4, 2013 at 3:42 AM, Russell King - ARM Linux
 wrote:
> On Mon, Jun 03, 2013 at 02:11:59PM -0700, Stephen Boyd wrote:
>> On 06/03/13 02:39, Russell King - ARM Linux wrote:
>> > On Sat, Jun 01, 2013 at 11:39:41PM -0700, Stephen Boyd wrote:
>> >> +}
>> >> +
>> >> +void __init
>> >> +setup_sched_clock_64(u64 (*read)(void), int bits, unsigned long rate)
>> >> +{
>> >> +  if (cd.rate > rate)
>> >> +  return;
>> >> +
>> >> +  BUG_ON(bits <= 32);
>> >> +  WARN_ON(!irqs_disabled());
>> >> +  read_sched_clock_64 = read;
>> >> +  sched_clock_func = sched_clock_64;
>> >> +  cd.rate = rate;
>> >> +  cd.mult = NSEC_PER_SEC / rate;
>> > Here, you don't check that the (2^bits) * mult results in a wrap of the
>> > resulting 64-bit number, which is a _basic_ requirement for sched_clock
>> > (hence all the code for <=32bit clocks, otherwise we wouldn't need this
>> > complexity in the first place.)
>>
>> Ok I will use clocks_calc_mult_shift() here.
>
> No, that's not the problem.
>
> If you have a 56-bit clock which ticks at a period of 1ns, then
> cd.rate = 1, and your sched_clock() values will be truncated to 56-bits.
> The scheduler always _requires_ 64-bits from sched_clock.  That's why we
> have the complicated code to extend the 32-bits-or-less to a _full_
> 64-bit value.
>
> Let me make this clearer: sched_clock() return values _must_ without
> exception monotonically increment from zero to 2^64-1 and then wrap
> back to zero.  No other behaviour is acceptable for sched_clock().

Probably a trivial question.I was wondering why this particular requirement
exists in the first place.I looked into this commit 112f38a4a3 but couldn't
gather the reason.

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

Re: [PATCHv2 4/6] sched_clock: Add support for 32 bit sched_clock

[anish singh]

On Tue, Jun 4, 2013 at 3:42 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Mon, Jun 03, 2013 at 02:11:59PM -0700, Stephen Boyd wrote:
 On 06/03/13 02:39, Russell King - ARM Linux wrote:
  On Sat, Jun 01, 2013 at 11:39:41PM -0700, Stephen Boyd wrote:
  +}
  +
  +void __init
  +setup_sched_clock_64(u64 (*read)(void), int bits, unsigned long rate)
  +{
  +  if (cd.rate  rate)
  +  return;
  +
  +  BUG_ON(bits = 32);
  +  WARN_ON(!irqs_disabled());
  +  read_sched_clock_64 = read;
  +  sched_clock_func = sched_clock_64;
  +  cd.rate = rate;
  +  cd.mult = NSEC_PER_SEC / rate;
  Here, you don't check that the (2^bits) * mult results in a wrap of the
  resulting 64-bit number, which is a _basic_ requirement for sched_clock
  (hence all the code for =32bit clocks, otherwise we wouldn't need this
  complexity in the first place.)

 Ok I will use clocks_calc_mult_shift() here.

 No, that's not the problem.

 If you have a 56-bit clock which ticks at a period of 1ns, then
 cd.rate = 1, and your sched_clock() values will be truncated to 56-bits.
 The scheduler always _requires_ 64-bits from sched_clock.  That's why we
 have the complicated code to extend the 32-bits-or-less to a _full_
 64-bit value.

 Let me make this clearer: sched_clock() return values _must_ without
 exception monotonically increment from zero to 2^64-1 and then wrap
 back to zero.  No other behaviour is acceptable for sched_clock().

Probably a trivial question.I was wondering why this particular requirement
exists in the first place.I looked into this commit 112f38a4a3 but couldn't
gather the reason.

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

Re: [PATCHv2 4/6] sched_clock: Add support for 32 bit sched_clock

[anish singh]

On Mon, Jun 10, 2013 at 9:08 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:

Least I can do is to say Thanks.
 On Mon, Jun 10, 2013 at 08:46:36PM +0530, anish singh wrote:
 Probably a trivial question.I was wondering why this particular requirement
 exists in the first place.I looked into this commit 112f38a4a3 but couldn't
 gather the reason.

 You're looking at a commit introducing an implementation.  The requirement
 isn't driven by the implementation.  It's driven by the code and the maths
 in the core scheduler, and its been a requirement for years.

 sched_clock() needs to be monotonic, and needs to wrap at 64-bit, because
 calculations are done by comparing the difference of two 64-bit values
 returned from this function.

Yes, and this is the question.If it is 32 bit then also it can overflow but
it will happen relatively fast.So I guess that is the reason why we use 64 bit
and this will avoid recalculations for recalibration.

 Let's take a trivial example - if you have a 16 bit counter, and you have
 a value of 0xc000 ns, and next time you read it, it has value 0x0001 ns,
 then what value do you end up with when you calculate the time passed
 using 64-bit maths.

 That's 0x0001 - 0xc000.  The answer is a very big
 number which is not the correct 16385.  This means that things like process
 timeslice counting and scheduler fairness is compromised - I'd expect even

So you mean when counter overflows the scheduler doesn't handle it?
 more so if you're running RT and this is being used to provide guarantees.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [checkpatch] - Confusion

[anish singh]

On Mon, Jun 10, 2013 at 11:21 AM, PINTU KUMAR  wrote:
> Hi,
>
> I wanted to submit my first patch.
> But I have some confusion about the /scripts/checkpatch.pl errors.
>
> After correcting some checkpatch errors, when I run checkpatch.pl, it showed 
> me 0 errors.
> But when I create patches are git format-patch, it is showing me 1 error.
did  you run the checkpatch.pl on the file which gets created
after git format-patch?
If yes, then I think it is not necessary.You can use git-am to apply
your own patch on a undisturbed file and if it applies properly then
you are good to go i.e. you can send your patch.
>
> If I fix error in patch, it showed me back again in files.
>
> Now, I am confused which error to fix while submitting patches, the file or 
> the patch errors.
>
> Please provide your opinion.
>
> File: mm/page_alloc.c
> Previous file errors:
> total: 16 errors, 110 warnings, 6255 lines checked
>
> After fixing errors:
> total: 0 errors, 105 warnings, 6255 lines checked
>
>
> And, after running on patch:
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #153: FILE: mm/page_alloc.c:5476:
> +int min_free_kbytes_sysctl_handler(ctl_table *table, int write,
>
>
>
>
> - Pintu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [checkpatch] - Confusion

[anish singh]

On Mon, Jun 10, 2013 at 11:21 AM, PINTU KUMAR pintu_agar...@yahoo.com wrote:
 Hi,

 I wanted to submit my first patch.
 But I have some confusion about the /scripts/checkpatch.pl errors.

 After correcting some checkpatch errors, when I run checkpatch.pl, it showed 
 me 0 errors.
 But when I create patches are git format-patch, it is showing me 1 error.
did  you run the checkpatch.pl on the file which gets created
after git format-patch?
If yes, then I think it is not necessary.You can use git-am to apply
your own patch on a undisturbed file and if it applies properly then
you are good to go i.e. you can send your patch.

 If I fix error in patch, it showed me back again in files.

 Now, I am confused which error to fix while submitting patches, the file or 
 the patch errors.

 Please provide your opinion.

 File: mm/page_alloc.c
 Previous file errors:
 total: 16 errors, 110 warnings, 6255 lines checked

 After fixing errors:
 total: 0 errors, 105 warnings, 6255 lines checked


 And, after running on patch:
 ERROR: need consistent spacing around '*' (ctx:WxV)
 #153: FILE: mm/page_alloc.c:5476:
 +int min_free_kbytes_sysctl_handler(ctl_table *table, int write,




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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Thu, Jun 6, 2013 at 10:11 AM, Guenter Roeck  wrote:
> On Thu, Jun 06, 2013 at 08:30:01AM +0530, anish singh wrote:
>> Hello Wim Van,
>> Can you look into below?
>>
> Please be patient. Wim tends to be busy.
Sorry, I will wait.
>
> Guenter
>
>> On Wed, Jun 5, 2013 at 8:39 AM, anish singh  
>> wrote:
>> > Hello Wim Van Sabroeck,
>> > Can I get your inputs on this?
>> >
>> > On Tue, Jun 4, 2013 at 8:39 AM, anish singh  
>> > wrote:
>> >> On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck  wrote:
>> >>> On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
>> >>>> On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck  
>> >>>> wrote:
>> >>>> > On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
>> >>>> >> Certain watchdog drivers use a timer to keep kicking the watchdog at
>> >>>> >> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>> >>>> >> we can't guarantee that watchdog will be pinged fast enough
>> >>>> >> for all system loads, especially if timeout is configured for
>> >>>> >> less than or equal to 1 second(basically small values).
>> >>>> >>
>> >>>> >> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>> >>>> >> add this functionality of individual watchdog drivers in the core
>> >>>> >> watchdog core.
>> >>>> >>
>> >>>> >> Signed-off-by: anish kumar 
>> >>>> >
>> >>>> > Not exactly what I had in mind. My idea was to enable the softdog 
>> >>>> > only if
>> >>>> > the hardware watchdog's maximum timeout was low (say, less than a 
>> >>>> > couple
>> >>>> > of minutes), and if a timeout larger than its maximum value was 
>> >>>> > configured.
>> >>>>
>> >>>> watchdog_timeout_invalid wouldn't this check will fail if the user 
>> >>>> space tries
>> >>>> to set maximum timeout more that what driver can support?It would work
>> >>>> for pika_wdt.c as it is old watchdog driver and doesn't register with 
>> >>>> watchdog
>> >>>> framwork but new drivers has to pass this api.
>> >>>>
>> >>>> OR
>> >>>>
>> >>>> Do you want to remove this check and go as explained by you?I would
>> >>>> favour this approach though.
>> >>>>
>> >>> One would still have a check, but the enforced limits would no longer be
>> >>> the driver limits, but larger limits implemented in the watchdog core.
>> >> How much larger would be the big question here?Should it be configurable
>> >> property(sysfs?) or some hardcoding based on existing drivers?
>> >>
>> >> Before going for next patch, it would be better for me to wait for some
>> >> more comments.
>> >>>
>> >>>> > In that case, I would have set the hardware watchdog to its maximum 
>> >>>> > value
>> >>>> > and use the softdog to ping it at a rate of, say, 50% of this maximum.
>> >>>> >
>> >>>> > If userspace would not ping the watchdog within its configured value,
>> >>>> > I would stop pinging the hardware watchdog and let it time out.
>> >>>>
>> >>>> One more question.Why is the return value of watchdog_ping int? Anyway
>> >>>> we discard it.
>> >>>
>> >>> I can not answer that question.
>> >>>
>> >>> Guenter
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Thu, Jun 6, 2013 at 10:11 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Thu, Jun 06, 2013 at 08:30:01AM +0530, anish singh wrote:
 Hello Wim Van,
 Can you look into below?

 Please be patient. Wim tends to be busy.
Sorry, I will wait.

 Guenter

 On Wed, Jun 5, 2013 at 8:39 AM, anish singh anish198519851...@gmail.com 
 wrote:
  Hello Wim Van Sabroeck,
  Can I get your inputs on this?
 
  On Tue, Jun 4, 2013 at 8:39 AM, anish singh anish198519851...@gmail.com 
  wrote:
  On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck li...@roeck-us.net wrote:
  On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
  On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck li...@roeck-us.net 
  wrote:
   On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
   Certain watchdog drivers use a timer to keep kicking the watchdog at
   a rate of 0.5s (HZ/2) untill userspace times out.They do this as
   we can't guarantee that watchdog will be pinged fast enough
   for all system loads, especially if timeout is configured for
   less than or equal to 1 second(basically small values).
  
   As suggested by Wim Van Sebroeck  Guenter Roeck we should
   add this functionality of individual watchdog drivers in the core
   watchdog core.
  
   Signed-off-by: anish kumar anish198519851...@gmail.com
  
   Not exactly what I had in mind. My idea was to enable the softdog 
   only if
   the hardware watchdog's maximum timeout was low (say, less than a 
   couple
   of minutes), and if a timeout larger than its maximum value was 
   configured.
 
  watchdog_timeout_invalid wouldn't this check will fail if the user 
  space tries
  to set maximum timeout more that what driver can support?It would work
  for pika_wdt.c as it is old watchdog driver and doesn't register with 
  watchdog
  framwork but new drivers has to pass this api.
 
  OR
 
  Do you want to remove this check and go as explained by you?I would
  favour this approach though.
 
  One would still have a check, but the enforced limits would no longer be
  the driver limits, but larger limits implemented in the watchdog core.
  How much larger would be the big question here?Should it be configurable
  property(sysfs?) or some hardcoding based on existing drivers?
 
  Before going for next patch, it would be better for me to wait for some
  more comments.
 
   In that case, I would have set the hardware watchdog to its maximum 
   value
   and use the softdog to ping it at a rate of, say, 50% of this maximum.
  
   If userspace would not ping the watchdog within its configured value,
   I would stop pinging the hardware watchdog and let it time out.
 
  One more question.Why is the return value of watchdog_ping int? Anyway
  we discard it.
 
  I can not answer that question.
 
  Guenter
 --
 To unsubscribe from this list: send the line unsubscribe linux-watchdog in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

Hello Wim Van,
Can you look into below?

On Wed, Jun 5, 2013 at 8:39 AM, anish singh  wrote:
> Hello Wim Van Sabroeck,
> Can I get your inputs on this?
>
> On Tue, Jun 4, 2013 at 8:39 AM, anish singh  
> wrote:
>> On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck  wrote:
>>> On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
>>>> On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck  wrote:
>>>> > On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
>>>> >> Certain watchdog drivers use a timer to keep kicking the watchdog at
>>>> >> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>>>> >> we can't guarantee that watchdog will be pinged fast enough
>>>> >> for all system loads, especially if timeout is configured for
>>>> >> less than or equal to 1 second(basically small values).
>>>> >>
>>>> >> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>>>> >> add this functionality of individual watchdog drivers in the core
>>>> >> watchdog core.
>>>> >>
>>>> >> Signed-off-by: anish kumar 
>>>> >
>>>> > Not exactly what I had in mind. My idea was to enable the softdog only if
>>>> > the hardware watchdog's maximum timeout was low (say, less than a couple
>>>> > of minutes), and if a timeout larger than its maximum value was 
>>>> > configured.
>>>>
>>>> watchdog_timeout_invalid wouldn't this check will fail if the user space 
>>>> tries
>>>> to set maximum timeout more that what driver can support?It would work
>>>> for pika_wdt.c as it is old watchdog driver and doesn't register with 
>>>> watchdog
>>>> framwork but new drivers has to pass this api.
>>>>
>>>> OR
>>>>
>>>> Do you want to remove this check and go as explained by you?I would
>>>> favour this approach though.
>>>>
>>> One would still have a check, but the enforced limits would no longer be
>>> the driver limits, but larger limits implemented in the watchdog core.
>> How much larger would be the big question here?Should it be configurable
>> property(sysfs?) or some hardcoding based on existing drivers?
>>
>> Before going for next patch, it would be better for me to wait for some
>> more comments.
>>>
>>>> > In that case, I would have set the hardware watchdog to its maximum value
>>>> > and use the softdog to ping it at a rate of, say, 50% of this maximum.
>>>> >
>>>> > If userspace would not ping the watchdog within its configured value,
>>>> > I would stop pinging the hardware watchdog and let it time out.
>>>>
>>>> One more question.Why is the return value of watchdog_ping int? Anyway
>>>> we discard it.
>>>
>>> I can not answer that question.
>>>
>>> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

Hello Wim Van,
Can you look into below?

On Wed, Jun 5, 2013 at 8:39 AM, anish singh anish198519851...@gmail.com wrote:
 Hello Wim Van Sabroeck,
 Can I get your inputs on this?

 On Tue, Jun 4, 2013 at 8:39 AM, anish singh anish198519851...@gmail.com 
 wrote:
 On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
 On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
  Certain watchdog drivers use a timer to keep kicking the watchdog at
  a rate of 0.5s (HZ/2) untill userspace times out.They do this as
  we can't guarantee that watchdog will be pinged fast enough
  for all system loads, especially if timeout is configured for
  less than or equal to 1 second(basically small values).
 
  As suggested by Wim Van Sebroeck  Guenter Roeck we should
  add this functionality of individual watchdog drivers in the core
  watchdog core.
 
  Signed-off-by: anish kumar anish198519851...@gmail.com
 
  Not exactly what I had in mind. My idea was to enable the softdog only if
  the hardware watchdog's maximum timeout was low (say, less than a couple
  of minutes), and if a timeout larger than its maximum value was 
  configured.

 watchdog_timeout_invalid wouldn't this check will fail if the user space 
 tries
 to set maximum timeout more that what driver can support?It would work
 for pika_wdt.c as it is old watchdog driver and doesn't register with 
 watchdog
 framwork but new drivers has to pass this api.

 OR

 Do you want to remove this check and go as explained by you?I would
 favour this approach though.

 One would still have a check, but the enforced limits would no longer be
 the driver limits, but larger limits implemented in the watchdog core.
 How much larger would be the big question here?Should it be configurable
 property(sysfs?) or some hardcoding based on existing drivers?

 Before going for next patch, it would be better for me to wait for some
 more comments.

  In that case, I would have set the hardware watchdog to its maximum value
  and use the softdog to ping it at a rate of, say, 50% of this maximum.
 
  If userspace would not ping the watchdog within its configured value,
  I would stop pinging the hardware watchdog and let it time out.

 One more question.Why is the return value of watchdog_ping int? Anyway
 we discard it.

 I can not answer that question.

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

Hello Wim Van Sabroeck,
Can I get your inputs on this?

On Tue, Jun 4, 2013 at 8:39 AM, anish singh  wrote:
> On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck  wrote:
>> On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
>>> On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck  wrote:
>>> > On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
>>> >> Certain watchdog drivers use a timer to keep kicking the watchdog at
>>> >> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>>> >> we can't guarantee that watchdog will be pinged fast enough
>>> >> for all system loads, especially if timeout is configured for
>>> >> less than or equal to 1 second(basically small values).
>>> >>
>>> >> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>>> >> add this functionality of individual watchdog drivers in the core
>>> >> watchdog core.
>>> >>
>>> >> Signed-off-by: anish kumar 
>>> >
>>> > Not exactly what I had in mind. My idea was to enable the softdog only if
>>> > the hardware watchdog's maximum timeout was low (say, less than a couple
>>> > of minutes), and if a timeout larger than its maximum value was 
>>> > configured.
>>>
>>> watchdog_timeout_invalid wouldn't this check will fail if the user space 
>>> tries
>>> to set maximum timeout more that what driver can support?It would work
>>> for pika_wdt.c as it is old watchdog driver and doesn't register with 
>>> watchdog
>>> framwork but new drivers has to pass this api.
>>>
>>> OR
>>>
>>> Do you want to remove this check and go as explained by you?I would
>>> favour this approach though.
>>>
>> One would still have a check, but the enforced limits would no longer be
>> the driver limits, but larger limits implemented in the watchdog core.
> How much larger would be the big question here?Should it be configurable
> property(sysfs?) or some hardcoding based on existing drivers?
>
> Before going for next patch, it would be better for me to wait for some
> more comments.
>>
>>> > In that case, I would have set the hardware watchdog to its maximum value
>>> > and use the softdog to ping it at a rate of, say, 50% of this maximum.
>>> >
>>> > If userspace would not ping the watchdog within its configured value,
>>> > I would stop pinging the hardware watchdog and let it time out.
>>>
>>> One more question.Why is the return value of watchdog_ping int? Anyway
>>> we discard it.
>>
>> I can not answer that question.
>>
>> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

Hello Wim Van Sabroeck,
Can I get your inputs on this?

On Tue, Jun 4, 2013 at 8:39 AM, anish singh anish198519851...@gmail.com wrote:
 On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
 On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
  Certain watchdog drivers use a timer to keep kicking the watchdog at
  a rate of 0.5s (HZ/2) untill userspace times out.They do this as
  we can't guarantee that watchdog will be pinged fast enough
  for all system loads, especially if timeout is configured for
  less than or equal to 1 second(basically small values).
 
  As suggested by Wim Van Sebroeck  Guenter Roeck we should
  add this functionality of individual watchdog drivers in the core
  watchdog core.
 
  Signed-off-by: anish kumar anish198519851...@gmail.com
 
  Not exactly what I had in mind. My idea was to enable the softdog only if
  the hardware watchdog's maximum timeout was low (say, less than a couple
  of minutes), and if a timeout larger than its maximum value was 
  configured.

 watchdog_timeout_invalid wouldn't this check will fail if the user space 
 tries
 to set maximum timeout more that what driver can support?It would work
 for pika_wdt.c as it is old watchdog driver and doesn't register with 
 watchdog
 framwork but new drivers has to pass this api.

 OR

 Do you want to remove this check and go as explained by you?I would
 favour this approach though.

 One would still have a check, but the enforced limits would no longer be
 the driver limits, but larger limits implemented in the watchdog core.
 How much larger would be the big question here?Should it be configurable
 property(sysfs?) or some hardcoding based on existing drivers?

 Before going for next patch, it would be better for me to wait for some
 more comments.

  In that case, I would have set the hardware watchdog to its maximum value
  and use the softdog to ping it at a rate of, say, 50% of this maximum.
 
  If userspace would not ping the watchdog within its configured value,
  I would stop pinging the hardware watchdog and let it time out.

 One more question.Why is the return value of watchdog_ping int? Anyway
 we discard it.

 I can not answer that question.

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck  wrote:
> On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
>> On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck  wrote:
>> > On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
>> >> Certain watchdog drivers use a timer to keep kicking the watchdog at
>> >> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>> >> we can't guarantee that watchdog will be pinged fast enough
>> >> for all system loads, especially if timeout is configured for
>> >> less than or equal to 1 second(basically small values).
>> >>
>> >> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>> >> add this functionality of individual watchdog drivers in the core
>> >> watchdog core.
>> >>
>> >> Signed-off-by: anish kumar 
>> >
>> > Not exactly what I had in mind. My idea was to enable the softdog only if
>> > the hardware watchdog's maximum timeout was low (say, less than a couple
>> > of minutes), and if a timeout larger than its maximum value was configured.
>>
>> watchdog_timeout_invalid wouldn't this check will fail if the user space 
>> tries
>> to set maximum timeout more that what driver can support?It would work
>> for pika_wdt.c as it is old watchdog driver and doesn't register with 
>> watchdog
>> framwork but new drivers has to pass this api.
>>
>> OR
>>
>> Do you want to remove this check and go as explained by you?I would
>> favour this approach though.
>>
> One would still have a check, but the enforced limits would no longer be
> the driver limits, but larger limits implemented in the watchdog core.
How much larger would be the big question here?Should it be configurable
property(sysfs?) or some hardcoding based on existing drivers?

Before going for next patch, it would be better for me to wait for some
more comments.
>
>> > In that case, I would have set the hardware watchdog to its maximum value
>> > and use the softdog to ping it at a rate of, say, 50% of this maximum.
>> >
>> > If userspace would not ping the watchdog within its configured value,
>> > I would stop pinging the hardware watchdog and let it time out.
>>
>> One more question.Why is the return value of watchdog_ping int? Anyway
>> we discard it.
>
> I can not answer that question.
>
> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck  wrote:
> On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
>> Certain watchdog drivers use a timer to keep kicking the watchdog at
>> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>> we can't guarantee that watchdog will be pinged fast enough
>> for all system loads, especially if timeout is configured for
>> less than or equal to 1 second(basically small values).
>>
>> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>> add this functionality of individual watchdog drivers in the core
>> watchdog core.
>>
>> Signed-off-by: anish kumar 
>
> Not exactly what I had in mind. My idea was to enable the softdog only if
> the hardware watchdog's maximum timeout was low (say, less than a couple
> of minutes), and if a timeout larger than its maximum value was configured.

watchdog_timeout_invalid wouldn't this check will fail if the user space tries
to set maximum timeout more that what driver can support?It would work
for pika_wdt.c as it is old watchdog driver and doesn't register with watchdog
framwork but new drivers has to pass this api.

OR

Do you want to remove this check and go as explained by you?I would
favour this approach though.

> In that case, I would have set the hardware watchdog to its maximum value
> and use the softdog to ping it at a rate of, say, 50% of this maximum.
>
> If userspace would not ping the watchdog within its configured value,
> I would stop pinging the hardware watchdog and let it time out.

One more question.Why is the return value of watchdog_ping int? Anyway
we discard it.
>
> Guenter
>
>> ---
>>  drivers/watchdog/watchdog_dev.c |   34 +-
>>  include/linux/watchdog.h|1 +
>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c 
>> b/drivers/watchdog/watchdog_dev.c
>> index faf4e18..0305803 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -41,9 +41,14 @@
>>  #include /* For handling misc devices */
>>  #include   /* For __init/__exit/... */
>>  #include/* For copy_to_user/put_user/... */
>> +#include 
>> +#include 
>>
>>  #include "watchdog_core.h"
>>
>> +/* Timer heartbeat (500ms) */
>> +#define WDT_TIMEOUT  (HZ/2) /* should this be sysfs? */
>> +
>>  /* the dev_t structure to store the dynamically allocated watchdog devices 
>> */
>>  static dev_t watchdog_devt;
>>  /* the watchdog device behind /dev/watchdog */
>> @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
>>   if (!watchdog_active(wddev))
>>   goto out_ping;
>>
>> - if (wddev->ops->ping)
>> - err = wddev->ops->ping(wddev);  /* ping the watchdog */
>> - else
>> - err = wddev->ops->start(wddev); /* restart watchdog */
>> + /* should we check ping interval value i.e. timeout value
>> +if it is less than certain threshold then only we
>> +should add this logic of periodic pinging? */
>> + if (time_before(jiffies, (unsigned long)wddev->timeout)) {
>> + if (wddev->ops->ping)
>> + err = wddev->ops->ping(wddev);/* ping the watchdog */
>> + else
>> + err = wddev->ops->start(wddev);/* restart watchdog */
>> + mod_timer(>timer, jiffies + WDT_TIMEOUT);
>> + } else {
>> + /*
>> +  *what we should when we find out that userspace
>> +  *has timed out?
>> +  **/
>> + }
>>
>>  out_ping:
>>   mutex_unlock(>lock);
>>   return err;
>>  }
>>
>> +static void watchdog_ping_wrapper(unsigned long priv)
>> +{
>> + struct watchdog_device *wdd = (void *)priv;
>> + watchdog_ping(wdd);
>> +}
>> +
>>  /*
>>   *   watchdog_start: wrapper to start the watchdog.
>>   *   @wddev: the watchdog device to start
>> @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
>>   err = wddev->ops->start(wddev);
>>   if (err == 0)
>>   set_bit(WDOG_ACTIVE, >status);
>> -
>> +
>> + mod_timer(>timer, jiffies + WDT_TIMEOUT);
>>  out_start:
>>   mutex_unlock(>lock);
>>   return err;
>> @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
>> *watchdog)
>>   old_wdd = NULL;
>>   }
>>   }
>> + setup_timer(>timer, 0, (long)watchdog_ping_wrapper);
>>   return err;
>>  }
>>
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index 2a3038e..e5f18f7 100644
>> --- a/include/linux/watchdog.h
>> +++ b/include/linux/watchdog.h
>> @@ -84,6 +84,7 @@ struct watchdog_device {
>>   const struct watchdog_ops *ops;
>>   unsigned int bootstatus;
>>   unsigned int timeout;
>> + struct timer_list timer;
>>   unsigned int min_timeout;
>>   unsigned int max_timeout;
>>   void *driver_data;
>> --
>> 1.7.10.4
>>
>>
--
To unsubscribe from this 

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

any inputs?

On Sun, Jun 2, 2013 at 3:56 PM, anish singh  wrote:
> On Sun, Jun 2, 2013 at 3:43 PM, anish kumar  
> wrote:
>> Certain watchdog drivers use a timer to keep kicking the watchdog at
>> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
>> we can't guarantee that watchdog will be pinged fast enough
>> for all system loads, especially if timeout is configured for
>> less than or equal to 1 second(basically small values).
>>
>> As suggested by Wim Van Sebroeck & Guenter Roeck we should
>> add this functionality of individual watchdog drivers in the core
>> watchdog core.
>>
>> Signed-off-by: anish kumar 
>> ---
>>  drivers/watchdog/watchdog_dev.c |   34 +-
>>  include/linux/watchdog.h|1 +
>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c 
>> b/drivers/watchdog/watchdog_dev.c
>> index faf4e18..0305803 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -41,9 +41,14 @@
>>  #include   /* For handling misc devices */
>>  #include /* For __init/__exit/... */
>>  #include  /* For copy_to_user/put_user/... */
>> +#include 
>> +#include 
>>
>>  #include "watchdog_core.h"
>>
>> +/* Timer heartbeat (500ms) */
>> +#define WDT_TIMEOUT(HZ/2) /* should this be sysfs? */
>> +
>>  /* the dev_t structure to store the dynamically allocated watchdog devices 
>> */
>>  static dev_t watchdog_devt;
>>  /* the watchdog device behind /dev/watchdog */
>> @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
> I think watchdog_ping function prototype should be changed as nowhere
> we use the return value.
>> if (!watchdog_active(wddev))
>> goto out_ping;
>>
>> -   if (wddev->ops->ping)
>> -   err = wddev->ops->ping(wddev);  /* ping the watchdog */
>> -   else
>> -   err = wddev->ops->start(wddev); /* restart watchdog */
>> +   /* should we check ping interval value i.e. timeout value
>> +  if it is less than certain threshold then only we
>> +  should add this logic of periodic pinging? */
>> +   if (time_before(jiffies, (unsigned long)wddev->timeout)) {
>> +   if (wddev->ops->ping)
>> +   err = wddev->ops->ping(wddev);/* ping the watchdog */
>> +   else
>> +   err = wddev->ops->start(wddev);/* restart watchdog */
>> +   mod_timer(>timer, jiffies + WDT_TIMEOUT);
>> +   } else {
>> +   /*
>> +*what we should when we find out that userspace
>> +*has timed out?
>> +**/
>> +   }
>>
>>  out_ping:
>> mutex_unlock(>lock);
>> return err;
>>  }
>>
>> +static void watchdog_ping_wrapper(unsigned long priv)
> if we change the watchdog_ping api then we don't need this wrapper.
>> +{
>> +   struct watchdog_device *wdd = (void *)priv;
>> +   watchdog_ping(wdd);
>> +}
>> +
>>  /*
>>   * watchdog_start: wrapper to start the watchdog.
>>   * @wddev: the watchdog device to start
>> @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
>> err = wddev->ops->start(wddev);
>> if (err == 0)
>> set_bit(WDOG_ACTIVE, >status);
>> -
>> +
>> +   mod_timer(>timer, jiffies + WDT_TIMEOUT);
>>  out_start:
>> mutex_unlock(>lock);
>> return err;
>> @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
>> *watchdog)
>> old_wdd = NULL;
>> }
>> }
>> +   setup_timer(>timer, 0, (long)watchdog_ping_wrapper);
>> return err;
>>  }
>>
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index 2a3038e..e5f18f7 100644
>> --- a/include/linux/watchdog.h
>> +++ b/include/linux/watchdog.h
>> @@ -84,6 +84,7 @@ struct watchdog_device {
>> const struct watchdog_ops *ops;
>> unsigned int bootstatus;
>> unsigned int timeout;
>> +   struct timer_list timer;
> should we use hrtimer? or timer would do?
>> unsigned int min_timeout;
>> unsigned int max_timeout;
>> void *driver_data;
>> --
>> 1.7.10.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

any inputs?

On Sun, Jun 2, 2013 at 3:56 PM, anish singh anish198519851...@gmail.com wrote:
 On Sun, Jun 2, 2013 at 3:43 PM, anish kumar anish198519851...@gmail.com 
 wrote:
 Certain watchdog drivers use a timer to keep kicking the watchdog at
 a rate of 0.5s (HZ/2) untill userspace times out.They do this as
 we can't guarantee that watchdog will be pinged fast enough
 for all system loads, especially if timeout is configured for
 less than or equal to 1 second(basically small values).

 As suggested by Wim Van Sebroeck  Guenter Roeck we should
 add this functionality of individual watchdog drivers in the core
 watchdog core.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  drivers/watchdog/watchdog_dev.c |   34 +-
  include/linux/watchdog.h|1 +
  2 files changed, 30 insertions(+), 5 deletions(-)

 diff --git a/drivers/watchdog/watchdog_dev.c 
 b/drivers/watchdog/watchdog_dev.c
 index faf4e18..0305803 100644
 --- a/drivers/watchdog/watchdog_dev.c
 +++ b/drivers/watchdog/watchdog_dev.c
 @@ -41,9 +41,14 @@
  #include linux/miscdevice.h  /* For handling misc devices */
  #include linux/init.h/* For __init/__exit/... */
  #include linux/uaccess.h /* For copy_to_user/put_user/... */
 +#include linux/timer.h
 +#include linux/jiffies.h

  #include watchdog_core.h

 +/* Timer heartbeat (500ms) */
 +#define WDT_TIMEOUT(HZ/2) /* should this be sysfs? */
 +
  /* the dev_t structure to store the dynamically allocated watchdog devices 
 */
  static dev_t watchdog_devt;
  /* the watchdog device behind /dev/watchdog */
 @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
 I think watchdog_ping function prototype should be changed as nowhere
 we use the return value.
 if (!watchdog_active(wddev))
 goto out_ping;

 -   if (wddev-ops-ping)
 -   err = wddev-ops-ping(wddev);  /* ping the watchdog */
 -   else
 -   err = wddev-ops-start(wddev); /* restart watchdog */
 +   /* should we check ping interval value i.e. timeout value
 +  if it is less than certain threshold then only we
 +  should add this logic of periodic pinging? */
 +   if (time_before(jiffies, (unsigned long)wddev-timeout)) {
 +   if (wddev-ops-ping)
 +   err = wddev-ops-ping(wddev);/* ping the watchdog */
 +   else
 +   err = wddev-ops-start(wddev);/* restart watchdog */
 +   mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
 +   } else {
 +   /*
 +*what we should when we find out that userspace
 +*has timed out?
 +**/
 +   }

  out_ping:
 mutex_unlock(wddev-lock);
 return err;
  }

 +static void watchdog_ping_wrapper(unsigned long priv)
 if we change the watchdog_ping api then we don't need this wrapper.
 +{
 +   struct watchdog_device *wdd = (void *)priv;
 +   watchdog_ping(wdd);
 +}
 +
  /*
   * watchdog_start: wrapper to start the watchdog.
   * @wddev: the watchdog device to start
 @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
 err = wddev-ops-start(wddev);
 if (err == 0)
 set_bit(WDOG_ACTIVE, wddev-status);
 -
 +
 +   mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
  out_start:
 mutex_unlock(wddev-lock);
 return err;
 @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
 *watchdog)
 old_wdd = NULL;
 }
 }
 +   setup_timer(watchdog-timer, 0, (long)watchdog_ping_wrapper);
 return err;
  }

 diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
 index 2a3038e..e5f18f7 100644
 --- a/include/linux/watchdog.h
 +++ b/include/linux/watchdog.h
 @@ -84,6 +84,7 @@ struct watchdog_device {
 const struct watchdog_ops *ops;
 unsigned int bootstatus;
 unsigned int timeout;
 +   struct timer_list timer;
 should we use hrtimer? or timer would do?
 unsigned int min_timeout;
 unsigned int max_timeout;
 void *driver_data;
 --
 1.7.10.4

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck li...@roeck-us.net wrote:
 On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
 Certain watchdog drivers use a timer to keep kicking the watchdog at
 a rate of 0.5s (HZ/2) untill userspace times out.They do this as
 we can't guarantee that watchdog will be pinged fast enough
 for all system loads, especially if timeout is configured for
 less than or equal to 1 second(basically small values).

 As suggested by Wim Van Sebroeck  Guenter Roeck we should
 add this functionality of individual watchdog drivers in the core
 watchdog core.

 Signed-off-by: anish kumar anish198519851...@gmail.com

 Not exactly what I had in mind. My idea was to enable the softdog only if
 the hardware watchdog's maximum timeout was low (say, less than a couple
 of minutes), and if a timeout larger than its maximum value was configured.

watchdog_timeout_invalid wouldn't this check will fail if the user space tries
to set maximum timeout more that what driver can support?It would work
for pika_wdt.c as it is old watchdog driver and doesn't register with watchdog
framwork but new drivers has to pass this api.

OR

Do you want to remove this check and go as explained by you?I would
favour this approach though.

 In that case, I would have set the hardware watchdog to its maximum value
 and use the softdog to ping it at a rate of, say, 50% of this maximum.

 If userspace would not ping the watchdog within its configured value,
 I would stop pinging the hardware watchdog and let it time out.

One more question.Why is the return value of watchdog_ping int? Anyway
we discard it.

 Guenter

 ---
  drivers/watchdog/watchdog_dev.c |   34 +-
  include/linux/watchdog.h|1 +
  2 files changed, 30 insertions(+), 5 deletions(-)

 diff --git a/drivers/watchdog/watchdog_dev.c 
 b/drivers/watchdog/watchdog_dev.c
 index faf4e18..0305803 100644
 --- a/drivers/watchdog/watchdog_dev.c
 +++ b/drivers/watchdog/watchdog_dev.c
 @@ -41,9 +41,14 @@
  #include linux/miscdevice.h/* For handling misc devices */
  #include linux/init.h  /* For __init/__exit/... */
  #include linux/uaccess.h   /* For copy_to_user/put_user/... */
 +#include linux/timer.h
 +#include linux/jiffies.h

  #include watchdog_core.h

 +/* Timer heartbeat (500ms) */
 +#define WDT_TIMEOUT  (HZ/2) /* should this be sysfs? */
 +
  /* the dev_t structure to store the dynamically allocated watchdog devices 
 */
  static dev_t watchdog_devt;
  /* the watchdog device behind /dev/watchdog */
 @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
   if (!watchdog_active(wddev))
   goto out_ping;

 - if (wddev-ops-ping)
 - err = wddev-ops-ping(wddev);  /* ping the watchdog */
 - else
 - err = wddev-ops-start(wddev); /* restart watchdog */
 + /* should we check ping interval value i.e. timeout value
 +if it is less than certain threshold then only we
 +should add this logic of periodic pinging? */
 + if (time_before(jiffies, (unsigned long)wddev-timeout)) {
 + if (wddev-ops-ping)
 + err = wddev-ops-ping(wddev);/* ping the watchdog */
 + else
 + err = wddev-ops-start(wddev);/* restart watchdog */
 + mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
 + } else {
 + /*
 +  *what we should when we find out that userspace
 +  *has timed out?
 +  **/
 + }

  out_ping:
   mutex_unlock(wddev-lock);
   return err;
  }

 +static void watchdog_ping_wrapper(unsigned long priv)
 +{
 + struct watchdog_device *wdd = (void *)priv;
 + watchdog_ping(wdd);
 +}
 +
  /*
   *   watchdog_start: wrapper to start the watchdog.
   *   @wddev: the watchdog device to start
 @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
   err = wddev-ops-start(wddev);
   if (err == 0)
   set_bit(WDOG_ACTIVE, wddev-status);
 -
 +
 + mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
  out_start:
   mutex_unlock(wddev-lock);
   return err;
 @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
 *watchdog)
   old_wdd = NULL;
   }
   }
 + setup_timer(watchdog-timer, 0, (long)watchdog_ping_wrapper);
   return err;
  }

 diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
 index 2a3038e..e5f18f7 100644
 --- a/include/linux/watchdog.h
 +++ b/include/linux/watchdog.h
 @@ -84,6 +84,7 @@ struct watchdog_device {
   const struct watchdog_ops *ops;
   unsigned int bootstatus;
   unsigned int timeout;
 + struct timer_list timer;
   unsigned int min_timeout;
   unsigned int max_timeout;
   void *driver_data;
 --
 1.7.10.4


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Tue, Jun 4, 2013 at 3:55 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Mon, Jun 03, 2013 at 10:23:04PM +0530, anish singh wrote:
 On Mon, Jun 3, 2013 at 8:57 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Sun, Jun 02, 2013 at 03:43:07PM +0530, anish kumar wrote:
  Certain watchdog drivers use a timer to keep kicking the watchdog at
  a rate of 0.5s (HZ/2) untill userspace times out.They do this as
  we can't guarantee that watchdog will be pinged fast enough
  for all system loads, especially if timeout is configured for
  less than or equal to 1 second(basically small values).
 
  As suggested by Wim Van Sebroeck  Guenter Roeck we should
  add this functionality of individual watchdog drivers in the core
  watchdog core.
 
  Signed-off-by: anish kumar anish198519851...@gmail.com
 
  Not exactly what I had in mind. My idea was to enable the softdog only if
  the hardware watchdog's maximum timeout was low (say, less than a couple
  of minutes), and if a timeout larger than its maximum value was configured.

 watchdog_timeout_invalid wouldn't this check will fail if the user space 
 tries
 to set maximum timeout more that what driver can support?It would work
 for pika_wdt.c as it is old watchdog driver and doesn't register with 
 watchdog
 framwork but new drivers has to pass this api.

 OR

 Do you want to remove this check and go as explained by you?I would
 favour this approach though.

 One would still have a check, but the enforced limits would no longer be
 the driver limits, but larger limits implemented in the watchdog core.
How much larger would be the big question here?Should it be configurable
property(sysfs?) or some hardcoding based on existing drivers?

Before going for next patch, it would be better for me to wait for some
more comments.

  In that case, I would have set the hardware watchdog to its maximum value
  and use the softdog to ping it at a rate of, say, 50% of this maximum.
 
  If userspace would not ping the watchdog within its configured value,
  I would stop pinging the hardware watchdog and let it time out.

 One more question.Why is the return value of watchdog_ping int? Anyway
 we discard it.

 I can not answer that question.

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

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Sun, Jun 2, 2013 at 3:43 PM, anish kumar  wrote:
> Certain watchdog drivers use a timer to keep kicking the watchdog at
> a rate of 0.5s (HZ/2) untill userspace times out.They do this as
> we can't guarantee that watchdog will be pinged fast enough
> for all system loads, especially if timeout is configured for
> less than or equal to 1 second(basically small values).
>
> As suggested by Wim Van Sebroeck & Guenter Roeck we should
> add this functionality of individual watchdog drivers in the core
> watchdog core.
>
> Signed-off-by: anish kumar 
> ---
>  drivers/watchdog/watchdog_dev.c |   34 +-
>  include/linux/watchdog.h|1 +
>  2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index faf4e18..0305803 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -41,9 +41,14 @@
>  #include   /* For handling misc devices */
>  #include /* For __init/__exit/... */
>  #include  /* For copy_to_user/put_user/... */
> +#include 
> +#include 
>
>  #include "watchdog_core.h"
>
> +/* Timer heartbeat (500ms) */
> +#define WDT_TIMEOUT(HZ/2) /* should this be sysfs? */
> +
>  /* the dev_t structure to store the dynamically allocated watchdog devices */
>  static dev_t watchdog_devt;
>  /* the watchdog device behind /dev/watchdog */
> @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
I think watchdog_ping function prototype should be changed as nowhere
we use the return value.
> if (!watchdog_active(wddev))
> goto out_ping;
>
> -   if (wddev->ops->ping)
> -   err = wddev->ops->ping(wddev);  /* ping the watchdog */
> -   else
> -   err = wddev->ops->start(wddev); /* restart watchdog */
> +   /* should we check ping interval value i.e. timeout value
> +  if it is less than certain threshold then only we
> +  should add this logic of periodic pinging? */
> +   if (time_before(jiffies, (unsigned long)wddev->timeout)) {
> +   if (wddev->ops->ping)
> +   err = wddev->ops->ping(wddev);/* ping the watchdog */
> +   else
> +   err = wddev->ops->start(wddev);/* restart watchdog */
> +   mod_timer(>timer, jiffies + WDT_TIMEOUT);
> +   } else {
> +   /*
> +*what we should when we find out that userspace
> +*has timed out?
> +**/
> +   }
>
>  out_ping:
> mutex_unlock(>lock);
> return err;
>  }
>
> +static void watchdog_ping_wrapper(unsigned long priv)
if we change the watchdog_ping api then we don't need this wrapper.
> +{
> +   struct watchdog_device *wdd = (void *)priv;
> +   watchdog_ping(wdd);
> +}
> +
>  /*
>   * watchdog_start: wrapper to start the watchdog.
>   * @wddev: the watchdog device to start
> @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
> err = wddev->ops->start(wddev);
> if (err == 0)
> set_bit(WDOG_ACTIVE, >status);
> -
> +
> +   mod_timer(>timer, jiffies + WDT_TIMEOUT);
>  out_start:
> mutex_unlock(>lock);
> return err;
> @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
> *watchdog)
> old_wdd = NULL;
> }
> }
> +   setup_timer(>timer, 0, (long)watchdog_ping_wrapper);
> return err;
>  }
>
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 2a3038e..e5f18f7 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -84,6 +84,7 @@ struct watchdog_device {
> const struct watchdog_ops *ops;
> unsigned int bootstatus;
> unsigned int timeout;
> +   struct timer_list timer;
should we use hrtimer? or timer would do?
> unsigned int min_timeout;
> unsigned int max_timeout;
> void *driver_data;
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC]Watchdog:core: constant pinging until userspace timesout when delay very less

[anish singh]

On Sun, Jun 2, 2013 at 3:43 PM, anish kumar anish198519851...@gmail.com wrote:
 Certain watchdog drivers use a timer to keep kicking the watchdog at
 a rate of 0.5s (HZ/2) untill userspace times out.They do this as
 we can't guarantee that watchdog will be pinged fast enough
 for all system loads, especially if timeout is configured for
 less than or equal to 1 second(basically small values).

 As suggested by Wim Van Sebroeck  Guenter Roeck we should
 add this functionality of individual watchdog drivers in the core
 watchdog core.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  drivers/watchdog/watchdog_dev.c |   34 +-
  include/linux/watchdog.h|1 +
  2 files changed, 30 insertions(+), 5 deletions(-)

 diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
 index faf4e18..0305803 100644
 --- a/drivers/watchdog/watchdog_dev.c
 +++ b/drivers/watchdog/watchdog_dev.c
 @@ -41,9 +41,14 @@
  #include linux/miscdevice.h  /* For handling misc devices */
  #include linux/init.h/* For __init/__exit/... */
  #include linux/uaccess.h /* For copy_to_user/put_user/... */
 +#include linux/timer.h
 +#include linux/jiffies.h

  #include watchdog_core.h

 +/* Timer heartbeat (500ms) */
 +#define WDT_TIMEOUT(HZ/2) /* should this be sysfs? */
 +
  /* the dev_t structure to store the dynamically allocated watchdog devices */
  static dev_t watchdog_devt;
  /* the watchdog device behind /dev/watchdog */
 @@ -73,16 +78,33 @@ static int watchdog_ping(struct watchdog_device *wddev)
I think watchdog_ping function prototype should be changed as nowhere
we use the return value.
 if (!watchdog_active(wddev))
 goto out_ping;

 -   if (wddev-ops-ping)
 -   err = wddev-ops-ping(wddev);  /* ping the watchdog */
 -   else
 -   err = wddev-ops-start(wddev); /* restart watchdog */
 +   /* should we check ping interval value i.e. timeout value
 +  if it is less than certain threshold then only we
 +  should add this logic of periodic pinging? */
 +   if (time_before(jiffies, (unsigned long)wddev-timeout)) {
 +   if (wddev-ops-ping)
 +   err = wddev-ops-ping(wddev);/* ping the watchdog */
 +   else
 +   err = wddev-ops-start(wddev);/* restart watchdog */
 +   mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
 +   } else {
 +   /*
 +*what we should when we find out that userspace
 +*has timed out?
 +**/
 +   }

  out_ping:
 mutex_unlock(wddev-lock);
 return err;
  }

 +static void watchdog_ping_wrapper(unsigned long priv)
if we change the watchdog_ping api then we don't need this wrapper.
 +{
 +   struct watchdog_device *wdd = (void *)priv;
 +   watchdog_ping(wdd);
 +}
 +
  /*
   * watchdog_start: wrapper to start the watchdog.
   * @wddev: the watchdog device to start
 @@ -109,7 +131,8 @@ static int watchdog_start(struct watchdog_device *wddev)
 err = wddev-ops-start(wddev);
 if (err == 0)
 set_bit(WDOG_ACTIVE, wddev-status);
 -
 +
 +   mod_timer(wddev-timer, jiffies + WDT_TIMEOUT);
  out_start:
 mutex_unlock(wddev-lock);
 return err;
 @@ -552,6 +575,7 @@ int watchdog_dev_register(struct watchdog_device 
 *watchdog)
 old_wdd = NULL;
 }
 }
 +   setup_timer(watchdog-timer, 0, (long)watchdog_ping_wrapper);
 return err;
  }

 diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
 index 2a3038e..e5f18f7 100644
 --- a/include/linux/watchdog.h
 +++ b/include/linux/watchdog.h
 @@ -84,6 +84,7 @@ struct watchdog_device {
 const struct watchdog_ops *ops;
 unsigned int bootstatus;
 unsigned int timeout;
 +   struct timer_list timer;
should we use hrtimer? or timer would do?
 unsigned int min_timeout;
 unsigned int max_timeout;
 void *driver_data;
 --
 1.7.10.4

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

Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

[anish singh]

Hi Guenter,

On Fri, May 31, 2013 at 3:32 AM, Guenter Roeck  wrote:
> On Thu, May 30, 2013 at 11:59:28PM +0200, Wim Van Sebroeck wrote:
>> Hi Guenter,
>>
>> > On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
>> > > On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
>> > > [...]
>> > > > > + watchdog_set_drvdata(_wdt, drv);
>> > > >
>> > > > I am missing the initialisation of the watchdog's timeout value here...
>> > >
>> > > This watchdog only knows two timeout values, 1s and 30s with the 
>> > > constraint
>> > > that you can't go back to 30s once your in a 1s timeout without a reset 
>> > > of the
>> > > CPLD. I could initially set it to 30s but that would be redundant.
>> > >
>> > I wonder - why bother with supporting one-second timeouts ?
>> >
>> > Is this realistic, ie can you guarantee that the watchdog will be pinged 
>> > fast
>> > enough to keep the system alive under all load conditions ? As far as I 
>> > know
>> > you can not even configure the watchdog application for less than 1 second
>> > ping intervals.
>>
>> that's why certain drivers use a timer to keep kicking the watchdog at
>> a rate of 0.5s (do a grep on HZ/2) untill userspace times out.
>> (example: drivers/watchdog/pika_wdt.c or drivers/watchdog/pcwd.c).
>>
> Yes, I have used the same trick in a couple of my drivers. That isn't done 
> here,
> though.
>
> I even thought about adding this capability to the infrastructure.

If you don't mind can I give it a try?
>
> Thanks,
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] watchdog: New watchdog driver for MEN A21 watchdogs

[anish singh]

Hi Guenter,

On Fri, May 31, 2013 at 3:32 AM, Guenter Roeck li...@roeck-us.net wrote:
 On Thu, May 30, 2013 at 11:59:28PM +0200, Wim Van Sebroeck wrote:
 Hi Guenter,

  On Tue, May 28, 2013 at 10:10:53AM +0200, Johannes Thumshirn wrote:
   On Mon, May 27, 2013 at 08:25:54PM +0200, Wim Van Sebroeck wrote:
   [...]
 + watchdog_set_drvdata(a21_wdt, drv);
   
I am missing the initialisation of the watchdog's timeout value here...
  
   This watchdog only knows two timeout values, 1s and 30s with the 
   constraint
   that you can't go back to 30s once your in a 1s timeout without a reset 
   of the
   CPLD. I could initially set it to 30s but that would be redundant.
  
  I wonder - why bother with supporting one-second timeouts ?
 
  Is this realistic, ie can you guarantee that the watchdog will be pinged 
  fast
  enough to keep the system alive under all load conditions ? As far as I 
  know
  you can not even configure the watchdog application for less than 1 second
  ping intervals.

 that's why certain drivers use a timer to keep kicking the watchdog at
 a rate of 0.5s (do a grep on HZ/2) untill userspace times out.
 (example: drivers/watchdog/pika_wdt.c or drivers/watchdog/pcwd.c).

 Yes, I have used the same trick in a couple of my drivers. That isn't done 
 here,
 though.

 I even thought about adding this capability to the infrastructure.

If you don't mind can I give it a try?

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

Re: What is listed in /sys/module?

[anish singh]

On Thu, May 30, 2013 at 2:04 PM, Jean Delvare  wrote:
> Hi Rusty,
>
> Thanks for the fast and helpful reply.
>
> On Thu, 30 May 2013 11:53:10 +0530, anish singh wrote:
>> On Thu, May 30, 2013 at 6:24 AM, Rusty Russell  wrote:
>> > Jean Delvare  writes:
>> >> Hi Greg, Rusty,
>> >>
>> >> I have a question related to /sys/module and can't seem to find the
>> >> answer by myself so I hope you can explain.
>> >>
>> >> I noticed that /sys/module contains more than /proc/modules. At first I
>> >> thought that any potentially modular piece of code would show up
>> >> in /sys/module, so /sys/module would include both actual modules and
>> >> "built-in modules".
>> >>
>> >> However I then noticed that some built-in modules do _not_ show up
>> >> in /sys/module. For example, I have USB and I2C core support built into
>> >> my 3.9.4 kernel, /sys/module/usbcore exists but /sys/module/i2c_core
>> >> does not. CONFIG_SENSORS_W83795=y did not give me /sys/module/w83795
>> >> either.
>> >
>> > Yes.  /sys/module entries are created for builtin "modules" with
>> > parameters.  This is because, the module names are discovered by
>
> Apparently having a module version defined is enough as well. For
> example xz_dec has no parameters, but it has a version,
> and /sys/module/xz_dec is created when built-in. The code for that is
> in version_sysfs_builtin().
>
>> So documentation is wrong about that.
>
> It is incomplete, as it doesn't mention any condition
> to /sys/module/MODULENAME being created, yes. I'll send a patch -
> unless someone has a plan to fix the issue itself.
>
>> > scouring the parameters: see param_sysfs_builtin().
>> >
>> > Two things to note about builtin modules:
>> >
>> > 1) There is nothing other than parameters in /sys/module/, except a
>> >uevent which is used for managing the parameters.  So, without
>> >parameters, it would be an empty directory.
>> >
>> > 2) We actually do generate a list of builtin modules these days, called
>> >modules.builtin.  So we could generate sysfs dirs from this.
>>
>> As I understand modules.builtin is the output of the kernel compilation.
>> So if we want to make /sys/module consistent then this file needs to
>> be parsed by kernel and corresponding sysfs created but what is the
>
> I don't know what you have in mind exactly, but I don't think the
> kernel is ever going to parse a user-space file (which may or may not
> be available at run-time, BTW.)

Well it does that in case of loading firmwares(loading from userspace)
but that is a different story altogether.
>
> Using include/config/tristate.conf directly seems impossible as well,
> as the kernel code has no way to look-up the module name from the
> Kconfig symbol name - only the build system knows it.
>
> So any approach based on modules.builtin or
> include/config/tristate.conf is going to be very tricky, I'm afraid. It
> might be easier to create a dummy section for built-in modules. This
> could be added to module_init(), except that some modules use
> postcore_initcall() or some such instead. Or maybe we have to create a
> dedicated call for it, but that would be touching a huge number of
> files - unless we make it optional. I can't think of a simple and clean
> solution, I'm afraid.
>
>> point of creating just the sysfs without having any parameters information
>> which is why this /sys/module exists in the first place right?
>
> My question was motivated by work I'm doing on the sensors-detect
> script. The purpose of that script is to tell the user which modules
> he/she should load to get hardware monitoring running on his/her
> system. The script has to load some modules for the detection itself
> (e.g. cpuid, i2c-dev, SMBus controller drivers) and outputs a list of
> drivers which should be loaded by the user (but the script itself does
> not load them.)
>
> Each of the drivers in question can be completely missing, or built-in,
> or built as a module but not loaded, or built as a module and loaded.
> In order to make the script as user-friendly as possible, we have to
> handle each case properly. If the driver is missing, we want to tell
> the user that we can't do the detection completely (if the driver was
> needed for probing) or he/she won't be able to make full use of the
> hardware (if the driver was needed for run-time.) If the driver is
> built-in or already loaded module, then all is fine, we don't have to
> load it (for probing) nor to tell the user to do it 

Re: What is listed in /sys/module?

[anish singh]

On Thu, May 30, 2013 at 6:24 AM, Rusty Russell  wrote:
> Jean Delvare  writes:
>> Hi Greg, Rusty,
>>
>> I have a question related to /sys/module and can't seem to find the
>> answer by myself so I hope you can explain.
>>
>> I noticed that /sys/module contains more than /proc/modules. At first I
>> thought that any potentially modular piece of code would show up
>> in /sys/module, so /sys/module would include both actual modules and
>> "built-in modules".
>>
>> However I then noticed that some built-in modules do _not_ show up
>> in /sys/module. For example, I have USB and I2C core support built into
>> my 3.9.4 kernel, /sys/module/usbcore exists but /sys/module/i2c_core
>> does not. CONFIG_SENSORS_W83795=y did not give me /sys/module/w83795
>> either.
>
> Yes.  /sys/module entries are created for builtin "modules" with
> parameters.  This is because, the module names are discovered by
So documentation is wrong about that.
> scouring the parameters: see param_sysfs_builtin().
>
> Two things to note about builtin modules:
>
> 1) There is nothing other than parameters in /sys/module/, except a
>uevent which is used for managing the parameters.  So, without
>parameters, it would be an empty directory.
>
> 2) We actually do generate a list of builtin modules these days, called
>modules.builtin.  So we could generate sysfs dirs from this.
As I understand modules.builtin is the output of the kernel compilation.
So if we want to make /sys/module consistent then this file needs to
be parsed by kernel and corresponding sysfs created but what is the
point of creating just the sysfs without having any parameters information
which is why this /sys/module exists in the first place right?
>
> If you want to make it consistent, I look forward to your patch!
I would be interested here.
>
> Cheers,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: What is listed in /sys/module?

[anish singh]

On Thu, May 30, 2013 at 6:24 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Jean Delvare kh...@linux-fr.org writes:
 Hi Greg, Rusty,

 I have a question related to /sys/module and can't seem to find the
 answer by myself so I hope you can explain.

 I noticed that /sys/module contains more than /proc/modules. At first I
 thought that any potentially modular piece of code would show up
 in /sys/module, so /sys/module would include both actual modules and
 built-in modules.

 However I then noticed that some built-in modules do _not_ show up
 in /sys/module. For example, I have USB and I2C core support built into
 my 3.9.4 kernel, /sys/module/usbcore exists but /sys/module/i2c_core
 does not. CONFIG_SENSORS_W83795=y did not give me /sys/module/w83795
 either.

 Yes.  /sys/module entries are created for builtin modules with
 parameters.  This is because, the module names are discovered by
So documentation is wrong about that.
 scouring the parameters: see param_sysfs_builtin().

 Two things to note about builtin modules:

 1) There is nothing other than parameters in /sys/module/, except a
uevent which is used for managing the parameters.  So, without
parameters, it would be an empty directory.

 2) We actually do generate a list of builtin modules these days, called
modules.builtin.  So we could generate sysfs dirs from this.
As I understand modules.builtin is the output of the kernel compilation.
So if we want to make /sys/module consistent then this file needs to
be parsed by kernel and corresponding sysfs created but what is the
point of creating just the sysfs without having any parameters information
which is why this /sys/module exists in the first place right?

 If you want to make it consistent, I look forward to your patch!
I would be interested here.

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

Re: What is listed in /sys/module?

[anish singh]

On Thu, May 30, 2013 at 2:04 PM, Jean Delvare kh...@linux-fr.org wrote:
 Hi Rusty,

 Thanks for the fast and helpful reply.

 On Thu, 30 May 2013 11:53:10 +0530, anish singh wrote:
 On Thu, May 30, 2013 at 6:24 AM, Rusty Russell ru...@rustcorp.com.au wrote:
  Jean Delvare kh...@linux-fr.org writes:
  Hi Greg, Rusty,
 
  I have a question related to /sys/module and can't seem to find the
  answer by myself so I hope you can explain.
 
  I noticed that /sys/module contains more than /proc/modules. At first I
  thought that any potentially modular piece of code would show up
  in /sys/module, so /sys/module would include both actual modules and
  built-in modules.
 
  However I then noticed that some built-in modules do _not_ show up
  in /sys/module. For example, I have USB and I2C core support built into
  my 3.9.4 kernel, /sys/module/usbcore exists but /sys/module/i2c_core
  does not. CONFIG_SENSORS_W83795=y did not give me /sys/module/w83795
  either.
 
  Yes.  /sys/module entries are created for builtin modules with
  parameters.  This is because, the module names are discovered by

 Apparently having a module version defined is enough as well. For
 example xz_dec has no parameters, but it has a version,
 and /sys/module/xz_dec is created when built-in. The code for that is
 in version_sysfs_builtin().

 So documentation is wrong about that.

 It is incomplete, as it doesn't mention any condition
 to /sys/module/MODULENAME being created, yes. I'll send a patch -
 unless someone has a plan to fix the issue itself.

  scouring the parameters: see param_sysfs_builtin().
 
  Two things to note about builtin modules:
 
  1) There is nothing other than parameters in /sys/module/, except a
 uevent which is used for managing the parameters.  So, without
 parameters, it would be an empty directory.
 
  2) We actually do generate a list of builtin modules these days, called
 modules.builtin.  So we could generate sysfs dirs from this.

 As I understand modules.builtin is the output of the kernel compilation.
 So if we want to make /sys/module consistent then this file needs to
 be parsed by kernel and corresponding sysfs created but what is the

 I don't know what you have in mind exactly, but I don't think the
 kernel is ever going to parse a user-space file (which may or may not
 be available at run-time, BTW.)

Well it does that in case of loading firmwares(loading from userspace)
but that is a different story altogether.

 Using include/config/tristate.conf directly seems impossible as well,
 as the kernel code has no way to look-up the module name from the
 Kconfig symbol name - only the build system knows it.

 So any approach based on modules.builtin or
 include/config/tristate.conf is going to be very tricky, I'm afraid. It
 might be easier to create a dummy section for built-in modules. This
 could be added to module_init(), except that some modules use
 postcore_initcall() or some such instead. Or maybe we have to create a
 dedicated call for it, but that would be touching a huge number of
 files - unless we make it optional. I can't think of a simple and clean
 solution, I'm afraid.

 point of creating just the sysfs without having any parameters information
 which is why this /sys/module exists in the first place right?

 My question was motivated by work I'm doing on the sensors-detect
 script. The purpose of that script is to tell the user which modules
 he/she should load to get hardware monitoring running on his/her
 system. The script has to load some modules for the detection itself
 (e.g. cpuid, i2c-dev, SMBus controller drivers) and outputs a list of
 drivers which should be loaded by the user (but the script itself does
 not load them.)

 Each of the drivers in question can be completely missing, or built-in,
 or built as a module but not loaded, or built as a module and loaded.
 In order to make the script as user-friendly as possible, we have to
 handle each case properly. If the driver is missing, we want to tell
 the user that we can't do the detection completely (if the driver was
 needed for probing) or he/she won't be able to make full use of the
 hardware (if the driver was needed for run-time.) If the driver is
 built-in or already loaded module, then all is fine, we don't have to
 load it (for probing) nor to tell the user to do it (for run-time.) If
 the driver is modular, we have to load and then unload it (for probing)
 or tell the user to do so (for run-time.) So we have 4 cases to handle:

 1* Driver missing
 2* Driver built-in
 3* Module not loaded
 4* Module loaded

 Case 4 is the easy one, we just look at /proc/modules. But having no
 way to tell if a given driver is built-in, makes it impossible to

 modprobe -l doesn't work?
 distinguish between cases 1, 2 and 3. Of course we can try to load the
 module anyway and see what happens, but then we have no way to tell a
 harmless failure (driver build-in) from a harmful one (driver missing).
 It is also harder

Re: [PATCH] power_supply: generic-adc-battery: Fix checking if none of the channels are supported

[anish singh]

On Fri, May 24, 2013 at 11:11 PM, Axel Lin  wrote:
> If none of the channels are supported, index is 0.
> Also ensure to return error code instead of 0 in goto second_mem_fail path.
>
> Signed-off-by: Axel Lin 
Acked-by: anish kumar 

I think anton would have already picked up.
> ---
>  drivers/power/generic-adc-battery.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/generic-adc-battery.c 
> b/drivers/power/generic-adc-battery.c
> index 8cb5d7f..59a1421 100644
> --- a/drivers/power/generic-adc-battery.c
> +++ b/drivers/power/generic-adc-battery.c
> @@ -299,8 +299,10 @@ static int gab_probe(struct platform_device *pdev)
> }
>
> /* none of the channels are supported so let's bail out */
> -   if (index == ARRAY_SIZE(gab_chan_name))
> +   if (index == 0) {
> +   ret = -ENODEV;
> goto second_mem_fail;
> +   }
>
> /*
>  * Total number of properties is equal to static properties
> --
> 1.8.1.2
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] power_supply: generic-adc-battery: Fix checking if none of the channels are supported

[anish singh]

On Fri, May 24, 2013 at 11:11 PM, Axel Lin axel@ingics.com wrote:
 If none of the channels are supported, index is 0.
 Also ensure to return error code instead of 0 in goto second_mem_fail path.

 Signed-off-by: Axel Lin axel@ingics.com
Acked-by: anish kumar anish198519851...@gmail.com

I think anton would have already picked up.
 ---
  drivers/power/generic-adc-battery.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/drivers/power/generic-adc-battery.c 
 b/drivers/power/generic-adc-battery.c
 index 8cb5d7f..59a1421 100644
 --- a/drivers/power/generic-adc-battery.c
 +++ b/drivers/power/generic-adc-battery.c
 @@ -299,8 +299,10 @@ static int gab_probe(struct platform_device *pdev)
 }

 /* none of the channels are supported so let's bail out */
 -   if (index == ARRAY_SIZE(gab_chan_name))
 +   if (index == 0) {
 +   ret = -ENODEV;
 goto second_mem_fail;
 +   }

 /*
  * Total number of properties is equal to static properties
 --
 1.8.1.2



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

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 6:52 PM, Ming Lei  wrote:
> On Mon, May 27, 2013 at 8:40 PM, anish singh
>  wrote:
>> On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai  wrote:
>>> At Mon, 27 May 2013 17:26:22 +0530,
>>> anish singh wrote:
>>>>
>>>> On Mon, May 27, 2013 at 4:00 PM, Ming Lei  wrote:
>>>> > Generally there are only two drivers which don't need uevent to
>>>> > handle firmware loading, so don't cache these firmwares during
>> Sorry but it confuses me further,
>
> Please put one blank line before your reply, otherwise it is a bit
> difficult to find your reply in email.

sorry.

>
>> If driver doesn't have information about these firmware then how
>> does it cache it?
>
> Driver only needs firmware's name for caching it, but driver can choose
> if it generates uevent to let userspace handle it. Without one uevent, the
> user space still can handle the request by waiting for the firmware sysfs
> device in advance. But if there is no one user space to handle the request,
> the request in kernel won't be completed and there is no timeout for this
> case, so will block suspend.

Now makes sense.Thanks!!
>
>
> Thanks,
> --
> Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai  wrote:
> At Mon, 27 May 2013 17:26:22 +0530,
> anish singh wrote:
>>
>> On Mon, May 27, 2013 at 4:00 PM, Ming Lei  wrote:
>> > Generally there are only two drivers which don't need uevent to
>> > handle firmware loading, so don't cache these firmwares during
Sorry but it confuses me further,
If driver doesn't have information about these firmware then how
does it cache it?
>> Sorry but this statement confuses me i.e. "drivers which don't need
>> uevent to handle firmware loading". Does this mean that driver is
>> dependent on uevent to load the firmware?
>
> No.
>
>> or does this mean
>> that driver generates uevent to userspace and userpace in turn
>> provides the firmware?
>
> No.
>
> The userspace doesn't require uevent, and the driver doesn't generate
> uevent, either.  The userspace just loads the file when ready.
> See Documentation/dell_rbu.txt for example.  (And yes, it's a bad
> design.)
>
>
> Takashi
>
>> > suspend for these drivers since doing that may block firmware
>> > loading forever.
>> Explanation about why would it block would really help me or
>> for that matter anyone who reads this commit. Or may be
>> a url which discussed this problem.
>> >
>> > Both the two drivers are involved in private firmware images, so
>> > they don't hit in direct loading too.
>> >
>> > Cc: Takashi Iwai 
>> > Signed-off-by: Ming Lei 
>> > ---
>> >  drivers/base/firmware_class.c |9 ++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> > index e650c25..64e7870 100644
>> > --- a/drivers/base/firmware_class.c
>> > +++ b/drivers/base/firmware_class.c
>> > @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware 
>> > **firmware_p, const char *name,
>> > return 1; /* need to load */
>> >  }
>> >
>> > -static int assign_firmware_buf(struct firmware *fw, struct device *device)
>> > +static int assign_firmware_buf(struct firmware *fw, struct device *device,
>> > +   bool skip_cache)
>> >  {
>> > struct firmware_buf *buf = fw->priv;
>> >
>> > @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, 
>> > struct device *device)
>> >  * device may has been deleted already, but the problem
>> >  * should be fixed in devres or driver core.
>> >  */
>> > -   if (device)
>> > +   if (device && !skip_cache)
>> > fw_add_devm_name(device, buf->fw_id);
>> >
>> > /*
>> > @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware 
>> > **firmware_p, const char *name,
>> > if (!fw_get_filesystem_firmware(device, fw->priv))
>> > ret = fw_load_from_user_helper(fw, name, device,
>> >uevent, nowait, timeout);
>> > +
>> > +   /* don't cache firmware handled without uevent */
>> > if (!ret)
>> > -   ret = assign_firmware_buf(fw, device);
>> > +   ret = assign_firmware_buf(fw, device, !uevent);
>> >
>> > usermodehelper_read_unlock();
>> >
>> > --
>> > 1.7.9.5
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majord...@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 4:00 PM, Ming Lei  wrote:
> Generally there are only two drivers which don't need uevent to
> handle firmware loading, so don't cache these firmwares during
Sorry but this statement confuses me i.e. "drivers which don't need
uevent to handle firmware loading". Does this mean that driver is
dependent on uevent to load the firmware?or does this mean
that driver generates uevent to userspace and userpace in turn
provides the firmware?
> suspend for these drivers since doing that may block firmware
> loading forever.
Explanation about why would it block would really help me or
for that matter anyone who reads this commit. Or may be
a url which discussed this problem.
>
> Both the two drivers are involved in private firmware images, so
> they don't hit in direct loading too.
>
> Cc: Takashi Iwai 
> Signed-off-by: Ming Lei 
> ---
>  drivers/base/firmware_class.c |9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index e650c25..64e7870 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, 
> const char *name,
> return 1; /* need to load */
>  }
>
> -static int assign_firmware_buf(struct firmware *fw, struct device *device)
> +static int assign_firmware_buf(struct firmware *fw, struct device *device,
> +   bool skip_cache)
>  {
> struct firmware_buf *buf = fw->priv;
>
> @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, 
> struct device *device)
>  * device may has been deleted already, but the problem
>  * should be fixed in devres or driver core.
>  */
> -   if (device)
> +   if (device && !skip_cache)
> fw_add_devm_name(device, buf->fw_id);
>
> /*
> @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, 
> const char *name,
> if (!fw_get_filesystem_firmware(device, fw->priv))
> ret = fw_load_from_user_helper(fw, name, device,
>uevent, nowait, timeout);
> +
> +   /* don't cache firmware handled without uevent */
> if (!ret)
> -   ret = assign_firmware_buf(fw, device);
> +   ret = assign_firmware_buf(fw, device, !uevent);
>
> usermodehelper_read_unlock();
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 4:00 PM, Ming Lei ming@canonical.com wrote:
 Generally there are only two drivers which don't need uevent to
 handle firmware loading, so don't cache these firmwares during
Sorry but this statement confuses me i.e. drivers which don't need
uevent to handle firmware loading. Does this mean that driver is
dependent on uevent to load the firmware?or does this mean
that driver generates uevent to userspace and userpace in turn
provides the firmware?
 suspend for these drivers since doing that may block firmware
 loading forever.
Explanation about why would it block would really help me or
for that matter anyone who reads this commit. Or may be
a url which discussed this problem.

 Both the two drivers are involved in private firmware images, so
 they don't hit in direct loading too.

 Cc: Takashi Iwai ti...@suse.de
 Signed-off-by: Ming Lei ming@canonical.com
 ---
  drivers/base/firmware_class.c |9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

 diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
 index e650c25..64e7870 100644
 --- a/drivers/base/firmware_class.c
 +++ b/drivers/base/firmware_class.c
 @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware **firmware_p, 
 const char *name,
 return 1; /* need to load */
  }

 -static int assign_firmware_buf(struct firmware *fw, struct device *device)
 +static int assign_firmware_buf(struct firmware *fw, struct device *device,
 +   bool skip_cache)
  {
 struct firmware_buf *buf = fw-priv;

 @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, 
 struct device *device)
  * device may has been deleted already, but the problem
  * should be fixed in devres or driver core.
  */
 -   if (device)
 +   if (device  !skip_cache)
 fw_add_devm_name(device, buf-fw_id);

 /*
 @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware **firmware_p, 
 const char *name,
 if (!fw_get_filesystem_firmware(device, fw-priv))
 ret = fw_load_from_user_helper(fw, name, device,
uevent, nowait, timeout);
 +
 +   /* don't cache firmware handled without uevent */
 if (!ret)
 -   ret = assign_firmware_buf(fw, device);
 +   ret = assign_firmware_buf(fw, device, !uevent);

 usermodehelper_read_unlock();

 --
 1.7.9.5

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

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai ti...@suse.de wrote:
 At Mon, 27 May 2013 17:26:22 +0530,
 anish singh wrote:

 On Mon, May 27, 2013 at 4:00 PM, Ming Lei ming@canonical.com wrote:
  Generally there are only two drivers which don't need uevent to
  handle firmware loading, so don't cache these firmwares during
Sorry but it confuses me further,
If driver doesn't have information about these firmware then how
does it cache it?
 Sorry but this statement confuses me i.e. drivers which don't need
 uevent to handle firmware loading. Does this mean that driver is
 dependent on uevent to load the firmware?

 No.

 or does this mean
 that driver generates uevent to userspace and userpace in turn
 provides the firmware?

 No.

 The userspace doesn't require uevent, and the driver doesn't generate
 uevent, either.  The userspace just loads the file when ready.
 See Documentation/dell_rbu.txt for example.  (And yes, it's a bad
 design.)


 Takashi

  suspend for these drivers since doing that may block firmware
  loading forever.
 Explanation about why would it block would really help me or
 for that matter anyone who reads this commit. Or may be
 a url which discussed this problem.
 
  Both the two drivers are involved in private firmware images, so
  they don't hit in direct loading too.
 
  Cc: Takashi Iwai ti...@suse.de
  Signed-off-by: Ming Lei ming@canonical.com
  ---
   drivers/base/firmware_class.c |9 ++---
   1 file changed, 6 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
  index e650c25..64e7870 100644
  --- a/drivers/base/firmware_class.c
  +++ b/drivers/base/firmware_class.c
  @@ -993,7 +993,8 @@ _request_firmware_prepare(struct firmware 
  **firmware_p, const char *name,
  return 1; /* need to load */
   }
 
  -static int assign_firmware_buf(struct firmware *fw, struct device *device)
  +static int assign_firmware_buf(struct firmware *fw, struct device *device,
  +   bool skip_cache)
   {
  struct firmware_buf *buf = fw-priv;
 
  @@ -1010,7 +1011,7 @@ static int assign_firmware_buf(struct firmware *fw, 
  struct device *device)
   * device may has been deleted already, but the problem
   * should be fixed in devres or driver core.
   */
  -   if (device)
  +   if (device  !skip_cache)
  fw_add_devm_name(device, buf-fw_id);
 
  /*
  @@ -1066,8 +1067,10 @@ _request_firmware(const struct firmware 
  **firmware_p, const char *name,
  if (!fw_get_filesystem_firmware(device, fw-priv))
  ret = fw_load_from_user_helper(fw, name, device,
 uevent, nowait, timeout);
  +
  +   /* don't cache firmware handled without uevent */
  if (!ret)
  -   ret = assign_firmware_buf(fw, device);
  +   ret = assign_firmware_buf(fw, device, !uevent);
 
  usermodehelper_read_unlock();
 
  --
  1.7.9.5
 
  --
  To unsubscribe from this list: send the line unsubscribe linux-kernel in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  Please read the FAQ at  http://www.tux.org/lkml/

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

Re: [PATCH 1/2] driver core: firmware loader: don't cache FW_ACTION_NOHOTPLUG firmware

[anish singh]

On Mon, May 27, 2013 at 6:52 PM, Ming Lei ming@canonical.com wrote:
 On Mon, May 27, 2013 at 8:40 PM, anish singh
 anish198519851...@gmail.com wrote:
 On Mon, May 27, 2013 at 5:53 PM, Takashi Iwai ti...@suse.de wrote:
 At Mon, 27 May 2013 17:26:22 +0530,
 anish singh wrote:

 On Mon, May 27, 2013 at 4:00 PM, Ming Lei ming@canonical.com wrote:
  Generally there are only two drivers which don't need uevent to
  handle firmware loading, so don't cache these firmwares during
 Sorry but it confuses me further,

 Please put one blank line before your reply, otherwise it is a bit
 difficult to find your reply in email.

sorry.


 If driver doesn't have information about these firmware then how
 does it cache it?

 Driver only needs firmware's name for caching it, but driver can choose
 if it generates uevent to let userspace handle it. Without one uevent, the
 user space still can handle the request by waiting for the firmware sysfs
 device in advance. But if there is no one user space to handle the request,
 the request in kernel won't be completed and there is no timeout for this
 case, so will block suspend.

Now makes sense.Thanks!!


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

Re: [I2C] informations + advice about messages handling

[anish singh]

On Fri, May 24, 2013 at 1:14 PM, Jean Delvare  wrote:
> Hi Anish, Mylène,
>
> On Fri, 24 May 2013 12:52:40 +0530, anish singh wrote:
>> On Fri, May 24, 2013 at 12:41 PM, Mylene Josserand
>>  wrote:
>> > I have read that this function "i2c_smbus_write_byte_data" uses
>> > "i2c_smbus_xfer" which uses "i2c_lock_adapter".
>> > In this function, there is a mutex so I thought that it will handle it
>> > but it says "Get exclusive access to an I2C bus segment". What is
>> > exactly an I2C segment ? Is it the device we are talking about ? Or is
>> > it the use of the i2c bus ?
>> Don't know what you are referring here.
>
> In the most simple case, your I2C bus has a single segment so "segment"
> or "bus" mean the same thing.
>
> It only starts mattering when I2C bus multiplexing comes into play.
> Then your bus is split into segments, with one segment (trunk) between
> the master (controller) and the multiplexer, and one or more segments
> (branches) hanging off the multiplexer.
>
> Take look at
> https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing
> for a sample topology.
>
> But anyway the comment in front of i2c_lock_adapter() is somewhat
> misleading. If you read the code you'll see that it always locks the
> whole bus tree, because it uses the root segment's mutex.
>
>> > I will certainly have to create an i2c driver and I would like to know
>> > if this "collision" handling (if it is handled) is done in old kernel
>> > (2.6.32) or is it handled only in new kernel versions ?
>> AFAIK collision handling and detection was not supported till now
>> in linux kernel until recently but I think this patch is doing that.
>> I may be wrong but I didn't see collision handling in earlier linux
>> kernels.
>> http://thread.gmane.org/gmane.linux.kernel/1410276
>
> This is for a specific case. The general case is handled by the
> per-adapter mutex for years now. 2.6.32 should be just fine in this
> respect.
I may be mis-understanding here but when two client wants to
communicate with same master and at the same
time how does that happen?
In the thread I mentioned client pulls the line low indicating that
it wants to own the bus and waits for some time and then checks
if the line is still low or not.If it is still low then it owns the bus
and start transaction and if clients see otherwise it releases the
bus and tries again after some time.How is that handled in the
previous kernel?
>
>> > If you have any documentation on how the i2c messages are handled in
>> > case of different devices uses, it will help me a lot ! I will search in
>> > the kernel documentation but there is many files about i2c.
>> > And if you know a good i2c driver that I can use as an example to design
>> > mine, it will be great !
>
> Best is to look at a driver for a device which is similar in
> functionality to yours.
>
> --
> Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [I2C] informations + advice about messages handling

[anish singh]

On Fri, May 24, 2013 at 12:41 PM, Mylene Josserand
 wrote:
> Hi all,
>
>
> I am learning how i2c is working and I read that, to write in an i2c
> register, I need to use the function "i2c_smbus_write_byte_data".
Only in case your device is smbus compliant.

> I wanted to know how the message are handled by using this function. If
> I use this function to talk with 2 different i2d devices, how it will
> handle "message collision" ? should I have to add a kind of mutex on the
Message collision and detection is the job of i2c controller and if I am
not wrong you are writing a chip driver.
> access of the i2c bus ?
> Is it possible that the message destined for one device is sent to
> another one ? Or a "mix" of messages is impossible ?
It is not possible as the data contains the chip address which is
unique.7/10 bit mode addressing is used for addresses.
>
> I have read that this function "i2c_smbus_write_byte_data" uses
> "i2c_smbus_xfer" which uses "i2c_lock_adapter".
> In this function, there is a mutex so I thought that it will handle it
> but it says "Get exclusive access to an I2C bus segment". What is
> exactly an I2C segment ? Is it the device we are talking about ? Or is
> it the use of the i2c bus ?
Don't know what you are referring here.
>
> I will certainly have to create an i2c driver and I would like to know
> if this "collision" handling (if it is handled) is done in old kernel
> (2.6.32) or is it handled only in new kernel versions ?
AFAIK collision handling and detection was not supported till now
in linux kernel until recently but I think this patch is doing that.
I may be wrong but I didn't see collision handling in earlier linux
kernels.
http://thread.gmane.org/gmane.linux.kernel/1410276
>
> If you have any documentation on how the i2c messages are handled in
> case of different devices uses, it will help me a lot ! I will search in
> the kernel documentation but there is many files about i2c.
> And if you know a good i2c driver that I can use as an example to design
> mine, it will be great !
>
>
> Thank you in advance,
>
> --
> Mylène JOSSERAND
>
> ___
> Kernelnewbies mailing list
> kernelnewb...@kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [I2C] informations + advice about messages handling

[anish singh]

On Fri, May 24, 2013 at 12:41 PM, Mylene Josserand
mylene.josser...@navocap.com wrote:
 Hi all,


 I am learning how i2c is working and I read that, to write in an i2c
 register, I need to use the function i2c_smbus_write_byte_data.
Only in case your device is smbus compliant.

 I wanted to know how the message are handled by using this function. If
 I use this function to talk with 2 different i2d devices, how it will
 handle message collision ? should I have to add a kind of mutex on the
Message collision and detection is the job of i2c controller and if I am
not wrong you are writing a chip driver.
 access of the i2c bus ?
 Is it possible that the message destined for one device is sent to
 another one ? Or a mix of messages is impossible ?
It is not possible as the data contains the chip address which is
unique.7/10 bit mode addressing is used for addresses.

 I have read that this function i2c_smbus_write_byte_data uses
 i2c_smbus_xfer which uses i2c_lock_adapter.
 In this function, there is a mutex so I thought that it will handle it
 but it says Get exclusive access to an I2C bus segment. What is
 exactly an I2C segment ? Is it the device we are talking about ? Or is
 it the use of the i2c bus ?
Don't know what you are referring here.

 I will certainly have to create an i2c driver and I would like to know
 if this collision handling (if it is handled) is done in old kernel
 (2.6.32) or is it handled only in new kernel versions ?
AFAIK collision handling and detection was not supported till now
in linux kernel until recently but I think this patch is doing that.
I may be wrong but I didn't see collision handling in earlier linux
kernels.
http://thread.gmane.org/gmane.linux.kernel/1410276

 If you have any documentation on how the i2c messages are handled in
 case of different devices uses, it will help me a lot ! I will search in
 the kernel documentation but there is many files about i2c.
 And if you know a good i2c driver that I can use as an example to design
 mine, it will be great !


 Thank you in advance,

 --
 Mylène JOSSERAND

 ___
 Kernelnewbies mailing list
 kernelnewb...@kernelnewbies.org
 http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [I2C] informations + advice about messages handling

[anish singh]

On Fri, May 24, 2013 at 1:14 PM, Jean Delvare kh...@linux-fr.org wrote:
 Hi Anish, Mylène,

 On Fri, 24 May 2013 12:52:40 +0530, anish singh wrote:
 On Fri, May 24, 2013 at 12:41 PM, Mylene Josserand
 mylene.josser...@navocap.com wrote:
  I have read that this function i2c_smbus_write_byte_data uses
  i2c_smbus_xfer which uses i2c_lock_adapter.
  In this function, there is a mutex so I thought that it will handle it
  but it says Get exclusive access to an I2C bus segment. What is
  exactly an I2C segment ? Is it the device we are talking about ? Or is
  it the use of the i2c bus ?
 Don't know what you are referring here.

 In the most simple case, your I2C bus has a single segment so segment
 or bus mean the same thing.

 It only starts mattering when I2C bus multiplexing comes into play.
 Then your bus is split into segments, with one segment (trunk) between
 the master (controller) and the multiplexer, and one or more segments
 (branches) hanging off the multiplexer.

 Take look at
 https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing
 for a sample topology.

 But anyway the comment in front of i2c_lock_adapter() is somewhat
 misleading. If you read the code you'll see that it always locks the
 whole bus tree, because it uses the root segment's mutex.

  I will certainly have to create an i2c driver and I would like to know
  if this collision handling (if it is handled) is done in old kernel
  (2.6.32) or is it handled only in new kernel versions ?
 AFAIK collision handling and detection was not supported till now
 in linux kernel until recently but I think this patch is doing that.
 I may be wrong but I didn't see collision handling in earlier linux
 kernels.
 http://thread.gmane.org/gmane.linux.kernel/1410276

 This is for a specific case. The general case is handled by the
 per-adapter mutex for years now. 2.6.32 should be just fine in this
 respect.
I may be mis-understanding here but when two client wants to
communicate with same master and at the same
time how does that happen?
In the thread I mentioned client pulls the line low indicating that
it wants to own the bus and waits for some time and then checks
if the line is still low or not.If it is still low then it owns the bus
and start transaction and if clients see otherwise it releases the
bus and tries again after some time.How is that handled in the
previous kernel?

  If you have any documentation on how the i2c messages are handled in
  case of different devices uses, it will help me a lot ! I will search in
  the kernel documentation but there is many files about i2c.
  And if you know a good i2c driver that I can use as an example to design
  mine, it will be great !

 Best is to look at a driver for a device which is similar in
 functionality to yours.

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

Re: microcode loading got really slow.

[anish singh]

On Thu, May 23, 2013 at 3:46 PM, Takashi Iwai  wrote:
> At Thu, 23 May 2013 10:06:56 +0200,
> Takashi Iwai wrote:
>>
>> At Thu, 23 May 2013 15:45:32 +0800,
>> Ming Lei wrote:
>> >
>> > On Thu, May 23, 2013 at 11:39 AM, Dave Jones  wrote:
>> > >  > On 05/21/2013 04:03 PM, Dave Jones wrote:
>> > >  >
>> > >  > [   72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
>> > >  > [  132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
>> > >  > [  192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
>> > >  > [  252.702055] microcode: Microcode Update Driver: v2.00 
>> > > , Peter Oruba
>> > >  >
>> > >  > For some reason the events for udev seem to be getting delayed 60s
>> > >  > for each core.
>> > >
>> > > Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER 
>> > > inadvertantly set
>> > > Odd though that it causes that 60 second delay, given that it's 
>> > > supposedly a
>> > > 'fallback' when the direct loading fails.
>> >
>> > udevd has the ugly problem previously at some situations(for example,
>> > request_firmware called in probe(), and that is why direct loading is
>> > introduced),
>> > but not sure why the direct loading is failed first.
>>
>> The microcode update is optional, so it's no error even if the
>> microcode firmwares are not found.
>>
>> But yes, this seems happening during the module probing.  The lines
>> "microcde: CPU..." show before "microcode: Microcode Update
>> Driver...", which means the f/w loading has been done before finishing
>> the module load.
>>
>> I thought (or hoped) this mess (60s stalls) was fixed in the recent
>> udev, but apparently not...?
>
> Thinking on this again, if the user-space continues to be broken in
> that point, we should provide request_firmware() variant without udev,
> e.g. request_firmware_direct(), and use it in known places like this?
Is it not already there?I was thinking that some time back this support
was added.
>
>
> Takashi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: microcode loading got really slow.

[anish singh]

On Thu, May 23, 2013 at 3:46 PM, Takashi Iwai ti...@suse.de wrote:
 At Thu, 23 May 2013 10:06:56 +0200,
 Takashi Iwai wrote:

 At Thu, 23 May 2013 15:45:32 +0800,
 Ming Lei wrote:
 
  On Thu, May 23, 2013 at 11:39 AM, Dave Jones da...@redhat.com wrote:
 On 05/21/2013 04:03 PM, Dave Jones wrote:

 [   72.318133] microcode: CPU1 sig=0x306c3, pf=0x2, revision=0x6
 [  132.446449] microcode: CPU2 sig=0x306c3, pf=0x2, revision=0x6
 [  192.573101] microcode: CPU3 sig=0x306c3, pf=0x2, revision=0x6
 [  252.702055] microcode: Microcode Update Driver: v2.00 
   tig...@aivazian.fsnet.co.uk, Peter Oruba

 For some reason the events for udev seem to be getting delayed 60s
 for each core.
  
   Screwed up my .config, and had CONFIG_FW_LOADER_USER_HELPER 
   inadvertantly set
   Odd though that it causes that 60 second delay, given that it's 
   supposedly a
   'fallback' when the direct loading fails.
 
  udevd has the ugly problem previously at some situations(for example,
  request_firmware called in probe(), and that is why direct loading is
  introduced),
  but not sure why the direct loading is failed first.

 The microcode update is optional, so it's no error even if the
 microcode firmwares are not found.

 But yes, this seems happening during the module probing.  The lines
 microcde: CPU... show before microcode: Microcode Update
 Driver..., which means the f/w loading has been done before finishing
 the module load.

 I thought (or hoped) this mess (60s stalls) was fixed in the recent
 udev, but apparently not...?

 Thinking on this again, if the user-space continues to be broken in
 that point, we should provide request_firmware() variant without udev,
 e.g. request_firmware_direct(), and use it in known places like this?
Is it not already there?I was thinking that some time back this support
was added.


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

Re: [RFC PATCH 6/8] kthread: Enable parking requests from setup() and unpark() callbacks

[anish singh]

On Tue, May 21, 2013 at 1:19 PM, Srivatsa S. Bhat
 wrote:
> On 05/21/2013 11:04 AM, anish singh wrote:
>> On Mon, May 20, 2013 at 9:31 PM, Frederic Weisbecker  
>> wrote:
>>> When the watchdog code is boot-disabled by the user, for example
>>> through the 'nmi_watchdog=0' boot option, the setup() callback of
>>> the watchdog kthread requests to park the task, and that until the
>>> user later re-enables the watchdog through sysctl or procfs.
>>>
>>> However the parking request is not well handled when done from
>>> the setup() callback. After ->setup() is called, the generic smpboot
>>> kthread loop directly tries to call the thread function or wait
>>> for some event if ->thread_should_run() is false.
>>>
>>> In the case of the watchdog kthread, ->thread_should_run() returns
>>> false and the kthread goes to sleep and wait for the watchdog timer
>>> to wake it up. But the timer is not enabled since the user requested
>>> to disable the watchdog. We want the kthread to park instead of waiting
>>> for events that can't happen.
>>>
>>> As a result, later unpark requests after sysctl write through
>>> 'sysctl -w kernel.watchdog=1' won't wake up/unpark the task as
>>> expected, since it's not parked anyway, leaving the value modified
>>> without triggering any action.
>> Out of curiosity, this can happen only for short period of time right?After
>> some time this will work right as the thread get back in action
>> after the schedule call.Then sysctl and procfs will work I think.
>
> kthread_unpark() can wake up a task only if the task is in TASK_PARKED
> state. But since the above task would be in TASK_INTERRUPTIBLE state
> (since it is not parked), kthread_unpark() will be powerless to do anything.
Yes but this will happen only for a short period of time right?
After the schdule() call the below code gets executed in while() loop.

if (kthread_should_park()) {
__set_current_state(TASK_RUNNING);
preempt_enable();
if (ht->park && td->status == HP_THREAD_ACTIVE) {
BUG_ON(td->cpu != smp_processor_id());
ht->park(td->cpu);
td->status = HP_THREAD_PARKED;
}
kthread_parkme();
/* We might have been woken for stop */
continue;
}

As we have already called kthread_park this above if() condition gets true and
it will park the thread wouldn't it?But this will happen after the schedule
call which is not right as mentioned by fredrick.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 6/8] kthread: Enable parking requests from setup() and unpark() callbacks

[anish singh]

On Tue, May 21, 2013 at 1:19 PM, Srivatsa S. Bhat
srivatsa.b...@linux.vnet.ibm.com wrote:
 On 05/21/2013 11:04 AM, anish singh wrote:
 On Mon, May 20, 2013 at 9:31 PM, Frederic Weisbecker fweis...@gmail.com 
 wrote:
 When the watchdog code is boot-disabled by the user, for example
 through the 'nmi_watchdog=0' boot option, the setup() callback of
 the watchdog kthread requests to park the task, and that until the
 user later re-enables the watchdog through sysctl or procfs.

 However the parking request is not well handled when done from
 the setup() callback. After -setup() is called, the generic smpboot
 kthread loop directly tries to call the thread function or wait
 for some event if -thread_should_run() is false.

 In the case of the watchdog kthread, -thread_should_run() returns
 false and the kthread goes to sleep and wait for the watchdog timer
 to wake it up. But the timer is not enabled since the user requested
 to disable the watchdog. We want the kthread to park instead of waiting
 for events that can't happen.

 As a result, later unpark requests after sysctl write through
 'sysctl -w kernel.watchdog=1' won't wake up/unpark the task as
 expected, since it's not parked anyway, leaving the value modified
 without triggering any action.
 Out of curiosity, this can happen only for short period of time right?After
 some time this will work right as the thread get back in action
 after the schedule call.Then sysctl and procfs will work I think.

 kthread_unpark() can wake up a task only if the task is in TASK_PARKED
 state. But since the above task would be in TASK_INTERRUPTIBLE state
 (since it is not parked), kthread_unpark() will be powerless to do anything.
Yes but this will happen only for a short period of time right?
After the schdule() call the below code gets executed in while() loop.

if (kthread_should_park()) {
__set_current_state(TASK_RUNNING);
preempt_enable();
if (ht-park  td-status == HP_THREAD_ACTIVE) {
BUG_ON(td-cpu != smp_processor_id());
ht-park(td-cpu);
td-status = HP_THREAD_PARKED;
}
kthread_parkme();
/* We might have been woken for stop */
continue;
}

As we have already called kthread_park this above if() condition gets true and
it will park the thread wouldn't it?But this will happen after the schedule
call which is not right as mentioned by fredrick.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 6/8] kthread: Enable parking requests from setup() and unpark() callbacks

[anish singh]

On Mon, May 20, 2013 at 9:31 PM, Frederic Weisbecker  wrote:
> When the watchdog code is boot-disabled by the user, for example
> through the 'nmi_watchdog=0' boot option, the setup() callback of
> the watchdog kthread requests to park the task, and that until the
> user later re-enables the watchdog through sysctl or procfs.
>
> However the parking request is not well handled when done from
> the setup() callback. After ->setup() is called, the generic smpboot
> kthread loop directly tries to call the thread function or wait
> for some event if ->thread_should_run() is false.
>
> In the case of the watchdog kthread, ->thread_should_run() returns
> false and the kthread goes to sleep and wait for the watchdog timer
> to wake it up. But the timer is not enabled since the user requested
> to disable the watchdog. We want the kthread to park instead of waiting
> for events that can't happen.
>
> As a result, later unpark requests after sysctl write through
> 'sysctl -w kernel.watchdog=1' won't wake up/unpark the task as
> expected, since it's not parked anyway, leaving the value modified
> without triggering any action.
Out of curiosity, this can happen only for short period of time right?After
some time this will work right as the thread get back in action
after the schedule call.Then sysctl and procfs will work I think.
>
> We could workaround some solution in the watchdog code like forcing
> one pass to the thread function and immediately return to park.
>
> But supporting parking requests from ->setup() or ->unpark()
> callbacks look like proper way to implement cancellation in
> general. So let's fix it that way.
>
> Signed-off-by: Frederic Weisbecker 
> Cc: Srivatsa S. Bhat 
> Cc: Steven Rostedt 
> Cc: Paul E. McKenney 
> Cc: Ingo Molnar 
> Cc: Thomas Gleixner 
> Cc: Peter Zijlstra 
> Cc: Borislav Petkov 
> Cc: Li Zhong 
> Cc: Don Zickus 
> ---
>  kernel/smpboot.c |6 ++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/smpboot.c b/kernel/smpboot.c
> index 02fc5c9..3394ed0 100644
> --- a/kernel/smpboot.c
> +++ b/kernel/smpboot.c
> @@ -151,6 +151,12 @@ static int smpboot_thread_fn(void *data)
> break;
> }
>
> +   /* Check if setup or unpark actually want us to park */
> +   if (kthread_should_stop() || kthread_should_park()) {
> +   preempt_enable();
> +   continue;
> +   }
> +
> if (!ht->thread_should_run(td->cpu)) {
> preempt_enable();
> schedule();
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 6/8] kthread: Enable parking requests from setup() and unpark() callbacks

[anish singh]

On Mon, May 20, 2013 at 9:31 PM, Frederic Weisbecker fweis...@gmail.com wrote:
 When the watchdog code is boot-disabled by the user, for example
 through the 'nmi_watchdog=0' boot option, the setup() callback of
 the watchdog kthread requests to park the task, and that until the
 user later re-enables the watchdog through sysctl or procfs.

 However the parking request is not well handled when done from
 the setup() callback. After -setup() is called, the generic smpboot
 kthread loop directly tries to call the thread function or wait
 for some event if -thread_should_run() is false.

 In the case of the watchdog kthread, -thread_should_run() returns
 false and the kthread goes to sleep and wait for the watchdog timer
 to wake it up. But the timer is not enabled since the user requested
 to disable the watchdog. We want the kthread to park instead of waiting
 for events that can't happen.

 As a result, later unpark requests after sysctl write through
 'sysctl -w kernel.watchdog=1' won't wake up/unpark the task as
 expected, since it's not parked anyway, leaving the value modified
 without triggering any action.
Out of curiosity, this can happen only for short period of time right?After
some time this will work right as the thread get back in action
after the schedule call.Then sysctl and procfs will work I think.

 We could workaround some solution in the watchdog code like forcing
 one pass to the thread function and immediately return to park.

 But supporting parking requests from -setup() or -unpark()
 callbacks look like proper way to implement cancellation in
 general. So let's fix it that way.

 Signed-off-by: Frederic Weisbecker fweis...@gmail.com
 Cc: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
 Cc: Steven Rostedt rost...@goodmis.org
 Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
 Cc: Ingo Molnar mi...@kernel.org
 Cc: Thomas Gleixner t...@linutronix.de
 Cc: Peter Zijlstra pet...@infradead.org
 Cc: Borislav Petkov b...@alien8.de
 Cc: Li Zhong zh...@linux.vnet.ibm.com
 Cc: Don Zickus dzic...@redhat.com
 ---
  kernel/smpboot.c |6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/kernel/smpboot.c b/kernel/smpboot.c
 index 02fc5c9..3394ed0 100644
 --- a/kernel/smpboot.c
 +++ b/kernel/smpboot.c
 @@ -151,6 +151,12 @@ static int smpboot_thread_fn(void *data)
 break;
 }

 +   /* Check if setup or unpark actually want us to park */
 +   if (kthread_should_stop() || kthread_should_park()) {
 +   preempt_enable();
 +   continue;
 +   }
 +
 if (!ht-thread_should_run(td-cpu)) {
 preempt_enable();
 schedule();
 --
 1.7.5.4

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

Re: [PATCH RESEND v3] bitmap: speed up bitmap_find_free_region

[anish singh]

On Tue, May 7, 2013 at 1:45 PM, Chanho Min  wrote:
> In bitmap_find_free_region, If we skip the all-ones words and find bits
> in a not-all-ones word, we can improve performance of it.
>
> For example, If bitmap_find_free_region() is called with order=0, First,
> It scans bitmap array by the increment of long type, then find 1 free bit
> within 1 long type value. In 32 bits system and 1024 bits size, in the worst
> case, We need 1024 for-loops to find 1 free bit. But, If this is applied,
> it takes 64 for-loops. Instead, It will be needed additional if-comparison
> of every word and It can take time slightly as 'Test case 3'. But, In many
> cases, It will speed up significantly.
>
> Test cases bellows show the time taken to execute bitmap_find_free_region()
> before and after patch.
>
> Test condition : ARMv7 1GHZ 32 bits system, 1024 bits size, gcc 4.6.2
>
> Test case 1: order is 0. all bits are one except that last one bit is zero.
>  before patch: 29727 ns
>  after patch: 2349 ns
How did you find out this time?Can  you please post your code
for that?
>
> Test case 2: order is 1. all bits are one except that last 2 contiguous bits
> are zero.
>  before patch: 15475 ns
>  after patch: 2225 ns
>
> Test case 3: order is 1. all words are not-all-ones and don't have 2 
> contiguous
> bits except that last 2 contiguous are zero.
>  before patch: 15475 ns
>  after patch: 16131 ns
>
> Changes compared to v1:
>  - Modified unnecessarily complicated code.
>  - Fixed the buggy code if `bits' is not an multiple of BITS_PER_LONG.
>
> Changes compared to v2:
>  - Removed bitmap_find_free_one and merged it to bitmap_find_free_region.
>
> Signed-off-by: Chanho Min 
> ---
>  lib/bitmap.c |   23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 06f7e4f..95e8efc 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1114,14 +1114,21 @@ done:
>   */
>  int bitmap_find_free_region(unsigned long *bitmap, int bits, int order)
>  {
> -   int pos, end;   /* scans bitmap by regions of size order */
> -
> -   for (pos = 0 ; (end = pos + (1 << order)) <= bits; pos = end) {
> -   if (!__reg_op(bitmap, pos, order, REG_OP_ISFREE))
> -   continue;
> -   __reg_op(bitmap, pos, order, REG_OP_ALLOC);
> -   return pos;
> -   }
> +   int pos, end, nbit, i;  /* scans bitmap by regions of size order */
> +   int nlongs = BITS_TO_LONGS(bits);
> +
> +   for (i = 0; i < nlongs; i++)
> +   if (bitmap[i] != ~0UL) {
> +   pos = i * BITS_PER_LONG;
> +   nbit = min(bits, pos + BITS_PER_LONG);
> +   for (; (end = pos + (1 << order)) <= nbit; pos = end) 
> {
> +   if (!__reg_op(bitmap, pos, order,
> +   REG_OP_ISFREE))
> +   continue;
> +   __reg_op(bitmap, pos, order, REG_OP_ALLOC);
> +   return pos;
> +   }
> +   }
> return -ENOMEM;
>  }
>  EXPORT_SYMBOL(bitmap_find_free_region);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Question pertaining to request_threaded_irq

[anish singh]

Hello Vijay,

Below I will try my best to answer.

On Wed, Mar 20, 2013 at 8:23 PM, Vijay Dixit  wrote:
>  Hello,
>
> I am new to the Kernel-Mailing list. I am not subscribed at the moment
> and would really appreciate it, if I can be CC'd for all the
> reply/responses for my question.
>
>
> I have searched all over the web but haven't found a convincing answer
> to a couple of related questions I have, with regard to the
> "request_threaded_irq" feature.
>
> QUESTION-1
> 
> Firstly, I was reading this article, regarding threaded IRQ's:
>
> http://lwn.net/Articles/302043/
>
> and there is this one line that isn't clear to me:
>
> "Converting an interrupt to threaded makes only sense when the handler
> code takes advantage of it by integrating tasklet/softirq
> functionality and simplifying the locking."
>
> I understand had we gone ahead with a "traditional", top half/bottom
> half approach, we would have needed either spin-locks or disable local
> IRQ to meddle with shared data. But, what I don't understand is, how
> would threaded interrupts simplify the need for locking by integrating
> tasklet/softirq functionality.
Some expert can help here.
>
>
> QUESTION-2
> 
> Secondly, what advantage (if any), does a request_threaded_handler
> approach have over a work_queue based bottom half approach ? In both
> cases it seems, as though the "work" is deferred to a dedicated
> thread. So, what is the difference ?
In request_threaded_irq we have top half and bottom half.
a. If bottom half dies then your irq will not be re-triggered.This will
save you from a lot of problems.
b. There is one more advantage but I am not clear about it so I will leave
for some expert to answer that.
"*   @thread_fn. This split handler design is necessary to support
 *shared interrupts."
kernel/irq/manage.c line no 1379 & 1380
>
>
> QUESTION-3
> 
> Lastly, in the following prototype:
>
> int request_threaded_irq(unsigned int irq, irq_handler_t handler,
> irq_handler_t thread_fn, unsigned long irqflags, const char *devname,
> void *dev_id)
>
> Is it possible that the "handler" part of the IRQ is continuously
> triggered by the relevant IRQ (say a UART receving characters at a
> high rate), even while the "thread_fn"(writing rx'd bytes to a
> circular buffer) part of the interrupt handler is busy processing
> IRQ's from previous wakeups ? So, wouldn't the handler be trying to
Yes it can happen but generally we prefer IRQS_ONESHOT along
with default primary handler set as irq_default_primary_handler().
As you would know, setting IRQS_ONESHOT flag means interrupt
is not reenabled after the hardirq handler finished and it is used by
Used by threaded interrupts which need to keep the irq line disabled
until the threaded handler has been run.

static irqreturn_t irq_default_primary_handler(int irq, void *dev_id)
{
   return IRQ_WAKE_THREAD;
}
So the problem described will not happen.There is one more code which
will avoid your said problem.

Whenever interrupt happens this function gets called and in the line
number 68 & 69 you can see that there is a check if the thread is currently
running or not.If running then this function will simply return.
RUNTHREAD bit is set when thread starts executing.

 54 static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 55 {
 56 /*
 57  * In case the thread crashed and was killed we just pretend that
 58  * we handled the interrupt. The hardirq handler has disabled the
 59  * device interrupt, so no irq storm is lurking.
 60  */
 61 if (action->thread->flags & PF_EXITING)
 62 return;
 63
 64 /*
 65  * Wake up the handler thread for this action. If the
 66  * RUNTHREAD bit is already set, nothing to do.
 67  */
 68 if (test_and_set_bit(IRQTF_RUNTHREAD, >thread_flags))
 69 return;

> "wakeup" an already running "thread_fn" ? How would the running irq
> "thread_fn" behave in that case ?
>
> I would really appreciate if someone can help me understand this.
>
> Thanks,
> vj
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Question pertaining to request_threaded_irq

[anish singh]

Hello Vijay,

Below I will try my best to answer.

On Wed, Mar 20, 2013 at 8:23 PM, Vijay Dixit thelonejo...@gmail.com wrote:
  Hello,

 I am new to the Kernel-Mailing list. I am not subscribed at the moment
 and would really appreciate it, if I can be CC'd for all the
 reply/responses for my question.


 I have searched all over the web but haven't found a convincing answer
 to a couple of related questions I have, with regard to the
 request_threaded_irq feature.

 QUESTION-1
 
 Firstly, I was reading this article, regarding threaded IRQ's:

 http://lwn.net/Articles/302043/

 and there is this one line that isn't clear to me:

 Converting an interrupt to threaded makes only sense when the handler
 code takes advantage of it by integrating tasklet/softirq
 functionality and simplifying the locking.

 I understand had we gone ahead with a traditional, top half/bottom
 half approach, we would have needed either spin-locks or disable local
 IRQ to meddle with shared data. But, what I don't understand is, how
 would threaded interrupts simplify the need for locking by integrating
 tasklet/softirq functionality.
Some expert can help here.


 QUESTION-2
 
 Secondly, what advantage (if any), does a request_threaded_handler
 approach have over a work_queue based bottom half approach ? In both
 cases it seems, as though the work is deferred to a dedicated
 thread. So, what is the difference ?
In request_threaded_irq we have top half and bottom half.
a. If bottom half dies then your irq will not be re-triggered.This will
save you from a lot of problems.
b. There is one more advantage but I am not clear about it so I will leave
for some expert to answer that.
*   @thread_fn. This split handler design is necessary to support
 *shared interrupts.
kernel/irq/manage.c line no 1379  1380


 QUESTION-3
 
 Lastly, in the following prototype:

 int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 irq_handler_t thread_fn, unsigned long irqflags, const char *devname,
 void *dev_id)

 Is it possible that the handler part of the IRQ is continuously
 triggered by the relevant IRQ (say a UART receving characters at a
 high rate), even while the thread_fn(writing rx'd bytes to a
 circular buffer) part of the interrupt handler is busy processing
 IRQ's from previous wakeups ? So, wouldn't the handler be trying to
Yes it can happen but generally we prefer IRQS_ONESHOT along
with default primary handler set as irq_default_primary_handler().
As you would know, setting IRQS_ONESHOT flag means interrupt
is not reenabled after the hardirq handler finished and it is used by
Used by threaded interrupts which need to keep the irq line disabled
until the threaded handler has been run.

static irqreturn_t irq_default_primary_handler(int irq, void *dev_id)
{
   return IRQ_WAKE_THREAD;
}
So the problem described will not happen.There is one more code which
will avoid your said problem.

Whenever interrupt happens this function gets called and in the line
number 68  69 you can see that there is a check if the thread is currently
running or not.If running then this function will simply return.
RUNTHREAD bit is set when thread starts executing.

 54 static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 55 {
 56 /*
 57  * In case the thread crashed and was killed we just pretend that
 58  * we handled the interrupt. The hardirq handler has disabled the
 59  * device interrupt, so no irq storm is lurking.
 60  */
 61 if (action-thread-flags  PF_EXITING)
 62 return;
 63
 64 /*
 65  * Wake up the handler thread for this action. If the
 66  * RUNTHREAD bit is already set, nothing to do.
 67  */
 68 if (test_and_set_bit(IRQTF_RUNTHREAD, action-thread_flags))
 69 return;

 wakeup an already running thread_fn ? How would the running irq
 thread_fn behave in that case ?

 I would really appreciate if someone can help me understand this.

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

Re: [PATCH] atomic: improve atomic_inc_unless_negative/atomic_dec_unless_positive

[anish singh]

On Tue, Mar 12, 2013 at 11:25 PM, Paul E. McKenney
 wrote:
> On Tue, Mar 12, 2013 at 04:02:47PM +0100, Frederic Weisbecker wrote:
>> 2013/3/12 Paul E. McKenney :
>> > On Tue, Mar 12, 2013 at 12:03:23PM +0800, Ming Lei wrote:
>> >> On Tue, Mar 12, 2013 at 11:39 AM, Paul E. McKenney
>> >>  wrote:
>> >> >
>> >> > Atomic operations that return a value are required to act as full
>> >> > memory
>> >> > barriers.  This means that code relying on ordering provided by
>> >> > these
>> >> > atomic operations must also do ordering, either by using an explicit
>> >> > memory barrier or by relying on guarantees from atomic operations.
>> >> >
>> >> > For example:
>> >> >
>> >> > CPU 0   CPU 1
>> >> >
>> >> > X = 1;  r1 = Z;
>> >> > if (atomic_inc_unless_negative()  smp_mb();
>> >> > do_something();
>> >> > Z = 1;  r2 = X;
>> >> >
>> >> > Assuming X and Z are initially zero, if r1==1, we are guaranteed
>> >> > that r2==1.  However, CPU 1 needs its smp_mb() in order to pair with
>> >> > the barrier implicit in atomic_inc_unless_negative().
>> >> >
>> >> > Make sense?
>> >>
>> >> Yes, it does, and thanks for the explanation.
>> >>
>> >> But looks the above example is not what Frederic described:
>> >>
>> >> "the above atomic_read() might return -1 because there is no
>> >> guarantee it's seeing the recent update on the remote CPU."
>> >>
>> >> Even I am not sure if adding one smp_mb() around atomic_read()
>> >> can guarantee that too.
>> >
>> > Frederic was likely thinking of some other scenario that would be
>> > broken by atomic_inc_unless_negative() failing to act as a full
>> > memory barrier.  Here is another example:
>> >
>> >
>> > CPU 0   CPU 1
>> >
>> > X = 1;
>> > if (atomic_inc_unless_negative()  r1 = atomic_xchg(,
>> > -1);
>> > r2 = X;
>> >
>> > If atomic_inc_unless_negative() acts as a full memory barrier, then
>> > if CPU 0 reaches the assignment from X, the results will be guaranteed
>> > to be 1.  Otherwise, there is no guarantee.
>>
>> Your scenarios show an interesting guarantee I did not think about.
>> But my concern was on such a situation:
>>
>>   CPU 0CPU 1
>>
>>   atomic_set(, -1)
>>atomic_inc()
>>   atomic_add_unless_negative(, 5)
>>
>> On the above situation, CPU 0 may still see X == -1 and thus not add
>> the 5. Of course all that only make sense with datas coming along.
>
> That could happen, but you would need CPU 1 to carry out some other
> reference for it to be a bug.  Otherwise, CPU 1's atomic_inc() just

  CPU 0CPU 1

  atomic_set(, -1)
A =5
= A
  atomic_add_unless_negative(, 5)

Do you mean this when you referred "carry out some other reference
for it to be a bug"?

> happened after all of CPU 0's code.  But yes, it would be possible
> to misorder with some larger scenario starting with this example.
> Especially given that atomic_inc() does not make any ordering guarantees.
>
> Thanx, Paul
>
> Thanx, Paul
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] atomic: improve atomic_inc_unless_negative/atomic_dec_unless_positive

[anish singh]

On Tue, Mar 12, 2013 at 11:25 PM, Paul E. McKenney
paul...@linux.vnet.ibm.com wrote:
 On Tue, Mar 12, 2013 at 04:02:47PM +0100, Frederic Weisbecker wrote:
 2013/3/12 Paul E. McKenney paul...@linux.vnet.ibm.com:
  On Tue, Mar 12, 2013 at 12:03:23PM +0800, Ming Lei wrote:
  On Tue, Mar 12, 2013 at 11:39 AM, Paul E. McKenney
  paul...@linux.vnet.ibm.com wrote:
  
   Atomic operations that return a value are required to act as full
   memory
   barriers.  This means that code relying on ordering provided by
   these
   atomic operations must also do ordering, either by using an explicit
   memory barrier or by relying on guarantees from atomic operations.
  
   For example:
  
   CPU 0   CPU 1
  
   X = 1;  r1 = Z;
   if (atomic_inc_unless_negative(Y)  smp_mb();
   do_something();
   Z = 1;  r2 = X;
  
   Assuming X and Z are initially zero, if r1==1, we are guaranteed
   that r2==1.  However, CPU 1 needs its smp_mb() in order to pair with
   the barrier implicit in atomic_inc_unless_negative().
  
   Make sense?
 
  Yes, it does, and thanks for the explanation.
 
  But looks the above example is not what Frederic described:
 
  the above atomic_read() might return -1 because there is no
  guarantee it's seeing the recent update on the remote CPU.
 
  Even I am not sure if adding one smp_mb() around atomic_read()
  can guarantee that too.
 
  Frederic was likely thinking of some other scenario that would be
  broken by atomic_inc_unless_negative() failing to act as a full
  memory barrier.  Here is another example:
 
 
  CPU 0   CPU 1
 
  X = 1;
  if (atomic_inc_unless_negative(Y)  r1 = atomic_xchg(Y,
  -1);
  r2 = X;
 
  If atomic_inc_unless_negative() acts as a full memory barrier, then
  if CPU 0 reaches the assignment from X, the results will be guaranteed
  to be 1.  Otherwise, there is no guarantee.

 Your scenarios show an interesting guarantee I did not think about.
 But my concern was on such a situation:

   CPU 0CPU 1

   atomic_set(X, -1)
atomic_inc(X)
   atomic_add_unless_negative(X, 5)

 On the above situation, CPU 0 may still see X == -1 and thus not add
 the 5. Of course all that only make sense with datas coming along.

 That could happen, but you would need CPU 1 to carry out some other
 reference for it to be a bug.  Otherwise, CPU 1's atomic_inc() just

  CPU 0CPU 1

  atomic_set(X, -1)
A =5
   X = A
  atomic_add_unless_negative(X, 5)

Do you mean this when you referred carry out some other reference
for it to be a bug?

 happened after all of CPU 0's code.  But yes, it would be possible
 to misorder with some larger scenario starting with this example.
 Especially given that atomic_inc() does not make any ordering guarantees.

 Thanx, Paul

 Thanx, Paul

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

Re: [PATCH] [Timer][Trivial] __clocksource_register_scale return value use?

[anish singh]

ACK or NACK this patch please.

On Sat, Mar 9, 2013 at 11:33 AM, anish singh
 wrote:
> ping
>
> On Thu, Mar 7, 2013 at 4:41 PM, anish kumar  
> wrote:
>> __clocksource_register_scale() currently returns int but it should
>> return void as there are no error paths in that function.
>> Making it void would help some amount of code to be removed at various
>> places.
>>
>> clocksource_register_hz/khz() return value is checked
>> in most of the places but I think it will translate to always
>> if(true) so let's remove those checks as well(patch will be sent
>> later for that).
>>
>> Is this return value for some future usecase(?), if yes then my
>> apologies.
>>
>> Signed-off-by: anish kumar 
>> ---
>>  include/linux/clocksource.h |6 +++---
>>  kernel/time/clocksource.c   |7 +--
>>  2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>> index 27cfda4..2b074cc 100644
>> --- a/include/linux/clocksource.h
>> +++ b/include/linux/clocksource.h
>> @@ -294,17 +294,17 @@ clocks_calc_mult_shift(u32 *mult, u32 *shift, u32
>> from, u32 to, u32 minsec);
>>   * Don't call __clocksource_register_scale directly, use
>>   * clocksource_register_hz/khz
>>   */
>> -extern int
>> +extern void
>>  __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
>> freq);
>>  extern void
>>  __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32
>> freq);
>>
>> -static inline int clocksource_register_hz(struct clocksource *cs, u32
>> hz)
>> +static inline void clocksource_register_hz(struct clocksource *cs, u32
>> hz)
>>  {
>> return __clocksource_register_scale(cs, 1, hz);
>>  }
>>
>> -static inline int clocksource_register_khz(struct clocksource *cs, u32
>> khz)
>> +static inline void clocksource_register_khz(struct clocksource *cs, u32
>> khz)
>>  {
>> return __clocksource_register_scale(cs, 1000, khz);
>>  }
>> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
>> index c958338..1915550 100644
>> --- a/kernel/time/clocksource.c
>> +++ b/kernel/time/clocksource.c
>> @@ -703,14 +703,11 @@ EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale);
>>   * @scale: Scale factor multiplied against freq to get clocksource hz
>>   * @freq:  clocksource frequency (cycles per second) divided by scale
>>   *
>> - * Returns -EBUSY if registration fails, zero otherwise.
>> - *
>>   * This *SHOULD NOT* be called directly! Please use the
>>   * clocksource_register_hz() or clocksource_register_khz helper
>> functions.
>>   */
>> -int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
>> freq)
>> +void __clocksource_register_scale(struct clocksource *cs, u32 scale,
>> u32 freq)
>>  {
>> -
>> /* Initialize mult/shift and max_idle_ns */
>> __clocksource_updatefreq_scale(cs, scale, freq);
>>
>> @@ -720,11 +717,9 @@ int __clocksource_register_scale(struct clocksource
>> *cs, u32 scale, u32 freq)
>> clocksource_enqueue_watchdog(cs);
>> clocksource_select();
>> mutex_unlock(_mutex);
>> -   return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(__clocksource_register_scale);
>>
>> -
>>  /**
>>   * clocksource_register - Used to install new clocksources
>>   * @cs:clocksource to be registered
>> --
>> 1.7.1
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Timer][Trivial] __clocksource_register_scale return value use?

[anish singh]

ACK or NACK this patch please.

On Sat, Mar 9, 2013 at 11:33 AM, anish singh
anish198519851...@gmail.com wrote:
 ping

 On Thu, Mar 7, 2013 at 4:41 PM, anish kumar anish198519851...@gmail.com 
 wrote:
 __clocksource_register_scale() currently returns int but it should
 return void as there are no error paths in that function.
 Making it void would help some amount of code to be removed at various
 places.

 clocksource_register_hz/khz() return value is checked
 in most of the places but I think it will translate to always
 if(true) so let's remove those checks as well(patch will be sent
 later for that).

 Is this return value for some future usecase(?), if yes then my
 apologies.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  include/linux/clocksource.h |6 +++---
  kernel/time/clocksource.c   |7 +--
  2 files changed, 4 insertions(+), 9 deletions(-)

 diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
 index 27cfda4..2b074cc 100644
 --- a/include/linux/clocksource.h
 +++ b/include/linux/clocksource.h
 @@ -294,17 +294,17 @@ clocks_calc_mult_shift(u32 *mult, u32 *shift, u32
 from, u32 to, u32 minsec);
   * Don't call __clocksource_register_scale directly, use
   * clocksource_register_hz/khz
   */
 -extern int
 +extern void
  __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
 freq);
  extern void
  __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32
 freq);

 -static inline int clocksource_register_hz(struct clocksource *cs, u32
 hz)
 +static inline void clocksource_register_hz(struct clocksource *cs, u32
 hz)
  {
 return __clocksource_register_scale(cs, 1, hz);
  }

 -static inline int clocksource_register_khz(struct clocksource *cs, u32
 khz)
 +static inline void clocksource_register_khz(struct clocksource *cs, u32
 khz)
  {
 return __clocksource_register_scale(cs, 1000, khz);
  }
 diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
 index c958338..1915550 100644
 --- a/kernel/time/clocksource.c
 +++ b/kernel/time/clocksource.c
 @@ -703,14 +703,11 @@ EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale);
   * @scale: Scale factor multiplied against freq to get clocksource hz
   * @freq:  clocksource frequency (cycles per second) divided by scale
   *
 - * Returns -EBUSY if registration fails, zero otherwise.
 - *
   * This *SHOULD NOT* be called directly! Please use the
   * clocksource_register_hz() or clocksource_register_khz helper
 functions.
   */
 -int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
 freq)
 +void __clocksource_register_scale(struct clocksource *cs, u32 scale,
 u32 freq)
  {
 -
 /* Initialize mult/shift and max_idle_ns */
 __clocksource_updatefreq_scale(cs, scale, freq);

 @@ -720,11 +717,9 @@ int __clocksource_register_scale(struct clocksource
 *cs, u32 scale, u32 freq)
 clocksource_enqueue_watchdog(cs);
 clocksource_select();
 mutex_unlock(clocksource_mutex);
 -   return 0;
  }
  EXPORT_SYMBOL_GPL(__clocksource_register_scale);

 -
  /**
   * clocksource_register - Used to install new clocksources
   * @cs:clocksource to be registered
 --
 1.7.1


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

Re: [PATCH] [Timer][Trivial] __clocksource_register_scale return value use?

[anish singh]

ping

On Thu, Mar 7, 2013 at 4:41 PM, anish kumar  wrote:
> __clocksource_register_scale() currently returns int but it should
> return void as there are no error paths in that function.
> Making it void would help some amount of code to be removed at various
> places.
>
> clocksource_register_hz/khz() return value is checked
> in most of the places but I think it will translate to always
> if(true) so let's remove those checks as well(patch will be sent
> later for that).
>
> Is this return value for some future usecase(?), if yes then my
> apologies.
>
> Signed-off-by: anish kumar 
> ---
>  include/linux/clocksource.h |6 +++---
>  kernel/time/clocksource.c   |7 +--
>  2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 27cfda4..2b074cc 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -294,17 +294,17 @@ clocks_calc_mult_shift(u32 *mult, u32 *shift, u32
> from, u32 to, u32 minsec);
>   * Don't call __clocksource_register_scale directly, use
>   * clocksource_register_hz/khz
>   */
> -extern int
> +extern void
>  __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
> freq);
>  extern void
>  __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32
> freq);
>
> -static inline int clocksource_register_hz(struct clocksource *cs, u32
> hz)
> +static inline void clocksource_register_hz(struct clocksource *cs, u32
> hz)
>  {
> return __clocksource_register_scale(cs, 1, hz);
>  }
>
> -static inline int clocksource_register_khz(struct clocksource *cs, u32
> khz)
> +static inline void clocksource_register_khz(struct clocksource *cs, u32
> khz)
>  {
> return __clocksource_register_scale(cs, 1000, khz);
>  }
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index c958338..1915550 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -703,14 +703,11 @@ EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale);
>   * @scale: Scale factor multiplied against freq to get clocksource hz
>   * @freq:  clocksource frequency (cycles per second) divided by scale
>   *
> - * Returns -EBUSY if registration fails, zero otherwise.
> - *
>   * This *SHOULD NOT* be called directly! Please use the
>   * clocksource_register_hz() or clocksource_register_khz helper
> functions.
>   */
> -int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
> freq)
> +void __clocksource_register_scale(struct clocksource *cs, u32 scale,
> u32 freq)
>  {
> -
> /* Initialize mult/shift and max_idle_ns */
> __clocksource_updatefreq_scale(cs, scale, freq);
>
> @@ -720,11 +717,9 @@ int __clocksource_register_scale(struct clocksource
> *cs, u32 scale, u32 freq)
> clocksource_enqueue_watchdog(cs);
> clocksource_select();
> mutex_unlock(_mutex);
> -   return 0;
>  }
>  EXPORT_SYMBOL_GPL(__clocksource_register_scale);
>
> -
>  /**
>   * clocksource_register - Used to install new clocksources
>   * @cs:clocksource to be registered
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Timer][Trivial] __clocksource_register_scale return value use?

[anish singh]

ping

On Thu, Mar 7, 2013 at 4:41 PM, anish kumar anish198519851...@gmail.com wrote:
 __clocksource_register_scale() currently returns int but it should
 return void as there are no error paths in that function.
 Making it void would help some amount of code to be removed at various
 places.

 clocksource_register_hz/khz() return value is checked
 in most of the places but I think it will translate to always
 if(true) so let's remove those checks as well(patch will be sent
 later for that).

 Is this return value for some future usecase(?), if yes then my
 apologies.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  include/linux/clocksource.h |6 +++---
  kernel/time/clocksource.c   |7 +--
  2 files changed, 4 insertions(+), 9 deletions(-)

 diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
 index 27cfda4..2b074cc 100644
 --- a/include/linux/clocksource.h
 +++ b/include/linux/clocksource.h
 @@ -294,17 +294,17 @@ clocks_calc_mult_shift(u32 *mult, u32 *shift, u32
 from, u32 to, u32 minsec);
   * Don't call __clocksource_register_scale directly, use
   * clocksource_register_hz/khz
   */
 -extern int
 +extern void
  __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
 freq);
  extern void
  __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32
 freq);

 -static inline int clocksource_register_hz(struct clocksource *cs, u32
 hz)
 +static inline void clocksource_register_hz(struct clocksource *cs, u32
 hz)
  {
 return __clocksource_register_scale(cs, 1, hz);
  }

 -static inline int clocksource_register_khz(struct clocksource *cs, u32
 khz)
 +static inline void clocksource_register_khz(struct clocksource *cs, u32
 khz)
  {
 return __clocksource_register_scale(cs, 1000, khz);
  }
 diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
 index c958338..1915550 100644
 --- a/kernel/time/clocksource.c
 +++ b/kernel/time/clocksource.c
 @@ -703,14 +703,11 @@ EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale);
   * @scale: Scale factor multiplied against freq to get clocksource hz
   * @freq:  clocksource frequency (cycles per second) divided by scale
   *
 - * Returns -EBUSY if registration fails, zero otherwise.
 - *
   * This *SHOULD NOT* be called directly! Please use the
   * clocksource_register_hz() or clocksource_register_khz helper
 functions.
   */
 -int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32
 freq)
 +void __clocksource_register_scale(struct clocksource *cs, u32 scale,
 u32 freq)
  {
 -
 /* Initialize mult/shift and max_idle_ns */
 __clocksource_updatefreq_scale(cs, scale, freq);

 @@ -720,11 +717,9 @@ int __clocksource_register_scale(struct clocksource
 *cs, u32 scale, u32 freq)
 clocksource_enqueue_watchdog(cs);
 clocksource_select();
 mutex_unlock(clocksource_mutex);
 -   return 0;
  }
  EXPORT_SYMBOL_GPL(__clocksource_register_scale);

 -
  /**
   * clocksource_register - Used to install new clocksources
   * @cs:clocksource to be registered
 --
 1.7.1


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

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

On Tue, Feb 19, 2013 at 2:18 AM, Don Zickus  wrote:
> On Sat, Feb 16, 2013 at 05:44:09PM +0530, anish kumar wrote:
>> From: anish kumar 
>>
>> This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
>> multifold.
>> Uses are:
>> 1. Check if smpboot_register_percpu_thread function passed.
>> 2. Makes sure that user enables and disables the watchdog in sequence
>>i.e. enable watchdog->disable watchdog->enable watchdog
>>Unlike enable watchdog->enable watchdog which is wrong.
>>
>> Signed-off-by: anish kumar 
>> ---
>>  kernel/watchdog.c |5 +
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 75a2ab3..8a20ebe 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int write,
>>   return ret;
>>
>>   set_sample_period();
>> + /*
>> +  * Watchdog threads shouldn't be enabled if they are
>> +  * disabled.'watchdog_disabled' variable check in
>
> Missing a 'The'  ^^^
>
> Other than that,
>
> Acked-by: Don Zickus 
Is this patch picked up?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

On Tue, Feb 19, 2013 at 2:18 AM, Don Zickus dzic...@redhat.com wrote:
 On Sat, Feb 16, 2013 at 05:44:09PM +0530, anish kumar wrote:
 From: anish kumar anish198519851...@gmail.com

 This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
 multifold.
 Uses are:
 1. Check if smpboot_register_percpu_thread function passed.
 2. Makes sure that user enables and disables the watchdog in sequence
i.e. enable watchdog-disable watchdog-enable watchdog
Unlike enable watchdog-enable watchdog which is wrong.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  kernel/watchdog.c |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)

 diff --git a/kernel/watchdog.c b/kernel/watchdog.c
 index 75a2ab3..8a20ebe 100644
 --- a/kernel/watchdog.c
 +++ b/kernel/watchdog.c
 @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int write,
   return ret;

   set_sample_period();
 + /*
 +  * Watchdog threads shouldn't be enabled if they are
 +  * disabled.'watchdog_disabled' variable check in

 Missing a 'The'  ^^^

 Other than that,

 Acked-by: Don Zickus dzic...@redhat.com
Is this patch picked up?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: I2C: Fix i2c fail problem when a process is terminated by a signal on octeon in 3.8

[anish singh]

On Tue, Feb 26, 2013 at 4:37 PM, Wolfram Sang  wrote:
> On Tue, Feb 26, 2013 at 11:02:17AM +0100, Jiri Kosina wrote:
>> On Fri, 22 Feb 2013, 송은봉 wrote:
>>
>> >
>> >
>> >
>> > I've been debugging the abnormal operation of i2c on octeon.
>> > If a process is terminated by signal in the middle of i2c operation,
>> > next i2c read operation which is done by another process was failed.
>> > So i changed to ignore signal in the middle of i2c operation.
>> > After that the problem was not reproduced.
>>
>> This is not really material directly for trivial.git. Adding maintainers
>> to CC.
>
> Yes, this should not go via trivial. Please resend to i2c list. Patch
> looks okay from a glimpse (and fixes an issue we have seen before and
> fixed the same way).
Just curios to know why reinitializing the i2c controller is a bad idea?
This fix looks perfect but if we know that there is only one i2c device
currently being controlled by controller.Can we(i know it is expensive
compare to the fix provided) not re-initialize the i2c-controller?

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

Re: I2C: Fix i2c fail problem when a process is terminated by a signal on octeon in 3.8

[anish singh]

On Tue, Feb 26, 2013 at 4:37 PM, Wolfram Sang w...@the-dreams.de wrote:
 On Tue, Feb 26, 2013 at 11:02:17AM +0100, Jiri Kosina wrote:
 On Fri, 22 Feb 2013, 송은봉 wrote:

 
 
 
  I've been debugging the abnormal operation of i2c on octeon.
  If a process is terminated by signal in the middle of i2c operation,
  next i2c read operation which is done by another process was failed.
  So i changed to ignore signal in the middle of i2c operation.
  After that the problem was not reproduced.

 This is not really material directly for trivial.git. Adding maintainers
 to CC.

 Yes, this should not go via trivial. Please resend to i2c list. Patch
 looks okay from a glimpse (and fixes an issue we have seen before and
 fixed the same way).
Just curios to know why reinitializing the i2c controller is a bad idea?
This fix looks perfect but if we know that there is only one i2c device
currently being controlled by controller.Can we(i know it is expensive
compare to the fix provided) not re-initialize the i2c-controller?

Thanks,

 Thanks,

Wolfram

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

Re: [patch] generic-adc-battery: forever loop in gab_remove()

[anish singh]

On Thu, Feb 14, 2013 at 2:06 PM, anish singh
 wrote:
> On Thu, Feb 14, 2013 at 12:56 PM, Dan Carpenter
>  wrote:
>> There is a forever loop calling iio_channel_release() because the
>> "chan < " part of the "chan < ARRAY_SIZE()" is missing.  This is in both
>> the error handling on probe and also in the remove function.
>>
>> The other thing is that it's possible for some of the elements of the
>> adc_bat->channel[chan] array to be an ERR_PTR().  I've changed them to
>> be NULL instead.  We're still not allowed to pass NULLs to
>> iio_channel_release() so I've added a check.
>>
>> Finally, I removed an unused "chan = ARRAY_SIZE(gab_chan_name);"
>> statement as a small cleanup.
>>
>> Signed-off-by: Dan Carpenter 
I think this will be picked by Anton.
>
>>
>> diff --git a/drivers/power/generic-adc-battery.c 
>> b/drivers/power/generic-adc-battery.c
>> index 42733c4..8cb5d7f 100644
>> --- a/drivers/power/generic-adc-battery.c
>> +++ b/drivers/power/generic-adc-battery.c
>> @@ -263,9 +263,6 @@ static int gab_probe(struct platform_device *pdev)
>> psy->external_power_changed = gab_ext_power_changed;
>> adc_bat->pdata = pdata;
>>
>> -   /* calculate the total number of channels */
>> -   chan = ARRAY_SIZE(gab_chan_name);
>> -
>> /*
>>  * copying the static properties and allocating extra memory for 
>> holding
>>  * the extra configurable properties received from platform data.
>> @@ -291,6 +288,7 @@ static int gab_probe(struct platform_device *pdev)
>>  
>> gab_chan_name[chan]);
>> if (IS_ERR(adc_bat->channel[chan])) {
>> ret = PTR_ERR(adc_bat->channel[chan]);
>> +   adc_bat->channel[chan] = NULL;
>> } else {
>> /* copying properties for supported channels only */
>> memcpy(properties + sizeof(*(psy->properties)) * 
>> index,
>> @@ -344,8 +342,10 @@ err_gpio:
>>  gpio_req_fail:
>> power_supply_unregister(psy);
>>  err_reg_fail:
>> -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
>> -   iio_channel_release(adc_bat->channel[chan]);
>> +   for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
>> +   if (adc_bat->channel[chan])
>> +   iio_channel_release(adc_bat->channel[chan]);
>> +   }
>>  second_mem_fail:
>> kfree(psy->properties);
>>  first_mem_fail:
>> @@ -365,8 +365,10 @@ static int gab_remove(struct platform_device *pdev)
>> gpio_free(pdata->gpio_charge_finished);
>> }
>>
>> -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
>> -   iio_channel_release(adc_bat->channel[chan]);
>> +   for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
>> +   if (adc_bat->channel[chan])
>> +   iio_channel_release(adc_bat->channel[chan]);
>> +   }
>>
>> kfree(adc_bat->psy.properties);
>> cancel_delayed_work(_bat->bat_work);
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] generic-adc-battery: forever loop in gab_remove()

[anish singh]

On Thu, Feb 14, 2013 at 12:56 PM, Dan Carpenter
 wrote:
> There is a forever loop calling iio_channel_release() because the
> "chan < " part of the "chan < ARRAY_SIZE()" is missing.  This is in both
> the error handling on probe and also in the remove function.
>
> The other thing is that it's possible for some of the elements of the
> adc_bat->channel[chan] array to be an ERR_PTR().  I've changed them to
> be NULL instead.  We're still not allowed to pass NULLs to
> iio_channel_release() so I've added a check.
>
> Finally, I removed an unused "chan = ARRAY_SIZE(gab_chan_name);"
> statement as a small cleanup.
>
> Signed-off-by: Dan Carpenter 

>
> diff --git a/drivers/power/generic-adc-battery.c 
> b/drivers/power/generic-adc-battery.c
> index 42733c4..8cb5d7f 100644
> --- a/drivers/power/generic-adc-battery.c
> +++ b/drivers/power/generic-adc-battery.c
> @@ -263,9 +263,6 @@ static int gab_probe(struct platform_device *pdev)
> psy->external_power_changed = gab_ext_power_changed;
> adc_bat->pdata = pdata;
>
> -   /* calculate the total number of channels */
> -   chan = ARRAY_SIZE(gab_chan_name);
> -
> /*
>  * copying the static properties and allocating extra memory for 
> holding
>  * the extra configurable properties received from platform data.
> @@ -291,6 +288,7 @@ static int gab_probe(struct platform_device *pdev)
>  gab_chan_name[chan]);
> if (IS_ERR(adc_bat->channel[chan])) {
> ret = PTR_ERR(adc_bat->channel[chan]);
> +   adc_bat->channel[chan] = NULL;
> } else {
> /* copying properties for supported channels only */
> memcpy(properties + sizeof(*(psy->properties)) * 
> index,
> @@ -344,8 +342,10 @@ err_gpio:
>  gpio_req_fail:
> power_supply_unregister(psy);
>  err_reg_fail:
> -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
> -   iio_channel_release(adc_bat->channel[chan]);
> +   for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
> +   if (adc_bat->channel[chan])
> +   iio_channel_release(adc_bat->channel[chan]);
> +   }
>  second_mem_fail:
> kfree(psy->properties);
>  first_mem_fail:
> @@ -365,8 +365,10 @@ static int gab_remove(struct platform_device *pdev)
> gpio_free(pdata->gpio_charge_finished);
> }
>
> -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
> -   iio_channel_release(adc_bat->channel[chan]);
> +   for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
> +   if (adc_bat->channel[chan])
> +   iio_channel_release(adc_bat->channel[chan]);
> +   }
>
> kfree(adc_bat->psy.properties);
> cancel_delayed_work(_bat->bat_work);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] generic-adc-battery: forever loop in gab_remove()

[anish singh]

On Thu, Feb 14, 2013 at 12:56 PM, Dan Carpenter
dan.carpen...@oracle.com wrote:
 There is a forever loop calling iio_channel_release() because the
 chan   part of the chan  ARRAY_SIZE() is missing.  This is in both
 the error handling on probe and also in the remove function.

 The other thing is that it's possible for some of the elements of the
 adc_bat-channel[chan] array to be an ERR_PTR().  I've changed them to
 be NULL instead.  We're still not allowed to pass NULLs to
 iio_channel_release() so I've added a check.

 Finally, I removed an unused chan = ARRAY_SIZE(gab_chan_name);
 statement as a small cleanup.

 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com


 diff --git a/drivers/power/generic-adc-battery.c 
 b/drivers/power/generic-adc-battery.c
 index 42733c4..8cb5d7f 100644
 --- a/drivers/power/generic-adc-battery.c
 +++ b/drivers/power/generic-adc-battery.c
 @@ -263,9 +263,6 @@ static int gab_probe(struct platform_device *pdev)
 psy-external_power_changed = gab_ext_power_changed;
 adc_bat-pdata = pdata;

 -   /* calculate the total number of channels */
 -   chan = ARRAY_SIZE(gab_chan_name);
 -
 /*
  * copying the static properties and allocating extra memory for 
 holding
  * the extra configurable properties received from platform data.
 @@ -291,6 +288,7 @@ static int gab_probe(struct platform_device *pdev)
  gab_chan_name[chan]);
 if (IS_ERR(adc_bat-channel[chan])) {
 ret = PTR_ERR(adc_bat-channel[chan]);
 +   adc_bat-channel[chan] = NULL;
 } else {
 /* copying properties for supported channels only */
 memcpy(properties + sizeof(*(psy-properties)) * 
 index,
 @@ -344,8 +342,10 @@ err_gpio:
  gpio_req_fail:
 power_supply_unregister(psy);
  err_reg_fail:
 -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
 -   iio_channel_release(adc_bat-channel[chan]);
 +   for (chan = 0; chan  ARRAY_SIZE(gab_chan_name); chan++) {
 +   if (adc_bat-channel[chan])
 +   iio_channel_release(adc_bat-channel[chan]);
 +   }
  second_mem_fail:
 kfree(psy-properties);
  first_mem_fail:
 @@ -365,8 +365,10 @@ static int gab_remove(struct platform_device *pdev)
 gpio_free(pdata-gpio_charge_finished);
 }

 -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
 -   iio_channel_release(adc_bat-channel[chan]);
 +   for (chan = 0; chan  ARRAY_SIZE(gab_chan_name); chan++) {
 +   if (adc_bat-channel[chan])
 +   iio_channel_release(adc_bat-channel[chan]);
 +   }

 kfree(adc_bat-psy.properties);
 cancel_delayed_work(adc_bat-bat_work);
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] generic-adc-battery: forever loop in gab_remove()

[anish singh]

On Thu, Feb 14, 2013 at 2:06 PM, anish singh
anish198519851...@gmail.com wrote:
 On Thu, Feb 14, 2013 at 12:56 PM, Dan Carpenter
 dan.carpen...@oracle.com wrote:
 There is a forever loop calling iio_channel_release() because the
 chan   part of the chan  ARRAY_SIZE() is missing.  This is in both
 the error handling on probe and also in the remove function.

 The other thing is that it's possible for some of the elements of the
 adc_bat-channel[chan] array to be an ERR_PTR().  I've changed them to
 be NULL instead.  We're still not allowed to pass NULLs to
 iio_channel_release() so I've added a check.

 Finally, I removed an unused chan = ARRAY_SIZE(gab_chan_name);
 statement as a small cleanup.

 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
I think this will be picked by Anton.


 diff --git a/drivers/power/generic-adc-battery.c 
 b/drivers/power/generic-adc-battery.c
 index 42733c4..8cb5d7f 100644
 --- a/drivers/power/generic-adc-battery.c
 +++ b/drivers/power/generic-adc-battery.c
 @@ -263,9 +263,6 @@ static int gab_probe(struct platform_device *pdev)
 psy-external_power_changed = gab_ext_power_changed;
 adc_bat-pdata = pdata;

 -   /* calculate the total number of channels */
 -   chan = ARRAY_SIZE(gab_chan_name);
 -
 /*
  * copying the static properties and allocating extra memory for 
 holding
  * the extra configurable properties received from platform data.
 @@ -291,6 +288,7 @@ static int gab_probe(struct platform_device *pdev)
  
 gab_chan_name[chan]);
 if (IS_ERR(adc_bat-channel[chan])) {
 ret = PTR_ERR(adc_bat-channel[chan]);
 +   adc_bat-channel[chan] = NULL;
 } else {
 /* copying properties for supported channels only */
 memcpy(properties + sizeof(*(psy-properties)) * 
 index,
 @@ -344,8 +342,10 @@ err_gpio:
  gpio_req_fail:
 power_supply_unregister(psy);
  err_reg_fail:
 -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
 -   iio_channel_release(adc_bat-channel[chan]);
 +   for (chan = 0; chan  ARRAY_SIZE(gab_chan_name); chan++) {
 +   if (adc_bat-channel[chan])
 +   iio_channel_release(adc_bat-channel[chan]);
 +   }
  second_mem_fail:
 kfree(psy-properties);
  first_mem_fail:
 @@ -365,8 +365,10 @@ static int gab_remove(struct platform_device *pdev)
 gpio_free(pdata-gpio_charge_finished);
 }

 -   for (chan = 0; ARRAY_SIZE(gab_chan_name); chan++)
 -   iio_channel_release(adc_bat-channel[chan]);
 +   for (chan = 0; chan  ARRAY_SIZE(gab_chan_name); chan++) {
 +   if (adc_bat-channel[chan])
 +   iio_channel_release(adc_bat-channel[chan]);
 +   }

 kfree(adc_bat-psy.properties);
 cancel_delayed_work(adc_bat-bat_work);
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] driver core: add wait event for deferred probe

[anish singh]

On Thu, Feb 14, 2013 at 3:06 AM, Grant Likely  wrote:
> On Tue, 12 Feb 2013 10:52:10 +0800, Haojian Zhuang 
>  wrote:
>> On 12 February 2013 07:10, Andrew Morton  wrote:
>> > On Sun, 10 Feb 2013 00:57:57 +0800
>> > Haojian Zhuang  wrote:
>> >
>> >> do_initcalls() could call all driver initialization code in kernel_init
>> >> thread. It means that probe() function will be also called from that
>> >> time. After this, kernel could access console & release __init section
>> >> in the same thread.
>> >>
>> >> But if device probe fails and moves into deferred probe list, a new
>> >> thread is created to reinvoke probe. If the device is serial console,
>> >> kernel has to open console failure & release __init section before
>> >> reinvoking failure. Because there's no synchronization mechanism.
>> >> Now add wait event to synchronize after do_initcalls().
>> >
>> > It sounds like this:
>> >
>> > static int __ref kernel_init(void *unused)
>> > {
>> > kernel_init_freeable();
>> > /* need to finish all async __init code before freeing the memory 
>> > */
>> > async_synchronize_full();
>> >
>> > is designed to prevent the problem you describe?
>> >
>> It can't prevent the problem that I described. Because deferred_probe()
>> is introduced recently.
>>
>> All synchronization should be finished just after do_initcalls(). Since
>> load_default_modules() is also called in the end of kernel_init_freeable(),
>> I'm not sure that whether I could remove async_synchronize_full()
>> here. So I didn't touch it.
>>
>> >> --- a/init/main.c
>> >> +++ b/init/main.c
>> >> @@ -786,6 +786,7 @@ static void __init do_basic_setup(void)
>> >>   do_ctors();
>> >>   usermodehelper_enable();
>> >>   do_initcalls();
>> >> + wait_for_device_probe();
>> >>  }
>> >
>> > Needs a nice comment here explaining what's going on.
>>
>> No problem. I'll add comment here.
>
> Actually, this approach will create new problems. There is no guarantee
> that a given device will be able to initialize before exiting
> do_basic_setup(). If, for instance, a device depends on a resource
> provided by a module, then it will just keep deferring. In that case
> you've got a hung kernel.
>
> I think what you really want is the following:
>
>  static int deferred_probe_initcall(void)
>  {
> deferred_wq = create_singlethread_workqueue("deferwq");
> if (WARN_ON(!deferred_wq))
> return -ENOMEM;
>
> driver_deferred_probe_enable = true;
> +   deferred_probe_work_func(NULL);
> -   driver_deferred_probe_trigger();
> return 0;
>  }
>  late_initcall(deferred_probe_initcall);
>
> Or something similar. That would guarantee that as many passes as are needed
> (which in practical terms only means a couple) for device probing to
> settle down before exiting the initcall processing. That should achieve
> the effect you desire.
>
> It still masks the __init section issue by making it a lot less likely,
Grant, Can you please explain me this problem?My understanding is below:
If all the detection of devices with there respective driver is done before
__init section is freed then we will not have the problem mentioned.
However if the driver requests the probing to be deferred then __init section
of the deferred driver will not be freed right?

I am afraid but the patch description is bit cryptic for me specially
this line "kernel has to open console failure & release __init section before
reinvoking failure".

> but it does ensure that all of the built-in driver dependency order
> issues are processed before continuing on to userspace.
>
> g.
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

On Wed, Feb 13, 2013 at 2:51 PM, Ingo Molnar  wrote:
>
> * anish singh  wrote:
>
>> Is the below patch picked up?
>>
>> On Sun, Feb 3, 2013 at 9:31 PM, anish kumar  
>> wrote:
>> > From: anish kumar 
>> >
>> > This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
>> > multifold.
>> > Uses are:
>> > 1. Check if smpboot_register_percpu_thread function passed.
>> > 2. Makes sure that user enables and disables the watchdog in sequence
>> >i.e. enable watchdog->disable watchdog->enable watchdog
>> >Unlike enable watchdog->enable watchdog which is wrong.
>> >
>> > Signed-off-by: anish kumar 
>> > ---
>> >  kernel/watchdog.c |5 +
>> >  1 files changed, 5 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> > index 75a2ab3..87a19aa 100644
>> > --- a/kernel/watchdog.c
>> > +++ b/kernel/watchdog.c
>> > @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int 
>> > write,
>> > return ret;
>> >
>> > set_sample_period();
>> > +   /*
>> > +* We shouldn't enable watchdog threads if it is
>> > +* disabled.This is done by watchdog_disabled
>> > +* variable check in watchdog_*_all_cpus function.
>
> It has two grammatic and a stylistic error in it, plus misses
Would you mind pointing it out to me the grammatical mistakes
as I am not that good with grammar.
I thought I followed the conventions as below:
/*
 *
 *
 */
> the convention that function names are mentioned with a '()'.
>
> Thanks,
>
> Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

On Wed, Feb 13, 2013 at 2:51 PM, Ingo Molnar mi...@kernel.org wrote:

 * anish singh anish198519851...@gmail.com wrote:

 Is the below patch picked up?

 On Sun, Feb 3, 2013 at 9:31 PM, anish kumar anish198519851...@gmail.com 
 wrote:
  From: anish kumar anish198519851...@gmail.com
 
  This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
  multifold.
  Uses are:
  1. Check if smpboot_register_percpu_thread function passed.
  2. Makes sure that user enables and disables the watchdog in sequence
 i.e. enable watchdog-disable watchdog-enable watchdog
 Unlike enable watchdog-enable watchdog which is wrong.
 
  Signed-off-by: anish kumar anish198519851...@gmail.com
  ---
   kernel/watchdog.c |5 +
   1 files changed, 5 insertions(+), 0 deletions(-)
 
  diff --git a/kernel/watchdog.c b/kernel/watchdog.c
  index 75a2ab3..87a19aa 100644
  --- a/kernel/watchdog.c
  +++ b/kernel/watchdog.c
  @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int 
  write,
  return ret;
 
  set_sample_period();
  +   /*
  +* We shouldn't enable watchdog threads if it is
  +* disabled.This is done by watchdog_disabled
  +* variable check in watchdog_*_all_cpus function.

 It has two grammatic and a stylistic error in it, plus misses
Would you mind pointing it out to me the grammatical mistakes
as I am not that good with grammar.
I thought I followed the conventions as below:
/*
 *
 *
 */
 the convention that function names are mentioned with a '()'.

 Thanks,

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

Re: [PATCH] driver core: add wait event for deferred probe

[anish singh]

On Thu, Feb 14, 2013 at 3:06 AM, Grant Likely grant.lik...@secretlab.ca wrote:
 On Tue, 12 Feb 2013 10:52:10 +0800, Haojian Zhuang 
 haojian.zhu...@linaro.org wrote:
 On 12 February 2013 07:10, Andrew Morton a...@linux-foundation.org wrote:
  On Sun, 10 Feb 2013 00:57:57 +0800
  Haojian Zhuang haojian.zhu...@linaro.org wrote:
 
  do_initcalls() could call all driver initialization code in kernel_init
  thread. It means that probe() function will be also called from that
  time. After this, kernel could access console  release __init section
  in the same thread.
 
  But if device probe fails and moves into deferred probe list, a new
  thread is created to reinvoke probe. If the device is serial console,
  kernel has to open console failure  release __init section before
  reinvoking failure. Because there's no synchronization mechanism.
  Now add wait event to synchronize after do_initcalls().
 
  It sounds like this:
 
  static int __ref kernel_init(void *unused)
  {
  kernel_init_freeable();
  /* need to finish all async __init code before freeing the memory 
  */
  async_synchronize_full();
 
  is designed to prevent the problem you describe?
 
 It can't prevent the problem that I described. Because deferred_probe()
 is introduced recently.

 All synchronization should be finished just after do_initcalls(). Since
 load_default_modules() is also called in the end of kernel_init_freeable(),
 I'm not sure that whether I could remove async_synchronize_full()
 here. So I didn't touch it.

  --- a/init/main.c
  +++ b/init/main.c
  @@ -786,6 +786,7 @@ static void __init do_basic_setup(void)
do_ctors();
usermodehelper_enable();
do_initcalls();
  + wait_for_device_probe();
   }
 
  Needs a nice comment here explaining what's going on.

 No problem. I'll add comment here.

 Actually, this approach will create new problems. There is no guarantee
 that a given device will be able to initialize before exiting
 do_basic_setup(). If, for instance, a device depends on a resource
 provided by a module, then it will just keep deferring. In that case
 you've got a hung kernel.

 I think what you really want is the following:

  static int deferred_probe_initcall(void)
  {
 deferred_wq = create_singlethread_workqueue(deferwq);
 if (WARN_ON(!deferred_wq))
 return -ENOMEM;

 driver_deferred_probe_enable = true;
 +   deferred_probe_work_func(NULL);
 -   driver_deferred_probe_trigger();
 return 0;
  }
  late_initcall(deferred_probe_initcall);

 Or something similar. That would guarantee that as many passes as are needed
 (which in practical terms only means a couple) for device probing to
 settle down before exiting the initcall processing. That should achieve
 the effect you desire.

 It still masks the __init section issue by making it a lot less likely,
Grant, Can you please explain me this problem?My understanding is below:
If all the detection of devices with there respective driver is done before
__init section is freed then we will not have the problem mentioned.
However if the driver requests the probing to be deferred then __init section
of the deferred driver will not be freed right?

I am afraid but the patch description is bit cryptic for me specially
this line kernel has to open console failure  release __init section before
reinvoking failure.

 but it does ensure that all of the built-in driver dependency order
 issues are processed before continuing on to userspace.

 g.

 --
 Grant Likely, B.Sc, P.Eng.
 Secret Lab Technologies, Ltd.
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

Is the below patch picked up?

On Sun, Feb 3, 2013 at 9:31 PM, anish kumar  wrote:
> From: anish kumar 
>
> This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
> multifold.
> Uses are:
> 1. Check if smpboot_register_percpu_thread function passed.
> 2. Makes sure that user enables and disables the watchdog in sequence
>i.e. enable watchdog->disable watchdog->enable watchdog
>Unlike enable watchdog->enable watchdog which is wrong.
>
> Signed-off-by: anish kumar 
> ---
>  kernel/watchdog.c |5 +
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 75a2ab3..87a19aa 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int write,
> return ret;
>
> set_sample_period();
> +   /*
> +* We shouldn't enable watchdog threads if it is
> +* disabled.This is done by watchdog_disabled
> +* variable check in watchdog_*_all_cpus function.
> +*/
> if (watchdog_enabled && watchdog_thresh)
> watchdog_enable_all_cpus();
> else
> --
> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [Watchdog][Trivial] Added comments to explain watchdog_disabled variable

[anish singh]

Is the below patch picked up?

On Sun, Feb 3, 2013 at 9:31 PM, anish kumar anish198519851...@gmail.com wrote:
 From: anish kumar anish198519851...@gmail.com

 This watchdog_disabled flag is bit of cryptic.Howerver it's usefullnes is 
 multifold.
 Uses are:
 1. Check if smpboot_register_percpu_thread function passed.
 2. Makes sure that user enables and disables the watchdog in sequence
i.e. enable watchdog-disable watchdog-enable watchdog
Unlike enable watchdog-enable watchdog which is wrong.

 Signed-off-by: anish kumar anish198519851...@gmail.com
 ---
  kernel/watchdog.c |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)

 diff --git a/kernel/watchdog.c b/kernel/watchdog.c
 index 75a2ab3..87a19aa 100644
 --- a/kernel/watchdog.c
 +++ b/kernel/watchdog.c
 @@ -518,6 +518,11 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 return ret;

 set_sample_period();
 +   /*
 +* We shouldn't enable watchdog threads if it is
 +* disabled.This is done by watchdog_disabled
 +* variable check in watchdog_*_all_cpus function.
 +*/
 if (watchdog_enabled  watchdog_thresh)
 watchdog_enable_all_cpus();
 else
 --
 1.7.1

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

Re: [PATCH V3] suspend: enable freeze timeout configuration through sys

[anish singh]

On Thu, Jan 31, 2013 at 2:43 PM, Li, Fei  wrote:
>> -Original Message-
>> From: Yasuaki Ishimatsu [mailto:isimatu.yasu...@jp.fujitsu.com]
>> Sent: Thursday, January 31, 2013 3:30 PM
>> To: Li, Fei
>> Cc: r...@sisk.pl; a...@linux-foundation.org; linux-kernel@vger.kernel.org;
>> linux...@vger.kernel.org; Liu, Chuansheng
>> Subject: Re: [PATCH V3] suspend: enable freeze timeout configuration through
>> sys
>>
>> 2013/01/31 13:55, fli24 wrote:
>> >
>> > At present, the value of timeout for freezing is 20s, which is
>> > meaningless in case that one thread is frozen with mutex locked
>> > and another thread is trying to lock the mutex, as this time of
>> > freezing will fail unavoidably.
>> > And if there is no new wakeup event registered, the system will
>> > waste at most 20s for such meaningless trying of freezing.
>> >
>> > With this patch, the value of timeout can be configured to smaller
>> > value, so such meaningless trying of freezing will be aborted in
>> > earlier time, and later freezing can be also triggered in earlier
>> > time. And more power will be saved.
>> > In normal case on mobile phone, it costs real little time to freeze
>> > processes. On some platform, it only costs about 20ms to freeze
>> > user space processes and 10ms to freeze kernel freezable threads.
>> >
>> > Signed-off-by: Liu Chuansheng 
>> > Signed-off-by: Li Fei 
>> > ---
>> >   Documentation/power/freezing-of-tasks.txt |5 +
>> >   include/linux/freezer.h   |5 +
>> >   kernel/power/main.c   |   27
>> +++
>> >   kernel/power/process.c|4 ++--
>> >   4 files changed, 39 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/Documentation/power/freezing-of-tasks.txt
>> b/Documentation/power/freezing-of-tasks.txt
>> > index 6ec291e..85894d8 100644
>> > --- a/Documentation/power/freezing-of-tasks.txt
>> > +++ b/Documentation/power/freezing-of-tasks.txt
>> > @@ -223,3 +223,8 @@ since they ask the freezer to skip freezing this task,
>> since it is anyway
>> >   only after the entire suspend/hibernation sequence is complete.
>> >   So, to summarize, use [un]lock_system_sleep() instead of directly using
>> >   mutex_[un]lock(_mutex). That would prevent freezing failures.
>> > +
>> > +V. Miscellaneous
>> > +/sys/power/pm_freeze_timeout controls how long it will cost at most to
>> freeze
>> > +all user space processes or all freezable kernel threads, in unit of 
>> > millisecond.
>> > +The default value is 2, with range of unsigned integer.
>> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
>> > index e4238ce..5a24a33 100644
>> > --- a/include/linux/freezer.h
>> > +++ b/include/linux/freezer.h
>> > @@ -13,6 +13,11 @@ extern bool pm_freezing; /* PM freezing in 
>> > effect
>> */
>> >   extern bool pm_nosig_freezing;/* PM nosig freezing in effect 
>> > */
>> >
>> >   /*
>> > + * Timeout for stopping processes
>> > + */
>> > +extern unsigned int sys_freeze_process_timeout_msecs;
>> > +
>> > +/*
>> >* Check if a process has been frozen
>> >*/
>> >   static inline bool frozen(struct task_struct *p)
>> > diff --git a/kernel/power/main.c b/kernel/power/main.c
>> > index 1c16f91..453ead1 100644
>> > --- a/kernel/power/main.c
>> > +++ b/kernel/power/main.c
>> > @@ -553,6 +553,30 @@ power_attr(pm_trace_dev_match);
>> >
>> >   #endif /* CONFIG_PM_TRACE */
>> >
>> > +#ifdef CONFIG_FREEZER
>> > +static ssize_t pm_freeze_timeout_show(struct kobject *kobj,
>> > + struct kobj_attribute *attr, char *buf)
>> > +{
>> > +   return sprintf(buf, "%u\n", sys_freeze_process_timeout_msecs);
>> > +}
>> > +
>> > +static ssize_t pm_freeze_timeout_store(struct kobject *kobj,
>> > +  struct kobj_attribute *attr,
>> > +  const char *buf, size_t n)
>> > +{
>> > +   unsigned long val;
>> > +
>> > +   if (kstrtoul(buf, 10, ))
>> > +   return -EINVAL;
>> > +
>> > +   sys_freeze_process_timeout_msecs = val;
>> > +   return n;
>> > +}
>> > +
>> > +power_attr(pm_freeze_timeout);
>> > +
>> > +#endif /* CONFIG_FREEZER*/
>> > +
>> >   static struct attribute * g[] = {
>> > _attr.attr,
>> >   #ifdef CONFIG_PM_TRACE
>> > @@ -576,6 +600,9 @@ static struct attribute * g[] = {
>> > _print_times_attr.attr,
>> >   #endif
>> >   #endif
>> > +#ifdef CONFIG_FREEZER
>> > +   _freeze_timeout_attr.attr,
>> > +#endif
>> > NULL,
>> >   };
>> >
>> > diff --git a/kernel/power/process.c b/kernel/power/process.c
>> > index d5a258b..ba45a26 100644
>> > --- a/kernel/power/process.c
>> > +++ b/kernel/power/process.c
>> > @@ -21,7 +21,7 @@
>> >   /*
>> >* Timeout for stopping processes
>> >*/
>>
>> > -#define TIMEOUT(20 * HZ)
>> > +unsigned int __read_mostly sys_freeze_process_timeout_msecs = 2;
>>
>> 2 does not mean 20 seconds since we can select HZ other than 1000.
>> So (20 * HZ) is better than 2.
>>
>