[PATCH v3 5/6] ARM: at91: add SAMx7 SoC detection

[Alexandre Belloni]

From: Szemző András 

Add SAME70/V71/S70/V70 chip-ids to SoC detection.

Signed-off-by: Szemző András 
Signed-off-by: Alexandre Belloni 
---
 drivers/soc/atmel/soc.c | 24 
 drivers/soc/atmel/soc.h | 26 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index 4790094b498e..7c588f99dcd1 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -107,6 +107,30 @@ static const struct at91_soc __initconst socs[] = {
AT91_SOC(SAMA5D4_CIDR_MATCH, SAMA5D44_EXID_MATCH,
 "sama5d44", "sama5d4"),
 #endif
+#ifdef CONFIG_SOC_SAMX7
+   AT91_SOC(SAME70Q21_CIDR_MATCH, SAME70Q21_EXID_MATCH,
+"same70q21", "samx7"),
+   AT91_SOC(SAME70Q20_CIDR_MATCH, SAME70Q20_EXID_MATCH,
+"same70q20", "samx7"),
+   AT91_SOC(SAME70Q19_CIDR_MATCH, SAME70Q19_EXID_MATCH,
+"same70q19", "samx7"),
+   AT91_SOC(SAMS70Q21_CIDR_MATCH, SAMS70Q21_EXID_MATCH,
+"sams70q21", "samx7"),
+   AT91_SOC(SAMS70Q20_CIDR_MATCH, SAMS70Q20_EXID_MATCH,
+"sams70q20", "samx7"),
+   AT91_SOC(SAMS70Q19_CIDR_MATCH, SAMS70Q19_EXID_MATCH,
+"sams70q19", "samx7"),
+   AT91_SOC(SAMV71Q21_CIDR_MATCH, SAMV71Q21_EXID_MATCH,
+"samv71q21", "samx7"),
+   AT91_SOC(SAMV71Q20_CIDR_MATCH, SAMV71Q20_EXID_MATCH,
+"samv71q20", "samx7"),
+   AT91_SOC(SAMV71Q19_CIDR_MATCH, SAMV71Q19_EXID_MATCH,
+"samv71q19", "samx7"),
+   AT91_SOC(SAMV70Q20_CIDR_MATCH, SAMV70Q20_EXID_MATCH,
+"samv70q20", "samx7"),
+   AT91_SOC(SAMV70Q19_CIDR_MATCH, SAMV70Q19_EXID_MATCH,
+"samv70q19", "samx7"),
+#endif
{ /* sentinel */ },
 };
 
diff --git a/drivers/soc/atmel/soc.h b/drivers/soc/atmel/soc.h
index 228efded5085..a90bd5b0ef8f 100644
--- a/drivers/soc/atmel/soc.h
+++ b/drivers/soc/atmel/soc.h
@@ -88,4 +88,30 @@ at91_soc_init(const struct at91_soc *socs);
 #define SAMA5D43_EXID_MATCH0x0003
 #define SAMA5D44_EXID_MATCH0x0004
 
+#define SAME70Q21_CIDR_MATCH   0x21020e00
+#define SAME70Q21_EXID_MATCH   0x0002
+#define SAME70Q20_CIDR_MATCH   0x21020c00
+#define SAME70Q20_EXID_MATCH   0x0002
+#define SAME70Q19_CIDR_MATCH   0x210d0a00
+#define SAME70Q19_EXID_MATCH   0x0002
+
+#define SAMS70Q21_CIDR_MATCH   0x21120e00
+#define SAMS70Q21_EXID_MATCH   0x0002
+#define SAMS70Q20_CIDR_MATCH   0x21120c00
+#define SAMS70Q20_EXID_MATCH   0x0002
+#define SAMS70Q19_CIDR_MATCH   0x211d0a00
+#define SAMS70Q19_EXID_MATCH   0x0002
+
+#define SAMV71Q21_CIDR_MATCH   0x21220e00
+#define SAMV71Q21_EXID_MATCH   0x0002
+#define SAMV71Q20_CIDR_MATCH   0x21220c00
+#define SAMV71Q20_EXID_MATCH   0x0002
+#define SAMV71Q19_CIDR_MATCH   0x212d0a00
+#define SAMV71Q19_EXID_MATCH   0x0002
+
+#define SAMV70Q20_CIDR_MATCH   0x21320c00
+#define SAMV70Q20_EXID_MATCH   0x0002
+#define SAMV70Q19_CIDR_MATCH   0x213d0a00
+#define SAMV70Q19_EXID_MATCH   0x0002
+
 #endif /* __AT91_SOC_H */
-- 
2.11.0

[PATCH v3 1/6] ARM: at91: Documentation: add samx7 families

[Alexandre Belloni]

The Atmel sams70, samv70 and samv71 are Cortex-M7 based MCUs that can run
Linux (without MMU).

Signed-off-by: Alexandre Belloni 
---
 Documentation/arm/Atmel/README | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm/Atmel/README b/Documentation/arm/Atmel/README
index 6ca78f818dbf..afb13c15389d 100644
--- a/Documentation/arm/Atmel/README
+++ b/Documentation/arm/Atmel/README
@@ -16,7 +16,7 @@ git branches/tags and email subject always contain this 
"at91" sub-string.
 
 AT91 SoCs
 -
-Documentation and detailled datasheet for each product are available on
+Documentation and detailed datasheet for each product are available on
 the Atmel website: http://www.atmel.com.
 
   Flavors:
@@ -101,6 +101,42 @@ the Atmel website: http://www.atmel.com.
 + Datasheet
   
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
 
+* ARM Cortex-M7 MCUs
+  - sams70 family
+- sams70j19
+- sams70j20
+- sams70j21
+- sams70n19
+- sams70n20
+- sams70n21
+- sams70q19
+- sams70q20
+- sams70q21
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-11242-32-bit-Cortex-M7-Microcontroller-SAM-S70Q-SAM-S70N-SAM-S70J_Datasheet.pdf
+
+  - samv70 family
+- samv70j19
+- samv70j20
+- samv70n19
+- samv70n20
+- samv70q19
+- samv70q20
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-11297-32-bit-Cortex-M7-Microcontroller-SAM-V70Q-SAM-V70N-SAM-V70J_Datasheet.pdf
+
+  - samv71 family
+- samv71j19
+- samv71j20
+- samv71j21
+- samv71n19
+- samv71n20
+- samv71n21
+- samv71q19
+- samv71q20
+- samv71q21
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-44003-32-bit-Cortex-M7-Microcontroller-SAM-V71Q-SAM-V71N-SAM-V71J_Datasheet.pdf
 
 Linux kernel information
 
-- 
2.11.0

[PATCH v3 5/6] ARM: at91: add SAMx7 SoC detection

[Alexandre Belloni]

From: Szemző András 

Add SAME70/V71/S70/V70 chip-ids to SoC detection.

Signed-off-by: Szemző András 
Signed-off-by: Alexandre Belloni 
---
 drivers/soc/atmel/soc.c | 24 
 drivers/soc/atmel/soc.h | 26 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index 4790094b498e..7c588f99dcd1 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -107,6 +107,30 @@ static const struct at91_soc __initconst socs[] = {
AT91_SOC(SAMA5D4_CIDR_MATCH, SAMA5D44_EXID_MATCH,
 "sama5d44", "sama5d4"),
 #endif
+#ifdef CONFIG_SOC_SAMX7
+   AT91_SOC(SAME70Q21_CIDR_MATCH, SAME70Q21_EXID_MATCH,
+"same70q21", "samx7"),
+   AT91_SOC(SAME70Q20_CIDR_MATCH, SAME70Q20_EXID_MATCH,
+"same70q20", "samx7"),
+   AT91_SOC(SAME70Q19_CIDR_MATCH, SAME70Q19_EXID_MATCH,
+"same70q19", "samx7"),
+   AT91_SOC(SAMS70Q21_CIDR_MATCH, SAMS70Q21_EXID_MATCH,
+"sams70q21", "samx7"),
+   AT91_SOC(SAMS70Q20_CIDR_MATCH, SAMS70Q20_EXID_MATCH,
+"sams70q20", "samx7"),
+   AT91_SOC(SAMS70Q19_CIDR_MATCH, SAMS70Q19_EXID_MATCH,
+"sams70q19", "samx7"),
+   AT91_SOC(SAMV71Q21_CIDR_MATCH, SAMV71Q21_EXID_MATCH,
+"samv71q21", "samx7"),
+   AT91_SOC(SAMV71Q20_CIDR_MATCH, SAMV71Q20_EXID_MATCH,
+"samv71q20", "samx7"),
+   AT91_SOC(SAMV71Q19_CIDR_MATCH, SAMV71Q19_EXID_MATCH,
+"samv71q19", "samx7"),
+   AT91_SOC(SAMV70Q20_CIDR_MATCH, SAMV70Q20_EXID_MATCH,
+"samv70q20", "samx7"),
+   AT91_SOC(SAMV70Q19_CIDR_MATCH, SAMV70Q19_EXID_MATCH,
+"samv70q19", "samx7"),
+#endif
{ /* sentinel */ },
 };
 
diff --git a/drivers/soc/atmel/soc.h b/drivers/soc/atmel/soc.h
index 228efded5085..a90bd5b0ef8f 100644
--- a/drivers/soc/atmel/soc.h
+++ b/drivers/soc/atmel/soc.h
@@ -88,4 +88,30 @@ at91_soc_init(const struct at91_soc *socs);
 #define SAMA5D43_EXID_MATCH0x0003
 #define SAMA5D44_EXID_MATCH0x0004
 
+#define SAME70Q21_CIDR_MATCH   0x21020e00
+#define SAME70Q21_EXID_MATCH   0x0002
+#define SAME70Q20_CIDR_MATCH   0x21020c00
+#define SAME70Q20_EXID_MATCH   0x0002
+#define SAME70Q19_CIDR_MATCH   0x210d0a00
+#define SAME70Q19_EXID_MATCH   0x0002
+
+#define SAMS70Q21_CIDR_MATCH   0x21120e00
+#define SAMS70Q21_EXID_MATCH   0x0002
+#define SAMS70Q20_CIDR_MATCH   0x21120c00
+#define SAMS70Q20_EXID_MATCH   0x0002
+#define SAMS70Q19_CIDR_MATCH   0x211d0a00
+#define SAMS70Q19_EXID_MATCH   0x0002
+
+#define SAMV71Q21_CIDR_MATCH   0x21220e00
+#define SAMV71Q21_EXID_MATCH   0x0002
+#define SAMV71Q20_CIDR_MATCH   0x21220c00
+#define SAMV71Q20_EXID_MATCH   0x0002
+#define SAMV71Q19_CIDR_MATCH   0x212d0a00
+#define SAMV71Q19_EXID_MATCH   0x0002
+
+#define SAMV70Q20_CIDR_MATCH   0x21320c00
+#define SAMV70Q20_EXID_MATCH   0x0002
+#define SAMV70Q19_CIDR_MATCH   0x213d0a00
+#define SAMV70Q19_EXID_MATCH   0x0002
+
 #endif /* __AT91_SOC_H */
-- 
2.11.0

[PATCH v3 1/6] ARM: at91: Documentation: add samx7 families

[Alexandre Belloni]

The Atmel sams70, samv70 and samv71 are Cortex-M7 based MCUs that can run
Linux (without MMU).

Signed-off-by: Alexandre Belloni 
---
 Documentation/arm/Atmel/README | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm/Atmel/README b/Documentation/arm/Atmel/README
index 6ca78f818dbf..afb13c15389d 100644
--- a/Documentation/arm/Atmel/README
+++ b/Documentation/arm/Atmel/README
@@ -16,7 +16,7 @@ git branches/tags and email subject always contain this 
"at91" sub-string.
 
 AT91 SoCs
 -
-Documentation and detailled datasheet for each product are available on
+Documentation and detailed datasheet for each product are available on
 the Atmel website: http://www.atmel.com.
 
   Flavors:
@@ -101,6 +101,42 @@ the Atmel website: http://www.atmel.com.
 + Datasheet
   
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
 
+* ARM Cortex-M7 MCUs
+  - sams70 family
+- sams70j19
+- sams70j20
+- sams70j21
+- sams70n19
+- sams70n20
+- sams70n21
+- sams70q19
+- sams70q20
+- sams70q21
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-11242-32-bit-Cortex-M7-Microcontroller-SAM-S70Q-SAM-S70N-SAM-S70J_Datasheet.pdf
+
+  - samv70 family
+- samv70j19
+- samv70j20
+- samv70n19
+- samv70n20
+- samv70q19
+- samv70q20
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-11297-32-bit-Cortex-M7-Microcontroller-SAM-V70Q-SAM-V70N-SAM-V70J_Datasheet.pdf
+
+  - samv71 family
+- samv71j19
+- samv71j20
+- samv71j21
+- samv71n19
+- samv71n20
+- samv71n21
+- samv71q19
+- samv71q20
+- samv71q21
++ Datasheet
+  
http://www.atmel.com/Images/Atmel-44003-32-bit-Cortex-M7-Microcontroller-SAM-V71Q-SAM-V71N-SAM-V71J_Datasheet.pdf
 
 Linux kernel information
 
-- 
2.11.0

Re: [RFC PATCH] vmalloc: show more detail info in vmallocinfo for clarify

[Tim Chen]

