Re: Linux 4.7: Reported regressions as of Sunday, 2016-06-26

[Kalle Valo]

Thorsten Leemhuis  writes:

> Hi! Here is my third regression report for Linux 4.7. It has 
> 24 entries; 9 of them are new; 4 regressions (not included here)
> were fixed since the last report(¹). 
>
> Please let me know if a regression is missing in the list; or if
> there is something on the list which shouldn't be there.

Suspend to ram has been broken on my Thinkpad x230 since v4.7-rc1.
Finally found the time and bisected it to this:

22e2f9fa63b0 iommu/vt-d: Use per-cpu IOVA caching

And after some googling found a fix and verified that it fixes the issue
for me:

iommu/vt-d: fix overflow of iommu->domains array
https://patchwork.kernel.org/patch/9158085/

The patch is not in Linus' tree yet, hopefully will be soon.

Thorsten, your regression list is _very_ useful and saves a lot of time.
I hope you have the time and energy to continue maintaining it.

-- 
Kalle Valo

Re: Linux 4.7: Reported regressions as of Sunday, 2016-06-26

[Kalle Valo]

Thorsten Leemhuis  writes:

> Hi! Here is my third regression report for Linux 4.7. It has 
> 24 entries; 9 of them are new; 4 regressions (not included here)
> were fixed since the last report(¹). 
>
> Please let me know if a regression is missing in the list; or if
> there is something on the list which shouldn't be there.

Suspend to ram has been broken on my Thinkpad x230 since v4.7-rc1.
Finally found the time and bisected it to this:

22e2f9fa63b0 iommu/vt-d: Use per-cpu IOVA caching

And after some googling found a fix and verified that it fixes the issue
for me:

iommu/vt-d: fix overflow of iommu->domains array
https://patchwork.kernel.org/patch/9158085/

The patch is not in Linus' tree yet, hopefully will be soon.

Thorsten, your regression list is _very_ useful and saves a lot of time.
I hope you have the time and energy to continue maintaining it.

-- 
Kalle Valo

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Dinh Nguyen]

On 06/27/2016 11:18 AM, Borislav Petkov wrote:
> On Mon, Jun 27, 2016 at 10:31:29AM -0500, Dinh Nguyen wrote:
>> I've applied this patch and will take through the arm-soc tree.
> 
> I already took the whole branch two days ago:
> 
> http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=for-next
> 
> So we need to sort this out as it has come up in the past. Either you
> pick up all patches and I ack them or I do and you ack the one. But
> splitting the patchset between trees is always calling for trouble.
> 
> So what are doing, wanna give me your Ack for that patch instead and I
> can carry the whole set upstream?
> 

Ok, sorry about that. Please carry the whole set:

Acked-by: Dinh Nguyen 

Dinh

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 9:17 AM, Brian Gerst  wrote:
> On Mon, Jun 27, 2016 at 11:54 AM, Andy Lutomirski  wrote:
>> On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
>>> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
 On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>>  #ifdef CONFIG_X86_64
>>  /* Runs on IST stack */
>>  dotraplinkage void do_double_fault(struct pt_regs *regs, long 
>> error_code)
>>  {
>> static const char str[] = "double fault";
>> struct task_struct *tsk = current;
>> +#ifdef CONFIG_VMAP_STACK
>> +   unsigned long cr2;
>> +#endif
>>
>>  #ifdef CONFIG_X86_ESPFIX64
>> extern unsigned char native_irq_return_iret[];
>> @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
>> *regs, long error_code)
>> tsk->thread.error_code = error_code;
>> tsk->thread.trap_nr = X86_TRAP_DF;
>>
>> +#ifdef CONFIG_VMAP_STACK
>> +   /*
>> +* If we overflow the stack into a guard page, the CPU will fail
>> +* to deliver #PF and will send #DF instead.  CR2 will contain
>> +* the linear address of the second fault, which will be in the
>> +* guard page below the bottom of the stack.
>> +*/
>> +   cr2 = read_cr2();
>> +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
>> +   handle_stack_overflow(
>> +   "kernel stack overflow (double-fault)",
>> +   regs, cr2);
>> +#endif
>
> Is there any other way to tell if this was from a page fault?  If it
> wasn't a page fault then CR2 is undefined.

 I guess it doesn't really matter, since the fault is fatal either way.
 The error message might be incorrect though.

>>>
>>> It's at least worth a comment, though.  Maybe I should check if
>>> regs->rsp is within 40 bytes of the bottom of the stack, too, such
>>> that delivery of an inner fault would have double-faulted assuming the
>>> inner fault didn't use an IST vector.
>>>
>>
>> How about:
>>
>> /*
>>  * If we overflow the stack into a guard page, the CPU will fail
>>  * to deliver #PF and will send #DF instead.  CR2 will contain
>>  * the linear address of the second fault, which will be in the
>>  * guard page below the bottom of the stack.
>>  *
>>  * We're limited to using heuristics here, since the CPU does
>>  * not tell us what type of fault failed and, if the first fault
>>  * wasn't a page fault, CR2 may contain stale garbage.  To mostly
>>  * rule out garbage, we check if the saved RSP is close enough to
>>  * the bottom of the stack to cause exception delivery to fail.
>>  * The worst case is 7 stack slots: one for alignment, five for
>>  * SS..RIP, and one for the error code.
>>  */
>> tsk_stack = (unsigned long)task_stack_page(tsk);
>> if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
>> /* A double-fault due to #PF delivery failure is plausible. */
>> cr2 = read_cr2();
>> if (tsk_stack - 1 - cr2 < PAGE_SIZE)
>> handle_stack_overflow(
>> "kernel stack overflow (double-fault)",
>> regs, cr2);
>> }
>
> I think RSP anywhere in the guard page would be best, since it could
> have been decremented by a function prologue into the guard page
> before an access that triggers the page fault.
>

I think that can miss some stack overflows.  Suppose that RSP points
very close to the bottom of the stack and we take an unrelated fault.
The CPU can fail to deliver that fault and we get a double fault
instead.  But I counted wrong, too.  Do you like this version and its
explanation?

/*
 * If we overflow the stack into a guard page, the CPU will fail
 * to deliver #PF and will send #DF instead.  Similarly, if we
 * take any non-IST exception while too close to the bottom of
 * the stack, the processor will get a page fault while
 * delivering the exception and will generate a double fault.
 *
 * According to the SDM (footnote in 6.15 under "Interrupt 14 -
 * Page-Fault Exception (#PF):
 *
 *   Processors update CR2 whenever a page fault is detected. If a
 *   second page fault occurs while an earlier page fault is being
 *   deliv- ered, the faulting linear address of the second fault will
 *   overwrite the contents of CR2 (replacing the previous
 *   address). These updates to CR2 occur even if the page fault
 *   results in a double fault or occurs during the delivery of a
 *   double fault.
 *
 * However, if we got here due to a non-page-fault exception while
 * 

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Dinh Nguyen]

On 06/27/2016 11:18 AM, Borislav Petkov wrote:
> On Mon, Jun 27, 2016 at 10:31:29AM -0500, Dinh Nguyen wrote:
>> I've applied this patch and will take through the arm-soc tree.
> 
> I already took the whole branch two days ago:
> 
> http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=for-next
> 
> So we need to sort this out as it has come up in the past. Either you
> pick up all patches and I ack them or I do and you ack the one. But
> splitting the patchset between trees is always calling for trouble.
> 
> So what are doing, wanna give me your Ack for that patch instead and I
> can carry the whole set upstream?
> 

Ok, sorry about that. Please carry the whole set:

Acked-by: Dinh Nguyen 

Dinh

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 9:17 AM, Brian Gerst  wrote:
> On Mon, Jun 27, 2016 at 11:54 AM, Andy Lutomirski  wrote:
>> On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
>>> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
 On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>>  #ifdef CONFIG_X86_64
>>  /* Runs on IST stack */
>>  dotraplinkage void do_double_fault(struct pt_regs *regs, long 
>> error_code)
>>  {
>> static const char str[] = "double fault";
>> struct task_struct *tsk = current;
>> +#ifdef CONFIG_VMAP_STACK
>> +   unsigned long cr2;
>> +#endif
>>
>>  #ifdef CONFIG_X86_ESPFIX64
>> extern unsigned char native_irq_return_iret[];
>> @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
>> *regs, long error_code)
>> tsk->thread.error_code = error_code;
>> tsk->thread.trap_nr = X86_TRAP_DF;
>>
>> +#ifdef CONFIG_VMAP_STACK
>> +   /*
>> +* If we overflow the stack into a guard page, the CPU will fail
>> +* to deliver #PF and will send #DF instead.  CR2 will contain
>> +* the linear address of the second fault, which will be in the
>> +* guard page below the bottom of the stack.
>> +*/
>> +   cr2 = read_cr2();
>> +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
>> +   handle_stack_overflow(
>> +   "kernel stack overflow (double-fault)",
>> +   regs, cr2);
>> +#endif
>
> Is there any other way to tell if this was from a page fault?  If it
> wasn't a page fault then CR2 is undefined.

 I guess it doesn't really matter, since the fault is fatal either way.
 The error message might be incorrect though.

>>>
>>> It's at least worth a comment, though.  Maybe I should check if
>>> regs->rsp is within 40 bytes of the bottom of the stack, too, such
>>> that delivery of an inner fault would have double-faulted assuming the
>>> inner fault didn't use an IST vector.
>>>
>>
>> How about:
>>
>> /*
>>  * If we overflow the stack into a guard page, the CPU will fail
>>  * to deliver #PF and will send #DF instead.  CR2 will contain
>>  * the linear address of the second fault, which will be in the
>>  * guard page below the bottom of the stack.
>>  *
>>  * We're limited to using heuristics here, since the CPU does
>>  * not tell us what type of fault failed and, if the first fault
>>  * wasn't a page fault, CR2 may contain stale garbage.  To mostly
>>  * rule out garbage, we check if the saved RSP is close enough to
>>  * the bottom of the stack to cause exception delivery to fail.
>>  * The worst case is 7 stack slots: one for alignment, five for
>>  * SS..RIP, and one for the error code.
>>  */
>> tsk_stack = (unsigned long)task_stack_page(tsk);
>> if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
>> /* A double-fault due to #PF delivery failure is plausible. */
>> cr2 = read_cr2();
>> if (tsk_stack - 1 - cr2 < PAGE_SIZE)
>> handle_stack_overflow(
>> "kernel stack overflow (double-fault)",
>> regs, cr2);
>> }
>
> I think RSP anywhere in the guard page would be best, since it could
> have been decremented by a function prologue into the guard page
> before an access that triggers the page fault.
>

I think that can miss some stack overflows.  Suppose that RSP points
very close to the bottom of the stack and we take an unrelated fault.
The CPU can fail to deliver that fault and we get a double fault
instead.  But I counted wrong, too.  Do you like this version and its
explanation?

