Re: OOM detection regressions since 4.7

[Arkadiusz Miskiewicz]

On Thursday 25 of August 2016, Michal Hocko wrote:
> On Tue 23-08-16 09:43:39, Michal Hocko wrote:
> > On Mon 22-08-16 15:05:17, Andrew Morton wrote:
> > > On Mon, 22 Aug 2016 15:42:28 +0200 Michal Hocko  
wrote:
> > > > Of course, if Linus/Andrew doesn't like to take those compaction
> > > > improvements this late then I will ask to merge the partial revert to
> > > > Linus tree as well and then there is not much to discuss.
> > > 
> > > This sounds like the prudent option.  Can we get 4.8 working
> > > well-enough, backport that into 4.7.x and worry about the fancier stuff
> > > for 4.9?
> > 
> > OK, fair enough.
> > 
> > I would really appreciate if the original reporters could retest with
> > this patch on top of the current Linus tree.
> 
> Any luck with the testing of this patch?

Here my "rm -rf && cp -al" 10x in parallel test finished without OOM, so

Tested-by: Arkadiusz Miśkiewicz 

-- 
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

Re: OOM detection regressions since 4.7

[Arkadiusz Miskiewicz]

On Thursday 25 of August 2016, Michal Hocko wrote:
> On Tue 23-08-16 09:43:39, Michal Hocko wrote:
> > On Mon 22-08-16 15:05:17, Andrew Morton wrote:
> > > On Mon, 22 Aug 2016 15:42:28 +0200 Michal Hocko  
wrote:
> > > > Of course, if Linus/Andrew doesn't like to take those compaction
> > > > improvements this late then I will ask to merge the partial revert to
> > > > Linus tree as well and then there is not much to discuss.
> > > 
> > > This sounds like the prudent option.  Can we get 4.8 working
> > > well-enough, backport that into 4.7.x and worry about the fancier stuff
> > > for 4.9?
> > 
> > OK, fair enough.
> > 
> > I would really appreciate if the original reporters could retest with
> > this patch on top of the current Linus tree.
> 
> Any luck with the testing of this patch?

Here my "rm -rf && cp -al" 10x in parallel test finished without OOM, so

Tested-by: Arkadiusz Miśkiewicz 

-- 
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

[PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions

[chengang]

From: Chen Gang 

Also use the same changing to asm-generic, and also use bool variable
instead of int variable for mips, mn10300, parisc and tile related
functions, and also avoid checkpatch.pl to report ERROR.

Originally, except powerpc and xtensa, all another architectures intend
to return 0 or 1. After this patch, also let powerpc and xtensa return 0
or 1.

The patch passes cross building for mips and parisc with default config.
All related contents are found by "grep test_bit, grep test_and" under
arch sub-directory.

Signed-off-by: Chen Gang 
---
 arch/alpha/include/asm/bitops.h | 16 
 arch/arc/include/asm/bitops.h   | 10 +-
 arch/arm/include/asm/bitops.h   | 12 ++--
 arch/arm64/include/asm/bitops.h |  6 +++---
 arch/avr32/include/asm/bitops.h |  6 +++---
 arch/blackfin/include/asm/bitops.h  | 16 
 arch/frv/include/asm/bitops.h   | 16 
 arch/h8300/include/asm/bitops.h |  4 ++--
 arch/hexagon/include/asm/bitops.h   | 14 +++---
 arch/ia64/include/asm/bitops.h  | 14 +++---
 arch/m32r/include/asm/bitops.h  |  6 +++---
 arch/m68k/include/asm/bitops.h  | 20 ++--
 arch/metag/include/asm/bitops.h |  6 +++---
 arch/mips/include/asm/bitops.h  | 16 
 arch/mips/lib/bitops.c  | 16 
 arch/mn10300/include/asm/bitops.h   |  7 ---
 arch/parisc/include/asm/bitops.h| 16 
 arch/powerpc/include/asm/bitops.h   | 10 +-
 arch/s390/include/asm/bitops.h  | 18 +-
 arch/sh/include/asm/bitops-cas.h|  6 +++---
 arch/sh/include/asm/bitops-grb.h|  6 +++---
 arch/sh/include/asm/bitops-llsc.h   |  6 +++---
 arch/sh/include/asm/bitops-op32.h   |  8 
 arch/sparc/include/asm/bitops_32.h  |  6 +++---
 arch/sparc/include/asm/bitops_64.h  |  6 +++---
 arch/tile/include/asm/bitops_32.h   |  6 +++---
 arch/tile/include/asm/bitops_64.h   | 10 +-
 arch/xtensa/include/asm/bitops.h|  6 +++---
 include/asm-generic/bitops/atomic.h |  6 +++---
 include/asm-generic/bitops/le.h | 10 +-
 include/asm-generic/bitops/non-atomic.h |  8 
 31 files changed, 157 insertions(+), 156 deletions(-)

diff --git a/arch/alpha/include/asm/bitops.h b/arch/alpha/include/asm/bitops.h
index 4bdfbd4..92d468f 100644
--- a/arch/alpha/include/asm/bitops.h
+++ b/arch/alpha/include/asm/bitops.h
@@ -125,7 +125,7 @@ __change_bit(unsigned long nr, volatile void * addr)
*m ^= 1 << (nr & 31);
 }
 
-static inline int
+static inline bool
 test_and_set_bit(unsigned long nr, volatile void *addr)
 {
unsigned long oldbit;
@@ -155,7 +155,7 @@ test_and_set_bit(unsigned long nr, volatile void *addr)
return oldbit != 0;
 }
 