On 05/19/2017 11:47 PM, Yisheng Xie wrote:
> When ioremap a 67112960 bytes vm_area with the vmallocinfo:
>  [..]
>  0xec79b000-0xec7fa000  389120 ftl_add_mtd+0x4d0/0x754 pages=94 vmalloc
>  0xec80-0xecbe1000 4067328 kbox_proc_mem_write+0x104/0x1c4 phys=8b52 
> ioremap
> 
> we get result:
>  0xf100-0xf5001000 67112960 devm_ioremap+0x38/0x7c phys=4000 ioremap
> 
> For the align for ioremap must be less than '1 << IOREMAP_MAX_ORDER':
>   if (flags & VM_IOREMAP)
>   align = 1ul << clamp_t(int, get_count_order_long(size),
>   PAGE_SHIFT, IOREMAP_MAX_ORDER);
> 
> So it makes idiot like me a litter puzzle why jump the vm_area from
> 0xec80-0xecbe1000 to 0xf100-0xf5001000, and leave
> 0xed00-0xf100 as a big hole.
> 
> This is to show all of vm_area, including which is freeing but still in
> vmap_area_list, to make it more clear about why we will get
> 0xf100-0xf5001000 int the above case. And we will get the
> vmallocinfo like:
>  [..]
>  0xec79b000-0xec7fa000  389120 ftl_add_mtd+0x4d0/0x754 pages=94 vmalloc
>  0xec80-0xecbe1000 4067328 kbox_proc_mem_write+0x104/0x1c4 phys=8b52 
> ioremap
>  [..]
>  0xece7c000-0xece7e0008192 freeing vm_area
>  0xece7e000-0xece83000   20480 vm_map_ram
>  0xf0099000-0xf00aa000   69632 vm_map_ram
>  0xf100-0xf5001000 67112960 devm_ioremap+0x38/0x7c phys=4000 ioremap
> after apply this patch.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  mm/vmalloc.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index b52aeed..dbb24fc 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -314,6 +314,7 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
>  
>  /*** Global kva allocator ***/
>  
> +#define VM_LAZY_FREE 0x02
>  #define VM_VM_AREA   0x04
>  
>  static DEFINE_SPINLOCK(vmap_area_lock);
> @@ -1486,6 +1487,7 @@ struct vm_struct *remove_vm_area(const void *addr)
>   spin_lock(_area_lock);
>   va->vm = NULL;
>   va->flags &= ~VM_VM_AREA;
> + va->flags |= VM_LAZY_FREE;
>   spin_unlock(_area_lock);
>  
>   vmap_debug_free_range(va->va_start, va->va_end);
> @@ -2684,8 +2686,14 @@ static int s_show(struct seq_file *m, void *p)
>* s_show can encounter race with remove_vm_area, !VM_VM_AREA on
>* behalf of vmap area is being tear down or vm_map_ram allocation.
>*/
> - if (!(va->flags & VM_VM_AREA))
> + if (!(va->flags & VM_VM_AREA)) {
> + seq_printf(m, "0x%pK-0x%pK %7ld %s\n",
> + (void *)va->va_start, (void *)va->va_end,
> + va->va_end - va->va_start,
> + va->flags & VM_LAZY_FREE ? "freeing vm_area" : 
> "vm_map_ram");

Will be clearer to say "unpurged vm_area" instead of "freeing vm_area".

Thanks.

Tim

Re: [RFC PATCH] vmalloc: show more detail info in vmallocinfo for clarify

[Tim Chen]

On 05/19/2017 11:47 PM, Yisheng Xie wrote:
> When ioremap a 67112960 bytes vm_area with the vmallocinfo:
>  [..]
>  0xec79b000-0xec7fa000  389120 ftl_add_mtd+0x4d0/0x754 pages=94 vmalloc
>  0xec80-0xecbe1000 4067328 kbox_proc_mem_write+0x104/0x1c4 phys=8b52 
> ioremap
> 
> we get result:
>  0xf100-0xf5001000 67112960 devm_ioremap+0x38/0x7c phys=4000 ioremap
> 
> For the align for ioremap must be less than '1 << IOREMAP_MAX_ORDER':
>   if (flags & VM_IOREMAP)
>   align = 1ul << clamp_t(int, get_count_order_long(size),
>   PAGE_SHIFT, IOREMAP_MAX_ORDER);
> 
> So it makes idiot like me a litter puzzle why jump the vm_area from
> 0xec80-0xecbe1000 to 0xf100-0xf5001000, and leave
> 0xed00-0xf100 as a big hole.
> 
> This is to show all of vm_area, including which is freeing but still in
> vmap_area_list, to make it more clear about why we will get
> 0xf100-0xf5001000 int the above case. And we will get the
> vmallocinfo like:
>  [..]
>  0xec79b000-0xec7fa000  389120 ftl_add_mtd+0x4d0/0x754 pages=94 vmalloc
>  0xec80-0xecbe1000 4067328 kbox_proc_mem_write+0x104/0x1c4 phys=8b52 
> ioremap
>  [..]
>  0xece7c000-0xece7e0008192 freeing vm_area
>  0xece7e000-0xece83000   20480 vm_map_ram
>  0xf0099000-0xf00aa000   69632 vm_map_ram
>  0xf100-0xf5001000 67112960 devm_ioremap+0x38/0x7c phys=4000 ioremap
> after apply this patch.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  mm/vmalloc.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index b52aeed..dbb24fc 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -314,6 +314,7 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
>  
>  /*** Global kva allocator ***/
>  
> +#define VM_LAZY_FREE 0x02
>  #define VM_VM_AREA   0x04
>  
>  static DEFINE_SPINLOCK(vmap_area_lock);
> @@ -1486,6 +1487,7 @@ struct vm_struct *remove_vm_area(const void *addr)
>   spin_lock(_area_lock);
>   va->vm = NULL;
>   va->flags &= ~VM_VM_AREA;
> + va->flags |= VM_LAZY_FREE;
>   spin_unlock(_area_lock);
>  
>   vmap_debug_free_range(va->va_start, va->va_end);
> @@ -2684,8 +2686,14 @@ static int s_show(struct seq_file *m, void *p)
>* s_show can encounter race with remove_vm_area, !VM_VM_AREA on
>* behalf of vmap area is being tear down or vm_map_ram allocation.
>*/
> - if (!(va->flags & VM_VM_AREA))
> + if (!(va->flags & VM_VM_AREA)) {
> + seq_printf(m, "0x%pK-0x%pK %7ld %s\n",
> + (void *)va->va_start, (void *)va->va_end,
> + va->va_end - va->va_start,
> + va->flags & VM_LAZY_FREE ? "freeing vm_area" : 
> "vm_map_ram");

Will be clearer to say "unpurged vm_area" instead of "freeing vm_area".

Thanks.

Tim

Re: signals: Bug or manpage inconsistency?

[Eric W. Biederman]

Thomas Gleixner  writes:

> On Tue, 30 May 2017, Linus Torvalds wrote:
>> On Tue, May 30, 2017 at 10:04 AM, Oleg Nesterov  wrote:
>> > Obviously this is a user-visible change and it can break something. Say, an
>> > application does sigwaitinfo(SIGCHLD) and SIGCHLD is ignored (SIG_IGN), 
>> > this
>> > will no longer work.
>> 
>> That's an interesting special case. Yes, SIG_IGN actually has magical
>> properties wrt SIGCHLD. It basically means the opposite of ignoring
>> it, it's an "implicit signal handler".  So I could imagine people
>> using SIG_IGN to avoid the signal handler, but then block SIG_CHLD and
>> using sigwait() for it.
>> 
>> That sounds nonportable as hell, but I could imagine people doing it
>> because it happens to work.
>
> Just that it does not work. See do_notify_parent()
>
>   if (!tsk->ptrace && sig == SIGCHLD &&
>   (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
>(psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
>   /*
>* We are exiting and our parent doesn't care.  POSIX.1
>* defines special semantics for setting SIGCHLD to SIG_IGN
>* or setting the SA_NOCLDWAIT flag: we should be reaped
>* automatically and not left for our parent's wait4 call.
>* Rather than having the parent do it as a magic kind of
>* signal handler, we just set this to tell do_exit that we
>* can be cleaned up without becoming a zombie.  Note that
>* we still call __wake_up_parent in this case, because a
>* blocked sys_wait4 might now return -ECHILD.
>*
>* Whether we send SIGCHLD or not for SA_NOCLDWAIT
>* is implementation-defined: we do (if you don't want
>* it, just use SIG_IGN instead).
>*/
>   autoreap = true;
>   if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
>   sig = 0;
>   }
> if (valid_signal(sig) && sig)
> __group_send_sig_info(sig, , tsk->parent);
>
> So if the oarent has SIG_IGN we do not send a signal at all. So it's not a
> really interesting special case and the magic properties are not that magic
> either. Test case below. The parent waits forever.

Which would suggests that to be consistent we should ignore
blocks for other signals on send when the signal handler is SIG_IGN.

Hmm.

For blocked signals because there is only one siginfo ever allocated
as I read it the code naturally blocks the signal until it is
dequeued and rearmed.

I suspect what you want to do is a little more in the magic
dequeue_signal for timers and look if the signal handler
is SIG_IGN.  I think the clean solution would be to
treat timers whose signal handler is SIG_IGN as blocked
signals and simply not dequeue them.

If they are not dequeued they won't reschedule and won't restart.
Then when the signal handler finally changes you immediately get
one pending signal and then the timers fire normally.

That gets tricky though because the signal numbers are not dedicated
to posix timers.

It might instead require noting that the handler is SIG_IGN when
dequeued and simply disabled the timer.  With an enable that kicks
in when someone calls sigaction and changes the handler.

Eric

Re: signals: Bug or manpage inconsistency?

[Eric W. Biederman]

Thomas Gleixner  writes:

> On Tue, 30 May 2017, Linus Torvalds wrote:
>> On Tue, May 30, 2017 at 10:04 AM, Oleg Nesterov  wrote:
>> > Obviously this is a user-visible change and it can break something. Say, an
>> > application does sigwaitinfo(SIGCHLD) and SIGCHLD is ignored (SIG_IGN), 
>> > this
>> > will no longer work.
>> 
>> That's an interesting special case. Yes, SIG_IGN actually has magical
>> properties wrt SIGCHLD. It basically means the opposite of ignoring
>> it, it's an "implicit signal handler".  So I could imagine people
>> using SIG_IGN to avoid the signal handler, but then block SIG_CHLD and
>> using sigwait() for it.
>> 
>> That sounds nonportable as hell, but I could imagine people doing it
>> because it happens to work.
>
> Just that it does not work. See do_notify_parent()
>
>   if (!tsk->ptrace && sig == SIGCHLD &&
>   (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
>(psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
>   /*
>* We are exiting and our parent doesn't care.  POSIX.1
>* defines special semantics for setting SIGCHLD to SIG_IGN
>* or setting the SA_NOCLDWAIT flag: we should be reaped
>* automatically and not left for our parent's wait4 call.
>* Rather than having the parent do it as a magic kind of
>* signal handler, we just set this to tell do_exit that we
>* can be cleaned up without becoming a zombie.  Note that
>* we still call __wake_up_parent in this case, because a
>* blocked sys_wait4 might now return -ECHILD.
>*
>* Whether we send SIGCHLD or not for SA_NOCLDWAIT
>* is implementation-defined: we do (if you don't want
>* it, just use SIG_IGN instead).
>*/
>   autoreap = true;
>   if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
>   sig = 0;
>   }
> if (valid_signal(sig) && sig)
> __group_send_sig_info(sig, , tsk->parent);
>
> So if the oarent has SIG_IGN we do not send a signal at all. So it's not a
> really interesting special case and the magic properties are not that magic
> either. Test case below. The parent waits forever.

Which would suggests that to be consistent we should ignore
blocks for other signals on send when the signal handler is SIG_IGN.

Hmm.

For blocked signals because there is only one siginfo ever allocated
as I read it the code naturally blocks the signal until it is
dequeued and rearmed.

I suspect what you want to do is a little more in the magic
dequeue_signal for timers and look if the signal handler
is SIG_IGN.  I think the clean solution would be to
treat timers whose signal handler is SIG_IGN as blocked
signals and simply not dequeue them.

If they are not dequeued they won't reschedule and won't restart.
Then when the signal handler finally changes you immediately get
one pending signal and then the timers fire normally.

That gets tricky though because the signal numbers are not dedicated
to posix timers.

It might instead require noting that the handler is SIG_IGN when
dequeued and simply disabled the timer.  With an enable that kicks
in when someone calls sigaction and changes the handler.

Eric

Re: [tip:x86/urgent] x86/PAT: Fix Xorg regression on CPUs that don't support PAT

[Doug Smythies]

Note Before:
I might not have the address list correct, as I have re-created this
e-mail from the web page archive, having found the thread after bisecting the
kernel.

On 2017.05.29 18:50:57 -0400 (EDT) Mikulas Patocka wrote:
> On Sun, 28 May 2017, Andy Lutomirski wrote:
>> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>>> Hi,
>>>
>>> this patch breaks the boot of my kernel. The last message is "Booting
>>> the kernel.".

It breaks my kernel boot also, and I know of at least two others with
the same, or similar, problem as of kernel 4.12-rc3.

>>> My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>>> Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>>> microcode of the E5450 and recognizes the CPU.

I do not think my test server setup is unusual.
I use Ubuntu 16.04.2 server edition as my distro, and
steal Ubuntu kernel configurations for compiling.
My processor is an older model i7
(Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)

> Hi
>
> Please do the following three tests and test if the kernel boots.
>
> 1. use the PAT patch and revert the change to the function pat_enabled()
> - i.e. change it to the original:
> bool pat_enabled(void)
> {
>   return !!__pat_enabled;
> }

Test 1 result: fail

>
> 2. use the PAT patch and revert the change to the function pat_ap_init
> - i.e. change it to the original:
> static void pat_ap_init(u64 pat)
> {
>   if (!boot_cpu_has(X86_FEATURE_PAT)) {

Test 2 result: pass

> 3. use the full PAT patch and apply the below patch on the top of it.
>

Test 3 result: fail

>> I think this patch is bogus.  pat_enabled() sure looks like it's
>> supposed to return true if PAT is *enabled*, and these days PAT is
>> "enabled" even if there's no HW PAT support.  Even if the patch were
>> somehow correct, it should have been split up into two patches, one to
>> change pat_enabled() and one to use this_cpu_has().
>> 
>> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> -stable knows not to backport it, and starting over with the fix.
>> From very brief inspection, the right fix is to make sure that
>> pat_init(), or at least init_cache_modes(), gets called on the
>>
> pat_init() needs to be called with cache disabled - and the cache disable 
> code (functions prepare_set() and post_set()) exists in 
> arch/x86/kernel/cpu/mtrr/generic.c - it may not be compiled if CONFIG_MTRR 
> is not set.
>
> Though, it is possible to call init_cache_modes() - see the patch below. 
> init_cache_modes() does nothing if it is called multiple times.
>
>> affected CPUs.
>> 
>> As a future cleanup, I think that pat_enabled() could be deleted
>> outright and, if needed, replaced by functions like have_memtype_wc()
>> or similar.  (Do we already have helpers like that?)  Toshi, am I
>> right?
>> 
>> --Andy

Re: [tip:x86/urgent] x86/PAT: Fix Xorg regression on CPUs that don't support PAT

[Doug Smythies]

Note Before:
I might not have the address list correct, as I have re-created this
e-mail from the web page archive, having found the thread after bisecting the
kernel.

On 2017.05.29 18:50:57 -0400 (EDT) Mikulas Patocka wrote:
> On Sun, 28 May 2017, Andy Lutomirski wrote:
>> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>>> Hi,
>>>
>>> this patch breaks the boot of my kernel. The last message is "Booting
>>> the kernel.".

It breaks my kernel boot also, and I know of at least two others with
the same, or similar, problem as of kernel 4.12-rc3.

>>> My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>>> Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>>> microcode of the E5450 and recognizes the CPU.

I do not think my test server setup is unusual.
I use Ubuntu 16.04.2 server edition as my distro, and
steal Ubuntu kernel configurations for compiling.
My processor is an older model i7
(Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)

> Hi
>
> Please do the following three tests and test if the kernel boots.
>
> 1. use the PAT patch and revert the change to the function pat_enabled()
> - i.e. change it to the original:
> bool pat_enabled(void)
> {
>   return !!__pat_enabled;
> }

Test 1 result: fail

>
> 2. use the PAT patch and revert the change to the function pat_ap_init
> - i.e. change it to the original:
> static void pat_ap_init(u64 pat)
> {
>   if (!boot_cpu_has(X86_FEATURE_PAT)) {

Test 2 result: pass

> 3. use the full PAT patch and apply the below patch on the top of it.
>

Test 3 result: fail

>> I think this patch is bogus.  pat_enabled() sure looks like it's
>> supposed to return true if PAT is *enabled*, and these days PAT is
>> "enabled" even if there's no HW PAT support.  Even if the patch were
>> somehow correct, it should have been split up into two patches, one to
>> change pat_enabled() and one to use this_cpu_has().
>> 
>> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> -stable knows not to backport it, and starting over with the fix.
>> From very brief inspection, the right fix is to make sure that
>> pat_init(), or at least init_cache_modes(), gets called on the
>>
> pat_init() needs to be called with cache disabled - and the cache disable 
> code (functions prepare_set() and post_set()) exists in 
> arch/x86/kernel/cpu/mtrr/generic.c - it may not be compiled if CONFIG_MTRR 
> is not set.
>
> Though, it is possible to call init_cache_modes() - see the patch below. 
> init_cache_modes() does nothing if it is called multiple times.
>
>> affected CPUs.
>> 
>> As a future cleanup, I think that pat_enabled() could be deleted
>> outright and, if needed, replaced by functions like have_memtype_wc()
>> or similar.  (Do we already have helpers like that?)  Toshi, am I
>> right?
>> 
>> --Andy

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Andrew Lunn]

On Tue, May 30, 2017 at 05:16:27PM -0700, Florian Fainelli wrote:
> On 05/30/2017 05:06 PM, Andrew Lunn wrote:
> >> - past the initial setup, if we start creating bridge devices and so on,
> >> we have no way to tell: group Ports 0-3 together and send traffic to CPU
> >> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
> >> DSA-only problem though, because we still have the CPU port(s) as
> >> independent network interfaces.
> > 
> > What is the problem here? Frames come out the master interface, get
> > untagged and passed to the slave interface and go upto the bridge. It
> > should all just work. Same in the reverse direction.
> 
> The problem is really that is you have multiple CPU ports, how do you
> define which one gets all the traffic by default? Ascending order of
> port number? Descending order?

I would probably default to round robin when allocating user ports to
CPU ports. That probably gives you the best default.

> I actually tend to think that most use cases our there are in the order
> of dedicating one CPU port to one corresponding switch port (user
> facing, or internal) in order to provided guaranteed bandwidth for that
> port.

Which is generally a waste of bandwidth. Best case, i get 40Mbps
Internet access. Meaning 960Mbps of a dedicated cpu port would be
wasted.

>  But as an user, I want to choose how the grouping is going to
> work, and right now, I cannot, unless this is hardcoded in Device Tree,
> which sounds both wrong and inadequate.

So how about round-robin default, and then devlink to move a user port
to a specific cpu port?

We also need to watch out for asymmetry. I think newer marvell chips
don't support egress to multiple CPU ports. Ingress to the switch i
think is unlimited. The older chips are more flexible in this
respect. So we need some degree of flexibility here.
 
Andrew

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Andrew Lunn]

On Tue, May 30, 2017 at 05:16:27PM -0700, Florian Fainelli wrote:
> On 05/30/2017 05:06 PM, Andrew Lunn wrote:
> >> - past the initial setup, if we start creating bridge devices and so on,
> >> we have no way to tell: group Ports 0-3 together and send traffic to CPU
> >> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
> >> DSA-only problem though, because we still have the CPU port(s) as
> >> independent network interfaces.
> > 
> > What is the problem here? Frames come out the master interface, get
> > untagged and passed to the slave interface and go upto the bridge. It
> > should all just work. Same in the reverse direction.
> 
> The problem is really that is you have multiple CPU ports, how do you
> define which one gets all the traffic by default? Ascending order of
> port number? Descending order?

I would probably default to round robin when allocating user ports to
CPU ports. That probably gives you the best default.

> I actually tend to think that most use cases our there are in the order
> of dedicating one CPU port to one corresponding switch port (user
> facing, or internal) in order to provided guaranteed bandwidth for that
> port.

Which is generally a waste of bandwidth. Best case, i get 40Mbps
Internet access. Meaning 960Mbps of a dedicated cpu port would be
wasted.

>  But as an user, I want to choose how the grouping is going to
> work, and right now, I cannot, unless this is hardcoded in Device Tree,
> which sounds both wrong and inadequate.

So how about round-robin default, and then devlink to move a user port
to a specific cpu port?

We also need to watch out for asymmetry. I think newer marvell chips
don't support egress to multiple CPU ports. Ingress to the switch i
think is unlimited. The older chips are more flexible in this
respect. So we need some degree of flexibility here.
 
Andrew

Re: [net-bluetooth] question about potential null pointer dereference

[Marcel Holtmann]

Hi Gustavo,

> While looking into Coverity ID 1357456 I ran into the following piece of code 
> at net/bluetooth/smp.c:166
> 
> 166/* The following functions map to the LE SC SMP crypto functions
> 167 * AES-CMAC, f4, f5, f6, g2 and h6.
> 168 */
> 169
> 170static int aes_cmac(struct crypto_shash *tfm, const u8 k[16], const u8 *m,
> 171size_t len, u8 mac[16])
> 172{
> 173uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
> 174SHASH_DESC_ON_STACK(desc, tfm);
> 175int err;
> 176
> 177if (len > CMAC_MSG_MAX)
> 178return -EFBIG;
> 179
> 180if (!tfm) {
> 181BT_ERR("tfm %p", tfm);
> 182return -EINVAL;
> 183}
> 184
> 185desc->tfm = tfm;
> 186desc->flags = 0;
> 187
> 188/* Swap key and message from LSB to MSB */
> 189swap_buf(k, tmp, 16);
> 190swap_buf(m, msg_msb, len);
> 191
> 192SMP_DBG("msg (len %zu) %*phN", len, (int) len, m);
> 193SMP_DBG("key %16phN", k);
> 194
> 195err = crypto_shash_setkey(tfm, tmp, 16);
> 196if (err) {
> 197BT_ERR("cipher setkey failed: %d", err);
> 198return err;
> 199}
> 200
> 201err = crypto_shash_digest(desc, msg_msb, len, mac_msb);
> 202shash_desc_zero(desc);
> 203if (err) {
> 204BT_ERR("Hash computation error %d", err);
> 205return err;
> 206}
> 207
> 208swap_buf(mac_msb, mac, 16);
> 209
> 210SMP_DBG("mac %16phN", mac);
> 211
> 212return 0;
> 213}
> 
> The issue here is that line 180 implies that pointer tfm might be NULL. If 
> this is the case, there is a potential NULL pointer dereference at line 174 
> once pointer tfm is indirectly dereferenced inside macro 
> SHASH_DESC_ON_STACK().
> 
> My question is if there is any chance that pointer tfm maybe be NULL when 
> calling macro SHASH_DESC_ON_STACK()?

I think the part you are after is this:

smp->tfm_cmac = crypto_alloc_shash("cmac(aes)", 0, 0);  
 
if (IS_ERR(smp->tfm_cmac)) {
 
BT_ERR("Unable to create CMAC crypto context"); 
 
crypto_free_cipher(smp->tfm_aes);   
 
kzfree(smp);
 
return NULL;
 
} 

So the tfm_cmac is part of the smp structure. However if there is no cipher, we 
destroy the smp structure and essentially run without SMP support. So it can 
not really be called anyway.

Maybe commenting this might be a good idea.

Regards

Marcel

Re: [net-bluetooth] question about potential null pointer dereference

[Marcel Holtmann]

Hi Gustavo,

> While looking into Coverity ID 1357456 I ran into the following piece of code 
> at net/bluetooth/smp.c:166
> 
> 166/* The following functions map to the LE SC SMP crypto functions
> 167 * AES-CMAC, f4, f5, f6, g2 and h6.
> 168 */
> 169
> 170static int aes_cmac(struct crypto_shash *tfm, const u8 k[16], const u8 *m,
> 171size_t len, u8 mac[16])
> 172{
> 173uint8_t tmp[16], mac_msb[16], msg_msb[CMAC_MSG_MAX];
> 174SHASH_DESC_ON_STACK(desc, tfm);
> 175int err;
> 176
> 177if (len > CMAC_MSG_MAX)
> 178return -EFBIG;
> 179
> 180if (!tfm) {
> 181BT_ERR("tfm %p", tfm);
> 182return -EINVAL;
> 183}
> 184
> 185desc->tfm = tfm;
> 186desc->flags = 0;
> 187
> 188/* Swap key and message from LSB to MSB */
> 189swap_buf(k, tmp, 16);
> 190swap_buf(m, msg_msb, len);
> 191
> 192SMP_DBG("msg (len %zu) %*phN", len, (int) len, m);
> 193SMP_DBG("key %16phN", k);
> 194
> 195err = crypto_shash_setkey(tfm, tmp, 16);
> 196if (err) {
> 197BT_ERR("cipher setkey failed: %d", err);
> 198return err;
> 199}
> 200
> 201err = crypto_shash_digest(desc, msg_msb, len, mac_msb);
> 202shash_desc_zero(desc);
> 203if (err) {
> 204BT_ERR("Hash computation error %d", err);
> 205return err;
> 206}
> 207
> 208swap_buf(mac_msb, mac, 16);
> 209
> 210SMP_DBG("mac %16phN", mac);
> 211
> 212return 0;
> 213}
> 
> The issue here is that line 180 implies that pointer tfm might be NULL. If 
> this is the case, there is a potential NULL pointer dereference at line 174 
> once pointer tfm is indirectly dereferenced inside macro 
> SHASH_DESC_ON_STACK().
> 
> My question is if there is any chance that pointer tfm maybe be NULL when 
> calling macro SHASH_DESC_ON_STACK()?

I think the part you are after is this:

smp->tfm_cmac = crypto_alloc_shash("cmac(aes)", 0, 0);  
 
if (IS_ERR(smp->tfm_cmac)) {
 
BT_ERR("Unable to create CMAC crypto context"); 
 
crypto_free_cipher(smp->tfm_aes);   
 
kzfree(smp);
 
return NULL;
 
} 

So the tfm_cmac is part of the smp structure. However if there is no cipher, we 
destroy the smp structure and essentially run without SMP support. So it can 
not really be called anyway.

Maybe commenting this might be a good idea.

Regards

Marcel

[rcu:rcu/next 96/97] init/main.o:undefined reference to `rcu_scheduler_starting'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git 
rcu/next
head:   19223c8730d289cd4c65b46250b3e02a6752803c
commit: 9c6175ccfb30f974cd87ac2dbbbd53a2e201a3b9 [96/97] srcu: Move 
rcu_scheduler_starting() from Tiny RCU to Tiny SRCU
config: parisc-allnoconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 9c6175ccfb30f974cd87ac2dbbbd53a2e201a3b9
# save the attached .config to linux build tree
make.cross ARCH=parisc 

All errors (new ones prefixed by >>):

   init/built-in.o: In function `rest_init':
>> init/main.o:(.ref.text+0x8): undefined reference to `rcu_scheduler_starting'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[rcu:rcu/next 96/97] init/main.o:undefined reference to `rcu_scheduler_starting'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git 
rcu/next
head:   19223c8730d289cd4c65b46250b3e02a6752803c
commit: 9c6175ccfb30f974cd87ac2dbbbd53a2e201a3b9 [96/97] srcu: Move 
rcu_scheduler_starting() from Tiny RCU to Tiny SRCU
config: parisc-allnoconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 9c6175ccfb30f974cd87ac2dbbbd53a2e201a3b9
# save the attached .config to linux build tree
make.cross ARCH=parisc 

All errors (new ones prefixed by >>):

   init/built-in.o: In function `rest_init':
>> init/main.o:(.ref.text+0x8): undefined reference to `rcu_scheduler_starting'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[rcu:rcu/next 97/97] arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git 
rcu/next
head:   19223c8730d289cd4c65b46250b3e02a6752803c
commit: 19223c8730d289cd4c65b46250b3e02a6752803c [97/97] module: Fix pr_fmt() 
bug for header use of printk
config: blackfin-allmodconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 19223c8730d289cd4c65b46250b3e02a6752803c
# save the attached .config to linux build tree
make.cross ARCH=blackfin 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
   arch/blackfin/kernel/module.c: In function 'apply_relocate_add':
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/dynamic_debug.h:79:14: note: in definition of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
  .format = (fmt),\
 ^~~
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA'
 DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
 ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/printk.h:285:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^~~
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
>> arch/blackfin/kernel/module.c:20:11: warning: format '%s' expects a matching 
>> 'char *' argument [-Wformat=]
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
  ^
   include/linux/printk.h:285:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^~~
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
   In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/dynamic_debug.h:79:14: note: in definition of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
  .format = (fmt),\
 ^~~
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA'
 DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
 ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
   arch/blackfin/kernel/module.c:186:3: 

[rcu:rcu/next 97/97] arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git 
rcu/next
head:   19223c8730d289cd4c65b46250b3e02a6752803c
commit: 19223c8730d289cd4c65b46250b3e02a6752803c [97/97] module: Fix pr_fmt() 
bug for header use of printk
config: blackfin-allmodconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 19223c8730d289cd4c65b46250b3e02a6752803c
# save the attached .config to linux build tree
make.cross ARCH=blackfin 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
   arch/blackfin/kernel/module.c: In function 'apply_relocate_add':
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/dynamic_debug.h:79:14: note: in definition of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
  .format = (fmt),\
 ^~~
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA'
 DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
 ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
   In file included from include/linux/kernel.h:13:0,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/printk.h:285:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^~~
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
>> arch/blackfin/kernel/module.c:20:11: warning: format '%s' expects a matching 
>> 'char *' argument [-Wformat=]
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
  ^
   include/linux/printk.h:285:21: note: in definition of macro 'pr_fmt'
#define pr_fmt(fmt) fmt
^~~
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:163:2: note: in expansion of macro 'mod_debug'
 mod_debug(mod, "applying relocate section %u to %u\n",
 ^
   In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/module.h:9,
from include/linux/moduleloader.h:5,
from arch/blackfin/kernel/module.c:7:
>> arch/blackfin/kernel/module.c:20:25: error: expected ')' before 'fmt'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
^
   include/linux/dynamic_debug.h:79:14: note: in definition of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA_KEY'
  .format = (fmt),\
 ^~~
   include/linux/dynamic_debug.h:124:2: note: in expansion of macro 
'DEFINE_DYNAMIC_DEBUG_METADATA'
 DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
 ^
   include/linux/printk.h:333:2: note: in expansion of macro 'dynamic_pr_debug'
 dynamic_pr_debug(fmt, ##__VA_ARGS__)
 ^~~~
>> arch/blackfin/kernel/module.c:20:2: note: in expansion of macro 'pr_debug'
 pr_debug("module %s: " fmt, (mod)->name, ##__VA_ARGS__)
 ^~~~
   arch/blackfin/kernel/module.c:186:3: 

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

[Stephen Boyd]

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> Usually *_dev best used for structures that embed a struct device in
> them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
> structure. Use an appropriate name for it.
> 
> Also there are many places in the driver that left shift the bit to
> generate a bit mask. Replace it with the BIT() macro.

That would be a different patch because the subject doesn't even
mention this.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 164 
> +--

Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.

>  1 file changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index df463d4..7f918ea 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -58,10 +58,10 @@
>  
>  /* Channel Status fields */
>  enum pmic_arb_chnl_status {
> - PMIC_ARB_STATUS_DONE= (1 << 0),
> - PMIC_ARB_STATUS_FAILURE = (1 << 1),
> - PMIC_ARB_STATUS_DENIED  = (1 << 2),
> - PMIC_ARB_STATUS_DROPPED = (1 << 3),
> + PMIC_ARB_STATUS_DONE= BIT(0),
> + PMIC_ARB_STATUS_FAILURE = BIT(1),
> + PMIC_ARB_STATUS_DENIED  = BIT(2),
> + PMIC_ARB_STATUS_DROPPED = BIT(3),
>  };
>  
>  /* Command register fields */
> @@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
>  struct pmic_arb_ver_ops;
>  
>  /**
> - * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
> + * spmi_pmic_arb - SPMI PMIC Arbiter object
>   *
>   * @rd_base: on v1 "core", on v2 "observer" register base off DT.
>   * @wr_base: on v1 "core", on v2 "chnls"register base off DT.
> @@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
>   * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping table.
>   *   v2 only.
>   */
> -struct spmi_pmic_arb_dev {
> +struct spmi_pmic_arb {
>   void __iomem*rd_base;
>   void __iomem*wr_base;
>   void __iomem*intr;
> @@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> - int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,

But we leave dev here? I'm losing faith that this patch is
worthwhile.

>   mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> - int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
> u32 *offset);
>   u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
>   int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
> @@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
>   u32 (*irq_clear)(u8 n);
>  };
>  
> -static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->wr_base + offset);
> + writel_relaxed(val, pa->wr_base + offset);

"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?

>  }
>  
> -static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->rd_base + offset);
> + writel_relaxed(val, pa->rd_base + offset);
>  }
>  
>  /**
> @@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
> spmi_pmic_arb_dev *dev,
>   * @reg: register's address
>   * @buf: output parameter, length must be bc + 1
>   */
> -static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
> bc)
> +static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)

In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

>  {
> - u32 data = __raw_readl(dev->rd_base + reg);
> + u32 data = __raw_readl(pa->rd_base + reg);
> +
>   memcpy(buf, , (bc & 3) + 1);
>  }
>  
> @@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
> u8 *buf, u32 reg, u8 bc)
>   * @buf: buffer to write. length must be bc + 1.
>   */
>  static void
> -pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
> +pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
>  {
>   u32 data = 0;
> +
>   memcpy(, buf, (bc & 3) + 1);
> - __raw_writel(data, 

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

[Stephen Boyd]

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> Usually *_dev best used for structures that embed a struct device in
> them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
> structure. Use an appropriate name for it.
> 
> Also there are many places in the driver that left shift the bit to
> generate a bit mask. Replace it with the BIT() macro.

That would be a different patch because the subject doesn't even
mention this.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 164 
> +--

Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.

>  1 file changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index df463d4..7f918ea 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -58,10 +58,10 @@
>  
>  /* Channel Status fields */
>  enum pmic_arb_chnl_status {
> - PMIC_ARB_STATUS_DONE= (1 << 0),
> - PMIC_ARB_STATUS_FAILURE = (1 << 1),
> - PMIC_ARB_STATUS_DENIED  = (1 << 2),
> - PMIC_ARB_STATUS_DROPPED = (1 << 3),
> + PMIC_ARB_STATUS_DONE= BIT(0),
> + PMIC_ARB_STATUS_FAILURE = BIT(1),
> + PMIC_ARB_STATUS_DENIED  = BIT(2),
> + PMIC_ARB_STATUS_DROPPED = BIT(3),
>  };
>  
>  /* Command register fields */
> @@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
>  struct pmic_arb_ver_ops;
>  
>  /**
> - * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
> + * spmi_pmic_arb - SPMI PMIC Arbiter object
>   *
>   * @rd_base: on v1 "core", on v2 "observer" register base off DT.
>   * @wr_base: on v1 "core", on v2 "chnls"register base off DT.
> @@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
>   * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping table.
>   *   v2 only.
>   */
> -struct spmi_pmic_arb_dev {
> +struct spmi_pmic_arb {
>   void __iomem*rd_base;
>   void __iomem*wr_base;
>   void __iomem*intr;
> @@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> - int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,

But we leave dev here? I'm losing faith that this patch is
worthwhile.

>   mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> - int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
> u32 *offset);
>   u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
>   int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
> @@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
>   u32 (*irq_clear)(u8 n);
>  };
>  
> -static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->wr_base + offset);
> + writel_relaxed(val, pa->wr_base + offset);

"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?

>  }
>  
> -static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->rd_base + offset);
> + writel_relaxed(val, pa->rd_base + offset);
>  }
>  
>  /**
> @@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
> spmi_pmic_arb_dev *dev,
>   * @reg: register's address
>   * @buf: output parameter, length must be bc + 1
>   */
> -static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
> bc)
> +static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)