/*
 * If we overflow the stack into a guard page, the CPU will fail
 * to deliver #PF and will send #DF instead.  Similarly, if we
 * take any non-IST exception while too close to the bottom of
 * the stack, the processor will get a page fault while
 * delivering the exception and will generate a double fault.
 *
 * According to the SDM (footnote in 6.15 under "Interrupt 14 -
 * Page-Fault Exception (#PF):
 *
 *   Processors update CR2 whenever a page fault is detected. If a
 *   second page fault occurs while an earlier page fault is being
 *   deliv- ered, the faulting linear address of the second fault will
 *   overwrite the contents of CR2 (replacing the previous
 *   address). These updates to CR2 occur even if the page fault
 *   results in a double fault or occurs during the delivery of a
 *   double fault.
 *
 * However, if we got here due to a non-page-fault exception while
 * delivering a non-page-fault exception, CR2 may contain a
 * stale value.
 *
 * As a heuristic: we consider this 

Re: [PATCH 4/5] sched/debug: remove several CONFIG_SCHEDSTATS guards

[Josh Poimboeuf]

On Mon, Jun 27, 2016 at 06:21:11PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 17, 2016 at 12:43:26PM -0500, Josh Poimboeuf wrote:
> > index 017a4f5..0aee5dd 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1626,10 +1626,15 @@ static inline int __set_cpus_allowed_ptr(struct 
> > task_struct *p,
> >  static void
> >  ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
> >  {
> > -#ifdef CONFIG_SCHEDSTATS
> > -   struct rq *rq = this_rq();
> > +   struct rq *rq;
> > +
> > +   if (!schedstat_enabled())
> > +   return;
> > +
> > +   rq = this_rq();
> >  
> >  #ifdef CONFIG_SMP
> > +{
> > int this_cpu = smp_processor_id();
> >  
> > if (cpu == this_cpu) {
> > @@ -1651,7 +1656,7 @@ ttwu_stat(struct task_struct *p, int cpu, int 
> > wake_flags)
> >  
> > if (wake_flags & WF_MIGRATED)
> > schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> > -
> > +}
> >  #endif /* CONFIG_SMP */
> >  
> > schedstat_inc(rq->ttwu_count);
> 
> I did:
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1636,10 +1636,7 @@ ttwu_stat(struct task_struct *p, int cpu
>   rq = this_rq();
>  
>  #ifdef CONFIG_SMP
> -{
> - int this_cpu = smp_processor_id();
> -
> - if (cpu == this_cpu) {
> + if (cpu == rq->cpu) {
>   schedstat_inc(rq->ttwu_local);
>   schedstat_inc(p->se.statistics.nr_wakeups_local);
>   } else {
> @@ -1647,7 +1644,7 @@ ttwu_stat(struct task_struct *p, int cpu
>  
>   schedstat_inc(p->se.statistics.nr_wakeups_remote);
>   rcu_read_lock();
> - for_each_domain(this_cpu, sd) {
> + for_each_domain(rq->cpu, sd) {
>   if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
>   schedstat_inc(sd->ttwu_wake_remote);
>   break;
> @@ -1658,7 +1655,6 @@ ttwu_stat(struct task_struct *p, int cpu
>  
>   if (wake_flags & WF_MIGRATED)
>   schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> -}
>  #endif /* CONFIG_SMP */
>  
>   schedstat_inc(rq->ttwu_count);

Much better, thanks.

-- 
Josh

Re: [PATCH 4/5] sched/debug: remove several CONFIG_SCHEDSTATS guards

[Josh Poimboeuf]

On Mon, Jun 27, 2016 at 06:21:11PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 17, 2016 at 12:43:26PM -0500, Josh Poimboeuf wrote:
> > index 017a4f5..0aee5dd 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1626,10 +1626,15 @@ static inline int __set_cpus_allowed_ptr(struct 
> > task_struct *p,
> >  static void
> >  ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
> >  {
> > -#ifdef CONFIG_SCHEDSTATS
> > -   struct rq *rq = this_rq();
> > +   struct rq *rq;
> > +
> > +   if (!schedstat_enabled())
> > +   return;
> > +
> > +   rq = this_rq();
> >  
> >  #ifdef CONFIG_SMP
> > +{
> > int this_cpu = smp_processor_id();
> >  
> > if (cpu == this_cpu) {
> > @@ -1651,7 +1656,7 @@ ttwu_stat(struct task_struct *p, int cpu, int 
> > wake_flags)
> >  
> > if (wake_flags & WF_MIGRATED)
> > schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> > -
> > +}
> >  #endif /* CONFIG_SMP */
> >  
> > schedstat_inc(rq->ttwu_count);
> 
> I did:
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1636,10 +1636,7 @@ ttwu_stat(struct task_struct *p, int cpu
>   rq = this_rq();
>  
>  #ifdef CONFIG_SMP
> -{
> - int this_cpu = smp_processor_id();
> -
> - if (cpu == this_cpu) {
> + if (cpu == rq->cpu) {
>   schedstat_inc(rq->ttwu_local);
>   schedstat_inc(p->se.statistics.nr_wakeups_local);
>   } else {
> @@ -1647,7 +1644,7 @@ ttwu_stat(struct task_struct *p, int cpu
>  
>   schedstat_inc(p->se.statistics.nr_wakeups_remote);
>   rcu_read_lock();
> - for_each_domain(this_cpu, sd) {
> + for_each_domain(rq->cpu, sd) {
>   if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
>   schedstat_inc(sd->ttwu_wake_remote);
>   break;
> @@ -1658,7 +1655,6 @@ ttwu_stat(struct task_struct *p, int cpu
>  
>   if (wake_flags & WF_MIGRATED)
>   schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> -}
>  #endif /* CONFIG_SMP */
>  
>   schedstat_inc(rq->ttwu_count);

Much better, thanks.

-- 
Josh

Re: [PATCH v2 1/3] block: provide helpers for reading block count

[Davidlohr Bueso]

On Thu, 23 Jun 2016, Arnd Bergmann wrote:


Several drivers use an expensive do_div() to compute the number
of logical or physical blocks in a blockdev, which can be done
more efficiently using a shift, since the blocksize is always
a power of two number.

Let's introduce bdev_logical_block_count() and bdev_physical_block_count()
helper functions mirroring the bdev_logical_block_size() and
bdev_physical_block_size() interfaces for the block size.



@@ -1226,6 +1226,13 @@ static inline unsigned short 
bdev_logical_block_size(struct block_device *bdev)
return queue_logical_block_size(bdev_get_queue(bdev));
}

+static inline sector_t bdev_logical_block_count(struct block_device *bdev)


Curious, why not just return u64 instead for all these instead of sector_t (ie
dealing with lba, it reads weird)?

Thanks,
Davidlohr

Re: [PATCH v2 1/3] block: provide helpers for reading block count

[Davidlohr Bueso]

On Thu, 23 Jun 2016, Arnd Bergmann wrote:


Several drivers use an expensive do_div() to compute the number
of logical or physical blocks in a blockdev, which can be done
more efficiently using a shift, since the blocksize is always
a power of two number.

Let's introduce bdev_logical_block_count() and bdev_physical_block_count()
helper functions mirroring the bdev_logical_block_size() and
bdev_physical_block_size() interfaces for the block size.



@@ -1226,6 +1226,13 @@ static inline unsigned short 
bdev_logical_block_size(struct block_device *bdev)
return queue_logical_block_size(bdev_get_queue(bdev));
}

+static inline sector_t bdev_logical_block_count(struct block_device *bdev)


Curious, why not just return u64 instead for all these instead of sector_t (ie
dealing with lba, it reads weird)?

Thanks,
Davidlohr

Re: [PATCH v6 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Daniel Lezcano]

On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:

Hi Sudeep,

On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
unify it and move to cpuidle-arm.h header.

Cc: Lorenzo Pieralisi 
Cc: Mark Rutland 
Cc: Daniel Lezcano 
Cc: "Rafael J. Wysocki" 
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Sudeep Holla 
---
  arch/arm64/kernel/cpuidle.c   | 17 +
  drivers/cpuidle/cpuidle-arm.c | 23 ++
  drivers/firmware/psci.c   | 56 +++
  include/linux/cpuidle-arm.h   | 30 +++
  4 files changed, 105 insertions(+), 21 deletions(-)
  create mode 100644 include/linux/cpuidle-arm.h


This patch seems fine by me, it would be good if Daniel can have
a look too.

Some minor comments below.

[...]


diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 03e04582791c..c6caa863d156 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@

  #define pr_fmt(fmt) "psci: " fmt

+#include 
  #include 
  #include 
  #include 
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node 
*cpu_node, int cpu)
return ret;
  }

+#ifdef CONFIG_ACPI
+#include 
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+   int i, count;
+   u32 *psci_states;
+   struct acpi_processor *pr;
+   struct acpi_lpi_state *lpi;
+
+   pr = per_cpu(processors, cpu);
+   if (unlikely(!pr || !pr->flags.has_lpi))
+   return -EINVAL;
+
+   /*
+* If the PSCI cpu_suspend function hook has not been initialized
+* idle states must not be enabled, so bail out
+*/
+   if (!psci_ops.cpu_suspend)
+   return -EOPNOTSUPP;
+
+   count = pr->power.count - 1;
+   if (count <= 0)
+   return -ENODEV;
+
+   psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+   if (!psci_states)
+   return -ENOMEM;
+
+   for (i = 0; i < count; i++) {
+   u32 state;
+
+   lpi = >power.lpi_states[i + 1];
+   state = lpi->address & 0x;


Why mask 'address' if 'state' is u32 ?


+   if (!psci_power_state_is_valid(state)) {
+   pr_warn("Invalid PSCI power state %#x\n", state);
+   kfree(psci_states);
+   return -EINVAL;
+   }
+   psci_states[i] = state;
+   }
+   /* Idle states parsed correctly, initialize per-cpu pointer */
+   per_cpu(psci_power_state, cpu) = psci_states;
+   return 0;


The ACPI and the PSCI code are not self contained here.

It would be nice to move this function to the ACPI code.


+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+   return -EINVAL;
+}
+#endif
+
  int psci_cpu_init_idle(unsigned int cpu)
  {
struct device_node *cpu_node;
int ret;

+   if (!acpi_disabled)
+   return psci_acpi_cpu_init_idle(cpu);
+


acpi_disabled - acpi_disabled - acpi_disabled everywhere :/

The enable-method approach is not straightforward and now it is polluted 
by acpi-disabled.


So IIUC,

smp_init_cpus (contains acpi_disabled)
  smp_cpu_setup
cpu_read_ops
  cpu_read_enable_method (contains acpi_disabled)
acpi_get_enable_method (returns 'psci' after checking 
psci_is_present)


Then psci_cpu_init_idle is called... and check again acpi_disabled.

IMO, the circumlocution with the psci vs acpi vs acpi_disabled is 
getting unnecessary too complex, is prone to error and will lead to 
unmaintainable code very soon.


I suggest to sort out encapsulation and self-contained code before 
adding more feature in this area.




cpu_node = of_get_cpu_node(cpu, NULL);
if (!cpu_node)
return -ENODEV;
diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
new file mode 100644
index ..b99bcb3f43dd
--- /dev/null
+++ b/include/linux/cpuidle-arm.h


arm-cpuidle.h for consistency with other (ARM) include/linux files ?


@@ -0,0 +1,30 @@
+#include 
+
+#include 
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ */
+static int arm_generic_enter_idle_state(int idx)
+{
+   int ret;
+
+   if (!idx) {
+   cpu_do_idle();
+   return idx;
+   }
+
+   ret = cpu_pm_enter();
+   if (!ret) {
+   /*
+* Pass idle state index to cpu_suspend which in turn will
+* call the CPU ops suspend protocol with idle index as a
+* parameter.
+

Re: [PATCH v6 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Daniel Lezcano]

On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:

Hi Sudeep,

On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
unify it and move to cpuidle-arm.h header.

Cc: Lorenzo Pieralisi 
Cc: Mark Rutland 
Cc: Daniel Lezcano 
Cc: "Rafael J. Wysocki" 
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Sudeep Holla 
---
  arch/arm64/kernel/cpuidle.c   | 17 +
  drivers/cpuidle/cpuidle-arm.c | 23 ++
  drivers/firmware/psci.c   | 56 +++
  include/linux/cpuidle-arm.h   | 30 +++
  4 files changed, 105 insertions(+), 21 deletions(-)
  create mode 100644 include/linux/cpuidle-arm.h


This patch seems fine by me, it would be good if Daniel can have
a look too.

Some minor comments below.

[...]


diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 03e04582791c..c6caa863d156 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@

  #define pr_fmt(fmt) "psci: " fmt

+#include 
  #include 
  #include 
  #include 
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node 
*cpu_node, int cpu)
return ret;
  }

+#ifdef CONFIG_ACPI
+#include 
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+   int i, count;
+   u32 *psci_states;
+   struct acpi_processor *pr;
+   struct acpi_lpi_state *lpi;
+
+   pr = per_cpu(processors, cpu);
+   if (unlikely(!pr || !pr->flags.has_lpi))
+   return -EINVAL;
+
+   /*
+* If the PSCI cpu_suspend function hook has not been initialized
+* idle states must not be enabled, so bail out
+*/
+   if (!psci_ops.cpu_suspend)
+   return -EOPNOTSUPP;
+
+   count = pr->power.count - 1;
+   if (count <= 0)
+   return -ENODEV;
+
+   psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+   if (!psci_states)
+   return -ENOMEM;
+
+   for (i = 0; i < count; i++) {
+   u32 state;
+
+   lpi = >power.lpi_states[i + 1];
+   state = lpi->address & 0x;


Why mask 'address' if 'state' is u32 ?


+   if (!psci_power_state_is_valid(state)) {
+   pr_warn("Invalid PSCI power state %#x\n", state);
+   kfree(psci_states);
+   return -EINVAL;
+   }
+   psci_states[i] = state;
+   }
+   /* Idle states parsed correctly, initialize per-cpu pointer */
+   per_cpu(psci_power_state, cpu) = psci_states;
+   return 0;


The ACPI and the PSCI code are not self contained here.

It would be nice to move this function to the ACPI code.


+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+   return -EINVAL;
+}
+#endif
+
  int psci_cpu_init_idle(unsigned int cpu)
  {
struct device_node *cpu_node;
int ret;

+   if (!acpi_disabled)
+   return psci_acpi_cpu_init_idle(cpu);
+


acpi_disabled - acpi_disabled - acpi_disabled everywhere :/

The enable-method approach is not straightforward and now it is polluted 
by acpi-disabled.


So IIUC,

smp_init_cpus (contains acpi_disabled)
  smp_cpu_setup
cpu_read_ops
  cpu_read_enable_method (contains acpi_disabled)
acpi_get_enable_method (returns 'psci' after checking 
psci_is_present)


Then psci_cpu_init_idle is called... and check again acpi_disabled.

IMO, the circumlocution with the psci vs acpi vs acpi_disabled is 
getting unnecessary too complex, is prone to error and will lead to 
unmaintainable code very soon.


I suggest to sort out encapsulation and self-contained code before 
adding more feature in this area.




cpu_node = of_get_cpu_node(cpu, NULL);
if (!cpu_node)
return -ENODEV;
diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
new file mode 100644
index ..b99bcb3f43dd
--- /dev/null
+++ b/include/linux/cpuidle-arm.h


arm-cpuidle.h for consistency with other (ARM) include/linux files ?


@@ -0,0 +1,30 @@
+#include 
+
+#include 
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ */
+static int arm_generic_enter_idle_state(int idx)
+{
+   int ret;
+
+   if (!idx) {
+   cpu_do_idle();
+   return idx;
+   }
+
+   ret = cpu_pm_enter();
+   if (!ret) {
+   /*
+* Pass idle state index to cpu_suspend which in turn will
+* call the CPU ops suspend protocol with idle index as a
+* parameter.
+*/
+   ret = arm_cpuidle_suspend(idx);
+
+   cpu_pm_exit();
+   }
+
+   

Re: [PATCH] dt-bindings: i2c: add bindings for nxp,pca9541

[Peter Rosin]

On 2016-06-27 15:17, Guenter Roeck wrote:
> On 06/27/2016 03:11 AM, Peter Rosin wrote:
>> Fill the gap for this pre-existing driver.
>>
>> Signed-off-by: Peter Rosin 
>> ---
>>   .../devicetree/bindings/i2c/i2c-arb-pca9541.txt| 33 
>> ++
>>   MAINTAINERS|  1 +
>>   2 files changed, 34 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>>
>> Hi!
>>
>> I'm wondering about this driver. It is not a trivial device, and yet it
>> has historically relied on the i2c core matching the chip w/o vendor
>> prefix. This is not ideal. But what to do about the driver implementing
>> this in terms of an i2c-mux, somthing which the chip is not; It is an
>> i2c arbitrator. It just happens to rely on the i2c mux core also handling
>> i2c gates and i2c arbitrators. But that seems like a Linux detail. So I
>> don't know what to do here?
>>
> 
> The concept of arbitrators didn't exist when I wrote the driver. I would not
> have a problem with renaming the file if that is what you are asking for.

No, that was not my issue, I just wanted to document bindings for pca9541,
and I didn't like how it turned out.

I don't really care if the bindings doc is named i2c-mux-pca9541.txt (that
would match the name of the driver, but it still wouldn't make the chip a mux).

>> That is, the patch - as is - describes something that would be trivial to
>> support today, but at the same time it seems to be too tied to Linux.
>>
>> The problem is that the i2c@0 intermediate node is not really needed, but
>> at the same time removing it would cause a disruption for the driver since
>> it can't really use the i2c mux core if that node isn't there. I don't
>> see a simple way to fix that in the i2c mux core either (but admittedly
>> haven't given it too much thought).
>>
> 
> The gpio arbitrator uses the same principle as well. Why not just leave it
> alone ? Besides, I think it is a good idea to have it, since it groups
> the i2c devices behind the chip together. I would not consider that to be
> a Linuxism, but a design choice.

The grouping argument would make sense if there was anything outside the
group. Also, the required reg property and the extra #address-cells and
#size-cells doesn't add anything and just gets in the way, and is indeed
the result of Linuxisms leaking back into device trees.

If there were no muxes and this was a new driver, the example bindings
would almost certainly have been something like:

i2c-arbitrator@74 {
compatible = "nxp,pca9541";
reg = <0x74>;

#address-cells = <1>;
#size-cells = <0>;

eeprom@54 {
compatible = "at,24c08";
reg = <0x54>;
};
};

which I find much nicer.

But, I can't find a way to implement that and keep backwards compatibility
with old existing device trees.

Which is why I submitted the patch I did. It documents the pca9541 bindings,
something which is lacking, in terms of i2c-mux as the driver is written.
At the same time, this feels ugly and exposes linuxism and I wanted to make
that clear up front. The above simply looks better than the example in the
patch.

I intended to mark the submission [RFC PATCH], but I now realize that that
went missing along the way, sorry.

Cheers,
Peter

> Guenter
> 
>> Any suggestions?
>>
>> Cheers,
>> Peter
>>
>> PS. The driver source is in drivers/i2c/muxes/i2c-mux-pca9541.c
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt 
>> b/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>> new file mode 100644
>> index ..edbe84935906
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>> @@ -0,0 +1,33 @@
>> +* NXP PCA9541 I2C bus master selector
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must be "nxp,pca9541"
>> +
>> +  - reg: The I2C address of the device.
>> +
>> +  The following required properties are defined externally:
>> +
>> +  - Standard I2C mux properties. See i2c-mux.txt in this directory.
>> +  - I2C child bus nodes. See i2c-mux.txt in this directory.
>> +
>> +
>> +Example:
>> +
>> +i2c-arbitrator@74 {
>> +compatible = "nxp,pca9541";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x74>;
>> +
>> +i2c@0 {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0>;
>> +
>> +eeprom@54 {
>> +compatible = "at,24c08";
>> +reg = <0x54>;
>> +};
>> +};
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e1b090f86e0d..3dd44d0d166c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5521,6 +5521,7 @@ S: Maintained
>>   F: 

Re: [PATCH] dt-bindings: i2c: add bindings for nxp,pca9541

[Peter Rosin]

On 2016-06-27 15:17, Guenter Roeck wrote:
> On 06/27/2016 03:11 AM, Peter Rosin wrote:
>> Fill the gap for this pre-existing driver.
>>
>> Signed-off-by: Peter Rosin 
>> ---
>>   .../devicetree/bindings/i2c/i2c-arb-pca9541.txt| 33 
>> ++
>>   MAINTAINERS|  1 +
>>   2 files changed, 34 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>>
>> Hi!
>>
>> I'm wondering about this driver. It is not a trivial device, and yet it
>> has historically relied on the i2c core matching the chip w/o vendor
>> prefix. This is not ideal. But what to do about the driver implementing
>> this in terms of an i2c-mux, somthing which the chip is not; It is an
>> i2c arbitrator. It just happens to rely on the i2c mux core also handling
>> i2c gates and i2c arbitrators. But that seems like a Linux detail. So I
>> don't know what to do here?
>>
> 
> The concept of arbitrators didn't exist when I wrote the driver. I would not
> have a problem with renaming the file if that is what you are asking for.

No, that was not my issue, I just wanted to document bindings for pca9541,
and I didn't like how it turned out.

I don't really care if the bindings doc is named i2c-mux-pca9541.txt (that
would match the name of the driver, but it still wouldn't make the chip a mux).

>> That is, the patch - as is - describes something that would be trivial to
>> support today, but at the same time it seems to be too tied to Linux.
>>
>> The problem is that the i2c@0 intermediate node is not really needed, but
>> at the same time removing it would cause a disruption for the driver since
>> it can't really use the i2c mux core if that node isn't there. I don't
>> see a simple way to fix that in the i2c mux core either (but admittedly
>> haven't given it too much thought).
>>
> 
> The gpio arbitrator uses the same principle as well. Why not just leave it
> alone ? Besides, I think it is a good idea to have it, since it groups
> the i2c devices behind the chip together. I would not consider that to be
> a Linuxism, but a design choice.

The grouping argument would make sense if there was anything outside the
group. Also, the required reg property and the extra #address-cells and
#size-cells doesn't add anything and just gets in the way, and is indeed
the result of Linuxisms leaking back into device trees.

If there were no muxes and this was a new driver, the example bindings
would almost certainly have been something like:

i2c-arbitrator@74 {
compatible = "nxp,pca9541";
reg = <0x74>;

#address-cells = <1>;
#size-cells = <0>;

eeprom@54 {
compatible = "at,24c08";
reg = <0x54>;
};
};

which I find much nicer.

But, I can't find a way to implement that and keep backwards compatibility
with old existing device trees.

Which is why I submitted the patch I did. It documents the pca9541 bindings,
something which is lacking, in terms of i2c-mux as the driver is written.
At the same time, this feels ugly and exposes linuxism and I wanted to make
that clear up front. The above simply looks better than the example in the
patch.

I intended to mark the submission [RFC PATCH], but I now realize that that
went missing along the way, sorry.

Cheers,
Peter

> Guenter
> 
>> Any suggestions?
>>
>> Cheers,
>> Peter
>>
>> PS. The driver source is in drivers/i2c/muxes/i2c-mux-pca9541.c
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt 
>> b/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>> new file mode 100644
>> index ..edbe84935906
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-arb-pca9541.txt
>> @@ -0,0 +1,33 @@
>> +* NXP PCA9541 I2C bus master selector
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must be "nxp,pca9541"
>> +
>> +  - reg: The I2C address of the device.
>> +
>> +  The following required properties are defined externally:
>> +
>> +  - Standard I2C mux properties. See i2c-mux.txt in this directory.
>> +  - I2C child bus nodes. See i2c-mux.txt in this directory.
>> +
>> +
>> +Example:
>> +
>> +i2c-arbitrator@74 {
>> +compatible = "nxp,pca9541";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x74>;
>> +
>> +i2c@0 {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0>;
>> +
>> +eeprom@54 {
>> +compatible = "at,24c08";
>> +reg = <0x54>;
>> +};
>> +};
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e1b090f86e0d..3dd44d0d166c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5521,6 +5521,7 @@ S: Maintained
>>   F: 

Re: [PATCH 4/5] sched/debug: remove several CONFIG_SCHEDSTATS guards

[Peter Zijlstra]

On Fri, Jun 17, 2016 at 12:43:26PM -0500, Josh Poimboeuf wrote:
> index 017a4f5..0aee5dd 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1626,10 +1626,15 @@ static inline int __set_cpus_allowed_ptr(struct 
> task_struct *p,
>  static void
>  ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
>  {
> -#ifdef CONFIG_SCHEDSTATS
> - struct rq *rq = this_rq();
> + struct rq *rq;
> +
> + if (!schedstat_enabled())
> + return;
> +
> + rq = this_rq();
>  
>  #ifdef CONFIG_SMP
> +{
>   int this_cpu = smp_processor_id();
>  
>   if (cpu == this_cpu) {
> @@ -1651,7 +1656,7 @@ ttwu_stat(struct task_struct *p, int cpu, int 
> wake_flags)
>  
>   if (wake_flags & WF_MIGRATED)
>   schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> -
> +}
>  #endif /* CONFIG_SMP */
>  
>   schedstat_inc(rq->ttwu_count);

I did:

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1636,10 +1636,7 @@ ttwu_stat(struct task_struct *p, int cpu
rq = this_rq();
 
 #ifdef CONFIG_SMP
-{
-   int this_cpu = smp_processor_id();
-
-   if (cpu == this_cpu) {
+   if (cpu == rq->cpu) {
schedstat_inc(rq->ttwu_local);
schedstat_inc(p->se.statistics.nr_wakeups_local);
} else {
@@ -1647,7 +1644,7 @@ ttwu_stat(struct task_struct *p, int cpu
 
schedstat_inc(p->se.statistics.nr_wakeups_remote);
rcu_read_lock();
-   for_each_domain(this_cpu, sd) {
+   for_each_domain(rq->cpu, sd) {
if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
schedstat_inc(sd->ttwu_wake_remote);
break;
@@ -1658,7 +1655,6 @@ ttwu_stat(struct task_struct *p, int cpu
 
if (wake_flags & WF_MIGRATED)
schedstat_inc(p->se.statistics.nr_wakeups_migrate);
-}
 #endif /* CONFIG_SMP */
 
schedstat_inc(rq->ttwu_count);

Re: [PATCH 4/5] sched/debug: remove several CONFIG_SCHEDSTATS guards

[Peter Zijlstra]

On Fri, Jun 17, 2016 at 12:43:26PM -0500, Josh Poimboeuf wrote:
> index 017a4f5..0aee5dd 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1626,10 +1626,15 @@ static inline int __set_cpus_allowed_ptr(struct 
> task_struct *p,
>  static void
>  ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
>  {
> -#ifdef CONFIG_SCHEDSTATS
> - struct rq *rq = this_rq();
> + struct rq *rq;
> +
> + if (!schedstat_enabled())
> + return;
> +
> + rq = this_rq();
>  
>  #ifdef CONFIG_SMP
> +{
>   int this_cpu = smp_processor_id();
>  
>   if (cpu == this_cpu) {
> @@ -1651,7 +1656,7 @@ ttwu_stat(struct task_struct *p, int cpu, int 
> wake_flags)
>  
>   if (wake_flags & WF_MIGRATED)
>   schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> -
> +}
>  #endif /* CONFIG_SMP */
>  
>   schedstat_inc(rq->ttwu_count);

I did:

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1636,10 +1636,7 @@ ttwu_stat(struct task_struct *p, int cpu
rq = this_rq();
 
 #ifdef CONFIG_SMP
-{
-   int this_cpu = smp_processor_id();
-
-   if (cpu == this_cpu) {
+   if (cpu == rq->cpu) {
schedstat_inc(rq->ttwu_local);
schedstat_inc(p->se.statistics.nr_wakeups_local);
} else {
@@ -1647,7 +1644,7 @@ ttwu_stat(struct task_struct *p, int cpu
 
schedstat_inc(p->se.statistics.nr_wakeups_remote);
rcu_read_lock();
-   for_each_domain(this_cpu, sd) {
+   for_each_domain(rq->cpu, sd) {
if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
schedstat_inc(sd->ttwu_wake_remote);
break;
@@ -1658,7 +1655,6 @@ ttwu_stat(struct task_struct *p, int cpu
 
if (wake_flags & WF_MIGRATED)
schedstat_inc(p->se.statistics.nr_wakeups_migrate);
-}
 #endif /* CONFIG_SMP */
 
schedstat_inc(rq->ttwu_count);

Re: [PATCH v3 3/9] kexec_file: Factor out kexec_locate_mem_hole from kexec_add_buffer.

[Dave Young]

On 06/23/16 at 12:37pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 23 Juni 2016, 01:44:07 schrieb Dave Young:
> > Hmm, hold on. For declaring a struct in a header file, comment should be
> > just after each fields, like below, your format is for a function instead:
> > struct pci_slot {
> > struct pci_bus *bus;/* The bus this slot is on */
> > struct list_head list;  /* node in list of slots on this
> > bus */ struct hotplug_slot *hotplug;   /* Hotplug info (migrate over
> > time) */ unsigned char number;   /* PCI_SLOT(pci_dev->devfn) */
> > struct kobject kobj;
> > };
> 
> The comment style you mention above is not extractable documentation. The 
> style I used is what is described in section "kernel-doc for structs, 
> unions, enums, and typedefs" in Documentation/kernel-doc-nano-HOWTO.txt.

You are right and I was wrong!

>  
> > BTW, what is @size? there's no size field in kexec_buf. I think it is not
> > necessary to add these comment, they are easy to understand. If you really
> > want, please rewrite them correctly, for example "image" description is
> > wrong. It is not only for searching memory only, top_down description is
> > also bad.
> 
> Sorry, I moved these comments from kexec_locate_mem_hole but forgot to 
> rename the parameters to what they are called in struct kexec_buf. @size 
> should have been @memsz (other fields also have wrong names, I'll fix them 
> as well). The image description is correct in the context of where struct 
> kexec_buf is used and explains what it will be used for in the function 
> taking kexec_buf as an argument. It is not meant as a general description of 
> the purpose of struct kimage. What is bad about the description of top_down?

It is not clear enough to me, I personally think the original one in
source code is better:
/* allocate from top of memory hole */

> 
> I decided to add these comments because struct kexec_buf is now part of the 
> kernel API for kexec. kernel-doc-nano-HOWTO.txt says:
> 
> > We definitely need kernel-doc formatted documentation for functions
> > that are exported to loadable modules using EXPORT_SYMBOL.
> > 
> > We also look to provide kernel-doc formatted documentation for
> > functions externally visible to other kernel files (not marked
> > "static").
> > 
> > We also recommend providing kernel-doc formatted documentation
> > for private (file "static") routines, for consistency of kernel
> > source code layout.  But this is lower priority and at the
> > discretion of the MAINTAINER of that kernel source file.
> 
> If you think they are not necessary or just add clutter I can leave them 
> out.

I'm fine with either way.

THanks
Dave

Re: [PATCH v3 3/9] kexec_file: Factor out kexec_locate_mem_hole from kexec_add_buffer.

[Dave Young]

On 06/23/16 at 12:37pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 23 Juni 2016, 01:44:07 schrieb Dave Young:
> > Hmm, hold on. For declaring a struct in a header file, comment should be
> > just after each fields, like below, your format is for a function instead:
> > struct pci_slot {
> > struct pci_bus *bus;/* The bus this slot is on */
> > struct list_head list;  /* node in list of slots on this
> > bus */ struct hotplug_slot *hotplug;   /* Hotplug info (migrate over
> > time) */ unsigned char number;   /* PCI_SLOT(pci_dev->devfn) */
> > struct kobject kobj;
> > };
> 
> The comment style you mention above is not extractable documentation. The 
> style I used is what is described in section "kernel-doc for structs, 
> unions, enums, and typedefs" in Documentation/kernel-doc-nano-HOWTO.txt.

You are right and I was wrong!

>  
> > BTW, what is @size? there's no size field in kexec_buf. I think it is not
> > necessary to add these comment, they are easy to understand. If you really
> > want, please rewrite them correctly, for example "image" description is
> > wrong. It is not only for searching memory only, top_down description is
> > also bad.
> 
> Sorry, I moved these comments from kexec_locate_mem_hole but forgot to 
> rename the parameters to what they are called in struct kexec_buf. @size 
> should have been @memsz (other fields also have wrong names, I'll fix them 
> as well). The image description is correct in the context of where struct 
> kexec_buf is used and explains what it will be used for in the function 
> taking kexec_buf as an argument. It is not meant as a general description of 
> the purpose of struct kimage. What is bad about the description of top_down?

It is not clear enough to me, I personally think the original one in
source code is better:
/* allocate from top of memory hole */

> 
> I decided to add these comments because struct kexec_buf is now part of the 
> kernel API for kexec. kernel-doc-nano-HOWTO.txt says:
> 
> > We definitely need kernel-doc formatted documentation for functions
> > that are exported to loadable modules using EXPORT_SYMBOL.
> > 
> > We also look to provide kernel-doc formatted documentation for
> > functions externally visible to other kernel files (not marked
> > "static").
> > 
> > We also recommend providing kernel-doc formatted documentation
> > for private (file "static") routines, for consistency of kernel
> > source code layout.  But this is lower priority and at the
> > discretion of the MAINTAINER of that kernel source file.
> 
> If you think they are not necessary or just add clutter I can leave them 
> out.

I'm fine with either way.

THanks
Dave

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Borislav Petkov]

On Mon, Jun 27, 2016 at 10:31:29AM -0500, Dinh Nguyen wrote:
> I've applied this patch and will take through the arm-soc tree.

I already took the whole branch two days ago:

http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=for-next

So we need to sort this out as it has come up in the past. Either you
pick up all patches and I ack them or I do and you ack the one. But
splitting the patchset between trees is always calling for trouble.

So what are doing, wanna give me your Ack for that patch instead and I
can carry the whole set upstream?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Brian Gerst]

On Mon, Jun 27, 2016 at 11:54 AM, Andy Lutomirski  wrote:
> On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
>> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
>>> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
 On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>  #ifdef CONFIG_X86_64
>  /* Runs on IST stack */
>  dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
>  {
> static const char str[] = "double fault";
> struct task_struct *tsk = current;
> +#ifdef CONFIG_VMAP_STACK
> +   unsigned long cr2;
> +#endif
>
>  #ifdef CONFIG_X86_ESPFIX64
> extern unsigned char native_irq_return_iret[];
> @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
> *regs, long error_code)
> tsk->thread.error_code = error_code;
> tsk->thread.trap_nr = X86_TRAP_DF;
>
> +#ifdef CONFIG_VMAP_STACK
> +   /*
> +* If we overflow the stack into a guard page, the CPU will fail
> +* to deliver #PF and will send #DF instead.  CR2 will contain
> +* the linear address of the second fault, which will be in the
> +* guard page below the bottom of the stack.
> +*/
> +   cr2 = read_cr2();
> +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
> +   handle_stack_overflow(
> +   "kernel stack overflow (double-fault)",
> +   regs, cr2);
> +#endif

 Is there any other way to tell if this was from a page fault?  If it
 wasn't a page fault then CR2 is undefined.
>>>
>>> I guess it doesn't really matter, since the fault is fatal either way.
>>> The error message might be incorrect though.
>>>
>>
>> It's at least worth a comment, though.  Maybe I should check if
>> regs->rsp is within 40 bytes of the bottom of the stack, too, such
>> that delivery of an inner fault would have double-faulted assuming the
>> inner fault didn't use an IST vector.
>>
>
> How about:
>
> /*
>  * If we overflow the stack into a guard page, the CPU will fail
>  * to deliver #PF and will send #DF instead.  CR2 will contain
>  * the linear address of the second fault, which will be in the
>  * guard page below the bottom of the stack.
>  *
>  * We're limited to using heuristics here, since the CPU does
>  * not tell us what type of fault failed and, if the first fault
>  * wasn't a page fault, CR2 may contain stale garbage.  To mostly
>  * rule out garbage, we check if the saved RSP is close enough to
>  * the bottom of the stack to cause exception delivery to fail.
>  * The worst case is 7 stack slots: one for alignment, five for
>  * SS..RIP, and one for the error code.
>  */
> tsk_stack = (unsigned long)task_stack_page(tsk);
> if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
> /* A double-fault due to #PF delivery failure is plausible. */
> cr2 = read_cr2();
> if (tsk_stack - 1 - cr2 < PAGE_SIZE)
> handle_stack_overflow(
> "kernel stack overflow (double-fault)",
> regs, cr2);
> }

I think RSP anywhere in the guard page would be best, since it could
have been decremented by a function prologue into the guard page
before an access that triggers the page fault.

--
Brian Gerst

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Borislav Petkov]

On Mon, Jun 27, 2016 at 10:31:29AM -0500, Dinh Nguyen wrote:
> I've applied this patch and will take through the arm-soc tree.

I already took the whole branch two days ago:

http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=for-next

So we need to sort this out as it has come up in the past. Either you
pick up all patches and I ack them or I do and you ack the one. But
splitting the patchset between trees is always calling for trouble.

So what are doing, wanna give me your Ack for that patch instead and I
can carry the whole set upstream?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Brian Gerst]

On Mon, Jun 27, 2016 at 11:54 AM, Andy Lutomirski  wrote:
> On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
>> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
>>> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
 On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>  #ifdef CONFIG_X86_64
>  /* Runs on IST stack */
>  dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
>  {
> static const char str[] = "double fault";
> struct task_struct *tsk = current;
> +#ifdef CONFIG_VMAP_STACK
> +   unsigned long cr2;
> +#endif
>
>  #ifdef CONFIG_X86_ESPFIX64
> extern unsigned char native_irq_return_iret[];
> @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
> *regs, long error_code)
> tsk->thread.error_code = error_code;
> tsk->thread.trap_nr = X86_TRAP_DF;
>
> +#ifdef CONFIG_VMAP_STACK
> +   /*
> +* If we overflow the stack into a guard page, the CPU will fail
> +* to deliver #PF and will send #DF instead.  CR2 will contain
> +* the linear address of the second fault, which will be in the
> +* guard page below the bottom of the stack.
> +*/
> +   cr2 = read_cr2();
> +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
> +   handle_stack_overflow(
> +   "kernel stack overflow (double-fault)",
> +   regs, cr2);
> +#endif

 Is there any other way to tell if this was from a page fault?  If it
 wasn't a page fault then CR2 is undefined.
>>>
>>> I guess it doesn't really matter, since the fault is fatal either way.
>>> The error message might be incorrect though.
>>>
>>
>> It's at least worth a comment, though.  Maybe I should check if
>> regs->rsp is within 40 bytes of the bottom of the stack, too, such
>> that delivery of an inner fault would have double-faulted assuming the
>> inner fault didn't use an IST vector.
>>
>
> How about:
>
> /*
>  * If we overflow the stack into a guard page, the CPU will fail
>  * to deliver #PF and will send #DF instead.  CR2 will contain
>  * the linear address of the second fault, which will be in the
>  * guard page below the bottom of the stack.
>  *
>  * We're limited to using heuristics here, since the CPU does
>  * not tell us what type of fault failed and, if the first fault
>  * wasn't a page fault, CR2 may contain stale garbage.  To mostly
>  * rule out garbage, we check if the saved RSP is close enough to
>  * the bottom of the stack to cause exception delivery to fail.
>  * The worst case is 7 stack slots: one for alignment, five for
>  * SS..RIP, and one for the error code.
>  */
> tsk_stack = (unsigned long)task_stack_page(tsk);
> if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
> /* A double-fault due to #PF delivery failure is plausible. */
> cr2 = read_cr2();
> if (tsk_stack - 1 - cr2 < PAGE_SIZE)
> handle_stack_overflow(
> "kernel stack overflow (double-fault)",
> regs, cr2);
> }

I think RSP anywhere in the guard page would be best, since it could
have been decremented by a function prologue into the guard page
before an access that triggers the page fault.

--
Brian Gerst

RE: [PATCH 4/5] rtc: m48t86: move m48t86.h to platform_data

[Hartley Sweeten]

On Sunday, June 26, 2016 3:03 PM, Alexandre Belloni wrote:
> m48t86.h belongs to include/linux/platform_data/
>
> Signed-off-by: Alexandre Belloni 
> ---
> Cc: Hartley Sweeten 
> Cc: Ryan Mallon 
> Cc: Alexander Clouter 
> Cc: Jason Cooper 
> Cc: Andrew Lunn 
> Cc: Sebastian Hesselbarth 
> Cc: Gregory Clement 
> Cc: linux-arm-ker...@lists.infradead.org
>
>  arch/arm/mach-ep93xx/ts72xx.c  | 2 +-
>  arch/arm/mach-orion5x/ts78xx-setup.c   | 2 +-
>  drivers/rtc/rtc-m48t86.c   | 2 +-
>  include/linux/{m48t86.h => platform_data/rtc-m48t86.h} | 0
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename include/linux/{m48t86.h => platform_data/rtc-m48t86.h} (100%)

For ep93xx:

Acked-by: H Hartley Sweeten 

RE: [PATCH 4/5] rtc: m48t86: move m48t86.h to platform_data

[Hartley Sweeten]

On Sunday, June 26, 2016 3:03 PM, Alexandre Belloni wrote:
> m48t86.h belongs to include/linux/platform_data/
>
> Signed-off-by: Alexandre Belloni 
> ---
> Cc: Hartley Sweeten 
> Cc: Ryan Mallon 
> Cc: Alexander Clouter 
> Cc: Jason Cooper 
> Cc: Andrew Lunn 
> Cc: Sebastian Hesselbarth 
> Cc: Gregory Clement 
> Cc: linux-arm-ker...@lists.infradead.org
>
>  arch/arm/mach-ep93xx/ts72xx.c  | 2 +-
>  arch/arm/mach-orion5x/ts78xx-setup.c   | 2 +-
>  drivers/rtc/rtc-m48t86.c   | 2 +-
>  include/linux/{m48t86.h => platform_data/rtc-m48t86.h} | 0
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename include/linux/{m48t86.h => platform_data/rtc-m48t86.h} (100%)

For ep93xx:

Acked-by: H Hartley Sweeten 

Re: [PATCH 4/5] rtc: m48t86: move m48t86.h to platform_data

[Jason Cooper]

Hey Alexandre,

On Mon, Jun 27, 2016 at 12:03:02AM +0200, Alexandre Belloni wrote:
> m48t86.h belongs to include/linux/platform_data/
> 
> Signed-off-by: Alexandre Belloni 
> ---
> Cc: Hartley Sweeten 
> Cc: Ryan Mallon 
> Cc: Alexander Clouter 
> Cc: Jason Cooper 
> Cc: Andrew Lunn 
> Cc: Sebastian Hesselbarth 
> Cc: Gregory Clement 
> Cc: linux-arm-ker...@lists.infradead.org
> 
>  arch/arm/mach-ep93xx/ts72xx.c  | 2 +-
>  arch/arm/mach-orion5x/ts78xx-setup.c   | 2 +-
>  drivers/rtc/rtc-m48t86.c   | 2 +-
>  include/linux/{m48t86.h => platform_data/rtc-m48t86.h} | 0
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename include/linux/{m48t86.h => platform_data/rtc-m48t86.h} (100%)

For the orion5x bit,

Acked-by: Jason Cooper 

thx,

Jason.

Re: [PATCH 4/5] rtc: m48t86: move m48t86.h to platform_data

[Jason Cooper]

Hey Alexandre,

On Mon, Jun 27, 2016 at 12:03:02AM +0200, Alexandre Belloni wrote:
> m48t86.h belongs to include/linux/platform_data/
> 
> Signed-off-by: Alexandre Belloni 
> ---
> Cc: Hartley Sweeten 
> Cc: Ryan Mallon 
> Cc: Alexander Clouter 
> Cc: Jason Cooper 
> Cc: Andrew Lunn 
> Cc: Sebastian Hesselbarth 
> Cc: Gregory Clement 
> Cc: linux-arm-ker...@lists.infradead.org
> 
>  arch/arm/mach-ep93xx/ts72xx.c  | 2 +-
>  arch/arm/mach-orion5x/ts78xx-setup.c   | 2 +-
>  drivers/rtc/rtc-m48t86.c   | 2 +-
>  include/linux/{m48t86.h => platform_data/rtc-m48t86.h} | 0
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename include/linux/{m48t86.h => platform_data/rtc-m48t86.h} (100%)

For the orion5x bit,

Acked-by: Jason Cooper 

thx,

Jason.

Re: [PATCH 1/1] regulator: max77620: check for valid regulator info

[Mark Brown]

On Mon, Jun 27, 2016 at 05:13:44PM +0530, Venkat Reddy Talla wrote:
> Check for valid regulator information data before
> configuring FPS source and FPS power up/down
> period to avoid NULL pointer exception if entries for
> PMIC regulators not provided through device tree.

This sounds like it's papering over a bug in the driver - the driver
should be able to instantiate without anything beyond the registration
of the device.  What's the driver relying on from the DT?

> SD4 regulator is not supported by MAX77620 PMIC, removing
> SD4 entry from regulator information list.

This appears to be a separate change to the above and should be a
separate patch.


signature.asc
Description: PGP signature

Re: [PATCH 1/1] regulator: max77620: check for valid regulator info

[Mark Brown]

On Mon, Jun 27, 2016 at 05:13:44PM +0530, Venkat Reddy Talla wrote:
> Check for valid regulator information data before
> configuring FPS source and FPS power up/down
> period to avoid NULL pointer exception if entries for
> PMIC regulators not provided through device tree.

This sounds like it's papering over a bug in the driver - the driver
should be able to instantiate without anything beyond the registration
of the device.  What's the driver relying on from the DT?

> SD4 regulator is not supported by MAX77620 PMIC, removing
> SD4 entry from regulator information list.

This appears to be a separate change to the above and should be a
separate patch.


signature.asc
Description: PGP signature

Re: [PATCH] geneve: fix max_mtu setting

[Jesse Gross]

On Sun, Jun 26, 2016 at 6:13 PM, 严海双  wrote:
>
>> On Jun 26, 2016, at 8:35 PM, zhuyj  wrote:
>>
>> +   if (geneve->remote.sa.sa_family == AF_INET)
>> +   max_mtu -= sizeof(struct iphdr);
>> +   else
>> +   max_mtu -= sizeof(struct ipv6hdr);
>>
>> Sorry, if sa_family is not AF_NET, it is AF_INET6?
>>
>> There is a lot of macros in include/linux/socket.h.
>>
>> Zhu Yanjun
>>
>
> There are only two enumerations AF_INET and AF_INET6 have been assigned in 
> geneve_newlink:

There's actually a third possibility: AF_UNSPEC, which is the default
if neither remote type is specified. This is used by lightweight
tunnels and should be able to work with either IPv4/v6. For the
purposes of the MTU calculation this means that the IPv4 header size
should be used to avoid disallowing potentially valid configurations.

Re: [PATCH] geneve: fix max_mtu setting

[Jesse Gross]

On Sun, Jun 26, 2016 at 6:13 PM, 严海双  wrote:
>
>> On Jun 26, 2016, at 8:35 PM, zhuyj  wrote:
>>
>> +   if (geneve->remote.sa.sa_family == AF_INET)
>> +   max_mtu -= sizeof(struct iphdr);
>> +   else
>> +   max_mtu -= sizeof(struct ipv6hdr);
>>
>> Sorry, if sa_family is not AF_NET, it is AF_INET6?
>>
>> There is a lot of macros in include/linux/socket.h.
>>
>> Zhu Yanjun
>>
>
> There are only two enumerations AF_INET and AF_INET6 have been assigned in 
> geneve_newlink:

There's actually a third possibility: AF_UNSPEC, which is the default
if neither remote type is specified. This is used by lightweight
tunnels and should be able to work with either IPv4/v6. For the
purposes of the MTU calculation this means that the IPv4 header size
should be used to avoid disallowing potentially valid configurations.

Re: [PATCH v5 00/10] NTB Selftest Script

[Logan Gunthorpe]

Great! Thanks Jon.

Logan

On 26/06/16 05:29 PM, Jon Mason wrote:
> On Mon, Jun 20, 2016 at 01:15:03PM -0600, Logan Gunthorpe wrote:
>> Sorry, I thought this was done but I found one more minor issue with
>> these patches so I'm resubmitting them one last time. Besides this isuse,
>> I think I have acks for all the patches and everything is working as I'd
>> like.
> 
> Applying patch 05/10 to my ntb branch (as it is a bug fix and should
> go into 4.7)
> 
> Applying the rest to my ntb-next branch (which should go into 4.8)
> 
> Note: I'm including patch 09/10 in my tree, even though it is for the
> selftest subsystem.  Given that Shuah Khan acked it, I assume this is
> the desired outcome.
> 
> Thanks,
> Jon
> 
>>
>> Changes since v4:
>>
>> 1) In patch 0006, when setting a translation fails in tool_setup_mw,
>> we now properly clean up the peer dma memory. Before that patch, it
>> wasn't required because if tool_setup_mw failed the module would clean
>> up all mws. After this patch, the clean up never happened, so it would
>> return an error to the user but the DMA memory would still be allocated
>> and the peer_trans file would report that it's ready but the debugfs
>> file would not have been created. In v5, after an error, no DMA memory
>> is allocated and it will still report that it's not ready to the user when
>> reading peer_trans.
>>
>> ---
>>
>> Changes since v3:
>>
>> 1) Check the error returned when setting the link in ntb_tool and pass
>> it back to the user.
>>
>> 2) If an error occurs when setting the link down during a link test in
>> ntb_test, just print unsupported and continue on. (For hardware that
>> does not support setting the link down.)
>>
>> 3) Fix a race condition problem introduced by clearing the link is
>> up flag in ntb_perf. We do this by getting rid of the link cleanup
>> work and doing the few actions in the link event handler.
>> Dave Jiang has already ok'd this approach.
>>
>> ---
>>
>> Changes since v2:
>>
>> 1) As per Allan's suggestion, I've added a patch to postpone the
>> peer memory window setup until the user requests it with a 'peer_trans*'
>> file. This pushes the ntb_tool API closer to the kernel API and allows
>> the link status file to return the raw status of the NTB link.
>>
>> 2) Change the link status file to return the value of ntb_link_is_up
>> instead of the local 'memory window ready' value.
>>
>> 3) Change the link_event file to just block on write. As it was in v2,
>> if multiple users attempted to use the link_event file they could
>> corrupt the state another user had set. Reads to this file are no
>> longer permitted.
>>
>> 4) Updates to the ntb_test script to accommodate the above changes.
>>
>> 5) Added some link tests to the ntb_test script. It will bring the link
>> down and check that the other side agrees.
>>
>> 6) Added a minor bug fix (Patch #10) to ntb_perf. During discussions
>> with Allen it was noticed that the link_is_up flag is never cleared if
>> the link goes away.
>>
>> ---
>>
>> Changes since v1:
>>
>> 1) Add a comment to explain the *15 in the buf size calculation,
>> as per Allen's feedback.
>>
>> 2) Clean up the changes to the pingpong client as there were some
>> sloppy copying mistakes.
>>
>> 3) Rework the 'link' file in ntb_tool as per Allen's suggestions.
>> I've added a 'link_event' file the works essentially how he's asked.
>> Though, I found no need to use a completion as suggested and the flow
>> is maybe slightly simpler than he's suggested. Just write a boolean
>> to the event file then read to wait for the link to be either up or
>> down. There's still some discussion on the best interface and it's
>> not much work to make additional minor functional changes.
>>
>> 4) Update the selftest script to use the new 'link_event' file.
>>
>> 5) Minor change to the way the selftest script lists devices thanks to
>> Allen's observation.
>>
>> ---
>>
>> I've written a ntb_test.sh script that would probably be useful if it
>> were included in the kernel. This series ends with that script and
>> includes some useful interface improvements and fixes to the existing ntb
>> test modules. Please see each individual commit for more information.
>> They are mostly independent.
>>
>> The series is based off of v4.6 plus the patches I've submitted that
>> have been accepted into ntb-next. They've been run through checkpatch
>> with --strict this time.
>>
>> As always, I'm happy to incorporate any feedback.
>>
>> Thanks,
>>
>> Logan
>>
>> ---
>>
>> Logan Gunthorpe (10):
>>   ntb_perf: Schedule based on time not on performance
>>   ntb_perf: Improve thread handling to increase robustness
>>   ntb_perf: Return results by reading the run file
>>   ntb_perf: Wait for link before running test
>>   ntb_tool: BUG: Ensure the buffer size is large enough to return all
>> spads
>>   ntb_tool: Postpone memory window initialization for the user
>>   ntb_tool: Add link status and files to debugfs
>>   ntb_pingpong: Add a debugfs file to get the 

Re: [PATCH v5 00/10] NTB Selftest Script

[Logan Gunthorpe]

Great! Thanks Jon.

Logan

On 26/06/16 05:29 PM, Jon Mason wrote:
> On Mon, Jun 20, 2016 at 01:15:03PM -0600, Logan Gunthorpe wrote:
>> Sorry, I thought this was done but I found one more minor issue with
>> these patches so I'm resubmitting them one last time. Besides this isuse,
>> I think I have acks for all the patches and everything is working as I'd
>> like.
> 
> Applying patch 05/10 to my ntb branch (as it is a bug fix and should
> go into 4.7)
> 
> Applying the rest to my ntb-next branch (which should go into 4.8)
> 
> Note: I'm including patch 09/10 in my tree, even though it is for the
> selftest subsystem.  Given that Shuah Khan acked it, I assume this is
> the desired outcome.
> 
> Thanks,
> Jon
> 
>>
>> Changes since v4:
>>
>> 1) In patch 0006, when setting a translation fails in tool_setup_mw,
>> we now properly clean up the peer dma memory. Before that patch, it
>> wasn't required because if tool_setup_mw failed the module would clean
>> up all mws. After this patch, the clean up never happened, so it would
>> return an error to the user but the DMA memory would still be allocated
>> and the peer_trans file would report that it's ready but the debugfs
>> file would not have been created. In v5, after an error, no DMA memory
>> is allocated and it will still report that it's not ready to the user when
>> reading peer_trans.
>>
>> ---
>>
>> Changes since v3:
>>
>> 1) Check the error returned when setting the link in ntb_tool and pass
>> it back to the user.
>>
>> 2) If an error occurs when setting the link down during a link test in
>> ntb_test, just print unsupported and continue on. (For hardware that
>> does not support setting the link down.)
>>
>> 3) Fix a race condition problem introduced by clearing the link is
>> up flag in ntb_perf. We do this by getting rid of the link cleanup
>> work and doing the few actions in the link event handler.
>> Dave Jiang has already ok'd this approach.
>>
>> ---
>>
>> Changes since v2:
>>
>> 1) As per Allan's suggestion, I've added a patch to postpone the
>> peer memory window setup until the user requests it with a 'peer_trans*'
>> file. This pushes the ntb_tool API closer to the kernel API and allows
>> the link status file to return the raw status of the NTB link.
>>
>> 2) Change the link status file to return the value of ntb_link_is_up
>> instead of the local 'memory window ready' value.
>>
>> 3) Change the link_event file to just block on write. As it was in v2,
>> if multiple users attempted to use the link_event file they could
>> corrupt the state another user had set. Reads to this file are no
>> longer permitted.
>>
>> 4) Updates to the ntb_test script to accommodate the above changes.
>>
>> 5) Added some link tests to the ntb_test script. It will bring the link
>> down and check that the other side agrees.
>>
>> 6) Added a minor bug fix (Patch #10) to ntb_perf. During discussions
>> with Allen it was noticed that the link_is_up flag is never cleared if
>> the link goes away.
>>
>> ---
>>
>> Changes since v1:
>>
>> 1) Add a comment to explain the *15 in the buf size calculation,
>> as per Allen's feedback.
>>
>> 2) Clean up the changes to the pingpong client as there were some
>> sloppy copying mistakes.
>>
>> 3) Rework the 'link' file in ntb_tool as per Allen's suggestions.
>> I've added a 'link_event' file the works essentially how he's asked.
>> Though, I found no need to use a completion as suggested and the flow
>> is maybe slightly simpler than he's suggested. Just write a boolean
>> to the event file then read to wait for the link to be either up or
>> down. There's still some discussion on the best interface and it's
>> not much work to make additional minor functional changes.
>>
>> 4) Update the selftest script to use the new 'link_event' file.
>>
>> 5) Minor change to the way the selftest script lists devices thanks to
>> Allen's observation.
>>
>> ---
>>
>> I've written a ntb_test.sh script that would probably be useful if it
>> were included in the kernel. This series ends with that script and
>> includes some useful interface improvements and fixes to the existing ntb
>> test modules. Please see each individual commit for more information.
>> They are mostly independent.
>>
>> The series is based off of v4.6 plus the patches I've submitted that
>> have been accepted into ntb-next. They've been run through checkpatch
>> with --strict this time.
>>
>> As always, I'm happy to incorporate any feedback.
>>
>> Thanks,
>>
>> Logan
>>
>> ---
>>
>> Logan Gunthorpe (10):
>>   ntb_perf: Schedule based on time not on performance
>>   ntb_perf: Improve thread handling to increase robustness
>>   ntb_perf: Return results by reading the run file
>>   ntb_perf: Wait for link before running test
>>   ntb_tool: BUG: Ensure the buffer size is large enough to return all
>> spads
>>   ntb_tool: Postpone memory window initialization for the user
>>   ntb_tool: Add link status and files to debugfs
>>   ntb_pingpong: Add a debugfs file to get the 

Re: [PATCH 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP

[Stephen Warren]

On 06/27/2016 03:02 AM, Joseph Lo wrote:

The BPMP is a specific processor in Tegra chip, which is designed for
booting process handling and offloading the power management tasks
from the CPU. The binding document defines the resources that would be
used by the BPMP firmware driver, which can create the interprocessor
communication (IPC) between the CPU and BPMP.



diff --git 
a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt 
b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt



+The BPMP is a specific processor in Tegra chip, which is designed for
+booting process handling and offloading the power management tasks from
+the CPU. The binding document defines the resources that would be used by
+the BPMP firmware driver, which can create the interprocessor
+communication (IPC) between the CPU and BPMP.


s/power management/power management, clock management, and reset control/?


+Required properties:
+- name : Should be bpmp
+- compatible : Should be "nvidia,tegra-bpmp"


Again, I'd suggest wording this as:

- compatible
Array of strings.
One of:
  - "nvidia,tegra186-bpmp"


+- mboxes : The phandle of mailbox controller and the channel ID


s/channel ID/mailbox specifier/.


+   See "Documentation/devicetree/bindings/mailbox/
+  nvidia,tegra186-hsp.txt" and "Documentation/devicetree/
+  bindings/mailbox/mailbox.txt" for more details about the generic
+  mailbox controller and mailbox client driver bindings.


I'd rather not split the filenames across lines, since that makes grep 
fail to match. Perhaps add the following text to the introductory 
section at the start of the file to avoid having to mention some of the 
filenames in an indented block of text:


==
This node is a mailbox consumer. See the following file for details of 
the mailbox subsystem, and the specifiers implemented by the relevant 
provider(s):


- Documentation/devicetree/bindings/mailbox/mailbox.txt
- Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt

This node is a clock and reset provider. See the following files for 
general documentation of those features, and the specifiers implemented 
by this node:


- Documentation/devicetree/bindings/clock/clock-bindings.txt
- include/dt-bindings/clock/tegra186-clock.h
- Documentation/devicetree/bindings/reset/reset.txt
- include/dt-bindings/reset/tegra186-reset.h
==

Related, I would expect those two header files (tegra186-clock.h and 
tegra186-reset.h) to be part of this patch, since they form part of the 
definition of this binding.



+The shared memory bindings for BPMP
+---
+
+The shared memory area for the IPC TX and RX between CPU and BPMP are
+predefined and work on top of sysram, which is a sram inside the chip.


s/a sram/an SRAM/.


+Example:

...

+bpmp@d000 {


There should be no unit address ("@d000") in the node name, since 
there's no reg property.

Re: [PATCH 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP

[Stephen Warren]

On 06/27/2016 03:02 AM, Joseph Lo wrote:

The BPMP is a specific processor in Tegra chip, which is designed for
booting process handling and offloading the power management tasks
from the CPU. The binding document defines the resources that would be
used by the BPMP firmware driver, which can create the interprocessor
communication (IPC) between the CPU and BPMP.



diff --git 
a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt 
b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt



+The BPMP is a specific processor in Tegra chip, which is designed for
+booting process handling and offloading the power management tasks from
+the CPU. The binding document defines the resources that would be used by
+the BPMP firmware driver, which can create the interprocessor
+communication (IPC) between the CPU and BPMP.


s/power management/power management, clock management, and reset control/?


+Required properties:
+- name : Should be bpmp
+- compatible : Should be "nvidia,tegra-bpmp"


Again, I'd suggest wording this as:

- compatible
Array of strings.
One of:
  - "nvidia,tegra186-bpmp"


+- mboxes : The phandle of mailbox controller and the channel ID


s/channel ID/mailbox specifier/.


+   See "Documentation/devicetree/bindings/mailbox/
+  nvidia,tegra186-hsp.txt" and "Documentation/devicetree/
+  bindings/mailbox/mailbox.txt" for more details about the generic
+  mailbox controller and mailbox client driver bindings.


I'd rather not split the filenames across lines, since that makes grep 
fail to match. Perhaps add the following text to the introductory 
section at the start of the file to avoid having to mention some of the 
filenames in an indented block of text:


==
This node is a mailbox consumer. See the following file for details of 
the mailbox subsystem, and the specifiers implemented by the relevant 
provider(s):


- Documentation/devicetree/bindings/mailbox/mailbox.txt
- Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt

This node is a clock and reset provider. See the following files for 
general documentation of those features, and the specifiers implemented 
by this node:


- Documentation/devicetree/bindings/clock/clock-bindings.txt
- include/dt-bindings/clock/tegra186-clock.h
- Documentation/devicetree/bindings/reset/reset.txt
- include/dt-bindings/reset/tegra186-reset.h
==

Related, I would expect those two header files (tegra186-clock.h and 
tegra186-reset.h) to be part of this patch, since they form part of the 
definition of this binding.



+The shared memory bindings for BPMP
+---
+
+The shared memory area for the IPC TX and RX between CPU and BPMP are
+predefined and work on top of sysram, which is a sram inside the chip.


s/a sram/an SRAM/.


+Example:

...

+bpmp@d000 {


There should be no unit address ("@d000") in the node name, since 
there's no reg property.

Re: [PATCH v4 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver

[Mark Rutland]

Hi,

On Sat, Jun 25, 2016 at 10:54:20AM -0700, Tai Tri Nguyen wrote:
> On Thu, Jun 23, 2016 at 7:32 AM, Mark Rutland  wrote:
> > On Wed, Jun 22, 2016 at 11:06:58AM -0700, Tai Nguyen wrote:
> > > +#define _GET_CNTR(ev) (ev->hw.extra_reg.reg)
> > > +#define _GET_EVENTID(ev)  (ev->hw.config & 0xFFULL)
> > > +#define _GET_AGENTID(ev)  (ev->hw.extra_reg.config & 0xULL)
> > > +#define _GET_AGENT1ID(ev) ((ev->hw.extra_reg.config >> 32) & 
> > > 0xULL)
> >
> > I don't think you need to use the extra_reg fields for this. It's a
> > little bit confusing to use them, as the extra_reg (and branch_reg)
> > fields are for separately allocated PMU state.
> >
> > _GET_CNTR can use hw_perf_event::idx,  and _GET_AGENT*_ID can use
> > config_base.
> 
> I need a 64 bit field for GET_AGENT*ID. The config_base is only 32 bit.
> Can you please suggest another field?

Judging by  config_base is an unsigned long, which
will be 64 bit for arm64, which is the only place this is used.

So unless I've missed something, that should be ok, no?

> > > +static u64 xgene_perf_event_update(struct perf_event *event)
> > > +{
> > > + struct xgene_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> > > + struct hw_perf_event *hwc = >hw;
> > > + u64 delta, prev_raw_count, new_raw_count;
> > > +
> > > +again:
> > > + prev_raw_count = local64_read(>prev_count);
> > > + new_raw_count = pmu_dev->max_period;
> >
> > I don't understand this. Why are we not reading the counter here?
> >
> > This means that the irq handler won't be reading the counter, which
> > means we're throwing away events, and I suspect other cases are broken
> > too.
> >
> 
> When the overflow interrupt occurs, the PMU counter wraps to 0 and
> continues to run.
> This event_update function is called only to handle the The PMU
> counter overflow interrupt occurs.
> I'm assuming that when the overflow happens, the read back counter
> value is the max period.

Well, it could be a larger value depending on the latency.

> Is this assumption incorrect? Do you have any suggestion what I should
> do. Because if I read the counter register,
> it returns the minor wrapped around value.
> Or should it be: new_count = counter read + max period?

We handle the wraparound when we caluclate the delta below. By setting
the interrupt to occur within half of the max period (as per the arm-cci
driver), we avoid (with a high degree of certainty) the risk of
overtaking the prev raw count again before handlnig the IRQ.

The raw_* values above should be the *raw* values from the HW, as their
names imply.

> > > + if (local64_cmpxchg(>prev_count, prev_raw_count,
> > > + new_raw_count) != prev_raw_count)
> > > + goto again;
> > > +
> > > + delta = (new_raw_count - prev_raw_count) & pmu_dev->max_period;
> > > +
> > > + local64_add(delta, >count);
> > > + local64_sub(delta, >period_left);
> >
> > Given we aren't sampling, does the period left matter? It looks like
> > drivers are inconsistent w.r.t. this, and I'm not immediately sure :(
> 
> I tried to drop it and  the perf event count still works properly for me.
> I'll remove it.

[...]

> > > +static irqreturn_t xgene_pmu_isr(int irq, void *dev_id)
> > > +{
> > > + struct xgene_pmu_dev_ctx *ctx, *temp_ctx;
> > > + struct xgene_pmu *xgene_pmu = dev_id;
> > > + u32 val;
> > > +
> > > + xgene_pmu_mask_int(xgene_pmu);
> >
> > Why do you need to mask the IRQ? This handler is called in hard IRQ
> > context.
> 
> Right. Let me change to use raw_spin_lock_irqsave here.

Interesting; I see we do that in the CCI PMU driver. What are we trying
to protect?

We don't do that in the CPU PMU drivers, and I'm missng something here.
Hopefully I'm just being thick...

Thanks,
Mark.

Re: [PATCH v4 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver

[Mark Rutland]

Hi,

On Sat, Jun 25, 2016 at 10:54:20AM -0700, Tai Tri Nguyen wrote:
> On Thu, Jun 23, 2016 at 7:32 AM, Mark Rutland  wrote:
> > On Wed, Jun 22, 2016 at 11:06:58AM -0700, Tai Nguyen wrote:
> > > +#define _GET_CNTR(ev) (ev->hw.extra_reg.reg)
> > > +#define _GET_EVENTID(ev)  (ev->hw.config & 0xFFULL)
> > > +#define _GET_AGENTID(ev)  (ev->hw.extra_reg.config & 0xULL)
> > > +#define _GET_AGENT1ID(ev) ((ev->hw.extra_reg.config >> 32) & 
> > > 0xULL)
> >
> > I don't think you need to use the extra_reg fields for this. It's a
> > little bit confusing to use them, as the extra_reg (and branch_reg)
> > fields are for separately allocated PMU state.
> >
> > _GET_CNTR can use hw_perf_event::idx,  and _GET_AGENT*_ID can use
> > config_base.
> 
> I need a 64 bit field for GET_AGENT*ID. The config_base is only 32 bit.
> Can you please suggest another field?

Judging by  config_base is an unsigned long, which
will be 64 bit for arm64, which is the only place this is used.

So unless I've missed something, that should be ok, no?

> > > +static u64 xgene_perf_event_update(struct perf_event *event)
> > > +{
> > > + struct xgene_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> > > + struct hw_perf_event *hwc = >hw;
> > > + u64 delta, prev_raw_count, new_raw_count;
> > > +
> > > +again:
> > > + prev_raw_count = local64_read(>prev_count);
> > > + new_raw_count = pmu_dev->max_period;
> >
> > I don't understand this. Why are we not reading the counter here?
> >
> > This means that the irq handler won't be reading the counter, which
> > means we're throwing away events, and I suspect other cases are broken
> > too.
> >
> 
> When the overflow interrupt occurs, the PMU counter wraps to 0 and
> continues to run.
> This event_update function is called only to handle the The PMU
> counter overflow interrupt occurs.
> I'm assuming that when the overflow happens, the read back counter
> value is the max period.

Well, it could be a larger value depending on the latency.

> Is this assumption incorrect? Do you have any suggestion what I should
> do. Because if I read the counter register,
> it returns the minor wrapped around value.
> Or should it be: new_count = counter read + max period?

We handle the wraparound when we caluclate the delta below. By setting
the interrupt to occur within half of the max period (as per the arm-cci
driver), we avoid (with a high degree of certainty) the risk of
overtaking the prev raw count again before handlnig the IRQ.

The raw_* values above should be the *raw* values from the HW, as their
names imply.

> > > + if (local64_cmpxchg(>prev_count, prev_raw_count,
> > > + new_raw_count) != prev_raw_count)
> > > + goto again;
> > > +
> > > + delta = (new_raw_count - prev_raw_count) & pmu_dev->max_period;
> > > +
> > > + local64_add(delta, >count);
> > > + local64_sub(delta, >period_left);
> >
> > Given we aren't sampling, does the period left matter? It looks like
> > drivers are inconsistent w.r.t. this, and I'm not immediately sure :(
> 
> I tried to drop it and  the perf event count still works properly for me.
> I'll remove it.

[...]

> > > +static irqreturn_t xgene_pmu_isr(int irq, void *dev_id)
> > > +{
> > > + struct xgene_pmu_dev_ctx *ctx, *temp_ctx;
> > > + struct xgene_pmu *xgene_pmu = dev_id;
> > > + u32 val;
> > > +
> > > + xgene_pmu_mask_int(xgene_pmu);
> >
> > Why do you need to mask the IRQ? This handler is called in hard IRQ
> > context.
> 
> Right. Let me change to use raw_spin_lock_irqsave here.

Interesting; I see we do that in the CCI PMU driver. What are we trying
to protect?

We don't do that in the CPU PMU drivers, and I'm missng something here.
Hopefully I'm just being thick...

Thanks,
Mark.

Re: [PATCH 01/10] Documentation: dt-bindings: mailbox: tegra: Add binding for HSP mailbox

[Stephen Warren]

On 06/27/2016 03:02 AM, Joseph Lo wrote:

Add DT binding for the Hardware Synchronization Primitives (HSP). The
HSP is designed for the processors to share resources and communicate
together. It provides a set of hardware synchronization primitives for
interprocessor communication. So the interprocessor communication (IPC)
protocols can use hardware synchronization primitive, when operating
between two processors not in an SMP relationship.


This binding is quite different to the binding you sent to internal IP 
review. I wonder why it changed? Specific comments below:



diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt 
b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt



+NVIDIA Tegra Hardware Synchronization Primitives (HSP)
+
+The HSP modules are used for the processors to share resources and communicate
+together. It provides a set of hardware synchronization primitives for
+interprocessor communication. So the interprocessor communication (IPC)
+protocols can use hardware synchronization primitives, when operating between
+two processors not in an SMP relationship.
+
+The features that HSP supported are shared mailboxes, shared semaphores,
+arbitrated semaphores and doorbells.
+
+Required properties:
+- name : Should be hsp
+- compatible : Should be "nvidia,tegra-hsp"


I think this should explicitly list the value values of the compatible 
property, rather than being a generic/wildcard description:


- compatible
Array of strings.
One of:
  - "nvidia,tegra186-hsp"

If/when this binding supports other SoCs in the future, we'll add more 
entries into that list.



+- reg : Offset and length of the register set for the device
+- interrupts : Should contain the HSP interrupts
+- interrupt-names: Should contain the names of the HSP interrupts that the
+  client are using.
+  "doorbell"


The binding should describe the HW, and not be affected by anything 
"that the client(s) are using". If there are multiple interrupts, we 
should list them all here, from the start.


When revising this, I would consider the following wording canonical:

- interrupt-names
Array of strings.
Contains a list of names for the interrupts described by the
interrupts property. May contain the following entries, in any
order:
- "doorbell"
- "..." (no doubt many more items will be listed here, e.g.
  for semaphores, etc.).
Users of this binding MUST look up entries in the interrupts
property by name, using this interrupts-names property to do so.
- interrupts
Array of interrupt specifiers.
Must contain one entry per entry in the interrupt-names property,
in a matching order.


+- nvidia,hsp-function : Specifies one of the HSP functions that the HSP unit
+   will be supported. The function ID can be found in the
+   header file .


This property wasn't in the internal patch.

This doesn't make sense. The HW feature-set is fixed. This sounds like 
some kind of software configuration option, or a way to allow different 
drivers to handle different aspects of the HW? In general, the binding 
shouldn't be influenced by software structure. Please delete this property.


Now, if you're attempting to set up a binding where each function 
(semaphores, shared mailboxes, doorbells, etc.) has a different DT node, 
then (a) splitting up HW modules into sub-blocks has usually turned out 
to be a mistake in the past, and (b) the differences should likely be 
represented by using a different compatible property for each 
sub-component, rather than via a custom property.



The following properties were included in the internal patch:

nvidia,num-SM = <0x8>;
nvidia,num-AS = <0x2>;
nvidia,num-SS = <0x2>;
nvidia,num-DB = <0x7>;
nvidia,num-SI = <0x8>;

... yet aren't here. True the compatible value implies those values; was 
that why the properties were removed?



+Example:
+
+hsp_top: hsp@3c0 {

...

+bpmp@d000 {
+   ...
+   mboxes = <_top HSP_DB_MASTER_BPMP>;
+   ...
+};


I'd suggest not including the bpmp node in the example, since it's not 
part of the HSP node. If you do, recall that bpmp has no reg property 
and hence the node name shouldn't include a unit address.



diff --git a/include/dt-bindings/mailbox/tegra-hsp.h 
b/include/dt-bindings/mailbox/tegra-hsp.h


This file should probably be named tegra186-hsp, since I doubt the 
master ID values will be stable between chips.



+/*
+ * This header provides constants for binding nvidia,tegra-hsp.


That should say "186" not ""


+#ifndef _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
+#define _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H


The two changes mentioned above would be consistent with that include 
guard's name including the text "186".



+#define HSP_SHARED_MAILBOX 0
+#define HSP_SHARED_SEMAPHORE   1

Re: [PATCH 01/10] Documentation: dt-bindings: mailbox: tegra: Add binding for HSP mailbox

[Stephen Warren]

On 06/27/2016 03:02 AM, Joseph Lo wrote:

Add DT binding for the Hardware Synchronization Primitives (HSP). The
HSP is designed for the processors to share resources and communicate
together. It provides a set of hardware synchronization primitives for
interprocessor communication. So the interprocessor communication (IPC)
protocols can use hardware synchronization primitive, when operating
between two processors not in an SMP relationship.


This binding is quite different to the binding you sent to internal IP 
review. I wonder why it changed? Specific comments below:



diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt 
b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt



+NVIDIA Tegra Hardware Synchronization Primitives (HSP)
+
+The HSP modules are used for the processors to share resources and communicate
+together. It provides a set of hardware synchronization primitives for
+interprocessor communication. So the interprocessor communication (IPC)
+protocols can use hardware synchronization primitives, when operating between
+two processors not in an SMP relationship.
+
+The features that HSP supported are shared mailboxes, shared semaphores,
+arbitrated semaphores and doorbells.
+
+Required properties:
+- name : Should be hsp
+- compatible : Should be "nvidia,tegra-hsp"


I think this should explicitly list the value values of the compatible 
property, rather than being a generic/wildcard description:


- compatible
Array of strings.
One of:
  - "nvidia,tegra186-hsp"

If/when this binding supports other SoCs in the future, we'll add more 
entries into that list.



+- reg : Offset and length of the register set for the device
+- interrupts : Should contain the HSP interrupts
+- interrupt-names: Should contain the names of the HSP interrupts that the
+  client are using.
+  "doorbell"


The binding should describe the HW, and not be affected by anything 
"that the client(s) are using". If there are multiple interrupts, we 
should list them all here, from the start.


When revising this, I would consider the following wording canonical:

- interrupt-names
Array of strings.
Contains a list of names for the interrupts described by the
interrupts property. May contain the following entries, in any
order:
- "doorbell"
- "..." (no doubt many more items will be listed here, e.g.
  for semaphores, etc.).
Users of this binding MUST look up entries in the interrupts
property by name, using this interrupts-names property to do so.
- interrupts
Array of interrupt specifiers.
Must contain one entry per entry in the interrupt-names property,
in a matching order.


+- nvidia,hsp-function : Specifies one of the HSP functions that the HSP unit
+   will be supported. The function ID can be found in the
+   header file .


This property wasn't in the internal patch.

This doesn't make sense. The HW feature-set is fixed. This sounds like 
some kind of software configuration option, or a way to allow different 
drivers to handle different aspects of the HW? In general, the binding 
shouldn't be influenced by software structure. Please delete this property.


Now, if you're attempting to set up a binding where each function 
(semaphores, shared mailboxes, doorbells, etc.) has a different DT node, 
then (a) splitting up HW modules into sub-blocks has usually turned out 
to be a mistake in the past, and (b) the differences should likely be 
represented by using a different compatible property for each 
sub-component, rather than via a custom property.



The following properties were included in the internal patch:

nvidia,num-SM = <0x8>;
nvidia,num-AS = <0x2>;
nvidia,num-SS = <0x2>;
nvidia,num-DB = <0x7>;
nvidia,num-SI = <0x8>;

... yet aren't here. True the compatible value implies those values; was 
that why the properties were removed?



+Example:
+
+hsp_top: hsp@3c0 {

...

+bpmp@d000 {
+   ...
+   mboxes = <_top HSP_DB_MASTER_BPMP>;
+   ...
+};


I'd suggest not including the bpmp node in the example, since it's not 
part of the HSP node. If you do, recall that bpmp has no reg property 
and hence the node name shouldn't include a unit address.



diff --git a/include/dt-bindings/mailbox/tegra-hsp.h 
b/include/dt-bindings/mailbox/tegra-hsp.h


This file should probably be named tegra186-hsp, since I doubt the 
master ID values will be stable between chips.



+/*
+ * This header provides constants for binding nvidia,tegra-hsp.


That should say "186" not ""


+#ifndef _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
+#define _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H


The two changes mentioned above would be consistent with that include 
guard's name including the text "186".



+#define HSP_SHARED_MAILBOX 0
+#define HSP_SHARED_SEMAPHORE   1

[PATCH] net: the space is required before the open parenthesis '('

[Wei Tang]

The space is missing before the open parenthesis '(', and this
will introduce much more noise when checking patch around.

Signed-off-by: Wei Tang 
---
 net/core/utils.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/utils.c b/net/core/utils.c
index 3d17ca8..cf5622b 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -133,7 +133,7 @@ int in4_pton(const char *src, int srclen,
s = src;
d = dbuf;
i = 0;
-   while(1) {
+   while (1) {
int c;
c = xdigit2bin(srclen > 0 ? *s : '\0', delim);
if (!(c & (IN6PTON_DIGIT | IN6PTON_DOT | IN6PTON_DELIM | 
IN6PTON_COLON_MASK))) {
@@ -283,11 +283,11 @@ cont:
i = 15; d--;
 
if (dc) {
-   while(d >= dc)
+   while (d >= dc)
dst[i--] = *d--;
-   while(i >= dc - dbuf)
+   while (i >= dc - dbuf)
dst[i--] = 0;
-   while(i >= 0)
+   while (i >= 0)
dst[i--] = *d--;
} else
memcpy(dst, dbuf, sizeof(dbuf));
-- 
1.9.1

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
>> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
>>> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
  #ifdef CONFIG_X86_64
  /* Runs on IST stack */
  dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
  {
 static const char str[] = "double fault";
 struct task_struct *tsk = current;
 +#ifdef CONFIG_VMAP_STACK
 +   unsigned long cr2;
 +#endif

  #ifdef CONFIG_X86_ESPFIX64
 extern unsigned char native_irq_return_iret[];
 @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
 *regs, long error_code)
 tsk->thread.error_code = error_code;
 tsk->thread.trap_nr = X86_TRAP_DF;

 +#ifdef CONFIG_VMAP_STACK
 +   /*
 +* If we overflow the stack into a guard page, the CPU will fail
 +* to deliver #PF and will send #DF instead.  CR2 will contain
 +* the linear address of the second fault, which will be in the
 +* guard page below the bottom of the stack.
 +*/
 +   cr2 = read_cr2();
 +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
 +   handle_stack_overflow(
 +   "kernel stack overflow (double-fault)",
 +   regs, cr2);
 +#endif
>>>
>>> Is there any other way to tell if this was from a page fault?  If it
>>> wasn't a page fault then CR2 is undefined.
>>
>> I guess it doesn't really matter, since the fault is fatal either way.
>> The error message might be incorrect though.
>>
>
> It's at least worth a comment, though.  Maybe I should check if
> regs->rsp is within 40 bytes of the bottom of the stack, too, such
> that delivery of an inner fault would have double-faulted assuming the
> inner fault didn't use an IST vector.
>

How about:

/*
 * If we overflow the stack into a guard page, the CPU will fail
 * to deliver #PF and will send #DF instead.  CR2 will contain
 * the linear address of the second fault, which will be in the
 * guard page below the bottom of the stack.
 *
 * We're limited to using heuristics here, since the CPU does
 * not tell us what type of fault failed and, if the first fault
 * wasn't a page fault, CR2 may contain stale garbage.  To mostly
 * rule out garbage, we check if the saved RSP is close enough to
 * the bottom of the stack to cause exception delivery to fail.
 * The worst case is 7 stack slots: one for alignment, five for
 * SS..RIP, and one for the error code.
 */
tsk_stack = (unsigned long)task_stack_page(tsk);
if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */
cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

--Andy
-- 
Andy Lutomirski
AMA Capital Management, LLC

[PATCH] net: the space is required before the open parenthesis '('

[Wei Tang]

The space is missing before the open parenthesis '(', and this
will introduce much more noise when checking patch around.

Signed-off-by: Wei Tang 
---
 net/core/utils.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/utils.c b/net/core/utils.c
index 3d17ca8..cf5622b 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -133,7 +133,7 @@ int in4_pton(const char *src, int srclen,
s = src;
d = dbuf;
i = 0;
-   while(1) {
+   while (1) {
int c;
c = xdigit2bin(srclen > 0 ? *s : '\0', delim);
if (!(c & (IN6PTON_DIGIT | IN6PTON_DOT | IN6PTON_DELIM | 
IN6PTON_COLON_MASK))) {
@@ -283,11 +283,11 @@ cont:
i = 15; d--;
 
if (dc) {
-   while(d >= dc)
+   while (d >= dc)
dst[i--] = *d--;
-   while(i >= dc - dbuf)
+   while (i >= dc - dbuf)
dst[i--] = 0;
-   while(i >= 0)
+   while (i >= 0)
dst[i--] = *d--;
} else
memcpy(dst, dbuf, sizeof(dbuf));
-- 
1.9.1

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 8:22 AM, Andy Lutomirski  wrote:
> On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
>> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
>>> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
  #ifdef CONFIG_X86_64
  /* Runs on IST stack */
  dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
  {
 static const char str[] = "double fault";
 struct task_struct *tsk = current;
 +#ifdef CONFIG_VMAP_STACK
 +   unsigned long cr2;
 +#endif

  #ifdef CONFIG_X86_ESPFIX64
 extern unsigned char native_irq_return_iret[];
 @@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs 
 *regs, long error_code)
 tsk->thread.error_code = error_code;
 tsk->thread.trap_nr = X86_TRAP_DF;

 +#ifdef CONFIG_VMAP_STACK
 +   /*
 +* If we overflow the stack into a guard page, the CPU will fail
 +* to deliver #PF and will send #DF instead.  CR2 will contain
 +* the linear address of the second fault, which will be in the
 +* guard page below the bottom of the stack.
 +*/
 +   cr2 = read_cr2();
 +   if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
 +   handle_stack_overflow(
 +   "kernel stack overflow (double-fault)",
 +   regs, cr2);
 +#endif
>>>
>>> Is there any other way to tell if this was from a page fault?  If it
>>> wasn't a page fault then CR2 is undefined.
>>
>> I guess it doesn't really matter, since the fault is fatal either way.
>> The error message might be incorrect though.
>>
>
> It's at least worth a comment, though.  Maybe I should check if
> regs->rsp is within 40 bytes of the bottom of the stack, too, such
> that delivery of an inner fault would have double-faulted assuming the
> inner fault didn't use an IST vector.
>

How about:

/*
 * If we overflow the stack into a guard page, the CPU will fail
 * to deliver #PF and will send #DF instead.  CR2 will contain
 * the linear address of the second fault, which will be in the
 * guard page below the bottom of the stack.
 *
 * We're limited to using heuristics here, since the CPU does
 * not tell us what type of fault failed and, if the first fault
 * wasn't a page fault, CR2 may contain stale garbage.  To mostly
 * rule out garbage, we check if the saved RSP is close enough to
 * the bottom of the stack to cause exception delivery to fail.
 * The worst case is 7 stack slots: one for alignment, five for
 * SS..RIP, and one for the error code.
 */
tsk_stack = (unsigned long)task_stack_page(tsk);
if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */
cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

--Andy
-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] sched/deadline: remove useless param from setup_new_dl_entity

[Peter Zijlstra]

On Fri, Jun 17, 2016 at 05:28:37PM +0100, Juri Lelli wrote:
> On 17/06/16 09:49, Steven Rostedt wrote:
> > On Fri, 17 Jun 2016 10:48:41 +0100
> > Juri Lelli  wrote:
> > 
> > > setup_new_dl_entity() takes two parameters, but it only actually uses
> > > one of them to setup a new dl_entity.
> > > 
> > 
> > Actually this patch is making it so that setup_new_dl_entity() only
> > uses one of the parameters. Can you note why that change happened.
> > Because this change log implies that the second parameter wasn't used
> > before this patch, and that is incorrect.
> > 
> 
> True, but we were practically already using the same parameter, under a
> different name though, after
> 
> 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
> 
> as we currently do:
> 
>   setup_new_dl_entity(>dl, >dl)
> 
> > This patch reverts part of the change done in
> > commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
> > logic"
> > 
> 
> Before Luca's change we were doing
> 
>  setup_new_dl_entity(dl_se, pi_se)
> 
> in update_dl_entity() for a dl_se->new entity. So, I guess the question
> is actually why we wanted to use pi_se's parameters (the potential PI
> donor) for setting up a new entity? Maybe we broke the situation where a
> task is currently boosted by a DEADLINE waiter and we swich the holder
> to DEADLINE?
> 
> > It would be nice to have the reason in the change log.
> > 
> 
> Thanks a lot for pointing out what might be more than inaccuracy in the
> changelog.

Will you be reposting with a new Changelog?

Re: [PATCH] sched/deadline: remove useless param from setup_new_dl_entity

[Peter Zijlstra]

On Fri, Jun 17, 2016 at 05:28:37PM +0100, Juri Lelli wrote:
> On 17/06/16 09:49, Steven Rostedt wrote:
> > On Fri, 17 Jun 2016 10:48:41 +0100
> > Juri Lelli  wrote:
> > 
> > > setup_new_dl_entity() takes two parameters, but it only actually uses
> > > one of them to setup a new dl_entity.
> > > 
> > 
> > Actually this patch is making it so that setup_new_dl_entity() only
> > uses one of the parameters. Can you note why that change happened.
> > Because this change log implies that the second parameter wasn't used
> > before this patch, and that is incorrect.
> > 
> 
> True, but we were practically already using the same parameter, under a
> different name though, after
> 
> 2f9f3fdc928 "sched/deadline: Remove dl_new from struct sched_dl_entity"
> 
> as we currently do:
> 
>   setup_new_dl_entity(>dl, >dl)
> 
> > This patch reverts part of the change done in
> > commit 2d3d891d334 "sched/deadline: Add SCHED_DEADLINE inheritance
> > logic"
> > 
> 
> Before Luca's change we were doing
> 
>  setup_new_dl_entity(dl_se, pi_se)
> 
> in update_dl_entity() for a dl_se->new entity. So, I guess the question
> is actually why we wanted to use pi_se's parameters (the potential PI
> donor) for setting up a new entity? Maybe we broke the situation where a
> task is currently boosted by a DEADLINE waiter and we swich the holder
> to DEADLINE?
> 
> > It would be nice to have the reason in the change log.
> > 
> 
> Thanks a lot for pointing out what might be more than inaccuracy in the
> changelog.

Will you be reposting with a new Changelog?

Re: [RFC PATCH 2/2] perf: Filter events based on perf-namespace

[Peter Zijlstra]

On Tue, Jun 14, 2016 at 10:19:51PM +0530, Aravinda Prasad wrote:
> Whenever perf tool is executed inside a container, this
> patch restricts the events to the perf-namespace in which
> the perf tool is executing.
> 
> This patch is based on the existing support available
> for tracing with cgroups.
> 
> TODO:
> - Avoid code duplication.

Can't you, at perf_event_open() time, convert a per-cpu event into a
per-cpu-per-cgroup event for these namespace thingies?

That seems to immediately and completely remove all that duplication.

Re: [RFC PATCH 2/2] perf: Filter events based on perf-namespace

[Peter Zijlstra]

On Tue, Jun 14, 2016 at 10:19:51PM +0530, Aravinda Prasad wrote:
> Whenever perf tool is executed inside a container, this
> patch restricts the events to the perf-namespace in which
> the perf tool is executing.
> 
> This patch is based on the existing support available
> for tracing with cgroups.
> 
> TODO:
> - Avoid code duplication.

Can't you, at perf_event_open() time, convert a per-cpu event into a
per-cpu-per-cgroup event for these namespace thingies?

That seems to immediately and completely remove all that duplication.

Re: linux-next: manual merge of the jc_docs tree with the drm tree

[Jonathan Corbet]

On Mon, 27 Jun 2016 13:31:24 +1000
Stephen Rothwell  wrote:

> Today's linux-next merge of the jc_docs tree got a conflict in:
> 
>   Documentation/index.rst
> 
> between commit:
> 
>   cb597fcea5c2 ("Documentation/gpu: add new gpu.rst converted from DocBook 
> gpu.tmpl")
> 
> from the drm tree and commit:
> 
>   17defc282fe6 ("Documentation: add meta-documentation for Sphinx and 
> kernel-doc")
> 
> from the jc_docs tree.
> 
> I fixed it up (see below) and can carry the fix as necessary. 

Sigh, I fear that index.rst could become a bit of a conflict magnet, at
least in the near future as documents are being added.

The fix is obviously correct, thanks.

jon

Re: linux-next: manual merge of the jc_docs tree with the drm tree

[Jonathan Corbet]

On Mon, 27 Jun 2016 13:31:24 +1000
Stephen Rothwell  wrote:

> Today's linux-next merge of the jc_docs tree got a conflict in:
> 
>   Documentation/index.rst
> 
> between commit:
> 
>   cb597fcea5c2 ("Documentation/gpu: add new gpu.rst converted from DocBook 
> gpu.tmpl")
> 
> from the drm tree and commit:
> 
>   17defc282fe6 ("Documentation: add meta-documentation for Sphinx and 
> kernel-doc")
> 
> from the jc_docs tree.
> 
> I fixed it up (see below) and can carry the fix as necessary. 

Sigh, I fear that index.rst could become a bit of a conflict magnet, at
least in the near future as documents are being added.

The fix is obviously correct, thanks.

jon

Re: linux-next: build failure after merge of the nfs tree

[Trond Myklebust]

> On Jun 26, 2016, at 20:58, Stephen Rothwell  wrote:
> 
> Hi Trond,
> 
> After merging the nfs tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
> 
> fs/nfs/pnfs_nfs.c: In function 'pnfs_nfs_generic_sync':
> fs/nfs/pnfs_nfs.c:947:8: error: too few arguments to function 
> 'nfs_commit_inode'
> ret = nfs_commit_inode(inode);
>   ^
> In file included from fs/nfs/pnfs_nfs.c:10:0:
> include/linux/nfs_fs.h:509:13: note: declared here
> extern int  nfs_commit_inode(struct inode *, int);
>^
> 
> Caused by commit
> 
> 292cc573fce6 ("pNFS: Files and flexfiles always need to commit before 
> layoutcommit")
> 
> I have used the nsf tree from next-20160624 for today
> 

Hi Stephen,

My apologies: That was a last minute typo when amending a commit. I’ve fixed 
the code in today’s linux-next branch.

Cheers
 Trond

Re: linux-next: build failure after merge of the nfs tree

[Trond Myklebust]

> On Jun 26, 2016, at 20:58, Stephen Rothwell  wrote:
> 
> Hi Trond,
> 
> After merging the nfs tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
> 
> fs/nfs/pnfs_nfs.c: In function 'pnfs_nfs_generic_sync':
> fs/nfs/pnfs_nfs.c:947:8: error: too few arguments to function 
> 'nfs_commit_inode'
> ret = nfs_commit_inode(inode);
>   ^
> In file included from fs/nfs/pnfs_nfs.c:10:0:
> include/linux/nfs_fs.h:509:13: note: declared here
> extern int  nfs_commit_inode(struct inode *, int);
>^
> 
> Caused by commit
> 
> 292cc573fce6 ("pNFS: Files and flexfiles always need to commit before 
> layoutcommit")
> 
> I have used the nsf tree from next-20160624 for today
> 

Hi Stephen,

My apologies: That was a last minute typo when amending a commit. I’ve fixed 
the code in today’s linux-next branch.

Cheers
 Trond

Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18)

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 7:54 AM, Oleg Nesterov  wrote:
> On 06/26, Andy Lutomirski wrote:
>>
>> kthread_stop is *sick*.
>>
>> struct kthread self;
>>
>> ...
>>
>> current->vfork_done = 
>>
>> ...
>>
>> do_exit(ret);
>>
>> And then some other thread goes and waits for the completion, which is
>> *on the stack*, which, in any sane world (e.g. with my series
>> applied), is long gone by then.
>
> Yes, I forgot this when we discussed the problems with ti->flags/etc...
>
>> But this is broken even without any changes: since when is gcc
>> guaranteed to preserve the stack contents when a function ends with a
>> sibling call, let alone with a __noreturn call?
>
> I don't know if gcc can actually drop the stack frame in this case,
> but even if it can this looks fixeable.
>
>> Is there seriously no way to directly wait for a struct task_struct to
>> exit?  Could we, say, kmalloc the completion (or maybe even the whole
>> struct kthread) and (ick!) hang it off ->vfork_done?
>
> Sure we can... And yes, I think we need to alloc the whole struct kthread.
> Just another (unfortunate) complication, the current code is simple.
>
> And probably kthread/kthread_stop should switch to task_work_exit().

Want to send a patch?  I could do it, but you understand this code
much better than I do.

Re: kthread_stop insanity (Re: [[DEBUG] force] 2642458962: BUG: unable to handle kernel paging request at ffffc90000997f18)

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 7:54 AM, Oleg Nesterov  wrote:
> On 06/26, Andy Lutomirski wrote:
>>
>> kthread_stop is *sick*.
>>
>> struct kthread self;
>>
>> ...
>>
>> current->vfork_done = 
>>
>> ...
>>
>> do_exit(ret);
>>
>> And then some other thread goes and waits for the completion, which is
>> *on the stack*, which, in any sane world (e.g. with my series
>> applied), is long gone by then.
>
> Yes, I forgot this when we discussed the problems with ti->flags/etc...
>
>> But this is broken even without any changes: since when is gcc
>> guaranteed to preserve the stack contents when a function ends with a
>> sibling call, let alone with a __noreturn call?
>
> I don't know if gcc can actually drop the stack frame in this case,
> but even if it can this looks fixeable.
>
>> Is there seriously no way to directly wait for a struct task_struct to
>> exit?  Could we, say, kmalloc the completion (or maybe even the whole
>> struct kthread) and (ick!) hang it off ->vfork_done?
>
> Sure we can... And yes, I think we need to alloc the whole struct kthread.
> Just another (unfortunate) complication, the current code is simple.
>
> And probably kthread/kthread_stop should switch to task_work_exit().

Want to send a patch?  I could do it, but you understand this code
much better than I do.

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Dinh Nguyen]

Hi Boris,

On 06/22/2016 08:58 AM, ttha...@opensource.altera.com wrote:
> From: Thor Thayer 
> 
> Add the device tree entries needed to support the Altera Ethernet
> FIFO buffer EDAC on the Arria10 chip.
> 
> Signed-off-by: Thor Thayer 
> ---
> v2  No change
> v3  Add interrupts for SBERR and DBERR.
> v4  No change
> v5  Change "parent" phandle to "altr,ecc-parent"
> ---
>  arch/arm/boot/dts/socfpga_arria10.dtsi |   16 
>  1 file changed, 16 insertions(+)
> 

I've applied this patch and will take through the arm-soc tree.

Thanks,
Dinh

Re: [PATCHv5 8/8] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[Dinh Nguyen]

Hi Boris,

On 06/22/2016 08:58 AM, ttha...@opensource.altera.com wrote:
> From: Thor Thayer 
> 
> Add the device tree entries needed to support the Altera Ethernet
> FIFO buffer EDAC on the Arria10 chip.
> 
> Signed-off-by: Thor Thayer 
> ---
> v2  No change
> v3  Add interrupts for SBERR and DBERR.
> v4  No change
> v5  Change "parent" phandle to "altr,ecc-parent"
> ---
>  arch/arm/boot/dts/socfpga_arria10.dtsi |   16 
>  1 file changed, 16 insertions(+)
> 

I've applied this patch and will take through the arm-soc tree.

Thanks,
Dinh

Re: [PATCH v3 1/2] remoteproc: qcom: Driver for the self-authenticating Hexagon v5

[Srinivas Kandagatla]

On 20/06/16 22:28, Bjorn Andersson wrote:

From: Bjorn Andersson 

This initial hack powers the q6v5, boots and authenticate the mba and
use that to load the mdt and subsequent bXX files.

Signed-off-by: Bjorn Andersson 
Signed-off-by: Bjorn Andersson 


Patch looks good to me,

Acked-by: Srinivas Kandagatla 



---

Changes since v2:
- Balancing the rom_clk calls
- Moved reset wait to the reset function
- Various cleanups per Srinivas review
- MODULE_DESCRIPTION and MODULE_LICENSE

Changes since v1:
- Corrected boot address in relocation case
- Using rproc_da_to_va() to clean up mdt loader api
- Dynamically allocating scratch space for mdt verification

  drivers/remoteproc/Kconfig   |  12 +
  drivers/remoteproc/Makefile  |   2 +
  drivers/remoteproc/qcom_mdt_loader.c | 179 +++
  drivers/remoteproc/qcom_mdt_loader.h |  13 +
  drivers/remoteproc/qcom_q6v5_pil.c   | 916 +++
  5 files changed, 1122 insertions(+)
  create mode 100644 drivers/remoteproc/qcom_mdt_loader.c
  create mode 100644 drivers/remoteproc/qcom_mdt_loader.h
  create mode 100644 drivers/remoteproc/qcom_q6v5_pil.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 72e97d7a5209..9ec66d99b978 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -77,6 +77,18 @@ config DA8XX_REMOTEPROC
  It's safe to say n here if you're not interested in multimedia
  offloading.

+config QCOM_MDT_LOADER
+   tristate
+
+config QCOM_Q6V5_PIL
+   tristate "Qualcomm Hexagon V5 Peripherial Image Loader"
+   depends on OF && ARCH_QCOM
+   select REMOTEPROC
+   select QCOM_MDT_LOADER
+   help
+ Say y here to support the Qualcomm Peripherial Image Loader for the
+ Hexagon V5 based remote processors.
+
  config ST_REMOTEPROC
tristate "ST remoteproc support"
depends on ARCH_STI
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 279cb2edc880..92d3758bd15c 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,4 +11,6 @@ obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
  obj-$(CONFIG_STE_MODEM_RPROC) += ste_modem_rproc.o
  obj-$(CONFIG_WKUP_M3_RPROC)   += wkup_m3_rproc.o
  obj-$(CONFIG_DA8XX_REMOTEPROC)+= da8xx_remoteproc.o
+obj-$(CONFIG_QCOM_MDT_LOADER)  += qcom_mdt_loader.o
+obj-$(CONFIG_QCOM_Q6V5_PIL)+= qcom_q6v5_pil.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
diff --git a/drivers/remoteproc/qcom_mdt_loader.c 
b/drivers/remoteproc/qcom_mdt_loader.c
new file mode 100644
index ..114e8e4cef67
--- /dev/null
+++ b/drivers/remoteproc/qcom_mdt_loader.c
@@ -0,0 +1,179 @@
+/*
+ * Qualcomm Peripheral Image Loader
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Copyright (C) 2015 Sony Mobile Communications Inc
+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+#include "qcom_mdt_loader.h"
+
+/**
+ * qcom_mdt_find_rsc_table() - provide dummy resource table for remoteproc
+ * @rproc: remoteproc handle
+ * @fw:firmware header
+ * @tablesz:   outgoing size of the table
+ *
+ * Returns a dummy table.
+ */
+struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
+  const struct firmware *fw,
+  int *tablesz)
+{
+   static struct resource_table table = { .ver = 1, };
+
+   *tablesz = sizeof(table);
+   return 
+}
+EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
+
+/**
+ * qcom_mdt_parse() - extract useful parameters from the mdt header
+ * @fw:firmware handle
+ * @fw_addr:   optional reference for base of the firmware's memory region
+ * @fw_size:   optional reference for size of the firmware's memory region
+ * @fw_relocate: optional reference for flagging if the firmware is relocatable
+ *
+ * Returns 0 on success, negative errno otherwise.
+ */
+int qcom_mdt_parse(const struct firmware *fw, phys_addr_t *fw_addr,
+  size_t *fw_size, bool *fw_relocate)
+{
+   const struct elf32_phdr *phdrs;
+   const struct elf32_phdr *phdr;
+   const struct elf32_hdr *ehdr;
+   phys_addr_t min_addr = (phys_addr_t)ULLONG_MAX;
+   

Re: [PATCH v3 1/2] remoteproc: qcom: Driver for the self-authenticating Hexagon v5

[Srinivas Kandagatla]

On 20/06/16 22:28, Bjorn Andersson wrote:

From: Bjorn Andersson 

This initial hack powers the q6v5, boots and authenticate the mba and
use that to load the mdt and subsequent bXX files.

Signed-off-by: Bjorn Andersson 
Signed-off-by: Bjorn Andersson 


Patch looks good to me,

Acked-by: Srinivas Kandagatla 



---

Changes since v2:
- Balancing the rom_clk calls
- Moved reset wait to the reset function
- Various cleanups per Srinivas review
- MODULE_DESCRIPTION and MODULE_LICENSE

Changes since v1:
- Corrected boot address in relocation case
- Using rproc_da_to_va() to clean up mdt loader api
- Dynamically allocating scratch space for mdt verification

  drivers/remoteproc/Kconfig   |  12 +
  drivers/remoteproc/Makefile  |   2 +
  drivers/remoteproc/qcom_mdt_loader.c | 179 +++
  drivers/remoteproc/qcom_mdt_loader.h |  13 +
  drivers/remoteproc/qcom_q6v5_pil.c   | 916 +++
  5 files changed, 1122 insertions(+)
  create mode 100644 drivers/remoteproc/qcom_mdt_loader.c
  create mode 100644 drivers/remoteproc/qcom_mdt_loader.h
  create mode 100644 drivers/remoteproc/qcom_q6v5_pil.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 72e97d7a5209..9ec66d99b978 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -77,6 +77,18 @@ config DA8XX_REMOTEPROC
  It's safe to say n here if you're not interested in multimedia
  offloading.

+config QCOM_MDT_LOADER
+   tristate
+
+config QCOM_Q6V5_PIL
+   tristate "Qualcomm Hexagon V5 Peripherial Image Loader"
+   depends on OF && ARCH_QCOM
+   select REMOTEPROC
+   select QCOM_MDT_LOADER
+   help
+ Say y here to support the Qualcomm Peripherial Image Loader for the
+ Hexagon V5 based remote processors.
+
  config ST_REMOTEPROC
tristate "ST remoteproc support"
depends on ARCH_STI
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 279cb2edc880..92d3758bd15c 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,4 +11,6 @@ obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
  obj-$(CONFIG_STE_MODEM_RPROC) += ste_modem_rproc.o
  obj-$(CONFIG_WKUP_M3_RPROC)   += wkup_m3_rproc.o
  obj-$(CONFIG_DA8XX_REMOTEPROC)+= da8xx_remoteproc.o
+obj-$(CONFIG_QCOM_MDT_LOADER)  += qcom_mdt_loader.o
+obj-$(CONFIG_QCOM_Q6V5_PIL)+= qcom_q6v5_pil.o
  obj-$(CONFIG_ST_REMOTEPROC)   += st_remoteproc.o
diff --git a/drivers/remoteproc/qcom_mdt_loader.c 
b/drivers/remoteproc/qcom_mdt_loader.c
new file mode 100644
index ..114e8e4cef67
--- /dev/null
+++ b/drivers/remoteproc/qcom_mdt_loader.c
@@ -0,0 +1,179 @@
+/*
+ * Qualcomm Peripheral Image Loader
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Copyright (C) 2015 Sony Mobile Communications Inc
+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+#include "qcom_mdt_loader.h"
+
+/**
+ * qcom_mdt_find_rsc_table() - provide dummy resource table for remoteproc
+ * @rproc: remoteproc handle
+ * @fw:firmware header
+ * @tablesz:   outgoing size of the table
+ *
+ * Returns a dummy table.
+ */
+struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
+  const struct firmware *fw,
+  int *tablesz)
+{
+   static struct resource_table table = { .ver = 1, };
+
+   *tablesz = sizeof(table);
+   return 
+}
+EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
+
+/**
+ * qcom_mdt_parse() - extract useful parameters from the mdt header
+ * @fw:firmware handle
+ * @fw_addr:   optional reference for base of the firmware's memory region
+ * @fw_size:   optional reference for size of the firmware's memory region
+ * @fw_relocate: optional reference for flagging if the firmware is relocatable
+ *
+ * Returns 0 on success, negative errno otherwise.
+ */
+int qcom_mdt_parse(const struct firmware *fw, phys_addr_t *fw_addr,
+  size_t *fw_size, bool *fw_relocate)
+{
+   const struct elf32_phdr *phdrs;
+   const struct elf32_phdr *phdr;
+   const struct elf32_hdr *ehdr;
+   phys_addr_t min_addr = (phys_addr_t)ULLONG_MAX;
+   phys_addr_t max_addr = 0;
+   bool relocate = false;
+   int i;
+
+   ehdr = (struct elf32_hdr *)fw->data;
+

Re: [PATCH] [perf]: update makefile message for installing required libs

[Arnaldo Carvalho de Melo]

Em Mon, Jun 27, 2016 at 06:59:57AM -0700, neerajbadl...@gmail.com escreveu:
> From: Neeraj Badlani 
> 
> In case of missing library (libslang), give hint to install library 
> (libslang2-dev)
> Since libslang-dev is not provided by Ubuntu's apt-package
> 
> Signed-off-by: Neeraj Badlani 
> ---
>  tools/perf/config/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index 5ad0255..e57d4d7 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -437,7 +437,7 @@ endif
>  
>  ifndef NO_SLANG
>ifneq ($(feature-libslang), 1)
> -msg := $(warning slang not found, disables TUI support. Please install 
> slang-devel or libslang-dev);
> +msg := $(warning slang not found, disables TUI support. Please install 
> slang-devel or libslang-dev or libslang2-dev);

Too many 'or', I'll apply it as:

+msg := $(warning slang not found, disables TUI support. Please install 
slang-devel, libslang-dev or libslang2-dev);

Thanks,

- Arnaldo

>  NO_SLANG := 1
>else
>  # Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
> -- 
> 2.7.4

Re: [PATCH] [perf]: update makefile message for installing required libs

[Arnaldo Carvalho de Melo]

Em Mon, Jun 27, 2016 at 06:59:57AM -0700, neerajbadl...@gmail.com escreveu:
> From: Neeraj Badlani 
> 
> In case of missing library (libslang), give hint to install library 
> (libslang2-dev)
> Since libslang-dev is not provided by Ubuntu's apt-package
> 
> Signed-off-by: Neeraj Badlani 
> ---
>  tools/perf/config/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index 5ad0255..e57d4d7 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -437,7 +437,7 @@ endif
>  
>  ifndef NO_SLANG
>ifneq ($(feature-libslang), 1)
> -msg := $(warning slang not found, disables TUI support. Please install 
> slang-devel or libslang-dev);
> +msg := $(warning slang not found, disables TUI support. Please install 
> slang-devel or libslang-dev or libslang2-dev);

Too many 'or', I'll apply it as:

+msg := $(warning slang not found, disables TUI support. Please install 
slang-devel, libslang-dev or libslang2-dev);

Thanks,

- Arnaldo

>  NO_SLANG := 1
>else
>  # Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
> -- 
> 2.7.4

Re: [PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Rob Herring]

On Mon, Jun 27, 2016 at 6:21 AM, Andrea Merello
 wrote:
> On Fri, Jun 10, 2016 at 7:36 PM, Rob Herring  wrote:
>> On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
>>> Signed-off-by: Andrea Merello 
>>> Cc: Stefan Kristiansson 
>>> Cc: Tomi Valkeinen 
>>> Cc: Francesco Diotalevi 
>>> Cc: Claudio Lorini 
>>> ---
>>>  .../bindings/display/opencores,ocdrm.txt   | 27 
>>> ++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt 
>>> b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> new file mode 100644
>>> index 000..8d36de5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> @@ -0,0 +1,27 @@
>>> +OpenCores VGA/LCD controller
>>> +
>>> +Required properties:
>>> +- compatible: "opencores,ocdrm"
>>
>> Name this based on what the h/w block is called, not a Linux subsystem.
>
> Ok.
>
> Something like "opencores,video" ?

The VGA/LCD controller IP block is just called "video"?

> The older framebuffer driver has "opencores,ocfb"; I assumed the
> suffix -fb was because of it is a "framebuffer" _driver_, but maybe it
> is because they consider it a "framebuffer" _device_.

Could be either. The FB and DRM "bindings" have been flawed in
general. The bindings should describe hardware blocks and the
connections between them, not what the kernel subsystem wants.

> Maybe I can keep the same "compatible" string? Is it ok to have two
> drivers (maybe temporarily, until the older one fades out) with the
> same "compatible" ?

In principal, that is exactly how it should work. However, the FB
binding is probably incomplete or flawed. If keeping the compatible
string is enough for it to work, then that is fine.

>> Is there any sort of versioning for OpenCore IP that you can include in
>> the name?
>
> Not sure about this. I'll check.
>
>>> +- reg: Physical base address and length of the controller's registers.
>>> +- clocks: Must contain an entry for the pixelclock generator.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +
>>> +Required sub-nodes:
>>> +- port: the connection to a DRM bridge.  The connection is modelled
>>
>> Don't include Linux driver details (DRM) in bindings.
>
> You mean telling that the port must be a connection to a DRM bridge,
> leaving out further details?

The binding itself and description should not contain the word DRM.
Just say connection to bridge chip or connector.

Rob

Re: [PATCH 2/2] drm: Add DT bindings documentation for OpenCores VGA/LCD controller

[Rob Herring]

On Mon, Jun 27, 2016 at 6:21 AM, Andrea Merello
 wrote:
> On Fri, Jun 10, 2016 at 7:36 PM, Rob Herring  wrote:
>> On Thu, Jun 09, 2016 at 03:33:19PM +0200, Andrea Merello wrote:
>>> Signed-off-by: Andrea Merello 
>>> Cc: Stefan Kristiansson 
>>> Cc: Tomi Valkeinen 
>>> Cc: Francesco Diotalevi 
>>> Cc: Claudio Lorini 
>>> ---
>>>  .../bindings/display/opencores,ocdrm.txt   | 27 
>>> ++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/opencores,ocdrm.txt 
>>> b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> new file mode 100644
>>> index 000..8d36de5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/opencores,ocdrm.txt
>>> @@ -0,0 +1,27 @@
>>> +OpenCores VGA/LCD controller
>>> +
>>> +Required properties:
>>> +- compatible: "opencores,ocdrm"
>>
>> Name this based on what the h/w block is called, not a Linux subsystem.
>
> Ok.
>
> Something like "opencores,video" ?

The VGA/LCD controller IP block is just called "video"?

> The older framebuffer driver has "opencores,ocfb"; I assumed the
> suffix -fb was because of it is a "framebuffer" _driver_, but maybe it
> is because they consider it a "framebuffer" _device_.

Could be either. The FB and DRM "bindings" have been flawed in
general. The bindings should describe hardware blocks and the
connections between them, not what the kernel subsystem wants.

> Maybe I can keep the same "compatible" string? Is it ok to have two
> drivers (maybe temporarily, until the older one fades out) with the
> same "compatible" ?

In principal, that is exactly how it should work. However, the FB
binding is probably incomplete or flawed. If keeping the compatible
string is enough for it to work, then that is fine.

>> Is there any sort of versioning for OpenCore IP that you can include in
>> the name?
>
> Not sure about this. I'll check.
>
>>> +- reg: Physical base address and length of the controller's registers.
>>> +- clocks: Must contain an entry for the pixelclock generator.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +
>>> +Required sub-nodes:
>>> +- port: the connection to a DRM bridge.  The connection is modelled
>>
>> Don't include Linux driver details (DRM) in bindings.
>
> You mean telling that the port must be a connection to a DRM bridge,
> leaving out further details?

The binding itself and description should not contain the word DRM.
Just say connection to bridge chip or connector.

Rob

Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver

[Andrew F. Davis]

On 06/23/2016 11:28 AM, Philipp Zabel wrote:
> Am Donnerstag, den 23.06.2016, 09:28 -0500 schrieb Andrew F. Davis:
>> On 06/23/2016 04:05 AM, Philipp Zabel wrote:
>>> Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis:
>>> [...]
>> +depends on HAS_IOMEM
>> +select MFD_SYSCON
>> +help
>> +  This enables the reset driver support for TI devices with
>> +  memory-mapped reset registers as part of a syscon device 
>> node. If
>> +  you wish to use the reset framework for such memory-mapped 
>> devices,
>> +  say Y here. Otherwise, say N.
>
> Actually, do you need the user configurable option at all?

 I'm not sure, right now it is selected by other things, but that is true
 for much of Kconfig, it is not platform dependent so it doesn't need to
 only be enabled by arch, it probably isn't hurting anything to leave it?
>>>
>>> No, that's okay. So the intention is to make it possible to enable it
>>> for COMPILE_TESTs on architectures other than TI?
>>
>> I was thinking more that it should be usable on non-TI architectures and
>> so can be user enabled if needed.
> 
> Those architectures could also just select it, then. Having the option
> might improve discoverability though.
> 
> [...]
>>> So far, I have seen the following variants. Depending on the hardware, a
>>> reset could be:
>>> - asserted by setting a bit
>>> - asserted by clearing a bit
>>> - deasserted by clearing/setting the same bit
>>> - deasserted by setting/clearing the same bit in another register
>>> - triggered to be asserted/deasserted automatically with some specific
>>>   timing that the hardware knows about (in that case the manual
>>>   assert/deassert is not available)
>>> The status of the reset line could be read via
>>> - the same bit that is used to assert/deassert
>>> - the same bit in another register
>>> - not at all
>>>
>>> What I've not yet seen but surely exists somewhere is the case where
>>> assert/deassert/status bits are at different bit positions either in the
>>> same register or in different registers.
>>
>> Exactly why I was thinking having a node for the resets would be more
>> future proof, we could add more properties later:
>>
>> pscrst-dsp: dsp {
>>  reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>;
>>  reset-status = <0x83c 8 RESET_ASSERT_CLEAR>;
>> +reset-deassert = <0x840 3 RESET_ASSERT_SET>;
>> +reset-toggle-time-ms = <20>;
>> };
>>
>> While a fixed length array does make for a more condensed binding we
>> don't get much flexibility.
>>
>> What we could also do is have a longer array then use macros to trim it
>> down for simple cases:
>>
>> reset-bits = <
>>  SINGLE_BIT_RESET(0xa3c, 8)
>>  SINGLE_BIT_RESET(0xa40, 7)
>>  SINGLE_BIT_RESET(0xa44, 6)
>>> ;
> 
> I think there has been opposition in the past against hiding things more
> complex than a single value behind macros.
> 
>> Each real entry could have 9 fields that the macro would expand into:
>>  (assert reg) (assert bit) (assert flag)
>>  (deassert reg) (deassert bit) (deassert flag)
>>  (status reg) (status bit) (status flag)
>>
>> This would be able to handle all of the above cases except the timing
>> one, that case can always be handled by a dedicated driver for that system.
> 
> How about seven:
>   (assert reg) (assert bit)
>   (deassert reg) (deassert bit)
>   (status reg) (status bit)
> (flags)
> 
> reset-bits = <
>   0xa3c 8 0xa3c 8 0x83c 8 (ASSERT_CLEAR|DEASSERT_SET|STATUS_SET)
>> ;
> 

I like this, we could also add a none flag:

STATUS_SET
STATUS_CLEAR
STATUS_NONE

to note that this reset doesn't support this and that the numbers given
are not valid.

>> My goal here, I would like to believe, aligns with the goals of DT in
>> general, I would like to see one kernel work on many platforms without
>> having to compile-in a bunch of SoC specific info. Some SoCs will need
>> their own reset driver for when they do something unique (like this
>> reset driver I will post next cycle which sends a message to a power
>> management chip for its resets:
>> http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y
>> )
>> but I see no reason a bunch of different drivers for simple register bit
>> resets with the only difference being a #define offset. I hope that this
>> effort will get ahead of these drivers and reduce the maintenance burden
>> for you.
> 
> The problem of reducing the amount of simple drivers is completely
> orthogonal to agreeing on a fitting DT binding. A common driver could
> just as well have static syscon_reset_control arrays per compatible
> compiled in.
> 
>> So how much is handled by this driver is up to you, (hopefully without
>> trying to handle every possible case :)).
> 
> It's also up to the DT maintainers to approve 

Re: [PATCH v4 2/2] reset: add TI SYSCON based reset driver

[Andrew F. Davis]

On 06/23/2016 11:28 AM, Philipp Zabel wrote:
> Am Donnerstag, den 23.06.2016, 09:28 -0500 schrieb Andrew F. Davis:
>> On 06/23/2016 04:05 AM, Philipp Zabel wrote:
>>> Am Mittwoch, den 22.06.2016, 14:46 -0500 schrieb Andrew F. Davis:
>>> [...]
>> +depends on HAS_IOMEM
>> +select MFD_SYSCON
>> +help
>> +  This enables the reset driver support for TI devices with
>> +  memory-mapped reset registers as part of a syscon device 
>> node. If
>> +  you wish to use the reset framework for such memory-mapped 
>> devices,
>> +  say Y here. Otherwise, say N.
>
> Actually, do you need the user configurable option at all?

 I'm not sure, right now it is selected by other things, but that is true
 for much of Kconfig, it is not platform dependent so it doesn't need to
 only be enabled by arch, it probably isn't hurting anything to leave it?
>>>
>>> No, that's okay. So the intention is to make it possible to enable it
>>> for COMPILE_TESTs on architectures other than TI?
>>
>> I was thinking more that it should be usable on non-TI architectures and
>> so can be user enabled if needed.
> 
> Those architectures could also just select it, then. Having the option
> might improve discoverability though.
> 
> [...]
>>> So far, I have seen the following variants. Depending on the hardware, a
>>> reset could be:
>>> - asserted by setting a bit
>>> - asserted by clearing a bit
>>> - deasserted by clearing/setting the same bit
>>> - deasserted by setting/clearing the same bit in another register
>>> - triggered to be asserted/deasserted automatically with some specific
>>>   timing that the hardware knows about (in that case the manual
>>>   assert/deassert is not available)
>>> The status of the reset line could be read via
>>> - the same bit that is used to assert/deassert
>>> - the same bit in another register
>>> - not at all
>>>
>>> What I've not yet seen but surely exists somewhere is the case where
>>> assert/deassert/status bits are at different bit positions either in the
>>> same register or in different registers.
>>
>> Exactly why I was thinking having a node for the resets would be more
>> future proof, we could add more properties later:
>>
>> pscrst-dsp: dsp {
>>  reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>;
>>  reset-status = <0x83c 8 RESET_ASSERT_CLEAR>;
>> +reset-deassert = <0x840 3 RESET_ASSERT_SET>;
>> +reset-toggle-time-ms = <20>;
>> };
>>
>> While a fixed length array does make for a more condensed binding we
>> don't get much flexibility.
>>
>> What we could also do is have a longer array then use macros to trim it
>> down for simple cases:
>>
>> reset-bits = <
>>  SINGLE_BIT_RESET(0xa3c, 8)
>>  SINGLE_BIT_RESET(0xa40, 7)
>>  SINGLE_BIT_RESET(0xa44, 6)
>>> ;
> 
> I think there has been opposition in the past against hiding things more
> complex than a single value behind macros.
> 
>> Each real entry could have 9 fields that the macro would expand into:
>>  (assert reg) (assert bit) (assert flag)
>>  (deassert reg) (deassert bit) (deassert flag)
>>  (status reg) (status bit) (status flag)
>>
>> This would be able to handle all of the above cases except the timing
>> one, that case can always be handled by a dedicated driver for that system.
> 
> How about seven:
>   (assert reg) (assert bit)
>   (deassert reg) (deassert bit)
>   (status reg) (status bit)
> (flags)
> 
> reset-bits = <
>   0xa3c 8 0xa3c 8 0x83c 8 (ASSERT_CLEAR|DEASSERT_SET|STATUS_SET)
>> ;
> 

I like this, we could also add a none flag:

STATUS_SET
STATUS_CLEAR
STATUS_NONE

to note that this reset doesn't support this and that the numbers given
are not valid.

>> My goal here, I would like to believe, aligns with the goals of DT in
>> general, I would like to see one kernel work on many platforms without
>> having to compile-in a bunch of SoC specific info. Some SoCs will need
>> their own reset driver for when they do something unique (like this
>> reset driver I will post next cycle which sends a message to a power
>> management chip for its resets:
>> http://git.ti.com/gitweb/?p=ti-linux-kernel/ti-linux-kernel.git;a=blob;f=drivers/reset/reset-ti-sci.c;h=3636dc176c8b5c4449eefc1fe13df7b912cec73e;hb=refs/heads/ti-lsk-linux-4.4.y
>> )
>> but I see no reason a bunch of different drivers for simple register bit
>> resets with the only difference being a #define offset. I hope that this
>> effort will get ahead of these drivers and reduce the maintenance burden
>> for you.
> 
> The problem of reducing the amount of simple drivers is completely
> orthogonal to agreeing on a fitting DT binding. A common driver could
> just as well have static syscon_reset_control arrays per compatible
> compiled in.
> 
>> So how much is handled by this driver is up to you, (hopefully without
>> trying to handle every possible case :)).
> 
> It's also up to the DT maintainers to approve 

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
>> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>>> This allows x86_64 kernels to enable vmapped stacks.  There are a
>>> couple of interesting bits.
>>>
>>> First, x86 lazily faults in top-level paging entries for the vmalloc
>>> area.  This won't work if we get a page fault while trying to access
>>> the stack: the CPU will promote it to a double-fault and we'll die.
>>> To avoid this problem, probe the new stack when switching stacks and
>>> forcibly populate the pgd entry for the stack when switching mms.
>>>
>>> Second, once we have guard pages around the stack, we'll want to
>>> detect and handle stack overflow.
>>>
>>> I didn't enable it on x86_32.  We'd need to rework the double-fault
>>> code a bit and I'm concerned about running out of vmalloc virtual
>>> addresses under some workloads.
>>>
>>> This patch, by itself, will behave somewhat erratically when the
>>> stack overflows while RSP is still more than a few tens of bytes
>>> above the bottom of the stack.  Specifically, we'll get #PF and make
>>> it to no_context and an oops without triggering a double-fault, and
>>> no_context doesn't know about stack overflows.  The next patch will
>>> improve that case.
>>>
>>> Signed-off-by: Andy Lutomirski 
>>> ---
>>>  arch/x86/Kconfig |  1 +
>>>  arch/x86/include/asm/switch_to.h | 28 +++-
>>>  arch/x86/kernel/traps.c  | 32 
>>>  arch/x86/mm/tlb.c| 15 +++
>>>  4 files changed, 75 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index d9a94da0c29f..afdcf96ef109 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -92,6 +92,7 @@ config X86
>>> select HAVE_ARCH_TRACEHOOK
>>> select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>>> select HAVE_EBPF_JITif X86_64
>>> +   select HAVE_ARCH_VMAP_STACK if X86_64
>>> select HAVE_CC_STACKPROTECTOR
>>> select HAVE_CMPXCHG_DOUBLE
>>> select HAVE_CMPXCHG_LOCAL
>>> diff --git a/arch/x86/include/asm/switch_to.h 
>>> b/arch/x86/include/asm/switch_to.h
>>> index 8f321a1b03a1..14e4b20f0aaf 100644
>>> --- a/arch/x86/include/asm/switch_to.h
>>> +++ b/arch/x86/include/asm/switch_to.h
>>> @@ -8,6 +8,28 @@ struct tss_struct;
>>>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct 
>>> *next_p,
>>>   struct tss_struct *tss);
>>>
>>> +/* This runs runs on the previous thread's stack. */
>>> +static inline void prepare_switch_to(struct task_struct *prev,
>>> +struct task_struct *next)
>>> +{
>>> +#ifdef CONFIG_VMAP_STACK
>>> +   /*
>>> +* If we switch to a stack that has a top-level paging entry
>>> +* that is not present in the current mm, the resulting #PF will
>>> +* will be promoted to a double-fault and we'll panic.  Probe
>>> +* the new stack now so that vmalloc_fault can fix up the page
>>> +* tables if needed.  This can only happen if we use a stack
>>> +* in vmap space.
>>> +*
>>> +* We assume that the stack is aligned so that it never spans
>>> +* more than one top-level paging entry.
>>> +*
>>> +* To minimize cache pollution, just follow the stack pointer.
>>> +*/
>>> +   READ_ONCE(*(unsigned char *)next->thread.sp);
>>> +#endif
>>> +}
>>> +
>>>  #ifdef CONFIG_X86_32
>>>
>>>  #ifdef CONFIG_CC_STACKPROTECTOR
>>> @@ -39,6 +61,8 @@ do {  
>>> \
>>>  */ \
>>> unsigned long ebx, ecx, edx, esi, edi;  \
>>> \
>>> +   prepare_switch_to(prev, next);  \
>>> +   \
>>> asm volatile("pushl %%ebp\n\t"  /* saveEBP   */ \
>>>  "movl %%esp,%[prev_sp]\n\t"/* saveESP   */ 
>>> \
>>>  "movl %[next_sp],%%esp\n\t"/* restore ESP   */ 
>>> \
>>> @@ -103,7 +127,9 @@ do {
>>> \
>>>   * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
>>>   * has no effect.
>>>   */
>>> -#define switch_to(prev, next, last) \
>>> +#define switch_to(prev, next, last)  \
>>> +   prepare_switch_to(prev, next);\
>>> + \
>>> asm volatile(SAVE_CONTEXT  

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 8:12 AM, Brian Gerst  wrote:
> On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
>> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>>> This allows x86_64 kernels to enable vmapped stacks.  There are a
>>> couple of interesting bits.
>>>
>>> First, x86 lazily faults in top-level paging entries for the vmalloc
>>> area.  This won't work if we get a page fault while trying to access
>>> the stack: the CPU will promote it to a double-fault and we'll die.
>>> To avoid this problem, probe the new stack when switching stacks and
>>> forcibly populate the pgd entry for the stack when switching mms.
>>>
>>> Second, once we have guard pages around the stack, we'll want to
>>> detect and handle stack overflow.
>>>
>>> I didn't enable it on x86_32.  We'd need to rework the double-fault
>>> code a bit and I'm concerned about running out of vmalloc virtual
>>> addresses under some workloads.
>>>
>>> This patch, by itself, will behave somewhat erratically when the
>>> stack overflows while RSP is still more than a few tens of bytes
>>> above the bottom of the stack.  Specifically, we'll get #PF and make
>>> it to no_context and an oops without triggering a double-fault, and
>>> no_context doesn't know about stack overflows.  The next patch will
>>> improve that case.
>>>
>>> Signed-off-by: Andy Lutomirski 
>>> ---
>>>  arch/x86/Kconfig |  1 +
>>>  arch/x86/include/asm/switch_to.h | 28 +++-
>>>  arch/x86/kernel/traps.c  | 32 
>>>  arch/x86/mm/tlb.c| 15 +++
>>>  4 files changed, 75 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index d9a94da0c29f..afdcf96ef109 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -92,6 +92,7 @@ config X86
>>> select HAVE_ARCH_TRACEHOOK
>>> select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>>> select HAVE_EBPF_JITif X86_64
>>> +   select HAVE_ARCH_VMAP_STACK if X86_64
>>> select HAVE_CC_STACKPROTECTOR
>>> select HAVE_CMPXCHG_DOUBLE
>>> select HAVE_CMPXCHG_LOCAL
>>> diff --git a/arch/x86/include/asm/switch_to.h 
>>> b/arch/x86/include/asm/switch_to.h
>>> index 8f321a1b03a1..14e4b20f0aaf 100644
>>> --- a/arch/x86/include/asm/switch_to.h
>>> +++ b/arch/x86/include/asm/switch_to.h
>>> @@ -8,6 +8,28 @@ struct tss_struct;
>>>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct 
>>> *next_p,
>>>   struct tss_struct *tss);
>>>
>>> +/* This runs runs on the previous thread's stack. */
>>> +static inline void prepare_switch_to(struct task_struct *prev,
>>> +struct task_struct *next)
>>> +{
>>> +#ifdef CONFIG_VMAP_STACK
>>> +   /*
>>> +* If we switch to a stack that has a top-level paging entry
>>> +* that is not present in the current mm, the resulting #PF will
>>> +* will be promoted to a double-fault and we'll panic.  Probe
>>> +* the new stack now so that vmalloc_fault can fix up the page
>>> +* tables if needed.  This can only happen if we use a stack
>>> +* in vmap space.
>>> +*
>>> +* We assume that the stack is aligned so that it never spans
>>> +* more than one top-level paging entry.
>>> +*
>>> +* To minimize cache pollution, just follow the stack pointer.
>>> +*/
>>> +   READ_ONCE(*(unsigned char *)next->thread.sp);
>>> +#endif
>>> +}
>>> +
>>>  #ifdef CONFIG_X86_32
>>>
>>>  #ifdef CONFIG_CC_STACKPROTECTOR
>>> @@ -39,6 +61,8 @@ do {  
>>> \
>>>  */ \
>>> unsigned long ebx, ecx, edx, esi, edi;  \
>>> \
>>> +   prepare_switch_to(prev, next);  \
>>> +   \
>>> asm volatile("pushl %%ebp\n\t"  /* saveEBP   */ \
>>>  "movl %%esp,%[prev_sp]\n\t"/* saveESP   */ 
>>> \
>>>  "movl %[next_sp],%%esp\n\t"/* restore ESP   */ 
>>> \
>>> @@ -103,7 +127,9 @@ do {
>>> \
>>>   * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
>>>   * has no effect.
>>>   */
>>> -#define switch_to(prev, next, last) \
>>> +#define switch_to(prev, next, last)  \
>>> +   prepare_switch_to(prev, next);\
>>> + \
>>> asm volatile(SAVE_CONTEXT \
>>>  "movq 

Re: [PATCH v2 1/6] Documentation: hid: Intel ISH HID document

[Srinivas Pandruvada]

On Sun, 2016-06-26 at 19:32 +0100, Jonathan Cameron wrote:
> On 22/06/16 06:40, Srinivas Pandruvada wrote:
> > 
> > Document explaining ISH HID operation and implementation.
> > 
> > Signed-off-by: Srinivas Pandruvada  > .com>
> A few really trivial point inline.  I unfortunately don't have the
> time to
> dive into this in sufficient depth to grasp every detail, but the
> description seems pretty comprehensive to me.
> 
> I would however, put a blank line between paragraphs to make it a
> touch
> easier to read...
Thanks. I will take care of your comments in the next revision.

> 
> Might even be worth taking this into a docbook file instead of
> straight
> text...
If we convert to docbook format, will it still go to Documentation/hid
folder, or somewhere else?

Thanks,
Srinivas
> 
> Jonathan
> > 
> > ---
> >  Documentation/hid/intel-ish-hid.txt | 449
> > 
> >  1 file changed, 449 insertions(+)
> >  create mode 100644 Documentation/hid/intel-ish-hid.txt
> > 
> > diff --git a/Documentation/hid/intel-ish-hid.txt
> > b/Documentation/hid/intel-ish-hid.txt
> > new file mode 100644
> > index 000..8557280
> > --- /dev/null
> > +++ b/Documentation/hid/intel-ish-hid.txt
> > @@ -0,0 +1,449 @@
> > +Intel Integrated Sensor Hub (ISH)
> > +===
> > +
> > +A sensor hub enables the ability to offload sensor polling and
> > algorithm
> > +processing to a dedicated low power co-processor. This allows the
> > core
> > +processor to go into low power modes more often, resulting in the
> > increased
> > +battery life.
> > +There are many vendors providing external sensor hubs confirming
> > to HID
> > +Sensor usage tables, and used in several tablets, 2 in 1
> > convertible laptops
> > +and embedded products. Linux had this support since Linux 3.9.
> > +
> > +Intel® introduced integrated sensor hubs as a part of the SoC
> > starting from
> > +Cherry Trail and now supported on multiple generations of CPU
> > packages. There
> > +are many commercial devices already shipped with Integrated Sensor
> > Hubs (ISH).
> > +These ISH also comply to HID sensor specification, but
> > the  difference is the
> > +transport protocol used for communication. The current external
> > sensor hubs
> > +mainly use HID over i2C or USB. But ISH doesn't use either i2c or
> > USB.
> > +
> > +Overview
> > +Using a analogy with a usbhid implementation, the ISH follows a
> > similar model
> > +for a very high speed communication:
> > +
> > +   -   --
> > +   |USB HID|   --> |ISH HID
> >  |
> > +   -   --
> > +   -   --
> > +   |  USB protocol |   --> |ISH
> > Transport   |
> > +   -   --
> > +   -   --
> > +   |  EHCI/XHCI|   --> |ISH IPC
> >  |
> > +   -   --
> > +     PCI    PCI
> > +   -   --
> > +|Host controller|  --> |ISH processor   |
> > +   -   --
> > +    USB Link
> > +   -   --
> > +   | USB End points|   --> |ISH Clients |
> > +   -   --
> > +
> > +Like USB protocol provides a method for device enumeration, link
> > management
> > +and user data encapsulation, the ISH also provides similar
> > services. But it is
> > +very light weight tailored to manage and communicate with ISH
> > client
> > +applications implemented in the firmware.
> > +The ISH allows multiple sensor management applications executing
> > in the
> > +firmware. Like USB endpoints the messaging can be to/from a
> > client. As part of
> > +enumeration process, these clients are identified. These clients
> > can be simple
> > +HID sensor applications, sensor calibration application or senor
> > firmware
> > +update application.
> > +The implementation model is similar, like usb bus, ISH transport
> > is also
> > +implemented as a bus. Each client application executing in the ISH
> > processor
> > +is registered as a device on this bus. The driver, which binds
> > each device
> > +(ISH HID driver) identifies the device type and registers with the
> > hid core.
> > +
> > +ISH Implementation: Block Diagram
> > +
> > +    ---
> > +   |  User Space Applications  |
> > +    ---
> > +
> > +IIO ABI
> > +    --
> > +   |  IIO Sensor Drivers     |
> > +    --
> > +    --
> > +   |    IIO core  

Re: [PATCH v2 1/6] Documentation: hid: Intel ISH HID document

[Srinivas Pandruvada]

On Sun, 2016-06-26 at 19:32 +0100, Jonathan Cameron wrote:
> On 22/06/16 06:40, Srinivas Pandruvada wrote:
> > 
> > Document explaining ISH HID operation and implementation.
> > 
> > Signed-off-by: Srinivas Pandruvada  > .com>
> A few really trivial point inline.  I unfortunately don't have the
> time to
> dive into this in sufficient depth to grasp every detail, but the
> description seems pretty comprehensive to me.
> 
> I would however, put a blank line between paragraphs to make it a
> touch
> easier to read...
Thanks. I will take care of your comments in the next revision.

> 
> Might even be worth taking this into a docbook file instead of
> straight
> text...
If we convert to docbook format, will it still go to Documentation/hid
folder, or somewhere else?

Thanks,
Srinivas
> 
> Jonathan
> > 
> > ---
> >  Documentation/hid/intel-ish-hid.txt | 449
> > 
> >  1 file changed, 449 insertions(+)
> >  create mode 100644 Documentation/hid/intel-ish-hid.txt
> > 
> > diff --git a/Documentation/hid/intel-ish-hid.txt
> > b/Documentation/hid/intel-ish-hid.txt
> > new file mode 100644
> > index 000..8557280
> > --- /dev/null
> > +++ b/Documentation/hid/intel-ish-hid.txt
> > @@ -0,0 +1,449 @@
> > +Intel Integrated Sensor Hub (ISH)
> > +===
> > +
> > +A sensor hub enables the ability to offload sensor polling and
> > algorithm
> > +processing to a dedicated low power co-processor. This allows the
> > core
> > +processor to go into low power modes more often, resulting in the
> > increased
> > +battery life.
> > +There are many vendors providing external sensor hubs confirming
> > to HID
> > +Sensor usage tables, and used in several tablets, 2 in 1
> > convertible laptops
> > +and embedded products. Linux had this support since Linux 3.9.
> > +
> > +Intel® introduced integrated sensor hubs as a part of the SoC
> > starting from
> > +Cherry Trail and now supported on multiple generations of CPU
> > packages. There
> > +are many commercial devices already shipped with Integrated Sensor
> > Hubs (ISH).
> > +These ISH also comply to HID sensor specification, but
> > the  difference is the
> > +transport protocol used for communication. The current external
> > sensor hubs
> > +mainly use HID over i2C or USB. But ISH doesn't use either i2c or
> > USB.
> > +
> > +Overview
> > +Using a analogy with a usbhid implementation, the ISH follows a
> > similar model
> > +for a very high speed communication:
> > +
> > +   -   --
> > +   |USB HID|   --> |ISH HID
> >  |
> > +   -   --
> > +   -   --
> > +   |  USB protocol |   --> |ISH
> > Transport   |
> > +   -   --
> > +   -   --
> > +   |  EHCI/XHCI|   --> |ISH IPC
> >  |
> > +   -   --
> > +     PCI    PCI
> > +   -   --
> > +|Host controller|  --> |ISH processor   |
> > +   -   --
> > +    USB Link
> > +   -   --
> > +   | USB End points|   --> |ISH Clients |
> > +   -   --
> > +
> > +Like USB protocol provides a method for device enumeration, link
> > management
> > +and user data encapsulation, the ISH also provides similar
> > services. But it is
> > +very light weight tailored to manage and communicate with ISH
> > client
> > +applications implemented in the firmware.
> > +The ISH allows multiple sensor management applications executing
> > in the
> > +firmware. Like USB endpoints the messaging can be to/from a
> > client. As part of
> > +enumeration process, these clients are identified. These clients
> > can be simple
> > +HID sensor applications, sensor calibration application or senor
> > firmware
> > +update application.
> > +The implementation model is similar, like usb bus, ISH transport
> > is also
> > +implemented as a bus. Each client application executing in the ISH
> > processor
> > +is registered as a device on this bus. The driver, which binds
> > each device
> > +(ISH HID driver) identifies the device type and registers with the
> > hid core.
> > +
> > +ISH Implementation: Block Diagram
> > +
> > +    ---
> > +   |  User Space Applications  |
> > +    ---
> > +
> > +IIO ABI
> > +    --
> > +   |  IIO Sensor Drivers     |
> > +    --
> > +    --
> > +   |    IIO core     |
> > +    

Re: [PATCH v2 2/2] namespaces: add transparent user namespaces

[Eric W. Biederman]

Added a few more relevant cc's.

Jann Horn  writes:

> This allows the admin of a user namespace to mark the namespace as
> transparent. All other namespaces, by default, are opaque.


I have just skimmed through this and at a high level this doesn't seem
too scary.  Having an identity mapped user namespace that just limits
you to using a subset of uids and gids while allowing displaying the
full range of uids and gids.

I don't quite get the use case and I would like to a little better
but in the long term this shouldn't cause any significant maintenance
issues, so I don't have any objects.

At the same time this isn't quite the time to merge this.  I am in the
process of slowly going through Seth's vfs changes to support things
such as truly unprivileged fuse support.  Those changes alter which
places can always be assumed to be init_user_ns (many fewer), and also
slightly change the set of from_kuid calls being made.

The changes that have made it through my review currently reside at:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git 
for-next

Those vfs changes make it conceivable and simple from an infrastructure
standpoint to transition fileystems to unprivileged user namespace
mounts, with perhaps as little work as just setting FS_USER_NS.  At the
same time that won't be recommend because of the difficulty verifying
evil filesystem contents can't cause fs implementations to do bad things
is difficult.

That change means your first patch that just zaps all from_kuid_munged
users in init_user_ns isn't a particularly good idea.  I don't think it
is a good idea to have one set of rules for things that will always be
init_user_ns and another set of rules for code that will change.

The long and short of this is I am asking you to wait a week or so and
rebase this on my for-next branch so that we can confirm this change
interacts nicely will all of the other on-going work.

Thank you,
Eric Biederman

Re: [PATCH v2 2/2] namespaces: add transparent user namespaces

[Eric W. Biederman]

Added a few more relevant cc's.

Jann Horn  writes:

> This allows the admin of a user namespace to mark the namespace as
> transparent. All other namespaces, by default, are opaque.


I have just skimmed through this and at a high level this doesn't seem
too scary.  Having an identity mapped user namespace that just limits
you to using a subset of uids and gids while allowing displaying the
full range of uids and gids.

I don't quite get the use case and I would like to a little better
but in the long term this shouldn't cause any significant maintenance
issues, so I don't have any objects.

At the same time this isn't quite the time to merge this.  I am in the
process of slowly going through Seth's vfs changes to support things
such as truly unprivileged fuse support.  Those changes alter which
places can always be assumed to be init_user_ns (many fewer), and also
slightly change the set of from_kuid calls being made.

The changes that have made it through my review currently reside at:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git 
for-next

Those vfs changes make it conceivable and simple from an infrastructure
standpoint to transition fileystems to unprivileged user namespace
mounts, with perhaps as little work as just setting FS_USER_NS.  At the
same time that won't be recommend because of the difficulty verifying
evil filesystem contents can't cause fs implementations to do bad things
is difficult.

That change means your first patch that just zaps all from_kuid_munged
users in init_user_ns isn't a particularly good idea.  I don't think it
is a good idea to have one set of rules for things that will always be
init_user_ns and another set of rules for code that will change.

The long and short of this is I am asking you to wait a week or so and
rebase this on my for-next branch so that we can confirm this change
interacts nicely will all of the other on-going work.

Thank you,
Eric Biederman

Re: [PATCH v4] audit: add fields to exclude filter by reusing user filter

[Paul Moore]

On Fri, Jun 24, 2016 at 4:35 PM, Richard Guy Briggs  wrote:
> RFE: add additional fields for use in audit filter exclude rules
> https://github.com/linux-audit/audit-kernel/issues/5
>
> Re-factor and combine audit_filter_type() with audit_filter_user() to
> use audit_filter_user_rules() to enable the exclude filter to
> additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
>
> The process of combining the similar audit_filter_user() and
> audit_filter_type() functions, required inverting the meaning and
> including the ALWAYS action of the latter.
>
> Include audit_filter_user_rules() into audit_filter(), removing unneeded
> logic in the process.
>
> Keep the check to quit early if the list is empty.
>
> Signed-off-by: Richard Guy Briggs 
> ---
> v4: rebase on 4.6-based audit/next.
>
> v3: pull audit_filter_user_rules() into audit_filter() and simplify
> logic.
>
> v2: combine audit_filter_user() and audit_filter_type() into
> audit_filter().
> ---
>  include/linux/audit.h |2 -
>  kernel/audit.c|4 +-
>  kernel/audit.h|2 +
>  kernel/auditfilter.c  |  151 
> +
>  4 files changed, 57 insertions(+), 102 deletions(-)

Merged, thanks.  Please remember to run scripts/checkpatch.pl on your
submissions, I had to fix up a couple of whitespace damaged lines.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index e38e3fc..9d4443f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -163,8 +163,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
>  extern int audit_update_lsm_rules(void);
>
> /* Private API (for audit.c only) */
> -extern int audit_filter_user(int type);
> -extern int audit_filter_type(int type);
>  extern int audit_rule_change(int type, __u32 portid, int seq,
> void *data, size_t datasz);
>  extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 678c3f0..994588e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -934,7 +934,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
> nlmsghdr *nlh)
> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> return 0;
>
> -   err = audit_filter_user(msg_type);
> +   err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> @@ -1382,7 +1382,7 @@ struct audit_buffer *audit_log_start(struct 
> audit_context *ctx, gfp_t gfp_mask,
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
> -   if (unlikely(audit_filter_type(type)))
> +   if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> return NULL;
>
> if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index cbbe6bb..1879f02 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
>  extern kuid_t audit_sig_uid;
>  extern u32 audit_sig_sid;
>
> +extern int audit_filter(int msgtype, unsigned int listtype);
> +
>  #ifdef CONFIG_AUDITSYSCALL
>  extern int __audit_signal_info(int sig, struct task_struct *t);
>  static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index ff59a5e..3a67acf 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1290,117 +1290,72 @@ int audit_compare_dname_path(const char *dname, 
> const char *path, int parentlen)
> return strncmp(p, dname, dlen);
>  }
>
> -static int audit_filter_user_rules(struct audit_krule *rule, int type,
> -  enum audit_state *state)
> -{
> -   int i;
> -
> -   for (i = 0; i < rule->field_count; i++) {
> -   struct audit_field *f = >fields[i];
> -   pid_t pid;
> -   int result = 0;
> -   u32 sid;
> -
> -   switch (f->type) {
> -   case AUDIT_PID:
> -   pid = task_pid_nr(current);
> -   result = audit_comparator(pid, f->op, f->val);
> -   break;
> -   case AUDIT_UID:
> -   result = audit_uid_comparator(current_uid(), f->op, 
> f->uid);
> -   break;
> -   case AUDIT_GID:
> -   result = audit_gid_comparator(current_gid(), f->op, 
> f->gid);
> -   break;
> -   case AUDIT_LOGINUID:
> -   result = 
> audit_uid_comparator(audit_get_loginuid(current),
> - f->op, f->uid);
> -   break;
> -   case AUDIT_LOGINUID_SET:
> -

Re: [PATCH v4] audit: add fields to exclude filter by reusing user filter

[Paul Moore]

On Fri, Jun 24, 2016 at 4:35 PM, Richard Guy Briggs  wrote:
> RFE: add additional fields for use in audit filter exclude rules
> https://github.com/linux-audit/audit-kernel/issues/5
>
> Re-factor and combine audit_filter_type() with audit_filter_user() to
> use audit_filter_user_rules() to enable the exclude filter to
> additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
>
> The process of combining the similar audit_filter_user() and
> audit_filter_type() functions, required inverting the meaning and
> including the ALWAYS action of the latter.
>
> Include audit_filter_user_rules() into audit_filter(), removing unneeded
> logic in the process.
>
> Keep the check to quit early if the list is empty.
>
> Signed-off-by: Richard Guy Briggs 
> ---
> v4: rebase on 4.6-based audit/next.
>
> v3: pull audit_filter_user_rules() into audit_filter() and simplify
> logic.
>
> v2: combine audit_filter_user() and audit_filter_type() into
> audit_filter().
> ---
>  include/linux/audit.h |2 -
>  kernel/audit.c|4 +-
>  kernel/audit.h|2 +
>  kernel/auditfilter.c  |  151 
> +
>  4 files changed, 57 insertions(+), 102 deletions(-)

Merged, thanks.  Please remember to run scripts/checkpatch.pl on your
submissions, I had to fix up a couple of whitespace damaged lines.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index e38e3fc..9d4443f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -163,8 +163,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
>  extern int audit_update_lsm_rules(void);
>
> /* Private API (for audit.c only) */
> -extern int audit_filter_user(int type);
> -extern int audit_filter_type(int type);
>  extern int audit_rule_change(int type, __u32 portid, int seq,
> void *data, size_t datasz);
>  extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 678c3f0..994588e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -934,7 +934,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
> nlmsghdr *nlh)
> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> return 0;
>
> -   err = audit_filter_user(msg_type);
> +   err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> @@ -1382,7 +1382,7 @@ struct audit_buffer *audit_log_start(struct 
> audit_context *ctx, gfp_t gfp_mask,
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
> -   if (unlikely(audit_filter_type(type)))
> +   if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> return NULL;
>
> if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index cbbe6bb..1879f02 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
>  extern kuid_t audit_sig_uid;
>  extern u32 audit_sig_sid;
>
> +extern int audit_filter(int msgtype, unsigned int listtype);
> +
>  #ifdef CONFIG_AUDITSYSCALL
>  extern int __audit_signal_info(int sig, struct task_struct *t);
>  static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index ff59a5e..3a67acf 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1290,117 +1290,72 @@ int audit_compare_dname_path(const char *dname, 
> const char *path, int parentlen)
> return strncmp(p, dname, dlen);
>  }
>
> -static int audit_filter_user_rules(struct audit_krule *rule, int type,
> -  enum audit_state *state)
> -{
> -   int i;
> -
> -   for (i = 0; i < rule->field_count; i++) {
> -   struct audit_field *f = >fields[i];
> -   pid_t pid;
> -   int result = 0;
> -   u32 sid;
> -
> -   switch (f->type) {
> -   case AUDIT_PID:
> -   pid = task_pid_nr(current);
> -   result = audit_comparator(pid, f->op, f->val);
> -   break;
> -   case AUDIT_UID:
> -   result = audit_uid_comparator(current_uid(), f->op, 
> f->uid);
> -   break;
> -   case AUDIT_GID:
> -   result = audit_gid_comparator(current_gid(), f->op, 
> f->gid);
> -   break;
> -   case AUDIT_LOGINUID:
> -   result = 
> audit_uid_comparator(audit_get_loginuid(current),
> - f->op, f->uid);
> -   break;
> -   case AUDIT_LOGINUID_SET:
> -   result = 

Re: [PATCH RESEND] trace: function graph: Fix filters for function_graph threshold

[Steven Rostedt]

On Mon, 27 Jun 2016 01:25:16 -0700
Joel Fernandes  wrote:

> Gentle ping. Could you anyone pick up this patch?
> 
>

Thanks for the reminder. As this isn't a regression (it never
worked ;-) do you think this should go to the stable releases, and/or
even the current release. Or are you fine with this just going in the
next merge window?

-- Steve

Re: [PATCH RESEND] trace: function graph: Fix filters for function_graph threshold

[Steven Rostedt]

On Mon, 27 Jun 2016 01:25:16 -0700
Joel Fernandes  wrote:

> Gentle ping. Could you anyone pick up this patch?
> 
>

Thanks for the reminder. As this isn't a regression (it never
worked ;-) do you think this should go to the stable releases, and/or
even the current release. Or are you fine with this just going in the
next merge window?

-- Steve

[PATCH V1] mfd: da9053: fix compiler warning message for uninitialised variable

[Steve Twiss]

From: Steve Twiss 

Fix compiler warning caused by an uninitialised variable inside
da9052_group_write() function. Defaulting the value to zero covers
the trivial case.

Signed-off-by: Steve Twiss 
Reported-by: Geert Uytterhoeven 

---
Hi Lee,

This compiler warning was found and reported by Geert Uytterhoeven as
part of the "Build regressions/improvements in v4.7-rc5" thread.

- https://lkml.org/lkml/2016/6/27/126

Regards,
Steve


 include/linux/mfd/da9052/da9052.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mfd/da9052/da9052.h 
b/include/linux/mfd/da9052/da9052.h
index c18a4c1..ce9230a 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -171,7 +171,7 @@ static inline int da9052_group_read(struct da9052 *da9052, 
unsigned char reg,
 static inline int da9052_group_write(struct da9052 *da9052, unsigned char reg,
  unsigned reg_cnt, unsigned char *val)
 {
-   int ret;
+   int ret = 0;
int i;
 
for (i = 0; i < reg_cnt; i++) {
-- 
end-of-patch for PATCH V1

[PATCH V1] mfd: da9053: fix compiler warning message for uninitialised variable

[Steve Twiss]

From: Steve Twiss 

Fix compiler warning caused by an uninitialised variable inside
da9052_group_write() function. Defaulting the value to zero covers
the trivial case.

Signed-off-by: Steve Twiss 
Reported-by: Geert Uytterhoeven 

---
Hi Lee,

This compiler warning was found and reported by Geert Uytterhoeven as
part of the "Build regressions/improvements in v4.7-rc5" thread.

- https://lkml.org/lkml/2016/6/27/126

Regards,
Steve


 include/linux/mfd/da9052/da9052.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mfd/da9052/da9052.h 
b/include/linux/mfd/da9052/da9052.h
index c18a4c1..ce9230a 100644
--- a/include/linux/mfd/da9052/da9052.h
+++ b/include/linux/mfd/da9052/da9052.h
@@ -171,7 +171,7 @@ static inline int da9052_group_read(struct da9052 *da9052, 
unsigned char reg,
 static inline int da9052_group_write(struct da9052 *da9052, unsigned char reg,
  unsigned reg_cnt, unsigned char *val)
 {
-   int ret;
+   int ret = 0;
int i;
 
for (i = 0; i < reg_cnt; i++) {
-- 
end-of-patch for PATCH V1

[PATCH v2 2/2] arm64: implement live patching

[Torsten Duwe]

On top of FTRACE_WITH_REGS and the klp changes that go into v4.7
this is straightforward.

Signed-off-by: Torsten Duwe 
---
 arch/arm64/Kconfig |  3 +++
 arch/arm64/include/asm/livepatch.h | 37 +
 arch/arm64/kernel/entry-ftrace.S   | 13 +
 3 files changed, 53 insertions(+)
 create mode 100644 arch/arm64/include/asm/livepatch.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 36a0e26..cb5adf3 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -80,6 +80,7 @@ config ARM64
select HAVE_GENERIC_DMA_COHERENT
select HAVE_HW_BREAKPOINT if PERF_EVENTS
select HAVE_IRQ_TIME_ACCOUNTING
+   select HAVE_LIVEPATCH
select HAVE_MEMBLOCK
select HAVE_MEMBLOCK_NODE_MAP if NUMA
select HAVE_PATA_PLATFORM
@@ -1042,4 +1043,6 @@ if CRYPTO
 source "arch/arm64/crypto/Kconfig"
 endif
 
+source "kernel/livepatch/Kconfig"
+
 source "lib/Kconfig"
diff --git a/arch/arm64/include/asm/livepatch.h 
b/arch/arm64/include/asm/livepatch.h
new file mode 100644
index 000..6b9a3d1
--- /dev/null
+++ b/arch/arm64/include/asm/livepatch.h
@@ -0,0 +1,37 @@
+/*
+ * livepatch.h - arm64-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 SUSE
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+#ifndef _ASM_ARM64_LIVEPATCH_H
+#define _ASM_ARM64_LIVEPATCH_H
+
+#include 
+#include 
+
+#ifdef CONFIG_LIVEPATCH
+static inline int klp_check_compiler_support(void)
+{
+   return 0;
+}
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+   regs->pc = ip;
+}
+#endif /* CONFIG_LIVEPATCH */
+
+#endif /* _ASM_ARM64_LIVEPATCH_H */
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 3ebe791..b166cbf 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -204,6 +204,9 @@ ENTRY(ftrace_caller)
str x9, [sp, #S_LR]
/* The program counter just after the ftrace call site */
str lr, [sp, #S_PC]
+#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER)
+   mov x19,lr  /* remember old return address */
+#endif
/* The stack pointer as it was on ftrace_caller entry... */
add x29, sp, #S_FRAME_SIZE+16   /* ...is also our new FP */
str x29, [sp, #S_SP]
@@ -219,6 +222,16 @@ ftrace_call:
 
bl  ftrace_stub
 
+#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER)
+   /* Is the trace function a live patcher an has messed with
+* the return address?
+   */
+   ldr x9, [sp, #S_PC]
+   cmp x9, x19 /* compare with the value we remembered */
+   /* to not call graph tracer's "call" mechanism twice! */
+   b.eqftrace_regs_return
+#endif
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
.global ftrace_graph_call
 ftrace_graph_call: // ftrace_graph_caller();
-- 
2.6.6

[PATCH v2 2/2] arm64: implement live patching

[Torsten Duwe]

On top of FTRACE_WITH_REGS and the klp changes that go into v4.7
this is straightforward.

Signed-off-by: Torsten Duwe 
---
 arch/arm64/Kconfig |  3 +++
 arch/arm64/include/asm/livepatch.h | 37 +
 arch/arm64/kernel/entry-ftrace.S   | 13 +
 3 files changed, 53 insertions(+)
 create mode 100644 arch/arm64/include/asm/livepatch.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 36a0e26..cb5adf3 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -80,6 +80,7 @@ config ARM64
select HAVE_GENERIC_DMA_COHERENT
select HAVE_HW_BREAKPOINT if PERF_EVENTS
select HAVE_IRQ_TIME_ACCOUNTING
+   select HAVE_LIVEPATCH
select HAVE_MEMBLOCK
select HAVE_MEMBLOCK_NODE_MAP if NUMA
select HAVE_PATA_PLATFORM
@@ -1042,4 +1043,6 @@ if CRYPTO
 source "arch/arm64/crypto/Kconfig"
 endif
 
+source "kernel/livepatch/Kconfig"
+
 source "lib/Kconfig"
diff --git a/arch/arm64/include/asm/livepatch.h 
b/arch/arm64/include/asm/livepatch.h
new file mode 100644
index 000..6b9a3d1
--- /dev/null
+++ b/arch/arm64/include/asm/livepatch.h
@@ -0,0 +1,37 @@
+/*
+ * livepatch.h - arm64-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 SUSE
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+#ifndef _ASM_ARM64_LIVEPATCH_H
+#define _ASM_ARM64_LIVEPATCH_H
+
+#include 
+#include 
+
+#ifdef CONFIG_LIVEPATCH
+static inline int klp_check_compiler_support(void)
+{
+   return 0;
+}
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+   regs->pc = ip;
+}
+#endif /* CONFIG_LIVEPATCH */
+
+#endif /* _ASM_ARM64_LIVEPATCH_H */
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 3ebe791..b166cbf 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -204,6 +204,9 @@ ENTRY(ftrace_caller)
str x9, [sp, #S_LR]
/* The program counter just after the ftrace call site */
str lr, [sp, #S_PC]
+#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER)
+   mov x19,lr  /* remember old return address */
+#endif
/* The stack pointer as it was on ftrace_caller entry... */
add x29, sp, #S_FRAME_SIZE+16   /* ...is also our new FP */
str x29, [sp, #S_SP]
@@ -219,6 +222,16 @@ ftrace_call:
 
bl  ftrace_stub
 
+#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER)
+   /* Is the trace function a live patcher an has messed with
+* the return address?
+   */
+   ldr x9, [sp, #S_PC]
+   cmp x9, x19 /* compare with the value we remembered */
+   /* to not call graph tracer's "call" mechanism twice! */
+   b.eqftrace_regs_return
+#endif
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
.global ftrace_graph_call
 ftrace_graph_call: // ftrace_graph_caller();
-- 
2.6.6

[PATCH v2 1/2] arm64: implement FTRACE_WITH_REGS

[Torsten Duwe]

Once gcc is enhanced to optionally generate NOPs at the beginning
of each function, like the concept proven in
https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01671.html
(sans the "fprintf (... pad_size);", which spoils the data structure
for kernel use), the generated pads can nicely be used to reroute
function calls for tracing/profiling, or live patching.

The pads look like
fc00081335f0 :
fc00081335f0:   d503201fnop
fc00081335f4:   d503201fnop
fc00081335f8:   a9bd7bfdstp x29, x30, [sp,#-48]!
fc00081335fc:   910003fdmov x29, sp
[...]

This patch gets the pad locations from the compiler-generated
__prolog_pads_loc into the _mcount_loc array, and provides the
code patching functions to turn the pads at runtime into

fc00081335f0 mov x9, x30
fc00081335f4 bl  0xfc00080a08c0 
fc00081335f8 stp x29, x30, [sp,#-48]!
fc00081335fc mov x29, sp

as well as an ftrace_caller that can handle these call sites.
Now ARCH_SUPPORTS_FTRACE_OPS as a benefit, and the graph caller
still works, too.

Signed-off-by: Li Bin 
Signed-off-by: Torsten Duwe 
---
 arch/arm64/Kconfig|  1 +
 arch/arm64/Makefile   |  4 ++
 arch/arm64/include/asm/ftrace.h   |  8 
 arch/arm64/kernel/Makefile|  6 +--
 arch/arm64/kernel/entry-ftrace.S  | 89 +++
 arch/arm64/kernel/ftrace.c| 43 +--
 include/asm-generic/vmlinux.lds.h |  2 +-
 include/linux/compiler.h  |  4 ++
 8 files changed, 150 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691..36a0e26 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -72,6 +72,7 @@ config ARM64
select HAVE_DMA_API_DEBUG
select HAVE_DMA_CONTIGUOUS
select HAVE_DYNAMIC_FTRACE
+   select HAVE_DYNAMIC_FTRACE_WITH_REGS
select HAVE_EFFICIENT_UNALIGNED_ACCESS
select HAVE_FTRACE_MCOUNT_RECORD
select HAVE_FUNCTION_TRACER
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 648a32c..e5e335c 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -35,6 +35,10 @@ KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS  += $(call cc-option, -mpc-relative-literal-loads)
 KBUILD_AFLAGS  += $(lseinstr)
 
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS), y)
+CC_FLAGS_FTRACE := -fprolog-pad=2 -DCC_USING_PROLOG_PAD
+endif
+
 ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
 KBUILD_CPPFLAGS+= -mbig-endian
 AS += -EB
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index caa955f..a569666 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -16,6 +16,14 @@
 #define MCOUNT_ADDR((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE   AARCH64_INSN_SIZE
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#define REC_IP_BRANCH_OFFSET 4
+#define FTRACE_REGS_ADDR FTRACE_ADDR
+#else
+#define REC_IP_BRANCH_OFFSET 0
+#endif
+
 #ifndef __ASSEMBLY__
 #include 
 
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 2173149..c26f3f8 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -6,9 +6,9 @@ CPPFLAGS_vmlinux.lds:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 AFLAGS_head.o  := -DTEXT_OFFSET=$(TEXT_OFFSET)
 CFLAGS_armv8_deprecated.o := -I$(src)
 
-CFLAGS_REMOVE_ftrace.o = -pg
-CFLAGS_REMOVE_insn.o = -pg
-CFLAGS_REMOVE_return_address.o = -pg
+CFLAGS_REMOVE_ftrace.o = -pg $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_insn.o = -pg $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_return_address.o = -pg $(CC_FLAGS_FTRACE)
 
 # Object file lists.
 arm64-obj-y:= debug-monitors.o entry.o irq.o fpsimd.o  
\
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 0f03a8f..3ebe791 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -12,6 +12,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * Gcc with -pg will put the following code in the beginning of each function:
@@ -132,6 +134,7 @@ skip_ftrace_call:
 ENDPROC(_mcount)
 
 #else /* CONFIG_DYNAMIC_FTRACE */
+#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 /*
  * _mcount() is used to build the kernel with -pg option, but all the branch
  * instructions to _mcount() are replaced to NOP initially at kernel start up,
@@ -171,6 +174,84 @@ ftrace_graph_call: // 
ftrace_graph_caller();
 
mcount_exit
 ENDPROC(ftrace_caller)
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+ENTRY(_mcount)
+   mov x10, lr
+   mov lr, x9
+   ret x10
+ENDPROC(_mcount)
+
+ENTRY(ftrace_caller)
+   stp x29, x9, [sp, #-16]!
+   sub sp, sp, #S_FRAME_SIZE
+
+   stp x0, x1, [sp]
+   stp x2, x3, [sp, #16]
+   stp x4, x5, [sp, #32]
+   stp x6, 

[PATCH v2 1/2] arm64: implement FTRACE_WITH_REGS

[Torsten Duwe]

Once gcc is enhanced to optionally generate NOPs at the beginning
of each function, like the concept proven in
https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01671.html
(sans the "fprintf (... pad_size);", which spoils the data structure
for kernel use), the generated pads can nicely be used to reroute
function calls for tracing/profiling, or live patching.

The pads look like
fc00081335f0 :
fc00081335f0:   d503201fnop
fc00081335f4:   d503201fnop
fc00081335f8:   a9bd7bfdstp x29, x30, [sp,#-48]!
fc00081335fc:   910003fdmov x29, sp
[...]

This patch gets the pad locations from the compiler-generated
__prolog_pads_loc into the _mcount_loc array, and provides the
code patching functions to turn the pads at runtime into

fc00081335f0 mov x9, x30
fc00081335f4 bl  0xfc00080a08c0 
fc00081335f8 stp x29, x30, [sp,#-48]!
fc00081335fc mov x29, sp

as well as an ftrace_caller that can handle these call sites.
Now ARCH_SUPPORTS_FTRACE_OPS as a benefit, and the graph caller
still works, too.

Signed-off-by: Li Bin 
Signed-off-by: Torsten Duwe 
---
 arch/arm64/Kconfig|  1 +
 arch/arm64/Makefile   |  4 ++
 arch/arm64/include/asm/ftrace.h   |  8 
 arch/arm64/kernel/Makefile|  6 +--
 arch/arm64/kernel/entry-ftrace.S  | 89 +++
 arch/arm64/kernel/ftrace.c| 43 +--
 include/asm-generic/vmlinux.lds.h |  2 +-
 include/linux/compiler.h  |  4 ++
 8 files changed, 150 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691..36a0e26 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -72,6 +72,7 @@ config ARM64
select HAVE_DMA_API_DEBUG
select HAVE_DMA_CONTIGUOUS
select HAVE_DYNAMIC_FTRACE
+   select HAVE_DYNAMIC_FTRACE_WITH_REGS
select HAVE_EFFICIENT_UNALIGNED_ACCESS
select HAVE_FTRACE_MCOUNT_RECORD
select HAVE_FUNCTION_TRACER
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 648a32c..e5e335c 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -35,6 +35,10 @@ KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS  += $(call cc-option, -mpc-relative-literal-loads)
 KBUILD_AFLAGS  += $(lseinstr)
 
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_REGS), y)
+CC_FLAGS_FTRACE := -fprolog-pad=2 -DCC_USING_PROLOG_PAD
+endif
+
 ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
 KBUILD_CPPFLAGS+= -mbig-endian
 AS += -EB
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index caa955f..a569666 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -16,6 +16,14 @@
 #define MCOUNT_ADDR((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE   AARCH64_INSN_SIZE
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#define REC_IP_BRANCH_OFFSET 4
+#define FTRACE_REGS_ADDR FTRACE_ADDR
+#else
+#define REC_IP_BRANCH_OFFSET 0
+#endif
+
 #ifndef __ASSEMBLY__
 #include 
 
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 2173149..c26f3f8 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -6,9 +6,9 @@ CPPFLAGS_vmlinux.lds:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 AFLAGS_head.o  := -DTEXT_OFFSET=$(TEXT_OFFSET)
 CFLAGS_armv8_deprecated.o := -I$(src)
 
-CFLAGS_REMOVE_ftrace.o = -pg
-CFLAGS_REMOVE_insn.o = -pg
-CFLAGS_REMOVE_return_address.o = -pg
+CFLAGS_REMOVE_ftrace.o = -pg $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_insn.o = -pg $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_return_address.o = -pg $(CC_FLAGS_FTRACE)
 
 # Object file lists.
 arm64-obj-y:= debug-monitors.o entry.o irq.o fpsimd.o  
\
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 0f03a8f..3ebe791 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -12,6 +12,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * Gcc with -pg will put the following code in the beginning of each function:
@@ -132,6 +134,7 @@ skip_ftrace_call:
 ENDPROC(_mcount)
 
 #else /* CONFIG_DYNAMIC_FTRACE */
+#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 /*
  * _mcount() is used to build the kernel with -pg option, but all the branch
  * instructions to _mcount() are replaced to NOP initially at kernel start up,
@@ -171,6 +174,84 @@ ftrace_graph_call: // 
ftrace_graph_caller();
 
mcount_exit
 ENDPROC(ftrace_caller)
+#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
+ENTRY(_mcount)
+   mov x10, lr
+   mov lr, x9
+   ret x10
+ENDPROC(_mcount)
+
+ENTRY(ftrace_caller)
+   stp x29, x9, [sp, #-16]!
+   sub sp, sp, #S_FRAME_SIZE
+
+   stp x0, x1, [sp]
+   stp x2, x3, [sp, #16]
+   stp x4, x5, [sp, #32]
+   stp x6, x7, [sp, #48]
+   stp x8, x9, 

[PATCH v2 0/2] arm64 live patching

[Torsten Duwe]

So here is a slightly updated FTRACE_WITH_REGS plus live patching.
Reminder: make sure you have a prolog-pad gcc, and this in your
top level Makefile:

ifdef CONFIG_LIVEPATCH
KBUILD_CFLAGS   += $(call cc-option,-fno-ipa-ra)
endif

Tested with v4.7-rc3 + gcc-6.1

Changes since v1:
  * instead of a comment "should be CC_USING_PROLOG_PAD":
do it. CC_FLAGS_FTRACE holds it now, and the IPA
disabler has become a separate issue (see above).

Torsten Duwe (2):
  arm64: implement FTRACE_WITH_REGS
  arm64: implement live patching

 arch/arm64/Kconfig |   4 ++
 arch/arm64/Makefile|   4 ++
 arch/arm64/include/asm/ftrace.h|   8 +++
 arch/arm64/include/asm/livepatch.h |  37 ++
 arch/arm64/kernel/Makefile |   6 +--
 arch/arm64/kernel/entry-ftrace.S   | 102 +
 arch/arm64/kernel/ftrace.c |  43 ++--
 include/asm-generic/vmlinux.lds.h  |   2 +-
 include/linux/compiler.h   |   4 ++
 9 files changed, 203 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm64/include/asm/livepatch.h

-- 
2.6.6

[PATCH v2 0/2] arm64 live patching

[Torsten Duwe]

So here is a slightly updated FTRACE_WITH_REGS plus live patching.
Reminder: make sure you have a prolog-pad gcc, and this in your
top level Makefile:

ifdef CONFIG_LIVEPATCH
KBUILD_CFLAGS   += $(call cc-option,-fno-ipa-ra)
endif

Tested with v4.7-rc3 + gcc-6.1

Changes since v1:
  * instead of a comment "should be CC_USING_PROLOG_PAD":
do it. CC_FLAGS_FTRACE holds it now, and the IPA
disabler has become a separate issue (see above).

Torsten Duwe (2):
  arm64: implement FTRACE_WITH_REGS
  arm64: implement live patching

 arch/arm64/Kconfig |   4 ++
 arch/arm64/Makefile|   4 ++
 arch/arm64/include/asm/ftrace.h|   8 +++
 arch/arm64/include/asm/livepatch.h |  37 ++
 arch/arm64/kernel/Makefile |   6 +--
 arch/arm64/kernel/entry-ftrace.S   | 102 +
 arch/arm64/kernel/ftrace.c |  43 ++--
 include/asm-generic/vmlinux.lds.h  |   2 +-
 include/linux/compiler.h   |   4 ++
 9 files changed, 203 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm64/include/asm/livepatch.h

-- 
2.6.6

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Brian Gerst]

On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>> This allows x86_64 kernels to enable vmapped stacks.  There are a
>> couple of interesting bits.
>>
>> First, x86 lazily faults in top-level paging entries for the vmalloc
>> area.  This won't work if we get a page fault while trying to access
>> the stack: the CPU will promote it to a double-fault and we'll die.
>> To avoid this problem, probe the new stack when switching stacks and
>> forcibly populate the pgd entry for the stack when switching mms.
>>
>> Second, once we have guard pages around the stack, we'll want to
>> detect and handle stack overflow.
>>
>> I didn't enable it on x86_32.  We'd need to rework the double-fault
>> code a bit and I'm concerned about running out of vmalloc virtual
>> addresses under some workloads.
>>
>> This patch, by itself, will behave somewhat erratically when the
>> stack overflows while RSP is still more than a few tens of bytes
>> above the bottom of the stack.  Specifically, we'll get #PF and make
>> it to no_context and an oops without triggering a double-fault, and
>> no_context doesn't know about stack overflows.  The next patch will
>> improve that case.
>>
>> Signed-off-by: Andy Lutomirski 
>> ---
>>  arch/x86/Kconfig |  1 +
>>  arch/x86/include/asm/switch_to.h | 28 +++-
>>  arch/x86/kernel/traps.c  | 32 
>>  arch/x86/mm/tlb.c| 15 +++
>>  4 files changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index d9a94da0c29f..afdcf96ef109 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -92,6 +92,7 @@ config X86
>> select HAVE_ARCH_TRACEHOOK
>> select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> select HAVE_EBPF_JITif X86_64
>> +   select HAVE_ARCH_VMAP_STACK if X86_64
>> select HAVE_CC_STACKPROTECTOR
>> select HAVE_CMPXCHG_DOUBLE
>> select HAVE_CMPXCHG_LOCAL
>> diff --git a/arch/x86/include/asm/switch_to.h 
>> b/arch/x86/include/asm/switch_to.h
>> index 8f321a1b03a1..14e4b20f0aaf 100644
>> --- a/arch/x86/include/asm/switch_to.h
>> +++ b/arch/x86/include/asm/switch_to.h
>> @@ -8,6 +8,28 @@ struct tss_struct;
>>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct 
>> *next_p,
>>   struct tss_struct *tss);
>>
>> +/* This runs runs on the previous thread's stack. */
>> +static inline void prepare_switch_to(struct task_struct *prev,
>> +struct task_struct *next)
>> +{
>> +#ifdef CONFIG_VMAP_STACK
>> +   /*
>> +* If we switch to a stack that has a top-level paging entry
>> +* that is not present in the current mm, the resulting #PF will
>> +* will be promoted to a double-fault and we'll panic.  Probe
>> +* the new stack now so that vmalloc_fault can fix up the page
>> +* tables if needed.  This can only happen if we use a stack
>> +* in vmap space.
>> +*
>> +* We assume that the stack is aligned so that it never spans
>> +* more than one top-level paging entry.
>> +*
>> +* To minimize cache pollution, just follow the stack pointer.
>> +*/
>> +   READ_ONCE(*(unsigned char *)next->thread.sp);
>> +#endif
>> +}
>> +
>>  #ifdef CONFIG_X86_32
>>
>>  #ifdef CONFIG_CC_STACKPROTECTOR
>> @@ -39,6 +61,8 @@ do {   
>>\
>>  */ \
>> unsigned long ebx, ecx, edx, esi, edi;  \
>> \
>> +   prepare_switch_to(prev, next);  \
>> +   \
>> asm volatile("pushl %%ebp\n\t"  /* saveEBP   */ \
>>  "movl %%esp,%[prev_sp]\n\t"/* saveESP   */ \
>>  "movl %[next_sp],%%esp\n\t"/* restore ESP   */ \
>> @@ -103,7 +127,9 @@ do { 
>>\
>>   * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
>>   * has no effect.
>>   */
>> -#define switch_to(prev, next, last) \
>> +#define switch_to(prev, next, last)  \
>> +   prepare_switch_to(prev, next);\
>> + \
>> asm volatile(SAVE_CONTEXT \
>>  "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */   \
>>  "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */  

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:11 PM, Sudeep Holla wrote:



On 27/06/16 16:08, Daniel Lezcano wrote:

On 06/27/2016 05:06 PM, Sudeep Holla wrote:


[...]


Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?



Yes, you can get it from [1]


Great. Thanks Sudeep !


Regards,
Sudeep

[1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi



--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 15/29] x86/mm/64: Enable vmapped stacks

[Brian Gerst]

On Mon, Jun 27, 2016 at 11:01 AM, Brian Gerst  wrote:
> On Sun, Jun 26, 2016 at 5:55 PM, Andy Lutomirski  wrote:
>> This allows x86_64 kernels to enable vmapped stacks.  There are a
>> couple of interesting bits.
>>
>> First, x86 lazily faults in top-level paging entries for the vmalloc
>> area.  This won't work if we get a page fault while trying to access
>> the stack: the CPU will promote it to a double-fault and we'll die.
>> To avoid this problem, probe the new stack when switching stacks and
>> forcibly populate the pgd entry for the stack when switching mms.
>>
>> Second, once we have guard pages around the stack, we'll want to
>> detect and handle stack overflow.
>>
>> I didn't enable it on x86_32.  We'd need to rework the double-fault
>> code a bit and I'm concerned about running out of vmalloc virtual
>> addresses under some workloads.
>>
>> This patch, by itself, will behave somewhat erratically when the
>> stack overflows while RSP is still more than a few tens of bytes
>> above the bottom of the stack.  Specifically, we'll get #PF and make
>> it to no_context and an oops without triggering a double-fault, and
>> no_context doesn't know about stack overflows.  The next patch will
>> improve that case.
>>
>> Signed-off-by: Andy Lutomirski 
>> ---
>>  arch/x86/Kconfig |  1 +
>>  arch/x86/include/asm/switch_to.h | 28 +++-
>>  arch/x86/kernel/traps.c  | 32 
>>  arch/x86/mm/tlb.c| 15 +++
>>  4 files changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index d9a94da0c29f..afdcf96ef109 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -92,6 +92,7 @@ config X86
>> select HAVE_ARCH_TRACEHOOK
>> select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> select HAVE_EBPF_JITif X86_64
>> +   select HAVE_ARCH_VMAP_STACK if X86_64
>> select HAVE_CC_STACKPROTECTOR
>> select HAVE_CMPXCHG_DOUBLE
>> select HAVE_CMPXCHG_LOCAL
>> diff --git a/arch/x86/include/asm/switch_to.h 
>> b/arch/x86/include/asm/switch_to.h
>> index 8f321a1b03a1..14e4b20f0aaf 100644
>> --- a/arch/x86/include/asm/switch_to.h
>> +++ b/arch/x86/include/asm/switch_to.h
>> @@ -8,6 +8,28 @@ struct tss_struct;
>>  void __switch_to_xtra(struct task_struct *prev_p, struct task_struct 
>> *next_p,
>>   struct tss_struct *tss);
>>
>> +/* This runs runs on the previous thread's stack. */
>> +static inline void prepare_switch_to(struct task_struct *prev,
>> +struct task_struct *next)
>> +{
>> +#ifdef CONFIG_VMAP_STACK
>> +   /*
>> +* If we switch to a stack that has a top-level paging entry
>> +* that is not present in the current mm, the resulting #PF will
>> +* will be promoted to a double-fault and we'll panic.  Probe
>> +* the new stack now so that vmalloc_fault can fix up the page
>> +* tables if needed.  This can only happen if we use a stack
>> +* in vmap space.
>> +*
>> +* We assume that the stack is aligned so that it never spans
>> +* more than one top-level paging entry.
>> +*
>> +* To minimize cache pollution, just follow the stack pointer.
>> +*/
>> +   READ_ONCE(*(unsigned char *)next->thread.sp);
>> +#endif
>> +}
>> +
>>  #ifdef CONFIG_X86_32
>>
>>  #ifdef CONFIG_CC_STACKPROTECTOR
>> @@ -39,6 +61,8 @@ do {   
>>\
>>  */ \
>> unsigned long ebx, ecx, edx, esi, edi;  \
>> \
>> +   prepare_switch_to(prev, next);  \
>> +   \
>> asm volatile("pushl %%ebp\n\t"  /* saveEBP   */ \
>>  "movl %%esp,%[prev_sp]\n\t"/* saveESP   */ \
>>  "movl %[next_sp],%%esp\n\t"/* restore ESP   */ \
>> @@ -103,7 +127,9 @@ do { 
>>\
>>   * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
>>   * has no effect.
>>   */
>> -#define switch_to(prev, next, last) \
>> +#define switch_to(prev, next, last)  \
>> +   prepare_switch_to(prev, next);\
>> + \
>> asm volatile(SAVE_CONTEXT \
>>  "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */   \
>>  "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */\
>> diff --git a/arch/x86/kernel/traps.c 

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:11 PM, Sudeep Holla wrote:



On 27/06/16 16:08, Daniel Lezcano wrote:

On 06/27/2016 05:06 PM, Sudeep Holla wrote:


[...]


Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?



Yes, you can get it from [1]


Great. Thanks Sudeep !


Regards,
Sudeep

[1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi



--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

On 27/06/16 16:08, Daniel Lezcano wrote:

On 06/27/2016 05:06 PM, Sudeep Holla wrote:


[...]


Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?



Yes, you can get it from [1]

--
Regards,
Sudeep

[1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

On 27/06/16 16:08, Daniel Lezcano wrote:

On 06/27/2016 05:06 PM, Sudeep Holla wrote:


[...]


Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?



Yes, you can get it from [1]

--
Regards,
Sudeep

[1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi

Re: wake_up on wait-queue called from hard-irq context in 3.10.63-rt65

[Steven Rostedt]

On Wed, Apr 27, 2016 at 12:55:47PM +0200, Andreas Starzer wrote:
> This Bug was already fixed in rcutiny_plugin.h with changing the
> wait-queue to simple-waiter.
> 
> Found this bug with 3.10.63-rt65 in rcutree_plugin.h too. (It is not
> fixed in current 3.10-release)
> 
> SVC (hard-irq-context) for scheduling tries to wake-up wait-queue
> waiters and therefore simple-waiter is needed.
> 
> I fixed the problem for myself with this patch, but I don't know if
> this is a correct fix.

Is this a bug with 4.1-rt too? This the stable rt releases get their fixes
from either mainline stable, or from rt-devel (the upstream RT development).

It looks to me that 4.1-rt would have the same issue here, although 4.6-rt has
this code rewritten a bit, although it appears that mainline itself converted
this to a swait (after swait made it into mainline).

But if this bug does affect 4.1-rt, it should go against that and be marked
for stable...@vger.kernel.org, and it will then be backported to the older
rt kernels.

-- Steve


> 
> Index: src/kernel/rcutree_plugin.h
> ===
> --- src/kernel/rcutree_plugin.h(revision 159)
> +++ src/kernel/rcutree_plugin.h(working copy)
> @@ -705,7 +705,8 @@
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu);
> 
> -static DECLARE_WAIT_QUEUE_HEAD(sync_rcu_preempt_exp_wq);
> +/* (stz): bug#16085 replace standard wq by simple waiter for call by
> SVC context. */
> +static DEFINE_SWAIT_HEAD(sync_rcu_preempt_exp_wq);
>  static unsigned long sync_rcu_preempt_exp_count;
>  static DEFINE_MUTEX(sync_rcu_preempt_exp_mutex);
> 
> @@ -762,8 +763,10 @@
>  }
>  if (rnp->parent == NULL) {
>  raw_spin_unlock_irqrestore(>lock, flags);
> -if (wake)
> -wake_up(_rcu_preempt_exp_wq);
> +if (wake) {
> +  /* (stz): bug#16085 replace standard wq by simple
> waiter for call by SVC context. */
> +  swait_wake(_rcu_preempt_exp_wq);
> +}
>  break;
>  }
>  mask = rnp->grpmask;
> @@ -883,8 +886,9 @@
> 
>  /* Wait for snapshotted ->blkd_tasks lists to drain. */
>  rnp = rcu_get_root(rsp);
> -wait_event(sync_rcu_preempt_exp_wq,
> -   sync_rcu_preempt_exp_done(rnp));
> +/* (stz): bug#16085 replace standard wq by simple waiter for call
> by SVC context. */
> +swait_event(sync_rcu_preempt_exp_wq,
> +   sync_rcu_preempt_exp_done(rnp));
> 
>  /* Clean up and exit. */
>  smp_mb(); /* ensure expedited GP seen before counter increment. */
> 
> 
> 
> --
> Detailed Output:
> --
> 
> [22503.069680] BUG: sleeping function called from invalid context at
> kernel/rtmutex.c:796
> [22503.069685] in_atomic(): 1, irqs_disabled(): 0, pid: 385, name: rstpd
> [22503.069707] Preemption disabled at:[<8000e28c>] svc_preempt+0x8/0x20
> [22503.069708]
> [22503.069717] CPU: 1 PID: 385 Comm: rstpd Tainted: G   O
> 3.10.63-sp4-rt65-svn160 #6
> [22503.069723] Backtrace:
> [22503.069742] [<80011d08>] (dump_backtrace+0x0/0x108) from
> [<80011f18>] (show_stack+0x18/0x1c)
> [22503.069753]  r6:0003 r5:8c906000 r4: r3:
> [22503.069772] [<80011f00>] (show_stack+0x0/0x1c) from [<8060ce58>]
> (dump_stack+0x24/0x28)
> [22503.069794] [<8060ce34>] (dump_stack+0x0/0x28) from [<80054524>]
> (__might_sleep+0x100/0x158)
> [22503.069810] [<80054424>] (__might_sleep+0x0/0x158) from
> [<806104f4>] (rt_spin_lock+0x24/0x30)
> [22503.069816]  r5:0001 r4:8086461c
> [22503.069831] [<806104d0>] (rt_spin_lock+0x0/0x30) from [<800565b0>]
> (__wake_up+0x2c/0x58)
> [22503.069837]  r4:8086461c r3:0001
> [22503.069850] [<80056584>] (__wake_up+0x0/0x58) from [<80089f9c>]
> (rcu_report_exp_rnp.isra.50+0xac/0xb4)
> [22503.069861]  r7:0001 r6:20010113 r5:0001 r4:80864340
> [22503.069874] [<80089ef0>] (rcu_report_exp_rnp.isra.50+0x0/0xb4) from
> [<8008b9d0>] (rcu_read_unlock_special+0x138/0x1cc)
> [22503.069885]  r7:40010113 r6:80864340 r5:0001 r4:80864340
> [22503.069897] [<8008b898>] (rcu_read_unlock_special+0x0/0x1cc) from
> [<8008bb10>] (rcu_note_context_switch+0xac/0x1fc)
> [22503.069912]  r9:0001 r8:8c906000 r7:0001 r6:8c3a6e80 r5:8c906008
> [22503.069912] r4:80853a38
> [22503.069924] [<8008ba64>] (rcu_note_context_switch+0x0/0x1fc) from
> [<8060eee4>] (__schedule+0x70/0x44c)
> [22503.069935] [<8060ee74>] (__schedule+0x0/0x44c) from [<8060f898>]
> (preempt_schedule_irq+0x40/0x6c)
> [22503.069947] [<8060f858>] (preempt_schedule_irq+0x0/0x6c) from
> [<8000e28c>] (svc_preempt+0x8/0x20)
> [22503.069953]  r4:800476bc r3:
> [22503.069979] [<8004766c>] (__rcu_read_unlock+0x0/0x5c) from
> [<8046db5c>] (dev_queue_xmit+0x200/0x444)
> [22503.070007]  r4:8c957c00 r3:1000
> [22503.070031] [<8046d95c>] (dev_queue_xmit+0x0/0x444) from
> [<8059f390>] (packet_sendmsg+0xb74/0xca0)
> [22503.070059] [<8059e81c>] (packet_sendmsg+0x0/0xca0) from

Re: wake_up on wait-queue called from hard-irq context in 3.10.63-rt65

[Steven Rostedt]

On Wed, Apr 27, 2016 at 12:55:47PM +0200, Andreas Starzer wrote:
> This Bug was already fixed in rcutiny_plugin.h with changing the
> wait-queue to simple-waiter.
> 
> Found this bug with 3.10.63-rt65 in rcutree_plugin.h too. (It is not
> fixed in current 3.10-release)
> 
> SVC (hard-irq-context) for scheduling tries to wake-up wait-queue
> waiters and therefore simple-waiter is needed.
> 
> I fixed the problem for myself with this patch, but I don't know if
> this is a correct fix.

Is this a bug with 4.1-rt too? This the stable rt releases get their fixes
from either mainline stable, or from rt-devel (the upstream RT development).

It looks to me that 4.1-rt would have the same issue here, although 4.6-rt has
this code rewritten a bit, although it appears that mainline itself converted
this to a swait (after swait made it into mainline).

But if this bug does affect 4.1-rt, it should go against that and be marked
for stable...@vger.kernel.org, and it will then be backported to the older
rt kernels.

-- Steve


> 
> Index: src/kernel/rcutree_plugin.h
> ===
> --- src/kernel/rcutree_plugin.h(revision 159)
> +++ src/kernel/rcutree_plugin.h(working copy)
> @@ -705,7 +705,8 @@
>  }
>  EXPORT_SYMBOL_GPL(synchronize_rcu);
> 
> -static DECLARE_WAIT_QUEUE_HEAD(sync_rcu_preempt_exp_wq);
> +/* (stz): bug#16085 replace standard wq by simple waiter for call by
> SVC context. */
> +static DEFINE_SWAIT_HEAD(sync_rcu_preempt_exp_wq);
>  static unsigned long sync_rcu_preempt_exp_count;
>  static DEFINE_MUTEX(sync_rcu_preempt_exp_mutex);
> 
> @@ -762,8 +763,10 @@
>  }
>  if (rnp->parent == NULL) {
>  raw_spin_unlock_irqrestore(>lock, flags);
> -if (wake)
> -wake_up(_rcu_preempt_exp_wq);
> +if (wake) {
> +  /* (stz): bug#16085 replace standard wq by simple
> waiter for call by SVC context. */
> +  swait_wake(_rcu_preempt_exp_wq);
> +}
>  break;
>  }
>  mask = rnp->grpmask;
> @@ -883,8 +886,9 @@
> 
>  /* Wait for snapshotted ->blkd_tasks lists to drain. */
>  rnp = rcu_get_root(rsp);
> -wait_event(sync_rcu_preempt_exp_wq,
> -   sync_rcu_preempt_exp_done(rnp));
> +/* (stz): bug#16085 replace standard wq by simple waiter for call
> by SVC context. */
> +swait_event(sync_rcu_preempt_exp_wq,
> +   sync_rcu_preempt_exp_done(rnp));
> 
>  /* Clean up and exit. */
>  smp_mb(); /* ensure expedited GP seen before counter increment. */
> 
> 
> 
> --
> Detailed Output:
> --
> 
> [22503.069680] BUG: sleeping function called from invalid context at
> kernel/rtmutex.c:796
> [22503.069685] in_atomic(): 1, irqs_disabled(): 0, pid: 385, name: rstpd
> [22503.069707] Preemption disabled at:[<8000e28c>] svc_preempt+0x8/0x20
> [22503.069708]
> [22503.069717] CPU: 1 PID: 385 Comm: rstpd Tainted: G   O
> 3.10.63-sp4-rt65-svn160 #6
> [22503.069723] Backtrace:
> [22503.069742] [<80011d08>] (dump_backtrace+0x0/0x108) from
> [<80011f18>] (show_stack+0x18/0x1c)
> [22503.069753]  r6:0003 r5:8c906000 r4: r3:
> [22503.069772] [<80011f00>] (show_stack+0x0/0x1c) from [<8060ce58>]
> (dump_stack+0x24/0x28)
> [22503.069794] [<8060ce34>] (dump_stack+0x0/0x28) from [<80054524>]
> (__might_sleep+0x100/0x158)
> [22503.069810] [<80054424>] (__might_sleep+0x0/0x158) from
> [<806104f4>] (rt_spin_lock+0x24/0x30)
> [22503.069816]  r5:0001 r4:8086461c
> [22503.069831] [<806104d0>] (rt_spin_lock+0x0/0x30) from [<800565b0>]
> (__wake_up+0x2c/0x58)
> [22503.069837]  r4:8086461c r3:0001
> [22503.069850] [<80056584>] (__wake_up+0x0/0x58) from [<80089f9c>]
> (rcu_report_exp_rnp.isra.50+0xac/0xb4)
> [22503.069861]  r7:0001 r6:20010113 r5:0001 r4:80864340
> [22503.069874] [<80089ef0>] (rcu_report_exp_rnp.isra.50+0x0/0xb4) from
> [<8008b9d0>] (rcu_read_unlock_special+0x138/0x1cc)
> [22503.069885]  r7:40010113 r6:80864340 r5:0001 r4:80864340
> [22503.069897] [<8008b898>] (rcu_read_unlock_special+0x0/0x1cc) from
> [<8008bb10>] (rcu_note_context_switch+0xac/0x1fc)
> [22503.069912]  r9:0001 r8:8c906000 r7:0001 r6:8c3a6e80 r5:8c906008
> [22503.069912] r4:80853a38
> [22503.069924] [<8008ba64>] (rcu_note_context_switch+0x0/0x1fc) from
> [<8060eee4>] (__schedule+0x70/0x44c)
> [22503.069935] [<8060ee74>] (__schedule+0x0/0x44c) from [<8060f898>]
> (preempt_schedule_irq+0x40/0x6c)
> [22503.069947] [<8060f858>] (preempt_schedule_irq+0x0/0x6c) from
> [<8000e28c>] (svc_preempt+0x8/0x20)
> [22503.069953]  r4:800476bc r3:
> [22503.069979] [<8004766c>] (__rcu_read_unlock+0x0/0x5c) from
> [<8046db5c>] (dev_queue_xmit+0x200/0x444)
> [22503.070007]  r4:8c957c00 r3:1000
> [22503.070031] [<8046d95c>] (dev_queue_xmit+0x0/0x444) from
> [<8059f390>] (packet_sendmsg+0xb74/0xca0)
> [22503.070059] [<8059e81c>] (packet_sendmsg+0x0/0xca0) from

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

[Mark Brown]

On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:

> + config.init_data = of_get_regulator_init_data(dev, dev->of_node, 
> _reg);
> + if (!config.init_data) {
> + return -EINVAL;
> + }

This is broken, constraints are entirely optional - the driver should
just instantiate no matter what (if anything) the constraints are.  This
means people can read the current state even if there is no need for
runtime configuration.

> + dev_info(>dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV 
> + 
> +  SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));

This is just noise, remove it - if we want to list the voltage for
regulators at probe we should be doing it consistently for all
regulators in the core rather than in individual drivers.

> +/*
> + * This is useless for OF-enabled devices, but it is needed by I2C subsystem
> + */
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

No, this is not the case - supporting DT does not mean that other
enumeration mechanisms can't be supported and there's no reason not to
list a sensible ID here.


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

[Mark Brown]

On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:

> + config.init_data = of_get_regulator_init_data(dev, dev->of_node, 
> _reg);
> + if (!config.init_data) {
> + return -EINVAL;
> + }

This is broken, constraints are entirely optional - the driver should
just instantiate no matter what (if anything) the constraints are.  This
means people can read the current state even if there is no need for
runtime configuration.

> + dev_info(>dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV 
> + 
> +  SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));

This is just noise, remove it - if we want to list the voltage for
regulators at probe we should be doing it consistently for all
regulators in the core rather than in individual drivers.

> +/*
> + * This is useless for OF-enabled devices, but it is needed by I2C subsystem
> + */
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

No, this is not the case - supporting DT does not mean that other
enumeration mechanisms can't be supported and there's no reason not to
list a sensible ID here.


signature.asc
Description: PGP signature

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:06 PM, Sudeep Holla wrote:



On 27/06/16 16:05, Daniel Lezcano wrote:

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do
inline comment.



Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:06 PM, Sudeep Holla wrote:



On 27/06/16 16:05, Daniel Lezcano wrote:

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do
inline comment.



Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.


Do you have a repo where I can see the code ?

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

On 27/06/16 16:05, Daniel Lezcano wrote:

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do
inline comment.



Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.

--
Regards,
Sudeep

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

On 27/06/16 16:05, Daniel Lezcano wrote:

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do
inline comment.



Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.

--
Regards,
Sudeep

[PATCH v2] ARM: at91: Document new TCB bindings

[Alexandre Belloni]

The current binding for the TCB is not flexible enough for some use cases
and prevents proper utilization of all the channels.

Cc: Daniel Lezcano 
Cc: Thierry Reding 
Cc: linux-...@vger.kernel.org
Cc: Rob Herring 
Cc: devicet...@vger.kernel.org
Signed-off-by: Alexandre Belloni 
---

Changes in v2:
 - added slow_clk in the examples
 - removed linuxisms

Rob, does that fit what you wanted? I prefer getting your ack on that one before
resending a 48 patches series.


 .../devicetree/bindings/arm/atmel-at91.txt | 32 ---
 .../devicetree/bindings/mfd/atmel-tcb.txt  | 62 ++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt  | 12 +++--
 3 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt 
b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index e1f5ad855f14..3dc758403d03 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -60,38 +60,6 @@ System Timer (ST) required properties:
 Its subnodes can be:
 - watchdog: compatible should be "atmel,at91rm9200-wdt"
 
-TC/TCLIB Timer required properties:
-- compatible: Should be "atmel,-tcb".
-   can be "at91rm9200" or "at91sam9x5"
-- reg: Should contain registers location and length
-- interrupts: Should contain all interrupts for the TC block
-  Note that you can specify several interrupt cells if the TC
-  block has one interrupt per channel.
-- clock-names: tuple listing input clock names.
-   Required elements: "t0_clk", "slow_clk"
-   Optional elements: "t1_clk", "t2_clk"
-- clocks: phandles to input clocks.
-
-Examples:
-
-One interrupt per TC block:
-   tcb0: timer@fff7c000 {
-   compatible = "atmel,at91rm9200-tcb";
-   reg = <0xfff7c000 0x100>;
-   interrupts = <18 4>;
-   clocks = <_clk>;
-   clock-names = "t0_clk";
-   };
-
-One interrupt per TC channel in a TC block:
-   tcb1: timer@fffdc000 {
-   compatible = "atmel,at91rm9200-tcb";
-   reg = <0xfffdc000 0x100>;
-   interrupts = <26 4 27 4 28 4>;
-   clocks = <_clk>;
-   clock-names = "t0_clk";
-   };
-
 RSTC Reset Controller required properties:
 - compatible: Should be "atmel,-rstc".
can be "at91sam9260" or "at91sam9g45" or "sama5d3"
diff --git a/Documentation/devicetree/bindings/mfd/atmel-tcb.txt 
b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
new file mode 100644
index ..71c8d83c01a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
@@ -0,0 +1,62 @@
+* Device tree bindings for Atmel Timer Counter Blocks
+- compatible: Should be "atmel,-tcb", "simple-mfd", "syscon".
+   can be "at91rm9200" or "at91sam9x5"
+- reg: Should contain registers location and length
+- #address-cells: has to be 1
+- #size-cells: has to be 0
+- interrupts: Should contain all interrupts for the TC block
+  Note that you can specify several interrupt cells if the TC
+  block has one interrupt per channel.
+- clock-names: tuple listing input clock names.
+   Required elements: "t0_clk", "slow_clk"
+   Optional elements: "t1_clk", "t2_clk"
+- clocks: phandles to input clocks.
+
+The TCB can expose multiple subdevices:
+ * a clocksource and clockevent device
+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+ counter width is 16 bits (at91rm9200-tcb), two consecutive
+ channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"
+   - reg: Should contain the TCB channel to be used
+
+ * a PWM chip: see ../pwm/atmel-tcb-pwm.txt
+
+Examples:
+
+One interrupt per TC block:
+   tcb0: timer@fff7c000 {
+   compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0xfff7c000 0x100>;
+   interrupts = <18 4>;
+   clocks = <_clk>, <>;
+   clock-names = "t0_clk", "slow_clk";
+
+   timer@0 {
+   compatible = "atmel,tcb-free-running-timer";
+   reg = <0>, <1>;
+   };
+
+   timer@2 {
+   compatible = "atmel,tcb-programmable-timer";
+   reg = <2>;
+   };
+   };
+
+One interrupt per TC channel in a TC block:
+   tcb1: timer@fffdc000 {
+   compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0xfffdc000 0x100>;
+   interrupts = <26 

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do 
inline comment.



if (max_cstate=0)
 max_cstate=1;

Probably this is because the POLL state is inserted, so there is always
an idle state. But for ARM, that is not the case.



Yes


Also, there are some places where the idle state index begins to 1. I
think it should be 0 for ARM.



Yes for LPI, it does start from 0.




--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v6 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Daniel Lezcano]

On 06/27/2016 05:03 PM, Sudeep Holla wrote:

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:

On 06/14/2016 04:48 PM, Sudeep Holla wrote:

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on
ACPI_PROCESSOR_IDLE

Cc: linux-arm-ker...@lists.infradead.org
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sudeep Holla 
---


Hi Sudeep,

now that ACPI processor supports ARM64 did you check the
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?



No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.


I deleted the patch 2/5 but there is a place where:



Sorry, I don't follow what you mean by that.


I meant I just deleted from my mailbox the patch 2/5, so I can't do 
inline comment.



if (max_cstate=0)
 max_cstate=1;

Probably this is because the POLL state is inserted, so there is always
an idle state. But for ARM, that is not the case.



Yes


Also, there are some places where the idle state index begins to 1. I
think it should be 0 for ARM.



Yes for LPI, it does start from 0.




--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

[PATCH v2] ARM: at91: Document new TCB bindings

[Alexandre Belloni]

The current binding for the TCB is not flexible enough for some use cases
and prevents proper utilization of all the channels.

Cc: Daniel Lezcano 
Cc: Thierry Reding 
Cc: linux-...@vger.kernel.org
Cc: Rob Herring 
Cc: devicet...@vger.kernel.org
Signed-off-by: Alexandre Belloni 
---

Changes in v2:
 - added slow_clk in the examples
 - removed linuxisms

Rob, does that fit what you wanted? I prefer getting your ack on that one before
resending a 48 patches series.


 .../devicetree/bindings/arm/atmel-at91.txt | 32 ---
 .../devicetree/bindings/mfd/atmel-tcb.txt  | 62 ++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt  | 12 +++--
 3 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt 
b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index e1f5ad855f14..3dc758403d03 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -60,38 +60,6 @@ System Timer (ST) required properties:
 Its subnodes can be:
 - watchdog: compatible should be "atmel,at91rm9200-wdt"
 
-TC/TCLIB Timer required properties:
-- compatible: Should be "atmel,-tcb".
-   can be "at91rm9200" or "at91sam9x5"
-- reg: Should contain registers location and length
-- interrupts: Should contain all interrupts for the TC block
-  Note that you can specify several interrupt cells if the TC
-  block has one interrupt per channel.
-- clock-names: tuple listing input clock names.
-   Required elements: "t0_clk", "slow_clk"
-   Optional elements: "t1_clk", "t2_clk"
-- clocks: phandles to input clocks.
-
-Examples:
-
-One interrupt per TC block:
-   tcb0: timer@fff7c000 {
-   compatible = "atmel,at91rm9200-tcb";
-   reg = <0xfff7c000 0x100>;
-   interrupts = <18 4>;
-   clocks = <_clk>;
-   clock-names = "t0_clk";
-   };
-
-One interrupt per TC channel in a TC block:
-   tcb1: timer@fffdc000 {
-   compatible = "atmel,at91rm9200-tcb";
-   reg = <0xfffdc000 0x100>;
-   interrupts = <26 4 27 4 28 4>;
-   clocks = <_clk>;
-   clock-names = "t0_clk";
-   };
-
 RSTC Reset Controller required properties:
 - compatible: Should be "atmel,-rstc".
can be "at91sam9260" or "at91sam9g45" or "sama5d3"
diff --git a/Documentation/devicetree/bindings/mfd/atmel-tcb.txt 
b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
new file mode 100644
index ..71c8d83c01a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
@@ -0,0 +1,62 @@
+* Device tree bindings for Atmel Timer Counter Blocks
+- compatible: Should be "atmel,-tcb", "simple-mfd", "syscon".
+   can be "at91rm9200" or "at91sam9x5"
+- reg: Should contain registers location and length
+- #address-cells: has to be 1
+- #size-cells: has to be 0
+- interrupts: Should contain all interrupts for the TC block
+  Note that you can specify several interrupt cells if the TC
+  block has one interrupt per channel.
+- clock-names: tuple listing input clock names.
+   Required elements: "t0_clk", "slow_clk"
+   Optional elements: "t1_clk", "t2_clk"
+- clocks: phandles to input clocks.
+
+The TCB can expose multiple subdevices:
+ * a clocksource and clockevent device
+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+ counter width is 16 bits (at91rm9200-tcb), two consecutive
+ channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"
+   - reg: Should contain the TCB channel to be used
+
+ * a PWM chip: see ../pwm/atmel-tcb-pwm.txt
+
+Examples:
+
+One interrupt per TC block:
+   tcb0: timer@fff7c000 {
+   compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0xfff7c000 0x100>;
+   interrupts = <18 4>;
+   clocks = <_clk>, <>;
+   clock-names = "t0_clk", "slow_clk";
+
+   timer@0 {
+   compatible = "atmel,tcb-free-running-timer";
+   reg = <0>, <1>;
+   };
+
+   timer@2 {
+   compatible = "atmel,tcb-programmable-timer";
+   reg = <2>;
+   };
+   };
+
+One interrupt per TC channel in a TC block:
+   tcb1: timer@fffdc000 {
+   compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0xfffdc000 0x100>;
+   interrupts = <26 4>, <27 4>, <28 4>;
+   clocks = <_clk>, <>;
+   clock-names = "t0_clk", "slow_clk";
+   

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Benjamin Tissoires]

On Jun 24 2016 or thereabouts, Dmitry Torokhov wrote:
> On Fri, Jun 24, 2016 at 09:19:32AM +0200, Benjamin Tissoires wrote:
> > On Jun 23 2016 or thereabouts, Dmitry Torokhov wrote:
> > > On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> > > > +
> > > > +static struct i2c_driver rmi_smb_driver = {
> > > > +   .driver = {
> > > > +   .owner  = THIS_MODULE,
> > > > +   .name   = "rmi4_smbus",
> > > > +   .pm = _smb_pm,
> > > > +   .resume = rmi_smb_resume,
> > > 
> > > Why rmi_smb_resume is not part of rmi_smb_pm?
> > > 
> > 
> > This is because rmi_smbus device both have a PS/2 interface and a SMBus
> > one. I'll have to check again now that I have a slightly different way
> > of binding smbus devices in my tree, but the issue was:
> > - having resume part of pm means it will get caught by PM directly
> > - the PS/2 node gets also resumed by PM
> > - calling PS/2 commands during resume switches the devices back into
> >   PS/2 and stops the SMBus communication.
> > 
> > So it's easier to wait only for the PS/2 PM resume call which will call
> > the SMBus resume function when the device is in a proper state.
> > 
> > I'll send out the updated patch with your comments next week hopefully.
> 
> Hmm, I think you will have to walk me through resume process. How do we
> tie in PS/2 and I2C on these devices abd have PS/2 code call into this
> driver?
> 

Sure.

For starters, here is my latest WIP (I need to rework on the series for
commit messages and probably squash some patches):
https://github.com/bentiss/linux/commits/synaptics-rmi4-smbus-v4.7-rc4%2B

Then, let me explain the problems we have with those touchpads and the
resume process.

The touchpads are both connected to PS/2 and SMBus as you know. However,
there is no SMBus enumeration entry anywhere in the system. Luckily,
the PS/2 chip is aware of the other bus, and can be polled to
request whether or not we are confronted to a RMI4 over SMBus device
(see 
https://github.com/bentiss/linux/commit/a3e67de764656201522962028bc783fc4b921de3
 )

Of course, to make those touchpads robust with reboots and allow legacy
drivers (PS/2) to use them, the firmware tend to switch back to PS/2 as
soon as you issue a PS/2 reset command or if you send some other PS/2
commands. The problem we have is that if you send some various PS/2
commands to the touchpad, it disconnects from the SMBus entirely (it
took me one year to understand why the device was not showing up on
I2C).

The last interesting fact for those touchpad is that you need to enable
SMBus communication by issuing a SMB_PROTOCOL_VERSION_ADDRESS command.
If you do not issue this after a PS/2 reset, the device is muted over
SMBus.

So, to be sure we can query the touchpad from PS/2 and to control the
PS/2 commands and the resume, I have in the series above a separate PS/2
driver for this touchpad. The regular psmouse driver probes the PS/2
chip, but aborts seeing the SMBus capability. Then rmi_ps2 takes over
and binds to the PS/2 chip to fill in the platform data required by
rmi_smbus 
(https://github.com/bentiss/linux/commit/3ceb7c80ecee17a86b8ae8476211c7498cc823d2)

If we simply unbind the PS/2 node and let it dangling, the serio driver
issues a reconnect after resume on both the PS/2 chip of the touchpad,
and the PS/2 pass-through node of the trackstick.

But if the PS/2 chip of the touchpad is left dangling, the resume
process will first call a reconnect on the pass-through, then on
the touchpad through the enumeration of the serio bus, which will reset
the touchpad and messed up the pass-through node and the rmi-smbus
driver.
So keeping around a reference to the PS/2 node allows to set this node
as a parent of the pass-through trackstick node and guarantees that the
touchpad will be resumed before the trackstick.

One final thing. I tried not having rmi_ps2 calling the .resume callback
of rmi_smbus and keeping rmi_smbus as a PM. But the issue is that the
serio driver sends the reset command in a workqueue as it can takes some
time:

- serio gets resume called -> prepare the worker for the
  resume/reconnect process
- rmi_smbus gets resumed -> OK
- the worker kicks in, reset the PS/2 lines, and shuts down the
  rmi_smbus device


I also tried one thing where I did not bind the PS/2 touchpad at all
(and having some kind of platform driver to bind rmi_smbus). And of
course, grub initializes the touchpad, so it disconnects on I2C and you
can't bind it on SMBus, ever :)


I think that's all I know on these touchpads and this is all the mess I can
present. If you have a better option, I'm all ears as this is not clean,
but I can't figure out a better way.

Cheers,
Benjamin

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Benjamin Tissoires]

On Jun 24 2016 or thereabouts, Dmitry Torokhov wrote:
> On Fri, Jun 24, 2016 at 09:19:32AM +0200, Benjamin Tissoires wrote:
> > On Jun 23 2016 or thereabouts, Dmitry Torokhov wrote:
> > > On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> > > > +
> > > > +static struct i2c_driver rmi_smb_driver = {
> > > > +   .driver = {
> > > > +   .owner  = THIS_MODULE,
> > > > +   .name   = "rmi4_smbus",
> > > > +   .pm = _smb_pm,
> > > > +   .resume = rmi_smb_resume,
> > > 
> > > Why rmi_smb_resume is not part of rmi_smb_pm?
> > > 
> > 
> > This is because rmi_smbus device both have a PS/2 interface and a SMBus
> > one. I'll have to check again now that I have a slightly different way
> > of binding smbus devices in my tree, but the issue was:
> > - having resume part of pm means it will get caught by PM directly
> > - the PS/2 node gets also resumed by PM
> > - calling PS/2 commands during resume switches the devices back into
> >   PS/2 and stops the SMBus communication.
> > 
> > So it's easier to wait only for the PS/2 PM resume call which will call
> > the SMBus resume function when the device is in a proper state.
> > 
> > I'll send out the updated patch with your comments next week hopefully.
> 
> Hmm, I think you will have to walk me through resume process. How do we
> tie in PS/2 and I2C on these devices abd have PS/2 code call into this
> driver?
> 

Sure.

For starters, here is my latest WIP (I need to rework on the series for
commit messages and probably squash some patches):
https://github.com/bentiss/linux/commits/synaptics-rmi4-smbus-v4.7-rc4%2B

Then, let me explain the problems we have with those touchpads and the
resume process.

The touchpads are both connected to PS/2 and SMBus as you know. However,
there is no SMBus enumeration entry anywhere in the system. Luckily,
the PS/2 chip is aware of the other bus, and can be polled to
request whether or not we are confronted to a RMI4 over SMBus device
(see 
https://github.com/bentiss/linux/commit/a3e67de764656201522962028bc783fc4b921de3
 )

Of course, to make those touchpads robust with reboots and allow legacy
drivers (PS/2) to use them, the firmware tend to switch back to PS/2 as
soon as you issue a PS/2 reset command or if you send some other PS/2
commands. The problem we have is that if you send some various PS/2
commands to the touchpad, it disconnects from the SMBus entirely (it
took me one year to understand why the device was not showing up on
I2C).

The last interesting fact for those touchpad is that you need to enable
SMBus communication by issuing a SMB_PROTOCOL_VERSION_ADDRESS command.
If you do not issue this after a PS/2 reset, the device is muted over
SMBus.

So, to be sure we can query the touchpad from PS/2 and to control the
PS/2 commands and the resume, I have in the series above a separate PS/2
driver for this touchpad. The regular psmouse driver probes the PS/2
chip, but aborts seeing the SMBus capability. Then rmi_ps2 takes over
and binds to the PS/2 chip to fill in the platform data required by
rmi_smbus 
(https://github.com/bentiss/linux/commit/3ceb7c80ecee17a86b8ae8476211c7498cc823d2)

If we simply unbind the PS/2 node and let it dangling, the serio driver
issues a reconnect after resume on both the PS/2 chip of the touchpad,
and the PS/2 pass-through node of the trackstick.

But if the PS/2 chip of the touchpad is left dangling, the resume
process will first call a reconnect on the pass-through, then on
the touchpad through the enumeration of the serio bus, which will reset
the touchpad and messed up the pass-through node and the rmi-smbus
driver.
So keeping around a reference to the PS/2 node allows to set this node
as a parent of the pass-through trackstick node and guarantees that the
touchpad will be resumed before the trackstick.

One final thing. I tried not having rmi_ps2 calling the .resume callback
of rmi_smbus and keeping rmi_smbus as a PM. But the issue is that the
serio driver sends the reset command in a workqueue as it can takes some
time:

- serio gets resume called -> prepare the worker for the
  resume/reconnect process
- rmi_smbus gets resumed -> OK
- the worker kicks in, reset the PS/2 lines, and shuts down the
  rmi_smbus device


I also tried one thing where I did not bind the PS/2 touchpad at all
(and having some kind of platform driver to bind rmi_smbus). And of
course, grub initializes the touchpad, so it disconnects on I2C and you
can't bind it on SMBus, ever :)


I think that's all I know on these touchpads and this is all the mess I can
present. If you have a better option, I'm all ears as this is not clean,
but I can't figure out a better way.

Cheers,
Benjamin