-static inline int
+static inline bool
 test_and_set_bit_lock(unsigned long nr, volatile void *addr)
 {
unsigned long oldbit;
@@ -185,7 +185,7 @@ test_and_set_bit_lock(unsigned long nr, volatile void *addr)
 /*
  * WARNING: non atomic version.
  */
-static inline int
+static inline bool
 __test_and_set_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -196,7 +196,7 @@ __test_and_set_bit(unsigned long nr, volatile void * addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_and_clear_bit(unsigned long nr, volatile void * addr)
 {
unsigned long oldbit;
@@ -229,7 +229,7 @@ test_and_clear_bit(unsigned long nr, volatile void * addr)
 /*
  * WARNING: non atomic version.
  */
-static inline int
+static inline bool
 __test_and_clear_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -240,7 +240,7 @@ __test_and_clear_bit(unsigned long nr, volatile void * addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_and_change_bit(unsigned long nr, volatile void * addr)
 {
unsigned long oldbit;
@@ -271,7 +271,7 @@ test_and_change_bit(unsigned long nr, volatile void * addr)
 /*
  * WARNING: non atomic version.
  */
-static __inline__ int
+static __inline__ bool
 __test_and_change_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -282,7 +282,7 @@ __test_and_change_bit(unsigned long nr, volatile void * 
addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_bit(int nr, const volatile void * addr)
 {
return (1UL & (((const int *) addr)[nr >> 5] >> (nr & 31))) != 0UL;
diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
index 8da87fee..e1976ab 100644
--- a/arch/arc/include/asm/bitops.h
+++ b/arch/arc/include/asm/bitops.h
@@ -60,7 +60,7 @@ static inline void op##_bit(unsigned long nr, volatile 
unsigned long *m)\
  * and 

[PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions

[chengang]

From: Chen Gang 

Also use the same changing to asm-generic, and also use bool variable
instead of int variable for mips, mn10300, parisc and tile related
functions, and also avoid checkpatch.pl to report ERROR.

Originally, except powerpc and xtensa, all another architectures intend
to return 0 or 1. After this patch, also let powerpc and xtensa return 0
or 1.

The patch passes cross building for mips and parisc with default config.
All related contents are found by "grep test_bit, grep test_and" under
arch sub-directory.

Signed-off-by: Chen Gang 
---
 arch/alpha/include/asm/bitops.h | 16 
 arch/arc/include/asm/bitops.h   | 10 +-
 arch/arm/include/asm/bitops.h   | 12 ++--
 arch/arm64/include/asm/bitops.h |  6 +++---
 arch/avr32/include/asm/bitops.h |  6 +++---
 arch/blackfin/include/asm/bitops.h  | 16 
 arch/frv/include/asm/bitops.h   | 16 
 arch/h8300/include/asm/bitops.h |  4 ++--
 arch/hexagon/include/asm/bitops.h   | 14 +++---
 arch/ia64/include/asm/bitops.h  | 14 +++---
 arch/m32r/include/asm/bitops.h  |  6 +++---
 arch/m68k/include/asm/bitops.h  | 20 ++--
 arch/metag/include/asm/bitops.h |  6 +++---
 arch/mips/include/asm/bitops.h  | 16 
 arch/mips/lib/bitops.c  | 16 
 arch/mn10300/include/asm/bitops.h   |  7 ---
 arch/parisc/include/asm/bitops.h| 16 
 arch/powerpc/include/asm/bitops.h   | 10 +-
 arch/s390/include/asm/bitops.h  | 18 +-
 arch/sh/include/asm/bitops-cas.h|  6 +++---
 arch/sh/include/asm/bitops-grb.h|  6 +++---
 arch/sh/include/asm/bitops-llsc.h   |  6 +++---
 arch/sh/include/asm/bitops-op32.h   |  8 
 arch/sparc/include/asm/bitops_32.h  |  6 +++---
 arch/sparc/include/asm/bitops_64.h  |  6 +++---
 arch/tile/include/asm/bitops_32.h   |  6 +++---
 arch/tile/include/asm/bitops_64.h   | 10 +-
 arch/xtensa/include/asm/bitops.h|  6 +++---
 include/asm-generic/bitops/atomic.h |  6 +++---
 include/asm-generic/bitops/le.h | 10 +-
 include/asm-generic/bitops/non-atomic.h |  8 
 31 files changed, 157 insertions(+), 156 deletions(-)

diff --git a/arch/alpha/include/asm/bitops.h b/arch/alpha/include/asm/bitops.h
index 4bdfbd4..92d468f 100644
--- a/arch/alpha/include/asm/bitops.h
+++ b/arch/alpha/include/asm/bitops.h
@@ -125,7 +125,7 @@ __change_bit(unsigned long nr, volatile void * addr)
*m ^= 1 << (nr & 31);
 }
 
-static inline int
+static inline bool
 test_and_set_bit(unsigned long nr, volatile void *addr)
 {
unsigned long oldbit;
@@ -155,7 +155,7 @@ test_and_set_bit(unsigned long nr, volatile void *addr)
return oldbit != 0;
 }
 
-static inline int
+static inline bool
 test_and_set_bit_lock(unsigned long nr, volatile void *addr)
 {
unsigned long oldbit;
@@ -185,7 +185,7 @@ test_and_set_bit_lock(unsigned long nr, volatile void *addr)
 /*
  * WARNING: non atomic version.
  */
-static inline int
+static inline bool
 __test_and_set_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -196,7 +196,7 @@ __test_and_set_bit(unsigned long nr, volatile void * addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_and_clear_bit(unsigned long nr, volatile void * addr)
 {
unsigned long oldbit;
@@ -229,7 +229,7 @@ test_and_clear_bit(unsigned long nr, volatile void * addr)
 /*
  * WARNING: non atomic version.
  */
-static inline int
+static inline bool
 __test_and_clear_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -240,7 +240,7 @@ __test_and_clear_bit(unsigned long nr, volatile void * addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_and_change_bit(unsigned long nr, volatile void * addr)
 {
unsigned long oldbit;
@@ -271,7 +271,7 @@ test_and_change_bit(unsigned long nr, volatile void * addr)
 /*
  * WARNING: non atomic version.
  */
-static __inline__ int
+static __inline__ bool
 __test_and_change_bit(unsigned long nr, volatile void * addr)
 {
unsigned long mask = 1 << (nr & 0x1f);
@@ -282,7 +282,7 @@ __test_and_change_bit(unsigned long nr, volatile void * 
addr)
return (old & mask) != 0;
 }
 
-static inline int
+static inline bool
 test_bit(int nr, const volatile void * addr)
 {
return (1UL & (((const int *) addr)[nr >> 5] >> (nr & 31))) != 0UL;
diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
index 8da87fee..e1976ab 100644
--- a/arch/arc/include/asm/bitops.h
+++ b/arch/arc/include/asm/bitops.h
@@ -60,7 +60,7 @@ static inline void op##_bit(unsigned long nr, volatile 
unsigned long *m)\
  * and the old value of bit is returned
  */
 #define 

Re: [PATCH] fscrypto: fix to null-terminate encrypted filename in fname_encrypt

[Theodore Ts'o]

On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
> From: Chao Yu 
> 
> This patch fixes to add null character at the end of encrypted filename
> in fname_encrypt, in order to avoid incorrectly traversing random data
> located after target filename. The call stack is as below:
> 
> - f2fs_add_link
>  - __f2fs_add_link
>   - fscrypt_setup_filename
>- fscrypt_fname_alloc_buffer   allocate buffer for @fname
>- fname_encryptdidn't set null character for @fname
>   - f2fs_add_regular_entryinit qstr with @fname
>- init_inode_metadata
> - f2fs_init_security
>  - security_inode_init_security
>   - selinux_inode_init_security
>- selinux_determine_inode_label
> - security_transition_sid
>- security_compute_sid
> - filename_compute_type
>  - hashtab_search
>   - filenametr_hash   traverse @fname as one which has null 
> character

The problem is not in fname_encrypt(), but rather that
security_inode_init_security() should be given the _unencrypted_
filename.

In ext4 security_inode_init_security() is called with the qstr from
the dentry, not the encrypted qstr --- in fact we call
security_inode_init_security before we call fname_encrypt.

SELinux needs the unencrypted filename in order to decide which
SELinux rules / labels should apply.

- Ted

Re: [PATCH] fscrypto: fix to null-terminate encrypted filename in fname_encrypt

[Theodore Ts'o]

On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
> From: Chao Yu 
> 
> This patch fixes to add null character at the end of encrypted filename
> in fname_encrypt, in order to avoid incorrectly traversing random data
> located after target filename. The call stack is as below:
> 
> - f2fs_add_link
>  - __f2fs_add_link
>   - fscrypt_setup_filename
>- fscrypt_fname_alloc_buffer   allocate buffer for @fname
>- fname_encryptdidn't set null character for @fname
>   - f2fs_add_regular_entryinit qstr with @fname
>- init_inode_metadata
> - f2fs_init_security
>  - security_inode_init_security
>   - selinux_inode_init_security
>- selinux_determine_inode_label
> - security_transition_sid
>- security_compute_sid
> - filename_compute_type
>  - hashtab_search
>   - filenametr_hash   traverse @fname as one which has null 
> character

The problem is not in fname_encrypt(), but rather that
security_inode_init_security() should be given the _unencrypted_
filename.

In ext4 security_inode_init_security() is called with the qstr from
the dentry, not the encrypted qstr --- in fact we call
security_inode_init_security before we call fname_encrypt.

SELinux needs the unencrypted filename in order to decide which
SELinux rules / labels should apply.

- Ted

Re: [PATCH 1/3] checkkconfigsymbols: port to python3

[Valentin Rothberg]

On Sat, Aug 27, 2016 at 7:53 PM, Greg KH  wrote:
> On Sat, Aug 27, 2016 at 05:25:29PM +0200, Valentin Rothberg wrote:
>> Port the script to python3 and fix some pylint warnings.
>
> "and" in a changelog description usually means this should be 2
> different patches.
>
> Can't you fix up the pylint warnings in a separate patch?

Sure.  I will prepare a new version of the patchset.

Thanks,
 Valentin

> thanks,
>
> greg k-h

Re: [PATCH 1/3] checkkconfigsymbols: port to python3

[Valentin Rothberg]

On Sat, Aug 27, 2016 at 7:53 PM, Greg KH  wrote:
> On Sat, Aug 27, 2016 at 05:25:29PM +0200, Valentin Rothberg wrote:
>> Port the script to python3 and fix some pylint warnings.
>
> "and" in a changelog description usually means this should be 2
> different patches.
>
> Can't you fix up the pylint warnings in a separate patch?

Sure.  I will prepare a new version of the patchset.

Thanks,
 Valentin

> thanks,
>
> greg k-h

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Wolfram Sang]

> Making a mess of I2C controllers seems to be a popular hobby among
> chip designers :P

Well, I2C is simple, what could go wrong? :/

> A lot of the details (including the completely bizarre behaviour of
> its innocuous-looking irq registers) would be quite non-trivial to
> figure out without putting in a similar effort.

Thanks. So, it is possible to make a proper I2C slave with OMAP, but you
need to know those 100 gory details?

   Wolfram



signature.asc
Description: PGP signature

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Wolfram Sang]

> Making a mess of I2C controllers seems to be a popular hobby among
> chip designers :P

Well, I2C is simple, what could go wrong? :/

> A lot of the details (including the completely bizarre behaviour of
> its innocuous-looking irq registers) would be quite non-trivial to
> figure out without putting in a similar effort.

Thanks. So, it is possible to make a proper I2C slave with OMAP, but you
need to know those 100 gory details?

   Wolfram



signature.asc
Description: PGP signature

[PATCH] fix:memory:omap-gpmc:mark symbols static where possible

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/memory/omap-gpmc.c:354:14: warning: no previous prototype for 
'gpmc_clk_ticks_to_ns' [-Wmissing-prototypes]

In fact, this function is only used in the file in which it is declared
and don't need a declaration, but can be made static.
so this patch marks it 'static'.

Signed-off-by: Baoyou Xie 
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 869c83f..f71cbf7 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -350,7 +350,7 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
 }
 
-unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs,
+static unsigned int gpmc_clk_ticks_to_ns(unsigned int ticks, int cs,
  enum gpmc_clk_domain cd)
 {
return ticks * gpmc_get_clk_period(cs, cd) / 1000;
-- 
2.7.4

[PATCH] fix:memory:omap-gpmc:mark symbols static where possible

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/memory/omap-gpmc.c:354:14: warning: no previous prototype for 
'gpmc_clk_ticks_to_ns' [-Wmissing-prototypes]

In fact, this function is only used in the file in which it is declared
and don't need a declaration, but can be made static.
so this patch marks it 'static'.

Signed-off-by: Baoyou Xie 
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 869c83f..f71cbf7 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -350,7 +350,7 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
 }
 
-unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs,
+static unsigned int gpmc_clk_ticks_to_ns(unsigned int ticks, int cs,
  enum gpmc_clk_domain cd)
 {
return ticks * gpmc_get_clk_period(cs, cd) / 1000;
-- 
2.7.4

[PATCH] fix:overlay: add missing header dependencies

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/gpu/drm/nouveau/dispnv04/overlay.c:496:1: warning: no previous 
prototype for 'nouveau_overlay_init' [-Wmissing-prototypes]

In fact, this function is declared in disp.h, so this patch
add missing header dependencies

Signed-off-by: Baoyou Xie 
---
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c 
b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
index ec444ea..a79514d 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
@@ -33,7 +33,7 @@
 #include "nouveau_connector.h"
 #include "nouveau_display.h"
 #include "nvreg.h"
-
+#include "disp.h"
 
 struct nouveau_plane {
struct drm_plane base;
-- 
2.7.4

[PATCH] fix:overlay: add missing header dependencies

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/gpu/drm/nouveau/dispnv04/overlay.c:496:1: warning: no previous 
prototype for 'nouveau_overlay_init' [-Wmissing-prototypes]

In fact, this function is declared in disp.h, so this patch
add missing header dependencies

Signed-off-by: Baoyou Xie 
---
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c 
b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
index ec444ea..a79514d 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
@@ -33,7 +33,7 @@
 #include "nouveau_connector.h"
 #include "nouveau_display.h"
 #include "nvreg.h"
-
+#include "disp.h"
 
 struct nouveau_plane {
struct drm_plane base;
-- 
2.7.4

drivers/gpio/gpio-mcp23s08.c:568:11: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   af56ff27eba54fceee5f5643e79bf6531f2e1739
commit: 2527ecc9195e9c66252af24c4689e8a67cd4ccb9 gpio: Fix OF build problem on 
UM
date:   9 days ago
config: um-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
git checkout 2527ecc9195e9c66252af24c4689e8a67cd4ccb9
# save the attached .config to linux build tree
make ARCH=um 

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-mcp23s08.c: In function 'mcp23s08_probe_one':
>> drivers/gpio/gpio-mcp23s08.c:568:11: error: 'struct gpio_chip' has no member 
>> named 'of_gpio_n_cells'
 mcp->chip.of_gpio_n_cells = 2;
  ^
>> drivers/gpio/gpio-mcp23s08.c:569:11: error: 'struct gpio_chip' has no member 
>> named 'of_node'
 mcp->chip.of_node = dev->of_node;
  ^

vim +568 drivers/gpio/gpio-mcp23s08.c

e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  562   
mcp->chip.direction_input = mcp23s08_direction_input;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  563   
mcp->chip.get = mcp23s08_get;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  564   
mcp->chip.direction_output = mcp23s08_direction_output;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  565   
mcp->chip.set = mcp23s08_set;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  566   
mcp->chip.dbg_show = mcp23s08_dbg_show;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04  567  #ifdef 
CONFIG_OF
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04 @568   
mcp->chip.of_gpio_n_cells = 2;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04 @569   
mcp->chip.of_node = dev->of_node;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04  570  #endif
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  571  
d62b98f3 drivers/gpio/gpio-mcp23s08.c Peter Korsgaard 2011-07-15  572   switch 
(type) {

:: The code at line 568 was first introduced by commit
:: 97ddb1c88b4ebe057b63346660abfee165ddd468 gpio: mcp23s08: convert driver 
to DT

:: TO: Lars Poeschel 
:: CC: Linus Walleij 

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


.config.gz
Description: Binary data

drivers/gpio/gpio-mcp23s08.c:568:11: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   af56ff27eba54fceee5f5643e79bf6531f2e1739
commit: 2527ecc9195e9c66252af24c4689e8a67cd4ccb9 gpio: Fix OF build problem on 
UM
date:   9 days ago
config: um-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
git checkout 2527ecc9195e9c66252af24c4689e8a67cd4ccb9
# save the attached .config to linux build tree
make ARCH=um 

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-mcp23s08.c: In function 'mcp23s08_probe_one':
>> drivers/gpio/gpio-mcp23s08.c:568:11: error: 'struct gpio_chip' has no member 
>> named 'of_gpio_n_cells'
 mcp->chip.of_gpio_n_cells = 2;
  ^
>> drivers/gpio/gpio-mcp23s08.c:569:11: error: 'struct gpio_chip' has no member 
>> named 'of_node'
 mcp->chip.of_node = dev->of_node;
  ^

vim +568 drivers/gpio/gpio-mcp23s08.c

e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  562   
mcp->chip.direction_input = mcp23s08_direction_input;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  563   
mcp->chip.get = mcp23s08_get;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  564   
mcp->chip.direction_output = mcp23s08_direction_output;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  565   
mcp->chip.set = mcp23s08_set;
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  566   
mcp->chip.dbg_show = mcp23s08_dbg_show;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04  567  #ifdef 
CONFIG_OF
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04 @568   
mcp->chip.of_gpio_n_cells = 2;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04 @569   
mcp->chip.of_node = dev->of_node;
97ddb1c8 drivers/gpio/gpio-mcp23s08.c Lars Poeschel   2013-04-04  570  #endif
e58b9e27 drivers/gpio/mcp23s08.c  David Brownell  2008-02-04  571  
d62b98f3 drivers/gpio/gpio-mcp23s08.c Peter Korsgaard 2011-07-15  572   switch 
(type) {

:: The code at line 568 was first introduced by commit
:: 97ddb1c88b4ebe057b63346660abfee165ddd468 gpio: mcp23s08: convert driver 
to DT

:: TO: Lars Poeschel 
:: CC: Linus Walleij 

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


.config.gz
Description: Binary data

Re: [PATCH v3 0/5] kexec_file: Add buffer hand-over for the next kernel

[Mimi Zohar]

On Thu, 2016-08-25 at 19:17 -0300, Thiago Jung Bauermann wrote:
> Am Donnerstag, 25 August 2016, 14:12:43 schrieb Andrew Morton:
> > I grabbed these two patch series.  I also merged the "IMA:
> > Demonstration code for kexec buffer passing." demonstration patch just
> > to get things a bit of testing.
> 
> Thank you very much!

Thank you, Andrew.

> > I assume that once the "ima: carry the
> > measurement list across kexec" series has stabilised, I should drop the
> > demo patch and also grab those?  If so, pelase start cc'ing me.
> 
> I'm not sure how Mimi is planning to upstream that series.

I'll re-post the patches shortly, cc'ing you.  It will contain support
for saving the binary measurement list on one architecture (eg. little
endian) and restoring it on another (eg. big endian).

Mimi

Re: [PATCH v3 0/5] kexec_file: Add buffer hand-over for the next kernel

[Mimi Zohar]

On Thu, 2016-08-25 at 19:17 -0300, Thiago Jung Bauermann wrote:
> Am Donnerstag, 25 August 2016, 14:12:43 schrieb Andrew Morton:
> > I grabbed these two patch series.  I also merged the "IMA:
> > Demonstration code for kexec buffer passing." demonstration patch just
> > to get things a bit of testing.
> 
> Thank you very much!

Thank you, Andrew.

> > I assume that once the "ima: carry the
> > measurement list across kexec" series has stabilised, I should drop the
> > demo patch and also grab those?  If so, pelase start cc'ing me.
> 
> I'm not sure how Mimi is planning to upstream that series.

I'll re-post the patches shortly, cc'ing you.  It will contain support
for saving the binary measurement list on one architecture (eg. little
endian) and restoring it on another (eg. big endian).

Mimi

[PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for 
'_free_qp' [-Wmissing-prototypes]

In fact, this function is only used in the file in which it is declared
and don't need a declaration, but can be made static.
so this patch marks it 'static'.

Signed-off-by: Baoyou Xie 
---
 drivers/infiniband/hw/cxgb4/qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index edb1172..6904352 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -683,7 +683,7 @@ static int build_inv_stag(union t4_wr *wqe, struct 
ib_send_wr *wr,
return 0;
 }
 
-void _free_qp(struct kref *kref)
+static void _free_qp(struct kref *kref)
 {
struct c4iw_qp *qhp;
 
-- 
2.7.4

[PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Baoyou Xie]

We get 1 warning when build kernel with W=1:
drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for 
'_free_qp' [-Wmissing-prototypes]

In fact, this function is only used in the file in which it is declared
and don't need a declaration, but can be made static.
so this patch marks it 'static'.

Signed-off-by: Baoyou Xie 
---
 drivers/infiniband/hw/cxgb4/qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index edb1172..6904352 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -683,7 +683,7 @@ static int build_inv_stag(union t4_wr *wqe, struct 
ib_send_wr *wr,
return 0;
 }
 
-void _free_qp(struct kref *kref)
+static void _free_qp(struct kref *kref)
 {
struct c4iw_qp *qhp;
 
-- 
2.7.4

Re: [PATCH v7 8/8] drm/rockchip: Add dmc notifier in vop driver

[kbuild test robot]

Hi Lin,

[auto build test ERROR on rockchip/for-next]
[cannot apply to v4.8-rc3 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Lin-Huang/rk3399-support-ddr-frequency-scaling/20160822-114057
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git 
for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/rockchip/rockchip_drm_vop.c:16:0:
>> include/linux/devfreq-event.h:190:21: error: redefinition of 
>> 'devfreq_event_get_drvdata'
static inline void *devfreq_event_get_drvdata(struct devfreq_event_dev 
*edev)
^
   include/linux/devfreq-event.h:151:21: note: previous definition of 
'devfreq_event_get_drvdata' was here
static inline void *devfreq_event_get_drvdata(struct devfreq_event_dev 
*edev)
^

vim +/devfreq_event_get_drvdata +190 include/linux/devfreq-event.h

f262f28c Chanwoo Choi 2015-01-26  184  
f262f28c Chanwoo Choi 2015-01-26  185  static inline void 
devm_devfreq_event_remove_edev(struct device *dev,
f262f28c Chanwoo Choi 2015-01-26  186   struct 
devfreq_event_dev *edev)
f262f28c Chanwoo Choi 2015-01-26  187  {
f262f28c Chanwoo Choi 2015-01-26  188  }
f262f28c Chanwoo Choi 2015-01-26  189  
f262f28c Chanwoo Choi 2015-01-26 @190  static inline void 
*devfreq_event_get_drvdata(struct devfreq_event_dev *edev)
f262f28c Chanwoo Choi 2015-01-26  191  {
f262f28c Chanwoo Choi 2015-01-26  192   return NULL;
f262f28c Chanwoo Choi 2015-01-26  193  }

:: The code at line 190 was first introduced by commit
:: f262f28c147051e7aa6daaf4fb5996833ffadff4 PM / devfreq: event: Add 
devfreq_event class

:: TO: Chanwoo Choi 
:: CC: MyungJoo Ham 

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


.config.gz
Description: Binary data

Re: [PATCH v7 8/8] drm/rockchip: Add dmc notifier in vop driver

[kbuild test robot]

Hi Lin,

[auto build test ERROR on rockchip/for-next]
[cannot apply to v4.8-rc3 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Lin-Huang/rk3399-support-ddr-frequency-scaling/20160822-114057
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git 
for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/rockchip/rockchip_drm_vop.c:16:0:
>> include/linux/devfreq-event.h:190:21: error: redefinition of 
>> 'devfreq_event_get_drvdata'
static inline void *devfreq_event_get_drvdata(struct devfreq_event_dev 
*edev)
^
   include/linux/devfreq-event.h:151:21: note: previous definition of 
'devfreq_event_get_drvdata' was here
static inline void *devfreq_event_get_drvdata(struct devfreq_event_dev 
*edev)
^

vim +/devfreq_event_get_drvdata +190 include/linux/devfreq-event.h

f262f28c Chanwoo Choi 2015-01-26  184  
f262f28c Chanwoo Choi 2015-01-26  185  static inline void 
devm_devfreq_event_remove_edev(struct device *dev,
f262f28c Chanwoo Choi 2015-01-26  186   struct 
devfreq_event_dev *edev)
f262f28c Chanwoo Choi 2015-01-26  187  {
f262f28c Chanwoo Choi 2015-01-26  188  }
f262f28c Chanwoo Choi 2015-01-26  189  
f262f28c Chanwoo Choi 2015-01-26 @190  static inline void 
*devfreq_event_get_drvdata(struct devfreq_event_dev *edev)
f262f28c Chanwoo Choi 2015-01-26  191  {
f262f28c Chanwoo Choi 2015-01-26  192   return NULL;
f262f28c Chanwoo Choi 2015-01-26  193  }

:: The code at line 190 was first introduced by commit
:: f262f28c147051e7aa6daaf4fb5996833ffadff4 PM / devfreq: event: Add 
devfreq_event class

:: TO: Chanwoo Choi 
:: CC: MyungJoo Ham 

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


.config.gz
Description: Binary data

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sat, Aug 27, 2016 at 09:42:59PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> > On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > > 
> > > > > - Making checkpatch check for (some) of the stable kernel rules
> > > > > (and possibly recommend adding the stable@ tag in certain cases?).
> > > > >   - Depends on: making checkpatch sane again
> > > > > >This sounds interesting.  What do you mean by "sane"?
> > > Sasha, can you expand your thoughts here please?
> > Sure. I have 2.5 concerns about the state of checkpatch:
> []
> > > Most all of the trivial spacing stuff can easily be
> > > ignored either by a human determining what's important
> > > or by using command line options like --ignore=spacing
> > 1.
> > This is the wrong default. By default checkpatch shouldn't be showing 
> > trivial
> > issues that encourage folks to try and work around them and as a result
> > produce worse code.
> > 
> > Look at the 80 character limit warning for example, what good does it do?
> 
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.

I'm not trying to be specific with the 80 character thing, it's also true for
a few other things that makes people produce less readable code than what it
would have looked like if they'd ignore the warning.
 
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

That makes sense to you, but it doesn't make sense to the newer folks who are
told not to submit any patches with checkpatch errors/warnings. You know to
ignore these 80-character warnings when it makes sense, they see it as "you
must make the warning disappear no matter what".
 
> I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
> would be a good change.
> 
> https://lkml.org/lkml/2015/7/16/568

Probably. Would you agree that by default we shouldn't show anything that's
not an error/defect?

> >  It
> > encourages people to do even stupider things to work around it and results 
> > in
> > a bunch of "fix checkpatch warning" that touch existing code just to make 
> > the
> > result harder to read and make 'git blame' harder to work with.
> 
> Almost all of the crud in git-blame can be avoided with -w

That doesn't deal with newlines people add to hide the 80 character stuff, nor 
it
deals with the "harder to read" part.

> > By default you should only get the most critical warnings we have in the
> > kernel like missing S-O-B or corrupt patch.
> 
> I don't think so, but if you do, add a filter for ERROR only.

I could, but the problem is the people who see the default output as "holy".

> > 2. A "who wrote these rules?": there seems to be a disconnect between the 
> > rules
> > checkpatch is trying to enforce and the accepted coding style enforced by
> > maintainers. 
> 
> Name some please.

Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID). 
checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg 
didn't get the memo.

> > Do a git-format-patch on all of the commits Linus authored in the past year 
> > or
> > two and see how many of them fail checkpatch (or do the same for any of the
> > commits that passed through and were accepted by the top maintainers),
> > according to checkpatch we need to make those guys stop touching the kernel.
> 
> Try it yourself and tell me what's wrong with the messages:
> 
> $ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | 
> \
>   grep -v " Linux [34]" | \
>   while read commit ; do \
>     echo $commit ; \
>     git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
>   ./scripts/checkpatch.pl - ; \
>   done
> 
> Here's a summary done with an additional
> 
>   grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
>   sort |uniq -c | sort -rn
> 
>      46 WARNING:LONG_LINE_COMMENT
>  45 WARNING:LEADING_SPACE
>  37 WARNING:LONG_LINE
>  16 ERROR:GIT_COMMIT_ID
>  11 WARNING:COMMIT_LOG_LONG_LINE
>   5 WARNING:BRACES
>   2 WARNING:BAD_SIGN_OFF
>   2 WARNING:AVOID_BUG
>   2 ERROR:SPACING
>   1 WARNING:SPLIT_STRING
>   1 WARNING:FILE_PATH_CHANGES
>   1 WARNING:ENOSYS
>   1 ERROR:MISSING_SIGN_OFF

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | 
grep -v " Linux [34]" | wc -l
64

Linus has more errors/warnings than commits. Why do we let him commit stuff?

> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob 
> > of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 
> > it's
> > 6142 lines, making it the 124th largest source file in the kernel, well 
> > within
> > the top 1% of source files in the kernel.
> > 
> > This combination of 

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sat, Aug 27, 2016 at 09:42:59PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> > On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > > 
> > > > > - Making checkpatch check for (some) of the stable kernel rules
> > > > > (and possibly recommend adding the stable@ tag in certain cases?).
> > > > >   - Depends on: making checkpatch sane again
> > > > > >This sounds interesting.  What do you mean by "sane"?
> > > Sasha, can you expand your thoughts here please?
> > Sure. I have 2.5 concerns about the state of checkpatch:
> []
> > > Most all of the trivial spacing stuff can easily be
> > > ignored either by a human determining what's important
> > > or by using command line options like --ignore=spacing
> > 1.
> > This is the wrong default. By default checkpatch shouldn't be showing 
> > trivial
> > issues that encourage folks to try and work around them and as a result
> > produce worse code.
> > 
> > Look at the 80 character limit warning for example, what good does it do?
> 
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.

I'm not trying to be specific with the 80 character thing, it's also true for
a few other things that makes people produce less readable code than what it
would have looked like if they'd ignore the warning.
 
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

That makes sense to you, but it doesn't make sense to the newer folks who are
told not to submit any patches with checkpatch errors/warnings. You know to
ignore these 80-character warnings when it makes sense, they see it as "you
must make the warning disappear no matter what".
 
> I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
> would be a good change.
> 
> https://lkml.org/lkml/2015/7/16/568

Probably. Would you agree that by default we shouldn't show anything that's
not an error/defect?

> >  It
> > encourages people to do even stupider things to work around it and results 
> > in
> > a bunch of "fix checkpatch warning" that touch existing code just to make 
> > the
> > result harder to read and make 'git blame' harder to work with.
> 
> Almost all of the crud in git-blame can be avoided with -w

That doesn't deal with newlines people add to hide the 80 character stuff, nor 
it
deals with the "harder to read" part.

> > By default you should only get the most critical warnings we have in the
> > kernel like missing S-O-B or corrupt patch.
> 
> I don't think so, but if you do, add a filter for ERROR only.

I could, but the problem is the people who see the default output as "holy".

> > 2. A "who wrote these rules?": there seems to be a disconnect between the 
> > rules
> > checkpatch is trying to enforce and the accepted coding style enforced by
> > maintainers. 
> 
> Name some please.

Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID). 
checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg 
didn't get the memo.

> > Do a git-format-patch on all of the commits Linus authored in the past year 
> > or
> > two and see how many of them fail checkpatch (or do the same for any of the
> > commits that passed through and were accepted by the top maintainers),
> > according to checkpatch we need to make those guys stop touching the kernel.
> 
> Try it yourself and tell me what's wrong with the messages:
> 
> $ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | 
> \
>   grep -v " Linux [34]" | \
>   while read commit ; do \
>     echo $commit ; \
>     git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
>   ./scripts/checkpatch.pl - ; \
>   done
> 
> Here's a summary done with an additional
> 
>   grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
>   sort |uniq -c | sort -rn
> 
>      46 WARNING:LONG_LINE_COMMENT
>  45 WARNING:LEADING_SPACE
>  37 WARNING:LONG_LINE
>  16 ERROR:GIT_COMMIT_ID
>  11 WARNING:COMMIT_LOG_LONG_LINE
>   5 WARNING:BRACES
>   2 WARNING:BAD_SIGN_OFF
>   2 WARNING:AVOID_BUG
>   2 ERROR:SPACING
>   1 WARNING:SPLIT_STRING
>   1 WARNING:FILE_PATH_CHANGES
>   1 WARNING:ENOSYS
>   1 ERROR:MISSING_SIGN_OFF

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | 
grep -v " Linux [34]" | wc -l
64

Linus has more errors/warnings than commits. Why do we let him commit stuff?

> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob 
> > of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 
> > it's
> > 6142 lines, making it the 124th largest source file in the kernel, well 
> > within
> > the top 1% of source files in the kernel.
> > 
> > This combination of 

Re: MAINTAINERS without commits in the last 3 years

[Joe Perches]

On Sat, 2016-08-27 at 22:31 -0400, Mark F. Brown wrote:
> I've continued to author patches for the Linux kernel, but they have been
> under my work email address. This email works but I do not monitor it very
> often anymore.

So should the MAINTAINERS address be changed ?

Re: MAINTAINERS without commits in the last 3 years

[Joe Perches]

On Sat, 2016-08-27 at 22:31 -0400, Mark F. Brown wrote:
> I've continued to author patches for the Linux kernel, but they have been
> under my work email address. This email works but I do not monitor it very
> often anymore.

So should the MAINTAINERS address be changed ?

Re: [PATCH] rtlwifi/rtl8192de: Fix print format string

[Larry Finger]

On 08/26/2016 10:12 PM, Oleg Drokin wrote:

%ul was likely meant as %lu to print an unsigned long,
not an unsigned with a letter l at the end.
But in fact the value printed is u32 anyway, so just drop
the l completely.


This patch looks OK, thus

Acked-by: Larry Finger 

Thanks,

Larry



Signed-off-by: Oleg Drokin 
---
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
index d334d2a..2a4810d 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
@@ -588,7 +588,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_Array_Table[0] is %ul 
Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_Array_Table[0] is %u 
Rtl819XPHY_REGArray[1] is %u\n",
 agctab_array_table[i],
 agctab_array_table[i + 1]);
}
@@ -604,7 +604,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_Array_Table[0] 
is %ul Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_Array_Table[0] 
is %u Rtl819XPHY_REGArray[1] is %u\n",
 agctab_array_table[i],
 agctab_array_table[i + 1]);
}
@@ -620,7 +620,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_5GArray_Table[0] 
is %ul Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_5GArray_Table[0] 
is %u Rtl819XPHY_REGArray[1] is %u\n",
 agctab_5garray_table[i],
 agctab_5garray_table[i + 1]);
}

Re: [PATCH] rtlwifi/rtl8192de: Fix print format string

[Larry Finger]

On 08/26/2016 10:12 PM, Oleg Drokin wrote:

%ul was likely meant as %lu to print an unsigned long,
not an unsigned with a letter l at the end.
But in fact the value printed is u32 anyway, so just drop
the l completely.


This patch looks OK, thus

Acked-by: Larry Finger 

Thanks,

Larry



Signed-off-by: Oleg Drokin 
---
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
index d334d2a..2a4810d 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
@@ -588,7 +588,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_Array_Table[0] is %ul 
Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_Array_Table[0] is %u 
Rtl819XPHY_REGArray[1] is %u\n",
 agctab_array_table[i],
 agctab_array_table[i + 1]);
}
@@ -604,7 +604,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_Array_Table[0] 
is %ul Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_Array_Table[0] 
is %u Rtl819XPHY_REGArray[1] is %u\n",
 agctab_array_table[i],
 agctab_array_table[i + 1]);
}
@@ -620,7 +620,7 @@ static bool _rtl92d_phy_config_bb_with_headerfile(struct 
ieee80211_hw *hw,
 * setting. */
udelay(1);
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-"The Rtl819XAGCTAB_5GArray_Table[0] 
is %ul Rtl819XPHY_REGArray[1] is %ul\n",
+"The Rtl819XAGCTAB_5GArray_Table[0] 
is %u Rtl819XPHY_REGArray[1] is %u\n",
 agctab_5garray_table[i],
 agctab_5garray_table[i + 1]);
}

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 18:42 -0700, Joe Perches wrote:

>      46 WARNING:LONG_LINE_COMMENT
>  45 WARNING:LEADING_SPACE
>  37 WARNING:LONG_LINE
>  16 ERROR:GIT_COMMIT_ID
>  11 WARNING:COMMIT_LOG_LONG_LINE
>   5 WARNING:BRACES
>   2 WARNING:BAD_SIGN_OFF
>   2 WARNING:AVOID_BUG
>   2 ERROR:SPACING
>   1 WARNING:SPLIT_STRING
>   1 WARNING:FILE_PATH_CHANGES
>   1 WARNING:ENOSYS
>   1 ERROR:MISSING_SIGN_OFF

My copy/pasta mistake, the above was Ingo Molnar's list

Here's Linus'

     37 WARNING:LONG_LINE
 19 ERROR:GIT_COMMIT_ID
  8 WARNING:PREFER_PR_LEVEL
  6 ERROR:SPACING
  4 WARNING:MACRO_WITH_FLOW_CONTROL
  3 WARNING:COMMIT_LOG_LONG_LINE
  2 WARNING:TYPO_SPELLING
  2 WARNING:BAD_SIGN_OFF
  2 ERROR:TRAILING_STATEMENTS
  1 WARNING:NEW_TYPEDEFS
  1 WARNING:LONG_LINE_COMMENT
  1 WARNING:LINE_SPACING
  1 WARNING:CONSTANT_COMPARISON
  1 WARNING:AVOID_BUG
  1 ERROR:STABLE_ADDRESS

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 18:42 -0700, Joe Perches wrote:

>      46 WARNING:LONG_LINE_COMMENT
>  45 WARNING:LEADING_SPACE
>  37 WARNING:LONG_LINE
>  16 ERROR:GIT_COMMIT_ID
>  11 WARNING:COMMIT_LOG_LONG_LINE
>   5 WARNING:BRACES
>   2 WARNING:BAD_SIGN_OFF
>   2 WARNING:AVOID_BUG
>   2 ERROR:SPACING
>   1 WARNING:SPLIT_STRING
>   1 WARNING:FILE_PATH_CHANGES
>   1 WARNING:ENOSYS
>   1 ERROR:MISSING_SIGN_OFF

My copy/pasta mistake, the above was Ingo Molnar's list

Here's Linus'

     37 WARNING:LONG_LINE
 19 ERROR:GIT_COMMIT_ID
  8 WARNING:PREFER_PR_LEVEL
  6 ERROR:SPACING
  4 WARNING:MACRO_WITH_FLOW_CONTROL
  3 WARNING:COMMIT_LOG_LONG_LINE
  2 WARNING:TYPO_SPELLING
  2 WARNING:BAD_SIGN_OFF
  2 ERROR:TRAILING_STATEMENTS
  1 WARNING:NEW_TYPEDEFS
  1 WARNING:LONG_LINE_COMMENT
  1 WARNING:LINE_SPACING
  1 WARNING:CONSTANT_COMPARISON
  1 WARNING:AVOID_BUG
  1 ERROR:STABLE_ADDRESS

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

Hi,
On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > What's the progress of this patch? Looks already have experts review 
> > > > > it.
> > > > > Why this patch didn't accept?
> > > > This patch is a little overkilled, and I have saved another simpler
> > > > version to only check the md5 hash (as people suggested) for it. I can 
> > > > post it later.
> > > > 
> > > 
> > > I am happy to test and review it.
> > >
> > Here it is. As Rafael is on travel, it would be grateful
> > if you can give some advance on this, thanks!
> 
> Better than last one.
> 
> > +   return -ENOMEM;
> > +
> > +   req = ahash_request_alloc(tfm, GFP_ATOMIC);
> 
> what context is this called from? GFP_ATOMIC allocations like to fail...
>
It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > +static int hibernation_e820_check(void *buf)
> > +{
> > +   int ret;
> > +   char result[MD5_HASH_SIZE] = {0};
> > +
> > +   ret = get_e820_md5(_saved, result);
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (memcmp(result, buf, MD5_HASH_SIZE))
> > +   e820_conflict = true;
> 
> Passing return value using global variable is ugly. Can you just print
> the warning and kill the box here?
Do you mean get rid of the panic hooker and just print the warning here?
> > +
> 
> > +   /*
> > +* A page has been allocated previously to store the hibernation
> > +* image header, so we can safely store the md5 result behind
> > +* struct restore_data_record, with size of 128 bytes.
> > +*/
> > +   hibernation_e820_save(addr + sizeof(struct restore_data_record));
> > +
> 
> Please just allocate space in struct restore_data_record . And I don't
> think md5 sum is 128 _bytes_.
>
OK. The md5 sum should be 128 bits thus 16 bytes.

Thanks!
Yu 

Re: [PATCH][v6] PM / hibernate: Print the possible panic reason when resuming with inconsistent e820 map

[Chen Yu]

Hi,
On Fri, Aug 26, 2016 at 09:56:54PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > What's the progress of this patch? Looks already have experts review 
> > > > > it.
> > > > > Why this patch didn't accept?
> > > > This patch is a little overkilled, and I have saved another simpler
> > > > version to only check the md5 hash (as people suggested) for it. I can 
> > > > post it later.
> > > > 
> > > 
> > > I am happy to test and review it.
> > >
> > Here it is. As Rafael is on travel, it would be grateful
> > if you can give some advance on this, thanks!
> 
> Better than last one.
> 
> > +   return -ENOMEM;
> > +
> > +   req = ahash_request_alloc(tfm, GFP_ATOMIC);
> 
> what context is this called from? GFP_ATOMIC allocations like to fail...
>
It is in normal process context, OK, I'll change it to GFP_KERNEL.
> > +static int hibernation_e820_check(void *buf)
> > +{
> > +   int ret;
> > +   char result[MD5_HASH_SIZE] = {0};
> > +
> > +   ret = get_e820_md5(_saved, result);
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (memcmp(result, buf, MD5_HASH_SIZE))
> > +   e820_conflict = true;
> 
> Passing return value using global variable is ugly. Can you just print
> the warning and kill the box here?
Do you mean get rid of the panic hooker and just print the warning here?
> > +
> 
> > +   /*
> > +* A page has been allocated previously to store the hibernation
> > +* image header, so we can safely store the md5 result behind
> > +* struct restore_data_record, with size of 128 bytes.
> > +*/
> > +   hibernation_e820_save(addr + sizeof(struct restore_data_record));
> > +
> 
> Please just allocate space in struct restore_data_record . And I don't
> think md5 sum is 128 _bytes_.
>
OK. The md5 sum should be 128 bits thus 16 bytes.

Thanks!
Yu 

Re: [PATCH][RFC v4] timekeeping: ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

On Sat, Aug 27, 2016 at 03:08:56PM +0800, Xunlei Pang wrote:
> On 2016/08/18 at 18:43, Chen Yu wrote:
> > Previously we encountered some memory overflow issues due to
> > the bogus sleep time brought by inconsistent rtc, which is
> > triggered when pm_trace is enabled, please refer to:
> > https://patchwork.kernel.org/patch/9286365/
> > It's improper in the first place to call __timekeeping_inject_sleeptime()
> > in case that pm_trace is enabled simply because that "hash" time value
> > will wreckage the timekeeping subsystem.
> >
> > So this patch ignores the sleep time if pm_trace is enabled in
> > the following situation:
> > 1. rtc is used as persist clock to compensate for sleep time,
> >(because system does not have a nonstop clocksource) or
> > 2. rtc is used to calculate the sleep time in rtc_resume.
> >
> > Cc: Rafael J. Wysocki 
> > Cc: John Stultz 
> > Cc: Thomas Gleixner 
> > Cc: Xunlei Pang 
> > Cc: Zhang Rui 
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux...@vger.kernel.org
> > Suggested-by: Rafael J. Wysocki 
> > Reported-by: Janek Kozicki 
> > Signed-off-by: Chen Yu 
> > ---
> 
> I suddenly think of a way to avoid adding this ugly __weak auxiliary function.
> 
> Add a special treatment for read_persistent_clock() in arch/x86/kernel/rtc.c 
> as follows,
> void read_persistent_clock(struct timespec *ts)
> {
> x86_platform.get_wallclock(ts);
> 
> /* Make rtc-based persistent clock unusable if pm_trace is enabled. */
> if (pm_trace_is_enabled() &&
> x86_platform.get_wallclock == mach_get_cmos_time) {
> ts->tv_sec = 0;
> ts->tv_nsec = 0;
> }
> }
> 
> In this way, we can avoid the touch of timekeeping core, after all ptrace is 
> currently x86-specific.
> 
> What do you think?
>
Good point! Will send another version based on this idea.

Thanks,
Yu

Re: [PATCH][RFC v4] timekeeping: ignore the bogus sleep time if pm_trace is enabled

[Chen Yu]

On Sat, Aug 27, 2016 at 03:08:56PM +0800, Xunlei Pang wrote:
> On 2016/08/18 at 18:43, Chen Yu wrote:
> > Previously we encountered some memory overflow issues due to
> > the bogus sleep time brought by inconsistent rtc, which is
> > triggered when pm_trace is enabled, please refer to:
> > https://patchwork.kernel.org/patch/9286365/
> > It's improper in the first place to call __timekeeping_inject_sleeptime()
> > in case that pm_trace is enabled simply because that "hash" time value
> > will wreckage the timekeeping subsystem.
> >
> > So this patch ignores the sleep time if pm_trace is enabled in
> > the following situation:
> > 1. rtc is used as persist clock to compensate for sleep time,
> >(because system does not have a nonstop clocksource) or
> > 2. rtc is used to calculate the sleep time in rtc_resume.
> >
> > Cc: Rafael J. Wysocki 
> > Cc: John Stultz 
> > Cc: Thomas Gleixner 
> > Cc: Xunlei Pang 
> > Cc: Zhang Rui 
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux...@vger.kernel.org
> > Suggested-by: Rafael J. Wysocki 
> > Reported-by: Janek Kozicki 
> > Signed-off-by: Chen Yu 
> > ---
> 
> I suddenly think of a way to avoid adding this ugly __weak auxiliary function.
> 
> Add a special treatment for read_persistent_clock() in arch/x86/kernel/rtc.c 
> as follows,
> void read_persistent_clock(struct timespec *ts)
> {
> x86_platform.get_wallclock(ts);
> 
> /* Make rtc-based persistent clock unusable if pm_trace is enabled. */
> if (pm_trace_is_enabled() &&
> x86_platform.get_wallclock == mach_get_cmos_time) {
> ts->tv_sec = 0;
> ts->tv_nsec = 0;
> }
> }
> 
> In this way, we can avoid the touch of timekeeping core, after all ptrace is 
> currently x86-specific.
> 
> What do you think?
>
Good point! Will send another version based on this idea.

Thanks,
Yu

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > 
> > > > - Making checkpatch check for (some) of the stable kernel rules
> > > > (and possibly recommend adding the stable@ tag in certain cases?).
> > > >   - Depends on: making checkpatch sane again
> > > > >This sounds interesting.  What do you mean by "sane"?
> > Sasha, can you expand your thoughts here please?
> Sure. I have 2.5 concerns about the state of checkpatch:
[]
> > Most all of the trivial spacing stuff can easily be
> > ignored either by a human determining what's important
> > or by using command line options like --ignore=spacing
> 1.
> This is the wrong default. By default checkpatch shouldn't be showing trivial
> issues that encourage folks to try and work around them and as a result
> produce worse code.
> 
> Look at the 80 character limit warning for example, what good does it do?

That argument's been done several times. It keeps Linus happy.
I don't care one way or another.

I think the biggest issue is the seriousness that some people
take checkpatch messages as dicta instead of ignorable bleats.

I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
would be a good change.

https://lkml.org/lkml/2015/7/16/568

>  It
> encourages people to do even stupider things to work around it and results in
> a bunch of "fix checkpatch warning" that touch existing code just to make the
> result harder to read and make 'git blame' harder to work with.

Almost all of the crud in git-blame can be avoided with -w

> By default you should only get the most critical warnings we have in the
> kernel like missing S-O-B or corrupt patch.

I don't think so, but if you do, add a filter for ERROR only.

> 2. A "who wrote these rules?": there seems to be a disconnect between the 
> rules
> checkpatch is trying to enforce and the accepted coding style enforced by
> maintainers. 

Name some please.

> Do a git-format-patch on all of the commits Linus authored in the past year or
> two and see how many of them fail checkpatch (or do the same for any of the
> commits that passed through and were accepted by the top maintainers),
> according to checkpatch we need to make those guys stop touching the kernel.

Try it yourself and tell me what's wrong with the messages:

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
  grep -v " Linux [34]" | \
  while read commit ; do \
    echo $commit ; \
    git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
  ./scripts/checkpatch.pl - ; \
  done

Here's a summary done with an additional

  grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
  sort |uniq -c | sort -rn

     46 WARNING:LONG_LINE_COMMENT
 45 WARNING:LEADING_SPACE
 37 WARNING:LONG_LINE
 16 ERROR:GIT_COMMIT_ID
 11 WARNING:COMMIT_LOG_LONG_LINE
  5 WARNING:BRACES
  2 WARNING:BAD_SIGN_OFF
  2 WARNING:AVOID_BUG
  2 ERROR:SPACING
  1 WARNING:SPLIT_STRING
  1 WARNING:FILE_PATH_CHANGES
  1 WARNING:ENOSYS
  1 ERROR:MISSING_SIGN_OFF

> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 
> it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

Meh, I'm not a perl guy either.

I think almost all of it is regexes and most people
aren't very good at those.

So it wouldn't matter if it was perl or python.

spatch isn't the right tool.

What would you suggest instead?

Re: checkkpatch (in)sanity ?

[Joe Perches]

On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > 
> > > > - Making checkpatch check for (some) of the stable kernel rules
> > > > (and possibly recommend adding the stable@ tag in certain cases?).
> > > >   - Depends on: making checkpatch sane again
> > > > >This sounds interesting.  What do you mean by "sane"?
> > Sasha, can you expand your thoughts here please?
> Sure. I have 2.5 concerns about the state of checkpatch:
[]
> > Most all of the trivial spacing stuff can easily be
> > ignored either by a human determining what's important
> > or by using command line options like --ignore=spacing
> 1.
> This is the wrong default. By default checkpatch shouldn't be showing trivial
> issues that encourage folks to try and work around them and as a result
> produce worse code.
> 
> Look at the 80 character limit warning for example, what good does it do?

That argument's been done several times. It keeps Linus happy.
I don't care one way or another.

I think the biggest issue is the seriousness that some people
take checkpatch messages as dicta instead of ignorable bleats.

I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
would be a good change.

https://lkml.org/lkml/2015/7/16/568

>  It
> encourages people to do even stupider things to work around it and results in
> a bunch of "fix checkpatch warning" that touch existing code just to make the
> result harder to read and make 'git blame' harder to work with.

Almost all of the crud in git-blame can be avoided with -w

> By default you should only get the most critical warnings we have in the
> kernel like missing S-O-B or corrupt patch.

I don't think so, but if you do, add a filter for ERROR only.

> 2. A "who wrote these rules?": there seems to be a disconnect between the 
> rules
> checkpatch is trying to enforce and the accepted coding style enforced by
> maintainers. 

Name some please.

> Do a git-format-patch on all of the commits Linus authored in the past year or
> two and see how many of them fail checkpatch (or do the same for any of the
> commits that passed through and were accepted by the top maintainers),
> according to checkpatch we need to make those guys stop touching the kernel.

Try it yourself and tell me what's wrong with the messages:

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
  grep -v " Linux [34]" | \
  while read commit ; do \
    echo $commit ; \
    git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
  ./scripts/checkpatch.pl - ; \
  done

Here's a summary done with an additional

  grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
  sort |uniq -c | sort -rn

     46 WARNING:LONG_LINE_COMMENT
 45 WARNING:LEADING_SPACE
 37 WARNING:LONG_LINE
 16 ERROR:GIT_COMMIT_ID
 11 WARNING:COMMIT_LOG_LONG_LINE
  5 WARNING:BRACES
  2 WARNING:BAD_SIGN_OFF
  2 WARNING:AVOID_BUG
  2 ERROR:SPACING
  1 WARNING:SPLIT_STRING
  1 WARNING:FILE_PATH_CHANGES
  1 WARNING:ENOSYS
  1 ERROR:MISSING_SIGN_OFF

> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 
> it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

Meh, I'm not a perl guy either.

I think almost all of it is regexes and most people
aren't very good at those.

So it wouldn't matter if it was perl or python.

spatch isn't the right tool.

What would you suggest instead?

[PATCH] fscrypto: fix to null-terminate encrypted filename in fname_encrypt

[Chao Yu]

From: Chao Yu 

This patch fixes to add null character at the end of encrypted filename
in fname_encrypt, in order to avoid incorrectly traversing random data
located after target filename. The call stack is as below:

- f2fs_add_link
 - __f2fs_add_link
  - fscrypt_setup_filename
   - fscrypt_fname_alloc_buffer allocate buffer for @fname
   - fname_encrypt  didn't set null character for @fname
  - f2fs_add_regular_entry  init qstr with @fname
   - init_inode_metadata
- f2fs_init_security
 - security_inode_init_security
  - selinux_inode_init_security
   - selinux_determine_inode_label
- security_transition_sid
 - security_compute_sid
  - filename_compute_type
   - hashtab_search
- filenametr_hash   traverse @fname as one which has null 
character

Signed-off-by: Chao Yu 
---
 fs/crypto/fname.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 5d6d491..5c356c0 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -110,6 +110,7 @@ static int fname_encrypt(struct inode *inode,
"%s: Error (error code %d)\n", __func__, res);
 
oname->len = ciphertext_len;
+   oname->name[oname->len] = 0;
return res;
 }
 
-- 
2.7.2

[PATCH] fscrypto: fix to null-terminate encrypted filename in fname_encrypt

[Chao Yu]

From: Chao Yu 

This patch fixes to add null character at the end of encrypted filename
in fname_encrypt, in order to avoid incorrectly traversing random data
located after target filename. The call stack is as below:

- f2fs_add_link
 - __f2fs_add_link
  - fscrypt_setup_filename
   - fscrypt_fname_alloc_buffer allocate buffer for @fname
   - fname_encrypt  didn't set null character for @fname
  - f2fs_add_regular_entry  init qstr with @fname
   - init_inode_metadata
- f2fs_init_security
 - security_inode_init_security
  - selinux_inode_init_security
   - selinux_determine_inode_label
- security_transition_sid
 - security_compute_sid
  - filename_compute_type
   - hashtab_search
- filenametr_hash   traverse @fname as one which has null 
character

Signed-off-by: Chao Yu 
---
 fs/crypto/fname.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 5d6d491..5c356c0 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -110,6 +110,7 @@ static int fname_encrypt(struct inode *inode,
"%s: Error (error code %d)\n", __func__, res);
 
oname->len = ciphertext_len;
+   oname->name[oname->len] = 0;
return res;
 }
 
-- 
2.7.2

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> >>- Making checkpatch check for (some) of the stable kernel rules
> >>(and possibly recommend adding the stable@ tag in certain cases?).
> >>  - Depends on: making checkpatch sane again.>This sounds interesting.  
> >>What do you mean by "sane"?
> 
> Sasha, can you expand your thoughts here please?

Sure. I have 2.5 concerns about the state of checkpatch:

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: 
> Most all of the trivial spacing stuff can easily be
> ignored either by a human determining what's important
> or by using command line options like --ignore=spacing

1.
This is the wrong default. By default checkpatch shouldn't be showing trivial
issues that encourage folks to try and work around them and as a result
produce worse code.

Look at the 80 character limit warning for example, what good does it do? It
encourages people to do even stupider things to work around it and results in
a bunch of "fix checkpatch warning" that touch existing code just to make the
result harder to read and make 'git blame' harder to work with.

By default you should only get the most critical warnings we have in the
kernel like missing S-O-B or corrupt patch.


2. A "who wrote these rules?": there seems to be a disconnect between the rules
checkpatch is trying to enforce and the accepted coding style enforced by
maintainers. 

Do a git-format-patch on all of the commits Linus authored in the past year or
two and see how many of them fail checkpatch (or do the same for any of the
commits that passed through and were accepted by the top maintainers),
according to checkpatch we need to make those guys stop touching the kernel.


3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
perl code that a fair amount of people don't know how to deal with. In 4.8 it's
6142 lines, making it the 124th largest source file in the kernel, well within
the top 1% of source files in the kernel.

This combination of size/language pushes people away from being involved in
what is supposed to be a central tool and gives them a reason to never use
it again after they see results they don't agree with (rather than fixing it).

-- 

Thanks,
Sasha

Re: checkkpatch (in)sanity ?

[Levin, Alexander]

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> >>- Making checkpatch check for (some) of the stable kernel rules
> >>(and possibly recommend adding the stable@ tag in certain cases?).
> >>  - Depends on: making checkpatch sane again.>This sounds interesting.  
> >>What do you mean by "sane"?
> 
> Sasha, can you expand your thoughts here please?

Sure. I have 2.5 concerns about the state of checkpatch:

On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: 
> Most all of the trivial spacing stuff can easily be
> ignored either by a human determining what's important
> or by using command line options like --ignore=spacing

1.
This is the wrong default. By default checkpatch shouldn't be showing trivial
issues that encourage folks to try and work around them and as a result
produce worse code.

Look at the 80 character limit warning for example, what good does it do? It
encourages people to do even stupider things to work around it and results in
a bunch of "fix checkpatch warning" that touch existing code just to make the
result harder to read and make 'git blame' harder to work with.

By default you should only get the most critical warnings we have in the
kernel like missing S-O-B or corrupt patch.


2. A "who wrote these rules?": there seems to be a disconnect between the rules
checkpatch is trying to enforce and the accepted coding style enforced by
maintainers. 

Do a git-format-patch on all of the commits Linus authored in the past year or
two and see how many of them fail checkpatch (or do the same for any of the
commits that passed through and were accepted by the top maintainers),
according to checkpatch we need to make those guys stop touching the kernel.


3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
perl code that a fair amount of people don't know how to deal with. In 4.8 it's
6142 lines, making it the 124th largest source file in the kernel, well within
the top 1% of source files in the kernel.

This combination of size/language pushes people away from being involved in
what is supposed to be a central tool and gives them a reason to never use
it again after they see results they don't agree with (rather than fixing it).

-- 

Thanks,
Sasha

[PATCH] staging: rts5208: Add two blank lines in comments.

[MingChia Chung]

This patch fixes a minor checkpatch warnings:

"WARNING: Block comments use a trailing */ on a separate line"

Signed-off-by: Ming-Chia Chung 
---
 drivers/staging/rts5208/rtsx.c | 42 --
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index e992e03..d75fa8d 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -81,14 +81,16 @@ static int slave_alloc(struct scsi_device *sdev)
 
 static int slave_configure(struct scsi_device *sdev)
 {
-   /* Scatter-gather buffers (all but the last) must have a length
+   /*
+* Scatter-gather buffers (all but the last) must have a length
 * divisible by the bulk maxpacket size.  Otherwise a data packet
 * would end up being short, causing a premature end to the data
 * transfer.  Since high-speed bulk pipes have a maxpacket size
 * of 512, we'll use that as the scsi device queue's DMA alignment
 * mask.  Guaranteeing proper alignment of the first buffer will
 * have the desired effect because, except at the beginning and
-* the end, scatter-gather buffers follow page boundaries. */
+* the end, scatter-gather buffers follow page boundaries.
+*/
blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
 
/* Set the SCSI level to at least 2.  We'll leave it at 3 if that's
@@ -185,8 +187,10 @@ static int command_abort(struct scsi_cmnd *srb)
return SUCCESS;
 }
 
-/* This invokes the transport reset mechanism to reset the state of the
- * device */
+/*
+ * This invokes the transport reset mechanism to reset the state of the
+ * device
+ */
 static int device_reset(struct scsi_cmnd *srb)
 {
int result = 0;
@@ -654,15 +658,19 @@ static void rtsx_release_resources(struct rtsx_dev *dev)
kfree(dev->chip);
 }
 
-/* First stage of disconnect processing: stop all commands and remove
- * the host */
+/*
+ * First stage of disconnect processing: stop all commands and remove
+ * the host
+ */
 static void quiesce_and_remove_host(struct rtsx_dev *dev)
 {
struct Scsi_Host *host = rtsx_to_host(dev);
struct rtsx_chip *chip = dev->chip;
 
-   /* Prevent new transfers, stop the current command, and
-* interrupt a SCSI-scan or device-reset delay */
+   /*
+* Prevent new transfers, stop the current command, and
+* interrupt a SCSI-scan or device-reset delay
+*/
mutex_lock(>dev_mutex);
scsi_lock(host);
rtsx_set_stat(chip, RTSX_STAT_DISCONNECT);
@@ -674,9 +682,11 @@ static void quiesce_and_remove_host(struct rtsx_dev *dev)
/* Wait some time to let other threads exist */
wait_timeout(100);
 
-   /* queuecommand won't accept any new commands and the control
+   /*
+* queuecommand won't accept any new commands and the control
 * thread won't execute a previously-queued command.  If there
-* is such a command pending, complete it with an error. */
+* is such a command pending, complete it with an error.
+*/
mutex_lock(>dev_mutex);
if (chip->srb) {
chip->srb->result = DID_NO_CONNECT << 16;
@@ -696,8 +706,10 @@ static void release_everything(struct rtsx_dev *dev)
 {
rtsx_release_resources(dev);
 
-   /* Drop our reference to the host; the SCSI core will free it
-* when the refcount becomes 0. */
+   /*
+* Drop our reference to the host; the SCSI core will free it
+* when the refcount becomes 0.
+*/
scsi_host_put(rtsx_to_host(dev));
 }
 
@@ -936,8 +948,10 @@ static int rtsx_probe(struct pci_dev *pci,
 
rtsx_init_chip(dev->chip);
 
-   /* set the supported max_lun and max_id for the scsi host
-* NOTE: the minimal value of max_id is 1 */
+   /*
+* set the supported max_lun and max_id for the scsi host
+* NOTE: the minimal value of max_id is 1
+*/
host->max_id = 1;
host->max_lun = dev->chip->max_lun;
 
-- 
2.9.2

[PATCH] staging: rts5208: Add two blank lines in comments.

[MingChia Chung]

This patch fixes a minor checkpatch warnings:

"WARNING: Block comments use a trailing */ on a separate line"

Signed-off-by: Ming-Chia Chung 
---
 drivers/staging/rts5208/rtsx.c | 42 --
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index e992e03..d75fa8d 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -81,14 +81,16 @@ static int slave_alloc(struct scsi_device *sdev)
 
 static int slave_configure(struct scsi_device *sdev)
 {
-   /* Scatter-gather buffers (all but the last) must have a length
+   /*
+* Scatter-gather buffers (all but the last) must have a length
 * divisible by the bulk maxpacket size.  Otherwise a data packet
 * would end up being short, causing a premature end to the data
 * transfer.  Since high-speed bulk pipes have a maxpacket size
 * of 512, we'll use that as the scsi device queue's DMA alignment
 * mask.  Guaranteeing proper alignment of the first buffer will
 * have the desired effect because, except at the beginning and
-* the end, scatter-gather buffers follow page boundaries. */
+* the end, scatter-gather buffers follow page boundaries.
+*/
blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
 
/* Set the SCSI level to at least 2.  We'll leave it at 3 if that's
@@ -185,8 +187,10 @@ static int command_abort(struct scsi_cmnd *srb)
return SUCCESS;
 }
 
-/* This invokes the transport reset mechanism to reset the state of the
- * device */
+/*
+ * This invokes the transport reset mechanism to reset the state of the
+ * device
+ */
 static int device_reset(struct scsi_cmnd *srb)
 {
int result = 0;
@@ -654,15 +658,19 @@ static void rtsx_release_resources(struct rtsx_dev *dev)
kfree(dev->chip);
 }
 
-/* First stage of disconnect processing: stop all commands and remove
- * the host */
+/*
+ * First stage of disconnect processing: stop all commands and remove
+ * the host
+ */
 static void quiesce_and_remove_host(struct rtsx_dev *dev)
 {
struct Scsi_Host *host = rtsx_to_host(dev);
struct rtsx_chip *chip = dev->chip;
 
-   /* Prevent new transfers, stop the current command, and
-* interrupt a SCSI-scan or device-reset delay */
+   /*
+* Prevent new transfers, stop the current command, and
+* interrupt a SCSI-scan or device-reset delay
+*/
mutex_lock(>dev_mutex);
scsi_lock(host);
rtsx_set_stat(chip, RTSX_STAT_DISCONNECT);
@@ -674,9 +682,11 @@ static void quiesce_and_remove_host(struct rtsx_dev *dev)
/* Wait some time to let other threads exist */
wait_timeout(100);
 
-   /* queuecommand won't accept any new commands and the control
+   /*
+* queuecommand won't accept any new commands and the control
 * thread won't execute a previously-queued command.  If there
-* is such a command pending, complete it with an error. */
+* is such a command pending, complete it with an error.
+*/
mutex_lock(>dev_mutex);
if (chip->srb) {
chip->srb->result = DID_NO_CONNECT << 16;
@@ -696,8 +706,10 @@ static void release_everything(struct rtsx_dev *dev)
 {
rtsx_release_resources(dev);
 
-   /* Drop our reference to the host; the SCSI core will free it
-* when the refcount becomes 0. */
+   /*
+* Drop our reference to the host; the SCSI core will free it
+* when the refcount becomes 0.
+*/
scsi_host_put(rtsx_to_host(dev));
 }
 
@@ -936,8 +948,10 @@ static int rtsx_probe(struct pci_dev *pci,
 
rtsx_init_chip(dev->chip);
 
-   /* set the supported max_lun and max_id for the scsi host
-* NOTE: the minimal value of max_id is 1 */
+   /*
+* set the supported max_lun and max_id for the scsi host
+* NOTE: the minimal value of max_id is 1
+*/
host->max_id = 1;
host->max_lun = dev->chip->max_lun;
 
-- 
2.9.2

LOANDS AFORDABLE 3% ANNUALLY

[BOTHA]

LOANS UP TO R150 000!
- Blacklisted welcome to apply
- Self-employed welcome to apply
- Must be at least 18 years of age
- Must have a valid ID document

LOANDS AFORDABLE 3% ANNUALLY

[BOTHA]

LOANS UP TO R150 000!
- Blacklisted welcome to apply
- Self-employed welcome to apply
- Must be at least 18 years of age
- Must have a valid ID document

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches]

On Sat, 2016-08-27 at 09:02 +0200, SF Markus Elfring wrote:
> > If you _really wanted to clear up this code and make it more
> > robust/better, it'd probably be nicer to convert the
> > struct list_head **sn_irq_lh to a single struct list_head *
> > That would be less data space overall given the alignment
> > waste of the individual allocs.
> Does this suggestion mean that I should drop my proposal
> around the software components "IRQ" and "TLB" for the system
> architecture "IA64" in such a questionable patch series?

While elimination of code duplication should be good,
what it means it you should avoid making changes that
are merely mechanical and strive to make changes that
improve code execution speed or reduce overall object
size while not impacting overall execution speed.

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches]

On Sat, 2016-08-27 at 09:02 +0200, SF Markus Elfring wrote:
> > If you _really wanted to clear up this code and make it more
> > robust/better, it'd probably be nicer to convert the
> > struct list_head **sn_irq_lh to a single struct list_head *
> > That would be less data space overall given the alignment
> > waste of the individual allocs.
> Does this suggestion mean that I should drop my proposal
> around the software components "IRQ" and "TLB" for the system
> architecture "IA64" in such a questionable patch series?

While elimination of code duplication should be good,
what it means it you should avoid making changes that
are merely mechanical and strive to make changes that
improve code execution speed or reduce overall object
size while not impacting overall execution speed.

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

On 27 August 2016 at 19:22, Wolfram Sang  wrote:
> Uh, that sounds like bad HW...

Making a mess of I2C controllers seems to be a popular hobby among
chip designers :P

( I also really like how the RPi handles clock stretching... *cough* )

> While it surely is nice to have super detailed information, can you give
> this overloaded maintainer a few-line, high level summary of what is
> written there?

What's written there is what should have been in the docs: correct (as
in reality-matching) details of how the thing actually works in slave
mode, and how to operate it without risking race conditions. They're
not comprehensive, and certainly not polished since they are my
personal notes I've made while studying the peripheral in detail, but
I figured both on E2E and here I might perhaps save time and
frustration by making them available.

A lot of the details (including the completely bizarre behaviour of
its innocuous-looking irq registers) would be quite non-trivial to
figure out without putting in a similar effort.

Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

On 27 August 2016 at 19:22, Wolfram Sang  wrote:
> Uh, that sounds like bad HW...

Making a mess of I2C controllers seems to be a popular hobby among
chip designers :P

( I also really like how the RPi handles clock stretching... *cough* )

> While it surely is nice to have super detailed information, can you give
> this overloaded maintainer a few-line, high level summary of what is
> written there?

What's written there is what should have been in the docs: correct (as
in reality-matching) details of how the thing actually works in slave
mode, and how to operate it without risking race conditions. They're
not comprehensive, and certainly not polished since they are my
personal notes I've made while studying the peripheral in detail, but
I figured both on E2E and here I might perhaps save time and
frustration by making them available.

A lot of the details (including the completely bizarre behaviour of
its innocuous-looking irq registers) would be quite non-trivial to
figure out without putting in a similar effort.

Matthijs

Re: [PATCH RFC v3] x86,mm,sched: make lazy TLB mode even lazier

[Linus Torvalds]

Yeah, with those small fixes from Ingo, I definitely don't think this
looks hacky at all. This all seems to be exactly what we should always
have done.

The only remaining comment is that I'd make that
lazy_tlb_can_skip_flush() function just use a switch table for the
tlbstate comparisons rather than the repeated conditionals.

I'd love to see the results from Benjamin - maybe it helps a lot, and
maybe it doesn't. But regardless, the patch makes sense to me.

So with the small fixups: Ack.

 Linus

Re: [PATCH RFC v3] x86,mm,sched: make lazy TLB mode even lazier

[Linus Torvalds]

Yeah, with those small fixes from Ingo, I definitely don't think this
looks hacky at all. This all seems to be exactly what we should always
have done.

The only remaining comment is that I'd make that
lazy_tlb_can_skip_flush() function just use a switch table for the
tlbstate comparisons rather than the repeated conditionals.

I'd love to see the results from Benjamin - maybe it helps a lot, and
maybe it doesn't. But regardless, the patch makes sense to me.

So with the small fixups: Ack.

 Linus

Re: Plan to support Rockchip VPU in DRM, is it a good idea

[Theodore Kilgore]

On Fri, 26 Aug 2016, Randy Li wrote:




On 08/26/2016 05:34 PM, Hans Verkuil wrote:

Hi Randi,

On 08/26/2016 04:13 AM, Randy Li wrote:

Hello,
   We always use some kind of hack work to make our Video Process
Unit(Multi-format Video Encoder/Decoder) work in kernel. From a
customize driver(vpu service) to the customize V4L2 driver. The V4L2
subsystem is really not suitable for the stateless Video process or it
could make driver too fat.
   After talking to some kindness Intel guys and moving our userspace
library to VA-API driver, I find the DRM may the good choice for us.
But I don't know whether it is welcome to to submit a video driver in
DRM subsystem?
   Also our VPU(Video process unit) is not just like the Intel's, we
don't have VCS, we based on registers to set the encoder/decoder. I
think we may need a lots of IOCTL then. Also we do have a IOMMU in VPU
but also not a isolated memory for VPU, I don't know I should use TT
memory or GEM memory.
   I am actually not a member of the department in charge of VPU, and I
am just beginning to learning DRM(thank the help from Intel again), I am
not so good at memory part as well(I am more familiar with CMA not the
IOMMU way), I may need know guide about the implementations when I am
going to submit driver, I hope I could get help from someone.



It makes no sense to do this in the DRM subsystem IMHO. There are already
quite a few HW codecs implemented in the V4L2 subsystem and more are in the
pipeline. Putting codec support in different subsystems will just make
userspace software much harder to write.

One of the codecs that was posted to linux-media was actually from 
Rockchip:


https://lkml.org/lkml/2016/2/29/861

There is also a libVA driver (I think) that sits on top of it:

https://github.com/rockchip-linux/rockchip-va-driver/tree/v4l2-libvpu

It is old version, I am the author of this
https://github.com/rockchip-linux/rockchip-va-driver


For the Allwinner a patch series was posted yesterday:

https://lkml.org/lkml/2016/8/25/246

They created a pretty generic libVA userspace that looks very promising at
first glance.

What these have in common is that they depend on the Request API and Frame 
API,
neither of which has been merged. The problem is that the Request API 
requires
more work since not only controls have to be part of a request, but also 
formats,
selection rectangles, and even dynamic routing changes. While that is not 
relevant
for codecs, it is relevant for Android CameraHAL in general and complex 
devices

like Google's Project Ara.
Actually just as the Intel did, our hardware decoder/encoder need full 
settings for them, most of them are relevant to the codec. You may notice 
that there is four extra control need to be set before. If the libvpu(a 
helper library we offered to parse each slice to generate decoder settings) 
is remove(in process now, only three decoder settings can't got from VA-API 
directly), it would be more clearly.

We really a lots decoder settings information to make the decoder work.


This is being worked on, but it is simply not yet ready. The core V4L2 
developers
involved in this plan to discuss this on the Monday before the ELCE in 
Berlin,
to see if we can fast track this work somehow so this support can be 
merged.


I am glad to hear that. I hope that I could have an opportunity to show our 
problems.
If there are missing features in V4L2 (other that the two APIs discussed 
above)
that prevent you from creating a good driver, then please discuss that with 
us.
We are always open to suggestions and improvements and want to work with 
you on

that.
I have a few experience with the s5p-mfc, and I do wrote a V4L2 encoder 
plugin for Gstreamer.  I don't think the V4L2 is good place for us stateless 
video processor, unless it would break the present implementation.


 The stateful and stateless are operated quite differently. The stateless 
must parse the header and set those settings for every frames.


Then one needs to incorporate in the driver a way to detect whether one 
has to support "stateless" or "stateful." There must be a way to do this 
kind of thing, even if it is not documented by anybody. One way, perhaps, 
might be to look at the data and see if there is any header, or not. Then 
parse the header if it is present, otherwise don't.


The request data may quite different from vendor to vendor, even chip to 
chip.


By "request data" and "chip to chip" I assume you mean the initialization 
of a video chip, or something analogous or similar. Believe it or not, 
this kind of problem has been seen before and dealt with. Look at 
drivers/media/usb/gspca/mr97310a.c for an example. The exact problem that 
we faced was that different cameras had the same USB controller chip (and 
therefore shared a USB vendor:product code), but they had different sensor 
chips with different initialization sequences. The camera vendors merely 
slipped a Windows driver CD into each package, and sometimes they grabbed 

Re: Plan to support Rockchip VPU in DRM, is it a good idea

[Theodore Kilgore]

On Fri, 26 Aug 2016, Randy Li wrote:




On 08/26/2016 05:34 PM, Hans Verkuil wrote:

Hi Randi,

On 08/26/2016 04:13 AM, Randy Li wrote:

Hello,
   We always use some kind of hack work to make our Video Process
Unit(Multi-format Video Encoder/Decoder) work in kernel. From a
customize driver(vpu service) to the customize V4L2 driver. The V4L2
subsystem is really not suitable for the stateless Video process or it
could make driver too fat.
   After talking to some kindness Intel guys and moving our userspace
library to VA-API driver, I find the DRM may the good choice for us.
But I don't know whether it is welcome to to submit a video driver in
DRM subsystem?
   Also our VPU(Video process unit) is not just like the Intel's, we
don't have VCS, we based on registers to set the encoder/decoder. I
think we may need a lots of IOCTL then. Also we do have a IOMMU in VPU
but also not a isolated memory for VPU, I don't know I should use TT
memory or GEM memory.
   I am actually not a member of the department in charge of VPU, and I
am just beginning to learning DRM(thank the help from Intel again), I am
not so good at memory part as well(I am more familiar with CMA not the
IOMMU way), I may need know guide about the implementations when I am
going to submit driver, I hope I could get help from someone.



It makes no sense to do this in the DRM subsystem IMHO. There are already
quite a few HW codecs implemented in the V4L2 subsystem and more are in the
pipeline. Putting codec support in different subsystems will just make
userspace software much harder to write.

One of the codecs that was posted to linux-media was actually from 
Rockchip:


https://lkml.org/lkml/2016/2/29/861

There is also a libVA driver (I think) that sits on top of it:

https://github.com/rockchip-linux/rockchip-va-driver/tree/v4l2-libvpu

It is old version, I am the author of this
https://github.com/rockchip-linux/rockchip-va-driver


For the Allwinner a patch series was posted yesterday:

https://lkml.org/lkml/2016/8/25/246

They created a pretty generic libVA userspace that looks very promising at
first glance.

What these have in common is that they depend on the Request API and Frame 
API,
neither of which has been merged. The problem is that the Request API 
requires
more work since not only controls have to be part of a request, but also 
formats,
selection rectangles, and even dynamic routing changes. While that is not 
relevant
for codecs, it is relevant for Android CameraHAL in general and complex 
devices

like Google's Project Ara.
Actually just as the Intel did, our hardware decoder/encoder need full 
settings for them, most of them are relevant to the codec. You may notice 
that there is four extra control need to be set before. If the libvpu(a 
helper library we offered to parse each slice to generate decoder settings) 
is remove(in process now, only three decoder settings can't got from VA-API 
directly), it would be more clearly.

We really a lots decoder settings information to make the decoder work.


This is being worked on, but it is simply not yet ready. The core V4L2 
developers
involved in this plan to discuss this on the Monday before the ELCE in 
Berlin,
to see if we can fast track this work somehow so this support can be 
merged.


I am glad to hear that. I hope that I could have an opportunity to show our 
problems.
If there are missing features in V4L2 (other that the two APIs discussed 
above)
that prevent you from creating a good driver, then please discuss that with 
us.
We are always open to suggestions and improvements and want to work with 
you on

that.
I have a few experience with the s5p-mfc, and I do wrote a V4L2 encoder 
plugin for Gstreamer.  I don't think the V4L2 is good place for us stateless 
video processor, unless it would break the present implementation.


 The stateful and stateless are operated quite differently. The stateless 
must parse the header and set those settings for every frames.


Then one needs to incorporate in the driver a way to detect whether one 
has to support "stateless" or "stateful." There must be a way to do this 
kind of thing, even if it is not documented by anybody. One way, perhaps, 
might be to look at the data and see if there is any header, or not. Then 
parse the header if it is present, otherwise don't.


The request data may quite different from vendor to vendor, even chip to 
chip.


By "request data" and "chip to chip" I assume you mean the initialization 
of a video chip, or something analogous or similar. Believe it or not, 
this kind of problem has been seen before and dealt with. Look at 
drivers/media/usb/gspca/mr97310a.c for an example. The exact problem that 
we faced was that different cameras had the same USB controller chip (and 
therefore shared a USB vendor:product code), but they had different sensor 
chips with different initialization sequences. The camera vendors merely 
slipped a Windows driver CD into each package, and sometimes they grabbed 

Re: [PATCH] ravb: avoid unused function warnings

[Sergei Shtylyov]

Hello.

On 08/27/2016 01:48 AM, Sergei Shtylyov wrote:


When CONFIG_PM_SLEEP is disabled, we get a couple of harmless warnings:

drivers/net/ethernet/renesas/ravb_main.c:2117:12: error: 'ravb_resume'
defined but not used [-Werror=unused-function]
drivers/net/ethernet/renesas/ravb_main.c:2104:12: error: 'ravb_suspend'
defined but not used [-Werror=unused-function]

The simplest solution here is to replace the #ifdef with __maybe_unused
annotations, which lets the compiler do the right thing by itself.

Signed-off-by: Arnd Bergmann 
Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")


   That was the patch I didn't review, sorry about that...


---
 drivers/net/ethernet/renesas/ravb_main.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index cad23ba06904..630536bc72f9 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

@@ -2166,17 +2165,12 @@ static const struct dev_pm_ops ravb_dev_pm_ops = {
 SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
 };

-#define RAVB_PM_OPS (_dev_pm_ops)
-#else
-#define RAVB_PM_OPS NULL
-#endif
-
 static struct platform_driver ravb_driver = {
 .probe= ravb_probe,
 .remove= ravb_remove,
 .driver = {
 .name= "ravb",
-.pm= RAVB_PM_OPS,
+.pm= _dev_pm_ops,


   I guess it's safe to have this pointing to the 'struct dev_pm_ops' filled
with NULLs? Looking at the PM code left me somewhat unsure...


   Well, this means CONFIG_PM[_SLEEP] not defined anyway and the PM code 
calling the methods absent, so it should be safe indeed...


Acked-by: Sergei Shtylyov 

MBR, Sergei

Re: [PATCH] ravb: avoid unused function warnings

[Sergei Shtylyov]

Hello.

On 08/27/2016 01:48 AM, Sergei Shtylyov wrote:


When CONFIG_PM_SLEEP is disabled, we get a couple of harmless warnings:

drivers/net/ethernet/renesas/ravb_main.c:2117:12: error: 'ravb_resume'
defined but not used [-Werror=unused-function]
drivers/net/ethernet/renesas/ravb_main.c:2104:12: error: 'ravb_suspend'
defined but not used [-Werror=unused-function]

The simplest solution here is to replace the #ifdef with __maybe_unused
annotations, which lets the compiler do the right thing by itself.

Signed-off-by: Arnd Bergmann 
Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")


   That was the patch I didn't review, sorry about that...


---
 drivers/net/ethernet/renesas/ravb_main.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index cad23ba06904..630536bc72f9 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

@@ -2166,17 +2165,12 @@ static const struct dev_pm_ops ravb_dev_pm_ops = {
 SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
 };

-#define RAVB_PM_OPS (_dev_pm_ops)
-#else
-#define RAVB_PM_OPS NULL
-#endif
-
 static struct platform_driver ravb_driver = {
 .probe= ravb_probe,
 .remove= ravb_remove,
 .driver = {
 .name= "ravb",
-.pm= RAVB_PM_OPS,
+.pm= _dev_pm_ops,


   I guess it's safe to have this pointing to the 'struct dev_pm_ops' filled
with NULLs? Looking at the PM code left me somewhat unsure...


   Well, this means CONFIG_PM[_SLEEP] not defined anyway and the PM code 
calling the methods absent, so it should be safe indeed...


Acked-by: Sergei Shtylyov 

MBR, Sergei

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Mickaël Salaün]

On 27/08/2016 22:56, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 20:19, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:

 On 27/08/2016 01:05, Alexei Starovoitov wrote:
> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>
>>> As far as safety and type checking that bpf programs has to do,
>>> I like the approach of patch 06/10:
>>> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
>>> +   PTR_TO_STRUCT_FILE, struct file *, file,
>>> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
>>> +)
>>> teaching verifier to recognize struct file, cred, sockaddr
>>> will let bpf program access them naturally without any overhead.
>>> Though:
>>> @@ -102,6 +102,9 @@ enum bpf_prog_type {
>>> BPF_PROG_TYPE_SCHED_CLS,
>>> BPF_PROG_TYPE_SCHED_ACT,
>>> BPF_PROG_TYPE_TRACEPOINT,
>>> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
>>> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
>>> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
>>>  };
>>> is a bit of overkill.
>>> I think it would be cleaner to have single
>>> BPF_PROG_TYPE_LSM and at program load time pass
>>> lsm_hook_id as well, so that verifier can do safety checks
>>> based on type info provided in LANDLOCK_HOOKs
>>
>> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
>> verifier check programs according to their types. If we need to check
>> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
>> dedicated program types. I don't see any other way to do it with the
>> current verifier code. Moreover it's the purpose of program types, right?
>
> Adding new bpf program type for every lsm hook is not acceptable.
> Either do one new program type + pass lsm_hook_id as suggested
> or please come up with an alternative approach.

 OK, so we have to modify the verifier to not only rely on the program
 type but on another value to check the context accesses. Do you have a
 hint from where this value could come from? Do we need to add a new bpf
 command to associate a program to a subtype?
>>>
>>> It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
>>> Both prog_type and prog_hook_id are used during verification.
>>> prog_type distinguishes the main aspects whereas prog_hook_id selects
>>> which lsm_hook's argument definition to apply.
>>> At the time of attaching to a hook, the prog_hook_id passed at the
>>> load time should match lsm's hook_id.
>>
>> OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
>> right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?
> 
> No new command. It will be an optional field to existing BPF_PROG_LOAD.
> In other words instead of replicating everything that different
> bpf_prog_type-s need, we can pass this hook_id field to fine tune
> the purpose (and applicability) of the program.
> And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks.
> For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety
> for all tracepoints while they all have different arguments.
> For tracepoints it's easier, since the only difference between them
> is the range of ctx access. Here we need strong type safety
> of arguments therefore need extra hook_id at load time.

OK, I will do it.



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Mickaël Salaün]

On 27/08/2016 22:56, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 20:19, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:

 On 27/08/2016 01:05, Alexei Starovoitov wrote:
> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>
>>> As far as safety and type checking that bpf programs has to do,
>>> I like the approach of patch 06/10:
>>> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
>>> +   PTR_TO_STRUCT_FILE, struct file *, file,
>>> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
>>> +)
>>> teaching verifier to recognize struct file, cred, sockaddr
>>> will let bpf program access them naturally without any overhead.
>>> Though:
>>> @@ -102,6 +102,9 @@ enum bpf_prog_type {
>>> BPF_PROG_TYPE_SCHED_CLS,
>>> BPF_PROG_TYPE_SCHED_ACT,
>>> BPF_PROG_TYPE_TRACEPOINT,
>>> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
>>> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
>>> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
>>>  };
>>> is a bit of overkill.
>>> I think it would be cleaner to have single
>>> BPF_PROG_TYPE_LSM and at program load time pass
>>> lsm_hook_id as well, so that verifier can do safety checks
>>> based on type info provided in LANDLOCK_HOOKs
>>
>> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
>> verifier check programs according to their types. If we need to check
>> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
>> dedicated program types. I don't see any other way to do it with the
>> current verifier code. Moreover it's the purpose of program types, right?
>
> Adding new bpf program type for every lsm hook is not acceptable.
> Either do one new program type + pass lsm_hook_id as suggested
> or please come up with an alternative approach.

 OK, so we have to modify the verifier to not only rely on the program
 type but on another value to check the context accesses. Do you have a
 hint from where this value could come from? Do we need to add a new bpf
 command to associate a program to a subtype?
>>>
>>> It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
>>> Both prog_type and prog_hook_id are used during verification.
>>> prog_type distinguishes the main aspects whereas prog_hook_id selects
>>> which lsm_hook's argument definition to apply.
>>> At the time of attaching to a hook, the prog_hook_id passed at the
>>> load time should match lsm's hook_id.
>>
>> OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
>> right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?
> 
> No new command. It will be an optional field to existing BPF_PROG_LOAD.
> In other words instead of replicating everything that different
> bpf_prog_type-s need, we can pass this hook_id field to fine tune
> the purpose (and applicability) of the program.
> And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks.
> For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety
> for all tracepoints while they all have different arguments.
> For tracepoints it's easier, since the only difference between them
> is the range of ctx access. Here we need strong type safety
> of arguments therefore need extra hook_id at load time.

OK, I will do it.



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

On 27/08/2016 22:43, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
 As said above, Landlock will not run an eBPF programs when not strictly
 needed. Attaching to a cgroup will have the same performance impact as
 attaching to a process hierarchy.
>>>
>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>> could come up with. If you see another way, please propose.
>>> current->seccomp.landlock_prog is not the answer.
>>
>> Hum, I don't see the difference from a performance point of view between
>> a cgroup-based or a process hierarchy-based system.
>>
>> Maybe a better option should be to use an array of pointers with N
>> entries, one for each supported hook, instead of a unique pointer list?
> 
> yes, clearly array dereference is faster than link list walk.
> Now the question is where to keep this prog_array[num_lsm_hooks] ?
> Since we cannot keep it inside task_struct, we have to allocate it.
> Every time the task is creted then. What to do on the fork? That
> will require changes all over. Then the obvious optimization would be
> to share this allocated array of prog pointers across multiple tasks...
> and little by little this new facility will look like cgroup.
> Hence the suggestion to put this array into cgroup from the start.

I see your point :)

> 
>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>> to use a process hierarchy). The downside will be to handle an LSM hook
>> program which is not triggered by a seccomp-filter, but this should be
>> needed anyway to handle interruptions.
> 
> what do you mean 'not triggered by seccomp' ?
> You're not suggesting that this lsm has to enable seccomp to be functional?
> imo that's non starter due to overhead.

Yes, for now, it is triggered by a new seccomp filter return value
RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
be needed but could be useful to bind a seccomp filter security policy
with a Landlock one. Waiting for Kees's point of view…



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

On 27/08/2016 22:43, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
>> On 27/08/2016 20:06, Alexei Starovoitov wrote:
>>> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
 As said above, Landlock will not run an eBPF programs when not strictly
 needed. Attaching to a cgroup will have the same performance impact as
 attaching to a process hierarchy.
>>>
>>> Having a prog per cgroup per lsm_hook is the only scalable way I
>>> could come up with. If you see another way, please propose.
>>> current->seccomp.landlock_prog is not the answer.
>>
>> Hum, I don't see the difference from a performance point of view between
>> a cgroup-based or a process hierarchy-based system.
>>
>> Maybe a better option should be to use an array of pointers with N
>> entries, one for each supported hook, instead of a unique pointer list?
> 
> yes, clearly array dereference is faster than link list walk.
> Now the question is where to keep this prog_array[num_lsm_hooks] ?
> Since we cannot keep it inside task_struct, we have to allocate it.
> Every time the task is creted then. What to do on the fork? That
> will require changes all over. Then the obvious optimization would be
> to share this allocated array of prog pointers across multiple tasks...
> and little by little this new facility will look like cgroup.
> Hence the suggestion to put this array into cgroup from the start.

I see your point :)

> 
>> Anyway, being able to attach an LSM hook program to a cgroup thanks to
>> the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
>> to use a process hierarchy). The downside will be to handle an LSM hook
>> program which is not triggered by a seccomp-filter, but this should be
>> needed anyway to handle interruptions.
> 
> what do you mean 'not triggered by seccomp' ?
> You're not suggesting that this lsm has to enable seccomp to be functional?
> imo that's non starter due to overhead.

Yes, for now, it is triggered by a new seccomp filter return value
RET_LANDLOCK, which can take a 16-bit value called cookie. This must not
be needed but could be useful to bind a seccomp filter security policy
with a Landlock one. Waiting for Kees's point of view…



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

On Sat, Aug 27, 2016 at 12:38:38PM +0100, Ben Hutchings wrote:
> On Sat, 2016-08-27 at 11:31 +0200, Willy Tarreau wrote:
> > Greg, Jiri,
> > 
> > I checked Jari's explanation below and found that v3.14.77 and v3.12.62
> > are missing the same fix as 3.10. In fact Al's original commit 3d56c25
> > ("fix d_walk()/non-delayed __d_free() race") used to mention to check 
> > this __d_materialise_dentry() function in the Cc: stable line, but this
> > got lost during the backports.
> > 
> > Normally all of our 3 kernels need to apply the following patch that
> > Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
> > right now.
> 
> I never did get positive confirmation that this is the right change in
> __d_materialise_dentry().  Al, could you please comment?

Well in my experience Al checks our reviews and steps in when there's
a mistake. Also your patch seems to reproduce the fix for the code
that was later killed by commit 63cf427 ("kill __d_materialise_dentry()")
which factors it out into __d_move() so I'm inclined to think that what
you did makes sense.

Cheers,
Willy

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

On Sat, Aug 27, 2016 at 12:38:38PM +0100, Ben Hutchings wrote:
> On Sat, 2016-08-27 at 11:31 +0200, Willy Tarreau wrote:
> > Greg, Jiri,
> > 
> > I checked Jari's explanation below and found that v3.14.77 and v3.12.62
> > are missing the same fix as 3.10. In fact Al's original commit 3d56c25
> > ("fix d_walk()/non-delayed __d_free() race") used to mention to check 
> > this __d_materialise_dentry() function in the Cc: stable line, but this
> > got lost during the backports.
> > 
> > Normally all of our 3 kernels need to apply the following patch that
> > Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
> > right now.
> 
> I never did get positive confirmation that this is the right change in
> __d_materialise_dentry().  Al, could you please comment?

Well in my experience Al checks our reviews and steps in when there's
a mistake. Also your patch seems to reproduce the fix for the code
that was later killed by commit 63cf427 ("kill __d_materialise_dentry()")
which factors it out into __d_move() so I'm inclined to think that what
you did makes sense.

Cheers,
Willy

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 20:19, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>>
> > As far as safety and type checking that bpf programs has to do,
> > I like the approach of patch 06/10:
> > +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> > +   PTR_TO_STRUCT_FILE, struct file *, file,
> > +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> > +)
> > teaching verifier to recognize struct file, cred, sockaddr
> > will let bpf program access them naturally without any overhead.
> > Though:
> > @@ -102,6 +102,9 @@ enum bpf_prog_type {
> > BPF_PROG_TYPE_SCHED_CLS,
> > BPF_PROG_TYPE_SCHED_ACT,
> > BPF_PROG_TYPE_TRACEPOINT,
> > +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> > +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> > +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >  };
> > is a bit of overkill.
> > I think it would be cleaner to have single
> > BPF_PROG_TYPE_LSM and at program load time pass
> > lsm_hook_id as well, so that verifier can do safety checks
> > based on type info provided in LANDLOCK_HOOKs
> 
>  I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
>  verifier check programs according to their types. If we need to check
>  specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
>  dedicated program types. I don't see any other way to do it with the
>  current verifier code. Moreover it's the purpose of program types, right?
> >>>
> >>> Adding new bpf program type for every lsm hook is not acceptable.
> >>> Either do one new program type + pass lsm_hook_id as suggested
> >>> or please come up with an alternative approach.
> >>
> >> OK, so we have to modify the verifier to not only rely on the program
> >> type but on another value to check the context accesses. Do you have a
> >> hint from where this value could come from? Do we need to add a new bpf
> >> command to associate a program to a subtype?
> > 
> > It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
> > Both prog_type and prog_hook_id are used during verification.
> > prog_type distinguishes the main aspects whereas prog_hook_id selects
> > which lsm_hook's argument definition to apply.
> > At the time of attaching to a hook, the prog_hook_id passed at the
> > load time should match lsm's hook_id.
> 
> OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
> right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?

No new command. It will be an optional field to existing BPF_PROG_LOAD.
In other words instead of replicating everything that different
bpf_prog_type-s need, we can pass this hook_id field to fine tune
the purpose (and applicability) of the program.
And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks.
For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety
for all tracepoints while they all have different arguments.
For tracepoints it's easier, since the only difference between them
is the range of ctx access. Here we need strong type safety
of arguments therefore need extra hook_id at load time.

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 20:19, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>>
> > As far as safety and type checking that bpf programs has to do,
> > I like the approach of patch 06/10:
> > +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> > +   PTR_TO_STRUCT_FILE, struct file *, file,
> > +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> > +)
> > teaching verifier to recognize struct file, cred, sockaddr
> > will let bpf program access them naturally without any overhead.
> > Though:
> > @@ -102,6 +102,9 @@ enum bpf_prog_type {
> > BPF_PROG_TYPE_SCHED_CLS,
> > BPF_PROG_TYPE_SCHED_ACT,
> > BPF_PROG_TYPE_TRACEPOINT,
> > +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> > +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> > +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >  };
> > is a bit of overkill.
> > I think it would be cleaner to have single
> > BPF_PROG_TYPE_LSM and at program load time pass
> > lsm_hook_id as well, so that verifier can do safety checks
> > based on type info provided in LANDLOCK_HOOKs
> 
>  I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
>  verifier check programs according to their types. If we need to check
>  specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
>  dedicated program types. I don't see any other way to do it with the
>  current verifier code. Moreover it's the purpose of program types, right?
> >>>
> >>> Adding new bpf program type for every lsm hook is not acceptable.
> >>> Either do one new program type + pass lsm_hook_id as suggested
> >>> or please come up with an alternative approach.
> >>
> >> OK, so we have to modify the verifier to not only rely on the program
> >> type but on another value to check the context accesses. Do you have a
> >> hint from where this value could come from? Do we need to add a new bpf
> >> command to associate a program to a subtype?
> > 
> > It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
> > Both prog_type and prog_hook_id are used during verification.
> > prog_type distinguishes the main aspects whereas prog_hook_id selects
> > which lsm_hook's argument definition to apply.
> > At the time of attaching to a hook, the prog_hook_id passed at the
> > load time should match lsm's hook_id.
> 
> OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
> right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?

No new command. It will be an optional field to existing BPF_PROG_LOAD.
In other words instead of replicating everything that different
bpf_prog_type-s need, we can pass this hook_id field to fine tune
the purpose (and applicability) of the program.
And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks.
For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety
for all tracepoints while they all have different arguments.
For tracepoints it's easier, since the only difference between them
is the range of ctx access. Here we need strong type safety
of arguments therefore need extra hook_id at load time.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> 
> >
> > - I don't think such 'for' loop can scale. The solution needs to work
> > with thousands of containers and thousands of cgroups.
> > In the patch 06/10 the proposal is to use 'current' as holder of
> > the programs:
> > +   for (prog = current->seccomp.landlock_prog;
> > +   prog; prog = prog->prev) {
> > +   if (prog->filter == landlock_ret->filter) {
> > +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> > +   break;
> > +   }
> > +   }
> > imo that's the root of scalability issue.
> > I think to be able to scale the bpf programs have to be attached to
> > cgroups instead of tasks.
> > That would be very different api. seccomp doesn't need to be touched.
> > But that is the only way I see to be able to scale.
> 
>  Landlock is inspired from seccomp which also use a BPF program per
>  thread. For seccomp, each BPF programs are executed for each syscall.
>  For Landlock, some BPF programs are executed for some LSM hooks. I don't
>  see why it is a scale issue for Landlock comparing to seccomp. I also
>  don't see why storing the BPF program list pointer in the cgroup struct
>  instead of the task struct change a lot here. The BPF programs execution
>  will be the same anyway (for each LSM hook). Kees should probably have a
>  better opinion on this.
> >>>
> >>> seccomp has its own issues and copying them doesn't make this lsm any 
> >>> better.
> >>> Like seccomp bpf programs are all gigantic switch statement that looks
> >>> for interesting syscall numbers. All syscalls of a task are paying
> >>> non-trivial seccomp penalty due to such design. If bpf was attached per
> >>> syscall it would have been much cheaper. Of course doing it this way
> >>> for seccomp is not easy, but for lsm such facility is already there.
> >>> Blank call of a single bpf prog for all lsm hooks is unnecessary
> >>> overhead that can and should be avoided.
> >>
> >> It's probably a misunderstanding. Contrary to seccomp which run all the
> >> thread's BPF programs for any syscall, Landlock only run eBPF programs
> >> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> >> multiple eBPF program types and contrary to seccomp, Landlock only run
> >> an eBPF program when needed. Landlock will have almost no performance
> >> overhead if the syscalls do not trigger the watched LSM hooks for the
> >> current process.
> > 
> > that's not what I see in the patch 06/10:
> > all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> > (which eventually means all lsm hooks) will call
> > static inline int landlock_hook_##NAME
> > which will call landlock_run_prog()
> > which does:
> > + for (landlock_ret = current->seccomp.landlock_ret;
> > +  landlock_ret; landlock_ret = landlock_ret->prev) {
> > +if (landlock_ret->triggered) {
> > +   ctx.cookie = landlock_ret->cookie;
> > +   for (prog = current->seccomp.landlock_prog;
> > +prog; prog = prog->prev) {
> > +   if (prog->filter == landlock_ret->filter) {
> > +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> > +   break;
> > +   }
> > +   }
> > 
> > that is unacceptable overhead and not a scalable design.
> > It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> > as soon as more lsm hooks are added.
> 
> Good catch! I forgot to check the program (sub)type in the loop to only
> run the needed programs for the current hook. I will fix this.
> 
> 
> > 
> >> As said above, Landlock will not run an eBPF programs when not strictly
> >> needed. Attaching to a cgroup will have the same performance impact as
> >> attaching to a process hierarchy.
> > 
> > Having a prog per cgroup per lsm_hook is the only scalable way I
> > could come up with. If you see another way, please propose.
> > current->seccomp.landlock_prog is not the answer.
> 
> Hum, I don't see the difference from a performance point of view between
> a cgroup-based or a process hierarchy-based system.
> 
> Maybe a better option should be to use an array of pointers with N
> entries, one for each supported hook, instead of a unique pointer list?

yes, clearly array dereference is faster than link list walk.
Now the question is where to keep this prog_array[num_lsm_hooks] ?
Since we cannot keep it inside task_struct, we have to allocate it.
Every time the task is creted then. What to do on the fork? That
will require changes all over. Then the obvious optimization would be
to share this allocated array of prog 

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 09:35:14PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 20:06, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> 
> >
> > - I don't think such 'for' loop can scale. The solution needs to work
> > with thousands of containers and thousands of cgroups.
> > In the patch 06/10 the proposal is to use 'current' as holder of
> > the programs:
> > +   for (prog = current->seccomp.landlock_prog;
> > +   prog; prog = prog->prev) {
> > +   if (prog->filter == landlock_ret->filter) {
> > +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> > +   break;
> > +   }
> > +   }
> > imo that's the root of scalability issue.
> > I think to be able to scale the bpf programs have to be attached to
> > cgroups instead of tasks.
> > That would be very different api. seccomp doesn't need to be touched.
> > But that is the only way I see to be able to scale.
> 
>  Landlock is inspired from seccomp which also use a BPF program per
>  thread. For seccomp, each BPF programs are executed for each syscall.
>  For Landlock, some BPF programs are executed for some LSM hooks. I don't
>  see why it is a scale issue for Landlock comparing to seccomp. I also
>  don't see why storing the BPF program list pointer in the cgroup struct
>  instead of the task struct change a lot here. The BPF programs execution
>  will be the same anyway (for each LSM hook). Kees should probably have a
>  better opinion on this.
> >>>
> >>> seccomp has its own issues and copying them doesn't make this lsm any 
> >>> better.
> >>> Like seccomp bpf programs are all gigantic switch statement that looks
> >>> for interesting syscall numbers. All syscalls of a task are paying
> >>> non-trivial seccomp penalty due to such design. If bpf was attached per
> >>> syscall it would have been much cheaper. Of course doing it this way
> >>> for seccomp is not easy, but for lsm such facility is already there.
> >>> Blank call of a single bpf prog for all lsm hooks is unnecessary
> >>> overhead that can and should be avoided.
> >>
> >> It's probably a misunderstanding. Contrary to seccomp which run all the
> >> thread's BPF programs for any syscall, Landlock only run eBPF programs
> >> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> >> multiple eBPF program types and contrary to seccomp, Landlock only run
> >> an eBPF program when needed. Landlock will have almost no performance
> >> overhead if the syscalls do not trigger the watched LSM hooks for the
> >> current process.
> > 
> > that's not what I see in the patch 06/10:
> > all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> > (which eventually means all lsm hooks) will call
> > static inline int landlock_hook_##NAME
> > which will call landlock_run_prog()
> > which does:
> > + for (landlock_ret = current->seccomp.landlock_ret;
> > +  landlock_ret; landlock_ret = landlock_ret->prev) {
> > +if (landlock_ret->triggered) {
> > +   ctx.cookie = landlock_ret->cookie;
> > +   for (prog = current->seccomp.landlock_prog;
> > +prog; prog = prog->prev) {
> > +   if (prog->filter == landlock_ret->filter) {
> > +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> > +   break;
> > +   }
> > +   }
> > 
> > that is unacceptable overhead and not a scalable design.
> > It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> > as soon as more lsm hooks are added.
> 
> Good catch! I forgot to check the program (sub)type in the loop to only
> run the needed programs for the current hook. I will fix this.
> 
> 
> > 
> >> As said above, Landlock will not run an eBPF programs when not strictly
> >> needed. Attaching to a cgroup will have the same performance impact as
> >> attaching to a process hierarchy.
> > 
> > Having a prog per cgroup per lsm_hook is the only scalable way I
> > could come up with. If you see another way, please propose.
> > current->seccomp.landlock_prog is not the answer.
> 
> Hum, I don't see the difference from a performance point of view between
> a cgroup-based or a process hierarchy-based system.
> 
> Maybe a better option should be to use an array of pointers with N
> entries, one for each supported hook, instead of a unique pointer list?

yes, clearly array dereference is faster than link list walk.
Now the question is where to keep this prog_array[num_lsm_hooks] ?
Since we cannot keep it inside task_struct, we have to allocate it.
Every time the task is creted then. What to do on the fork? That
will require changes all over. Then the obvious optimization would be
to share this allocated array of prog 

checkkpatch (in)sanity ?

[Joe Perches]

On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
>>- Making checkpatch check for (some) of the stable kernel rules
>>(and possibly recommend adding the stable@ tag in certain cases?).
>>  - Depends on: making checkpatch sane again.>This sounds interesting.  
>>What do you mean by "sane"?

Sasha, can you expand your thoughts here please?

Most all of the trivial spacing stuff can easily be
ignored either by a human determining what's important
or by using command line options like --ignore=spacing

checkkpatch (in)sanity ?

[Joe Perches]

On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
>>- Making checkpatch check for (some) of the stable kernel rules
>>(and possibly recommend adding the stable@ tag in certain cases?).
>>  - Depends on: making checkpatch sane again.>This sounds interesting.  
>>What do you mean by "sane"?

Sasha, can you expand your thoughts here please?

Most all of the trivial spacing stuff can easily be
ignored either by a human determining what's important
or by using command line options like --ignore=spacing

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Mickaël Salaün]

On 27/08/2016 20:19, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 01:05, Alexei Starovoitov wrote:
>>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>>>
> As far as safety and type checking that bpf programs has to do,
> I like the approach of patch 06/10:
> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> +   PTR_TO_STRUCT_FILE, struct file *, file,
> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> +)
> teaching verifier to recognize struct file, cred, sockaddr
> will let bpf program access them naturally without any overhead.
> Though:
> @@ -102,6 +102,9 @@ enum bpf_prog_type {
> BPF_PROG_TYPE_SCHED_CLS,
> BPF_PROG_TYPE_SCHED_ACT,
> BPF_PROG_TYPE_TRACEPOINT,
> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
>  };
> is a bit of overkill.
> I think it would be cleaner to have single
> BPF_PROG_TYPE_LSM and at program load time pass
> lsm_hook_id as well, so that verifier can do safety checks
> based on type info provided in LANDLOCK_HOOKs

 I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
 verifier check programs according to their types. If we need to check
 specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
 dedicated program types. I don't see any other way to do it with the
 current verifier code. Moreover it's the purpose of program types, right?
>>>
>>> Adding new bpf program type for every lsm hook is not acceptable.
>>> Either do one new program type + pass lsm_hook_id as suggested
>>> or please come up with an alternative approach.
>>
>> OK, so we have to modify the verifier to not only rely on the program
>> type but on another value to check the context accesses. Do you have a
>> hint from where this value could come from? Do we need to add a new bpf
>> command to associate a program to a subtype?
> 
> It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
> Both prog_type and prog_hook_id are used during verification.
> prog_type distinguishes the main aspects whereas prog_hook_id selects
> which lsm_hook's argument definition to apply.
> At the time of attaching to a hook, the prog_hook_id passed at the
> load time should match lsm's hook_id.

OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Mickaël Salaün]

On 27/08/2016 20:19, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 01:05, Alexei Starovoitov wrote:
>>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
>>>
> As far as safety and type checking that bpf programs has to do,
> I like the approach of patch 06/10:
> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> +   PTR_TO_STRUCT_FILE, struct file *, file,
> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> +)
> teaching verifier to recognize struct file, cred, sockaddr
> will let bpf program access them naturally without any overhead.
> Though:
> @@ -102,6 +102,9 @@ enum bpf_prog_type {
> BPF_PROG_TYPE_SCHED_CLS,
> BPF_PROG_TYPE_SCHED_ACT,
> BPF_PROG_TYPE_TRACEPOINT,
> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
>  };
> is a bit of overkill.
> I think it would be cleaner to have single
> BPF_PROG_TYPE_LSM and at program load time pass
> lsm_hook_id as well, so that verifier can do safety checks
> based on type info provided in LANDLOCK_HOOKs

 I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
 verifier check programs according to their types. If we need to check
 specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
 dedicated program types. I don't see any other way to do it with the
 current verifier code. Moreover it's the purpose of program types, right?
>>>
>>> Adding new bpf program type for every lsm hook is not acceptable.
>>> Either do one new program type + pass lsm_hook_id as suggested
>>> or please come up with an alternative approach.
>>
>> OK, so we have to modify the verifier to not only rely on the program
>> type but on another value to check the context accesses. Do you have a
>> hint from where this value could come from? Do we need to add a new bpf
>> command to associate a program to a subtype?
> 
> It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
> Both prog_type and prog_hook_id are used during verification.
> prog_type distinguishes the main aspects whereas prog_hook_id selects
> which lsm_hook's argument definition to apply.
> At the time of attaching to a hook, the prog_hook_id passed at the
> load time should match lsm's hook_id.

OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

On 27/08/2016 20:06, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 01:05, Alexei Starovoitov wrote:
>>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:

>
> - I don't think such 'for' loop can scale. The solution needs to work
> with thousands of containers and thousands of cgroups.
> In the patch 06/10 the proposal is to use 'current' as holder of
> the programs:
> +   for (prog = current->seccomp.landlock_prog;
> +   prog; prog = prog->prev) {
> +   if (prog->filter == landlock_ret->filter) {
> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> +   break;
> +   }
> +   }
> imo that's the root of scalability issue.
> I think to be able to scale the bpf programs have to be attached to
> cgroups instead of tasks.
> That would be very different api. seccomp doesn't need to be touched.
> But that is the only way I see to be able to scale.

 Landlock is inspired from seccomp which also use a BPF program per
 thread. For seccomp, each BPF programs are executed for each syscall.
 For Landlock, some BPF programs are executed for some LSM hooks. I don't
 see why it is a scale issue for Landlock comparing to seccomp. I also
 don't see why storing the BPF program list pointer in the cgroup struct
 instead of the task struct change a lot here. The BPF programs execution
 will be the same anyway (for each LSM hook). Kees should probably have a
 better opinion on this.
>>>
>>> seccomp has its own issues and copying them doesn't make this lsm any 
>>> better.
>>> Like seccomp bpf programs are all gigantic switch statement that looks
>>> for interesting syscall numbers. All syscalls of a task are paying
>>> non-trivial seccomp penalty due to such design. If bpf was attached per
>>> syscall it would have been much cheaper. Of course doing it this way
>>> for seccomp is not easy, but for lsm such facility is already there.
>>> Blank call of a single bpf prog for all lsm hooks is unnecessary
>>> overhead that can and should be avoided.
>>
>> It's probably a misunderstanding. Contrary to seccomp which run all the
>> thread's BPF programs for any syscall, Landlock only run eBPF programs
>> for the triggered LSM hooks, if their type match. Indeed, thanks to the
>> multiple eBPF program types and contrary to seccomp, Landlock only run
>> an eBPF program when needed. Landlock will have almost no performance
>> overhead if the syscalls do not trigger the watched LSM hooks for the
>> current process.
> 
> that's not what I see in the patch 06/10:
> all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> (which eventually means all lsm hooks) will call
> static inline int landlock_hook_##NAME
> which will call landlock_run_prog()
> which does:
> + for (landlock_ret = current->seccomp.landlock_ret;
> +  landlock_ret; landlock_ret = landlock_ret->prev) {
> +if (landlock_ret->triggered) {
> +   ctx.cookie = landlock_ret->cookie;
> +   for (prog = current->seccomp.landlock_prog;
> +prog; prog = prog->prev) {
> +   if (prog->filter == landlock_ret->filter) {
> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> +   break;
> +   }
> +   }
> 
> that is unacceptable overhead and not a scalable design.
> It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> as soon as more lsm hooks are added.

Good catch! I forgot to check the program (sub)type in the loop to only
run the needed programs for the current hook. I will fix this.


> 
>> As said above, Landlock will not run an eBPF programs when not strictly
>> needed. Attaching to a cgroup will have the same performance impact as
>> attaching to a process hierarchy.
> 
> Having a prog per cgroup per lsm_hook is the only scalable way I
> could come up with. If you see another way, please propose.
> current->seccomp.landlock_prog is not the answer.

Hum, I don't see the difference from a performance point of view between
a cgroup-based or a process hierarchy-based system.

Maybe a better option should be to use an array of pointers with N
entries, one for each supported hook, instead of a unique pointer list?

Anyway, being able to attach an LSM hook program to a cgroup thanks to
the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
to use a process hierarchy). The downside will be to handle an LSM hook
program which is not triggered by a seccomp-filter, but this should be
needed anyway to handle interruptions.



signature.asc
Description: OpenPGP digital signature

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Mickaël Salaün]

On 27/08/2016 20:06, Alexei Starovoitov wrote:
> On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
>>
>> On 27/08/2016 01:05, Alexei Starovoitov wrote:
>>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:

>
> - I don't think such 'for' loop can scale. The solution needs to work
> with thousands of containers and thousands of cgroups.
> In the patch 06/10 the proposal is to use 'current' as holder of
> the programs:
> +   for (prog = current->seccomp.landlock_prog;
> +   prog; prog = prog->prev) {
> +   if (prog->filter == landlock_ret->filter) {
> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> +   break;
> +   }
> +   }
> imo that's the root of scalability issue.
> I think to be able to scale the bpf programs have to be attached to
> cgroups instead of tasks.
> That would be very different api. seccomp doesn't need to be touched.
> But that is the only way I see to be able to scale.

 Landlock is inspired from seccomp which also use a BPF program per
 thread. For seccomp, each BPF programs are executed for each syscall.
 For Landlock, some BPF programs are executed for some LSM hooks. I don't
 see why it is a scale issue for Landlock comparing to seccomp. I also
 don't see why storing the BPF program list pointer in the cgroup struct
 instead of the task struct change a lot here. The BPF programs execution
 will be the same anyway (for each LSM hook). Kees should probably have a
 better opinion on this.
>>>
>>> seccomp has its own issues and copying them doesn't make this lsm any 
>>> better.
>>> Like seccomp bpf programs are all gigantic switch statement that looks
>>> for interesting syscall numbers. All syscalls of a task are paying
>>> non-trivial seccomp penalty due to such design. If bpf was attached per
>>> syscall it would have been much cheaper. Of course doing it this way
>>> for seccomp is not easy, but for lsm such facility is already there.
>>> Blank call of a single bpf prog for all lsm hooks is unnecessary
>>> overhead that can and should be avoided.
>>
>> It's probably a misunderstanding. Contrary to seccomp which run all the
>> thread's BPF programs for any syscall, Landlock only run eBPF programs
>> for the triggered LSM hooks, if their type match. Indeed, thanks to the
>> multiple eBPF program types and contrary to seccomp, Landlock only run
>> an eBPF program when needed. Landlock will have almost no performance
>> overhead if the syscalls do not trigger the watched LSM hooks for the
>> current process.
> 
> that's not what I see in the patch 06/10:
> all lsm_hooks in 'static struct security_hook_list landlock_hooks'
> (which eventually means all lsm hooks) will call
> static inline int landlock_hook_##NAME
> which will call landlock_run_prog()
> which does:
> + for (landlock_ret = current->seccomp.landlock_ret;
> +  landlock_ret; landlock_ret = landlock_ret->prev) {
> +if (landlock_ret->triggered) {
> +   ctx.cookie = landlock_ret->cookie;
> +   for (prog = current->seccomp.landlock_prog;
> +prog; prog = prog->prev) {
> +   if (prog->filter == landlock_ret->filter) {
> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> +   break;
> +   }
> +   }
> 
> that is unacceptable overhead and not a scalable design.
> It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
> as soon as more lsm hooks are added.

Good catch! I forgot to check the program (sub)type in the loop to only
run the needed programs for the current hook. I will fix this.


> 
>> As said above, Landlock will not run an eBPF programs when not strictly
>> needed. Attaching to a cgroup will have the same performance impact as
>> attaching to a process hierarchy.
> 
> Having a prog per cgroup per lsm_hook is the only scalable way I
> could come up with. If you see another way, please propose.
> current->seccomp.landlock_prog is not the answer.

Hum, I don't see the difference from a performance point of view between
a cgroup-based or a process hierarchy-based system.

Maybe a better option should be to use an array of pointers with N
entries, one for each supported hook, instead of a unique pointer list?

Anyway, being able to attach an LSM hook program to a cgroup thanks to
the new BPF_PROG_ATTACH seems a good idea (while keeping the possibility
to use a process hierarchy). The downside will be to handle an LSM hook
program which is not triggered by a seccomp-filter, but this should be
needed anyway to handle interruptions.



signature.asc
Description: OpenPGP digital signature

constification and cocci / kernel build test robot ?

[Joe Perches]

On Sat, 2016-08-27 at 20:59 +0200, Julia Lawall wrote:
>  Make sure (of/i2c/platform)_device_id tables are NULL terminated
> Generated by: scripts/coccinelle/misc/of_table.cocci

Along the same lines, I submitted a manually generated
patch to add const to some of these structs.

https://lkml.org/lkml/2016/8/26/494

Could cocci and/or the kbuild test robot check for structs
that could or should be const?


> Please take the patch only if it's a positive warning. Thanks!
[]
> +++ b/drivers/thermal/intel_bxt_pmic_thermal.c
> @@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
> .name = "bxt_wcove_thermal",
> .driver_data = (kernel_ulong_t)_thermal_data,
> },
> +   {},
>  };
> 
>  static struct platform_driver pmic_thermal_driver = {

constification and cocci / kernel build test robot ?

[Joe Perches]

On Sat, 2016-08-27 at 20:59 +0200, Julia Lawall wrote:
>  Make sure (of/i2c/platform)_device_id tables are NULL terminated
> Generated by: scripts/coccinelle/misc/of_table.cocci

Along the same lines, I submitted a manually generated
patch to add const to some of these structs.

https://lkml.org/lkml/2016/8/26/494

Could cocci and/or the kbuild test robot check for structs
that could or should be const?


> Please take the patch only if it's a positive warning. Thanks!
[]
> +++ b/drivers/thermal/intel_bxt_pmic_thermal.c
> @@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
> .name = "bxt_wcove_thermal",
> .driver_data = (kernel_ulong_t)_thermal_data,
> },
> +   {},
>  };
> 
>  static struct platform_driver pmic_thermal_driver = {

Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process()

[Julia Lawall]

On Fri, 26 Aug 2016, SF Markus Elfring wrote:

> From: Markus Elfring 
> Date: Fri, 26 Aug 2016 13:38:30 +0200
>
> Move the assignment for the local variable "nooutpages" behind
> the source code for memory allocations by this function.
>
> Signed-off-by: Markus Elfring 
> ---
>  arch/cris/arch-v32/drivers/cryptocop.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
> b/arch/cris/arch-v32/drivers/cryptocop.c
> index 26347a2..cd34723 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>   struct page **inpages = NULL;
>   struct page **outpages = NULL;
>   int noinpages = 0;
> - int nooutpages = 0;
> + int nooutpages;
>
>   struct cryptocop_desc   descs[5]; /* Max 5 descriptors are 
> needed, there are three transforms that
>  * can get 
> connected/disconnected on different places in the indata. */
> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>   err = -ENOMEM;
>   goto free_inpages;
>   }
> + } else {
> + nooutpages = 0;

Why is it better?  4 characters have becomes 2 lines.

julia

>   }
>
>   /* Acquire the mm page semaphore. */
> --
> 2.9.3
>
>

Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process()

[Julia Lawall]

On Fri, 26 Aug 2016, SF Markus Elfring wrote:

> From: Markus Elfring 
> Date: Fri, 26 Aug 2016 13:38:30 +0200
>
> Move the assignment for the local variable "nooutpages" behind
> the source code for memory allocations by this function.
>
> Signed-off-by: Markus Elfring 
> ---
>  arch/cris/arch-v32/drivers/cryptocop.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
> b/arch/cris/arch-v32/drivers/cryptocop.c
> index 26347a2..cd34723 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>   struct page **inpages = NULL;
>   struct page **outpages = NULL;
>   int noinpages = 0;
> - int nooutpages = 0;
> + int nooutpages;
>
>   struct cryptocop_desc   descs[5]; /* Max 5 descriptors are 
> needed, there are three transforms that
>  * can get 
> connected/disconnected on different places in the indata. */
> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>   err = -ENOMEM;
>   goto free_inpages;
>   }
> + } else {
> + nooutpages = 0;

Why is it better?  4 characters have becomes 2 lines.

julia

>   }
>
>   /* Acquire the mm page semaphore. */
> --
> 2.9.3
>
>

Re: [PATCH 8/8] cris-cryptocop: Apply another recommendation from "checkpatch.pl"

[Julia Lawall]

On Fri, 26 Aug 2016, SF Markus Elfring wrote:

> From: Markus Elfring 
> Date: Fri, 26 Aug 2016 14:23:06 +0200
>
> The script "checkpatch.pl" can point out that assignments should usually
> not be performed within condition checks.
> Thus move the assignments for a local variable to separate statements
> in three functions.
>
> Signed-off-by: Markus Elfring 
> ---
>  arch/cris/arch-v32/drivers/cryptocop.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
> b/arch/cris/arch-v32/drivers/cryptocop.c
> index 1a966dc..00231a7 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -1510,7 +1510,8 @@ int cryptocop_new_session(cryptocop_session_id *sid, 
> struct cryptocop_transform_
>   while (tfrm_in){
>   int err;
>   ++no_tfrms;
> - if ((err = transform_ok(tfrm_in))) {
> + err = transform_ok(tfrm_in);
> + if (err) {
>   DEBUG_API(printk("cryptocop_new_session, bad 
> transform\n"));
>   return err;
>   }
> @@ -2276,7 +2277,10 @@ static int cryptocop_job_setup(struct 
> cryptocop_prio_job **pj, struct cryptocop_
>   (*pj)->iop->ctx_in.saved_data = operation->list_op.inlist;
>   (*pj)->iop->ctx_in.saved_data_buf = 
> operation->list_op.in_data_buf;
>   } else {
> - if ((err = cryptocop_setup_dma_list(operation, &(*pj)->iop, 
> alloc_flag))) {
> + err = cryptocop_setup_dma_list(operation,
> +&(*pj)->iop,
> +alloc_flag);

Checkpatch didn't say to put every argument on a different line, and that
wasn't done before, so why do it now?  There is plenty of room for at
least &(*pj)->iop on the line before.

julia

> + if (err) {
>   DEBUG_API(printk("cryptocop_job_setup: 
> cryptocop_setup_dma_list failed %d\n", err));
>   kfree(*pj);
>   return err;
> @@ -2867,7 +2871,8 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>
>   DEBUG(printk("cryptocop_ioctl_process: inserting job, cb_data=0x%p\n", 
> cop->cb_data));
>
> - if ((err = cryptocop_job_queue_insert_user_job(cop)) != 0) {
> + err = cryptocop_job_queue_insert_user_job(cop);
> + if (err) {
>   DEBUG_API(printk("cryptocop_ioctl_process: insert job %d\n", 
> err));
>   err = -EINVAL;
>   goto mark_outpages_dirty;
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 8/8] cris-cryptocop: Apply another recommendation from "checkpatch.pl"

[Julia Lawall]

On Fri, 26 Aug 2016, SF Markus Elfring wrote:

> From: Markus Elfring 
> Date: Fri, 26 Aug 2016 14:23:06 +0200
>
> The script "checkpatch.pl" can point out that assignments should usually
> not be performed within condition checks.
> Thus move the assignments for a local variable to separate statements
> in three functions.
>
> Signed-off-by: Markus Elfring 
> ---
>  arch/cris/arch-v32/drivers/cryptocop.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
> b/arch/cris/arch-v32/drivers/cryptocop.c
> index 1a966dc..00231a7 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -1510,7 +1510,8 @@ int cryptocop_new_session(cryptocop_session_id *sid, 
> struct cryptocop_transform_
>   while (tfrm_in){
>   int err;
>   ++no_tfrms;
> - if ((err = transform_ok(tfrm_in))) {
> + err = transform_ok(tfrm_in);
> + if (err) {
>   DEBUG_API(printk("cryptocop_new_session, bad 
> transform\n"));
>   return err;
>   }
> @@ -2276,7 +2277,10 @@ static int cryptocop_job_setup(struct 
> cryptocop_prio_job **pj, struct cryptocop_
>   (*pj)->iop->ctx_in.saved_data = operation->list_op.inlist;
>   (*pj)->iop->ctx_in.saved_data_buf = 
> operation->list_op.in_data_buf;
>   } else {
> - if ((err = cryptocop_setup_dma_list(operation, &(*pj)->iop, 
> alloc_flag))) {
> + err = cryptocop_setup_dma_list(operation,
> +&(*pj)->iop,
> +alloc_flag);

Checkpatch didn't say to put every argument on a different line, and that
wasn't done before, so why do it now?  There is plenty of room for at
least &(*pj)->iop on the line before.

julia

> + if (err) {
>   DEBUG_API(printk("cryptocop_job_setup: 
> cryptocop_setup_dma_list failed %d\n", err));
>   kfree(*pj);
>   return err;
> @@ -2867,7 +2871,8 @@ static int cryptocop_ioctl_process(struct inode *inode, 
> struct file *filp, unsig
>
>   DEBUG(printk("cryptocop_ioctl_process: inserting job, cb_data=0x%p\n", 
> cop->cb_data));
>
> - if ((err = cryptocop_job_queue_insert_user_job(cop)) != 0) {
> + err = cryptocop_job_queue_insert_user_job(cop);
> + if (err) {
>   DEBUG_API(printk("cryptocop_ioctl_process: insert job %d\n", 
> err));
>   err = -EINVAL;
>   goto mark_outpages_dirty;
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH] clk: sunxi-ng: Fix wrong reset register offsets

[jorik]

From: Jorik Jonker 

The reset register offsets for UART*, I2C* and SCR were off by a few bytes.

Signed-off-by: Jorik Jonker 

---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index feabc0f..280ba74 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -783,14 +783,14 @@ static struct ccu_reset_map sun8i_h3_ccu_resets[] = {
[RST_BUS_I2S1]  =  { 0x2d0, BIT(13) },
[RST_BUS_I2S2]  =  { 0x2d0, BIT(14) },
 
-   [RST_BUS_I2C0]  =  { 0x2d4, BIT(0) },
-   [RST_BUS_I2C1]  =  { 0x2d4, BIT(1) },
-   [RST_BUS_I2C2]  =  { 0x2d4, BIT(2) },
-   [RST_BUS_UART0] =  { 0x2d4, BIT(16) },
-   [RST_BUS_UART1] =  { 0x2d4, BIT(17) },
-   [RST_BUS_UART2] =  { 0x2d4, BIT(18) },
-   [RST_BUS_UART3] =  { 0x2d4, BIT(19) },
-   [RST_BUS_SCR]   =  { 0x2d4, BIT(20) },
+   [RST_BUS_I2C0]  =  { 0x2d8, BIT(0) },
+   [RST_BUS_I2C1]  =  { 0x2d8, BIT(1) },
+   [RST_BUS_I2C2]  =  { 0x2d8, BIT(2) },
+   [RST_BUS_UART0] =  { 0x2d8, BIT(16) },
+   [RST_BUS_UART1] =  { 0x2d8, BIT(17) },
+   [RST_BUS_UART2] =  { 0x2d8, BIT(18) },
+   [RST_BUS_UART3] =  { 0x2d8, BIT(19) },
+   [RST_BUS_SCR]   =  { 0x2d8, BIT(20) },
 };
 
 static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
-- 
2.7.4

[PATCH] clk: sunxi-ng: Fix wrong reset register offsets

[jorik]

From: Jorik Jonker 

The reset register offsets for UART*, I2C* and SCR were off by a few bytes.

Signed-off-by: Jorik Jonker 

---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index feabc0f..280ba74 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -783,14 +783,14 @@ static struct ccu_reset_map sun8i_h3_ccu_resets[] = {
[RST_BUS_I2S1]  =  { 0x2d0, BIT(13) },
[RST_BUS_I2S2]  =  { 0x2d0, BIT(14) },
 
-   [RST_BUS_I2C0]  =  { 0x2d4, BIT(0) },
-   [RST_BUS_I2C1]  =  { 0x2d4, BIT(1) },
-   [RST_BUS_I2C2]  =  { 0x2d4, BIT(2) },
-   [RST_BUS_UART0] =  { 0x2d4, BIT(16) },
-   [RST_BUS_UART1] =  { 0x2d4, BIT(17) },
-   [RST_BUS_UART2] =  { 0x2d4, BIT(18) },
-   [RST_BUS_UART3] =  { 0x2d4, BIT(19) },
-   [RST_BUS_SCR]   =  { 0x2d4, BIT(20) },
+   [RST_BUS_I2C0]  =  { 0x2d8, BIT(0) },
+   [RST_BUS_I2C1]  =  { 0x2d8, BIT(1) },
+   [RST_BUS_I2C2]  =  { 0x2d8, BIT(2) },
+   [RST_BUS_UART0] =  { 0x2d8, BIT(16) },
+   [RST_BUS_UART1] =  { 0x2d8, BIT(17) },
+   [RST_BUS_UART2] =  { 0x2d8, BIT(18) },
+   [RST_BUS_UART3] =  { 0x2d8, BIT(19) },
+   [RST_BUS_SCR]   =  { 0x2d8, BIT(20) },
 };
 
 static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
-- 
2.7.4

[PATCH] thermal: fix of_table.cocci warnings

[Julia Lawall]

 Make sure (of/i2c/platform)_device_id tables are NULL terminated
Generated by: scripts/coccinelle/misc/of_table.cocci

CC: Yegnesh S Iyer 
Signed-off-by: Julia Lawall 
Signed-off-by: Fengguang Wu 
---

Please take the patch only if it's a positive warning. Thanks!

 intel_bxt_pmic_thermal.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/thermal/intel_bxt_pmic_thermal.c
+++ b/drivers/thermal/intel_bxt_pmic_thermal.c
@@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
.name = "bxt_wcove_thermal",
.driver_data = (kernel_ulong_t)_thermal_data,
},
+   {},
 };

 static struct platform_driver pmic_thermal_driver = {

[PATCH] thermal: fix of_table.cocci warnings

[Julia Lawall]

 Make sure (of/i2c/platform)_device_id tables are NULL terminated
Generated by: scripts/coccinelle/misc/of_table.cocci

CC: Yegnesh S Iyer 
Signed-off-by: Julia Lawall 
Signed-off-by: Fengguang Wu 
---

Please take the patch only if it's a positive warning. Thanks!

 intel_bxt_pmic_thermal.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/thermal/intel_bxt_pmic_thermal.c
+++ b/drivers/thermal/intel_bxt_pmic_thermal.c
@@ -281,6 +281,7 @@ static struct platform_device_id pmic_th
.name = "bxt_wcove_thermal",
.driver_data = (kernel_ulong_t)_thermal_data,
},
+   {},
 };

 static struct platform_driver pmic_thermal_driver = {

Re: [RFC v2 09/10] landlock: Handle cgroups (netfilter match)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:19:05PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >> To sum up, there is four related patchsets:
> >> * "Landlock LSM: Unprivileged sandboxing" (this series)
> >> * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
> >> * "Networking cgroup controller" (Anoop Naravaram)
> >> * "Add eBPF hooks for cgroups" (Daniel Mack)
> 
> >>> Anoop Naravaram's use case is to control the ports the applications
> >>> under cgroup can bind and listen on.
> >>> Such use case can be solved by such 'lsm cgroup controller' by
> >>> attaching bpf program to security_socket_bind lsm hook and
> >>> filtering sockaddr.
> >>> Furthermore Sargun's use case is to allow further sockaddr rewrites
> >>> from the bpf program which can be done as natural extension
> >>> of such mechanism.
> >>>
> >>> If I understood Daniel's Anoop's Sargun's and yours use cases
> >>> correctly the common piece of kernel infrastructure that can solve
> >>> them all can start from Daniel's current set of patches that
> >>> establish a mechanism of attaching bpf program to a cgroup.
> >>> Then adding lsm hooks to it and later allowing argument rewrite
> >>> (since they're already in the kernel and no ToCToU problems exist)
> 
> >> For the network-related series, I think it make more sense to simply
> >> create a netfilter rule matching a cgroup and then add more features to
> >> netfilter (restrict port ranges and so on) thanks to eBPF programs.
> >> Containers are (usually) in a dedicated network namespace, which open
> >> the possibility to not only rely on cgroups (e.g. match UID,
> >> netmask...). It would also be more flexible to be able to load a BPF
> >> program in netfilter and update its maps on the fly to make dynamic
> >> rules, like ipset does, but in a more generic way.
> 
> What do the netdev folks think about this design?

such design doesn't scale when used for container management and
that's what we need to solve.
netns has its overhead and management issues. There are proposals to
solve that but that is orthogonal to containers in general.
A lot of them don't use netns.

Re: [RFC v2 09/10] landlock: Handle cgroups (netfilter match)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:19:05PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >> To sum up, there is four related patchsets:
> >> * "Landlock LSM: Unprivileged sandboxing" (this series)
> >> * "Add Checmate, BPF-driven minor LSM" (Sargun Dhillon)
> >> * "Networking cgroup controller" (Anoop Naravaram)
> >> * "Add eBPF hooks for cgroups" (Daniel Mack)
> 
> >>> Anoop Naravaram's use case is to control the ports the applications
> >>> under cgroup can bind and listen on.
> >>> Such use case can be solved by such 'lsm cgroup controller' by
> >>> attaching bpf program to security_socket_bind lsm hook and
> >>> filtering sockaddr.
> >>> Furthermore Sargun's use case is to allow further sockaddr rewrites
> >>> from the bpf program which can be done as natural extension
> >>> of such mechanism.
> >>>
> >>> If I understood Daniel's Anoop's Sargun's and yours use cases
> >>> correctly the common piece of kernel infrastructure that can solve
> >>> them all can start from Daniel's current set of patches that
> >>> establish a mechanism of attaching bpf program to a cgroup.
> >>> Then adding lsm hooks to it and later allowing argument rewrite
> >>> (since they're already in the kernel and no ToCToU problems exist)
> 
> >> For the network-related series, I think it make more sense to simply
> >> create a netfilter rule matching a cgroup and then add more features to
> >> netfilter (restrict port ranges and so on) thanks to eBPF programs.
> >> Containers are (usually) in a dedicated network namespace, which open
> >> the possibility to not only rely on cgroups (e.g. match UID,
> >> netmask...). It would also be more flexible to be able to load a BPF
> >> program in netfilter and update its maps on the fly to make dynamic
> >> rules, like ipset does, but in a more generic way.
> 
> What do the netdev folks think about this design?

such design doesn't scale when used for container management and
that's what we need to solve.
netns has its overhead and management issues. There are proposals to
solve that but that is orthogonal to containers in general.
A lot of them don't use netns.

Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex

[Ingo Molnar]

* Peter Zijlstra  wrote:

> Its because the mutex wasn't quite exclusive enough :-) If you let in 
> multiple 
> owner, like with that race you found, you get big gains in throughput ...

Btw., do we know which mutex that was?

That it didn't crash with a full AIM run suggests that whatever it is 
protecting 
it could probably be parallelized some more while still having a mostly working 
kernel! ;-)

Thanks,

Ingo

Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex

[Ingo Molnar]

* Peter Zijlstra  wrote:

> Its because the mutex wasn't quite exclusive enough :-) If you let in 
> multiple 
> owner, like with that race you found, you get big gains in throughput ...

Btw., do we know which mutex that was?

That it didn't crash with a full AIM run suggests that whatever it is 
protecting 
it could probably be parallelized some more while still having a mostly working 
kernel! ;-)

Thanks,

Ingo

Re: [PATCH] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Yuval Shaia]

One minor comment inline.
Besides that:
Reviewed-by: Yuval Shaia 

On Sun, Aug 28, 2016 at 12:00:18AM +0800, Baoyou Xie wrote:
> We get 1 warning when biuld kernel with W=1:

s/biuld/build

> drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for 
> '_free_qp' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared
> and don't need a declaration, but can be made static.
> so this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie 
> ---
>  drivers/infiniband/hw/cxgb4/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
> b/drivers/infiniband/hw/cxgb4/qp.c
> index edb1172..6904352 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -683,7 +683,7 @@ static int build_inv_stag(union t4_wr *wqe, struct 
> ib_send_wr *wr,
>   return 0;
>  }
>  
> -void _free_qp(struct kref *kref)
> +static void _free_qp(struct kref *kref)
>  {
>   struct c4iw_qp *qhp;
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Yuval Shaia]

One minor comment inline.
Besides that:
Reviewed-by: Yuval Shaia 

On Sun, Aug 28, 2016 at 12:00:18AM +0800, Baoyou Xie wrote:
> We get 1 warning when biuld kernel with W=1:

s/biuld/build

> drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for 
> '_free_qp' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared
> and don't need a declaration, but can be made static.
> so this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie 
> ---
>  drivers/infiniband/hw/cxgb4/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
> b/drivers/infiniband/hw/cxgb4/qp.c
> index edb1172..6904352 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -683,7 +683,7 @@ static int build_inv_stag(union t4_wr *wqe, struct 
> ib_send_wr *wr,
>   return 0;
>  }
>  
> -void _free_qp(struct kref *kref)
> +static void _free_qp(struct kref *kref)
>  {
>   struct c4iw_qp *qhp;
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >
> >>> As far as safety and type checking that bpf programs has to do,
> >>> I like the approach of patch 06/10:
> >>> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> >>> +   PTR_TO_STRUCT_FILE, struct file *, file,
> >>> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> >>> +)
> >>> teaching verifier to recognize struct file, cred, sockaddr
> >>> will let bpf program access them naturally without any overhead.
> >>> Though:
> >>> @@ -102,6 +102,9 @@ enum bpf_prog_type {
> >>> BPF_PROG_TYPE_SCHED_CLS,
> >>> BPF_PROG_TYPE_SCHED_ACT,
> >>> BPF_PROG_TYPE_TRACEPOINT,
> >>> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> >>> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> >>> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >>>  };
> >>> is a bit of overkill.
> >>> I think it would be cleaner to have single
> >>> BPF_PROG_TYPE_LSM and at program load time pass
> >>> lsm_hook_id as well, so that verifier can do safety checks
> >>> based on type info provided in LANDLOCK_HOOKs
> >>
> >> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
> >> verifier check programs according to their types. If we need to check
> >> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
> >> dedicated program types. I don't see any other way to do it with the
> >> current verifier code. Moreover it's the purpose of program types, right?
> > 
> > Adding new bpf program type for every lsm hook is not acceptable.
> > Either do one new program type + pass lsm_hook_id as suggested
> > or please come up with an alternative approach.
> 
> OK, so we have to modify the verifier to not only rely on the program
> type but on another value to check the context accesses. Do you have a
> hint from where this value could come from? Do we need to add a new bpf
> command to associate a program to a subtype?

It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
Both prog_type and prog_hook_id are used during verification.
prog_type distinguishes the main aspects whereas prog_hook_id selects
which lsm_hook's argument definition to apply.
At the time of attaching to a hook, the prog_hook_id passed at the
load time should match lsm's hook_id.

Re: [RFC v2 09/10] landlock: Handle cgroups (program types)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >
> >>> As far as safety and type checking that bpf programs has to do,
> >>> I like the approach of patch 06/10:
> >>> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> >>> +   PTR_TO_STRUCT_FILE, struct file *, file,
> >>> +   PTR_TO_STRUCT_CRED, const struct cred *, cred
> >>> +)
> >>> teaching verifier to recognize struct file, cred, sockaddr
> >>> will let bpf program access them naturally without any overhead.
> >>> Though:
> >>> @@ -102,6 +102,9 @@ enum bpf_prog_type {
> >>> BPF_PROG_TYPE_SCHED_CLS,
> >>> BPF_PROG_TYPE_SCHED_ACT,
> >>> BPF_PROG_TYPE_TRACEPOINT,
> >>> +   BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> >>> +   BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> >>> +   BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >>>  };
> >>> is a bit of overkill.
> >>> I think it would be cleaner to have single
> >>> BPF_PROG_TYPE_LSM and at program load time pass
> >>> lsm_hook_id as well, so that verifier can do safety checks
> >>> based on type info provided in LANDLOCK_HOOKs
> >>
> >> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
> >> verifier check programs according to their types. If we need to check
> >> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
> >> dedicated program types. I don't see any other way to do it with the
> >> current verifier code. Moreover it's the purpose of program types, right?
> > 
> > Adding new bpf program type for every lsm hook is not acceptable.
> > Either do one new program type + pass lsm_hook_id as suggested
> > or please come up with an alternative approach.
> 
> OK, so we have to modify the verifier to not only rely on the program
> type but on another value to check the context accesses. Do you have a
> hint from where this value could come from? Do we need to add a new bpf
> command to associate a program to a subtype?

It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
Both prog_type and prog_hook_id are used during verification.
prog_type distinguishes the main aspects whereas prog_hook_id selects
which lsm_hook's argument definition to apply.
At the time of attaching to a hook, the prog_hook_id passed at the
load time should match lsm's hook_id.

Re: [RFC v2 09/10] landlock: Handle cgroups

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 12:30:36AM -0700, Andy Lutomirski wrote:
> > cgroup is the common way to group multiple tasks.
> > Without cgroup only parent<->child relationship will be possible,
> > which will limit usability of such lsm to a master task that controls
> > its children. Such api restriction would have been ok, if we could
> > extend it in the future, but unfortunately task-centric won't allow it
> > without creating a parallel lsm that is cgroup based.
> > Therefore I think we have to go with cgroup-centric api and your
> > application has to use cgroups from the start though only parent-child
> > would have been enough.
> > Also I don't think the kernel can afford two bpf based lsm. One task
> > based and another cgroup based, so we have to find common ground
> > that suits both use cases.
> > Having unprivliged access is a subset. There is no strong reason why
> > cgroup+lsm+bpf should be limited to root only always.
> > When we can guarantee no pointer leaks, we can allow unpriv.
> 
> I don't really understand what you mean.  In the context of landlock,
> which is a *sandbox*, can one of you explain a use case that
> materially benefits from this type of cgroup usage?  I haven't thought
> of one.

In case of seccomp-like sandbox where parent controls child processes
cgroup is not needed. It's needed when container management software
needs to control a set of applications. If we can have one bpf-based lsm
that works via cgroup and without, I'd be fine with it. Right now
I haven't seen a plausible proposal to do that. Therefore cgroup based
api is a common api that works for sandbox as well, though requiring
parent to create a cgroup just to control a single child is cumbersome.

Re: [RFC v2 09/10] landlock: Handle cgroups

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 12:30:36AM -0700, Andy Lutomirski wrote:
> > cgroup is the common way to group multiple tasks.
> > Without cgroup only parent<->child relationship will be possible,
> > which will limit usability of such lsm to a master task that controls
> > its children. Such api restriction would have been ok, if we could
> > extend it in the future, but unfortunately task-centric won't allow it
> > without creating a parallel lsm that is cgroup based.
> > Therefore I think we have to go with cgroup-centric api and your
> > application has to use cgroups from the start though only parent-child
> > would have been enough.
> > Also I don't think the kernel can afford two bpf based lsm. One task
> > based and another cgroup based, so we have to find common ground
> > that suits both use cases.
> > Having unprivliged access is a subset. There is no strong reason why
> > cgroup+lsm+bpf should be limited to root only always.
> > When we can guarantee no pointer leaks, we can allow unpriv.
> 
> I don't really understand what you mean.  In the context of landlock,
> which is a *sandbox*, can one of you explain a use case that
> materially benefits from this type of cgroup usage?  I haven't thought
> of one.

In case of seccomp-like sandbox where parent controls child processes
cgroup is not needed. It's needed when container management software
needs to control a set of applications. If we can have one bpf-based lsm
that works via cgroup and without, I'd be fine with it. Right now
I haven't seen a plausible proposal to do that. Therefore cgroup based
api is a common api that works for sandbox as well, though requiring
parent to create a cgroup just to control a single child is cumbersome.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>
> >>>
> >>> - I don't think such 'for' loop can scale. The solution needs to work
> >>> with thousands of containers and thousands of cgroups.
> >>> In the patch 06/10 the proposal is to use 'current' as holder of
> >>> the programs:
> >>> +   for (prog = current->seccomp.landlock_prog;
> >>> +   prog; prog = prog->prev) {
> >>> +   if (prog->filter == landlock_ret->filter) {
> >>> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> >>> +   break;
> >>> +   }
> >>> +   }
> >>> imo that's the root of scalability issue.
> >>> I think to be able to scale the bpf programs have to be attached to
> >>> cgroups instead of tasks.
> >>> That would be very different api. seccomp doesn't need to be touched.
> >>> But that is the only way I see to be able to scale.
> >>
> >> Landlock is inspired from seccomp which also use a BPF program per
> >> thread. For seccomp, each BPF programs are executed for each syscall.
> >> For Landlock, some BPF programs are executed for some LSM hooks. I don't
> >> see why it is a scale issue for Landlock comparing to seccomp. I also
> >> don't see why storing the BPF program list pointer in the cgroup struct
> >> instead of the task struct change a lot here. The BPF programs execution
> >> will be the same anyway (for each LSM hook). Kees should probably have a
> >> better opinion on this.
> > 
> > seccomp has its own issues and copying them doesn't make this lsm any 
> > better.
> > Like seccomp bpf programs are all gigantic switch statement that looks
> > for interesting syscall numbers. All syscalls of a task are paying
> > non-trivial seccomp penalty due to such design. If bpf was attached per
> > syscall it would have been much cheaper. Of course doing it this way
> > for seccomp is not easy, but for lsm such facility is already there.
> > Blank call of a single bpf prog for all lsm hooks is unnecessary
> > overhead that can and should be avoided.
> 
> It's probably a misunderstanding. Contrary to seccomp which run all the
> thread's BPF programs for any syscall, Landlock only run eBPF programs
> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> multiple eBPF program types and contrary to seccomp, Landlock only run
> an eBPF program when needed. Landlock will have almost no performance
> overhead if the syscalls do not trigger the watched LSM hooks for the
> current process.

that's not what I see in the patch 06/10:
all lsm_hooks in 'static struct security_hook_list landlock_hooks'
(which eventually means all lsm hooks) will call
static inline int landlock_hook_##NAME
which will call landlock_run_prog()
which does:
+ for (landlock_ret = current->seccomp.landlock_ret;
+  landlock_ret; landlock_ret = landlock_ret->prev) {
+if (landlock_ret->triggered) {
+   ctx.cookie = landlock_ret->cookie;
+   for (prog = current->seccomp.landlock_prog;
+prog; prog = prog->prev) {
+   if (prog->filter == landlock_ret->filter) {
+   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
+   break;
+   }
+   }

that is unacceptable overhead and not a scalable design.
It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
as soon as more lsm hooks are added.

> As said above, Landlock will not run an eBPF programs when not strictly
> needed. Attaching to a cgroup will have the same performance impact as
> attaching to a process hierarchy.

Having a prog per cgroup per lsm_hook is the only scalable way I
could come up with. If you see another way, please propose.
current->seccomp.landlock_prog is not the answer.

Re: [RFC v2 09/10] landlock: Handle cgroups (performance)

[Alexei Starovoitov]

On Sat, Aug 27, 2016 at 04:06:38PM +0200, Mickaël Salaün wrote:
> 
> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> > On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>
> >>>
> >>> - I don't think such 'for' loop can scale. The solution needs to work
> >>> with thousands of containers and thousands of cgroups.
> >>> In the patch 06/10 the proposal is to use 'current' as holder of
> >>> the programs:
> >>> +   for (prog = current->seccomp.landlock_prog;
> >>> +   prog; prog = prog->prev) {
> >>> +   if (prog->filter == landlock_ret->filter) {
> >>> +   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
> >>> +   break;
> >>> +   }
> >>> +   }
> >>> imo that's the root of scalability issue.
> >>> I think to be able to scale the bpf programs have to be attached to
> >>> cgroups instead of tasks.
> >>> That would be very different api. seccomp doesn't need to be touched.
> >>> But that is the only way I see to be able to scale.
> >>
> >> Landlock is inspired from seccomp which also use a BPF program per
> >> thread. For seccomp, each BPF programs are executed for each syscall.
> >> For Landlock, some BPF programs are executed for some LSM hooks. I don't
> >> see why it is a scale issue for Landlock comparing to seccomp. I also
> >> don't see why storing the BPF program list pointer in the cgroup struct
> >> instead of the task struct change a lot here. The BPF programs execution
> >> will be the same anyway (for each LSM hook). Kees should probably have a
> >> better opinion on this.
> > 
> > seccomp has its own issues and copying them doesn't make this lsm any 
> > better.
> > Like seccomp bpf programs are all gigantic switch statement that looks
> > for interesting syscall numbers. All syscalls of a task are paying
> > non-trivial seccomp penalty due to such design. If bpf was attached per
> > syscall it would have been much cheaper. Of course doing it this way
> > for seccomp is not easy, but for lsm such facility is already there.
> > Blank call of a single bpf prog for all lsm hooks is unnecessary
> > overhead that can and should be avoided.
> 
> It's probably a misunderstanding. Contrary to seccomp which run all the
> thread's BPF programs for any syscall, Landlock only run eBPF programs
> for the triggered LSM hooks, if their type match. Indeed, thanks to the
> multiple eBPF program types and contrary to seccomp, Landlock only run
> an eBPF program when needed. Landlock will have almost no performance
> overhead if the syscalls do not trigger the watched LSM hooks for the
> current process.

that's not what I see in the patch 06/10:
all lsm_hooks in 'static struct security_hook_list landlock_hooks'
(which eventually means all lsm hooks) will call
static inline int landlock_hook_##NAME
which will call landlock_run_prog()
which does:
+ for (landlock_ret = current->seccomp.landlock_ret;
+  landlock_ret; landlock_ret = landlock_ret->prev) {
+if (landlock_ret->triggered) {
+   ctx.cookie = landlock_ret->cookie;
+   for (prog = current->seccomp.landlock_prog;
+prog; prog = prog->prev) {
+   if (prog->filter == landlock_ret->filter) {
+   cur_ret = BPF_PROG_RUN(prog->prog, (void *));
+   break;
+   }
+   }

that is unacceptable overhead and not a scalable design.
It kinda works for 3 lsm_hooks as in patch 6, but doesn't scale
as soon as more lsm hooks are added.

> As said above, Landlock will not run an eBPF programs when not strictly
> needed. Attaching to a cgroup will have the same performance impact as
> attaching to a process hierarchy.

Having a prog per cgroup per lsm_hook is the only scalable way I
could come up with. If you see another way, please propose.
current->seccomp.landlock_prog is not the answer.

Re: [PATCHv3 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_*

[kbuild test robot]

Hi Dmitry,

[auto build test ERROR on v4.8-rc3]
[also build test ERROR on next-20160825]
[cannot apply to tip/x86/core tip/x86/vdso linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Dmitry-Safonov/x86-32-bit-compatible-C-R-on-x86_64/20160827-011727
config: x86_64-randconfig-v0-08280034 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `do_arch_prctl':
>> (.text+0x29865): undefined reference to `vdso_image_32'
   arch/x86/built-in.o: In function `do_arch_prctl':
   (.text+0x29877): undefined reference to `vdso_image_32'

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


.config.gz
Description: Binary data

Re: [PATCHv3 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_*

[kbuild test robot]

Hi Dmitry,

[auto build test ERROR on v4.8-rc3]
[also build test ERROR on next-20160825]
[cannot apply to tip/x86/core tip/x86/vdso linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Dmitry-Safonov/x86-32-bit-compatible-C-R-on-x86_64/20160827-011727
config: x86_64-randconfig-v0-08280034 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `do_arch_prctl':
>> (.text+0x29865): undefined reference to `vdso_image_32'
   arch/x86/built-in.o: In function `do_arch_prctl':
   (.text+0x29877): undefined reference to `vdso_image_32'

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


.config.gz
Description: Binary data

Re: [PATCH] fix:memory:of_memory:add missing header dependencies

[Greg KH]

On Sun, Aug 28, 2016 at 01:31:28AM +0800, Baoyou Xie wrote:
> We get 2 warnings when biuld kernel with W=1:

Why would you do that?

> drivers/memory/of_memory.c:30:30: warning: no previous prototype for 
> 'of_get_min_tck' [-Wmissing-prototypes]
> drivers/memory/of_memory.c:106:30: warning: no previous prototype for 
> 'of_get_ddr_timings' [-Wmissing-prototypes]
> 
> In fact, these functions are declared in drivers/memory/of_memory.h
> so this patch add missing header dependencies.

Why am I not seeing this warning on a "normal" build?  If there's no
prototype of a function, that seems odd...  What about sparse, does that
give you the same error here?

thanks,

greg k-h

Re: [PATCH] fix:memory:of_memory:add missing header dependencies

[Greg KH]

On Sun, Aug 28, 2016 at 01:31:28AM +0800, Baoyou Xie wrote:
> We get 2 warnings when biuld kernel with W=1:

Why would you do that?

> drivers/memory/of_memory.c:30:30: warning: no previous prototype for 
> 'of_get_min_tck' [-Wmissing-prototypes]
> drivers/memory/of_memory.c:106:30: warning: no previous prototype for 
> 'of_get_ddr_timings' [-Wmissing-prototypes]
> 
> In fact, these functions are declared in drivers/memory/of_memory.h
> so this patch add missing header dependencies.

Why am I not seeing this warning on a "normal" build?  If there's no
prototype of a function, that seems odd...  What about sparse, does that
give you the same error here?

thanks,

greg k-h

Re: [PATCH 1/3] checkkconfigsymbols: port to python3

[Greg KH]

On Sat, Aug 27, 2016 at 05:25:29PM +0200, Valentin Rothberg wrote:
> Port the script to python3 and fix some pylint warnings.

"and" in a changelog description usually means this should be 2
different patches.

Can't you fix up the pylint warnings in a separate patch?

thanks,

greg k-h

Re: [PATCH 1/3] checkkconfigsymbols: port to python3

[Greg KH]

On Sat, Aug 27, 2016 at 05:25:29PM +0200, Valentin Rothberg wrote:
> Port the script to python3 and fix some pylint warnings.

"and" in a changelog description usually means this should be 2
different patches.

Can't you fix up the pylint warnings in a separate patch?

thanks,

greg k-h

[GIT PULL] KVM fixes for Linux 4.8-rc4

[Paolo Bonzini]

Linus,

The following changes since commit 184ca823481c99dadd7d946e5afd4bb921eab30d:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2016-08-17 
17:26:58 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/kvm.git tags/for-linus

for you to fetch changes up to ba913e4f72fc9cfd03dad968dfb110eb49211d80:

  MIPS: KVM: Check for pfn noslot case (2016-08-19 17:22:26 +0200)


* ARM fixes:
** fixes for ITS init issues, error handling, IRQ leakage, race conditions
** An erratum workaround for timers
** Some removal of misleading use of errors and comments
** A fix for GICv3 on 32-bit guests
* MIPS fix where the guest could wrongly map the first page of physical memory
* x86 nested virtualization fixes



Andre Przywara (4):
  KVM: arm64: ITS: return 1 on successful MSI injection
  KVM: arm64: ITS: move ITS registration into first VCPU run
  KVM: arm64: check for ITS device on MSI injection
  KVM: arm64: ITS: avoid re-mapping LPIs

Christoffer Dall (4):
  KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
  KVM: arm64: vgic-its: Plug race in vgic_put_irq
  KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic
  KVM: arm/arm64: Change misleading use of is_error_pfn

James Hogan (1):
  MIPS: KVM: Check for pfn noslot case

Marc Zyngier (2):
  arm64: Document workaround for Cortex-A72 erratum #853709
  KVM: arm/arm64: timer: Workaround misconfigured timer interrupt

Paolo Bonzini (1):
  Merge tag 'kvm-arm-for-v4.8-rc3' of 
git://git.kernel.org/.../kvmarm/kvmarm into HEAD

Peter Feiner (1):
  kvm: nVMX: fix nested tsc scaling

Radim Krčmář (2):
  KVM: nVMX: fix msr bitmaps to prevent L2 from accessing L0 x2APIC
  KVM: nVMX: postpone VMCS changes on MSR_IA32_APICBASE write

Vladimir Murzin (2):
  arm64: KVM: remove misleading comment on pmu status
  arm64: KVM: report configured SRE value to 32-bit world

 Documentation/arm64/silicon-errata.txt |   1 +
 arch/arm/kvm/mmu.c |   2 +-
 arch/arm64/kvm/hyp/switch.c|   2 +-
 arch/arm64/kvm/sys_regs.c  |  10 +--
 arch/mips/kvm/mmu.c|   2 +-
 arch/x86/kvm/vmx.c | 136 ++--
 include/linux/irqchip/arm-gic-v3.h |   1 +
 virt/kvm/arm/arch_timer.c  |  11 ++-
 virt/kvm/arm/vgic/vgic-its.c   | 158 -
 virt/kvm/arm/vgic/vgic-mmio-v3.c   |  26 +++---
 virt/kvm/arm/vgic/vgic-v3.c|   8 ++
 virt/kvm/arm/vgic/vgic.c   |  10 +--
 virt/kvm/arm/vgic/vgic.h   |   6 ++
 13 files changed, 234 insertions(+), 139 deletions(-)

[GIT PULL] KVM fixes for Linux 4.8-rc4

[Paolo Bonzini]

Linus,

The following changes since commit 184ca823481c99dadd7d946e5afd4bb921eab30d:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2016-08-17 
17:26:58 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/kvm.git tags/for-linus

for you to fetch changes up to ba913e4f72fc9cfd03dad968dfb110eb49211d80:

  MIPS: KVM: Check for pfn noslot case (2016-08-19 17:22:26 +0200)


* ARM fixes:
** fixes for ITS init issues, error handling, IRQ leakage, race conditions
** An erratum workaround for timers
** Some removal of misleading use of errors and comments
** A fix for GICv3 on 32-bit guests
* MIPS fix where the guest could wrongly map the first page of physical memory
* x86 nested virtualization fixes



Andre Przywara (4):
  KVM: arm64: ITS: return 1 on successful MSI injection
  KVM: arm64: ITS: move ITS registration into first VCPU run
  KVM: arm64: check for ITS device on MSI injection
  KVM: arm64: ITS: avoid re-mapping LPIs

Christoffer Dall (4):
  KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
  KVM: arm64: vgic-its: Plug race in vgic_put_irq
  KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic
  KVM: arm/arm64: Change misleading use of is_error_pfn

James Hogan (1):
  MIPS: KVM: Check for pfn noslot case

Marc Zyngier (2):
  arm64: Document workaround for Cortex-A72 erratum #853709
  KVM: arm/arm64: timer: Workaround misconfigured timer interrupt

Paolo Bonzini (1):
  Merge tag 'kvm-arm-for-v4.8-rc3' of 
git://git.kernel.org/.../kvmarm/kvmarm into HEAD

Peter Feiner (1):
  kvm: nVMX: fix nested tsc scaling

Radim Krčmář (2):
  KVM: nVMX: fix msr bitmaps to prevent L2 from accessing L0 x2APIC
  KVM: nVMX: postpone VMCS changes on MSR_IA32_APICBASE write

Vladimir Murzin (2):
  arm64: KVM: remove misleading comment on pmu status
  arm64: KVM: report configured SRE value to 32-bit world

 Documentation/arm64/silicon-errata.txt |   1 +
 arch/arm/kvm/mmu.c |   2 +-
 arch/arm64/kvm/hyp/switch.c|   2 +-
 arch/arm64/kvm/sys_regs.c  |  10 +--
 arch/mips/kvm/mmu.c|   2 +-
 arch/x86/kvm/vmx.c | 136 ++--
 include/linux/irqchip/arm-gic-v3.h |   1 +
 virt/kvm/arm/arch_timer.c  |  11 ++-
 virt/kvm/arm/vgic/vgic-its.c   | 158 -
 virt/kvm/arm/vgic/vgic-mmio-v3.c   |  26 +++---
 virt/kvm/arm/vgic/vgic-v3.c|   8 ++
 virt/kvm/arm/vgic/vgic.c   |  10 +--
 virt/kvm/arm/vgic/vgic.h   |   6 ++
 13 files changed, 234 insertions(+), 139 deletions(-)