In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

>  {
> - u32 data = __raw_readl(dev->rd_base + reg);
> + u32 data = __raw_readl(pa->rd_base + reg);
> +
>   memcpy(buf, , (bc & 3) + 1);
>  }
>  
> @@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
> u8 *buf, u32 reg, u8 bc)
>   * @buf: buffer to write. length must be bc + 1.
>   */
>  static void
> -pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
> +pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
>  {
>   u32 data = 0;
> +
>   memcpy(, buf, (bc & 3) + 1);
> - __raw_writel(data, dev->wr_base + reg);
> + pmic_arb_base_write(pa, reg, data);

This is an 

[PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.

[Andrew Pinski]

This allows the compiler to optimize the divide by 1000.
And remove the other divide.

On ThunderX, gettimeofday improves by 32%.  On ThunderX 2,
gettimeofday improves by 18%.

Note I noticed a bug in the old implementation of __kernel_clock_getres;
it was checking only the lower 32bits of the pointer; this would work
for most cases but could fail in a few.

Changes from v1:
* Fixed bug in __kernel_clock_getres for checking the pointer argument.
* Fix comments to refer to functions in arm64.


Signed-off-by: Andrew Pinski 
---
 arch/arm64/kernel/vdso/Makefile   |  13 +-
 arch/arm64/kernel/vdso/gettimeofday.S | 329 
 arch/arm64/kernel/vdso/gettimeofday.c | 345 ++
 3 files changed, 351 insertions(+), 336 deletions(-)
 delete mode 100644 arch/arm64/kernel/vdso/gettimeofday.S
 create mode 100644 arch/arm64/kernel/vdso/gettimeofday.c

diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index 62c84f7..55f352f 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -11,10 +11,15 @@ obj-vdso := gettimeofday.o note.o sigreturn.o
 targets := $(obj-vdso) vdso.so vdso.so.dbg
 obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
 
-ccflags-y := -shared -fno-common -fno-builtin
+ccflags-y := -shared -fno-common -fno-builtin -fno-stack-protector
+ccflags-y += -DDISABLE_BRANCH_PROFILING
 ccflags-y += -nostdlib -Wl,-soname=linux-vdso.so.1 \
$(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
 
+# Force -O2 to avoid libgcc dependencies
+CFLAGS_REMOVE_gettimeofday.o = -pg -Os
+CFLAGS_gettimeofday.o = -O2 -mcmodel=tiny
+
 # Disable gcov profiling for VDSO code
 GCOV_PROFILE := n
 
@@ -48,15 +53,9 @@ endef
 include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
 
-# Assembly rules for the .S files
-$(obj-vdso): %.o: %.S FORCE
-   $(call if_changed_dep,vdsoas)
-
 # Actual build commands
 quiet_cmd_vdsold = VDSOL   $@
   cmd_vdsold = $(CC) $(c_flags) -Wl,-n -Wl,-T $^ -o $@
-quiet_cmd_vdsoas = VDSOA   $@
-  cmd_vdsoas = $(CC) $(a_flags) -c -o $@ $<
 
 # Install commands for the unstripped file
 quiet_cmd_vdso_install = INSTALL $@
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S 
b/arch/arm64/kernel/vdso/gettimeofday.S
deleted file mode 100644
index e00b467..000
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ /dev/null
@@ -1,329 +0,0 @@
-/*
- * Userspace implementations of gettimeofday() and friends.
- *
- * Copyright (C) 2012 ARM Limited
- *
- * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see .
- *
- * Author: Will Deacon 
- */
-
-#include 
-#include 
-#include 
-
-#define NSEC_PER_SEC_LO16  0xca00
-#define NSEC_PER_SEC_HI16  0x3b9a
-
-vdso_data  .reqx6
-seqcnt .reqw7
-w_tmp  .reqw8
-x_tmp  .reqx8
-
-/*
- * Conventions for macro arguments:
- * - An argument is write-only if its name starts with "res".
- * - All other arguments are read-only, unless otherwise specified.
- */
-
-   .macro  seqcnt_acquire
-:  ldr seqcnt, [vdso_data, #VDSO_TB_SEQ_COUNT]
-   tbnzseqcnt, #0, b
-   dmb ishld
-   .endm
-
-   .macro  seqcnt_check fail
-   dmb ishld
-   ldr w_tmp, [vdso_data, #VDSO_TB_SEQ_COUNT]
-   cmp w_tmp, seqcnt
-   b.ne\fail
-   .endm
-
-   .macro  syscall_check fail
-   ldr w_tmp, [vdso_data, #VDSO_USE_SYSCALL]
-   cbnzw_tmp, \fail
-   .endm
-
-   .macro get_nsec_per_sec res
-   mov \res, #NSEC_PER_SEC_LO16
-   movk\res, #NSEC_PER_SEC_HI16, lsl #16
-   .endm
-
-   /*
-* Returns the clock delta, in nanoseconds left-shifted by the clock
-* shift.
-*/
-   .macro  get_clock_shifted_nsec res, cycle_last, mult
-   /* Read the virtual counter. */
-   isb
-   mrs x_tmp, cntvct_el0
-   /* Calculate cycle delta and convert to ns. */
-   sub \res, x_tmp, \cycle_last
-   /* We can only guarantee 56 bits of precision. */
-   movnx_tmp, #0xff00, lsl #48
-   and \res, x_tmp, \res
-   mul \res, \res, \mult
-   .endm
-
-   /*
-* Returns in res_{sec,nsec} the REALTIME timespec, based on the
-* "wall time" (xtime) and the clock_mono delta.
-*/
-   .macro  get_ts_realtime res_sec, res_nsec, \
-   

[PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.

[Andrew Pinski]

This allows the compiler to optimize the divide by 1000.
And remove the other divide.

On ThunderX, gettimeofday improves by 32%.  On ThunderX 2,
gettimeofday improves by 18%.

Note I noticed a bug in the old implementation of __kernel_clock_getres;
it was checking only the lower 32bits of the pointer; this would work
for most cases but could fail in a few.

Changes from v1:
* Fixed bug in __kernel_clock_getres for checking the pointer argument.
* Fix comments to refer to functions in arm64.


Signed-off-by: Andrew Pinski 
---
 arch/arm64/kernel/vdso/Makefile   |  13 +-
 arch/arm64/kernel/vdso/gettimeofday.S | 329 
 arch/arm64/kernel/vdso/gettimeofday.c | 345 ++
 3 files changed, 351 insertions(+), 336 deletions(-)
 delete mode 100644 arch/arm64/kernel/vdso/gettimeofday.S
 create mode 100644 arch/arm64/kernel/vdso/gettimeofday.c

diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index 62c84f7..55f352f 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -11,10 +11,15 @@ obj-vdso := gettimeofday.o note.o sigreturn.o
 targets := $(obj-vdso) vdso.so vdso.so.dbg
 obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
 
-ccflags-y := -shared -fno-common -fno-builtin
+ccflags-y := -shared -fno-common -fno-builtin -fno-stack-protector
+ccflags-y += -DDISABLE_BRANCH_PROFILING
 ccflags-y += -nostdlib -Wl,-soname=linux-vdso.so.1 \
$(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
 
+# Force -O2 to avoid libgcc dependencies
+CFLAGS_REMOVE_gettimeofday.o = -pg -Os
+CFLAGS_gettimeofday.o = -O2 -mcmodel=tiny
+
 # Disable gcov profiling for VDSO code
 GCOV_PROFILE := n
 
@@ -48,15 +53,9 @@ endef
 include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
 
-# Assembly rules for the .S files
-$(obj-vdso): %.o: %.S FORCE
-   $(call if_changed_dep,vdsoas)
-
 # Actual build commands
 quiet_cmd_vdsold = VDSOL   $@
   cmd_vdsold = $(CC) $(c_flags) -Wl,-n -Wl,-T $^ -o $@
-quiet_cmd_vdsoas = VDSOA   $@
-  cmd_vdsoas = $(CC) $(a_flags) -c -o $@ $<
 
 # Install commands for the unstripped file
 quiet_cmd_vdso_install = INSTALL $@
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S 
b/arch/arm64/kernel/vdso/gettimeofday.S
deleted file mode 100644
index e00b467..000
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ /dev/null
@@ -1,329 +0,0 @@
-/*
- * Userspace implementations of gettimeofday() and friends.
- *
- * Copyright (C) 2012 ARM Limited
- *
- * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see .
- *
- * Author: Will Deacon 
- */
-
-#include 
-#include 
-#include 
-
-#define NSEC_PER_SEC_LO16  0xca00
-#define NSEC_PER_SEC_HI16  0x3b9a
-
-vdso_data  .reqx6
-seqcnt .reqw7
-w_tmp  .reqw8
-x_tmp  .reqx8
-
-/*
- * Conventions for macro arguments:
- * - An argument is write-only if its name starts with "res".
- * - All other arguments are read-only, unless otherwise specified.
- */
-
-   .macro  seqcnt_acquire
-:  ldr seqcnt, [vdso_data, #VDSO_TB_SEQ_COUNT]
-   tbnzseqcnt, #0, b
-   dmb ishld
-   .endm
-
-   .macro  seqcnt_check fail
-   dmb ishld
-   ldr w_tmp, [vdso_data, #VDSO_TB_SEQ_COUNT]
-   cmp w_tmp, seqcnt
-   b.ne\fail
-   .endm
-
-   .macro  syscall_check fail
-   ldr w_tmp, [vdso_data, #VDSO_USE_SYSCALL]
-   cbnzw_tmp, \fail
-   .endm
-
-   .macro get_nsec_per_sec res
-   mov \res, #NSEC_PER_SEC_LO16
-   movk\res, #NSEC_PER_SEC_HI16, lsl #16
-   .endm
-
-   /*
-* Returns the clock delta, in nanoseconds left-shifted by the clock
-* shift.
-*/
-   .macro  get_clock_shifted_nsec res, cycle_last, mult
-   /* Read the virtual counter. */
-   isb
-   mrs x_tmp, cntvct_el0
-   /* Calculate cycle delta and convert to ns. */
-   sub \res, x_tmp, \cycle_last
-   /* We can only guarantee 56 bits of precision. */
-   movnx_tmp, #0xff00, lsl #48
-   and \res, x_tmp, \res
-   mul \res, \res, \mult
-   .endm
-
-   /*
-* Returns in res_{sec,nsec} the REALTIME timespec, based on the
-* "wall time" (xtime) and the clock_mono delta.
-*/
-   .macro  get_ts_realtime res_sec, res_nsec, \
-   clock_nsec, xtime_sec, 

[PATCH 2/2] arm64:vdso: Remove ISB from gettimeofday.

[Andrew Pinski]

ISB is normally required before mrs CNTVCT if we want the
mrs to completed after the loads. In this case it is not.
As we are taking the difference and if that difference
was going to be negative, we just use the last counter value
instead.

Signed-off-by: Andrew Pinski 
---
 arch/arm64/kernel/vdso/gettimeofday.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/vdso/gettimeofday.c 
b/arch/arm64/kernel/vdso/gettimeofday.c
index 1293786..49edd35 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.c
+++ b/arch/arm64/kernel/vdso/gettimeofday.c
@@ -117,10 +117,20 @@ static notrace u64 get_clock_shifted_nsec(u64 cycle_last, 
u64 mult)
u64 res;
 
/* Read the virtual counter. */
-   isb();
+   /*
+* This normally requires an ISB but since we know the
+* read of the last cycle will always be after the
+* read of the values are valid word.
+*/
asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
 
-   res = res - cycle_last;
+   /*
+* If the current cycle is greater than the last,
+*  then get the difference.
+*/
+   if (res > cycle_last)
+   res = res - cycle_last;
+
/* We can only guarantee 56 bits of precision. */
res &= ~(0xff00ull<<48);
return res * mult;
-- 
2.7.4

[PATCH 2/2] arm64:vdso: Remove ISB from gettimeofday.

[Andrew Pinski]

ISB is normally required before mrs CNTVCT if we want the
mrs to completed after the loads. In this case it is not.
As we are taking the difference and if that difference
was going to be negative, we just use the last counter value
instead.

Signed-off-by: Andrew Pinski 
---
 arch/arm64/kernel/vdso/gettimeofday.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/vdso/gettimeofday.c 
b/arch/arm64/kernel/vdso/gettimeofday.c
index 1293786..49edd35 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.c
+++ b/arch/arm64/kernel/vdso/gettimeofday.c
@@ -117,10 +117,20 @@ static notrace u64 get_clock_shifted_nsec(u64 cycle_last, 
u64 mult)
u64 res;
 
/* Read the virtual counter. */
-   isb();
+   /*
+* This normally requires an ISB but since we know the
+* read of the last cycle will always be after the
+* read of the values are valid word.
+*/
asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
 
-   res = res - cycle_last;
+   /*
+* If the current cycle is greater than the last,
+*  then get the difference.
+*/
+   if (res > cycle_last)
+   res = res - cycle_last;
+
/* We can only guarantee 56 bits of precision. */
res &= ~(0xff00ull<<48);
return res * mult;
-- 
2.7.4

Re: [PATCH 4/7] driver core: fix automatic pinctrl management

[Linus Walleij]

On Tue, May 30, 2017 at 6:25 PM, Johan Hovold  wrote:

> Commit ab78029ecc34 ("drivers/pinctrl: grab default handles from device
> core") added automatic pin-control management to driver core by looking
> up and setting any default pinctrl state found in device tree while a
> device is being probed.

Actually we do not just support device tree, but also passing pin control
states from board files. It is handled by the core all the same.
So it's not a device tree thing.

One of those days we will have ACPI passing state tables too...

But I understand what you mean.

> Fix this by checking the new of_node_reused flag and skipping automatic
> pinctrl configuration during probe if set.

Seems like a solid idea. I hope we don't need another quirk for ACPI.
Acked-by: Linus Walleij 

Yours,
Linus Walleij

Re: [PATCH 4/7] driver core: fix automatic pinctrl management

[Linus Walleij]

On Tue, May 30, 2017 at 6:25 PM, Johan Hovold  wrote:

> Commit ab78029ecc34 ("drivers/pinctrl: grab default handles from device
> core") added automatic pin-control management to driver core by looking
> up and setting any default pinctrl state found in device tree while a
> device is being probed.

Actually we do not just support device tree, but also passing pin control
states from board files. It is handled by the core all the same.
So it's not a device tree thing.

One of those days we will have ACPI passing state tables too...

But I understand what you mean.

> Fix this by checking the new of_node_reused flag and skipping automatic
> pinctrl configuration during probe if set.

Seems like a solid idea. I hope we don't need another quirk for ACPI.
Acked-by: Linus Walleij 

Yours,
Linus Walleij

Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes

[Stephen Boyd]

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> The system crashes due to bad access when reading from an non configured
> peripheral and when writing to peripheral which is not owned by current
> ee. This patch verifies ownership to avoid crashing on
> write.

What systems? As far as I know we don't have any bad accesses
happening right now. If they are happening, we should fix the
code that's accessing hardware that isn't owned by them.

> For reads, since the forward mapping table, data_channel->ppid, is
> towards the end of the block, we use the core size to figure the
> max number of ppids supported. The table starts at an offset of 0x800
> within the block, so size - 0x800 will give us the area used by the
> table. Since each table is 4 bytes long (core_size - 0x800) / 4 will
> gives us the number of data_channel supported.
> This new protection is functional on hw v2.

Which brings us to the next question which is why do we need this
patch at all? We aren't probing hardware to see what we have
access to and then populating device structures based on that.
Instead, we're just populating DT nodes that we've hardcoded in
the dts files, so I'm a little lost on why we would have a node
in there that we couldn't access. Please add such details to the
commit text.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 84 
> +++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 5ec3a59..df463d4 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -111,6 +111,7 @@ enum pmic_arb_cmd_op_code {
>   * @ee:  the current Execution Environment
>   * @min_apid:minimum APID (used for bounding IRQ search)
>   * @max_apid:maximum APID
> + * @max_periph:  maximum number of PMIC peripherals supported by 
> HW.

Nitpick: Most of these lines don't end with a full-stop.

>   * @mapping_table:   in-memory copy of PPID -> APID mapping table.
>   * @domain:  irq domain object for PMIC IRQ domain
>   * @spmic:   SPMI controller object
> @@ -132,6 +133,7 @@ struct spmi_pmic_arb_dev {
>   u8  ee;
>   u16 min_apid;
>   u16 max_apid;
> + u16 max_periph;
>   u32 *mapping_table;
>   DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>   struct irq_domain   *domain;
> @@ -140,11 +142,13 @@ struct spmi_pmic_arb_dev {
>   const struct pmic_arb_ver_ops *ver_ops;
>   u16 *ppid_to_chan;
>   u16 last_channel;
> + u8  *chan_to_owner;

And we didn't document this one.

>  };
>  
>  /**
>   * pmic_arb_ver: version dependent functionality.
>   *
> + * @mode:access rights to specified pmic peripheral.
>   * @non_data_cmd:on v1 issues an spmi non-data command.
>   *   on v2 no HW support, returns -EOPNOTSUPP.
>   * @offset:  on v1 offset of per-ee channel.
> @@ -160,6 +164,8 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> + int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
>   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> u32 *offset);
> @@ -313,11 +319,23 @@ static int pmic_arb_read_cmd(struct spmi_controller 
> *ctrl, u8 opc, u8 sid,
>   u32 cmd;
>   int rc;
>   u32 offset;
> + mode_t mode;
>  
>   rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, );
>   if (rc)
>   return rc;
>  
> + rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, );
> + if (rc)
> + return rc;
> +
> + if (!(mode & S_IRUSR)) {

Using mode_t for hardware access is odd. Perhaps just come up
with some sort of READ/WRITE enum instead (if this sort of
checking is even needed)?

> + dev_err(_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

> + "error: impermissible read from peripheral sid:%d 
> addr:0x%x\n",
> + sid, addr);
> + return -EPERM;
> + }
> +
>   if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
>   dev_err(>dev,
>   "pmic-arb supports 1..%d bytes per trans, but:%zu 
> requested",
> @@ -364,11 +382,23 @@ static int pmic_arb_write_cmd(struct spmi_controller 
> *ctrl, u8 opc, u8 sid,
>   u32 cmd;
>   int rc;
>   u32 offset;
> + mode_t mode;
>  
>   rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, );
>

Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes

[Stephen Boyd]

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> The system crashes due to bad access when reading from an non configured
> peripheral and when writing to peripheral which is not owned by current
> ee. This patch verifies ownership to avoid crashing on
> write.

What systems? As far as I know we don't have any bad accesses
happening right now. If they are happening, we should fix the
code that's accessing hardware that isn't owned by them.

> For reads, since the forward mapping table, data_channel->ppid, is
> towards the end of the block, we use the core size to figure the
> max number of ppids supported. The table starts at an offset of 0x800
> within the block, so size - 0x800 will give us the area used by the
> table. Since each table is 4 bytes long (core_size - 0x800) / 4 will
> gives us the number of data_channel supported.
> This new protection is functional on hw v2.

Which brings us to the next question which is why do we need this
patch at all? We aren't probing hardware to see what we have
access to and then populating device structures based on that.
Instead, we're just populating DT nodes that we've hardcoded in
the dts files, so I'm a little lost on why we would have a node
in there that we couldn't access. Please add such details to the
commit text.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 84 
> +++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 5ec3a59..df463d4 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -111,6 +111,7 @@ enum pmic_arb_cmd_op_code {
>   * @ee:  the current Execution Environment
>   * @min_apid:minimum APID (used for bounding IRQ search)
>   * @max_apid:maximum APID
> + * @max_periph:  maximum number of PMIC peripherals supported by 
> HW.

Nitpick: Most of these lines don't end with a full-stop.

>   * @mapping_table:   in-memory copy of PPID -> APID mapping table.
>   * @domain:  irq domain object for PMIC IRQ domain
>   * @spmic:   SPMI controller object
> @@ -132,6 +133,7 @@ struct spmi_pmic_arb_dev {
>   u8  ee;
>   u16 min_apid;
>   u16 max_apid;
> + u16 max_periph;
>   u32 *mapping_table;
>   DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>   struct irq_domain   *domain;
> @@ -140,11 +142,13 @@ struct spmi_pmic_arb_dev {
>   const struct pmic_arb_ver_ops *ver_ops;
>   u16 *ppid_to_chan;
>   u16 last_channel;
> + u8  *chan_to_owner;

And we didn't document this one.

>  };
>  
>  /**
>   * pmic_arb_ver: version dependent functionality.
>   *
> + * @mode:access rights to specified pmic peripheral.
>   * @non_data_cmd:on v1 issues an spmi non-data command.
>   *   on v2 no HW support, returns -EOPNOTSUPP.
>   * @offset:  on v1 offset of per-ee channel.
> @@ -160,6 +164,8 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> + int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
>   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> u32 *offset);
> @@ -313,11 +319,23 @@ static int pmic_arb_read_cmd(struct spmi_controller 
> *ctrl, u8 opc, u8 sid,
>   u32 cmd;
>   int rc;
>   u32 offset;
> + mode_t mode;
>  
>   rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, );
>   if (rc)
>   return rc;
>  
> + rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, );
> + if (rc)
> + return rc;
> +
> + if (!(mode & S_IRUSR)) {

Using mode_t for hardware access is odd. Perhaps just come up
with some sort of READ/WRITE enum instead (if this sort of
checking is even needed)?

> + dev_err(_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

> + "error: impermissible read from peripheral sid:%d 
> addr:0x%x\n",
> + sid, addr);
> + return -EPERM;
> + }
> +
>   if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
>   dev_err(>dev,
>   "pmic-arb supports 1..%d bytes per trans, but:%zu 
> requested",
> @@ -364,11 +382,23 @@ static int pmic_arb_write_cmd(struct spmi_controller 
> *ctrl, u8 opc, u8 sid,
>   u32 cmd;
>   int rc;
>   u32 offset;
> + mode_t mode;
>  
>   rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, );
>   if (rc)
>   return rc;
>  
> + rc = 

Re: [PATCH] pinctrl: stm32: Fix bad function call

[Linus Walleij]

On Tue, May 30, 2017 at 4:43 PM, Alexandre TORGUE
 wrote:

> In stm32_pconf_parse_conf function, stm32_pmx_gpio_set_direction is
> called with wrong parameter value. Indeed, using NULL value for range
> will raise an oops.
>
> Fixes: aceb16dc2da5 ("pinctrl: Add STM32 MCUs support")
> Reported-by: Dan Carpenter 
> Signed-off-by: Alexandre TORGUE 

Patch applied for fixes.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: stm32: Fix bad function call

[Linus Walleij]

On Tue, May 30, 2017 at 4:43 PM, Alexandre TORGUE
 wrote:

> In stm32_pconf_parse_conf function, stm32_pmx_gpio_set_direction is
> called with wrong parameter value. Indeed, using NULL value for range
> will raise an oops.
>
> Fixes: aceb16dc2da5 ("pinctrl: Add STM32 MCUs support")
> Reported-by: Dan Carpenter 
> Signed-off-by: Alexandre TORGUE 

Patch applied for fixes.

Yours,
Linus Walleij

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Florian Fainelli]

On 05/30/2017 05:06 PM, Andrew Lunn wrote:
>> - past the initial setup, if we start creating bridge devices and so on,
>> we have no way to tell: group Ports 0-3 together and send traffic to CPU
>> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
>> DSA-only problem though, because we still have the CPU port(s) as
>> independent network interfaces.
> 
> What is the problem here? Frames come out the master interface, get
> untagged and passed to the slave interface and go upto the bridge. It
> should all just work. Same in the reverse direction.

The problem is really that is you have multiple CPU ports, how do you
define which one gets all the traffic by default? Ascending order of
port number? Descending order?

> 
> In order to make best use of the extra bandwidth of having two cpu
> ports, i probably want the user ports reasonably evenly distributed
> between the CPU ports. Dedicating one CPU port to one user port is
> probably sub-optimal. How many people have 1Gbps Fibre to the home,
> which could fully utilise a one-to-one mapping for the WAN port?

I actually tend to think that most use cases our there are in the order
of dedicating one CPU port to one corresponding switch port (user
facing, or internal) in order to provided guaranteed bandwidth for that
port. But as an user, I want to choose how the grouping is going to
work, and right now, I cannot, unless this is hardcoded in Device Tree,
which sounds both wrong and inadequate.

> 
>> Now, that would still force the user to configure two bridges in order
>> to properly steer traffic towards the requested ports but it would allow
>> us to be very flexible (which is probably desired here) in how ports are
>> grouped together.
> 
> We want a sensible default, spreading the slave ports evenly over the
> CPU ports. We could add a devlink command to change the defaults at
> runtime.

Sensible default is fine for the first time boot, but we should let
users be entirely flexible in how they want their user-facing ports to
map to a CPU port as you say, and IMHO using separate bridges to
configure that is a possible way to go since there is already knowledge
in the bridge join/leave code in DSA that already knows the
dwnstream/user-facing ports, but does not yet know about CPU ports.

Code speaks better, so let me see if I can cook something to illustrate
this.

Thanks!
-- 
Florian

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Florian Fainelli]

On 05/30/2017 05:06 PM, Andrew Lunn wrote:
>> - past the initial setup, if we start creating bridge devices and so on,
>> we have no way to tell: group Ports 0-3 together and send traffic to CPU
>> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
>> DSA-only problem though, because we still have the CPU port(s) as
>> independent network interfaces.
> 
> What is the problem here? Frames come out the master interface, get
> untagged and passed to the slave interface and go upto the bridge. It
> should all just work. Same in the reverse direction.

The problem is really that is you have multiple CPU ports, how do you
define which one gets all the traffic by default? Ascending order of
port number? Descending order?

> 
> In order to make best use of the extra bandwidth of having two cpu
> ports, i probably want the user ports reasonably evenly distributed
> between the CPU ports. Dedicating one CPU port to one user port is
> probably sub-optimal. How many people have 1Gbps Fibre to the home,
> which could fully utilise a one-to-one mapping for the WAN port?

I actually tend to think that most use cases our there are in the order
of dedicating one CPU port to one corresponding switch port (user
facing, or internal) in order to provided guaranteed bandwidth for that
port. But as an user, I want to choose how the grouping is going to
work, and right now, I cannot, unless this is hardcoded in Device Tree,
which sounds both wrong and inadequate.

> 
>> Now, that would still force the user to configure two bridges in order
>> to properly steer traffic towards the requested ports but it would allow
>> us to be very flexible (which is probably desired here) in how ports are
>> grouped together.
> 
> We want a sensible default, spreading the slave ports evenly over the
> CPU ports. We could add a devlink command to change the defaults at
> runtime.

Sensible default is fine for the first time boot, but we should let
users be entirely flexible in how they want their user-facing ports to
map to a CPU port as you say, and IMHO using separate bridges to
configure that is a possible way to go since there is already knowledge
in the bridge join/leave code in DSA that already knows the
dwnstream/user-facing ports, but does not yet know about CPU ports.

Code speaks better, so let me see if I can cook something to illustrate
this.

Thanks!
-- 
Florian

Re: [PATCH] gpio: xra1403: select REGMAP_SPI

[Linus Walleij]

On Tue, May 30, 2017 at 1:13 PM, Arnd Bergmann  wrote:

> Without the regmap code, we get a link error:
>
> drivers/gpio/built-in.o: In function `xra1403_probe':
> (.text+0x132e0): undefined reference to `__devm_regmap_init_spi'
>
> Fixes: 5704520d7880 ("gpio: xra1403: Add EXAR XRA1403 SPI GPIO expander 
> driver")
> Signed-off-by: Arnd Bergmann 

Patch applied.

Yours,
Linus Walleij

Re: [PATCH] gpio: xra1403: select REGMAP_SPI

[Linus Walleij]

On Tue, May 30, 2017 at 1:13 PM, Arnd Bergmann  wrote:

> Without the regmap code, we get a link error:
>
> drivers/gpio/built-in.o: In function `xra1403_probe':
> (.text+0x132e0): undefined reference to `__devm_regmap_init_spi'
>
> Fixes: 5704520d7880 ("gpio: xra1403: Add EXAR XRA1403 SPI GPIO expander 
> driver")
> Signed-off-by: Arnd Bergmann 

Patch applied.

Yours,
Linus Walleij

Re: [RFC][PATCH 0/4] Fixes for two recently found timekeeping bugs

[Daniel Mentz]

I successfully tested these four patches on a HiKey (ARMv8) board
using a 4.9 kernel. I cherry picked various commits from upstream that
touched kernel/time/timekeeping.c to ensure that John's patches apply
cleanly. The inconsistency-check from kselftest that previously failed
because of CLOCK_MONOTONIC_RAW jumping backwards by 1 ns is now
passing.

Tested-by: Daniel Mentz 

On Tue, May 30, 2017 at 5:10 PM, Daniel Mentz  wrote:
> I successfully tested these four patches on a HiKey (ARMv8) board using a
> 4.9 kernel. I cherry picked various commits from upstream that touched
> kernel/time/timekeeping.c to ensure that John's patches apply cleanly. The
> inconsistency-check from kselftest that previously failed because of
> CLOCK_MONOTONIC_RAW jumping backwards by 1 ns is now passing.
>
> Tested-by: Daniel Mentz 
>
> On Fri, May 26, 2017 at 8:33 PM, John Stultz  wrote:
>>
>> As part of the Linaro Linux Kernel Functional Test (LKFT)
>> effort, test failures from kselftest/timer's
>> inconsistency-check were reported connected to
>> CLOCK_MONOTONIC_RAW, on the HiKey platform.
>>
>> Digging in I found that an old issue with how sub-ns accounting
>> is handled with the RAW time which was fixed long ago with the
>> CLOCK_MONOTONIC/REALTIME ids, but missed with RAW time, was
>> present.
>>
>> Additionally, running further tests, I uncovered an issue with
>> how the clocksource read function is handled when clocksources
>> are changed, which can cause crashes.
>>
>> Both of these issues have not been uncovered in x86 based
>> testing due to x86 not using vDSO to accelerate
>> CLOCK_MONOTONIC_RAW, combined with the HiKey's arch_timer
>> clocksource being fast to access but incrementing slowly enough
>> to get multiple reads using the same counter value (which helps
>> uncover time handing issues), along with the fact that none of
>> the x86 clocksources making use of the clocksource argument
>> passed to the read function.
>>
>> This patchset addresses these two issues.
>>
>> Thanks so much to help from Will Deacon in getting the needed
>> adjustments to the arm64 vDSO in place. Also thanks to Daniel
>> Mentz who also properly diagnosed the MONOTONIC_RAW issue in
>> parallel while I was woking on this patchset.
>>
>> I wanted to send these out for some initial review. I'm
>> unfortunately still chasing a third issue (related to
>> inconsistencies triggered during extreme freq adjustments) I've
>> uncovered on HiKey, which doesn't seem to be related to the
>> previous two.
>>
>> As always feedback would be appreciated!
>>
>> thanks
>> -john
>>
>> Cc: Thomas Gleixner 
>> Cc: Ingo Molnar 
>> Cc: Miroslav Lichvar 
>> Cc: Richard Cochran 
>> Cc: Prarit Bhargava 
>> Cc: Stephen Boyd 
>> Cc: Daniel Mentz 
>>
>>
>> John Stultz (3):
>>   time: Fix clock->read(clock) race around clocksource changes
>>   time: Fix CLOCK_MONOTONIC_RAW sub-nanosecond accounting
>>   time: Clean up CLOCK_MONOTONIC_RAW time handling
>>
>> Will Deacon (1):
>>   arm64: vdso: Fix nsec handling for CLOCK_MONOTONIC_RAW
>>
>>  arch/arm64/kernel/vdso.c  |  5 +-
>>  arch/arm64/kernel/vdso/gettimeofday.S |  1 -
>>  include/linux/timekeeper_internal.h   |  8 +--
>>  kernel/time/timekeeping.c | 96
>> ++-
>>  4 files changed, 66 insertions(+), 44 deletions(-)
>>
>> --
>> 2.7.4
>>
>

Re: [RFC][PATCH 0/4] Fixes for two recently found timekeeping bugs

[Daniel Mentz]

I successfully tested these four patches on a HiKey (ARMv8) board
using a 4.9 kernel. I cherry picked various commits from upstream that
touched kernel/time/timekeeping.c to ensure that John's patches apply
cleanly. The inconsistency-check from kselftest that previously failed
because of CLOCK_MONOTONIC_RAW jumping backwards by 1 ns is now
passing.

Tested-by: Daniel Mentz 

On Tue, May 30, 2017 at 5:10 PM, Daniel Mentz  wrote:
> I successfully tested these four patches on a HiKey (ARMv8) board using a
> 4.9 kernel. I cherry picked various commits from upstream that touched
> kernel/time/timekeeping.c to ensure that John's patches apply cleanly. The
> inconsistency-check from kselftest that previously failed because of
> CLOCK_MONOTONIC_RAW jumping backwards by 1 ns is now passing.
>
> Tested-by: Daniel Mentz 
>
> On Fri, May 26, 2017 at 8:33 PM, John Stultz  wrote:
>>
>> As part of the Linaro Linux Kernel Functional Test (LKFT)
>> effort, test failures from kselftest/timer's
>> inconsistency-check were reported connected to
>> CLOCK_MONOTONIC_RAW, on the HiKey platform.
>>
>> Digging in I found that an old issue with how sub-ns accounting
>> is handled with the RAW time which was fixed long ago with the
>> CLOCK_MONOTONIC/REALTIME ids, but missed with RAW time, was
>> present.
>>
>> Additionally, running further tests, I uncovered an issue with
>> how the clocksource read function is handled when clocksources
>> are changed, which can cause crashes.
>>
>> Both of these issues have not been uncovered in x86 based
>> testing due to x86 not using vDSO to accelerate
>> CLOCK_MONOTONIC_RAW, combined with the HiKey's arch_timer
>> clocksource being fast to access but incrementing slowly enough
>> to get multiple reads using the same counter value (which helps
>> uncover time handing issues), along with the fact that none of
>> the x86 clocksources making use of the clocksource argument
>> passed to the read function.
>>
>> This patchset addresses these two issues.
>>
>> Thanks so much to help from Will Deacon in getting the needed
>> adjustments to the arm64 vDSO in place. Also thanks to Daniel
>> Mentz who also properly diagnosed the MONOTONIC_RAW issue in
>> parallel while I was woking on this patchset.
>>
>> I wanted to send these out for some initial review. I'm
>> unfortunately still chasing a third issue (related to
>> inconsistencies triggered during extreme freq adjustments) I've
>> uncovered on HiKey, which doesn't seem to be related to the
>> previous two.
>>
>> As always feedback would be appreciated!
>>
>> thanks
>> -john
>>
>> Cc: Thomas Gleixner 
>> Cc: Ingo Molnar 
>> Cc: Miroslav Lichvar 
>> Cc: Richard Cochran 
>> Cc: Prarit Bhargava 
>> Cc: Stephen Boyd 
>> Cc: Daniel Mentz 
>>
>>
>> John Stultz (3):
>>   time: Fix clock->read(clock) race around clocksource changes
>>   time: Fix CLOCK_MONOTONIC_RAW sub-nanosecond accounting
>>   time: Clean up CLOCK_MONOTONIC_RAW time handling
>>
>> Will Deacon (1):
>>   arm64: vdso: Fix nsec handling for CLOCK_MONOTONIC_RAW
>>
>>  arch/arm64/kernel/vdso.c  |  5 +-
>>  arch/arm64/kernel/vdso/gettimeofday.S |  1 -
>>  include/linux/timekeeper_internal.h   |  8 +--
>>  kernel/time/timekeeping.c | 96
>> ++-
>>  4 files changed, 66 insertions(+), 44 deletions(-)
>>
>> --
>> 2.7.4
>>
>

Re: [patch] compiler, clang: suppress warning for unused static inline functions

[David Rientjes]

On Wed, 24 May 2017, Doug Anderson wrote:

> * Matthias has been sending out individual patches that take each
> particular case into account to try to remove the warnings.  In some
> cases this removes totally dead code.  In other cases this adds
> __maybe_unused.  ...and as a last resort it uses #ifdef.  In each of
> these individual patches we wouldn't want a list of all other patches,
> I think.
> 

Again, I defer to maintainers like Andrew and Ingo who have to deal with 
an enormous amount of patches on how they would like to handle it; I don't 
think myself or anybody else who doesn't deal with a large number of 
patches should be mandating how it's handled.

For reference, the patchset that this patch originated from added 8 lines 
and removed 1, so I disagree that this cleans anything up; in reality, it 
obfuscates the code and makes the #ifdef nesting more complex.

> If you just want a list of things in response to this thread...
> 
> Clang's behavior has found some dead code, as shown by:
> 
> * https://patchwork.kernel.org/patch/9732161/
>   ring-buffer: Remove unused function __rb_data_page_index()
> * https://patchwork.kernel.org/patch/9735027/
>   r8152: Remove unused function usb_ocp_read()
> * https://patchwork.kernel.org/patch/9735053/
>   net1080: Remove unused function nc_dump_ttl()
> * https://patchwork.kernel.org/patch/9741513/
>   crypto: rng: Remove unused function __crypto_rng_cast()
> * https://patchwork.kernel.org/patch/9741539/
>   x86/ioapic: Remove unused function IO_APIC_irq_trigger()
> * https://patchwork.kernel.org/patch/9741549/
>   ASoC: Intel: sst: Remove unused function sst_restore_shim64()
> * https://patchwork.kernel.org/patch/9743225/
>   ASoC: cht_bsw_max98090_ti: Remove unused function cht_get_codec_dai()
> 
> ...plus more examples...
> 

Let us please not confuse the matter by suggesting that you cannot 
continue to do this work by simply removing the __attribute__((unused)) 
and emailing kernel-janitors to cleanup unused code (which should already 
be significantly small by the sheer fact that it is inlined).

> However, clang's behavior has also led to patches that add a
> "__maybe_unused" attribute (usually no increase in LOC unless it
> causes word wrap) and also added a handful of #ifdefs, as you've
> pointed out.  The example we already talked about was:
> 

The good work to remove truly dead code may easily continue while not 
adding more and more LOC to suppress these warnings for a compiler that is 
very heavily in the minority.

Re: [patch] compiler, clang: suppress warning for unused static inline functions

[David Rientjes]

On Wed, 24 May 2017, Doug Anderson wrote:

> * Matthias has been sending out individual patches that take each
> particular case into account to try to remove the warnings.  In some
> cases this removes totally dead code.  In other cases this adds
> __maybe_unused.  ...and as a last resort it uses #ifdef.  In each of
> these individual patches we wouldn't want a list of all other patches,
> I think.
> 

Again, I defer to maintainers like Andrew and Ingo who have to deal with 
an enormous amount of patches on how they would like to handle it; I don't 
think myself or anybody else who doesn't deal with a large number of 
patches should be mandating how it's handled.

For reference, the patchset that this patch originated from added 8 lines 
and removed 1, so I disagree that this cleans anything up; in reality, it 
obfuscates the code and makes the #ifdef nesting more complex.

> If you just want a list of things in response to this thread...
> 
> Clang's behavior has found some dead code, as shown by:
> 
> * https://patchwork.kernel.org/patch/9732161/
>   ring-buffer: Remove unused function __rb_data_page_index()
> * https://patchwork.kernel.org/patch/9735027/
>   r8152: Remove unused function usb_ocp_read()
> * https://patchwork.kernel.org/patch/9735053/
>   net1080: Remove unused function nc_dump_ttl()
> * https://patchwork.kernel.org/patch/9741513/
>   crypto: rng: Remove unused function __crypto_rng_cast()
> * https://patchwork.kernel.org/patch/9741539/
>   x86/ioapic: Remove unused function IO_APIC_irq_trigger()
> * https://patchwork.kernel.org/patch/9741549/
>   ASoC: Intel: sst: Remove unused function sst_restore_shim64()
> * https://patchwork.kernel.org/patch/9743225/
>   ASoC: cht_bsw_max98090_ti: Remove unused function cht_get_codec_dai()
> 
> ...plus more examples...
> 

Let us please not confuse the matter by suggesting that you cannot 
continue to do this work by simply removing the __attribute__((unused)) 
and emailing kernel-janitors to cleanup unused code (which should already 
be significantly small by the sheer fact that it is inlined).

> However, clang's behavior has also led to patches that add a
> "__maybe_unused" attribute (usually no increase in LOC unless it
> causes word wrap) and also added a handful of #ifdefs, as you've
> pointed out.  The example we already talked about was:
> 

The good work to remove truly dead code may easily continue while not 
adding more and more LOC to suppress these warnings for a compiler that is 
very heavily in the minority.

Re: [PATCH v2 2/3] pinctrl: stm32: Implement .get_direction gpio_chip callback

[Linus Walleij]

On Mon, May 29, 2017 at 6:17 PM, Alexandre TORGUE
 wrote:

> Add .get_direction() gpiochip callback in STM32 pinctrl driver.
>
> Signed-off-by: Alexandre TORGUE 

Good feature.

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] pinctrl: stm32: Implement .get_direction gpio_chip callback

[Linus Walleij]

On Mon, May 29, 2017 at 6:17 PM, Alexandre TORGUE
 wrote:

> Add .get_direction() gpiochip callback in STM32 pinctrl driver.
>
> Signed-off-by: Alexandre TORGUE 

Good feature.

Patch applied.

Yours,
Linus Walleij

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Andrew Lunn]

> - past the initial setup, if we start creating bridge devices and so on,
> we have no way to tell: group Ports 0-3 together and send traffic to CPU
> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
> DSA-only problem though, because we still have the CPU port(s) as
> independent network interfaces.

What is the problem here? Frames come out the master interface, get
untagged and passed to the slave interface and go upto the bridge. It
should all just work. Same in the reverse direction.

In order to make best use of the extra bandwidth of having two cpu
ports, i probably want the user ports reasonably evenly distributed
between the CPU ports. Dedicating one CPU port to one user port is
probably sub-optimal. How many people have 1Gbps Fibre to the home,
which could fully utilise a one-to-one mapping for the WAN port?

> Now, that would still force the user to configure two bridges in order
> to properly steer traffic towards the requested ports but it would allow
> us to be very flexible (which is probably desired here) in how ports are
> grouped together.

We want a sensible default, spreading the slave ports evenly over the
CPU ports. We could add a devlink command to change the defaults at
runtime.

Andrew

Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support

[Andrew Lunn]

> - past the initial setup, if we start creating bridge devices and so on,
> we have no way to tell: group Ports 0-3 together and send traffic to CPU
> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a
> DSA-only problem though, because we still have the CPU port(s) as
> independent network interfaces.

What is the problem here? Frames come out the master interface, get
untagged and passed to the slave interface and go upto the bridge. It
should all just work. Same in the reverse direction.

In order to make best use of the extra bandwidth of having two cpu
ports, i probably want the user ports reasonably evenly distributed
between the CPU ports. Dedicating one CPU port to one user port is
probably sub-optimal. How many people have 1Gbps Fibre to the home,
which could fully utilise a one-to-one mapping for the WAN port?

> Now, that would still force the user to configure two bridges in order
> to properly steer traffic towards the requested ports but it would allow
> us to be very flexible (which is probably desired here) in how ports are
> grouped together.

We want a sensible default, spreading the slave ports evenly over the
CPU ports. We could add a devlink command to change the defaults at
runtime.

Andrew

[PATCH] char: tpm: remove unnecessary code

[Gustavo A. R. Silva]

Remove unnecessary code.
Pointer chip cannot be NULL in st33zp24_send().

Addresses-Coverity-ID: 1397648
Cc: Jason Gunthorpe 
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/char/tpm/st33zp24/st33zp24.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/st33zp24.c 
b/drivers/char/tpm/st33zp24/st33zp24.c
index 4d1dc8b..ca74c24 100644
--- a/drivers/char/tpm/st33zp24/st33zp24.c
+++ b/drivers/char/tpm/st33zp24/st33zp24.c
@@ -373,8 +373,6 @@ static int st33zp24_send(struct tpm_chip *chip, unsigned 
char *buf,
int ret;
u8 data;
 
-   if (!chip)
-   return -EBUSY;
if (len < TPM_HEADER_SIZE)
return -EBUSY;
 
-- 
2.5.0

[PATCH] char: tpm: remove unnecessary code

[Gustavo A. R. Silva]

Remove unnecessary code.
Pointer chip cannot be NULL in st33zp24_send().

Addresses-Coverity-ID: 1397648
Cc: Jason Gunthorpe 
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/char/tpm/st33zp24/st33zp24.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/st33zp24.c 
b/drivers/char/tpm/st33zp24/st33zp24.c
index 4d1dc8b..ca74c24 100644
--- a/drivers/char/tpm/st33zp24/st33zp24.c
+++ b/drivers/char/tpm/st33zp24/st33zp24.c
@@ -373,8 +373,6 @@ static int st33zp24_send(struct tpm_chip *chip, unsigned 
char *buf,
int ret;
u8 data;
 
-   if (!chip)
-   return -EBUSY;
if (len < TPM_HEADER_SIZE)
return -EBUSY;
 
-- 
2.5.0

Re: [PATCH] xen: avoid type warning in xchg_xen_ulong

[Stefano Stabellini]

On Mon, 29 May 2017, Arnd Bergmann wrote:
> The improved type-checking version of container_of() triggers a warning for
> xchg_xen_ulong, pointing out that 'xen_ulong_t' is unsigned, but atomic64_t
> contains a signed value:
> 
> drivers/xen/events/events_2l.c: In function 'evtchn_2l_handle_events':
> arch/x86/include/asm/cmpxchg.h:87:21: error: 'pending_words' is used 
> uninitialized in this function [-Werror=uninitialized]

I don't think this is the warning message you wanted to paste here?
Other than that, the patch is fine.


> This adds a cast to work around the warning
> 
> Cc: Ian Abbott 
> Fixes: 85323a991d40 ("xen: arm: mandate EABI and use generic atomic 
> operations.")
> Fixes: daa2ac80834d ("kernel.h: handle pointers to arrays better in 
> container_of()")
> Signed-off-by: Arnd Bergmann 
> ---
>  arch/arm/include/asm/xen/events.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/xen/events.h 
> b/arch/arm/include/asm/xen/events.h
> index 71e473d05fcc..620dc75362e5 100644
> --- a/arch/arm/include/asm/xen/events.h
> +++ b/arch/arm/include/asm/xen/events.h
> @@ -16,7 +16,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
>   return raw_irqs_disabled_flags(regs->ARM_cpsr);
>  }
>  
> -#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr),   \
> +#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((long 
> long*)(ptr),\
>   atomic64_t, \
>   counter), (val))

Re: [PATCH] xen: avoid type warning in xchg_xen_ulong

[Stefano Stabellini]

On Mon, 29 May 2017, Arnd Bergmann wrote:
> The improved type-checking version of container_of() triggers a warning for
> xchg_xen_ulong, pointing out that 'xen_ulong_t' is unsigned, but atomic64_t
> contains a signed value:
> 
> drivers/xen/events/events_2l.c: In function 'evtchn_2l_handle_events':
> arch/x86/include/asm/cmpxchg.h:87:21: error: 'pending_words' is used 
> uninitialized in this function [-Werror=uninitialized]

I don't think this is the warning message you wanted to paste here?
Other than that, the patch is fine.


> This adds a cast to work around the warning
> 
> Cc: Ian Abbott 
> Fixes: 85323a991d40 ("xen: arm: mandate EABI and use generic atomic 
> operations.")
> Fixes: daa2ac80834d ("kernel.h: handle pointers to arrays better in 
> container_of()")
> Signed-off-by: Arnd Bergmann 
> ---
>  arch/arm/include/asm/xen/events.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/xen/events.h 
> b/arch/arm/include/asm/xen/events.h
> index 71e473d05fcc..620dc75362e5 100644
> --- a/arch/arm/include/asm/xen/events.h
> +++ b/arch/arm/include/asm/xen/events.h
> @@ -16,7 +16,7 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
>   return raw_irqs_disabled_flags(regs->ARM_cpsr);
>  }
>  
> -#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((ptr),   \
> +#define xchg_xen_ulong(ptr, val) atomic64_xchg(container_of((long 
> long*)(ptr),\
>   atomic64_t, \
>   counter), (val))

Re: [PATCH v2 1/3] pinctrl: stm32: set pin to gpio input when used as interrupt

[Linus Walleij]

On Mon, May 29, 2017 at 6:17 PM, Alexandre TORGUE
 wrote:

> This patch ensures that pin is correctly set as gpio input when it is used
> as an interrupt.
>
> Signed-off-by: Alexandre TORGUE 

Nice!

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] pinctrl: stm32: set pin to gpio input when used as interrupt

[Linus Walleij]

On Mon, May 29, 2017 at 6:17 PM, Alexandre TORGUE
 wrote:

> This patch ensures that pin is correctly set as gpio input when it is used
> as an interrupt.
>
> Signed-off-by: Alexandre TORGUE 

Nice!

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2]: perf/core: addressing 4x slowdown during per-process, profiling of STREAM benchmark on Intel Xeon Phi

[Arun Kalyanasundaram]

Hi Alexey,

I am interested in validating this fix. Can you please share some of
your testcases or let me know if you use any standard OpenMP
benchmarks?

- Arun

Re: [PATCH v2]: perf/core: addressing 4x slowdown during per-process, profiling of STREAM benchmark on Intel Xeon Phi

[Arun Kalyanasundaram]

Hi Alexey,

I am interested in validating this fix. Can you please share some of
your testcases or let me know if you use any standard OpenMP
benchmarks?

- Arun

Re: [PATCH v3 00/10] serial/gpio: exar: Fixes and support for IOT2000

[Linus Walleij]

On Mon, May 29, 2017 at 3:41 PM, Jan Kiszka  wrote:
> On 2017-05-29 14:39, Linus Walleij wrote:

>> Should I merge them all? Should Greg merge them all?
>> Should one of us produce an immutable branch?
>
> Half of the patch are affecting both serial and GPIO subsystem, most
> have GPIO focus, though. Just patch 10 or more serial than GPIO. All in
> all, maybe Greg can ack and things can flow through the GPIO tree?

OK with me, let's see what Greg says.

Yours,
Linus Walleij

Re: [PATCH v3 00/10] serial/gpio: exar: Fixes and support for IOT2000

[Linus Walleij]

On Mon, May 29, 2017 at 3:41 PM, Jan Kiszka  wrote:
> On 2017-05-29 14:39, Linus Walleij wrote:

>> Should I merge them all? Should Greg merge them all?
>> Should one of us produce an immutable branch?
>
> Half of the patch are affecting both serial and GPIO subsystem, most
> have GPIO focus, though. Just patch 10 or more serial than GPIO. All in
> all, maybe Greg can ack and things can flow through the GPIO tree?

OK with me, let's see what Greg says.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mcp23s08: improve I2C Kconfig dependency

[Linus Walleij]

On Tue, May 30, 2017 at 11:11 AM, Arnd Bergmann  wrote:

> With "SPI_MASTER=y && I2C=m", we can build mcp23s08 as a built-in driver,
> which then results in a link failure:
>
> drivers/pinctrl/built-in.o: In function `mcp23s08_probe_one.isra.0':
> :(.text+0x7910): undefined reference to `__devm_regmap_init_i2c'
> drivers/pinctrl/built-in.o: In function `mcp23s08_init':
> :(.init.text+0x110): undefined reference to `i2c_register_driver'
> drivers/pinctrl/built-in.o: In function `mcp23s08_exit':
> :(.exit.text+0x3c): undefined reference to `i2c_del_driver'
>
> To avoid the problem, this adds another dependency on I2C that enforces
> mcp23s08 to be a loadable module whenever the I2C core is a module.
>
> Fixes: 64ac43e6fa28 ("gpio: mcp23s08: move to pinctrl")
> Signed-off-by: Arnd Bergmann 

Patch applied!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mcp23s08: improve I2C Kconfig dependency

[Linus Walleij]

On Tue, May 30, 2017 at 11:11 AM, Arnd Bergmann  wrote:

> With "SPI_MASTER=y && I2C=m", we can build mcp23s08 as a built-in driver,
> which then results in a link failure:
>
> drivers/pinctrl/built-in.o: In function `mcp23s08_probe_one.isra.0':
> :(.text+0x7910): undefined reference to `__devm_regmap_init_i2c'
> drivers/pinctrl/built-in.o: In function `mcp23s08_init':
> :(.init.text+0x110): undefined reference to `i2c_register_driver'
> drivers/pinctrl/built-in.o: In function `mcp23s08_exit':
> :(.exit.text+0x3c): undefined reference to `i2c_del_driver'
>
> To avoid the problem, this adds another dependency on I2C that enforces
> mcp23s08 to be a loadable module whenever the I2C core is a module.
>
> Fixes: 64ac43e6fa28 ("gpio: mcp23s08: move to pinctrl")
> Signed-off-by: Arnd Bergmann 

Patch applied!

Yours,
Linus Walleij

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 7:40 PM, Daniel Micay wrote:
> On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote:
>> On 5/30/17 4:22 PM, Daniel Micay wrote:
 Thanks, I didn't know that android was doing this. I still think
 this
 feature
 is worthwhile for people to be able to harden their systems
 against
 this attack
 vector without having to implement a MAC.
>>>
>>> Since there's a capable LSM hook for ioctl already, it means it
>>> could go
>>> in Yama with ptrace_scope but core kernel code would still need to
>>> be
>>> changed to track the owning tty. I think Yama vs. core kernel
>>> shouldn't
>>> matter much anymore due to stackable LSMs.
>>>
>>
>> What does everyone think about a v8 that moves this feature under Yama
>> and uses
>> the file_ioctl LSM hook?
> 
> It would only make a difference if it could be fully contained there, as
> in not depending on tracking the tty owner.
> 

For the reasons discussed earlier (to allow for nested containers where one of
the containers is privileged) we want to track the user namespace that owns the 
tty.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 7:40 PM, Daniel Micay wrote:
> On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote:
>> On 5/30/17 4:22 PM, Daniel Micay wrote:
 Thanks, I didn't know that android was doing this. I still think
 this
 feature
 is worthwhile for people to be able to harden their systems
 against
 this attack
 vector without having to implement a MAC.
>>>
>>> Since there's a capable LSM hook for ioctl already, it means it
>>> could go
>>> in Yama with ptrace_scope but core kernel code would still need to
>>> be
>>> changed to track the owning tty. I think Yama vs. core kernel
>>> shouldn't
>>> matter much anymore due to stackable LSMs.
>>>
>>
>> What does everyone think about a v8 that moves this feature under Yama
>> and uses
>> the file_ioctl LSM hook?
> 
> It would only make a difference if it could be fully contained there, as
> in not depending on tracking the tty owner.
> 

For the reasons discussed earlier (to allow for nested containers where one of
the containers is privileged) we want to track the user namespace that owns the 
tty.

Re: [PATCH] gpiolib: remove unused variable

[Linus Walleij]

On Tue, May 30, 2017 at 11:21 AM, Arnd Bergmann  wrote:

> This was left behind by a cleanup patch:
>
> drivers/gpio/gpiolib.c: In function 'gpiochip_irqchip_init_valid_mask':
> drivers/gpio/gpiolib.c:1474:6: error: unused variable 'i' 
> [-Werror=unused-variable]
>
> Fixes: 923a654c186c ("gpiolib: Re-use bitmap_fill() instead of open coded 
> loop")
> Signed-off-by: Arnd Bergmann 

Patch applied.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: remove unused variable

[Linus Walleij]

On Tue, May 30, 2017 at 11:21 AM, Arnd Bergmann  wrote:

> This was left behind by a cleanup patch:
>
> drivers/gpio/gpiolib.c: In function 'gpiochip_irqchip_init_valid_mask':
> drivers/gpio/gpiolib.c:1474:6: error: unused variable 'i' 
> [-Werror=unused-variable]
>
> Fixes: 923a654c186c ("gpiolib: Re-use bitmap_fill() instead of open coded 
> loop")
> Signed-off-by: Arnd Bergmann 

Patch applied.

Yours,
Linus Walleij

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Alan Cox]

> This is my point. Apps will continue to shoot themselves in the foot. Of 
> course
> the correct response to one of these vulns is to not pass ttys across a
> security boundary. We have an opportunity here to reduce the impact of this 
> bug
> class at the kernel level. 

Not really.

If you pass me your console for example I can mmap your framebuffer and
spy on you all day. Or I could reprogram your fonts, your keyboard, your
video mode, or use set and paste selection to write stuff. If you are
using X and you can't get tty handles right you'll no doubt pass me a
copy of your X file descriptor in which case I own your display, your
keyboard and your mouse and I don't need to use TIOCSTI there either.

There are so many different attacks based upon that screwup that the
kernel cannot defend against them. You aren't exactly reducing the impact.

Alan

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Alan Cox]

> This is my point. Apps will continue to shoot themselves in the foot. Of 
> course
> the correct response to one of these vulns is to not pass ttys across a
> security boundary. We have an opportunity here to reduce the impact of this 
> bug
> class at the kernel level. 

Not really.

If you pass me your console for example I can mmap your framebuffer and
spy on you all day. Or I could reprogram your fonts, your keyboard, your
video mode, or use set and paste selection to write stuff. If you are
using X and you can't get tty handles right you'll no doubt pass me a
copy of your X file descriptor in which case I own your display, your
keyboard and your mouse and I don't need to use TIOCSTI there either.

There are so many different attacks based upon that screwup that the
kernel cannot defend against them. You aren't exactly reducing the impact.

Alan

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Daniel Micay]

On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote:
> On 5/30/17 4:22 PM, Daniel Micay wrote:
> > > Thanks, I didn't know that android was doing this. I still think
> > > this
> > > feature
> > > is worthwhile for people to be able to harden their systems
> > > against
> > > this attack
> > > vector without having to implement a MAC.
> > 
> > Since there's a capable LSM hook for ioctl already, it means it
> > could go
> > in Yama with ptrace_scope but core kernel code would still need to
> > be
> > changed to track the owning tty. I think Yama vs. core kernel
> > shouldn't
> > matter much anymore due to stackable LSMs.
> > 
> 
> What does everyone think about a v8 that moves this feature under Yama
> and uses
> the file_ioctl LSM hook?

It would only make a difference if it could be fully contained there, as
in not depending on tracking the tty owner.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Daniel Micay]

On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote:
> On 5/30/17 4:22 PM, Daniel Micay wrote:
> > > Thanks, I didn't know that android was doing this. I still think
> > > this
> > > feature
> > > is worthwhile for people to be able to harden their systems
> > > against
> > > this attack
> > > vector without having to implement a MAC.
> > 
> > Since there's a capable LSM hook for ioctl already, it means it
> > could go
> > in Yama with ptrace_scope but core kernel code would still need to
> > be
> > changed to track the owning tty. I think Yama vs. core kernel
> > shouldn't
> > matter much anymore due to stackable LSMs.
> > 
> 
> What does everyone think about a v8 that moves this feature under Yama
> and uses
> the file_ioctl LSM hook?

It would only make a difference if it could be fully contained there, as
in not depending on tracking the tty owner.

[PATCH] drm: Use vsnprintf extension %ph

[Joe Perches]

Using the extension saves a bit of code.

Miscellanea:

o Neaten and simplify dump_dp_payload_table
o Removed trailing blank space from output

$ size drivers/gpu/drm/drm_dp_mst_topology.o.* drivers/gpu/drm/tinydrm/*.o*
   textdata bss dec hex filename
  25848   0  16   258646508 
drivers/gpu/drm/drm_dp_mst_topology.o.new
  26091   0  16   2610765fb 
drivers/gpu/drm/drm_dp_mst_topology.o.old
   3362   2   03364 d24 drivers/gpu/drm/tinydrm/mipi-dbi.o.new
   3376   2   03378 d32 drivers/gpu/drm/tinydrm/mipi-dbi.o.old

Signed-off-by: Joe Perches 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++
 drivers/gpu/drm/tinydrm/mipi-dbi.c|  7 ++---
 2 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 222eb1a8549b..bfd237c15e76 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2836,16 +2836,15 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
 static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
  char *buf)
 {
-   int ret;
int i;
-   for (i = 0; i < 4; i++) {
-   ret = drm_dp_dpcd_read(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS 
+ (i * 16), [i * 16], 16);
-   if (ret != 16)
-   break;
+
+   for (i = 0; i < 64; i += 16) {
+   if (drm_dp_dpcd_read(mgr->aux,
+DP_PAYLOAD_TABLE_UPDATE_STATUS + i,
+[i], 16) != 16)
+   return false;
}
-   if (i == 4)
-   return true;
-   return false;
+   return true;
 }
 
 static void fetch_monitor_name(struct drm_dp_mst_topology_mgr *mgr,
@@ -2909,42 +2908,24 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
mutex_lock(>lock);
if (mgr->mst_primary) {
u8 buf[64];
-   bool bret;
int ret;
+
ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, buf, 
DP_RECEIVER_CAP_SIZE);
-   seq_printf(m, "dpcd: ");
-   for (i = 0; i < DP_RECEIVER_CAP_SIZE; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "dpcd: %*ph\n", DP_RECEIVER_CAP_SIZE, buf);
ret = drm_dp_dpcd_read(mgr->aux, DP_FAUX_CAP, buf, 2);
-   seq_printf(m, "faux/mst: ");
-   for (i = 0; i < 2; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "faux/mst: %*ph\n", 2, buf);
ret = drm_dp_dpcd_read(mgr->aux, DP_MSTM_CTRL, buf, 1);
-   seq_printf(m, "mst ctrl: ");
-   for (i = 0; i < 1; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "mst ctrl: %*ph\n", 1, buf);
 
/* dump the standard OUI branch header */
ret = drm_dp_dpcd_read(mgr->aux, DP_BRANCH_OUI, buf, 
DP_BRANCH_OUI_HEADER_SIZE);
-   seq_printf(m, "branch oui: ");
-   for (i = 0; i < 0x3; i++)
-   seq_printf(m, "%02x", buf[i]);
-   seq_printf(m, " devid: ");
+   seq_printf(m, "branch oui: %*phN devid: ", 3, buf);
for (i = 0x3; i < 0x8 && buf[i]; i++)
seq_printf(m, "%c", buf[i]);
-
-   seq_printf(m, " revision: hw: %x.%x sw: %x.%x", buf[0x9] >> 4, 
buf[0x9] & 0xf, buf[0xa], buf[0xb]);
-   seq_printf(m, "\n");
-   bret = dump_dp_payload_table(mgr, buf);
-   if (bret == true) {
-   seq_printf(m, "payload table: ");
-   for (i = 0; i < 63; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
-   }
+   seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n",
+  buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa], buf[0xb]);
+   if (dump_dp_payload_table(mgr, buf))
+   seq_printf(m, "payload table: %*ph\n", 63, buf);
 
}
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c 
b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index f4eb412f3604..c83eeb7a34b0 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -914,7 +914,7 @@ static int mipi_dbi_debugfs_command_show(struct seq_file 
*m, void *unused)
 {
struct mipi_dbi *mipi = m->private;
u8 cmd, val[4];
-   size_t len, i;
+   size_t len;
int ret;
 
for (cmd = 0; cmd < 255; cmd++) {
@@ -943,10 +943,7 @@ static int mipi_dbi_debugfs_command_show(struct seq_file 
*m, void *unused)
seq_puts(m, 

[PATCH] drm: Use vsnprintf extension %ph

[Joe Perches]

Using the extension saves a bit of code.

Miscellanea:

o Neaten and simplify dump_dp_payload_table
o Removed trailing blank space from output

$ size drivers/gpu/drm/drm_dp_mst_topology.o.* drivers/gpu/drm/tinydrm/*.o*
   textdata bss dec hex filename
  25848   0  16   258646508 
drivers/gpu/drm/drm_dp_mst_topology.o.new
  26091   0  16   2610765fb 
drivers/gpu/drm/drm_dp_mst_topology.o.old
   3362   2   03364 d24 drivers/gpu/drm/tinydrm/mipi-dbi.o.new
   3376   2   03378 d32 drivers/gpu/drm/tinydrm/mipi-dbi.o.old

Signed-off-by: Joe Perches 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++
 drivers/gpu/drm/tinydrm/mipi-dbi.c|  7 ++---
 2 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 222eb1a8549b..bfd237c15e76 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2836,16 +2836,15 @@ static void drm_dp_mst_dump_mstb(struct seq_file *m,
 static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
  char *buf)
 {
-   int ret;
int i;
-   for (i = 0; i < 4; i++) {
-   ret = drm_dp_dpcd_read(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS 
+ (i * 16), [i * 16], 16);
-   if (ret != 16)
-   break;
+
+   for (i = 0; i < 64; i += 16) {
+   if (drm_dp_dpcd_read(mgr->aux,
+DP_PAYLOAD_TABLE_UPDATE_STATUS + i,
+[i], 16) != 16)
+   return false;
}
-   if (i == 4)
-   return true;
-   return false;
+   return true;
 }
 
 static void fetch_monitor_name(struct drm_dp_mst_topology_mgr *mgr,
@@ -2909,42 +2908,24 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
mutex_lock(>lock);
if (mgr->mst_primary) {
u8 buf[64];
-   bool bret;
int ret;
+
ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, buf, 
DP_RECEIVER_CAP_SIZE);
-   seq_printf(m, "dpcd: ");
-   for (i = 0; i < DP_RECEIVER_CAP_SIZE; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "dpcd: %*ph\n", DP_RECEIVER_CAP_SIZE, buf);
ret = drm_dp_dpcd_read(mgr->aux, DP_FAUX_CAP, buf, 2);
-   seq_printf(m, "faux/mst: ");
-   for (i = 0; i < 2; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "faux/mst: %*ph\n", 2, buf);
ret = drm_dp_dpcd_read(mgr->aux, DP_MSTM_CTRL, buf, 1);
-   seq_printf(m, "mst ctrl: ");
-   for (i = 0; i < 1; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
+   seq_printf(m, "mst ctrl: %*ph\n", 1, buf);
 
/* dump the standard OUI branch header */
ret = drm_dp_dpcd_read(mgr->aux, DP_BRANCH_OUI, buf, 
DP_BRANCH_OUI_HEADER_SIZE);
-   seq_printf(m, "branch oui: ");
-   for (i = 0; i < 0x3; i++)
-   seq_printf(m, "%02x", buf[i]);
-   seq_printf(m, " devid: ");
+   seq_printf(m, "branch oui: %*phN devid: ", 3, buf);
for (i = 0x3; i < 0x8 && buf[i]; i++)
seq_printf(m, "%c", buf[i]);
-
-   seq_printf(m, " revision: hw: %x.%x sw: %x.%x", buf[0x9] >> 4, 
buf[0x9] & 0xf, buf[0xa], buf[0xb]);
-   seq_printf(m, "\n");
-   bret = dump_dp_payload_table(mgr, buf);
-   if (bret == true) {
-   seq_printf(m, "payload table: ");
-   for (i = 0; i < 63; i++)
-   seq_printf(m, "%02x ", buf[i]);
-   seq_printf(m, "\n");
-   }
+   seq_printf(m, " revision: hw: %x.%x sw: %x.%x\n",
+  buf[0x9] >> 4, buf[0x9] & 0xf, buf[0xa], buf[0xb]);
+   if (dump_dp_payload_table(mgr, buf))
+   seq_printf(m, "payload table: %*ph\n", 63, buf);
 
}
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c 
b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index f4eb412f3604..c83eeb7a34b0 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -914,7 +914,7 @@ static int mipi_dbi_debugfs_command_show(struct seq_file 
*m, void *unused)
 {
struct mipi_dbi *mipi = m->private;
u8 cmd, val[4];
-   size_t len, i;
+   size_t len;
int ret;
 
for (cmd = 0; cmd < 255; cmd++) {
@@ -943,10 +943,7 @@ static int mipi_dbi_debugfs_command_show(struct seq_file 
*m, void *unused)
seq_puts(m, "XX\n");
   

Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

[Jakub Kicinski]

On Tue, 30 May 2017 18:07:18 -0500, Bjorn Helgaas wrote:
> On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> > On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:  
> > > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:  
> > > > PCI core sets the driver pointer before calling ->probe() and only
> > > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > > callback will happily race with probe() and remove(), most likely
> > > > leading to BUGs, since drivers don't expect this.
> > > 
> > > I guess you're referring to the pci_dev->driver pointer set by
> > > local_pci_probe(), and this is important because sriov_numvfs_store()
> > > checks that pointer, right?  
> > 
> > Yes, exactly.  I initially thought this is how the safety of sriov
> > callback may have been ensured, but since the order of
> > local_pci_probe() and the assignment is what it is, it can't.  
> 
> Right.  I was hoping other subsystems would establish a convention
> about whether we set the ->driver pointer before or after calling the
> driver probe() method, but if there is one, I don't see it.
> local_pci_probe() and really_probe() set ->driver first, but
> pnp_device_probe() calls the probe() method first.

I didn't dig into reordering the pointer setting, to be honest.  I
thought establishing that driver callbacks should generally hold device
lock, whenever possible, would be even better than pointer setting
conventions.

If we order the assignments better, wouldn't we still need appropriate
memory barriers to rely on the order? (:

> Can you expand on how you reproduce this problem?  The only real way I
> see to call ->sriov_configure() is via the sysfs entry point, and I
> would think user-space code would typically not touch that until after
> it knows the driver has claimed a device.  But I can certainly imagine
> targeted test code that could hit this problem.

Correct.  It's not something that users should be triggering often in
normal use.  I also found it by code inspection rather than by getting
an oops.

OTOH if the driver performs FW load or other time-consuming operations
in ->probe() the time window when this can be triggered may be counted
in seconds.

Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

[Jakub Kicinski]

On Tue, 30 May 2017 18:07:18 -0500, Bjorn Helgaas wrote:
> On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> > On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:  
> > > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:  
> > > > PCI core sets the driver pointer before calling ->probe() and only
> > > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > > callback will happily race with probe() and remove(), most likely
> > > > leading to BUGs, since drivers don't expect this.
> > > 
> > > I guess you're referring to the pci_dev->driver pointer set by
> > > local_pci_probe(), and this is important because sriov_numvfs_store()
> > > checks that pointer, right?  
> > 
> > Yes, exactly.  I initially thought this is how the safety of sriov
> > callback may have been ensured, but since the order of
> > local_pci_probe() and the assignment is what it is, it can't.  
> 
> Right.  I was hoping other subsystems would establish a convention
> about whether we set the ->driver pointer before or after calling the
> driver probe() method, but if there is one, I don't see it.
> local_pci_probe() and really_probe() set ->driver first, but
> pnp_device_probe() calls the probe() method first.

I didn't dig into reordering the pointer setting, to be honest.  I
thought establishing that driver callbacks should generally hold device
lock, whenever possible, would be even better than pointer setting
conventions.

If we order the assignments better, wouldn't we still need appropriate
memory barriers to rely on the order? (:

> Can you expand on how you reproduce this problem?  The only real way I
> see to call ->sriov_configure() is via the sysfs entry point, and I
> would think user-space code would typically not touch that until after
> it knows the driver has claimed a device.  But I can certainly imagine
> targeted test code that could hit this problem.

Correct.  It's not something that users should be triggering often in
normal use.  I also found it by code inspection rather than by getting
an oops.

OTOH if the driver performs FW load or other time-consuming operations
in ->probe() the time window when this can be triggered may be counted
in seconds.

Re: [PATCH] staging: android: uapi: drop definitions of removed ION_IOC_{FREE,SHARE} ioctls

[Laura Abbott]

On 05/30/2017 07:11 AM, Gleb Fotengauer-Malinovskiy wrote:
> This problem was found by strace ioctl list generator.
> 
> Fixes: 15c6098cfec5 ("staging: android: ion: Remove ion_handle and 
> ion_client")
> Signed-off-by: Gleb Fotengauer-Malinovskiy 
> ---
>  drivers/staging/android/uapi/ion.h | 18 --
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/staging/android/uapi/ion.h 
> b/drivers/staging/android/uapi/ion.h
> index b76db1b..a291b12 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -131,24 +131,6 @@ struct ion_heap_query {
> struct ion_allocation_data)
>  
>  /**
> - * DOC: ION_IOC_FREE - free memory
> - *
> - * Takes an ion_handle_data struct and frees the handle.
> - */
> -#define ION_IOC_FREE _IOWR(ION_IOC_MAGIC, 1, struct ion_handle_data)
> -
> -/**
> - * DOC: ION_IOC_SHARE - creates a file descriptor to use to share an 
> allocation
> - *
> - * Takes an ion_fd_data struct with the handle field populated with a valid
> - * opaque handle.  Returns the struct with the fd field set to a file
> - * descriptor open in the current address space.  This file descriptor
> - * can then be passed to another process.  The corresponding opaque handle 
> can
> - * be retrieved via ION_IOC_IMPORT.
> - */
> -#define ION_IOC_SHARE_IOWR(ION_IOC_MAGIC, 4, struct 
> ion_fd_data)
> -
> -/**
>   * DOC: ION_IOC_HEAP_QUERY - information about available heaps
>   *
>   * Takes an ion_heap_query structure and populates information about
> 

Acked-by: Laura Abbott 

Re: [PATCH] staging: android: uapi: drop definitions of removed ION_IOC_{FREE,SHARE} ioctls

[Laura Abbott]

On 05/30/2017 07:11 AM, Gleb Fotengauer-Malinovskiy wrote:
> This problem was found by strace ioctl list generator.
> 
> Fixes: 15c6098cfec5 ("staging: android: ion: Remove ion_handle and 
> ion_client")
> Signed-off-by: Gleb Fotengauer-Malinovskiy 
> ---
>  drivers/staging/android/uapi/ion.h | 18 --
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/staging/android/uapi/ion.h 
> b/drivers/staging/android/uapi/ion.h
> index b76db1b..a291b12 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -131,24 +131,6 @@ struct ion_heap_query {
> struct ion_allocation_data)
>  
>  /**
> - * DOC: ION_IOC_FREE - free memory
> - *
> - * Takes an ion_handle_data struct and frees the handle.
> - */
> -#define ION_IOC_FREE _IOWR(ION_IOC_MAGIC, 1, struct ion_handle_data)
> -
> -/**
> - * DOC: ION_IOC_SHARE - creates a file descriptor to use to share an 
> allocation
> - *
> - * Takes an ion_fd_data struct with the handle field populated with a valid
> - * opaque handle.  Returns the struct with the fd field set to a file
> - * descriptor open in the current address space.  This file descriptor
> - * can then be passed to another process.  The corresponding opaque handle 
> can
> - * be retrieved via ION_IOC_IMPORT.
> - */
> -#define ION_IOC_SHARE_IOWR(ION_IOC_MAGIC, 4, struct 
> ion_fd_data)
> -
> -/**
>   * DOC: ION_IOC_HEAP_QUERY - information about available heaps
>   *
>   * Takes an ion_heap_query structure and populates information about
> 

Acked-by: Laura Abbott 

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Sorry to respond to myself. Thomas, your reply to another mail in this
series helped me to notice:

On Tue, May 30, 2017 at 04:19:58PM -0700, Brian Norris wrote:
> Side note: for issues like the first problem above, I wonder why there
> isn't a flag that once could pass to request_irq() that suggests the IRQ
> should be initially disabled?

Is that what IRQ_NOAUTOEN is for?

> I know this wouldn't work for shared
> interrupts (but request_irq() could reject that combination, no?)

Hehe, but then I see this, for example, when grepping around:

drivers/usb/dwc3/dwc3-omap.c:

irq_set_status_flags(omap->irq, IRQ_NOAUTOEN);
ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
dwc3_omap_interrupt_thread, IRQF_SHARED,
"dwc3-omap", omap);

IIUC, that's quite broken, no?

Brian

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Sorry to respond to myself. Thomas, your reply to another mail in this
series helped me to notice:

On Tue, May 30, 2017 at 04:19:58PM -0700, Brian Norris wrote:
> Side note: for issues like the first problem above, I wonder why there
> isn't a flag that once could pass to request_irq() that suggests the IRQ
> should be initially disabled?

Is that what IRQ_NOAUTOEN is for?

> I know this wouldn't work for shared
> interrupts (but request_irq() could reject that combination, no?)

Hehe, but then I see this, for example, when grepping around:

drivers/usb/dwc3/dwc3-omap.c:

irq_set_status_flags(omap->irq, IRQ_NOAUTOEN);
ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
dwc3_omap_interrupt_thread, IRQF_SHARED,
"dwc3-omap", omap);

IIUC, that's quite broken, no?

Brian

Re: [PATCH] ARM64: dts: meson-gx: Add SPICC nodes

[Kevin Hilman]

Kevin Hilman  writes:

> Neil Armstrong  writes:
>
>> Add nodes for the SPICC controller on GX common dtsi, GXBB and
>> GXL dtsi files.
>>
>> Signed-off-by: Neil Armstrong 
>
> Applied to v4.13/dt64,

Oops, this one will have to wait until we have an immutable branch in
the clk tree.

Kevin

Re: [PATCH] ARM64: dts: meson-gx: Add SPICC nodes

[Kevin Hilman]

Kevin Hilman  writes:

> Neil Armstrong  writes:
>
>> Add nodes for the SPICC controller on GX common dtsi, GXBB and
>> GXL dtsi files.
>>
>> Signed-off-by: Neil Armstrong 
>
> Applied to v4.13/dt64,

Oops, this one will have to wait until we have an immutable branch in
the clk tree.

Kevin

Re: [PATCH 3/3] zswap: Delete an error message for a failed memory allocation in zswap_dstmem_prepare()

[Dan Streetman]

On Sun, May 21, 2017 at 4:27 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sun, 21 May 2017 09:29:25 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 3f0a9a1daef4..ed7312291df9 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -374,7 +374,6 @@ static int zswap_dstmem_prepare(unsigned int cpu)
> -   if (!dst) {
> -   pr_err("can't allocate compressor buffer\n");
> +   if (!dst)
> return -ENOMEM;
> -   }
> +
> per_cpu(zswap_dstmem, cpu) = dst;
> return 0;
>  }
> --
> 2.13.0
>

Re: [PATCH 3/3] zswap: Delete an error message for a failed memory allocation in zswap_dstmem_prepare()

[Dan Streetman]

On Sun, May 21, 2017 at 4:27 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sun, 21 May 2017 09:29:25 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 3f0a9a1daef4..ed7312291df9 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -374,7 +374,6 @@ static int zswap_dstmem_prepare(unsigned int cpu)
> -   if (!dst) {
> -   pr_err("can't allocate compressor buffer\n");
> +   if (!dst)
> return -ENOMEM;
> -   }
> +
> per_cpu(zswap_dstmem, cpu) = dst;
> return 0;
>  }
> --
> 2.13.0
>

Re: [PATCH 2/3] zswap: Improve a size determination in zswap_frontswap_init()

[Dan Streetman]

On Sun, May 21, 2017 at 4:26 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 20 May 2017 22:44:03 +0200
>
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
>
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 18d8e87119a6..a6e67633be03 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1156,5 +1156,5 @@ static void zswap_frontswap_init(unsigned type)
>  {
> struct zswap_tree *tree;
>
> -   tree = kzalloc(sizeof(struct zswap_tree), GFP_KERNEL);
> +   tree = kzalloc(sizeof(*tree), GFP_KERNEL);
> if (!tree) {
> --
> 2.13.0
>

Re: [PATCH 2/3] zswap: Improve a size determination in zswap_frontswap_init()

[Dan Streetman]

On Sun, May 21, 2017 at 4:26 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 20 May 2017 22:44:03 +0200
>
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
>
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 18d8e87119a6..a6e67633be03 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1156,5 +1156,5 @@ static void zswap_frontswap_init(unsigned type)
>  {
> struct zswap_tree *tree;
>
> -   tree = kzalloc(sizeof(struct zswap_tree), GFP_KERNEL);
> +   tree = kzalloc(sizeof(*tree), GFP_KERNEL);
> if (!tree) {
> --
> 2.13.0
>

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Hi,

To address a tangent brought up here:

On Sat, May 27, 2017 at 10:16:37AM +0200, Thomas Gleixner wrote:
> On Sat, 27 May 2017, jeffy wrote:
> > for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) try 
> > to
> > do these:
> > 
> > devm_request_irq->irq_startup->irq_enable
> > disable_irq <-- disabled and masked
> > devm_free_irq->irq_shutdown <-- disable it again
> 
> This driver is broken as hell.

No argument on the general statement :)

> It requests the interrupt _BEFORE_ the whole
> thing is initialized. If there is a pending interrupt on that line, it will
> explode nicely before it is able to disable the irq. But that's a different
> problem.

For that particular interrupt, it's mostly an informational interrupt
regarding wakeups. We don't do anything that could blow up there, except
report a (spurious) wakeup event. (And this spurious wakeup event only
occurs because the Wifi firmware may toggle its "wake" pin even when the
system is already awake. A weird behavior...)

So yes, the pattern isn't great, but no, it's not going to blow up,
AFAIK.

However, if you were to look at the same driver's .../mwifiex/pcie.c,
you would see a similar problem, and you *would* be right if you claimed
that things could blow up badly there! mwifiex_pcie_request_irq() is
called much too early, and if an interrupt gets queued up at the wrong
time, we won't handle it very nicely.

Anyway, I just thought I'd mention it, in case someone else following
this thread is curious. Coincidentally, I'm already working on patching
this on linux-wireless@.

Side note: for issues like the first problem above, I wonder why there
isn't a flag that once could pass to request_irq() that suggests the IRQ
should be initially disabled? I know this wouldn't work for shared
interrupts (but request_irq() could reject that combination, no?), but
it seems like there are plenty of cases where it might be useful. Some
devices simply don't have a device-level interrupt mask, and always
expect to have a dedicated interrupt. With the status quo, a driver for
such a device has to defer their request_irq() until
sometimes-inconvient times [1], or else accept some subpar behavior (see
above "spurious wakeup reporting").

Regards,
Brian

[1] Note that, for one, request_irq() can fail, whereas enable_irq()
cannot.

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Hi,

To address a tangent brought up here:

On Sat, May 27, 2017 at 10:16:37AM +0200, Thomas Gleixner wrote:
> On Sat, 27 May 2017, jeffy wrote:
> > for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) try 
> > to
> > do these:
> > 
> > devm_request_irq->irq_startup->irq_enable
> > disable_irq <-- disabled and masked
> > devm_free_irq->irq_shutdown <-- disable it again
> 
> This driver is broken as hell.

No argument on the general statement :)

> It requests the interrupt _BEFORE_ the whole
> thing is initialized. If there is a pending interrupt on that line, it will
> explode nicely before it is able to disable the irq. But that's a different
> problem.

For that particular interrupt, it's mostly an informational interrupt
regarding wakeups. We don't do anything that could blow up there, except
report a (spurious) wakeup event. (And this spurious wakeup event only
occurs because the Wifi firmware may toggle its "wake" pin even when the
system is already awake. A weird behavior...)

So yes, the pattern isn't great, but no, it's not going to blow up,
AFAIK.

However, if you were to look at the same driver's .../mwifiex/pcie.c,
you would see a similar problem, and you *would* be right if you claimed
that things could blow up badly there! mwifiex_pcie_request_irq() is
called much too early, and if an interrupt gets queued up at the wrong
time, we won't handle it very nicely.

Anyway, I just thought I'd mention it, in case someone else following
this thread is curious. Coincidentally, I'm already working on patching
this on linux-wireless@.

Side note: for issues like the first problem above, I wonder why there
isn't a flag that once could pass to request_irq() that suggests the IRQ
should be initially disabled? I know this wouldn't work for shared
interrupts (but request_irq() could reject that combination, no?), but
it seems like there are plenty of cases where it might be useful. Some
devices simply don't have a device-level interrupt mask, and always
expect to have a dedicated interrupt. With the status quo, a driver for
such a device has to defer their request_irq() until
sometimes-inconvient times [1], or else accept some subpar behavior (see
above "spurious wakeup reporting").

Regards,
Brian

[1] Note that, for one, request_irq() can fail, whereas enable_irq()
cannot.

Re: [PATCH 1/3] zswap: Delete an error message for a failed memory allocation in zswap_pool_create()

[Dan Streetman]

On Sun, May 21, 2017 at 4:25 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 20 May 2017 22:33:21 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index eedc27894b10..18d8e87119a6 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -518,7 +518,5 @@ static struct zswap_pool *zswap_pool_create(char *type, 
> char *compressor)
> -   if (!pool) {
> -   pr_err("pool alloc failed\n");
> +   if (!pool)
> return NULL;
> -   }
>
> /* unique name for each pool specifically required by zsmalloc */
> snprintf(name, 38, "zswap%x", atomic_inc_return(_pools_count));
> --
> 2.13.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 6:51 PM, Alan Cox wrote:
> On Tue, 30 May 2017 12:28:59 -0400
> Matt Brown  wrote:
> 
>> On 5/30/17 8:24 AM, Alan Cox wrote:
>>> Look there are two problems here
>>>
>>> 1. TIOCSTI has users  
>>
>> I don't see how this is a problem.
> 
> Which is unfortunate. To start with if it didn't have users we could just
> delete it.
> 
>>>
>>> 2. You don't actually fix anything
>>>
>>> The underlying problem is that if you give your tty handle to another
>>> process which you don't trust you are screwed. It's fundamental to the
>>> design of the Unix tty model and it's made worse in Linux by the fact
>>> that we use the tty descriptor to access all sorts of other console state
>>> (which makes a ton of sense).
>>>
>>> Many years ago a few people got this wrong. All those apps got fixes back
>>> then. They allocate a tty/pty pair and create a new session over that.
>>> The potentially hostile other app only gets to screw itself.
>>>   
>>
>> Many years ago? We already got one in 2017, as well as a bunch last year.
>> See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti
> 
> All the apps got fixed at the time. The fact the next generation of
> forgot to learn from it is unfortunate but hardly new. Also every single
> one of those that exposes a tty in that way allows other annoying
> behaviours via other ioctl interfaces so none of them would have been
> properly mitigated.
> 

This is my point. Apps will continue to shoot themselves in the foot. Of course
the correct response to one of these vulns is to not pass ttys across a
security boundary. We have an opportunity here to reduce the impact of this bug
class at the kernel level. Rejecting this mitigation because the real solution
is to use a tty/pty pair is like saying we should reject ASLR because the real
solution to buffer overflows is proper bounds checking.

> If you really want to do that particular bit of snake oiling then you can
> use the existing SELinux, seccomp and related interfaces. They can even
> do the job properly by whitelisting or blocking long lists of ioctls.
> 
>> This protections seeks to protect users from programs that don't do things
>> correctly. Rather than killing bugs, this feature attempts to kill an entire
>> bug class that shows little sign of slowing down in the world of containers 
>> and
>> sandboxes.
> 
> Well maybe the people writing them need to learn what they are doing and
> stop passing random file descriptors into their container (I've even seen
> people handing X file handles into their 'container').
> 
> The kernel can do some things to help programmers but it can't stop
> people writing crap. Anyone writing code that crosses security boundaries
> should have at least a vague idea of what they are doing.
> 
> The only way you'd actually really prevent this would be to magically
> open a new pty/tty pair and substitute the file handlers plus a data
> copying thread when someone created a namespace.
> 
> Now you can actually do that with the ptrace functionality in seccomp but
> it would still be fairly insane to expect the kernel to handle.
> 
> Alan
> [Actually even more sensible would be to revert the entire sorry
> container mess and use VMs but it's a bit late for that ;-)]
> 

Totally agree. VMs >> Containers but the cat is out of the bag and we can't put
it back.

Re: [PATCH 1/3] zswap: Delete an error message for a failed memory allocation in zswap_pool_create()

[Dan Streetman]

On Sun, May 21, 2017 at 4:25 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 20 May 2017 22:33:21 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring 

Acked-by: Dan Streetman 

> ---
>  mm/zswap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index eedc27894b10..18d8e87119a6 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -518,7 +518,5 @@ static struct zswap_pool *zswap_pool_create(char *type, 
> char *compressor)
> -   if (!pool) {
> -   pr_err("pool alloc failed\n");
> +   if (!pool)
> return NULL;
> -   }
>
> /* unique name for each pool specifically required by zsmalloc */
> snprintf(name, 38, "zswap%x", atomic_inc_return(_pools_count));
> --
> 2.13.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 6:51 PM, Alan Cox wrote:
> On Tue, 30 May 2017 12:28:59 -0400
> Matt Brown  wrote:
> 
>> On 5/30/17 8:24 AM, Alan Cox wrote:
>>> Look there are two problems here
>>>
>>> 1. TIOCSTI has users  
>>
>> I don't see how this is a problem.
> 
> Which is unfortunate. To start with if it didn't have users we could just
> delete it.
> 
>>>
>>> 2. You don't actually fix anything
>>>
>>> The underlying problem is that if you give your tty handle to another
>>> process which you don't trust you are screwed. It's fundamental to the
>>> design of the Unix tty model and it's made worse in Linux by the fact
>>> that we use the tty descriptor to access all sorts of other console state
>>> (which makes a ton of sense).
>>>
>>> Many years ago a few people got this wrong. All those apps got fixes back
>>> then. They allocate a tty/pty pair and create a new session over that.
>>> The potentially hostile other app only gets to screw itself.
>>>   
>>
>> Many years ago? We already got one in 2017, as well as a bunch last year.
>> See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti
> 
> All the apps got fixed at the time. The fact the next generation of
> forgot to learn from it is unfortunate but hardly new. Also every single
> one of those that exposes a tty in that way allows other annoying
> behaviours via other ioctl interfaces so none of them would have been
> properly mitigated.
> 

This is my point. Apps will continue to shoot themselves in the foot. Of course
the correct response to one of these vulns is to not pass ttys across a
security boundary. We have an opportunity here to reduce the impact of this bug
class at the kernel level. Rejecting this mitigation because the real solution
is to use a tty/pty pair is like saying we should reject ASLR because the real
solution to buffer overflows is proper bounds checking.

> If you really want to do that particular bit of snake oiling then you can
> use the existing SELinux, seccomp and related interfaces. They can even
> do the job properly by whitelisting or blocking long lists of ioctls.
> 
>> This protections seeks to protect users from programs that don't do things
>> correctly. Rather than killing bugs, this feature attempts to kill an entire
>> bug class that shows little sign of slowing down in the world of containers 
>> and
>> sandboxes.
> 
> Well maybe the people writing them need to learn what they are doing and
> stop passing random file descriptors into their container (I've even seen
> people handing X file handles into their 'container').
> 
> The kernel can do some things to help programmers but it can't stop
> people writing crap. Anyone writing code that crosses security boundaries
> should have at least a vague idea of what they are doing.
> 
> The only way you'd actually really prevent this would be to magically
> open a new pty/tty pair and substitute the file handlers plus a data
> copying thread when someone created a namespace.
> 
> Now you can actually do that with the ptrace functionality in seccomp but
> it would still be fairly insane to expect the kernel to handle.
> 
> Alan
> [Actually even more sensible would be to revert the entire sorry
> container mess and use VMs but it's a bit late for that ;-)]
> 

Totally agree. VMs >> Containers but the cat is out of the bag and we can't put
it back.

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Leonard Crestez]

On Tue, 2017-05-30 at 15:19 -0700, Florian Fainelli wrote:
> On 05/30/2017 03:08 PM, Leonard Crestez wrote:
> > On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:

> > > Should not we actually call kszphy_config_init() in order to restore
> > > broadcast and nand disable bits as well?
> > 
> > I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
> > NAND_TREE_ON is already off by the time it gets to linux.
> > 
> > The bit that get lost seem to disappear just as the phy is resumed. I
> > added some prints and they look like this:
> > 
> > PM: early resume of devices complete after 6.534 msecs
> > begin resume
> > 0x1F=0x8190 0x16=0x202
> > after genphy_resume 0x1F=0x8100 0x16=0x202
> > end
> > resume 0x1F=0x8190 0x16=0x202
> 
> OK, so it actually would not hurt to call config_init() again here, right?

No, but if only some registers are lost then why reconfigure others? In
particular it seems that only MII_KSZPHY_CTRL_2 is lost but not
MII_KSZPHY_OMSO.

However a few extra phy reads don't really matter. Calling config_init
is the path of least resistance.

Another problem is that the ksz9031 driver uses kszphy_resume but has a
completely different init function. The bits I am concerned about do
not seem to exist in hardware but it does something completely
unrelated with parsing OF properties. Should I split this into
ksz8021_resume and ksz9031_resume?

It seems likely that more of the kszphy drivers need suspend/resume
code but nobody got around to testing them.

> > > If not, I would be more comfortable if we did create a specific function
> > > that takes care of setting the reference clock and LED mode.
> > 
> > Ok, I can add a function called kszphy_config_reset() with a comment
> > explaining it's for bits lost on reset/resume.
> > 
> > Or perhaps a better option would be to just save/restore the entire
> > 0x1F register?
> 
> Register 0 through 15 are standardized, but those after that are not,
> and PHYs with a gazillion of registers already need to have a specific
> set of suspend/resume sequence due to their proprietary indirect
> register scheme, so we cannot quite come up with a generic function that
> would cache all 16 or 32 registers and flat out restore those.
> 
> Similarly, having phy_resume() systematically calling into config_init()
> is a bit of a stretch and can be pretty inefficient.

I'm not suggesting changing this at the phy core level. Just that maybe
kszphy_resume specifically could save/restore register 0x1F instead of
going through config logic again (which would involve extra reads and
writes).

However it seems that this has complications, for example on some
versions the leds are written to a different register. It's not really
worth optimizing to such an extent.

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Leonard Crestez]

On Tue, 2017-05-30 at 15:19 -0700, Florian Fainelli wrote:
> On 05/30/2017 03:08 PM, Leonard Crestez wrote:
> > On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:

> > > Should not we actually call kszphy_config_init() in order to restore
> > > broadcast and nand disable bits as well?
> > 
> > I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
> > NAND_TREE_ON is already off by the time it gets to linux.
> > 
> > The bit that get lost seem to disappear just as the phy is resumed. I
> > added some prints and they look like this:
> > 
> > PM: early resume of devices complete after 6.534 msecs
> > begin resume
> > 0x1F=0x8190 0x16=0x202
> > after genphy_resume 0x1F=0x8100 0x16=0x202
> > end
> > resume 0x1F=0x8190 0x16=0x202
> 
> OK, so it actually would not hurt to call config_init() again here, right?

No, but if only some registers are lost then why reconfigure others? In
particular it seems that only MII_KSZPHY_CTRL_2 is lost but not
MII_KSZPHY_OMSO.

However a few extra phy reads don't really matter. Calling config_init
is the path of least resistance.

Another problem is that the ksz9031 driver uses kszphy_resume but has a
completely different init function. The bits I am concerned about do
not seem to exist in hardware but it does something completely
unrelated with parsing OF properties. Should I split this into
ksz8021_resume and ksz9031_resume?

It seems likely that more of the kszphy drivers need suspend/resume
code but nobody got around to testing them.

> > > If not, I would be more comfortable if we did create a specific function
> > > that takes care of setting the reference clock and LED mode.
> > 
> > Ok, I can add a function called kszphy_config_reset() with a comment
> > explaining it's for bits lost on reset/resume.
> > 
> > Or perhaps a better option would be to just save/restore the entire
> > 0x1F register?
> 
> Register 0 through 15 are standardized, but those after that are not,
> and PHYs with a gazillion of registers already need to have a specific
> set of suspend/resume sequence due to their proprietary indirect
> register scheme, so we cannot quite come up with a generic function that
> would cache all 16 or 32 registers and flat out restore those.
> 
> Similarly, having phy_resume() systematically calling into config_init()
> is a bit of a stretch and can be pretty inefficient.

I'm not suggesting changing this at the phy core level. Just that maybe
kszphy_resume specifically could save/restore register 0x1F instead of
going through config logic again (which would involve extra reads and
writes).

However it seems that this has complications, for example on some
versions the leds are written to a different register. It's not really
worth optimizing to such an extent.

Re: [PATCH 1/4] dt-bindings: mtk-sysirq: Correct bindings for supported SoCs

[Rob Herring]

On Mon, May 22, 2017 at 11:40:18AM +0200, Matthias Brugger wrote:
> All SoCs supported up to now rely on the fallback binding of mt6577.
> Fix the binding description to reflect this.
> 
> Signed-off-by: Matthias Brugger 
> ---
>  .../interrupt-controller/mediatek,sysirq.txt   | 24 
> +++---
>  1 file changed, 12 insertions(+), 12 deletions(-)

Acked-by: Rob Herring 

Re: [PATCH 1/4] dt-bindings: mtk-sysirq: Correct bindings for supported SoCs

[Rob Herring]

On Mon, May 22, 2017 at 11:40:18AM +0200, Matthias Brugger wrote:
> All SoCs supported up to now rely on the fallback binding of mt6577.
> Fix the binding description to reflect this.
> 
> Signed-off-by: Matthias Brugger 
> ---
>  .../interrupt-controller/mediatek,sysirq.txt   | 24 
> +++---
>  1 file changed, 12 insertions(+), 12 deletions(-)

Acked-by: Rob Herring 

Re: [RFC 2/3] misc: Add w2sg0004 (gps receiver) power control driver

[Rob Herring]

On Sun, May 21, 2017 at 12:44:03PM +0200, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.
> 
> Use serdev API hooks to monitor and forward the UART traffic to /dev/BTn
> and turn on/off the module. It also detects if the module is turned on (sends 
> data)
> but should be off, e.g. if it was already turned on during boot or 
> power-on-reset.
> 
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
> 
> The driver concept is based on code developed by NeilBrown 
> but simplified and adapted to use the new serdev API introduced in 4.11.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/misc/wi2wi,w2sg0004.txt|  20 +
>  .../devicetree/bindings/vendor-prefixes.txt|   1 +

Please split binding patches.

>  drivers/misc/Kconfig   |  16 +
>  drivers/misc/Makefile  |   1 +
>  drivers/misc/w2sg0004.c| 646 
> +
>  include/linux/w2sg0004.h   |  27 +
>  6 files changed, 711 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>  create mode 100644 drivers/misc/w2sg0004.c
>  create mode 100644 include/linux/w2sg0004.h
> 
> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> new file mode 100644
> index ..b7125c7a598c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> @@ -0,0 +1,20 @@
> +Wi2Wi GPS module connected through UART
> +
> +Should be a subnode of the SoC UART it is connected to (serdev).
> +
> +Required properties:
> +- compatible:wi2wi,w2sg0004 or wi2wi,w2sg0084

Reformat to one per line.

> +- on-off-gpio:   the GPIO that controls the module's on-off toggle input

Does enable-gpios or powerdown-gpios work as those are semi-standard 
names. Also, need to state the active state.

> +
> +Optional properties:
> +- lna-suppy: an (optional) LNA regulator that is enabled together with the 
> GPS receiver

typo

> +
> +Example:
> +
> + {
> + gps: w2sg0004 {

w2sg0004: gps {

The node name should be generic. The label can be whatever you like.

> + compatible = "wi2wi,w2sg0004";
> + lna-supply = <>;   /* LNA regulator */
> + on-off-gpios = < 17 GPIO_ACTIVE_HIGH>;/* GPIO_145: 
> trigger for turning on/off w2sg0004 */
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index c03d20140366..c56b3181b266 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -345,6 +345,7 @@ voipacVoipac Technologies s.r.o.
>  wd   Western Digital Corp.
>  wetekWeTek Electronics, limited.
>  wexler   Wexler
> +wi2wiWi2Wi, Inc.
>  winbond Winbond Electronics corp.
>  winstar  Winstar Display Corp.
>  wlf  Wolfson Microelectronics

Re: [RFC 2/3] misc: Add w2sg0004 (gps receiver) power control driver

[Rob Herring]

On Sun, May 21, 2017 at 12:44:03PM +0200, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.
> 
> Use serdev API hooks to monitor and forward the UART traffic to /dev/BTn
> and turn on/off the module. It also detects if the module is turned on (sends 
> data)
> but should be off, e.g. if it was already turned on during boot or 
> power-on-reset.
> 
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
> 
> The driver concept is based on code developed by NeilBrown 
> but simplified and adapted to use the new serdev API introduced in 4.11.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/misc/wi2wi,w2sg0004.txt|  20 +
>  .../devicetree/bindings/vendor-prefixes.txt|   1 +

Please split binding patches.

>  drivers/misc/Kconfig   |  16 +
>  drivers/misc/Makefile  |   1 +
>  drivers/misc/w2sg0004.c| 646 
> +
>  include/linux/w2sg0004.h   |  27 +
>  6 files changed, 711 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>  create mode 100644 drivers/misc/w2sg0004.c
>  create mode 100644 include/linux/w2sg0004.h
> 
> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> new file mode 100644
> index ..b7125c7a598c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
> @@ -0,0 +1,20 @@
> +Wi2Wi GPS module connected through UART
> +
> +Should be a subnode of the SoC UART it is connected to (serdev).
> +
> +Required properties:
> +- compatible:wi2wi,w2sg0004 or wi2wi,w2sg0084

Reformat to one per line.

> +- on-off-gpio:   the GPIO that controls the module's on-off toggle input

Does enable-gpios or powerdown-gpios work as those are semi-standard 
names. Also, need to state the active state.

> +
> +Optional properties:
> +- lna-suppy: an (optional) LNA regulator that is enabled together with the 
> GPS receiver

typo

> +
> +Example:
> +
> + {
> + gps: w2sg0004 {

w2sg0004: gps {

The node name should be generic. The label can be whatever you like.

> + compatible = "wi2wi,w2sg0004";
> + lna-supply = <>;   /* LNA regulator */
> + on-off-gpios = < 17 GPIO_ACTIVE_HIGH>;/* GPIO_145: 
> trigger for turning on/off w2sg0004 */
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index c03d20140366..c56b3181b266 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -345,6 +345,7 @@ voipacVoipac Technologies s.r.o.
>  wd   Western Digital Corp.
>  wetekWeTek Electronics, limited.
>  wexler   Wexler
> +wi2wiWi2Wi, Inc.
>  winbond Winbond Electronics corp.
>  winstar  Winstar Display Corp.
>  wlf  Wolfson Microelectronics

Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

[Bjorn Helgaas]

On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:
> > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:
> > > PCI core sets the driver pointer before calling ->probe() and only
> > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > callback will happily race with probe() and remove(), most likely
> > > leading to BUGs, since drivers don't expect this.  
> > 
> > I guess you're referring to the pci_dev->driver pointer set by
> > local_pci_probe(), and this is important because sriov_numvfs_store()
> > checks that pointer, right?
> 
> Yes, exactly.  I initially thought this is how the safety of sriov
> callback may have been ensured, but since the order of
> local_pci_probe() and the assignment is what it is, it can't.

Right.  I was hoping other subsystems would establish a convention
about whether we set the ->driver pointer before or after calling the
driver probe() method, but if there is one, I don't see it.
local_pci_probe() and really_probe() set ->driver first, but
pnp_device_probe() calls the probe() method first.

Can you expand on how you reproduce this problem?  The only real way I
see to call ->sriov_configure() is via the sysfs entry point, and I
would think user-space code would typically not touch that until after
it knows the driver has claimed a device.  But I can certainly imagine
targeted test code that could hit this problem.

Bjorn

Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

[Bjorn Helgaas]

On Fri, May 26, 2017 at 04:58:20PM -0700, Jakub Kicinski wrote:
> On Fri, 26 May 2017 18:47:26 -0500, Bjorn Helgaas wrote:
> > On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:
> > > PCI core sets the driver pointer before calling ->probe() and only
> > > clears it after ->remove().  This means driver's ->sriov_configure()
> > > callback will happily race with probe() and remove(), most likely
> > > leading to BUGs, since drivers don't expect this.  
> > 
> > I guess you're referring to the pci_dev->driver pointer set by
> > local_pci_probe(), and this is important because sriov_numvfs_store()
> > checks that pointer, right?
> 
> Yes, exactly.  I initially thought this is how the safety of sriov
> callback may have been ensured, but since the order of
> local_pci_probe() and the assignment is what it is, it can't.

Right.  I was hoping other subsystems would establish a convention
about whether we set the ->driver pointer before or after calling the
driver probe() method, but if there is one, I don't see it.
local_pci_probe() and really_probe() set ->driver first, but
pnp_device_probe() calls the probe() method first.

Can you expand on how you reproduce this problem?  The only real way I
see to call ->sriov_configure() is via the sysfs entry point, and I
would think user-space code would typically not touch that until after
it knows the driver has claimed a device.  But I can certainly imagine
targeted test code that could hit this problem.

Bjorn

Re: [PATCH] LSM: Convert security_hook_heads into explicit array of struct list_head

[James Morris]

On Tue, 30 May 2017, Alan Cox wrote:

> On Tue, 30 May 2017 23:29:10 +0900
> Tetsuo Handa  wrote:
> 
> > James Morris wrote:
> > > On Sun, 28 May 2017, Tetsuo Handa wrote:
> > >   
> > > > can afford enabling". And we know that we cannot merge all security 
> > > > modules
> > > > into mainline. Thus, allowing LKM-based LSM modules is inevitable.  
> > > 
> > > Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> > > 
> > > I'm not sure why you persist against this.  
> > 
> > Then, we are willing to accept LSM modules with users less than 10, aren't 
> > we?
> 
> Why not if they are properly written and maintained. Historically we've
> supported an entire architecture that had one machine ever built. We
> supported a strange subclass of x86 machines for many years because James
> Bottomley cared enough to do the work. We still support M68K, PA-RISC and
> other stuff as well as plenty of hardware which probably has few users -
> providing it doesn't cause maintenance problems.

This is what we as a community came up with in 2008:

  "Essentially, any new security project—not limited to LSMs—should be 
   accompanied by a clear and concise document outlining its requirements 
   and expected uses. This is to allow both security and regular folk to 
   perform review of the code in terms of how it meets the specified 
   requirements, and to avoid getting bogged down in unresolvable 
   discussions about the project’s security model."

>From https://blog.namei.org/2008/12/11/the-arjan-protocol/

(Not sure if the original document at Kernel Trap still exists, alas).

So what we need is clear design documentation that the code can be 
reviewed against.  There is nothing about the number of users.  If the 
code is simply using the existing API and meets its design goals, it's 
pretty straightforward.  If changes to the API or core kernel are also 
required, then more discussion and review will be needed.




- James
-- 
James Morris 

Re: [PATCH] LSM: Convert security_hook_heads into explicit array of struct list_head

[James Morris]

On Tue, 30 May 2017, Alan Cox wrote:

> On Tue, 30 May 2017 23:29:10 +0900
> Tetsuo Handa  wrote:
> 
> > James Morris wrote:
> > > On Sun, 28 May 2017, Tetsuo Handa wrote:
> > >   
> > > > can afford enabling". And we know that we cannot merge all security 
> > > > modules
> > > > into mainline. Thus, allowing LKM-based LSM modules is inevitable.  
> > > 
> > > Nope, it's not inevitable.  The LSM API only caters to in-tree users.
> > > 
> > > I'm not sure why you persist against this.  
> > 
> > Then, we are willing to accept LSM modules with users less than 10, aren't 
> > we?
> 
> Why not if they are properly written and maintained. Historically we've
> supported an entire architecture that had one machine ever built. We
> supported a strange subclass of x86 machines for many years because James
> Bottomley cared enough to do the work. We still support M68K, PA-RISC and
> other stuff as well as plenty of hardware which probably has few users -
> providing it doesn't cause maintenance problems.

This is what we as a community came up with in 2008:

  "Essentially, any new security project—not limited to LSMs—should be 
   accompanied by a clear and concise document outlining its requirements 
   and expected uses. This is to allow both security and regular folk to 
   perform review of the code in terms of how it meets the specified 
   requirements, and to avoid getting bogged down in unresolvable 
   discussions about the project’s security model."

>From https://blog.namei.org/2008/12/11/the-arjan-protocol/

(Not sure if the original document at Kernel Trap still exists, alas).

So what we need is clear design documentation that the code can be 
reviewed against.  There is nothing about the number of users.  If the 
code is simply using the existing API and meets its design goals, it's 
pretty straightforward.  If changes to the API or core kernel are also 
required, then more discussion and review will be needed.




- James
-- 
James Morris 

Re: [PATCH v5 08/10] x86/hyper-v: use hypercall for remote TLB flush

[Stephen Hemminger]

On Tue, 30 May 2017 19:17:46 +
Jork Loeser  wrote:

> > -Original Message-
> > From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> > Sent: Tuesday, May 30, 2017 09:53
> > To: Vitaly Kuznetsov 
> > Cc: x...@kernel.org; de...@linuxdriverproject.org; linux-
> > ker...@vger.kernel.org; KY Srinivasan ; Haiyang Zhang
> > ; Stephen Hemminger ;
> > Thomas Gleixner ; Ingo Molnar ; H.
> > Peter Anvin ; Steven Rostedt ; Jork
> > Loeser ; Simon Xiao ;
> > Andy Lutomirski 
> > Subject: Re: [PATCH v5 08/10] x86/hyper-v: use hypercall for remote TLB 
> > flush
> > 
> > On Tue, May 30, 2017 at 2:34 PM, Vitaly Kuznetsov 
> > wrote:  
> > > +#define HV_FLUSH_ALL_PROCESSORS0x0001
> > > +#define HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES0x0002
> > > +#define HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY  0x0004
> > > +#define HV_FLUSH_USE_EXTENDED_RANGE_FORMAT 0x0008  
> > 
> > BIT() ?  
> 
> Certainly a matter of taste. Given that the Hyper-V spec lists these as hex 
> numbers, I find the explicit numbers appropriate.
> 
> Regards,
> Jork

Keep the hex numbers, it makes more sense not to change it since rest of 
arch/x86/hyperv
uses hex values.

Re: [PATCH v5 08/10] x86/hyper-v: use hypercall for remote TLB flush

[Stephen Hemminger]

On Tue, 30 May 2017 19:17:46 +
Jork Loeser  wrote:

> > -Original Message-
> > From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> > Sent: Tuesday, May 30, 2017 09:53
> > To: Vitaly Kuznetsov 
> > Cc: x...@kernel.org; de...@linuxdriverproject.org; linux-
> > ker...@vger.kernel.org; KY Srinivasan ; Haiyang Zhang
> > ; Stephen Hemminger ;
> > Thomas Gleixner ; Ingo Molnar ; H.
> > Peter Anvin ; Steven Rostedt ; Jork
> > Loeser ; Simon Xiao ;
> > Andy Lutomirski 
> > Subject: Re: [PATCH v5 08/10] x86/hyper-v: use hypercall for remote TLB 
> > flush
> > 
> > On Tue, May 30, 2017 at 2:34 PM, Vitaly Kuznetsov 
> > wrote:  
> > > +#define HV_FLUSH_ALL_PROCESSORS0x0001
> > > +#define HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES0x0002
> > > +#define HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY  0x0004
> > > +#define HV_FLUSH_USE_EXTENDED_RANGE_FORMAT 0x0008  
> > 
> > BIT() ?  
> 
> Certainly a matter of taste. Given that the Hyper-V spec lists these as hex 
> numbers, I find the explicit numbers appropriate.
> 
> Regards,
> Jork

Keep the hex numbers, it makes more sense not to change it since rest of 
arch/x86/hyperv
uses hex values.

Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Mon, 29 May 2017, jeffy.chen wrote:
> i think if we want to make all irq enable/disable balance, maybe we can:
> 
> 1/ only call irq_enable/disable from enable/disable_irq(change other
> irq_enable/disable to enable/disable_irq), so they would be protected by the
> refcnt(deph)

You cannot call enable/disable_irq() from places which call
irq_enable/disable() due to locking reasons.

__disable_irq()/__enable_irq() can/must be called with desc->lock held, but
__enable_irq() does more than just calling irq_enable().

So no, it's not that simple.

> 2/ disable lazy stuff in irq_shoutdown(we already did this in free_irq)

No, irq_shutdown() is called from other places as well. 

> 3/ in irq_shutdown, set depth to 0 if it's not disabled and masked(for lazy 
> stuff)
> before calling disable_irq, 

Uurgh, no. That's a unholy hack.

So what should be done to fix this is to make consequent use of the state bits.

 irq_disable()
if (irqd_irq_disabled())
   return;
irqd_set_irq_disabled();


This should be done for both mask/unmask disable/enable. You get the idea.

We probably want a third state bit for STARTED_UP and do the same dance in
startup/shutdown as well. Which brings me to a different issue, which is
outside the scope of your problem, but looking at the code made me find it.

If a interrupt is marked IRQ_NOAUTOEN then request_irq() will not invoke
irq_startup(). The interrupt is just completely set up, but stays disabled.
It is enabled later via enable_irq(). That works so far with no complaints,
but there is an interesting twist:

In that NOAUTOEN case nothing ever calls irq_startup() on that irq, which
means that in case the irq_chip has a irq_startup() callback nothing will
invoke it and also irq_domain_activate_irq() will never be invoked on such
an irq.

Looks like all implementations which use IRQ_NOAUTOEN are not sensitive to
that. It's been broken forever.

Fixing this needs the extra state bit IRQD_ STARTED_UP as we cannot reuse
the IRQD_ACTIVATED bit because some of the interrupts are actually
activated before they are requested.

Too tired to fix that now, but I'll have a look tomorrow. Once this is
fixed, you can add the extra bits to prevent this disable/enable calls
which cause the imbalance deeper down.

Thanks,

tglx

Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Mon, 29 May 2017, jeffy.chen wrote:
> i think if we want to make all irq enable/disable balance, maybe we can:
> 
> 1/ only call irq_enable/disable from enable/disable_irq(change other
> irq_enable/disable to enable/disable_irq), so they would be protected by the
> refcnt(deph)

You cannot call enable/disable_irq() from places which call
irq_enable/disable() due to locking reasons.

__disable_irq()/__enable_irq() can/must be called with desc->lock held, but
__enable_irq() does more than just calling irq_enable().

So no, it's not that simple.

> 2/ disable lazy stuff in irq_shoutdown(we already did this in free_irq)

No, irq_shutdown() is called from other places as well. 

> 3/ in irq_shutdown, set depth to 0 if it's not disabled and masked(for lazy 
> stuff)
> before calling disable_irq, 

Uurgh, no. That's a unholy hack.

So what should be done to fix this is to make consequent use of the state bits.

 irq_disable()
if (irqd_irq_disabled())
   return;
irqd_set_irq_disabled();


This should be done for both mask/unmask disable/enable. You get the idea.

We probably want a third state bit for STARTED_UP and do the same dance in
startup/shutdown as well. Which brings me to a different issue, which is
outside the scope of your problem, but looking at the code made me find it.

If a interrupt is marked IRQ_NOAUTOEN then request_irq() will not invoke
irq_startup(). The interrupt is just completely set up, but stays disabled.
It is enabled later via enable_irq(). That works so far with no complaints,
but there is an interesting twist:

In that NOAUTOEN case nothing ever calls irq_startup() on that irq, which
means that in case the irq_chip has a irq_startup() callback nothing will
invoke it and also irq_domain_activate_irq() will never be invoked on such
an irq.

Looks like all implementations which use IRQ_NOAUTOEN are not sensitive to
that. It's been broken forever.

Fixing this needs the extra state bit IRQD_ STARTED_UP as we cannot reuse
the IRQD_ACTIVATED bit because some of the interrupts are actually
activated before they are requested.

Too tired to fix that now, but I'll have a look tomorrow. Once this is
fixed, you can add the extra bits to prevent this disable/enable calls
which cause the imbalance deeper down.

Thanks,

tglx

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 4:22 PM, Daniel Micay wrote:
>> Thanks, I didn't know that android was doing this. I still think this
>> feature
>> is worthwhile for people to be able to harden their systems against
>> this attack
>> vector without having to implement a MAC.
> 
> Since there's a capable LSM hook for ioctl already, it means it could go
> in Yama with ptrace_scope but core kernel code would still need to be
> changed to track the owning tty. I think Yama vs. core kernel shouldn't
> matter much anymore due to stackable LSMs.
> 

What does everyone think about a v8 that moves this feature under Yama and uses
the file_ioctl LSM hook?

> Not the case for perf_event_paranoid=3 where a) there's already a sysctl
> exposed which would be unfortunate to duplicate, b) there isn't an LSM
> hook yet (AFAIK).
> 
> The toggles for ptrace and perf events are more useful though since
> they're very commonly used debugging features vs. this obscure, rarely
> used ioctl that in practice no one will notice is missing. It's still
> friendlier to have a toggle than a seccomp policy requiring a reboot to
> get rid of it, or worse compiling it out of the kernel.
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 4:22 PM, Daniel Micay wrote:
>> Thanks, I didn't know that android was doing this. I still think this
>> feature
>> is worthwhile for people to be able to harden their systems against
>> this attack
>> vector without having to implement a MAC.
> 
> Since there's a capable LSM hook for ioctl already, it means it could go
> in Yama with ptrace_scope but core kernel code would still need to be
> changed to track the owning tty. I think Yama vs. core kernel shouldn't
> matter much anymore due to stackable LSMs.
> 

What does everyone think about a v8 that moves this feature under Yama and uses
the file_ioctl LSM hook?

> Not the case for perf_event_paranoid=3 where a) there's already a sysctl
> exposed which would be unfortunate to duplicate, b) there isn't an LSM
> hook yet (AFAIK).
> 
> The toggles for ptrace and perf events are more useful though since
> they're very commonly used debugging features vs. this obscure, rarely
> used ioctl that in practice no one will notice is missing. It's still
> friendlier to have a toggle than a seccomp policy requiring a reboot to
> get rid of it, or worse compiling it out of the kernel.
> 

Re: [PATCH 6/7] thermal: max77620: fix device-node reference imbalance

[Tyrel Datwyler]

On 05/30/2017 09:25 AM, Johan Hovold wrote:
> The thermal child device reuses the parent MFD-device device-tree node
> when registering a thermal zone, but did not take a reference to the
> node.
> 
> This leads to a reference imbalance, and potential use-after-free, when
> the node reference is dropped by the platform-bus device destructor
> (once for the child and later again for the parent).
> 
> Fix this by dropping any reference already held to a device-tree node
> and getting a reference to the parent's node which will be balanced on
> reprobe or on platform-device release, whichever comes first.
> 
> Note that simply clearing the of_node pointer on probe errors and on
> driver unbind would not allow the use of device-managed resources as
> specifically thermal_zone_of_sensor_unregister() claims that a valid
> device-tree node pointer is needed during deregistration (even if it
> currently does not seem to use it).
> 
> Fixes: ec4664b3fd6d ("thermal: max77620: Add thermal driver for reporting 
> junction temp")
> Cc: stable  # 4.9
> Cc: Laxman Dewangan 
> Signed-off-by: Johan Hovold 
> ---
>  drivers/thermal/max77620_thermal.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/max77620_thermal.c 
> b/drivers/thermal/max77620_thermal.c
> index e9a1fe342760..71d35f3c9215 100644
> --- a/drivers/thermal/max77620_thermal.c
> +++ b/drivers/thermal/max77620_thermal.c
> @@ -104,8 +104,6 @@ static int max77620_thermal_probe(struct platform_device 
> *pdev)
>   return -EINVAL;
>   }
>  
> - pdev->dev.of_node = pdev->dev.parent->of_node;
> -
>   mtherm->dev = >dev;
>   mtherm->rmap = dev_get_regmap(pdev->dev.parent, NULL);
>   if (!mtherm->rmap) {
> @@ -113,6 +111,14 @@ static int max77620_thermal_probe(struct platform_device 
> *pdev)
>   return -ENODEV;
>   }
>  
> + /*
> +  * Drop any current reference to a device-tree node and get a
> +  * reference to the parent's node which will be balanced on reprobe or
> +  * on platform-device release.
> +  */
> + of_node_put(pdev->dev.of_node);
> + pdev->dev.of_node = of_node_get(pdev->dev.parent->of_node);
> +

This seems like needless churn. Can't this just be squashed into patch #7?

-Tyrel

>   mtherm->tz_device = devm_thermal_zone_of_sensor_register(>dev, 0,
>   mtherm, _thermal_ops);
>   if (IS_ERR(mtherm->tz_device)) {
> 

Re: [PATCH 6/7] thermal: max77620: fix device-node reference imbalance

[Tyrel Datwyler]

On 05/30/2017 09:25 AM, Johan Hovold wrote:
> The thermal child device reuses the parent MFD-device device-tree node
> when registering a thermal zone, but did not take a reference to the
> node.
> 
> This leads to a reference imbalance, and potential use-after-free, when
> the node reference is dropped by the platform-bus device destructor
> (once for the child and later again for the parent).
> 
> Fix this by dropping any reference already held to a device-tree node
> and getting a reference to the parent's node which will be balanced on
> reprobe or on platform-device release, whichever comes first.
> 
> Note that simply clearing the of_node pointer on probe errors and on
> driver unbind would not allow the use of device-managed resources as
> specifically thermal_zone_of_sensor_unregister() claims that a valid
> device-tree node pointer is needed during deregistration (even if it
> currently does not seem to use it).
> 
> Fixes: ec4664b3fd6d ("thermal: max77620: Add thermal driver for reporting 
> junction temp")
> Cc: stable  # 4.9
> Cc: Laxman Dewangan 
> Signed-off-by: Johan Hovold 
> ---
>  drivers/thermal/max77620_thermal.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/max77620_thermal.c 
> b/drivers/thermal/max77620_thermal.c
> index e9a1fe342760..71d35f3c9215 100644
> --- a/drivers/thermal/max77620_thermal.c
> +++ b/drivers/thermal/max77620_thermal.c
> @@ -104,8 +104,6 @@ static int max77620_thermal_probe(struct platform_device 
> *pdev)
>   return -EINVAL;
>   }
>  
> - pdev->dev.of_node = pdev->dev.parent->of_node;
> -
>   mtherm->dev = >dev;
>   mtherm->rmap = dev_get_regmap(pdev->dev.parent, NULL);
>   if (!mtherm->rmap) {
> @@ -113,6 +111,14 @@ static int max77620_thermal_probe(struct platform_device 
> *pdev)
>   return -ENODEV;
>   }
>  
> + /*
> +  * Drop any current reference to a device-tree node and get a
> +  * reference to the parent's node which will be balanced on reprobe or
> +  * on platform-device release.
> +  */
> + of_node_put(pdev->dev.of_node);
> + pdev->dev.of_node = of_node_get(pdev->dev.parent->of_node);
> +

This seems like needless churn. Can't this just be squashed into patch #7?

-Tyrel

>   mtherm->tz_device = devm_thermal_zone_of_sensor_register(>dev, 0,
>   mtherm, _thermal_ops);
>   if (IS_ERR(mtherm->tz_device)) {
>