smp_mb__after_spinlock requirement too strong?

[焦晓冬]

Peter pointed out in this patch https://patchwork.kernel.org/patch/9771921/
that the spinning-lock used at __schedule() should be RCsc to ensure
visibility of writes prior to __schedule when the task is to be migrated to
another CPU.

And this is emphasized at the comment of the newly introduced
smp_mb__after_spinlock(),

 * This barrier must provide two things:
 *
 *   - it must guarantee a STORE before the spin_lock() is ordered against a
 * LOAD after it, see the comments at its two usage sites.
 *
 *   - it must ensure the critical section is RCsc.
 *
 * The latter is important for cases where we observe values written by other
 * CPUs in spin-loops, without barriers, while being subject to scheduling.
 *
 * CPU0 CPU1CPU2
 *
 *  for (;;) {
 *if (READ_ONCE(X))
 *  break;
 *  }
 * X=1
 *  
 *  
 *  r = X;
 *
 * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
 * we get migrated and CPU2 sees X==0.

which is used at,

__schedule(bool preempt) {
...
rq_lock(rq, );
smp_mb__after_spinlock();
...
}
.

If I didn't miss something, I found this kind of visibility is __not__
necessarily
depends on the spinning-lock at __schedule being RCsc.

In fact, as for runnable task A, the migration would be,

 CPU0 CPU1CPU2



lock(rq0)
schedule out A
unock(rq0)

  lock(rq0)
  remove A from rq0
  unlock(rq0)

  lock(rq2)
  add A into rq2
  unlock(rq2)
lock(rq2)
schedule in A
unlock(rq2)



 happens-before
unlock(rq0) happends-before
lock(rq0) happends-before
unlock(rq2) happens-before
lock(rq2) happens-before


And for stopped tasks,

 CPU0 CPU1CPU2



lock(rq0)
schedule out A
remove A from rq0
store-release(A->on_cpu)
unock(rq0)

  load_acquire(A->on_cpu)
  set_task_cpu(A, 2)

  lock(rq2)
  add A into rq2
  unlock(rq2)

lock(rq2)
schedule in A
unlock(rq2)



 happens-before
store-release(A->on_cpu)  happens-before
load_acquire(A->on_cpu)  happens-before
unlock(rq2) happens-before
lock(rq2) happens-before


So, I think the only requirement to smp_mb__after_spinlock is
to guarantee a STORE before the spin_lock() is ordered
against a LOAD after it. So we could remove the RCsc requirement
to allow more efficient implementation.

Did I miss something or this RCsc requirement does not really matter?

smp_mb__after_spinlock requirement too strong?

[焦晓冬]

Peter pointed out in this patch https://patchwork.kernel.org/patch/9771921/
that the spinning-lock used at __schedule() should be RCsc to ensure
visibility of writes prior to __schedule when the task is to be migrated to
another CPU.

And this is emphasized at the comment of the newly introduced
smp_mb__after_spinlock(),

 * This barrier must provide two things:
 *
 *   - it must guarantee a STORE before the spin_lock() is ordered against a
 * LOAD after it, see the comments at its two usage sites.
 *
 *   - it must ensure the critical section is RCsc.
 *
 * The latter is important for cases where we observe values written by other
 * CPUs in spin-loops, without barriers, while being subject to scheduling.
 *
 * CPU0 CPU1CPU2
 *
 *  for (;;) {
 *if (READ_ONCE(X))
 *  break;
 *  }
 * X=1
 *  
 *  
 *  r = X;
 *
 * without transitivity it could be that CPU1 observes X!=0 breaks the loop,
 * we get migrated and CPU2 sees X==0.

which is used at,

__schedule(bool preempt) {
...
rq_lock(rq, );
smp_mb__after_spinlock();
...
}
.

If I didn't miss something, I found this kind of visibility is __not__
necessarily
depends on the spinning-lock at __schedule being RCsc.

In fact, as for runnable task A, the migration would be,

 CPU0 CPU1CPU2



lock(rq0)
schedule out A
unock(rq0)

  lock(rq0)
  remove A from rq0
  unlock(rq0)

  lock(rq2)
  add A into rq2
  unlock(rq2)
lock(rq2)
schedule in A
unlock(rq2)



 happens-before
unlock(rq0) happends-before
lock(rq0) happends-before
unlock(rq2) happens-before
lock(rq2) happens-before


And for stopped tasks,

 CPU0 CPU1CPU2



lock(rq0)
schedule out A
remove A from rq0
store-release(A->on_cpu)
unock(rq0)

  load_acquire(A->on_cpu)
  set_task_cpu(A, 2)

  lock(rq2)
  add A into rq2
  unlock(rq2)

lock(rq2)
schedule in A
unlock(rq2)



 happens-before
store-release(A->on_cpu)  happens-before
load_acquire(A->on_cpu)  happens-before
unlock(rq2) happens-before
lock(rq2) happens-before


So, I think the only requirement to smp_mb__after_spinlock is
to guarantee a STORE before the spin_lock() is ordered
against a LOAD after it. So we could remove the RCsc requirement
to allow more efficient implementation.

Did I miss something or this RCsc requirement does not really matter?

RE: [RFC/RFT][PATCH v3 0/6] sched/cpuidle: Idle loop rework

[Doug Smythies]

On 2018.03.10 15:55 Rafael J. Wysocki wrote: 
>On Saturday, March 10, 2018 5:07:36 PM CET Doug Smythies wrote:
>> On 2018.03.10 01:00 Rafael J. Wysocki wrote:
>
... [snip] ...

> The information that they often spend more time than a tick
> period in state 0 in one go *is* relevant, though.
>
>
> That issue can be dealt with in a couple of ways and the patch below is a
> rather straightforward attempt to do that.  The idea, basically, is to discard
> the result of governor prediction if the tick has been stopped alread and
> the predicted idle duration is within the tick range.
>
> Please try it on top of the v3 and tell me if you see an improvement.

It seems pretty good so far.
See a new line added to the previous graph, "rjwv3plus".

http://fast.smythies.com/rjwv3plus_100.png

I'll do another 100% load on one CPU test overnight, this time with
a trace.

... Doug

RE: [RFC/RFT][PATCH v3 0/6] sched/cpuidle: Idle loop rework

[Doug Smythies]

On 2018.03.10 15:55 Rafael J. Wysocki wrote: 
>On Saturday, March 10, 2018 5:07:36 PM CET Doug Smythies wrote:
>> On 2018.03.10 01:00 Rafael J. Wysocki wrote:
>
... [snip] ...

> The information that they often spend more time than a tick
> period in state 0 in one go *is* relevant, though.
>
>
> That issue can be dealt with in a couple of ways and the patch below is a
> rather straightforward attempt to do that.  The idea, basically, is to discard
> the result of governor prediction if the tick has been stopped alread and
> the predicted idle duration is within the tick range.
>
> Please try it on top of the v3 and tell me if you see an improvement.

It seems pretty good so far.
See a new line added to the previous graph, "rjwv3plus".

http://fast.smythies.com/rjwv3plus_100.png

I'll do another 100% load on one CPU test overnight, this time with
a trace.

... Doug

Re: Fwd: [PATCH v4.15.7 1/1] on Intel, VDSO should handle CLOCK_MONOTONIC_RAW and export 'tsc_calibration' pointer

[Jason Vas Dias]

Hi Thomas -

Thanks very much for your help & guidance in previous mail:

RE: On 08/03/2018, Thomas Gleixner  wrote:
> 
> The right way to do that is to put the raw conversion values and the raw
> seconds base value into the vdso data and implement the counterpart of
> getrawmonotonic64(). And if that is done, then it can be done for _ALL_
> clocksources which support VDSO access and not just for the TSC.
>

I have done this now with a new patch, sent in mail with subject :
  
'[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle 
CLOCK_MONOTONIC_RAW' 

which should address all the concerns you raise.

> I already  know how that works, really.

I never doubted or meant to impugn that !

I am beginning to know a little how that works also, thanks in great
part to your help last week - thanks for your patience.

I was impatient last week to get access to low latency timers for a work
project, and was trying to read the unadjusted clock .

> instead of making completely false claims about the correctness of the kernel
> timekeeping infrastructure.

I really didn't mean to make any such claims - I'm sorry if I did .  I was just 
trying
to say that by the time the results of clock_gettime(CLOCK_MONOTONIC_RAW,) 
were
available to the caller they were not of much use because of the
latencies often dwarfing the time differences .

Anyway, I hope sometime you will consider putting such a patch in the
kernel.

I have developed a verson for ARM also, but that depends on making
CNTPCT + CNTFRQ registers readable in user-space, which is not meant
to be secure and is not normally done , but does work - but it is
against the Texas Instruments (ti-linux) kernel and can be enabled
with a new KConfig option, and brings latencies down from > 300ns
to < 20ns . Maybe I should post that also to kernel.org, or to
ti.com ?

I have a separate patch for the vdso_tsc_calibration export of the
tsc_khz and calibration which no longer returns pointers into the VDSO -
I can post this as a patch if you like.

Thanks & Best Regards,
Jason Vas Dias 

diff -up linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4	2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c	2018-03-11 05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
 	return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline stall "barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx 
+tsc  = u64)tsc_hi) & 0xUL) << 32) | (((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+	if (likely(tsc >= last))
+		return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
 	u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
 	return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+	u64 v;
+	cycles_t cycles;
+
+	if (gtod->vclock_mode == VCLOCK_TSC)
+		cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+	else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+		cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+	else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+		cycles = vread_hvclock(mode);
+#endif
+	else
+		return 0;
+	v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+	return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
 	return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+	unsigned long seq;
+	u64 ns;
+	int mode;
+
+	do {
+		seq = gtod_read_begin(gtod);
+		mode = gtod->vclock_mode;
+		ts->tv_sec = gtod->monotonic_time_raw_sec;
+		ns = gtod->monotonic_time_raw_nsec;
+		ns += vgetsns_raw();
+		ns >>= gtod->raw_shift;
+	} while (unlikely(gtod_read_retry(gtod, seq)));
+
+	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+	ts->tv_nsec = ns;
+
+	return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
 	unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
 		if (do_monotonic(ts) == VCLOCK_NONE)
 			goto fallback;
 		break;
+	case CLOCK_MONOTONIC_RAW:
+		if 

Re: Fwd: [PATCH v4.15.7 1/1] on Intel, VDSO should handle CLOCK_MONOTONIC_RAW and export 'tsc_calibration' pointer

[Jason Vas Dias]

Hi Thomas -

Thanks very much for your help & guidance in previous mail:

RE: On 08/03/2018, Thomas Gleixner  wrote:
> 
> The right way to do that is to put the raw conversion values and the raw
> seconds base value into the vdso data and implement the counterpart of
> getrawmonotonic64(). And if that is done, then it can be done for _ALL_
> clocksources which support VDSO access and not just for the TSC.
>

I have done this now with a new patch, sent in mail with subject :
  
'[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle 
CLOCK_MONOTONIC_RAW' 

which should address all the concerns you raise.

> I already  know how that works, really.

I never doubted or meant to impugn that !

I am beginning to know a little how that works also, thanks in great
part to your help last week - thanks for your patience.

I was impatient last week to get access to low latency timers for a work
project, and was trying to read the unadjusted clock .

> instead of making completely false claims about the correctness of the kernel
> timekeeping infrastructure.

I really didn't mean to make any such claims - I'm sorry if I did .  I was just 
trying
to say that by the time the results of clock_gettime(CLOCK_MONOTONIC_RAW,) 
were
available to the caller they were not of much use because of the
latencies often dwarfing the time differences .

Anyway, I hope sometime you will consider putting such a patch in the
kernel.

I have developed a verson for ARM also, but that depends on making
CNTPCT + CNTFRQ registers readable in user-space, which is not meant
to be secure and is not normally done , but does work - but it is
against the Texas Instruments (ti-linux) kernel and can be enabled
with a new KConfig option, and brings latencies down from > 300ns
to < 20ns . Maybe I should post that also to kernel.org, or to
ti.com ?

I have a separate patch for the vdso_tsc_calibration export of the
tsc_khz and calibration which no longer returns pointers into the VDSO -
I can post this as a patch if you like.

Thanks & Best Regards,
Jason Vas Dias 

diff -up linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4	2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c	2018-03-11 05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
 	return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline stall "barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx 
+tsc  = u64)tsc_hi) & 0xUL) << 32) | (((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+	if (likely(tsc >= last))
+		return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
 	u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
 	return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+	u64 v;
+	cycles_t cycles;
+
+	if (gtod->vclock_mode == VCLOCK_TSC)
+		cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+	else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+		cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+	else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+		cycles = vread_hvclock(mode);
+#endif
+	else
+		return 0;
+	v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+	return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
 	return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+	unsigned long seq;
+	u64 ns;
+	int mode;
+
+	do {
+		seq = gtod_read_begin(gtod);
+		mode = gtod->vclock_mode;
+		ts->tv_sec = gtod->monotonic_time_raw_sec;
+		ns = gtod->monotonic_time_raw_nsec;
+		ns += vgetsns_raw();
+		ns >>= gtod->raw_shift;
+	} while (unlikely(gtod_read_retry(gtod, seq)));
+
+	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+	ts->tv_nsec = ns;
+
+	return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
 	unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
 		if (do_monotonic(ts) == VCLOCK_NONE)
 			goto fallback;
 		break;
+	case CLOCK_MONOTONIC_RAW:
+		if (do_monotonic_raw(ts) == VCLOCK_NONE)
+			goto 

Re: [PATCH v6 09/17] media: rkisp1: add rockchip isp1 core driver

[Baruch Siach]

Hi Jacob,

On Thu, Mar 08, 2018 at 05:47:59PM +0800, Jacob Chen wrote:
> +config VIDEO_ROCKCHIP_ISP1
> + tristate "Rockchip Image Signal Processing v1 Unit driver"
> + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + default n
> + ---help---
> +   Support for ISP1 on the rockchip SoC.

I added 'select VIDEOBUF2_VMALLOC' here to fix link failure:

drivers/media/platform/rockchip/isp1/isp_stats.o: In function 
`rkisp1_register_stats_vdev':
isp_stats.c:(.text+0x80c): undefined reference to `vb2_vmalloc_memops'
isp_stats.c:(.text+0x814): undefined reference to `vb2_vmalloc_memops'
drivers/media/platform/rockchip/isp1/isp_params.o: In function 
`rkisp1_register_params_vdev':
isp_params.c:(.text+0x29b4): undefined reference to `vb2_vmalloc_memops'
isp_params.c:(.text+0x29bc): undefined reference to `vb2_vmalloc_memops'

baruch

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH v6 09/17] media: rkisp1: add rockchip isp1 core driver

[Baruch Siach]

Hi Jacob,

On Thu, Mar 08, 2018 at 05:47:59PM +0800, Jacob Chen wrote:
> +config VIDEO_ROCKCHIP_ISP1
> + tristate "Rockchip Image Signal Processing v1 Unit driver"
> + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + default n
> + ---help---
> +   Support for ISP1 on the rockchip SoC.

I added 'select VIDEOBUF2_VMALLOC' here to fix link failure:

drivers/media/platform/rockchip/isp1/isp_stats.o: In function 
`rkisp1_register_stats_vdev':
isp_stats.c:(.text+0x80c): undefined reference to `vb2_vmalloc_memops'
isp_stats.c:(.text+0x814): undefined reference to `vb2_vmalloc_memops'
drivers/media/platform/rockchip/isp1/isp_params.o: In function 
`rkisp1_register_params_vdev':
isp_params.c:(.text+0x29b4): undefined reference to `vb2_vmalloc_memops'
isp_params.c:(.text+0x29bc): undefined reference to `vb2_vmalloc_memops'

baruch

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

  Currently the VDSO does not handle
 clock_gettime( CLOCK_MONOTONIC_RAW,  )
  on Intel / AMD - it calls
 vdso_fallback_gettime()
  for this clock, which issues a syscall, having an unacceptably high
  latency (minimum measurable time or time between measurements)
  of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C
  machines under various versions of Linux.
 
  This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO ,
  by exporting the raw clock calibration, last cycles, last xtime_nsec,
  and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() .

  Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns
  on average, and the test program:
   tools/testing/selftest/timers/inconsistency-check.c
  succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value.

  The patch is against Linus' latest 4.16-rc4 tree,
  current HEAD of :
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  .

  The patch affects only files:
  
   arch/x86/include/asm/vgtod.h
   arch/x86/entry/vdso/vclock_gettime.c
   arch/x86/entry/vsyscall/vsyscall_gtod.c


  Best Regards,
 Jason Vas Dias  .
 
---
diff -up linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4
2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c 2018-03-11 
05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline stall 
"barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT 
rax, rdx, rcx 
+tsc  = u64)tsc_hi) & 0xUL) << 32) | 
(((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+   if (likely(tsc >= last))
+   return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+   u64 v;
+   cycles_t cycles;
+
+   if (gtod->vclock_mode == VCLOCK_TSC)
+   cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+   else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+   cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+   else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+   cycles = vread_hvclock(mode);
+#endif
+   else
+   return 0;
+   v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+   return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+   unsigned long seq;
+   u64 ns;
+   int mode;
+
+   do {
+   seq = gtod_read_begin(gtod);
+   mode = gtod->vclock_mode;
+   ts->tv_sec = gtod->monotonic_time_raw_sec;
+   ns = gtod->monotonic_time_raw_nsec;
+   ns += vgetsns_raw();
+   ns >>= gtod->raw_shift;
+   } while (unlikely(gtod_read_retry(gtod, seq)));
+
+   ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+   ts->tv_nsec = ns;
+
+   return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
if (do_monotonic(ts) == VCLOCK_NONE)
goto fallback;
break;
+   case CLOCK_MONOTONIC_RAW:
+   if (do_monotonic_raw(ts) == VCLOCK_NONE)
+   goto fallback;
+   break;
case CLOCK_REALTIME_COARSE:
do_realtime_coarse(ts);
break;
diff -up linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c
--- linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c  2018-03-11 
05:10:57.371197747 +
@@ -16,6 +16,7 @@
 

[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

  Currently the VDSO does not handle
 clock_gettime( CLOCK_MONOTONIC_RAW,  )
  on Intel / AMD - it calls
 vdso_fallback_gettime()
  for this clock, which issues a syscall, having an unacceptably high
  latency (minimum measurable time or time between measurements)
  of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C
  machines under various versions of Linux.
 
  This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO ,
  by exporting the raw clock calibration, last cycles, last xtime_nsec,
  and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() .

  Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns
  on average, and the test program:
   tools/testing/selftest/timers/inconsistency-check.c
  succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value.

  The patch is against Linus' latest 4.16-rc4 tree,
  current HEAD of :
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  .

  The patch affects only files:
  
   arch/x86/include/asm/vgtod.h
   arch/x86/entry/vdso/vclock_gettime.c
   arch/x86/entry/vsyscall/vsyscall_gtod.c


  Best Regards,
 Jason Vas Dias  .
 
---
diff -up linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4
2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c 2018-03-11 
05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline stall 
"barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT 
rax, rdx, rcx 
+tsc  = u64)tsc_hi) & 0xUL) << 32) | 
(((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+   if (likely(tsc >= last))
+   return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+   u64 v;
+   cycles_t cycles;
+
+   if (gtod->vclock_mode == VCLOCK_TSC)
+   cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+   else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+   cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+   else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+   cycles = vread_hvclock(mode);
+#endif
+   else
+   return 0;
+   v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+   return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+   unsigned long seq;
+   u64 ns;
+   int mode;
+
+   do {
+   seq = gtod_read_begin(gtod);
+   mode = gtod->vclock_mode;
+   ts->tv_sec = gtod->monotonic_time_raw_sec;
+   ns = gtod->monotonic_time_raw_nsec;
+   ns += vgetsns_raw();
+   ns >>= gtod->raw_shift;
+   } while (unlikely(gtod_read_retry(gtod, seq)));
+
+   ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+   ts->tv_nsec = ns;
+
+   return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
if (do_monotonic(ts) == VCLOCK_NONE)
goto fallback;
break;
+   case CLOCK_MONOTONIC_RAW:
+   if (do_monotonic_raw(ts) == VCLOCK_NONE)
+   goto fallback;
+   break;
case CLOCK_REALTIME_COARSE:
do_realtime_coarse(ts);
break;
diff -up linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c
--- linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
2018-03-04 22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c  2018-03-11 
05:10:57.371197747 +
@@ -16,6 +16,7 @@
 

Re: [PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

Oops, please disregard 1st mail on  $subject - I guess use of Quoted Printable
is not a way of getting past the email line length.
Patch I tried to send is attached as attachment - will resend inline using
other method.

Sorry, Regards, Jason


vdso_monotonic_raw-v4.16-rc4.patch
Description: Binary data

Re: [PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

Oops, please disregard 1st mail on  $subject - I guess use of Quoted Printable
is not a way of getting past the email line length.
Patch I tried to send is attached as attachment - will resend inline using
other method.

Sorry, Regards, Jason


vdso_monotonic_raw-v4.16-rc4.patch
Description: Binary data

[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

  Currently the VDSO does not handle
 clock_gettime( CLOCK_MONOTONIC_RAW,  )
  on Intel / AMD - it calls
 vdso_fallback_gettime()
  for this clock, which issues a syscall, having an unacceptably high
  latency (minimum measurable time or time between measurements)
  of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C
  machines under various versions of Linux.

  This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO ,
  by exporting the raw clock calibration, last cycles, last xtime_nsec,
  and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() .

  Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns
  on average, and the test program:
   tools/testing/selftest/timers/inconsistency-check.c
  succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value.

  The patch is against Linus' latest 4.16-rc4 tree,
  current HEAD of :
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  .

  The patch affects only files:

   arch/x86/include/asm/vgtod.h
   arch/x86/entry/vdso/vclock_gettime.c
   arch/x86/entry/vsyscall/vsyscall_gtod.c


  Best Regards,
 Jason Vas Dias  .

---
diff -up 
linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 2018-03-04 
22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c 2018-03-11 
05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
return last;
 }

+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline 
stall"barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT 
rax, rdx, rcx
+tsc  = u64)tsc_hi) & 0xUL) << 32) | 
(((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+   if (likely(tsc >= last))
+   return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
return v * gtod->mult;
 }

+notrace static inline u64 vgetsns_raw(int *mode)
+{
+   u64 v;
+   cycles_t cycles;
+
+   if (gtod->vclock_mode == VCLOCK_TSC)
+   cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+   else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+   cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+   else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+   cycles = vread_hvclock(mode);
+#endif
+   else
+   return 0;
+   v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+   return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
return mode;
 }

+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+   unsigned long seq;
+   u64 ns;
+   int mode;
+
+   do {
+   seq = gtod_read_begin(gtod);
+   mode = gtod->vclock_mode;
+   ts->tv_sec = gtod->monotonic_time_raw_sec;
+   ns = gtod->monotonic_time_raw_nsec;
+   ns += vgetsns_raw();
+   ns >>= gtod->raw_shift;
+   } while (unlikely(gtod_read_retry(gtod, seq)));
+
+   ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+   ts->tv_nsec = ns;
+
+   return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
if (do_monotonic(ts) == VCLOCK_NONE)
goto fallback;
break;
+   case CLOCK_MONOTONIC_RAW:
+   if (do_monotonic_raw(ts) == VCLOCK_NONE)
+   goto fallback;
+   break;
case CLOCK_REALTIME_COARSE:
do_realtime_coarse(ts);
break;
diff -up linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c
--- linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 2018-03-04 
22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c 2018-03-11 
05:10:57.371197747 +
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 

 int vclocks_used __read_mostly;

@@ -45,6 

[PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

  Currently the VDSO does not handle
 clock_gettime( CLOCK_MONOTONIC_RAW,  )
  on Intel / AMD - it calls
 vdso_fallback_gettime()
  for this clock, which issues a syscall, having an unacceptably high
  latency (minimum measurable time or time between measurements)
  of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C
  machines under various versions of Linux.

  This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO ,
  by exporting the raw clock calibration, last cycles, last xtime_nsec,
  and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() .

  Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns
  on average, and the test program:
   tools/testing/selftest/timers/inconsistency-check.c
  succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value.

  The patch is against Linus' latest 4.16-rc4 tree,
  current HEAD of :
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  .

  The patch affects only files:

   arch/x86/include/asm/vgtod.h
   arch/x86/entry/vdso/vclock_gettime.c
   arch/x86/entry/vsyscall/vsyscall_gtod.c


  Best Regards,
 Jason Vas Dias  .

---
diff -up 
linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
--- linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 2018-03-04 
22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c 2018-03-11 
05:08:31.137681337 +
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
return last;
 }

+notrace static u64 vread_tsc_raw(void)
+{
+u64 tsc, last=gtod->raw_cycle_last;
+if( likely( gtod->has_rdtscp ) ) {
+u32 tsc_lo, tsc_hi,
+tsc_cpu __attribute__((unused));
+asm volatile
+( "rdtscp"
+/* ^- has built-in cancellation point / pipeline 
stall"barrier" */
+: "=a" (tsc_lo)
+, "=d" (tsc_hi)
+, "=c" (tsc_cpu)
+); // since all variables 32-bit, eax, edx, ecx used - NOT 
rax, rdx, rcx
+tsc  = u64)tsc_hi) & 0xUL) << 32) | 
(((u64)tsc_lo) & 0xUL);
+} else {
+tsc  = rdtsc_ordered();
+}
+   if (likely(tsc >= last))
+   return tsc;
+asm volatile ("");
+return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
return v * gtod->mult;
 }

+notrace static inline u64 vgetsns_raw(int *mode)
+{
+   u64 v;
+   cycles_t cycles;
+
+   if (gtod->vclock_mode == VCLOCK_TSC)
+   cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+   else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+   cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+   else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+   cycles = vread_hvclock(mode);
+#endif
+   else
+   return 0;
+   v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+   return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
return mode;
 }

+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+   unsigned long seq;
+   u64 ns;
+   int mode;
+
+   do {
+   seq = gtod_read_begin(gtod);
+   mode = gtod->vclock_mode;
+   ts->tv_sec = gtod->monotonic_time_raw_sec;
+   ns = gtod->monotonic_time_raw_nsec;
+   ns += vgetsns_raw();
+   ns >>= gtod->raw_shift;
+   } while (unlikely(gtod_read_retry(gtod, seq)));
+
+   ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, );
+   ts->tv_nsec = ns;
+
+   return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
if (do_monotonic(ts) == VCLOCK_NONE)
goto fallback;
break;
+   case CLOCK_MONOTONIC_RAW:
+   if (do_monotonic_raw(ts) == VCLOCK_NONE)
+   goto fallback;
+   break;
case CLOCK_REALTIME_COARSE:
do_realtime_coarse(ts);
break;
diff -up linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 
linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c
--- linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 2018-03-04 
22:54:11.0 +
+++ linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c 2018-03-11 
05:10:57.371197747 +
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 

 int vclocks_used __read_mostly;

@@ -45,6 

[PATCH 0/4] driver core: use put_device() instead of kfree()

[Arvind Yadav]

Never directly free @dev after calling device_register(), even
if it returned an error! Always use put_device() to give up the
reference initialized.

Arvind Yadav (4):
  [PATCH 1/4] base: soc: use put_device() instead of kfree()
  [PATCH 2/4] driver core: platform: use put_device() if device_register fail
  [PATCH 3/4] driver core: node: use put_device() if device_register fail
  [PATCH 4/4] driver core: cpu: use put_device() if device_register fail

 drivers/base/cpu.c  | 4 +++-
 drivers/base/node.c | 4 +++-
 drivers/base/platform.c | 4 +++-
 drivers/base/soc.c  | 2 ++
 4 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 0/4] driver core: use put_device() instead of kfree()

[Arvind Yadav]

Never directly free @dev after calling device_register(), even
if it returned an error! Always use put_device() to give up the
reference initialized.

Arvind Yadav (4):
  [PATCH 1/4] base: soc: use put_device() instead of kfree()
  [PATCH 2/4] driver core: platform: use put_device() if device_register fail
  [PATCH 3/4] driver core: node: use put_device() if device_register fail
  [PATCH 4/4] driver core: cpu: use put_device() if device_register fail

 drivers/base/cpu.c  | 4 +++-
 drivers/base/node.c | 4 +++-
 drivers/base/platform.c | 4 +++-
 drivers/base/soc.c  | 2 ++
 4 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/4] base: soc: use put_device() instead of kfree()

[Arvind Yadav]

Never directly free @dev after calling device_register(), even
if it returned an error! Always use put_device() to give up the
reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/soc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 4e80f48..10b280f 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -150,6 +150,8 @@ struct soc_device *soc_device_register(struct 
soc_device_attribute *soc_dev_attr
 
 out3:
ida_simple_remove(_ida, soc_dev->soc_dev_num);
+   put_device(_dev->dev);
+   soc_dev = NULL;
 out2:
kfree(soc_dev);
 out1:
-- 
2.7.4

[PATCH 4/4] driver core: cpu: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index d21a2d9..2da998b 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -382,8 +382,10 @@ int register_cpu(struct cpu *cpu, int num)
if (cpu->hotpluggable)
cpu->dev.groups = hotplugable_cpu_attr_groups;
error = device_register(>dev);
-   if (error)
+   if (error) {
+   put_device(>dev);
return error;
+   }
 
per_cpu(cpu_sys_devices, num) = >dev;
register_cpu_under_node(num, cpu_to_node(num));
-- 
2.7.4

[PATCH 3/4] driver core: node: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/node.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index ee090ab..c5f81fc 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -315,7 +315,9 @@ static int register_node(struct node *node, int num)
node->dev.groups = node_dev_groups;
error = device_register(>dev);
 
-   if (!error){
+   if (error)
+   put_device(>dev);
+   else {
hugetlb_register_node(node);
 
compaction_register_node(node);
-- 
2.7.4

[PATCH 2/4] driver core: platform: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f1bf7b3..8075ddc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1153,8 +1153,10 @@ int __init platform_bus_init(void)
early_platform_cleanup();
 
error = device_register(_bus);
-   if (error)
+   if (error) {
+   put_device(_bus);
return error;
+   }
error =  bus_register(_bus_type);
if (error)
device_unregister(_bus);
-- 
2.7.4

[PATCH 1/4] base: soc: use put_device() instead of kfree()

[Arvind Yadav]

Never directly free @dev after calling device_register(), even
if it returned an error! Always use put_device() to give up the
reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/soc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 4e80f48..10b280f 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -150,6 +150,8 @@ struct soc_device *soc_device_register(struct 
soc_device_attribute *soc_dev_attr
 
 out3:
ida_simple_remove(_ida, soc_dev->soc_dev_num);
+   put_device(_dev->dev);
+   soc_dev = NULL;
 out2:
kfree(soc_dev);
 out1:
-- 
2.7.4

[PATCH 4/4] driver core: cpu: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index d21a2d9..2da998b 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -382,8 +382,10 @@ int register_cpu(struct cpu *cpu, int num)
if (cpu->hotpluggable)
cpu->dev.groups = hotplugable_cpu_attr_groups;
error = device_register(>dev);
-   if (error)
+   if (error) {
+   put_device(>dev);
return error;
+   }
 
per_cpu(cpu_sys_devices, num) = >dev;
register_cpu_under_node(num, cpu_to_node(num));
-- 
2.7.4

[PATCH 3/4] driver core: node: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/node.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index ee090ab..c5f81fc 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -315,7 +315,9 @@ static int register_node(struct node *node, int num)
node->dev.groups = node_dev_groups;
error = device_register(>dev);
 
-   if (!error){
+   if (error)
+   put_device(>dev);
+   else {
hugetlb_register_node(node);
 
compaction_register_node(node);
-- 
2.7.4

[PATCH 2/4] driver core: platform: use put_device() if device_register fail

[Arvind Yadav]

if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav 
---
 drivers/base/platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f1bf7b3..8075ddc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1153,8 +1153,10 @@ int __init platform_bus_init(void)
early_platform_cleanup();
 
error = device_register(_bus);
-   if (error)
+   if (error) {
+   put_device(_bus);
return error;
+   }
error =  bus_register(_bus_type);
if (error)
device_unregister(_bus);
-- 
2.7.4

Re: [RFC v2 00/83] NOVA: a new file system for persistent memory

[Andiry Xu]

On Sat, Mar 10, 2018 at 6:14 PM, Theodore Y. Ts'o  wrote:
> FYI, your patch set doesn't even compile for me without these fixups.
> I'm not sure why you were trying to declare inline functions in a
> header file without the function body?
>

Thanks for catching this. I will fix it in the next version and adopt
stricter flags next time.

Thanks,
Andiry

> - Ted
>
> diff --git a/fs/nova/balloc.c b/fs/nova/balloc.c
> index 8e992156f28c..9c7b74aa712e 100644
> --- a/fs/nova/balloc.c
> +++ b/fs/nova/balloc.c
> @@ -74,12 +74,12 @@ static void nova_init_free_list(struct super_block *sb,
> free_list->block_end -= sbi->tail_reserved_blocks;
>  }
>
> -inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
> +struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
>  {
> return nova_alloc_range_node(sb);
>  }
>
> -inline void nova_free_blocknode(struct super_block *sb,
> +void nova_free_blocknode(struct super_block *sb,
> struct nova_range_node *node)
>  {
> nova_free_range_node(node);
> @@ -206,7 +206,7 @@ int nova_insert_range_node(struct rb_root *tree,
> return 0;
>  }
>
> -inline int nova_insert_blocktree(struct nova_sb_info *sbi,
> +int nova_insert_blocktree(struct nova_sb_info *sbi,
> struct rb_root *tree, struct nova_range_node *new_node)
>  {
> int ret;
> @@ -659,7 +659,7 @@ static int nova_new_blocks(struct super_block *sb, 
> unsigned long *blocknr,
>
>  // Allocate data blocks.  The offset for the allocated block comes back in
>  // blocknr.  Return the number of blocks allocated.
> -inline int nova_new_data_blocks(struct super_block *sb,
> +int nova_new_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long *blocknr,
> unsigned long start_blk, unsigned int num,
> enum nova_alloc_init zero, int cpu,
> diff --git a/fs/nova/balloc.h b/fs/nova/balloc.h
> index 463fbac99eff..aca7e8c18dde 100644
> --- a/fs/nova/balloc.h
> +++ b/fs/nova/balloc.h
> @@ -62,18 +62,18 @@ enum alloc_type {
>
>  int nova_alloc_block_free_lists(struct super_block *sb);
>  void nova_delete_free_lists(struct super_block *sb);
> -inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
> -inline void nova_free_blocknode(struct super_block *sb,
> +struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
> +void nova_free_blocknode(struct super_block *sb,
> struct nova_range_node *bnode);
>  extern void nova_init_blockmap(struct super_block *sb, int recovery);
>  extern unsigned long nova_count_free_blocks(struct super_block *sb);
> -inline int nova_insert_blocktree(struct nova_sb_info *sbi,
> +int nova_insert_blocktree(struct nova_sb_info *sbi,
> struct rb_root *tree, struct nova_range_node *new_node);
>  extern int nova_free_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long blocknr, int num);
>  extern int nova_free_log_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long blocknr, int num);
> -extern inline int nova_new_data_blocks(struct super_block *sb,
> +extern int nova_new_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long *blocknr,
> unsigned long start_blk, unsigned int num,
> enum nova_alloc_init zero, int cpu,
> diff --git a/fs/nova/inode.c b/fs/nova/inode.c
> index 21be31a05d26..31ef258978ba 100644
> --- a/fs/nova/inode.c
> +++ b/fs/nova/inode.c
> @@ -440,7 +440,7 @@ struct inode *nova_iget(struct super_block *sb, unsigned 
> long ino)
> return ERR_PTR(err);
>  }
>
> -inline int nova_insert_inodetree(struct nova_sb_info *sbi,
> +int nova_insert_inodetree(struct nova_sb_info *sbi,
> struct nova_range_node *new_node, int cpu)
>  {
> struct rb_root *tree;
> diff --git a/fs/nova/inode.h b/fs/nova/inode.h
> index 086a7cba8ac3..1097e15ff7af 100644
> --- a/fs/nova/inode.h
> +++ b/fs/nova/inode.h
> @@ -254,7 +254,7 @@ int nova_init_inode_table(struct super_block *sb);
>  int nova_get_inode_address(struct super_block *sb, u64 ino,
> u64 *pi_addr, int extendable);
>  struct inode *nova_iget(struct super_block *sb, unsigned long ino);
> -inline int nova_insert_inodetree(struct nova_sb_info *sbi,
> +int nova_insert_inodetree(struct nova_sb_info *sbi,
> struct nova_range_node *new_node, int cpu);
>  u64 nova_new_nova_inode(struct super_block *sb, u64 *pi_addr);
>  struct inode *nova_new_vfs_inode(enum nova_new_inode_type type,
> diff --git a/fs/nova/super.c b/fs/nova/super.c
> index 039c003b698b..9f06ec847c95 100644
> --- a/fs/nova/super.c
> +++ b/fs/nova/super.c
> @@ -795,23 +795,23 @@ static void nova_put_super(struct super_block *sb)
> sb->s_fs_info = NULL;
>  }
>
> -inline void nova_free_range_node(struct nova_range_node *node)
> +void nova_free_range_node(struct nova_range_node *node)

Re: [RFC v2 00/83] NOVA: a new file system for persistent memory

[Andiry Xu]

On Sat, Mar 10, 2018 at 6:14 PM, Theodore Y. Ts'o  wrote:
> FYI, your patch set doesn't even compile for me without these fixups.
> I'm not sure why you were trying to declare inline functions in a
> header file without the function body?
>

Thanks for catching this. I will fix it in the next version and adopt
stricter flags next time.

Thanks,
Andiry

> - Ted
>
> diff --git a/fs/nova/balloc.c b/fs/nova/balloc.c
> index 8e992156f28c..9c7b74aa712e 100644
> --- a/fs/nova/balloc.c
> +++ b/fs/nova/balloc.c
> @@ -74,12 +74,12 @@ static void nova_init_free_list(struct super_block *sb,
> free_list->block_end -= sbi->tail_reserved_blocks;
>  }
>
> -inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
> +struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
>  {
> return nova_alloc_range_node(sb);
>  }
>
> -inline void nova_free_blocknode(struct super_block *sb,
> +void nova_free_blocknode(struct super_block *sb,
> struct nova_range_node *node)
>  {
> nova_free_range_node(node);
> @@ -206,7 +206,7 @@ int nova_insert_range_node(struct rb_root *tree,
> return 0;
>  }
>
> -inline int nova_insert_blocktree(struct nova_sb_info *sbi,
> +int nova_insert_blocktree(struct nova_sb_info *sbi,
> struct rb_root *tree, struct nova_range_node *new_node)
>  {
> int ret;
> @@ -659,7 +659,7 @@ static int nova_new_blocks(struct super_block *sb, 
> unsigned long *blocknr,
>
>  // Allocate data blocks.  The offset for the allocated block comes back in
>  // blocknr.  Return the number of blocks allocated.
> -inline int nova_new_data_blocks(struct super_block *sb,
> +int nova_new_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long *blocknr,
> unsigned long start_blk, unsigned int num,
> enum nova_alloc_init zero, int cpu,
> diff --git a/fs/nova/balloc.h b/fs/nova/balloc.h
> index 463fbac99eff..aca7e8c18dde 100644
> --- a/fs/nova/balloc.h
> +++ b/fs/nova/balloc.h
> @@ -62,18 +62,18 @@ enum alloc_type {
>
>  int nova_alloc_block_free_lists(struct super_block *sb);
>  void nova_delete_free_lists(struct super_block *sb);
> -inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
> -inline void nova_free_blocknode(struct super_block *sb,
> +struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
> +void nova_free_blocknode(struct super_block *sb,
> struct nova_range_node *bnode);
>  extern void nova_init_blockmap(struct super_block *sb, int recovery);
>  extern unsigned long nova_count_free_blocks(struct super_block *sb);
> -inline int nova_insert_blocktree(struct nova_sb_info *sbi,
> +int nova_insert_blocktree(struct nova_sb_info *sbi,
> struct rb_root *tree, struct nova_range_node *new_node);
>  extern int nova_free_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long blocknr, int num);
>  extern int nova_free_log_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long blocknr, int num);
> -extern inline int nova_new_data_blocks(struct super_block *sb,
> +extern int nova_new_data_blocks(struct super_block *sb,
> struct nova_inode_info_header *sih, unsigned long *blocknr,
> unsigned long start_blk, unsigned int num,
> enum nova_alloc_init zero, int cpu,
> diff --git a/fs/nova/inode.c b/fs/nova/inode.c
> index 21be31a05d26..31ef258978ba 100644
> --- a/fs/nova/inode.c
> +++ b/fs/nova/inode.c
> @@ -440,7 +440,7 @@ struct inode *nova_iget(struct super_block *sb, unsigned 
> long ino)
> return ERR_PTR(err);
>  }
>
> -inline int nova_insert_inodetree(struct nova_sb_info *sbi,
> +int nova_insert_inodetree(struct nova_sb_info *sbi,
> struct nova_range_node *new_node, int cpu)
>  {
> struct rb_root *tree;
> diff --git a/fs/nova/inode.h b/fs/nova/inode.h
> index 086a7cba8ac3..1097e15ff7af 100644
> --- a/fs/nova/inode.h
> +++ b/fs/nova/inode.h
> @@ -254,7 +254,7 @@ int nova_init_inode_table(struct super_block *sb);
>  int nova_get_inode_address(struct super_block *sb, u64 ino,
> u64 *pi_addr, int extendable);
>  struct inode *nova_iget(struct super_block *sb, unsigned long ino);
> -inline int nova_insert_inodetree(struct nova_sb_info *sbi,
> +int nova_insert_inodetree(struct nova_sb_info *sbi,
> struct nova_range_node *new_node, int cpu);
>  u64 nova_new_nova_inode(struct super_block *sb, u64 *pi_addr);
>  struct inode *nova_new_vfs_inode(enum nova_new_inode_type type,
> diff --git a/fs/nova/super.c b/fs/nova/super.c
> index 039c003b698b..9f06ec847c95 100644
> --- a/fs/nova/super.c
> +++ b/fs/nova/super.c
> @@ -795,23 +795,23 @@ static void nova_put_super(struct super_block *sb)
> sb->s_fs_info = NULL;
>  }
>
> -inline void nova_free_range_node(struct nova_range_node *node)
> +void nova_free_range_node(struct nova_range_node *node)
>  {
> 

Re: [PATCH] platform/x86: fujitsu-laptop: Revert UNSUPPORTED_CMD back to an int

[Jonathan Woithe]

On Sat, Mar 10, 2018 at 09:43:53PM +0100, Micha?? K??pie?? wrote:
> UNSUPPORTED_CMD was previously 0x8000 (int), but commit 819cddae7cfa
> ("platform/x86: fujitsu-laptop: Clean up constants") changed it into an
> unsigned long due to BIT() being used to define it.  As call_fext_func()
> returns an int, 0x8000 would get type promoted when compared to an
> unsigned long, which on a 64-bit system would cause it to become
> 0x8000 due to sign extension.  This causes one logical
> condition in fujitsu-laptop to always be true and another one to always
> be false on 64-bit systems.  Fix this by reverting UNSUPPORTED_CMD back
> to an int.
> 
> This patch fixes the following smatch warnings:
> 
> drivers/platform/x86/fujitsu-laptop.c:763 acpi_fujitsu_laptop_leds_register() 
> warn: always true condition '(call_fext_func(device, ((1 << (12)) | (1 << 
> (0))), 2, (1 << (16)), 0) != (1 << (31))) => (s32min-s32max != 2147483648)'
> drivers/platform/x86/fujitsu-laptop.c:816 acpi_fujitsu_laptop_add() warn: 
> impossible condition '(priv->flags_supported == (1 << (31))) => 
> (0-2147483647,18446744071562067968-u64max == 2147483648)'
> 
> Fixes: 819cddae7cfa ("platform/x86: fujitsu-laptop: Clean up constants")
> Reported-by: Dan Carpenter 
> Signed-off-by: Micha?? K??pie?? 
> ---
> This fixes a bug introduced by a commit queued for 4.17, so it needs to
> be applied on top of for-next.
> 
>  drivers/platform/x86/fujitsu-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c 
> b/drivers/platform/x86/fujitsu-laptop.c
> index 13bcdfea5349..6f4a55a53ced 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -85,7 +85,7 @@
>  #define FUNC_BACKLIGHT   (BIT(12) | BIT(2))
>  
>  /* FUNC interface - responses */
> -#define UNSUPPORTED_CMD  BIT(31)
> +#define UNSUPPORTED_CMD  0x8000
>  
>  /* FUNC interface - status flags */
>  #define FLAG_RFKILL  BIT(5)

This looks like a sensible, succinct solution to the regression.

Reviewed-by: Jonathan Woithe 

Regards
  jonathan

Re: [PATCH] platform/x86: fujitsu-laptop: Revert UNSUPPORTED_CMD back to an int

[Jonathan Woithe]

On Sat, Mar 10, 2018 at 09:43:53PM +0100, Micha?? K??pie?? wrote:
> UNSUPPORTED_CMD was previously 0x8000 (int), but commit 819cddae7cfa
> ("platform/x86: fujitsu-laptop: Clean up constants") changed it into an
> unsigned long due to BIT() being used to define it.  As call_fext_func()
> returns an int, 0x8000 would get type promoted when compared to an
> unsigned long, which on a 64-bit system would cause it to become
> 0x8000 due to sign extension.  This causes one logical
> condition in fujitsu-laptop to always be true and another one to always
> be false on 64-bit systems.  Fix this by reverting UNSUPPORTED_CMD back
> to an int.
> 
> This patch fixes the following smatch warnings:
> 
> drivers/platform/x86/fujitsu-laptop.c:763 acpi_fujitsu_laptop_leds_register() 
> warn: always true condition '(call_fext_func(device, ((1 << (12)) | (1 << 
> (0))), 2, (1 << (16)), 0) != (1 << (31))) => (s32min-s32max != 2147483648)'
> drivers/platform/x86/fujitsu-laptop.c:816 acpi_fujitsu_laptop_add() warn: 
> impossible condition '(priv->flags_supported == (1 << (31))) => 
> (0-2147483647,18446744071562067968-u64max == 2147483648)'
> 
> Fixes: 819cddae7cfa ("platform/x86: fujitsu-laptop: Clean up constants")
> Reported-by: Dan Carpenter 
> Signed-off-by: Micha?? K??pie?? 
> ---
> This fixes a bug introduced by a commit queued for 4.17, so it needs to
> be applied on top of for-next.
> 
>  drivers/platform/x86/fujitsu-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c 
> b/drivers/platform/x86/fujitsu-laptop.c
> index 13bcdfea5349..6f4a55a53ced 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -85,7 +85,7 @@
>  #define FUNC_BACKLIGHT   (BIT(12) | BIT(2))
>  
>  /* FUNC interface - responses */
> -#define UNSUPPORTED_CMD  BIT(31)
> +#define UNSUPPORTED_CMD  0x8000
>  
>  /* FUNC interface - status flags */
>  #define FLAG_RFKILL  BIT(5)

This looks like a sensible, succinct solution to the regression.

Reviewed-by: Jonathan Woithe 

Regards
  jonathan

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[joeyli]

On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote:
> On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > > what's the status of this please? Distributors (I checked SUSE,
> > > RedHat and Ubuntu) have to carry these patches and every of them
> > > have to forward-port the patches to new kernels. So are you going
> > > to resend the PR to have this merged?
> [...]
> > Just because I trust the platform keys prior to booting the kernel,
> > doesn't mean that I *want* to trust those keys once booted.  There
> > are, however, places where we need access to those keys to verify a
> > signature (eg. kexec kernel image).
> 
> Which is essentially the reason I always give when these patches come
> back
>

Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed"
patch checks MokIgnoreDB variable to ignore db:

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi=7c395b30a33a617c5cc2cdd419300af71277b79a

I think that we can consider to use MokAllowDB. Which means that kernel
ignores DB by default.

> > Nayna Jain's "certs: define a trusted platform keyring" patch set
> > introduces a new, separate keyring for these platform keys.
> 
> Perhaps, to break the deadlock, we should ask Jiří what the reason is
> the distros want these keys to be trusted.  Apart from the Microsoft
> key, it will also give you an OEM key in your trusted keyring.  Is it
> something to do with OEM supplied modules?
>

As I remember that some manufacturers uses certificate in db to
sign their kernel module. We need to discuss with them for switching
to mok. Currently I do not know all use cases for using db.

There have some benefits for using db:

 - User does not need to deal with shim-mokmanager to enroll mok.
   Target machine doesn't need to reboot and user doesn't need to
   face to mokmanager UI.  

 - The db is a authenticated variable, it's still secure when secure
   boot is disabled.
   The db is a authenticated variable that it can only be modified
   by manufacturer's key. Kernel can trust it when secure boot
   is disabled. It's useful for we do not need to taint kernel
   for loading a manufacturer's kernel module even secure boot is
   disabled.

 - Do not need to worry about the space of NVRAM and the EFI firmware
   implementation for writing a boot time variable.
  
But I also agree that we should not trust all keys (like Microsoft key)
in db by default.

Thanks a lot!
Joey Lee

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[joeyli]

On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote:
> On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > > what's the status of this please? Distributors (I checked SUSE,
> > > RedHat and Ubuntu) have to carry these patches and every of them
> > > have to forward-port the patches to new kernels. So are you going
> > > to resend the PR to have this merged?
> [...]
> > Just because I trust the platform keys prior to booting the kernel,
> > doesn't mean that I *want* to trust those keys once booted.  There
> > are, however, places where we need access to those keys to verify a
> > signature (eg. kexec kernel image).
> 
> Which is essentially the reason I always give when these patches come
> back
>

Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed"
patch checks MokIgnoreDB variable to ignore db:

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi=7c395b30a33a617c5cc2cdd419300af71277b79a

I think that we can consider to use MokAllowDB. Which means that kernel
ignores DB by default.

> > Nayna Jain's "certs: define a trusted platform keyring" patch set
> > introduces a new, separate keyring for these platform keys.
> 
> Perhaps, to break the deadlock, we should ask Jiří what the reason is
> the distros want these keys to be trusted.  Apart from the Microsoft
> key, it will also give you an OEM key in your trusted keyring.  Is it
> something to do with OEM supplied modules?
>

As I remember that some manufacturers uses certificate in db to
sign their kernel module. We need to discuss with them for switching
to mok. Currently I do not know all use cases for using db.

There have some benefits for using db:

 - User does not need to deal with shim-mokmanager to enroll mok.
   Target machine doesn't need to reboot and user doesn't need to
   face to mokmanager UI.  

 - The db is a authenticated variable, it's still secure when secure
   boot is disabled.
   The db is a authenticated variable that it can only be modified
   by manufacturer's key. Kernel can trust it when secure boot
   is disabled. It's useful for we do not need to taint kernel
   for loading a manufacturer's kernel module even secure boot is
   disabled.

 - Do not need to worry about the space of NVRAM and the EFI firmware
   implementation for writing a boot time variable.
  
But I also agree that we should not trust all keys (like Microsoft key)
in db by default.

Thanks a lot!
Joey Lee

ERROR: "sst_context_init" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 4772c16ede522d46219a59646503d2020841a6f4 ASoC: Intel: Kconfig: 
Simplify-clarify ACPI/PCI dependencies
date:   9 weeks ago
config: x86_64-randconfig-ws0-03110850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 4772c16ede522d46219a59646503d2020841a6f4
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   WARNING: modpost: missing MODULE_LICENSE() in 
drivers/auxdisplay/img-ascii-lcd.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/iio/accel/kxsd9-i2c.o
   see include/linux/module.h for more information
>> ERROR: "sst_context_init" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] 
>> undefined!
>> ERROR: "sst_context_cleanup" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!
>> ERROR: "sst_alloc_drv_context" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!
>> ERROR: "intel_sst_pm" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] 
>> undefined!
>> ERROR: "sst_configure_runtime_pm" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!

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


.config.gz
Description: application/gzip

ERROR: "sst_context_init" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 4772c16ede522d46219a59646503d2020841a6f4 ASoC: Intel: Kconfig: 
Simplify-clarify ACPI/PCI dependencies
date:   9 weeks ago
config: x86_64-randconfig-ws0-03110850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 4772c16ede522d46219a59646503d2020841a6f4
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   WARNING: modpost: missing MODULE_LICENSE() in 
drivers/auxdisplay/img-ascii-lcd.o
   see include/linux/module.h for more information
   WARNING: modpost: missing MODULE_LICENSE() in drivers/iio/accel/kxsd9-i2c.o
   see include/linux/module.h for more information
>> ERROR: "sst_context_init" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] 
>> undefined!
>> ERROR: "sst_context_cleanup" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!
>> ERROR: "sst_alloc_drv_context" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!
>> ERROR: "intel_sst_pm" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] 
>> undefined!
>> ERROR: "sst_configure_runtime_pm" 
>> [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko] undefined!

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


.config.gz
Description: application/gzip

Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

[Victor Kamensky]

On Tue, 20 Feb 2018, Stephen Smalley wrote:


On Fri, 2018-02-16 at 20:33 +, Taras Kondratiuk wrote:

From: Victor Kamensky 

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Note new DELAYAFTERINIT_MNT super block flag is introduced
to only mark rootfs for such behavior. For other types of
tmpfs original logic is still used.


(cc selinux maintainers)

Wondering if we shouldn't just do this always, for all filesystem
types.


Ok, I think it makes sense. The one that do not support xattrs
will not reach selinux_inode_post_setxattr anyway. And try
to cache sid while !ss_initialized is not good idea for any
filesystem types.


Also, I think this should likely also be done in
selinux_inode_setsecurity() for consistency.


I am not sure that I follow selinux_inode_setsecurity suggestion.
selinux_inode_setsecurity is about permission check. And
selinux_inode_post_setxattr deals with processing and setting
side effects if xattr was "security.selinux", it does not
matter what happens in selinux_inode_setsecurity if it
returns access_ok, LSM will still call selinux_inode_post_setxattr
and we would need to check and not produce any sid caching
side effects if !ss_initialized.

Sitll keeping logic in selinux_inode_post_setxattr, checked
that the following with much simple code works too:


From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:06:08 -0800
Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy
 loaded

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 31303ed..4c13759 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry 
*dentry, const char *name,
return;
}

+if (!ss_initialized) {
+return;
+}
+
rc = security_context_to_sid_force(value, size, );
if (rc) {
printk(KERN_ERR "SELinux:  unable to map context to SID"
--
2.7.4

Thanks,
Victor



Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c| 9 -
 security/selinux/include/security.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f3fe65589f02..bb25268f734e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct
super_block *sb,
 */
if (!strncmp(sb->s_type->name, "rootfs",
 sizeof("rootfs")))
-   sbsec->flags |= SBLABEL_MNT;
+   sbsec->flags |=
SBLABEL_MNT|DELAYAFTERINIT_MNT;

/* Defer initialization until
selinux_complete_init,
   after the initial policy is loaded and
the security
@@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct
dentry *dentry, const char *name,
 {
struct inode *inode = d_backing_inode(dentry);
struct inode_security_struct *isec;
+   struct superblock_security_struct *sbsec;
u32 newsid;
int rc;

@@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct
dentry *dentry, const 

Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

[Victor Kamensky]

On Tue, 20 Feb 2018, Stephen Smalley wrote:


On Fri, 2018-02-16 at 20:33 +, Taras Kondratiuk wrote:

From: Victor Kamensky 

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Note new DELAYAFTERINIT_MNT super block flag is introduced
to only mark rootfs for such behavior. For other types of
tmpfs original logic is still used.


(cc selinux maintainers)

Wondering if we shouldn't just do this always, for all filesystem
types.


Ok, I think it makes sense. The one that do not support xattrs
will not reach selinux_inode_post_setxattr anyway. And try
to cache sid while !ss_initialized is not good idea for any
filesystem types.


Also, I think this should likely also be done in
selinux_inode_setsecurity() for consistency.


I am not sure that I follow selinux_inode_setsecurity suggestion.
selinux_inode_setsecurity is about permission check. And
selinux_inode_post_setxattr deals with processing and setting
side effects if xattr was "security.selinux", it does not
matter what happens in selinux_inode_setsecurity if it
returns access_ok, LSM will still call selinux_inode_post_setxattr
and we would need to check and not produce any sid caching
side effects if !ss_initialized.

Sitll keeping logic in selinux_inode_post_setxattr, checked
that the following with much simple code works too:


From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:06:08 -0800
Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy
 loaded

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 31303ed..4c13759 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry 
*dentry, const char *name,
return;
}

+if (!ss_initialized) {
+return;
+}
+
rc = security_context_to_sid_force(value, size, );
if (rc) {
printk(KERN_ERR "SELinux:  unable to map context to SID"
--
2.7.4

Thanks,
Victor



Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c| 9 -
 security/selinux/include/security.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f3fe65589f02..bb25268f734e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct
super_block *sb,
 */
if (!strncmp(sb->s_type->name, "rootfs",
 sizeof("rootfs")))
-   sbsec->flags |= SBLABEL_MNT;
+   sbsec->flags |=
SBLABEL_MNT|DELAYAFTERINIT_MNT;

/* Defer initialization until
selinux_complete_init,
   after the initial policy is loaded and
the security
@@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct
dentry *dentry, const char *name,
 {
struct inode *inode = d_backing_inode(dentry);
struct inode_security_struct *isec;
+   struct superblock_security_struct *sbsec;
u32 newsid;
int rc;

@@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct
dentry *dentry, const char *name,
return;
}

+   if (!ss_initialized) {
+ 

Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them

[Victor Kamensky]

On Tue, 20 Feb 2018, Stephen Smalley wrote:


On Fri, 2018-02-16 at 20:33 +, Taras Kondratiuk wrote:

From: Victor Kamensky 

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable SBLABEL_MNT on rootfs even if secrurity server is
not initialized yet.


What if we were to instead skip the SBLABEL_MNT check in
selinux_inode_setxattr() if !ss_initialized?  Not dependent on
filesystem type.


Stephen, thank you for looking into this. Sorry, for dealyed reponse -
I needed to find time to require context about these changes.

As you suggested I've tried this and it works:


From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on file systems if policy is not
 loaded

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code because file system is not
yet marked as one that support labeling (SBLABEL_MNT flag).

Solution do not refuse setxattr even if SBLABEL_MNT is not set
for file systems when policy is not loaded yet.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..31303ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3120,7 +3120,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, 
const char *name,
return selinux_inode_setotherxattr(dentry, name);

sbsec = inode->i_sb->s_security;
-   if (!(sbsec->flags & SBLABEL_MNT))
+   if (!(sbsec->flags & SBLABEL_MNT) && ss_initialized)
return -EOPNOTSUPP;

if (!inode_owner_or_capable(inode))
--
2.7.4

But with this change it would mean for that filesystem types, that
never are supposed to get SBLABEL_MNT flag, code may go through
if !ss_initialized. I have hard time evaluating impication of this,
but it is not existing case or not a big deal.

Generally I agree with your concern that the issue is not "rootfs"
specific. Other thought that it could be solved with use of
selinux_is_sblabel_mnt instead of "rootfs" specific check inside
of selinux_set_mnt_opts, in addition to similar code
in sb_finish_set_opts function. I.e something like this:


From a94548b5ecde43ccc9c2b02b29becc086b4343a3 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on rootfs so initramfs code can
 set them

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable set SBLABEL_MNT on file systems for which we can
figure out that they support securit labels even if secrurity
server is not initialized yet.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..326aca9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -701,6 +701,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+/*
+ * For some of file systems we can mark them as
+ * supporting security labels even before policy
+ * loaded. It may be used by code that may want
+ * to do setxatts before polict load.
+ *
+ * Note after policy loaded this check and marking
+ * happens again.
+ */
+if (selinux_is_sblabel_mnt(sb))
+sbsec->flags |= SBLABEL_MNT;
+
/* Defer initialization until selinux_complete_init,
   after the initial policy is loaded and the security
   server is ready to handle calls. */
--
2.7.4

Thanks,
Victor



Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..f3fe65589f02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -706,6 +706,18 @@ static int 

Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them

[Victor Kamensky]

On Tue, 20 Feb 2018, Stephen Smalley wrote:


On Fri, 2018-02-16 at 20:33 +, Taras Kondratiuk wrote:

From: Victor Kamensky 

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable SBLABEL_MNT on rootfs even if secrurity server is
not initialized yet.


What if we were to instead skip the SBLABEL_MNT check in
selinux_inode_setxattr() if !ss_initialized?  Not dependent on
filesystem type.


Stephen, thank you for looking into this. Sorry, for dealyed reponse -
I needed to find time to require context about these changes.

As you suggested I've tried this and it works:


From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on file systems if policy is not
 loaded

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code because file system is not
yet marked as one that support labeling (SBLABEL_MNT flag).

Solution do not refuse setxattr even if SBLABEL_MNT is not set
for file systems when policy is not loaded yet.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..31303ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3120,7 +3120,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, 
const char *name,
return selinux_inode_setotherxattr(dentry, name);

sbsec = inode->i_sb->s_security;
-   if (!(sbsec->flags & SBLABEL_MNT))
+   if (!(sbsec->flags & SBLABEL_MNT) && ss_initialized)
return -EOPNOTSUPP;

if (!inode_owner_or_capable(inode))
--
2.7.4

But with this change it would mean for that filesystem types, that
never are supposed to get SBLABEL_MNT flag, code may go through
if !ss_initialized. I have hard time evaluating impication of this,
but it is not existing case or not a big deal.

Generally I agree with your concern that the issue is not "rootfs"
specific. Other thought that it could be solved with use of
selinux_is_sblabel_mnt instead of "rootfs" specific check inside
of selinux_set_mnt_opts, in addition to similar code
in sb_finish_set_opts function. I.e something like this:


From a94548b5ecde43ccc9c2b02b29becc086b4343a3 Mon Sep 17 00:00:00 2001

From: Victor Kamensky 
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on rootfs so initramfs code can
 set them

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable set SBLABEL_MNT on file systems for which we can
figure out that they support securit labels even if secrurity
server is not initialized yet.

Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..326aca9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -701,6 +701,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+/*
+ * For some of file systems we can mark them as
+ * supporting security labels even before policy
+ * loaded. It may be used by code that may want
+ * to do setxatts before polict load.
+ *
+ * Note after policy loaded this check and marking
+ * happens again.
+ */
+if (selinux_is_sblabel_mnt(sb))
+sbsec->flags |= SBLABEL_MNT;
+
/* Defer initialization until selinux_complete_init,
   after the initial policy is loaded and the security
   server is ready to handle calls. */
--
2.7.4

Thanks,
Victor



Signed-off-by: Victor Kamensky 
---
 security/selinux/hooks.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..f3fe65589f02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct
super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+   

Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

[Andy Lutomirski]

On Sat, Mar 10, 2018 at 1:43 AM, Alexei Starovoitov  wrote:
> On 3/9/18 11:37 AM, Andy Lutomirski wrote:
>>
>> On Fri, Mar 9, 2018 at 6:55 PM, David Miller  wrote:
>>>
>>> From: Alexei Starovoitov 
>>> Date: Fri, 9 Mar 2018 10:50:49 -0800
>>>
 On 3/9/18 10:23 AM, Andy Lutomirski wrote:
>
> It might not be totally crazy to back it by tmpfs.


 interesting. how do you propose to do it?
 Something like:
 - create /umh_module_tempxxx dir
 - mount tmpfs there
 - copy elf into it and exec it?
>>>
>>>
>>> I think the idea is that it's an internal tmpfs mount that only
>>> the kernel has access too.
>>
>>
>> That's what I was imagining.  There's precedent.  For example, there's
>> a very short piece of code that does it in
>> drivers/gpu/drm/i915/i915_gemfs.c.
>
>
> I can do "monkey see monkey do" approach which will look like:
> type = get_fs_type("tmpfs");
> fs = kern_mount(type);
>
> /* for each request_umh("foo") */
> file = shmem_file_setup_with_mnt(fs, "umh_foo");
> do {
>   pagecache_write_begin(file,...);
>   memcpy()
>   pagecache_write_end();
> } while (umh_elf_size);
> do_execve_file(file);
> fput(file);
>
> while keeping fs mounted forever?
> is there better way?
>

Nice!  I'm definitely not a pagecache expert, but it looks generally
sane.  Once the thing is actually functional, we can bang on it, and
I'm sure that linux-mm will have some suggestions to tidy it up.

As for the actual lifetime of the filesystem, I think it should be
mounted once and never unmounted.  Whenever it gains a second user,
the whole thing can be moved to mm/ or lib/ and all the users can
share the same mount.

Minor caveat: I would arrange the code a bit differently, like this:

static (or extern) unsigned char __initdata the_blob[];
static struct file *umh_blob_file;

static int __init my_module_init_function(void)
{
 /* for each request_umh("foo") */
 umh_blob_file = shmem_file_setup_with_mnt(fs, "umh_foo");
 do {
   pagecache_write_begin(umh_file,...);
   memcpy()
   pagecache_write_end();
 } while (umh_elf_size);

 /* the_blob is implicitly freed after this returns */
}

and then actually use the struct file later on.  If and when you're
sure you're not going to spawn another copy, you can fput() it.  This
way the memory becomes swappable immediately on load.

As for request_module() vs request_module_umh(), my advice would be to
write the code and then see what interface makes sense.  I wouldn't be
surprised if it ends up making more sense to keep all of this entirely
independent from the module system.

P.S. I suspect that, before this hits a release, someone's going to
have to fiddle with the LSM hooks in do_execve() a bit to make sure
that LSM unconditionally approves this type of umh program.  Otherwise
there might be pointless failures on some more locked down
configurations.  But that can wait until it's more final and the
security folks review the code.

Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

[Andy Lutomirski]

On Sat, Mar 10, 2018 at 1:43 AM, Alexei Starovoitov  wrote:
> On 3/9/18 11:37 AM, Andy Lutomirski wrote:
>>
>> On Fri, Mar 9, 2018 at 6:55 PM, David Miller  wrote:
>>>
>>> From: Alexei Starovoitov 
>>> Date: Fri, 9 Mar 2018 10:50:49 -0800
>>>
 On 3/9/18 10:23 AM, Andy Lutomirski wrote:
>
> It might not be totally crazy to back it by tmpfs.


 interesting. how do you propose to do it?
 Something like:
 - create /umh_module_tempxxx dir
 - mount tmpfs there
 - copy elf into it and exec it?
>>>
>>>
>>> I think the idea is that it's an internal tmpfs mount that only
>>> the kernel has access too.
>>
>>
>> That's what I was imagining.  There's precedent.  For example, there's
>> a very short piece of code that does it in
>> drivers/gpu/drm/i915/i915_gemfs.c.
>
>
> I can do "monkey see monkey do" approach which will look like:
> type = get_fs_type("tmpfs");
> fs = kern_mount(type);
>
> /* for each request_umh("foo") */
> file = shmem_file_setup_with_mnt(fs, "umh_foo");
> do {
>   pagecache_write_begin(file,...);
>   memcpy()
>   pagecache_write_end();
> } while (umh_elf_size);
> do_execve_file(file);
> fput(file);
>
> while keeping fs mounted forever?
> is there better way?
>

Nice!  I'm definitely not a pagecache expert, but it looks generally
sane.  Once the thing is actually functional, we can bang on it, and
I'm sure that linux-mm will have some suggestions to tidy it up.

As for the actual lifetime of the filesystem, I think it should be
mounted once and never unmounted.  Whenever it gains a second user,
the whole thing can be moved to mm/ or lib/ and all the users can
share the same mount.

Minor caveat: I would arrange the code a bit differently, like this:

static (or extern) unsigned char __initdata the_blob[];
static struct file *umh_blob_file;

static int __init my_module_init_function(void)
{
 /* for each request_umh("foo") */
 umh_blob_file = shmem_file_setup_with_mnt(fs, "umh_foo");
 do {
   pagecache_write_begin(umh_file,...);
   memcpy()
   pagecache_write_end();
 } while (umh_elf_size);

 /* the_blob is implicitly freed after this returns */
}

and then actually use the struct file later on.  If and when you're
sure you're not going to spawn another copy, you can fput() it.  This
way the memory becomes swappable immediately on load.

As for request_module() vs request_module_umh(), my advice would be to
write the code and then see what interface makes sense.  I wouldn't be
surprised if it ends up making more sense to keep all of this entirely
independent from the module system.

P.S. I suspect that, before this hits a release, someone's going to
have to fiddle with the LSM hooks in do_execve() a bit to make sure
that LSM unconditionally approves this type of umh program.  Otherwise
there might be pointless failures on some more locked down
configurations.  But that can wait until it's more final and the
security folks review the code.

Re: [RFC v2 00/83] NOVA: a new file system for persistent memory

[Theodore Y. Ts'o]

FYI, your patch set doesn't even compile for me without these fixups.
I'm not sure why you were trying to declare inline functions in a
header file without the function body?

- Ted

diff --git a/fs/nova/balloc.c b/fs/nova/balloc.c
index 8e992156f28c..9c7b74aa712e 100644
--- a/fs/nova/balloc.c
+++ b/fs/nova/balloc.c
@@ -74,12 +74,12 @@ static void nova_init_free_list(struct super_block *sb,
free_list->block_end -= sbi->tail_reserved_blocks;
 }
 
-inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
+struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
 {
return nova_alloc_range_node(sb);
 }
 
-inline void nova_free_blocknode(struct super_block *sb,
+void nova_free_blocknode(struct super_block *sb,
struct nova_range_node *node)
 {
nova_free_range_node(node);
@@ -206,7 +206,7 @@ int nova_insert_range_node(struct rb_root *tree,
return 0;
 }
 
-inline int nova_insert_blocktree(struct nova_sb_info *sbi,
+int nova_insert_blocktree(struct nova_sb_info *sbi,
struct rb_root *tree, struct nova_range_node *new_node)
 {
int ret;
@@ -659,7 +659,7 @@ static int nova_new_blocks(struct super_block *sb, unsigned 
long *blocknr,
 
 // Allocate data blocks.  The offset for the allocated block comes back in
 // blocknr.  Return the number of blocks allocated.
-inline int nova_new_data_blocks(struct super_block *sb,
+int nova_new_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long *blocknr,
unsigned long start_blk, unsigned int num,
enum nova_alloc_init zero, int cpu,
diff --git a/fs/nova/balloc.h b/fs/nova/balloc.h
index 463fbac99eff..aca7e8c18dde 100644
--- a/fs/nova/balloc.h
+++ b/fs/nova/balloc.h
@@ -62,18 +62,18 @@ enum alloc_type {
 
 int nova_alloc_block_free_lists(struct super_block *sb);
 void nova_delete_free_lists(struct super_block *sb);
-inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
-inline void nova_free_blocknode(struct super_block *sb,
+struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
+void nova_free_blocknode(struct super_block *sb,
struct nova_range_node *bnode);
 extern void nova_init_blockmap(struct super_block *sb, int recovery);
 extern unsigned long nova_count_free_blocks(struct super_block *sb);
-inline int nova_insert_blocktree(struct nova_sb_info *sbi,
+int nova_insert_blocktree(struct nova_sb_info *sbi,
struct rb_root *tree, struct nova_range_node *new_node);
 extern int nova_free_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long blocknr, int num);
 extern int nova_free_log_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long blocknr, int num);
-extern inline int nova_new_data_blocks(struct super_block *sb,
+extern int nova_new_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long *blocknr,
unsigned long start_blk, unsigned int num,
enum nova_alloc_init zero, int cpu,
diff --git a/fs/nova/inode.c b/fs/nova/inode.c
index 21be31a05d26..31ef258978ba 100644
--- a/fs/nova/inode.c
+++ b/fs/nova/inode.c
@@ -440,7 +440,7 @@ struct inode *nova_iget(struct super_block *sb, unsigned 
long ino)
return ERR_PTR(err);
 }
 
-inline int nova_insert_inodetree(struct nova_sb_info *sbi,
+int nova_insert_inodetree(struct nova_sb_info *sbi,
struct nova_range_node *new_node, int cpu)
 {
struct rb_root *tree;
diff --git a/fs/nova/inode.h b/fs/nova/inode.h
index 086a7cba8ac3..1097e15ff7af 100644
--- a/fs/nova/inode.h
+++ b/fs/nova/inode.h
@@ -254,7 +254,7 @@ int nova_init_inode_table(struct super_block *sb);
 int nova_get_inode_address(struct super_block *sb, u64 ino,
u64 *pi_addr, int extendable);
 struct inode *nova_iget(struct super_block *sb, unsigned long ino);
-inline int nova_insert_inodetree(struct nova_sb_info *sbi,
+int nova_insert_inodetree(struct nova_sb_info *sbi,
struct nova_range_node *new_node, int cpu);
 u64 nova_new_nova_inode(struct super_block *sb, u64 *pi_addr);
 struct inode *nova_new_vfs_inode(enum nova_new_inode_type type,
diff --git a/fs/nova/super.c b/fs/nova/super.c
index 039c003b698b..9f06ec847c95 100644
--- a/fs/nova/super.c
+++ b/fs/nova/super.c
@@ -795,23 +795,23 @@ static void nova_put_super(struct super_block *sb)
sb->s_fs_info = NULL;
 }
 
-inline void nova_free_range_node(struct nova_range_node *node)
+void nova_free_range_node(struct nova_range_node *node)
 {
kmem_cache_free(nova_range_node_cachep, node);
 }
 
-inline void nova_free_inode_node(struct super_block *sb,
+void nova_free_inode_node(struct super_block *sb,
struct nova_range_node *node)
 {
nova_free_range_node(node);
 }
 
-inline void nova_free_file_write_item(struct nova_file_write_item *item)
+void nova_free_file_write_item(struct nova_file_write_item 

Re: [RFC v2 00/83] NOVA: a new file system for persistent memory

[Theodore Y. Ts'o]

FYI, your patch set doesn't even compile for me without these fixups.
I'm not sure why you were trying to declare inline functions in a
header file without the function body?

- Ted

diff --git a/fs/nova/balloc.c b/fs/nova/balloc.c
index 8e992156f28c..9c7b74aa712e 100644
--- a/fs/nova/balloc.c
+++ b/fs/nova/balloc.c
@@ -74,12 +74,12 @@ static void nova_init_free_list(struct super_block *sb,
free_list->block_end -= sbi->tail_reserved_blocks;
 }
 
-inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
+struct nova_range_node *nova_alloc_blocknode(struct super_block *sb)
 {
return nova_alloc_range_node(sb);
 }
 
-inline void nova_free_blocknode(struct super_block *sb,
+void nova_free_blocknode(struct super_block *sb,
struct nova_range_node *node)
 {
nova_free_range_node(node);
@@ -206,7 +206,7 @@ int nova_insert_range_node(struct rb_root *tree,
return 0;
 }
 
-inline int nova_insert_blocktree(struct nova_sb_info *sbi,
+int nova_insert_blocktree(struct nova_sb_info *sbi,
struct rb_root *tree, struct nova_range_node *new_node)
 {
int ret;
@@ -659,7 +659,7 @@ static int nova_new_blocks(struct super_block *sb, unsigned 
long *blocknr,
 
 // Allocate data blocks.  The offset for the allocated block comes back in
 // blocknr.  Return the number of blocks allocated.
-inline int nova_new_data_blocks(struct super_block *sb,
+int nova_new_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long *blocknr,
unsigned long start_blk, unsigned int num,
enum nova_alloc_init zero, int cpu,
diff --git a/fs/nova/balloc.h b/fs/nova/balloc.h
index 463fbac99eff..aca7e8c18dde 100644
--- a/fs/nova/balloc.h
+++ b/fs/nova/balloc.h
@@ -62,18 +62,18 @@ enum alloc_type {
 
 int nova_alloc_block_free_lists(struct super_block *sb);
 void nova_delete_free_lists(struct super_block *sb);
-inline struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
-inline void nova_free_blocknode(struct super_block *sb,
+struct nova_range_node *nova_alloc_blocknode(struct super_block *sb);
+void nova_free_blocknode(struct super_block *sb,
struct nova_range_node *bnode);
 extern void nova_init_blockmap(struct super_block *sb, int recovery);
 extern unsigned long nova_count_free_blocks(struct super_block *sb);
-inline int nova_insert_blocktree(struct nova_sb_info *sbi,
+int nova_insert_blocktree(struct nova_sb_info *sbi,
struct rb_root *tree, struct nova_range_node *new_node);
 extern int nova_free_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long blocknr, int num);
 extern int nova_free_log_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long blocknr, int num);
-extern inline int nova_new_data_blocks(struct super_block *sb,
+extern int nova_new_data_blocks(struct super_block *sb,
struct nova_inode_info_header *sih, unsigned long *blocknr,
unsigned long start_blk, unsigned int num,
enum nova_alloc_init zero, int cpu,
diff --git a/fs/nova/inode.c b/fs/nova/inode.c
index 21be31a05d26..31ef258978ba 100644
--- a/fs/nova/inode.c
+++ b/fs/nova/inode.c
@@ -440,7 +440,7 @@ struct inode *nova_iget(struct super_block *sb, unsigned 
long ino)
return ERR_PTR(err);
 }
 
-inline int nova_insert_inodetree(struct nova_sb_info *sbi,
+int nova_insert_inodetree(struct nova_sb_info *sbi,
struct nova_range_node *new_node, int cpu)
 {
struct rb_root *tree;
diff --git a/fs/nova/inode.h b/fs/nova/inode.h
index 086a7cba8ac3..1097e15ff7af 100644
--- a/fs/nova/inode.h
+++ b/fs/nova/inode.h
@@ -254,7 +254,7 @@ int nova_init_inode_table(struct super_block *sb);
 int nova_get_inode_address(struct super_block *sb, u64 ino,
u64 *pi_addr, int extendable);
 struct inode *nova_iget(struct super_block *sb, unsigned long ino);
-inline int nova_insert_inodetree(struct nova_sb_info *sbi,
+int nova_insert_inodetree(struct nova_sb_info *sbi,
struct nova_range_node *new_node, int cpu);
 u64 nova_new_nova_inode(struct super_block *sb, u64 *pi_addr);
 struct inode *nova_new_vfs_inode(enum nova_new_inode_type type,
diff --git a/fs/nova/super.c b/fs/nova/super.c
index 039c003b698b..9f06ec847c95 100644
--- a/fs/nova/super.c
+++ b/fs/nova/super.c
@@ -795,23 +795,23 @@ static void nova_put_super(struct super_block *sb)
sb->s_fs_info = NULL;
 }
 
-inline void nova_free_range_node(struct nova_range_node *node)
+void nova_free_range_node(struct nova_range_node *node)
 {
kmem_cache_free(nova_range_node_cachep, node);
 }
 
-inline void nova_free_inode_node(struct super_block *sb,
+void nova_free_inode_node(struct super_block *sb,
struct nova_range_node *node)
 {
nova_free_range_node(node);
 }
 
-inline void nova_free_file_write_item(struct nova_file_write_item *item)
+void nova_free_file_write_item(struct nova_file_write_item 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier  wrote:
> On Fri, 09 Mar 2018 21:36:12 +,
> Christoffer Dall wrote:
>>
>> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
>> > I'd be more confident if we did forbid P+A for such interrupts
>> > altogether, as they really feel like another kind of HW interrupt.
>>
>> How about a slightly bigger hammer:  Can we avoid doing P+A for level
>> interrupts completely?  I don't think that really makes much sense, and
>> I think we simply everything if we just come back out and resample the
>> line.  For an edge, something like a network card, there's a potential
>> performance win to appending a new pending state, but I doubt that this
>> is the case for level interrupts.
>
> I started implementing the same thing yesterday. Somehow, it feels
> slightly better to have the same flow for all level interrupts,
> including the timer, and we only use the MI on EOI as a way to trigger
> the next state of injection. Still testing, but looking good so far.
>
> I'm still puzzled that we have this level-but-not-quite behaviour for
> VFIO interrupts. At some point, it is going to bite us badly.
>

Where is the departure from level-triggered behavior with VFIO?  As
far as I can tell, the GIC flow of the interrupts will be just a level
interrupt, but we just need to make sure the resamplefd mechanism is
supported for both types of interrupts.  Whether or not that's a
decent mechanism seems orthogonal to me, but that's a discussion for
another day I think.

Thanks,
-Christoffer

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier  wrote:
> On Fri, 09 Mar 2018 21:36:12 +,
> Christoffer Dall wrote:
>>
>> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
>> > I'd be more confident if we did forbid P+A for such interrupts
>> > altogether, as they really feel like another kind of HW interrupt.
>>
>> How about a slightly bigger hammer:  Can we avoid doing P+A for level
>> interrupts completely?  I don't think that really makes much sense, and
>> I think we simply everything if we just come back out and resample the
>> line.  For an edge, something like a network card, there's a potential
>> performance win to appending a new pending state, but I doubt that this
>> is the case for level interrupts.
>
> I started implementing the same thing yesterday. Somehow, it feels
> slightly better to have the same flow for all level interrupts,
> including the timer, and we only use the MI on EOI as a way to trigger
> the next state of injection. Still testing, but looking good so far.
>
> I'm still puzzled that we have this level-but-not-quite behaviour for
> VFIO interrupts. At some point, it is going to bite us badly.
>

Where is the departure from level-triggered behavior with VFIO?  As
far as I can tell, the GIC flow of the interrupts will be just a level
interrupt, but we just need to make sure the resamplefd mechanism is
supported for both types of interrupts.  Whether or not that's a
decent mechanism seems orthogonal to me, but that's a discussion for
another day I think.

Thanks,
-Christoffer

Re: [RFC/RFT][PATCH v3 5/6] sched: idle: Select idle state before stopping the tick

[Frederic Weisbecker]

On Fri, Mar 09, 2018 at 10:46:55AM +0100, Rafael J. Wysocki wrote:
> --- linux-pm.orig/kernel/time/tick-sched.h
> +++ linux-pm/kernel/time/tick-sched.h
> @@ -30,6 +30,7 @@ enum tick_nohz_mode {
>   *   when the CPU returns from nohz sleep.
>   * @next_tick:   Next tick to be fired when in dynticks mode.
>   * @tick_stopped:Indicator that the idle tick has been stopped
> + * @tick_may_stop:   Indicator that the idle tick may be stopped shortly

Perhaps we can set timer_expires to 0 instead when we want to invalidate
the last value?

>   * @idle_jiffies:jiffies at the entry to idle for idle time accounting
>   * @idle_calls:  Total number of idle calls
>   * @idle_sleeps: Number of idle calls, where the sched tick was stopped
> @@ -38,7 +39,6 @@ enum tick_nohz_mode {
>   * @idle_exittime:   Time when the idle state was left
>   * @idle_sleeptime:  Sum of the time slept in idle with sched tick stopped
>   * @iowait_sleeptime:Sum of the time slept in idle with sched tick 
> stopped, with IO outstanding
> - * @sleep_length:Duration of the current idle sleep
>   * @do_timer_lst:CPU was the last one doing do_timer before going idle
>   */
>  struct tick_sched {
> @@ -49,6 +49,7 @@ struct tick_sched {
>   ktime_t next_tick;
>   int inidle;
>   int tick_stopped;
> + int tick_may_stop;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> @@ -58,8 +59,9 @@ struct tick_sched {
>   ktime_t idle_exittime;
>   ktime_t idle_sleeptime;
>   ktime_t iowait_sleeptime;
> - ktime_t sleep_length;
>   unsigned long   last_jiffies;
> + u64 timer_expires;
> + u64 timer_expires_basemono;

We may need documentation for the above fields too.

> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -652,13 +652,10 @@ static inline bool local_timer_softirq_p
>   return local_softirq_pending() & TIMER_SOFTIRQ;
>  }
>  
> -static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> -  ktime_t now, int cpu)
> +static ktime_t __tick_nohz_next_event(struct tick_sched *ts, int cpu)

Since we don't seem to have a lower level version, can we remove the 
underscores?

>  {
> - struct clock_event_device *dev = 
> __this_cpu_read(tick_cpu_device.evtdev);
>   u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
>   unsigned long seq, basejiff;
> - ktime_t tick;
>  
>   /* Read jiffies and the time when jiffies were updated last */
>   do {
[...]


> +static void __tick_nohz_stop_tick(struct tick_sched *ts, int cpu)

(Same comment here about the underscores).

> +{
> + struct clock_event_device *dev = 
> __this_cpu_read(tick_cpu_device.evtdev);
> + u64 basemono = ts->timer_expires_basemono;
> + u64 expires = ts->timer_expires;
> + ktime_t tick = expires;
> +
> + /* Make sure we won't be trying to stop it twice in a row. */
> + ts->tick_may_stop = 0;
> +
> + /*
> +  * If this CPU is the one which updates jiffies, then give up
> +  * the assignment and let it be taken by the CPU which runs
> +  * the tick timer next, which might be this CPU as well. If we
> +  * don't drop this here the jiffies might be stale and
> +  * do_timer() never invoked. Keep track of the fact that it
> +  * was the one which had the do_timer() duty last.
> +  */
> + if (cpu == tick_do_timer_cpu) {
> + tick_do_timer_cpu = TICK_DO_TIMER_NONE;
> + ts->do_timer_last = 1;
> + }
>  
>   /* Skip reprogram of event if its not changed */
>   if (ts->tick_stopped && (expires == ts->next_tick)) {
>   /* Sanity check: make sure clockevent is actually programmed */
>   if (tick == KTIME_MAX || ts->next_tick == 
> hrtimer_get_expires(>sched_timer))
> - goto out;
> + return;
>  
>   WARN_ON_ONCE(1);
>   printk_once("basemono: %llu ts->next_tick: %llu 
> dev->next_event: %llu timer->active: %d timer->expires: %llu\n",
[...]
> +void tick_nohz_idle_retain_tick(void)
> +{
> + __this_cpu_write(tick_cpu_sched.tick_may_stop, 0);
> +}
> +

So, I've become overly-paranoid about cached expiration on nohz code, we've run
into bugs that took months to debug before. It seems that the cached version 
shouldn't
leak in any way there, still can we have checks such as this in 
tick_nohz_idle_enter/exit()?

 

Re: [RFC/RFT][PATCH v3 5/6] sched: idle: Select idle state before stopping the tick

[Frederic Weisbecker]

On Fri, Mar 09, 2018 at 10:46:55AM +0100, Rafael J. Wysocki wrote:
> --- linux-pm.orig/kernel/time/tick-sched.h
> +++ linux-pm/kernel/time/tick-sched.h
> @@ -30,6 +30,7 @@ enum tick_nohz_mode {
>   *   when the CPU returns from nohz sleep.
>   * @next_tick:   Next tick to be fired when in dynticks mode.
>   * @tick_stopped:Indicator that the idle tick has been stopped
> + * @tick_may_stop:   Indicator that the idle tick may be stopped shortly

Perhaps we can set timer_expires to 0 instead when we want to invalidate
the last value?

>   * @idle_jiffies:jiffies at the entry to idle for idle time accounting
>   * @idle_calls:  Total number of idle calls
>   * @idle_sleeps: Number of idle calls, where the sched tick was stopped
> @@ -38,7 +39,6 @@ enum tick_nohz_mode {
>   * @idle_exittime:   Time when the idle state was left
>   * @idle_sleeptime:  Sum of the time slept in idle with sched tick stopped
>   * @iowait_sleeptime:Sum of the time slept in idle with sched tick 
> stopped, with IO outstanding
> - * @sleep_length:Duration of the current idle sleep
>   * @do_timer_lst:CPU was the last one doing do_timer before going idle
>   */
>  struct tick_sched {
> @@ -49,6 +49,7 @@ struct tick_sched {
>   ktime_t next_tick;
>   int inidle;
>   int tick_stopped;
> + int tick_may_stop;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> @@ -58,8 +59,9 @@ struct tick_sched {
>   ktime_t idle_exittime;
>   ktime_t idle_sleeptime;
>   ktime_t iowait_sleeptime;
> - ktime_t sleep_length;
>   unsigned long   last_jiffies;
> + u64 timer_expires;
> + u64 timer_expires_basemono;

We may need documentation for the above fields too.

> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -652,13 +652,10 @@ static inline bool local_timer_softirq_p
>   return local_softirq_pending() & TIMER_SOFTIRQ;
>  }
>  
> -static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> -  ktime_t now, int cpu)
> +static ktime_t __tick_nohz_next_event(struct tick_sched *ts, int cpu)

Since we don't seem to have a lower level version, can we remove the 
underscores?

>  {
> - struct clock_event_device *dev = 
> __this_cpu_read(tick_cpu_device.evtdev);
>   u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
>   unsigned long seq, basejiff;
> - ktime_t tick;
>  
>   /* Read jiffies and the time when jiffies were updated last */
>   do {
[...]


> +static void __tick_nohz_stop_tick(struct tick_sched *ts, int cpu)

(Same comment here about the underscores).

> +{
> + struct clock_event_device *dev = 
> __this_cpu_read(tick_cpu_device.evtdev);
> + u64 basemono = ts->timer_expires_basemono;
> + u64 expires = ts->timer_expires;
> + ktime_t tick = expires;
> +
> + /* Make sure we won't be trying to stop it twice in a row. */
> + ts->tick_may_stop = 0;
> +
> + /*
> +  * If this CPU is the one which updates jiffies, then give up
> +  * the assignment and let it be taken by the CPU which runs
> +  * the tick timer next, which might be this CPU as well. If we
> +  * don't drop this here the jiffies might be stale and
> +  * do_timer() never invoked. Keep track of the fact that it
> +  * was the one which had the do_timer() duty last.
> +  */
> + if (cpu == tick_do_timer_cpu) {
> + tick_do_timer_cpu = TICK_DO_TIMER_NONE;
> + ts->do_timer_last = 1;
> + }
>  
>   /* Skip reprogram of event if its not changed */
>   if (ts->tick_stopped && (expires == ts->next_tick)) {
>   /* Sanity check: make sure clockevent is actually programmed */
>   if (tick == KTIME_MAX || ts->next_tick == 
> hrtimer_get_expires(>sched_timer))
> - goto out;
> + return;
>  
>   WARN_ON_ONCE(1);
>   printk_once("basemono: %llu ts->next_tick: %llu 
> dev->next_event: %llu timer->active: %d timer->expires: %llu\n",
[...]
> +void tick_nohz_idle_retain_tick(void)
> +{
> + __this_cpu_write(tick_cpu_sched.tick_may_stop, 0);
> +}
> +

So, I've become overly-paranoid about cached expiration on nohz code, we've run
into bugs that took months to debug before. It seems that the cached version 
shouldn't
leak in any way there, still can we have checks such as this in 
tick_nohz_idle_enter/exit()?

 

htmldocs: include/net/cfg80211.h:4115: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 84ce5b987783d362ee4e737b653d6e2feacfa40c scripts: kernel-doc: improve 
nested logic to handle multiple identifiers
date:   3 months ago
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick 
(https://www.imagemagick.org)
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' 

htmldocs: include/net/cfg80211.h:4115: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 84ce5b987783d362ee4e737b653d6e2feacfa40c scripts: kernel-doc: improve 
nested logic to handle multiple identifiers
date:   3 months ago
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick 
(https://www.imagemagick.org)
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/crypto/hash.h:89: warning: duplicate section name 'Note'
   include/crypto/hash.h:95: warning: duplicate section name 'Note'
   include/crypto/hash.h:102: warning: duplicate section name 'Note'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.compress' not described in 'crypto_alg'
   include/linux/crypto.h:469: warning: Function parameter or member 
'cra_u.ablkcipher' 

Re: a Heisenbug tale

[Rasmus Villemoes]

On 2018-03-09 10:45, Ard Biesheuvel wrote:
> On 8 March 2018 at 23:19, Rasmus Villemoes  wrote:
>> On 2018-03-07 20:25, Leonard Crestez wrote:
>>> Hello,
>>>

>>
>> What we ended up doing was to explicitly set the mtime of every file in
>> the repo to the same reference time after the git checkout (find ... |
>> xargs touch --date=...). I also wanted to send a patch to change the
>> Makefile to use the filechk mechanism to avoid updating the .S_shipped
>> file when the script produced identical output, but never got around to it.
>>
> 
> I had no idea that _shipped files were causing issues like this, and
> AFAICT, this is not specific to this use case in arch/arm/crypto,
> right?
> 
> Russell, would you mind if we removed the _shipped.S file here and
> just assume that perl is available?
> 

Well, in that case I won't need to write a proper changelog for the
below, but this seems to work. It will of course still give the spurious
build failures when perl is not available and one hits the "files got
checked out at almost but not quite the same time", but it would have
prevented the spurious -dirty bug.


diff --git a/arch/arm/crypto/Makefile b/arch/arm/crypto/Makefile
index 30ef8e291271..f0cec9a92fd8 100644
--- a/arch/arm/crypto/Makefile
+++ b/arch/arm/crypto/Makefile
@@ -54,13 +54,14 @@ crct10dif-arm-ce-y  := crct10dif-ce-core.o
crct10dif-ce-glue.o
 crc32-arm-ce-y:= crc32-ce-core.o crc32-ce-glue.o
 chacha20-neon-y := chacha20-neon-core.o chacha20-neon-glue.o

-quiet_cmd_perl = PERL$@
-  cmd_perl = $(PERL) $(<) > $(@)
+define filechk_perl
+   perl $<
+endef

 $(src)/sha256-core.S_shipped: $(src)/sha256-armv4.pl
-   $(call cmd,perl)
+   $(call filechk,perl)

 $(src)/sha512-core.S_shipped: $(src)/sha512-armv4.pl
-   $(call cmd,perl)
+   $(call filechk,perl)

 .PRECIOUS: $(obj)/sha256-core.S $(obj)/sha512-core.S

Re: a Heisenbug tale

[Rasmus Villemoes]

On 2018-03-09 10:45, Ard Biesheuvel wrote:
> On 8 March 2018 at 23:19, Rasmus Villemoes  wrote:
>> On 2018-03-07 20:25, Leonard Crestez wrote:
>>> Hello,
>>>

>>
>> What we ended up doing was to explicitly set the mtime of every file in
>> the repo to the same reference time after the git checkout (find ... |
>> xargs touch --date=...). I also wanted to send a patch to change the
>> Makefile to use the filechk mechanism to avoid updating the .S_shipped
>> file when the script produced identical output, but never got around to it.
>>
> 
> I had no idea that _shipped files were causing issues like this, and
> AFAICT, this is not specific to this use case in arch/arm/crypto,
> right?
> 
> Russell, would you mind if we removed the _shipped.S file here and
> just assume that perl is available?
> 

Well, in that case I won't need to write a proper changelog for the
below, but this seems to work. It will of course still give the spurious
build failures when perl is not available and one hits the "files got
checked out at almost but not quite the same time", but it would have
prevented the spurious -dirty bug.


diff --git a/arch/arm/crypto/Makefile b/arch/arm/crypto/Makefile
index 30ef8e291271..f0cec9a92fd8 100644
--- a/arch/arm/crypto/Makefile
+++ b/arch/arm/crypto/Makefile
@@ -54,13 +54,14 @@ crct10dif-arm-ce-y  := crct10dif-ce-core.o
crct10dif-ce-glue.o
 crc32-arm-ce-y:= crc32-ce-core.o crc32-ce-glue.o
 chacha20-neon-y := chacha20-neon-core.o chacha20-neon-glue.o

-quiet_cmd_perl = PERL$@
-  cmd_perl = $(PERL) $(<) > $(@)
+define filechk_perl
+   perl $<
+endef

 $(src)/sha256-core.S_shipped: $(src)/sha256-armv4.pl
-   $(call cmd,perl)
+   $(call filechk,perl)

 $(src)/sha512-core.S_shipped: $(src)/sha512-armv4.pl
-   $(call cmd,perl)
+   $(call filechk,perl)

 .PRECIOUS: $(obj)/sha256-core.S $(obj)/sha512-core.S

[PATCH] media: ivtv: add parameter to enable ivtvfb on x86 PAT systems

[Nick French]

ivtvfb was previously disabled for x86 PAT-enabled systems
by commit 1bf1735b4780 ("x86/mm/pat, drivers/media/ivtv:
Use arch_phys_wc_add() and require PAT disabled") as a
workaround to abstract MTRR code away from device drivers.

The driver is not easily upgradable to the PAT-aware
ioremap_wc() API since the firmware hides the address
ranges that should be marked write-combined from the driver.
However, since a write-combined cache on the framebuffer
is only a performance enhancement not a requirement for
the framebuffer to function, completely disabling the driver
in this configuration is not necessary.

Add force_pat module parameter and a corresponding kernel
configuration parameter to optionally force initialization
on PAT-enabled x86 systems with a warning about the lack of
write-combined caching.

Signed-off-by: Nick French 
---
 drivers/media/pci/ivtv/Kconfig  | 19 ---
 drivers/media/pci/ivtv/ivtvfb.c | 19 +--
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index c72cbbd2d40c..9c63370ed721 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -70,8 +70,21 @@ config VIDEO_FB_IVTV
  This is used in the Hauppauge PVR-350 card. There is a driver
  homepage at .
 
- In order to use this module, you will need to boot with PAT disabled
- on x86 systems, using the nopat kernel parameter.
-
  To compile this driver as a module, choose M here: the
  module will be called ivtvfb.
+
+config VIDEO_FB_IVTV_FORCE_PAT
+   bool "force cx23415 frambuffer init with x86 PAT enabled"
+   depends on VIDEO_FB_IVTV && X86_PAT
+   default n
+   ---help---
+ With PAT enabled, the cx23415 framebuffer driver is not able to
+ utilize write-combined caching on the framebuffer memory.
+ The default behaviour is to disable the framebuffer completely
+ when it detects PAT is enabled in the kernel (i.e. not using
+ the nopat kernel parameter)
+
+ With this setting enabled, the framebuffer will initialize on
+ PAT-enabled systems but the framebuffer memory will be uncached.
+
+ If unsure, say N.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..5df74721aa19 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -55,6 +55,7 @@
 /* card parameters */
 static int ivtvfb_card_id = -1;
 static int ivtvfb_debug = 0;
+static bool ivtvfb_force_pat = IS_ENABLED(CONFIG_VIDEO_FB_IVTV_FORCE_PAT);
 static bool osd_laced;
 static int osd_depth;
 static int osd_upper;
@@ -64,6 +65,7 @@ static int osd_xres;
 
 module_param(ivtvfb_card_id, int, 0444);
 module_param_named(debug,ivtvfb_debug, int, 0644);
+module_param_named(force_pat, ivtvfb_force_pat, bool, 0644);
 module_param(osd_laced, bool, 0444);
 module_param(osd_depth, int, 0444);
 module_param(osd_upper, int, 0444);
@@ -79,6 +81,9 @@ MODULE_PARM_DESC(debug,
 "Debug level (bitmask). Default: errors only\n"
 "\t\t\t(debug = 3 gives full debugging)");
 
+MODULE_PARM_DESC(force_pat,
+"Force initialization on x86 PAT-enabled systems (bool).\n");
+
 /* Why upper, left, xres, yres, depth, laced ? To match terminology used
by fbset.
Why start at 1 for left & upper coordinate ? Because X doesn't allow 0 */
@@ -1169,8 +1174,18 @@ static int ivtvfb_init_card(struct ivtv *itv)
 
 #ifdef CONFIG_X86_64
if (pat_enabled()) {
-   pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel 
parameter\n");
-   return -ENODEV;
+   if (ivtvfb_force_pat) {
+   pr_info("PAT is enabled. Write-combined framebuffer "
+   "caching will be disabled. To enable caching, "
+   "boot with nopat kernel parameter\n");
+   } else {
+   pr_warn("ivtvfb needs PAT disabled for write-combined "
+   "framebuffer caching. Boot with nopat kernel "
+   "parameter to use caching, or use the "
+   "force_pat module parameter to run with "
+   "caching disabled\n");
+   return -ENODEV;
+   }
}
 #endif
 
-- 
2.13.6

[PATCH] media: ivtv: add parameter to enable ivtvfb on x86 PAT systems

[Nick French]

ivtvfb was previously disabled for x86 PAT-enabled systems
by commit 1bf1735b4780 ("x86/mm/pat, drivers/media/ivtv:
Use arch_phys_wc_add() and require PAT disabled") as a
workaround to abstract MTRR code away from device drivers.

The driver is not easily upgradable to the PAT-aware
ioremap_wc() API since the firmware hides the address
ranges that should be marked write-combined from the driver.
However, since a write-combined cache on the framebuffer
is only a performance enhancement not a requirement for
the framebuffer to function, completely disabling the driver
in this configuration is not necessary.

Add force_pat module parameter and a corresponding kernel
configuration parameter to optionally force initialization
on PAT-enabled x86 systems with a warning about the lack of
write-combined caching.

Signed-off-by: Nick French 
---
 drivers/media/pci/ivtv/Kconfig  | 19 ---
 drivers/media/pci/ivtv/ivtvfb.c | 19 +--
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index c72cbbd2d40c..9c63370ed721 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -70,8 +70,21 @@ config VIDEO_FB_IVTV
  This is used in the Hauppauge PVR-350 card. There is a driver
  homepage at .
 
- In order to use this module, you will need to boot with PAT disabled
- on x86 systems, using the nopat kernel parameter.
-
  To compile this driver as a module, choose M here: the
  module will be called ivtvfb.
+
+config VIDEO_FB_IVTV_FORCE_PAT
+   bool "force cx23415 frambuffer init with x86 PAT enabled"
+   depends on VIDEO_FB_IVTV && X86_PAT
+   default n
+   ---help---
+ With PAT enabled, the cx23415 framebuffer driver is not able to
+ utilize write-combined caching on the framebuffer memory.
+ The default behaviour is to disable the framebuffer completely
+ when it detects PAT is enabled in the kernel (i.e. not using
+ the nopat kernel parameter)
+
+ With this setting enabled, the framebuffer will initialize on
+ PAT-enabled systems but the framebuffer memory will be uncached.
+
+ If unsure, say N.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..5df74721aa19 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -55,6 +55,7 @@
 /* card parameters */
 static int ivtvfb_card_id = -1;
 static int ivtvfb_debug = 0;
+static bool ivtvfb_force_pat = IS_ENABLED(CONFIG_VIDEO_FB_IVTV_FORCE_PAT);
 static bool osd_laced;
 static int osd_depth;
 static int osd_upper;
@@ -64,6 +65,7 @@ static int osd_xres;
 
 module_param(ivtvfb_card_id, int, 0444);
 module_param_named(debug,ivtvfb_debug, int, 0644);
+module_param_named(force_pat, ivtvfb_force_pat, bool, 0644);
 module_param(osd_laced, bool, 0444);
 module_param(osd_depth, int, 0444);
 module_param(osd_upper, int, 0444);
@@ -79,6 +81,9 @@ MODULE_PARM_DESC(debug,
 "Debug level (bitmask). Default: errors only\n"
 "\t\t\t(debug = 3 gives full debugging)");
 
+MODULE_PARM_DESC(force_pat,
+"Force initialization on x86 PAT-enabled systems (bool).\n");
+
 /* Why upper, left, xres, yres, depth, laced ? To match terminology used
by fbset.
Why start at 1 for left & upper coordinate ? Because X doesn't allow 0 */
@@ -1169,8 +1174,18 @@ static int ivtvfb_init_card(struct ivtv *itv)
 
 #ifdef CONFIG_X86_64
if (pat_enabled()) {
-   pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel 
parameter\n");
-   return -ENODEV;
+   if (ivtvfb_force_pat) {
+   pr_info("PAT is enabled. Write-combined framebuffer "
+   "caching will be disabled. To enable caching, "
+   "boot with nopat kernel parameter\n");
+   } else {
+   pr_warn("ivtvfb needs PAT disabled for write-combined "
+   "framebuffer caching. Boot with nopat kernel "
+   "parameter to use caching, or use the "
+   "force_pat module parameter to run with "
+   "caching disabled\n");
+   return -ENODEV;
+   }
}
 #endif
 
-- 
2.13.6

[PATCH] platform/x86: dell_smbios: Resolve dependency error on ACPI_WMI

[Darren Hart]

Similarly to DCDBAS for DELL_SMBIOS_SMM, if DELL_SMBIOS_WMI is enabled,
DELL_SMBIOS becomes dependent on ACPI_WMI. Update the depends line to
prevent a configuration where DELL_SMBIOS=y and either backend
dependency =m. Update the comment accordingly.

Cc: Mario Limonciello 
Cc: Andy Shevchenko 
Signed-off-by: Darren Hart (VMware) 
---

Linus, just a heads up on this. Unlikely you would encounter this in
your builds, but just in case, I wanted you to know we (0-day) caught it
and are verifying the fix.

 drivers/platform/x86/Kconfig | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index e55b008..eef5eef 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -106,13 +106,13 @@ config ASUS_LAPTOP
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
 #
-# If the DELL_SMBIOS_SMM feature is enabled, the DELL_SMBIOS driver
-# becomes dependent on the DCDBAS driver. The "depends" line prevents a
-# configuration where DELL_SMBIOS=y while DCDBAS=m.
+# The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
+# backends are selected. The "depends" line prevents a configuration
+# where DELL_SMBIOS=y while either of those dependencies =m.
 #
 config DELL_SMBIOS
tristate "Dell SMBIOS driver"
-   depends on DCDBAS || DCDBAS=n
+   depends on (DCDBAS || DCDBAS=n) && (ACPI_WMI || ACPI_WMI=n)
---help---
This provides support for the Dell SMBIOS calling interface.
If you have a Dell computer you should enable this option.
-- 
2.9.3


-- 
Darren Hart
VMware Open Source Technology Center

[PATCH] platform/x86: dell_smbios: Resolve dependency error on ACPI_WMI

[Darren Hart]

Similarly to DCDBAS for DELL_SMBIOS_SMM, if DELL_SMBIOS_WMI is enabled,
DELL_SMBIOS becomes dependent on ACPI_WMI. Update the depends line to
prevent a configuration where DELL_SMBIOS=y and either backend
dependency =m. Update the comment accordingly.

Cc: Mario Limonciello 
Cc: Andy Shevchenko 
Signed-off-by: Darren Hart (VMware) 
---

Linus, just a heads up on this. Unlikely you would encounter this in
your builds, but just in case, I wanted you to know we (0-day) caught it
and are verifying the fix.

 drivers/platform/x86/Kconfig | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index e55b008..eef5eef 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -106,13 +106,13 @@ config ASUS_LAPTOP
  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
 #
-# If the DELL_SMBIOS_SMM feature is enabled, the DELL_SMBIOS driver
-# becomes dependent on the DCDBAS driver. The "depends" line prevents a
-# configuration where DELL_SMBIOS=y while DCDBAS=m.
+# The DELL_SMBIOS driver depends on ACPI_WMI and/or DCDBAS if those
+# backends are selected. The "depends" line prevents a configuration
+# where DELL_SMBIOS=y while either of those dependencies =m.
 #
 config DELL_SMBIOS
tristate "Dell SMBIOS driver"
-   depends on DCDBAS || DCDBAS=n
+   depends on (DCDBAS || DCDBAS=n) && (ACPI_WMI || ACPI_WMI=n)
---help---
This provides support for the Dell SMBIOS calling interface.
If you have a Dell computer you should enable this option.
-- 
2.9.3


-- 
Darren Hart
VMware Open Source Technology Center

Re: [RFC/RFT][PATCH v3 0/6] sched/cpuidle: Idle loop rework

[Rafael J. Wysocki]

On Saturday, March 10, 2018 5:07:36 PM CET Doug Smythies wrote:
> On 2018.03.10 01:00 Rafael J. Wysocki wrote:
> > On Saturday, March 10, 2018 8:41:39 AM CET Doug Smythies wrote:
> >> 
> >> With apologies to those that do not like the term "PowerNightmares",
> >
> > OK, and what exactly do you count as "PowerNightmares"?
> 
> I'll snip some below and then explain:
> 
> ...[snip]...
> 
> >> 
> >> Kernel 4.16-rc4: Summary: Average processor package power 27.41 watts
> >> 
> >> Idle State 0: Total Entries: 9096 : PowerNightmares: 6540 : Not PN time 
> >> (seconds): 0.051532 : PN time: 7886.309553 : Ratio:
> 153037.133492
> >> Idle State 1: Total Entries: 28731 : PowerNightmares: 215 : Not PN time 
> >> (seconds): 0.211999 : PN time: 77.395467 : Ratio:
> 365.074679
> >> Idle State 2: Total Entries: 4474 : PowerNightmares: 97 : Not PN time 
> >> (seconds): 1.959059 : PN time: 0.874112 : Ratio: 0.446190
> >> Idle State 3: Total Entries: 2319 : PowerNightmares: 0 : Not PN time 
> >> (seconds): 1.663376 : PN time: 0.00 : Ratio: 0.00
> 
> O.K. let's go deeper than the summary, and focus on idle state 0, which has 
> been my area of interest in this saga.
> 
> Idle State 0:
> CPU: 0: Entries: 2093 : PowerNightmares: 1136 : Not PN time (seconds): 
> 0.024840 : PN time: 1874.417840 : Ratio: 75459.655439
> CPU: 1: Entries: 1051 : PowerNightmares: 721 : Not PN time (seconds): 
> 0.004668 : PN time: 198.845193 : Ratio: 42597.513425
> CPU: 2: Entries: 759 : PowerNightmares: 583 : Not PN time (seconds): 0.003299 
> : PN time: 1099.069256 : Ratio: 333152.246028
> CPU: 3: Entries: 1033 : PowerNightmares: 1008 : Not PN time (seconds): 
> 0.000361 : PN time: 1930.340683 : Ratio: 5347203.995237
> CPU: 4: Entries: 1310 : PowerNightmares: 1025 : Not PN time (seconds): 
> 0.006214 : PN time: 1332.497114 : Ratio: 214434.682950
> CPU: 5: Entries: 1097 : PowerNightmares: 848 : Not PN time (seconds): 
> 0.005029 : PN time: 785.366864 : Ratio: 156167.601340
> CPU: 6: Entries: 1753 : PowerNightmares: 1219 : Not PN time (seconds): 
> 0.007121 : PN time: 665.772603 : Ratio: 93494.256958
> 
> Note: CPU 7 is busy and doesn't go into idle at all.
> 
> And also look at the histograms of the times spent in idle state 0:
> CPU 3 might be the most interesting:
> 
> Idle State: 0  CPU: 3:
> 4 1
> 5 3
> 7 2
> 11 1
> 12 1
> 13 2
> 14 3
> 15 3
> 17 4
> 18 1
> 19 2
> 28 2
> 7563 1
> 8012 1
>  1006
> 
> Where:
> Column 1 is the time in microseconds it was in that idle state
> up to  microseconds, which includes anything more.
> Column 2 is the number of occurrences of that time.
> 
> Notice that 1008 times out of the 1033, it spent an excessive amount
> of time in idle state 0, leading to excessive power consumption.
> I adopted Thomas Ilsche's "Powernightmare" term for this several
> months ago.
> 
> This CPU 3 example was pretty clear, but sometimes it is not so
> obvious. I admit that my thresholds for is it a "powernigthmare" or
> not are somewhat arbitrary, and I'll change them to whatever anybody
> wants. Currently:
> 
> #define THRESHOLD_0 100   /* Idle state 0 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_1 1000  /* Idle state 1 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_2 2000  /* Idle state 2 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_3 4000  /* Idle state 3 PowerNightmare threshold in 
> microseconds */

That's clear, thanks!

Well, the main reason why I have a problem with the "powernigthmare" term is
because it is somewhat arbitrary overall.  After all, you ended up having to
explain what you meant in detail even though you have used it in the
previous message, so it doesn't appear all that useful to me. :-)

Also, the current work isn't even concerned about idle times below the
length of the tick period, so the information that some CPUs spent over
100 us in state 0 for a certain number of times during the test is not that
relevant here.  The information that they often spend more time than a tick
period in state 0 in one go *is* relevant, though.

The $subject patch series, on the one hand, is about adding a safety net for
possible governor mispredictions using the existing tick infrastructure,
along with avoiding unnecessary timer manipulation overhead related to the
stopping and starting of the tick, on the other hand.  Of course, the safety
net will not improve the accuracy of governor predictions anyway, it may only
reduce their impact.

That said, it doesn't catch one case which turns out to be quite significant
and which is when the tick is stopped already and the governor predicts short
idle.  That, again, may cause the CPU to spend a long time in a shallow idle
state which then will qualify as a "powernightmare" I suppose.  If I'm reading
your data correctly, that is the main reason for the majority of cases in which
CPUs spend  us and more in state 0 on your system.

That issue can be dealt with in a couple of ways and the 

Re: [RFC/RFT][PATCH v3 0/6] sched/cpuidle: Idle loop rework

[Rafael J. Wysocki]

On Saturday, March 10, 2018 5:07:36 PM CET Doug Smythies wrote:
> On 2018.03.10 01:00 Rafael J. Wysocki wrote:
> > On Saturday, March 10, 2018 8:41:39 AM CET Doug Smythies wrote:
> >> 
> >> With apologies to those that do not like the term "PowerNightmares",
> >
> > OK, and what exactly do you count as "PowerNightmares"?
> 
> I'll snip some below and then explain:
> 
> ...[snip]...
> 
> >> 
> >> Kernel 4.16-rc4: Summary: Average processor package power 27.41 watts
> >> 
> >> Idle State 0: Total Entries: 9096 : PowerNightmares: 6540 : Not PN time 
> >> (seconds): 0.051532 : PN time: 7886.309553 : Ratio:
> 153037.133492
> >> Idle State 1: Total Entries: 28731 : PowerNightmares: 215 : Not PN time 
> >> (seconds): 0.211999 : PN time: 77.395467 : Ratio:
> 365.074679
> >> Idle State 2: Total Entries: 4474 : PowerNightmares: 97 : Not PN time 
> >> (seconds): 1.959059 : PN time: 0.874112 : Ratio: 0.446190
> >> Idle State 3: Total Entries: 2319 : PowerNightmares: 0 : Not PN time 
> >> (seconds): 1.663376 : PN time: 0.00 : Ratio: 0.00
> 
> O.K. let's go deeper than the summary, and focus on idle state 0, which has 
> been my area of interest in this saga.
> 
> Idle State 0:
> CPU: 0: Entries: 2093 : PowerNightmares: 1136 : Not PN time (seconds): 
> 0.024840 : PN time: 1874.417840 : Ratio: 75459.655439
> CPU: 1: Entries: 1051 : PowerNightmares: 721 : Not PN time (seconds): 
> 0.004668 : PN time: 198.845193 : Ratio: 42597.513425
> CPU: 2: Entries: 759 : PowerNightmares: 583 : Not PN time (seconds): 0.003299 
> : PN time: 1099.069256 : Ratio: 333152.246028
> CPU: 3: Entries: 1033 : PowerNightmares: 1008 : Not PN time (seconds): 
> 0.000361 : PN time: 1930.340683 : Ratio: 5347203.995237
> CPU: 4: Entries: 1310 : PowerNightmares: 1025 : Not PN time (seconds): 
> 0.006214 : PN time: 1332.497114 : Ratio: 214434.682950
> CPU: 5: Entries: 1097 : PowerNightmares: 848 : Not PN time (seconds): 
> 0.005029 : PN time: 785.366864 : Ratio: 156167.601340
> CPU: 6: Entries: 1753 : PowerNightmares: 1219 : Not PN time (seconds): 
> 0.007121 : PN time: 665.772603 : Ratio: 93494.256958
> 
> Note: CPU 7 is busy and doesn't go into idle at all.
> 
> And also look at the histograms of the times spent in idle state 0:
> CPU 3 might be the most interesting:
> 
> Idle State: 0  CPU: 3:
> 4 1
> 5 3
> 7 2
> 11 1
> 12 1
> 13 2
> 14 3
> 15 3
> 17 4
> 18 1
> 19 2
> 28 2
> 7563 1
> 8012 1
>  1006
> 
> Where:
> Column 1 is the time in microseconds it was in that idle state
> up to  microseconds, which includes anything more.
> Column 2 is the number of occurrences of that time.
> 
> Notice that 1008 times out of the 1033, it spent an excessive amount
> of time in idle state 0, leading to excessive power consumption.
> I adopted Thomas Ilsche's "Powernightmare" term for this several
> months ago.
> 
> This CPU 3 example was pretty clear, but sometimes it is not so
> obvious. I admit that my thresholds for is it a "powernigthmare" or
> not are somewhat arbitrary, and I'll change them to whatever anybody
> wants. Currently:
> 
> #define THRESHOLD_0 100   /* Idle state 0 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_1 1000  /* Idle state 1 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_2 2000  /* Idle state 2 PowerNightmare threshold in 
> microseconds */
> #define THRESHOLD_3 4000  /* Idle state 3 PowerNightmare threshold in 
> microseconds */

That's clear, thanks!

Well, the main reason why I have a problem with the "powernigthmare" term is
because it is somewhat arbitrary overall.  After all, you ended up having to
explain what you meant in detail even though you have used it in the
previous message, so it doesn't appear all that useful to me. :-)

Also, the current work isn't even concerned about idle times below the
length of the tick period, so the information that some CPUs spent over
100 us in state 0 for a certain number of times during the test is not that
relevant here.  The information that they often spend more time than a tick
period in state 0 in one go *is* relevant, though.

The $subject patch series, on the one hand, is about adding a safety net for
possible governor mispredictions using the existing tick infrastructure,
along with avoiding unnecessary timer manipulation overhead related to the
stopping and starting of the tick, on the other hand.  Of course, the safety
net will not improve the accuracy of governor predictions anyway, it may only
reduce their impact.

That said, it doesn't catch one case which turns out to be quite significant
and which is when the tick is stopped already and the governor predicts short
idle.  That, again, may cause the CPU to spend a long time in a shallow idle
state which then will qualify as a "powernightmare" I suppose.  If I'm reading
your data correctly, that is the main reason for the majority of cases in which
CPUs spend  us and more in state 0 on your system.

That issue can be dealt with in a couple of ways and the 

drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 'stb0899_attach' being applied to a already existing, static definition

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 6cdeaed3b1420bd2569891be0c4123ff59628e9e media: dvb_usb_pctv452e: 
module refcount changes were unbalanced
date:   3 months ago
config: x86_64-randconfig-a0-03110606 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 6cdeaed3b1420bd2569891be0c4123ff59628e9e
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from drivers/media/usb/dvb-usb/pctv452e.c:20:0:
   drivers/media/usb/dvb-usb/pctv452e.c: In function 'pctv452e_frontend_attach':
>> drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 
>> 'stb0899_attach' being applied to a already existing, static definition
static inline struct dvb_frontend *stb0899_attach(struct stb0899_config 
*config,
   ^~

vim +/stb0899_attach +151 drivers/media/dvb-frontends/stb0899_drv.h

ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  150 
 
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08 @151 
 static inline struct dvb_frontend *stb0899_attach(struct stb0899_config 
*config,
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  152 
  struct i2c_adapter *i2c)
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  153 
 {
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  154 
printk(KERN_WARNING "%s: Driver disabled by Kconfig\n", __func__);
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  155 
return NULL;
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  156 
 }
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  157 
 

:: The code at line 151 was first introduced by commit
:: ae9902da96b4d2d82707706c7fbc93a8e501dde8 V4L/DVB (9417): DVB_ATTACH for 
STB0899, STB6100, TDA8261

:: TO: Manu Abraham 
:: CC: Mauro Carvalho Chehab 

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


.config.gz
Description: application/gzip

drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 'stb0899_attach' being applied to a already existing, static definition

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 6cdeaed3b1420bd2569891be0c4123ff59628e9e media: dvb_usb_pctv452e: 
module refcount changes were unbalanced
date:   3 months ago
config: x86_64-randconfig-a0-03110606 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 6cdeaed3b1420bd2569891be0c4123ff59628e9e
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from drivers/media/usb/dvb-usb/pctv452e.c:20:0:
   drivers/media/usb/dvb-usb/pctv452e.c: In function 'pctv452e_frontend_attach':
>> drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 
>> 'stb0899_attach' being applied to a already existing, static definition
static inline struct dvb_frontend *stb0899_attach(struct stb0899_config 
*config,
   ^~

vim +/stb0899_attach +151 drivers/media/dvb-frontends/stb0899_drv.h

ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  150 
 
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08 @151 
 static inline struct dvb_frontend *stb0899_attach(struct stb0899_config 
*config,
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  152 
  struct i2c_adapter *i2c)
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  153 
 {
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  154 
printk(KERN_WARNING "%s: Driver disabled by Kconfig\n", __func__);
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  155 
return NULL;
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  156 
 }
ae9902da drivers/media/dvb/frontends/stb0899_drv.h Manu Abraham 2007-10-08  157 
 

:: The code at line 151 was first introduced by commit
:: ae9902da96b4d2d82707706c7fbc93a8e501dde8 V4L/DVB (9417): DVB_ATTACH for 
STB0899, STB6100, TDA8261

:: TO: Manu Abraham 
:: CC: Mauro Carvalho Chehab 

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


.config.gz
Description: application/gzip

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Daniel Micay]

> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
> not really going to show a lot of variation. This array will always have the
> same size on the stack.

The issue is that unlike in C++, a `static const` can't be used in a
constant expression in C. It's unclear why C is defined that way but
it's how it is and there isn't currently a GCC extension making more
things into constant expressions like C++.

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Daniel Micay]

> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
> not really going to show a lot of variation. This array will always have the
> same size on the stack.

The issue is that unlike in C++, a `static const` can't be used in a
constant expression in C. It's unclear why C is defined that way but
it's how it is and there isn't currently a GCC extension making more
things into constant expressions like C++.

Re: Simplifying our RCU models

[Andrea Parri]

On Sat, Mar 10, 2018 at 02:47:26PM -0800, Paul E. McKenney wrote:
> On Sat, Mar 10, 2018 at 05:29:46PM +0100, Andrea Parri wrote:
> > On Sat, Mar 10, 2018 at 08:04:09AM -0800, Paul E. McKenney wrote:
> > > On Fri, Mar 09, 2018 at 10:55:20AM +0100, Andrea Parri wrote:
> > > > On Thu, Mar 08, 2018 at 04:51:45PM -0800, Paul E. McKenney wrote:
> > > > > [ Dropping CC ]
> > > > 
> > > > [...]
> > > > 
> > > > > > > Ah, and any thoughts on how best to get feedback from the various 
> > > > > > > people
> > > > > > > who would need to reprogram their fingers?  Or is everyone 
> > > > > > > already on
> > > > > > > board with changing these various names?
> > > > > > 
> > > > > > Experienced should get there in a week (gcc help); newbies would 
> > > > > > (have to)
> > > > > > rely on either on _properly updated_ documentation or weeks/months 
> > > > > > of code
> > > > > > paging; scripts do the renaming.  What am I missing?
> > > > > 
> > > > > Linus's reply to my email?  ;-)
> > > > > 
> > > > > More seriously, people who use RCU only occasionally would likely
> > > > > have more difficulty adjusting.  "What the heck is the new name of
> > > > > synchronize_rcu()???  Oh forget it, I will just use a lock.  My system
> > > > > isn't all that large anyway!!!"
> > > > 
> > > > I did miss this group of people.  Thanks,
> > > 
> > > I should hasten to add that we have changed the names of RCU-related APIs
> > > before, including synchronize_kernel() -> synchronize_sched() back in
> > > the day and SLAB_DESTROY_BY_RCU -> SLAB_TYPESAFE_BY_RCU more recently.
> > > There was some discussion around this last change, and one of the things
> > > we did to help was to add big comments relating the old and new names.
> > > That way, someone grepping for the old name can easily find the new name.
> > > 
> > > But it does cause some churn.  So name changes can be a good thing,
> > > but we don't undertake them lightly.  That said, it has been more than
> > > a decades since the last name change in the core RCU API, so it is not
> > > too early to consider it.  As Linus says, however, we won't be changing
> > > just for change's sake.  ;-)
> > 
> > Absolutely!
> > 
> > And thank you for these remarks (you know, certainly, I was not properly
> > "watching" RCU commits a decade ago or so... ;).  But maybe other people
> > can find these remarks interesting: please feel free to forward to LKML.
> 
> I should probasbly also add that the name change from SLAB_DESTROY_BY_RCU to
> SLAB_TYPESAFE_BY_RCU was motivated by several groups misinterpreting the
> old name, and thus spending months each chasing weird race conditions...

[ Bringing back CC ]

Thanks,
  Andrea


> 
>   Thanx, Paul
> 

Re: Simplifying our RCU models

[Andrea Parri]

On Sat, Mar 10, 2018 at 02:47:26PM -0800, Paul E. McKenney wrote:
> On Sat, Mar 10, 2018 at 05:29:46PM +0100, Andrea Parri wrote:
> > On Sat, Mar 10, 2018 at 08:04:09AM -0800, Paul E. McKenney wrote:
> > > On Fri, Mar 09, 2018 at 10:55:20AM +0100, Andrea Parri wrote:
> > > > On Thu, Mar 08, 2018 at 04:51:45PM -0800, Paul E. McKenney wrote:
> > > > > [ Dropping CC ]
> > > > 
> > > > [...]
> > > > 
> > > > > > > Ah, and any thoughts on how best to get feedback from the various 
> > > > > > > people
> > > > > > > who would need to reprogram their fingers?  Or is everyone 
> > > > > > > already on
> > > > > > > board with changing these various names?
> > > > > > 
> > > > > > Experienced should get there in a week (gcc help); newbies would 
> > > > > > (have to)
> > > > > > rely on either on _properly updated_ documentation or weeks/months 
> > > > > > of code
> > > > > > paging; scripts do the renaming.  What am I missing?
> > > > > 
> > > > > Linus's reply to my email?  ;-)
> > > > > 
> > > > > More seriously, people who use RCU only occasionally would likely
> > > > > have more difficulty adjusting.  "What the heck is the new name of
> > > > > synchronize_rcu()???  Oh forget it, I will just use a lock.  My system
> > > > > isn't all that large anyway!!!"
> > > > 
> > > > I did miss this group of people.  Thanks,
> > > 
> > > I should hasten to add that we have changed the names of RCU-related APIs
> > > before, including synchronize_kernel() -> synchronize_sched() back in
> > > the day and SLAB_DESTROY_BY_RCU -> SLAB_TYPESAFE_BY_RCU more recently.
> > > There was some discussion around this last change, and one of the things
> > > we did to help was to add big comments relating the old and new names.
> > > That way, someone grepping for the old name can easily find the new name.
> > > 
> > > But it does cause some churn.  So name changes can be a good thing,
> > > but we don't undertake them lightly.  That said, it has been more than
> > > a decades since the last name change in the core RCU API, so it is not
> > > too early to consider it.  As Linus says, however, we won't be changing
> > > just for change's sake.  ;-)
> > 
> > Absolutely!
> > 
> > And thank you for these remarks (you know, certainly, I was not properly
> > "watching" RCU commits a decade ago or so... ;).  But maybe other people
> > can find these remarks interesting: please feel free to forward to LKML.
> 
> I should probasbly also add that the name change from SLAB_DESTROY_BY_RCU to
> SLAB_TYPESAFE_BY_RCU was motivated by several groups misinterpreting the
> old name, and thus spending months each chasing weird race conditions...

[ Bringing back CC ]

Thanks,
  Andrea


> 
>   Thanx, Paul
> 

kernel/jump_label.c:377:51: warning: cast to pointer from integer of different size

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: dc1dd184c2f0016bec35c0d7a48c057e0ad763d3 jump_label: Warn on failed 
jump_label patching attempt
date:   2 weeks ago
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout dc1dd184c2f0016bec35c0d7a48c057e0ad763d3
# save the attached .config to linux build tree
make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   In file included from arch/sparc/include/asm/bug.h:21:0,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from arch/sparc/include/asm/current.h:15,
from include/linux/mutex.h:14,
from include/linux/kernfs.h:13,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/device.h:16,
from include/linux/node.h:18,
from include/linux/memory.h:19,
from kernel/jump_label.c:8:
   kernel/jump_label.c: In function '__jump_label_update':
>> kernel/jump_label.c:377:51: warning: cast to pointer from integer of 
>> different size [-Wint-to-pointer-cast]
WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
  ^
   include/asm-generic/bug.h:91:69: note: in definition of macro '__WARN_printf'
#define __WARN_printf(arg...) warn_slowpath_fmt(__FILE__, __LINE__, arg)
^~~
>> include/asm-generic/bug.h:153:3: note: in expansion of macro 'WARN'
  WARN(1, format);\
  ^~~~
>> kernel/jump_label.c:377:5: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
^

vim +377 kernel/jump_label.c

   363  
   364  static void __jump_label_update(struct static_key *key,
   365  struct jump_entry *entry,
   366  struct jump_entry *stop)
   367  {
   368  for (; (entry < stop) && (jump_entry_key(entry) == key); 
entry++) {
   369  /*
   370   * An entry->code of 0 indicates an entry which has been
   371   * disabled because it was in an init text area.
   372   */
   373  if (entry->code) {
   374  if (kernel_text_address(entry->code))
   375  arch_jump_label_transform(entry, 
jump_label_type(entry));
   376  else
 > 377  WARN_ONCE(1, "can't patch jump_label at 
 > %pS", (void *)entry->code);
   378  }
   379  }
   380  }
   381  

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


.config.gz
Description: application/gzip

kernel/jump_label.c:377:51: warning: cast to pointer from integer of different size

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: dc1dd184c2f0016bec35c0d7a48c057e0ad763d3 jump_label: Warn on failed 
jump_label patching attempt
date:   2 weeks ago
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout dc1dd184c2f0016bec35c0d7a48c057e0ad763d3
# save the attached .config to linux build tree
make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   In file included from arch/sparc/include/asm/bug.h:21:0,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from arch/sparc/include/asm/current.h:15,
from include/linux/mutex.h:14,
from include/linux/kernfs.h:13,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/device.h:16,
from include/linux/node.h:18,
from include/linux/memory.h:19,
from kernel/jump_label.c:8:
   kernel/jump_label.c: In function '__jump_label_update':
>> kernel/jump_label.c:377:51: warning: cast to pointer from integer of 
>> different size [-Wint-to-pointer-cast]
WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
  ^
   include/asm-generic/bug.h:91:69: note: in definition of macro '__WARN_printf'
#define __WARN_printf(arg...) warn_slowpath_fmt(__FILE__, __LINE__, arg)
^~~
>> include/asm-generic/bug.h:153:3: note: in expansion of macro 'WARN'
  WARN(1, format);\
  ^~~~
>> kernel/jump_label.c:377:5: note: in expansion of macro 'WARN_ONCE'
WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
^

vim +377 kernel/jump_label.c

   363  
   364  static void __jump_label_update(struct static_key *key,
   365  struct jump_entry *entry,
   366  struct jump_entry *stop)
   367  {
   368  for (; (entry < stop) && (jump_entry_key(entry) == key); 
entry++) {
   369  /*
   370   * An entry->code of 0 indicates an entry which has been
   371   * disabled because it was in an init text area.
   372   */
   373  if (entry->code) {
   374  if (kernel_text_address(entry->code))
   375  arch_jump_label_transform(entry, 
jump_label_type(entry));
   376  else
 > 377  WARN_ONCE(1, "can't patch jump_label at 
 > %pS", (void *)entry->code);
   378  }
   379  }
   380  }
   381  

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


.config.gz
Description: application/gzip

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Gustavo A. R. Silva]

On 03/10/2018 05:12 PM, Kees Cook wrote:

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 wrote:

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:


The kernel would like to have all stack VLA usage removed.



I think there was a remark made earlier to give more explanation here. It
should explain why we want "VLA on stack" removed.


Signed-off-by: Andreas Christoforou 
---
   drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

   /* we need at least 3 deltas / 4 samples for a reliable chirp detection
*/
   #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);



What about this instead?

#define FFT_NUM_SAMPLES (NUM_DIFFS + 1)


   /* Threshold for difference of delta peaks */
   static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc,
u8 *data,
  int datalen, bool is_ctl, bool is_ext)
   {
 int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];



Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
not really going to show a lot of variation. This array will always have the
same size on the stack.


The problem is that it's not a "constant expression", so the compiler
frontend still yells about it under -Wvla. I would characterize this
mainly as a fix for "accidental VLA" or "misdetected VLA" or something
like that. AIUI, there really isn't a functional change here.

-Kees



--
Gustavo

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Gustavo A. R. Silva]

On 03/10/2018 05:12 PM, Kees Cook wrote:

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 wrote:

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:


The kernel would like to have all stack VLA usage removed.



I think there was a remark made earlier to give more explanation here. It
should explain why we want "VLA on stack" removed.


Signed-off-by: Andreas Christoforou 
---
   drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

   /* we need at least 3 deltas / 4 samples for a reliable chirp detection
*/
   #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);



What about this instead?

#define FFT_NUM_SAMPLES (NUM_DIFFS + 1)


   /* Threshold for difference of delta peaks */
   static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc,
u8 *data,
  int datalen, bool is_ctl, bool is_ext)
   {
 int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];



Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
not really going to show a lot of variation. This array will always have the
same size on the stack.


The problem is that it's not a "constant expression", so the compiler
frontend still yells about it under -Wvla. I would characterize this
mainly as a fix for "accidental VLA" or "misdetected VLA" or something
like that. AIUI, there really isn't a functional change here.

-Kees



--
Gustavo

mm/percpu.c:2723:2: warning: #warning "the CRIS architecture has physical and virtual addresses confused"

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: abee210500ed15a22787009d9210b9a34911afcc percpu: hack to let the CRIS 
architecture to boot until they clean up
date:   3 months ago
config: cris-etrax-100lx_v2_defconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout abee210500ed15a22787009d9210b9a34911afcc
# save the attached .config to linux build tree
make.cross ARCH=cris 

All warnings (new ones prefixed by >>):

   mm/percpu.c: In function 'setup_per_cpu_areas':
>> mm/percpu.c:2723:2: warning: #warning "the CRIS architecture has physical 
>> and virtual addresses confused" [-Wcpp]
#warning "the CRIS architecture has physical and virtual addresses confused"
 ^~~

vim +2723 mm/percpu.c

  2688  
  2689  /*
  2690   * UP percpu area setup.
  2691   *
  2692   * UP always uses km-based percpu allocator with identity mapping.
  2693   * Static percpu variables are indistinguishable from the usual static
  2694   * variables and don't require any special preparation.
  2695   */
  2696  void __init setup_per_cpu_areas(void)
  2697  {
  2698  const size_t unit_size =
  2699  roundup_pow_of_two(max_t(size_t, PCPU_MIN_UNIT_SIZE,
  2700   PERCPU_DYNAMIC_RESERVE));
  2701  struct pcpu_alloc_info *ai;
  2702  void *fc;
  2703  
  2704  ai = pcpu_alloc_alloc_info(1, 1);
  2705  fc = memblock_virt_alloc_from_nopanic(unit_size,
  2706PAGE_SIZE,
  2707__pa(MAX_DMA_ADDRESS));
  2708  if (!ai || !fc)
  2709  panic("Failed to allocate memory for percpu areas.");
  2710  /* kmemleak tracks the percpu allocations separately */
  2711  kmemleak_free(fc);
  2712  
  2713  ai->dyn_size = unit_size;
  2714  ai->unit_size = unit_size;
  2715  ai->atom_size = unit_size;
  2716  ai->alloc_size = unit_size;
  2717  ai->groups[0].nr_units = 1;
  2718  ai->groups[0].cpu_map[0] = 0;
  2719  
  2720  if (pcpu_setup_first_chunk(ai, fc) < 0)
  2721  panic("Failed to initialize percpu areas.");
  2722  #ifdef CONFIG_CRIS
> 2723  #warning "the CRIS architecture has physical and virtual addresses 
> confused"
  2724  #else
  2725  pcpu_free_alloc_info(ai);
  2726  #endif
  2727  }
  2728  

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


.config.gz
Description: application/gzip

mm/percpu.c:2723:2: warning: #warning "the CRIS architecture has physical and virtual addresses confused"

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: abee210500ed15a22787009d9210b9a34911afcc percpu: hack to let the CRIS 
architecture to boot until they clean up
date:   3 months ago
config: cris-etrax-100lx_v2_defconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout abee210500ed15a22787009d9210b9a34911afcc
# save the attached .config to linux build tree
make.cross ARCH=cris 

All warnings (new ones prefixed by >>):

   mm/percpu.c: In function 'setup_per_cpu_areas':
>> mm/percpu.c:2723:2: warning: #warning "the CRIS architecture has physical 
>> and virtual addresses confused" [-Wcpp]
#warning "the CRIS architecture has physical and virtual addresses confused"
 ^~~

vim +2723 mm/percpu.c

  2688  
  2689  /*
  2690   * UP percpu area setup.
  2691   *
  2692   * UP always uses km-based percpu allocator with identity mapping.
  2693   * Static percpu variables are indistinguishable from the usual static
  2694   * variables and don't require any special preparation.
  2695   */
  2696  void __init setup_per_cpu_areas(void)
  2697  {
  2698  const size_t unit_size =
  2699  roundup_pow_of_two(max_t(size_t, PCPU_MIN_UNIT_SIZE,
  2700   PERCPU_DYNAMIC_RESERVE));
  2701  struct pcpu_alloc_info *ai;
  2702  void *fc;
  2703  
  2704  ai = pcpu_alloc_alloc_info(1, 1);
  2705  fc = memblock_virt_alloc_from_nopanic(unit_size,
  2706PAGE_SIZE,
  2707__pa(MAX_DMA_ADDRESS));
  2708  if (!ai || !fc)
  2709  panic("Failed to allocate memory for percpu areas.");
  2710  /* kmemleak tracks the percpu allocations separately */
  2711  kmemleak_free(fc);
  2712  
  2713  ai->dyn_size = unit_size;
  2714  ai->unit_size = unit_size;
  2715  ai->atom_size = unit_size;
  2716  ai->alloc_size = unit_size;
  2717  ai->groups[0].nr_units = 1;
  2718  ai->groups[0].cpu_map[0] = 0;
  2719  
  2720  if (pcpu_setup_first_chunk(ai, fc) < 0)
  2721  panic("Failed to initialize percpu areas.");
  2722  #ifdef CONFIG_CRIS
> 2723  #warning "the CRIS architecture has physical and virtual addresses 
> confused"
  2724  #else
  2725  pcpu_free_alloc_info(ai);
  2726  #endif
  2727  }
  2728  

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


.config.gz
Description: application/gzip

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Kees Cook]

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 wrote:
> On 3/9/2018 1:30 PM, Andreas Christoforou wrote:
>>
>> The kernel would like to have all stack VLA usage removed.
>
>
> I think there was a remark made earlier to give more explanation here. It
> should explain why we want "VLA on stack" removed.
>
>> Signed-off-by: Andreas Christoforou 
>> ---
>>   drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c
>> b/drivers/net/wireless/ath/ath9k/dfs.c
>> index 6fee9a4..cfb0f84 100644
>> --- a/drivers/net/wireless/ath/ath9k/dfs.c
>> +++ b/drivers/net/wireless/ath/ath9k/dfs.c
>> @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;
>>
>>   /* we need at least 3 deltas / 4 samples for a reliable chirp detection
>> */
>>   #define NUM_DIFFS 3
>> -static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);
>>
>>   /* Threshold for difference of delta peaks */
>>   static const int MAX_DIFF = 2;
>> @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc,
>> u8 *data,
>>  int datalen, bool is_ctl, bool is_ext)
>>   {
>> int i;
>> -   int max_bin[FFT_NUM_SAMPLES];
>> +   int max_bin[NUM_DIFFS + 1];
>
>
> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
> not really going to show a lot of variation. This array will always have the
> same size on the stack.

The problem is that it's not a "constant expression", so the compiler
frontend still yells about it under -Wvla. I would characterize this
mainly as a fix for "accidental VLA" or "misdetected VLA" or something
like that. AIUI, there really isn't a functional change here.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Kees Cook]

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 wrote:
> On 3/9/2018 1:30 PM, Andreas Christoforou wrote:
>>
>> The kernel would like to have all stack VLA usage removed.
>
>
> I think there was a remark made earlier to give more explanation here. It
> should explain why we want "VLA on stack" removed.
>
>> Signed-off-by: Andreas Christoforou 
>> ---
>>   drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c
>> b/drivers/net/wireless/ath/ath9k/dfs.c
>> index 6fee9a4..cfb0f84 100644
>> --- a/drivers/net/wireless/ath/ath9k/dfs.c
>> +++ b/drivers/net/wireless/ath/ath9k/dfs.c
>> @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;
>>
>>   /* we need at least 3 deltas / 4 samples for a reliable chirp detection
>> */
>>   #define NUM_DIFFS 3
>> -static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);
>>
>>   /* Threshold for difference of delta peaks */
>>   static const int MAX_DIFF = 2;
>> @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc,
>> u8 *data,
>>  int datalen, bool is_ctl, bool is_ext)
>>   {
>> int i;
>> -   int max_bin[FFT_NUM_SAMPLES];
>> +   int max_bin[NUM_DIFFS + 1];
>
>
> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so
> not really going to show a lot of variation. This array will always have the
> same size on the stack.

The problem is that it's not a "constant expression", so the compiler
frontend still yells about it under -Wvla. I would characterize this
mainly as a fix for "accidental VLA" or "misdetected VLA" or something
like that. AIUI, there really isn't a functional change here.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Arend van Spriel]

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:

The kernel would like to have all stack VLA usage removed.


I think there was a remark made earlier to give more explanation here. 
It should explain why we want "VLA on stack" removed.



Signed-off-by: Andreas Christoforou 
---
  drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c 
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

  /* we need at least 3 deltas / 4 samples for a reliable chirp detection */
  #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);

  /* Threshold for difference of delta peaks */
  static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 
*data,
 int datalen, bool is_ctl, bool is_ext)
  {
int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];


Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const 
so not really going to show a lot of variation. This array will always 
have the same size on the stack.


Regards,
Arend

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Arend van Spriel]

On 3/9/2018 1:30 PM, Andreas Christoforou wrote:

The kernel would like to have all stack VLA usage removed.


I think there was a remark made earlier to give more explanation here. 
It should explain why we want "VLA on stack" removed.



Signed-off-by: Andreas Christoforou 
---
  drivers/net/wireless/ath/ath9k/dfs.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/dfs.c 
b/drivers/net/wireless/ath/ath9k/dfs.c
index 6fee9a4..cfb0f84 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX= 10;

  /* we need at least 3 deltas / 4 samples for a reliable chirp detection */
  #define NUM_DIFFS 3
-static const int FFT_NUM_SAMPLES   = (NUM_DIFFS + 1);

  /* Threshold for difference of delta peaks */
  static const int MAX_DIFF = 2;
@@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 
*data,
 int datalen, bool is_ctl, bool is_ext)
  {
int i;
-   int max_bin[FFT_NUM_SAMPLES];
+   int max_bin[NUM_DIFFS + 1];


Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const 
so not really going to show a lot of variation. This array will always 
have the same size on the stack.


Regards,
Arend

htmldocs: include/linux/crypto.h:469: warning: Function parameter or member 'cra_u.ablkcipher' not described in 'crypto_alg'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 151c468b44a89a9f3173ab8575690014b7249893 scripts: kernel-doc: print the 
declaration name on warnings
date:   3 months ago
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in 

htmldocs: include/linux/crypto.h:469: warning: Function parameter or member 'cra_u.ablkcipher' not described in 'crypto_alg'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   3266b5bd97eaa72793df0b6e5a106c69ccc166c4
commit: 151c468b44a89a9f3173ab8575690014b7249893 scripts: kernel-doc: print the 
declaration name on warnings
date:   3 months ago
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1036.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in regex; marked by <-- HERE in 
m/(struct|union)([^{};]+){ <-- HERE ([^{}]*)}([^{}\;]*)\;/ at 
scripts/kernel-doc line 1058.
   Unescaped left brace in regex is deprecated here (and will be fatal in Perl 
5.30), passed through in 

[PATCH] rslib: Remove VLAs by setting upper bound on nroots

[Kees Cook]

Avoid stack VLAs[1] by always allocating the upper bound of stack space
needed. The existing users of rslib appear to max out at 24 roots[2],
so use that as the upper bound until we have a reason to change it.

Alternative considered: make init_rs() a true caller-instance and
pre-allocate the workspaces. This would possibly need locking and
a refactoring of the returned structure.

Using kmalloc in this path doesn't look great, especially since at
least one caller (pstore) is sensitive to allocations during rslib
usage (it expects to run it during an Oops, for example).

[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/9/838

Signed-off-by: Kees Cook 
---
 lib/reed_solomon/decode_rs.c| 7 ---
 lib/reed_solomon/reed_solomon.c | 5 -
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/reed_solomon/decode_rs.c b/lib/reed_solomon/decode_rs.c
index 0ec3f257ffdf..3e3becb836a6 100644
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -31,9 +31,10 @@
 * of nroots is 8. So the necessary stack size will be about
 * 220 bytes max.
 */
-   uint16_t lambda[nroots + 1], syn[nroots];
-   uint16_t b[nroots + 1], t[nroots + 1], omega[nroots + 1];
-   uint16_t root[nroots], reg[nroots + 1], loc[nroots];
+   uint16_t lambda[RS_MAX_ROOTS + 1], syn[RS_MAX_ROOTS];
+   uint16_t b[RS_MAX_ROOTS + 1], t[RS_MAX_ROOTS + 1];
+   uint16_t omega[RS_MAX_ROOTS + 1], root[RS_MAX_ROOTS];
+   uint16_t reg[RS_MAX_ROOTS + 1], loc[RS_MAX_ROOTS];
int count = 0;
uint16_t msk = (uint16_t) rs->nn;
 
diff --git a/lib/reed_solomon/reed_solomon.c b/lib/reed_solomon/reed_solomon.c
index 06d04cfa9339..3e218e70ac2e 100644
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -51,6 +51,9 @@ static LIST_HEAD (rslist);
 /* Protection for the list */
 static DEFINE_MUTEX(rslistlock);
 
+/* Ultimately controls the upper bounds of the on-stack buffers. */
+#define RS_MAX_ROOTS   24
+
 /**
  * rs_init - Initialize a Reed-Solomon codec
  * @symsize:   symbol size, bits (1-8)
@@ -210,7 +213,7 @@ static struct rs_control *init_rs_internal(int symsize, int 
gfpoly,
return NULL;
if (prim <= 0 || prim >= (1<= (1<= (1< RS_MAX_ROOTS)
return NULL;
 
mutex_lock();
-- 
2.7.4


-- 
Kees Cook
Pixel Security

[PATCH] rslib: Remove VLAs by setting upper bound on nroots

[Kees Cook]

Avoid stack VLAs[1] by always allocating the upper bound of stack space
needed. The existing users of rslib appear to max out at 24 roots[2],
so use that as the upper bound until we have a reason to change it.

Alternative considered: make init_rs() a true caller-instance and
pre-allocate the workspaces. This would possibly need locking and
a refactoring of the returned structure.

Using kmalloc in this path doesn't look great, especially since at
least one caller (pstore) is sensitive to allocations during rslib
usage (it expects to run it during an Oops, for example).

[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/9/838

Signed-off-by: Kees Cook 
---
 lib/reed_solomon/decode_rs.c| 7 ---
 lib/reed_solomon/reed_solomon.c | 5 -
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/reed_solomon/decode_rs.c b/lib/reed_solomon/decode_rs.c
index 0ec3f257ffdf..3e3becb836a6 100644
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -31,9 +31,10 @@
 * of nroots is 8. So the necessary stack size will be about
 * 220 bytes max.
 */
-   uint16_t lambda[nroots + 1], syn[nroots];
-   uint16_t b[nroots + 1], t[nroots + 1], omega[nroots + 1];
-   uint16_t root[nroots], reg[nroots + 1], loc[nroots];
+   uint16_t lambda[RS_MAX_ROOTS + 1], syn[RS_MAX_ROOTS];
+   uint16_t b[RS_MAX_ROOTS + 1], t[RS_MAX_ROOTS + 1];
+   uint16_t omega[RS_MAX_ROOTS + 1], root[RS_MAX_ROOTS];
+   uint16_t reg[RS_MAX_ROOTS + 1], loc[RS_MAX_ROOTS];
int count = 0;
uint16_t msk = (uint16_t) rs->nn;
 
diff --git a/lib/reed_solomon/reed_solomon.c b/lib/reed_solomon/reed_solomon.c
index 06d04cfa9339..3e218e70ac2e 100644
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -51,6 +51,9 @@ static LIST_HEAD (rslist);
 /* Protection for the list */
 static DEFINE_MUTEX(rslistlock);
 
+/* Ultimately controls the upper bounds of the on-stack buffers. */
+#define RS_MAX_ROOTS   24
+
 /**
  * rs_init - Initialize a Reed-Solomon codec
  * @symsize:   symbol size, bits (1-8)
@@ -210,7 +213,7 @@ static struct rs_control *init_rs_internal(int symsize, int 
gfpoly,
return NULL;
if (prim <= 0 || prim >= (1<= (1<= (1< RS_MAX_ROOTS)
return NULL;
 
mutex_lock();
-- 
2.7.4


-- 
Kees Cook
Pixel Security

[PATCH v2 2/2] iio: chemical: sgp30: Support Sensirion SGPxx sensors

[Andreas Brauchli]

Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

Supported Features:

* Indoor Air Quality (IAQ) concentrations for
  - tVOC (in_concentration_voc_input)
  - CO2eq (in_concentration_co2_input) - SGP30 only
  IAQ concentrations are periodically read out by a background thread
  to allow the sensor to maintain its internal baseline.
* Baseline support for IAQ (in_iaq_baseline, set_iaq_baseline)
* Gas concentration signals
  - Ethanol (in_concentration_ethanol_raw)
  - H2 (in_concentration_h2_raw) - SGP30 only
* On-chip self test (in_selftest)
* Sensor interface version (in_feature_set_version)
* Sensor serial number (in_serial_id)
* Humidity compensation
  With the help of an external humidity signal, the gas signals can be
  humidity-compensated. The sensor performs the compensation on-chip.
* Power mode switching between low power mode (1/2Hz) and
  ultra low power mode (1/30Hz) sampling - SGPC3 only
* Checksummed I2C communication

For all features, refer to the data sheet or the documentation in
Documentation/iio/chemical/sgp30.txt for more details.

The ABI is documented in
Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30

Signed-off-by: Andreas Brauchli 
---
 .../ABI/testing/sysfs-bus-iio-chemical-sgp30   |   62 ++
 .../bindings/iio/chemical/sensirion,sgp30.txt  |   15 +
 Documentation/iio/chemical/sgp30.txt   |   97 ++
 drivers/iio/chemical/Kconfig   |   13 +
 drivers/iio/chemical/Makefile  |1 +
 drivers/iio/chemical/sgp30.c   | 1120 
 6 files changed, 1308 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
 create mode 100644 
Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
 create mode 100644 Documentation/iio/chemical/sgp30.txt
 create mode 100644 drivers/iio/chemical/sgp30.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30 
b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
new file mode 100644
index ..3b97c0bd5878
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
@@ -0,0 +1,62 @@
+what:  /sys/bus/iio/devices/iio:devicex/in_featureset_version
+date:  March 2018
+kernelversion: 4.17
+contact:   andreas brauchli 
+description:
+   Retrieve the sensor interface version. The sensor-interface 
should
+   remain forward-compatible across minor versions.
+
+what:  /sys/bus/iio/devices/iio:devicex/in_iaq_baseline
+date:  March 2018
+kernelversion: 4.17
+contact:   andreas brauchli 
+description:
+   Retrieve the on-chip iaq baseline state. the baseline is an 
internal
+   state that should not be modified.
+
+What:  /sys/bus/iio/devices/iio:deviceX/set_iaq_baseline
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Restore the on-chip IAQ baseline state to a previous state. 
Useful for
+   fast-resuming after a restart. A baseline is valid for one week 
when the
+   sensor is not operated.
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_selftest
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Run the on-chip self test. Output values:
+   * OK
+   * FAILED
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_serial_id
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Retrieve the unique sensor serial number. May be useful to 
associate
+   with the IAQ baseline when the baseline is stored remotely or 
the sensor
+   is on a plug-and-play device.
+
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Set the absolute humidity in milligrams per cubic meter 
(mg/m^3) to
+   enable on-sensor humidity compensation. Affects all gas signals 
and IAQ
+   values. Accepted values:
+   * 0 (disables humidity compensation)
+   * 1..256000 (mg/m^3)
+
+What:  /sys/bus/iio/devices/iio:deviceX/set_power_mode
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Change the power mode on an SGPC3 with ultra-low power mode 
support.
+   Accepted values:
+   * "low" (default/startup mode)
+   * "ultra-low" (default/startup mode)
diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt 

[PATCH v2 2/2] iio: chemical: sgp30: Support Sensirion SGPxx sensors

[Andreas Brauchli]

Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

Supported Features:

* Indoor Air Quality (IAQ) concentrations for
  - tVOC (in_concentration_voc_input)
  - CO2eq (in_concentration_co2_input) - SGP30 only
  IAQ concentrations are periodically read out by a background thread
  to allow the sensor to maintain its internal baseline.
* Baseline support for IAQ (in_iaq_baseline, set_iaq_baseline)
* Gas concentration signals
  - Ethanol (in_concentration_ethanol_raw)
  - H2 (in_concentration_h2_raw) - SGP30 only
* On-chip self test (in_selftest)
* Sensor interface version (in_feature_set_version)
* Sensor serial number (in_serial_id)
* Humidity compensation
  With the help of an external humidity signal, the gas signals can be
  humidity-compensated. The sensor performs the compensation on-chip.
* Power mode switching between low power mode (1/2Hz) and
  ultra low power mode (1/30Hz) sampling - SGPC3 only
* Checksummed I2C communication

For all features, refer to the data sheet or the documentation in
Documentation/iio/chemical/sgp30.txt for more details.

The ABI is documented in
Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30

Signed-off-by: Andreas Brauchli 
---
 .../ABI/testing/sysfs-bus-iio-chemical-sgp30   |   62 ++
 .../bindings/iio/chemical/sensirion,sgp30.txt  |   15 +
 Documentation/iio/chemical/sgp30.txt   |   97 ++
 drivers/iio/chemical/Kconfig   |   13 +
 drivers/iio/chemical/Makefile  |1 +
 drivers/iio/chemical/sgp30.c   | 1120 
 6 files changed, 1308 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
 create mode 100644 
Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
 create mode 100644 Documentation/iio/chemical/sgp30.txt
 create mode 100644 drivers/iio/chemical/sgp30.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30 
b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
new file mode 100644
index ..3b97c0bd5878
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
@@ -0,0 +1,62 @@
+what:  /sys/bus/iio/devices/iio:devicex/in_featureset_version
+date:  March 2018
+kernelversion: 4.17
+contact:   andreas brauchli 
+description:
+   Retrieve the sensor interface version. The sensor-interface 
should
+   remain forward-compatible across minor versions.
+
+what:  /sys/bus/iio/devices/iio:devicex/in_iaq_baseline
+date:  March 2018
+kernelversion: 4.17
+contact:   andreas brauchli 
+description:
+   Retrieve the on-chip iaq baseline state. the baseline is an 
internal
+   state that should not be modified.
+
+What:  /sys/bus/iio/devices/iio:deviceX/set_iaq_baseline
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Restore the on-chip IAQ baseline state to a previous state. 
Useful for
+   fast-resuming after a restart. A baseline is valid for one week 
when the
+   sensor is not operated.
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_selftest
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Run the on-chip self test. Output values:
+   * OK
+   * FAILED
+
+What:  /sys/bus/iio/devices/iio:deviceX/in_serial_id
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Retrieve the unique sensor serial number. May be useful to 
associate
+   with the IAQ baseline when the baseline is stored remotely or 
the sensor
+   is on a plug-and-play device.
+
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Set the absolute humidity in milligrams per cubic meter 
(mg/m^3) to
+   enable on-sensor humidity compensation. Affects all gas signals 
and IAQ
+   values. Accepted values:
+   * 0 (disables humidity compensation)
+   * 1..256000 (mg/m^3)
+
+What:  /sys/bus/iio/devices/iio:deviceX/set_power_mode
+Date:  March 2018
+KernelVersion: 4.17
+Contact:   Andreas Brauchli 
+Description:
+   Change the power mode on an SGPC3 with ultra-low power mode 
support.
+   Accepted values:
+   * "low" (default/startup mode)
+   * "ultra-low" (default/startup mode)
diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt 
b/Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
new file mode 100644
index ..bb909b0f1aba
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
@@ -0,0 +1,15 @@
+* Sensirion SGPxx multi-pixel Gas Sensor
+
+Required properties:
+
+  - compatible: must be one 

[PATCH v2 0/2] iio: chemical: sgp30: Support SGP30 / SGPC3 Gas Sensors

[Andreas Brauchli]

This patch series adds support for Sensirion SGP30 and SGPC3 I2C gas
sensors.

Further product specs available from:
https://www.sensirion.com/en/environmental-sensors/gas-sensors/multi-pixel-gas-sensors/
https://www.sensirion.com/en/about-us/links/#c15360

Patch 1/2 add new IIO modifiers used by the driver
Patch 2/2 add sgp30/sgpc3 driver and documentation

Major changes to v1:
* Driver renamed to sgp30
* No or minimal sensor knowledge required to use the device:
  automatic fixed interval polling (see next point) and initialization.
* Background thread for IAQ polling
* Remove triggered-buffers support
* Add new IIO modifiers for ethanol and H2
* Support for SGPC3 product increment ("feature set 0.6") including 
  humidity compensation and an ultra-low power mode. Unfortunately, the
  datasheet for this new SGPC3 has not been released yet.
* ABI/DT Documentation

Many thanks to Peter Meerwald-Standler and Jonathan Cameron for their most 
valuable feedback on v1.

Andreas Brauchli (2):
  iio: Add modifiers for ethanol and H2 gases
  iio: chemical: sgp30: Support Sensirion SGPxx sensors

 Documentation/ABI/testing/sysfs-bus-iio|4 +
 .../ABI/testing/sysfs-bus-iio-chemical-sgp30   |   62 ++
 .../bindings/iio/chemical/sensirion,sgp30.txt  |   15 +
 Documentation/iio/chemical/sgp30.txt   |   97 ++
 drivers/iio/chemical/Kconfig   |   13 +
 drivers/iio/chemical/Makefile  |1 +
 drivers/iio/chemical/sgp30.c   | 1120 
 include/uapi/linux/iio/types.h |2 +
 tools/iio/iio_event_monitor.c  |4 +
 9 files changed, 1318 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
 create mode 100644 
Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
 create mode 100644 Documentation/iio/chemical/sgp30.txt
 create mode 100644 drivers/iio/chemical/sgp30.c

-- 
2.15.1

[PATCH v2 1/2] iio: Add modifiers for ethanol and H2 gases

[Andreas Brauchli]

Add ethanol and H2 gas modifiers:
* IIO_MOD_ETHANOL
* IIO_MOD_H2

Signed-off-by: Andreas Brauchli 
---
 Documentation/ABI/testing/sysfs-bus-iio | 4 
 include/uapi/linux/iio/types.h  | 2 ++
 tools/iio/iio_event_monitor.c   | 4 
 3 files changed, 10 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio 
b/Documentation/ABI/testing/sysfs-bus-iio
index 6a5f34b4d5b9..6856fe1c05e5 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1530,6 +1530,10 @@ What:
/sys/bus/iio/devices/iio:deviceX/in_concentration_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_co2_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_co2_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_ethanol_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_ethanol_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_h2_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_h2_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_voc_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_voc_raw
 KernelVersion: 4.3
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf88e3c..587233143e1e 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -82,6 +82,8 @@ enum iio_modifier {
IIO_MOD_I,
IIO_MOD_Q,
IIO_MOD_CO2,
+   IIO_MOD_ETHANOL,
+   IIO_MOD_H2,
IIO_MOD_VOC,
IIO_MOD_LIGHT_UV,
 };
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e1181d..a49fe0d181f7 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -111,6 +111,8 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_I] = "i",
[IIO_MOD_Q] = "q",
[IIO_MOD_CO2] = "co2",
+   [IIO_MOD_ETHANOL] = "ethanol",
+   [IIO_MOD_H2] = "h2",
[IIO_MOD_VOC] = "voc",
 };
 
@@ -193,6 +195,8 @@ static bool event_is_known(struct iio_event_data *event)
case IIO_MOD_I:
case IIO_MOD_Q:
case IIO_MOD_CO2:
+   case IIO_MOD_ETHANOL:
+   case IIO_MOD_H2:
case IIO_MOD_VOC:
break;
default:
-- 
2.15.1

[PATCH v2 0/2] iio: chemical: sgp30: Support SGP30 / SGPC3 Gas Sensors

[Andreas Brauchli]

This patch series adds support for Sensirion SGP30 and SGPC3 I2C gas
sensors.

Further product specs available from:
https://www.sensirion.com/en/environmental-sensors/gas-sensors/multi-pixel-gas-sensors/
https://www.sensirion.com/en/about-us/links/#c15360

Patch 1/2 add new IIO modifiers used by the driver
Patch 2/2 add sgp30/sgpc3 driver and documentation

Major changes to v1:
* Driver renamed to sgp30
* No or minimal sensor knowledge required to use the device:
  automatic fixed interval polling (see next point) and initialization.
* Background thread for IAQ polling
* Remove triggered-buffers support
* Add new IIO modifiers for ethanol and H2
* Support for SGPC3 product increment ("feature set 0.6") including 
  humidity compensation and an ultra-low power mode. Unfortunately, the
  datasheet for this new SGPC3 has not been released yet.
* ABI/DT Documentation

Many thanks to Peter Meerwald-Standler and Jonathan Cameron for their most 
valuable feedback on v1.

Andreas Brauchli (2):
  iio: Add modifiers for ethanol and H2 gases
  iio: chemical: sgp30: Support Sensirion SGPxx sensors

 Documentation/ABI/testing/sysfs-bus-iio|4 +
 .../ABI/testing/sysfs-bus-iio-chemical-sgp30   |   62 ++
 .../bindings/iio/chemical/sensirion,sgp30.txt  |   15 +
 Documentation/iio/chemical/sgp30.txt   |   97 ++
 drivers/iio/chemical/Kconfig   |   13 +
 drivers/iio/chemical/Makefile  |1 +
 drivers/iio/chemical/sgp30.c   | 1120 
 include/uapi/linux/iio/types.h |2 +
 tools/iio/iio_event_monitor.c  |4 +
 9 files changed, 1318 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
 create mode 100644 
Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
 create mode 100644 Documentation/iio/chemical/sgp30.txt
 create mode 100644 drivers/iio/chemical/sgp30.c

-- 
2.15.1

[PATCH v2 1/2] iio: Add modifiers for ethanol and H2 gases

[Andreas Brauchli]

Add ethanol and H2 gas modifiers:
* IIO_MOD_ETHANOL
* IIO_MOD_H2

Signed-off-by: Andreas Brauchli 
---
 Documentation/ABI/testing/sysfs-bus-iio | 4 
 include/uapi/linux/iio/types.h  | 2 ++
 tools/iio/iio_event_monitor.c   | 4 
 3 files changed, 10 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio 
b/Documentation/ABI/testing/sysfs-bus-iio
index 6a5f34b4d5b9..6856fe1c05e5 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1530,6 +1530,10 @@ What:
/sys/bus/iio/devices/iio:deviceX/in_concentration_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_co2_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_co2_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_ethanol_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_ethanol_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_h2_raw
+What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_h2_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentration_voc_raw
 What:  /sys/bus/iio/devices/iio:deviceX/in_concentrationX_voc_raw
 KernelVersion: 4.3
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf88e3c..587233143e1e 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -82,6 +82,8 @@ enum iio_modifier {
IIO_MOD_I,
IIO_MOD_Q,
IIO_MOD_CO2,
+   IIO_MOD_ETHANOL,
+   IIO_MOD_H2,
IIO_MOD_VOC,
IIO_MOD_LIGHT_UV,
 };
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e1181d..a49fe0d181f7 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -111,6 +111,8 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_I] = "i",
[IIO_MOD_Q] = "q",
[IIO_MOD_CO2] = "co2",
+   [IIO_MOD_ETHANOL] = "ethanol",
+   [IIO_MOD_H2] = "h2",
[IIO_MOD_VOC] = "voc",
 };
 
@@ -193,6 +195,8 @@ static bool event_is_known(struct iio_event_data *event)
case IIO_MOD_I:
case IIO_MOD_Q:
case IIO_MOD_CO2:
+   case IIO_MOD_ETHANOL:
+   case IIO_MOD_H2:
case IIO_MOD_VOC:
break;
default:
-- 
2.15.1

Re: [PATCH 2/2] iio: chemical: sgpxx: triggered buffer support

[Andreas Brauchli]

On Sat, 2017-11-25 at 17:48 +, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 17:11:29 +0100
> Andreas Brauchli  wrote:
> 
> > Support triggered buffer for use with e.g. hrtimer for automated
> > polling to ensure that the sensor's internal baseline is correctly
> > updated independently of the use-case.
> 
> Given the really strict timing requirements for this device and slow
> read rates, I wouldn't involve a triggered buffer at all, but
> actually
> do it with a thread / timer within the initial driver.   The
> sysfs interface is more than adequate for a 1Hz device so using
> the buffered option is just adding unnecessary complexity...

Thanks for the suggestions, I went with that in v2.

> 
> I reviewed the code anyway.  Key thing is though the file would be
> small, there should be a separate .c file for the buffered support
> if you are going to make it optional. That way we don't get ifdefs
> in the c file, but rather stubs provided in the header.

FWIW, I learned a few things

> You could decide to add stubs to include/linux/iio/triggered_buffer.h
> /buffer.h
> 
> and then use __maybe_unusued to mark your trigger function.
> The compiler would drop it anyway and this would suppress build
> warnings.  
> 
> However, I don't think this device wants to be supported via the
> buffered interfaces at all.  More smartness needed in the core
> driver to maintain the timing etc.
> 
> Jonathan
> > 
> > Triggered buffer support is only enabled when IIO_BUFFER is set.
> > 
> > Signed-off-by: Andreas Brauchli 
> > ---
> >  drivers/iio/chemical/Kconfig |  3 +++
> >  drivers/iio/chemical/sgpxx.c | 38
> > ++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iio/chemical/Kconfig
> > b/drivers/iio/chemical/Kconfig
> > index 4574dd687513..6710fbfc6451 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -42,12 +42,15 @@ config SENSIRION_SGPXX
> > tristate "Sensirion SGPxx gas sensors"
> > depends on I2C
> > select CRC8
> > +   select IIO_TRIGGERED_BUFFER if (IIO_BUFFER)
> > help
> >   Say Y here to build I2C interface support for the
> > following
> >   Sensirion SGP gas sensors:
> > * SGP30 gas sensor
> > * SGPC3 gas sensor
> >  
> > + Also select IIO_BUFFER to enable triggered buffers.
> > +
> >   To compile this driver as module, choose M here: the
> >   module will be called sgpxx.
> >  
> > diff --git a/drivers/iio/chemical/sgpxx.c
> > b/drivers/iio/chemical/sgpxx.c
> > index aea55e41d4cc..025206448f73 100644
> > --- a/drivers/iio/chemical/sgpxx.c
> > +++ b/drivers/iio/chemical/sgpxx.c
> > @@ -27,6 +27,10 @@
> >  #include 
> >  #include 
> >  #include 
> > +#ifdef CONFIG_IIO_BUFFER
> > +#include 
> > +#include 
> > +#endif /* CONFIG_IIO_BUFFER */
> >  #include 
> >  
> >  #define SGP_WORD_LEN   2
> > @@ -789,6 +793,26 @@ static const struct of_device_id sgp_dt_ids[]
> > = {
> > { }
> >  };
> >  
> > +#ifdef CONFIG_IIO_BUFFER
> 
> All this ifdef fun is rather messy.
> Split it out to a separate file like most other drivers with
> optional buffer support. 
> 
> General rule (more or less) of kernel drivers is that
> any optional ifdef stuff should be in headers to provide stubs
> when code isn't available, not down in the drivers making them
> harder to read.
> 
> > +static irqreturn_t sgp_trigger_handler(int irq, void *p)
> > +{
> > +   struct iio_poll_func *pf = p;
> > +   struct iio_dev *indio_dev = pf->indio_dev;
> > +   struct sgp_data *data = iio_priv(indio_dev);
> > +   int ret;
> > +
> > +   ret = sgp_get_measurement(data, data->measure_iaq_cmd,
> > + SGP_MEASURE_MODE_IAQ);
> > +   if (!ret)
> > +   iio_push_to_buffers_with_timestamp(indio_dev,
> > +  
> > >buffer.start,
> > +  pf->timestamp);
> > +
> > +   iio_trigger_notify_done(indio_dev->trig);
> > +   return IRQ_HANDLED;
> > +}
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> >  static int sgp_probe(struct i2c_client *client,
> >  const struct i2c_device_id *id)
> >  {
> > @@ -846,6 +870,17 @@ static int sgp_probe(struct i2c_client
> > *client,
> > indio_dev->channels = chip->channels;
> > indio_dev->num_channels = chip->num_channels;
> >  
> > +#ifdef CONFIG_IIO_BUFFER
> > +   ret = iio_triggered_buffer_setup(indio_dev,
> > +iio_pollfunc_store_time,
> > +sgp_trigger_handler,
> > +NULL);
> > +   if (ret) {
> > +   dev_err(>dev, "failed to setup iio
> > triggered buffer\n");
> > +   goto fail_free;
> > +   }
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> > ret = devm_iio_device_register(>dev, indio_dev);
> > if (!ret)
> > return ret;
> > @@ 

Re: [PATCH 2/2] iio: chemical: sgpxx: triggered buffer support

[Andreas Brauchli]

On Sat, 2017-11-25 at 17:48 +, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 17:11:29 +0100
> Andreas Brauchli  wrote:
> 
> > Support triggered buffer for use with e.g. hrtimer for automated
> > polling to ensure that the sensor's internal baseline is correctly
> > updated independently of the use-case.
> 
> Given the really strict timing requirements for this device and slow
> read rates, I wouldn't involve a triggered buffer at all, but
> actually
> do it with a thread / timer within the initial driver.   The
> sysfs interface is more than adequate for a 1Hz device so using
> the buffered option is just adding unnecessary complexity...

Thanks for the suggestions, I went with that in v2.

> 
> I reviewed the code anyway.  Key thing is though the file would be
> small, there should be a separate .c file for the buffered support
> if you are going to make it optional. That way we don't get ifdefs
> in the c file, but rather stubs provided in the header.

FWIW, I learned a few things

> You could decide to add stubs to include/linux/iio/triggered_buffer.h
> /buffer.h
> 
> and then use __maybe_unusued to mark your trigger function.
> The compiler would drop it anyway and this would suppress build
> warnings.  
> 
> However, I don't think this device wants to be supported via the
> buffered interfaces at all.  More smartness needed in the core
> driver to maintain the timing etc.
> 
> Jonathan
> > 
> > Triggered buffer support is only enabled when IIO_BUFFER is set.
> > 
> > Signed-off-by: Andreas Brauchli 
> > ---
> >  drivers/iio/chemical/Kconfig |  3 +++
> >  drivers/iio/chemical/sgpxx.c | 38
> > ++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iio/chemical/Kconfig
> > b/drivers/iio/chemical/Kconfig
> > index 4574dd687513..6710fbfc6451 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -42,12 +42,15 @@ config SENSIRION_SGPXX
> > tristate "Sensirion SGPxx gas sensors"
> > depends on I2C
> > select CRC8
> > +   select IIO_TRIGGERED_BUFFER if (IIO_BUFFER)
> > help
> >   Say Y here to build I2C interface support for the
> > following
> >   Sensirion SGP gas sensors:
> > * SGP30 gas sensor
> > * SGPC3 gas sensor
> >  
> > + Also select IIO_BUFFER to enable triggered buffers.
> > +
> >   To compile this driver as module, choose M here: the
> >   module will be called sgpxx.
> >  
> > diff --git a/drivers/iio/chemical/sgpxx.c
> > b/drivers/iio/chemical/sgpxx.c
> > index aea55e41d4cc..025206448f73 100644
> > --- a/drivers/iio/chemical/sgpxx.c
> > +++ b/drivers/iio/chemical/sgpxx.c
> > @@ -27,6 +27,10 @@
> >  #include 
> >  #include 
> >  #include 
> > +#ifdef CONFIG_IIO_BUFFER
> > +#include 
> > +#include 
> > +#endif /* CONFIG_IIO_BUFFER */
> >  #include 
> >  
> >  #define SGP_WORD_LEN   2
> > @@ -789,6 +793,26 @@ static const struct of_device_id sgp_dt_ids[]
> > = {
> > { }
> >  };
> >  
> > +#ifdef CONFIG_IIO_BUFFER
> 
> All this ifdef fun is rather messy.
> Split it out to a separate file like most other drivers with
> optional buffer support. 
> 
> General rule (more or less) of kernel drivers is that
> any optional ifdef stuff should be in headers to provide stubs
> when code isn't available, not down in the drivers making them
> harder to read.
> 
> > +static irqreturn_t sgp_trigger_handler(int irq, void *p)
> > +{
> > +   struct iio_poll_func *pf = p;
> > +   struct iio_dev *indio_dev = pf->indio_dev;
> > +   struct sgp_data *data = iio_priv(indio_dev);
> > +   int ret;
> > +
> > +   ret = sgp_get_measurement(data, data->measure_iaq_cmd,
> > + SGP_MEASURE_MODE_IAQ);
> > +   if (!ret)
> > +   iio_push_to_buffers_with_timestamp(indio_dev,
> > +  
> > >buffer.start,
> > +  pf->timestamp);
> > +
> > +   iio_trigger_notify_done(indio_dev->trig);
> > +   return IRQ_HANDLED;
> > +}
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> >  static int sgp_probe(struct i2c_client *client,
> >  const struct i2c_device_id *id)
> >  {
> > @@ -846,6 +870,17 @@ static int sgp_probe(struct i2c_client
> > *client,
> > indio_dev->channels = chip->channels;
> > indio_dev->num_channels = chip->num_channels;
> >  
> > +#ifdef CONFIG_IIO_BUFFER
> > +   ret = iio_triggered_buffer_setup(indio_dev,
> > +iio_pollfunc_store_time,
> > +sgp_trigger_handler,
> > +NULL);
> > +   if (ret) {
> > +   dev_err(>dev, "failed to setup iio
> > triggered buffer\n");
> > +   goto fail_free;
> > +   }
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> > ret = devm_iio_device_register(>dev, indio_dev);
> > if (!ret)
> > return ret;
> > @@ -863,6 +898,9 @@ static int sgp_remove(struct i2c_client
> > 

Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

[Andreas Brauchli]

Dear Peter, Jonathan,

Thanks for the thourough and speedy review and apologies for the delayed reply.

Many of your comments are integrated in the v2 patch series - details below.

On Sam, 2017-11-25 at 17:41 +, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 22:46:07 +0100 (CET)
> Peter Meerwald-Stadler  wrote:
> 
> > Hello,
> > 
> > some quick comments on this driver below
> 
> A few additional bits from me but I think Peter got most of the key
> stuff.
> 
> > 
> > I think documentation is missing and the ABI is a bit problematic and 
> > unusual 

In v2, the interface is definitely more user-friendly and with that,
hopefully, less exotic.

> > 
> > > Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors  
> > 
> > generally, we tend to avoid wildcard driver names; sgp30 would be 
> > preferred over sgpxx

renamed to sgp30 in v2

> >  
> > > Supported Features:
> > > 
> > > * Indoor Air Quality (IAQ) concentrations for
> > >   SGP30 and SGPC3:
> > >   - tVOC (in_concentration_voc_input)
> > >   SGP30 only:
> > >   - CO2eq (in_concentration_co2_input)
> > >   IAQ must first be initialized by writing a non-empty value to
> > >   out_iaq_init. After initializing IAQ, at least one IAQ signal must
> > >   be read out every second (SGP30) / every two seconds (SGPC3) for the
> > >   sensor to correctly maintain its internal baseline
> > > * Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
> > > * Gas concentration signals for
> > >   SGP30 and SGPC3:
> > >   - Ethanol (in_concentration_ethanol_raw)
> > >   SGP30 only:
> > >   - H2 (in_concentration_h2_raw)
> > > * On-chip self test (in_selftest)
> > >   The self test interferes with IAQ operations. If needed, first
> > >   retrieve the current baseline, then reset it after the self test
> > > * Sensor interface version (in_feature_set_version)
> > > * Sensor serial number (in_serial_id)
> > > * Humidity compensation for SGP30
> > >   With the help of a humidity signal, the gas signals can be
> > >   humidity-compensated.
> > > * Checksummed I2C communication  
> > 
> > 
> >  
> > > For all features, refer to the data sheet or the documentation in
> > > Documentation/iio/chemical/sgpxx.txt for more details.  
> > 
> > may some brief TODOs; heat controller?

The heater interface is not publicly accessible and not part of the datasheet.
We obviously can't recommend playing with it.

> >  
> > > Signed-off-by: Andreas Brauchli 
> > > ---
> > >  Documentation/iio/chemical/sgpxx.txt | 112 +
> > >  drivers/iio/chemical/Kconfig |  13 +
> > >  drivers/iio/chemical/Makefile|   1 +
> > >  drivers/iio/chemical/sgpxx.c | 894 
> > > +++
> > >  4 files changed, 1020 insertions(+)
> > >  create mode 100644 Documentation/iio/chemical/sgpxx.txt
> > >  create mode 100644 drivers/iio/chemical/sgpxx.c
> > > 
> > > diff --git a/Documentation/iio/chemical/sgpxx.txt 
> > > b/Documentation/iio/chemical/sgpxx.txt
> > > new file mode 100644
> > > index ..f49b2f365df3
> > > --- /dev/null
> > > +++ b/Documentation/iio/chemical/sgpxx.txt
> > > @@ -0,0 +1,112 @@
> > > +sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
> > > +
> > > +1. Overview
> > > +
> > > +The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas 
> > > sensors.
> > > +
> > > +Datasheets:
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
> 
> Put this stuff in the driver itself.

already there, so removed from doc in v2.

> > > +
> > > +2. Modes of Operation
> > > +
> > > +2.1. Driver Instantiation
> > > +
> > > +The sgpxx driver must be instantiated on the corresponding i2c bus with 
> > > the
> > > +product name (sgp30 or sgpc3) and i2c address (0x58).
> > > +
> > > +Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
> > > +
> > > +$ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
> > > +
> > > +Using the wrong product name results in an instantiation error. Check 
> > > dmesg.  
> > 
> > I'd rather drop this section, the only specific information is the I2C 
> > address

dropped in v2

> Which belongs in the device tree bindings not here.
> 
> > 
> > > +
> > > +2.2. Indoor Air Quality (IAQ) concentrations
> > > +
> > > +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> > > +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 
> > > only
> > > +
> > > +2.2.1. IAQ Initialization
> > > +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
> > > +initialized by writing a non-empty value to out_iaq_init:
> > > +
> > > +$ echo init > out_iaq_init  
> > 
> > can't this be done on probe()?

Yes, v2 now uses this approach and removes 

Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

[Andreas Brauchli]

Dear Peter, Jonathan,

Thanks for the thourough and speedy review and apologies for the delayed reply.

Many of your comments are integrated in the v2 patch series - details below.

On Sam, 2017-11-25 at 17:41 +, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 22:46:07 +0100 (CET)
> Peter Meerwald-Stadler  wrote:
> 
> > Hello,
> > 
> > some quick comments on this driver below
> 
> A few additional bits from me but I think Peter got most of the key
> stuff.
> 
> > 
> > I think documentation is missing and the ABI is a bit problematic and 
> > unusual 

In v2, the interface is definitely more user-friendly and with that,
hopefully, less exotic.

> > 
> > > Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors  
> > 
> > generally, we tend to avoid wildcard driver names; sgp30 would be 
> > preferred over sgpxx

renamed to sgp30 in v2

> >  
> > > Supported Features:
> > > 
> > > * Indoor Air Quality (IAQ) concentrations for
> > >   SGP30 and SGPC3:
> > >   - tVOC (in_concentration_voc_input)
> > >   SGP30 only:
> > >   - CO2eq (in_concentration_co2_input)
> > >   IAQ must first be initialized by writing a non-empty value to
> > >   out_iaq_init. After initializing IAQ, at least one IAQ signal must
> > >   be read out every second (SGP30) / every two seconds (SGPC3) for the
> > >   sensor to correctly maintain its internal baseline
> > > * Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
> > > * Gas concentration signals for
> > >   SGP30 and SGPC3:
> > >   - Ethanol (in_concentration_ethanol_raw)
> > >   SGP30 only:
> > >   - H2 (in_concentration_h2_raw)
> > > * On-chip self test (in_selftest)
> > >   The self test interferes with IAQ operations. If needed, first
> > >   retrieve the current baseline, then reset it after the self test
> > > * Sensor interface version (in_feature_set_version)
> > > * Sensor serial number (in_serial_id)
> > > * Humidity compensation for SGP30
> > >   With the help of a humidity signal, the gas signals can be
> > >   humidity-compensated.
> > > * Checksummed I2C communication  
> > 
> > 
> >  
> > > For all features, refer to the data sheet or the documentation in
> > > Documentation/iio/chemical/sgpxx.txt for more details.  
> > 
> > may some brief TODOs; heat controller?

The heater interface is not publicly accessible and not part of the datasheet.
We obviously can't recommend playing with it.

> >  
> > > Signed-off-by: Andreas Brauchli 
> > > ---
> > >  Documentation/iio/chemical/sgpxx.txt | 112 +
> > >  drivers/iio/chemical/Kconfig |  13 +
> > >  drivers/iio/chemical/Makefile|   1 +
> > >  drivers/iio/chemical/sgpxx.c | 894 
> > > +++
> > >  4 files changed, 1020 insertions(+)
> > >  create mode 100644 Documentation/iio/chemical/sgpxx.txt
> > >  create mode 100644 drivers/iio/chemical/sgpxx.c
> > > 
> > > diff --git a/Documentation/iio/chemical/sgpxx.txt 
> > > b/Documentation/iio/chemical/sgpxx.txt
> > > new file mode 100644
> > > index ..f49b2f365df3
> > > --- /dev/null
> > > +++ b/Documentation/iio/chemical/sgpxx.txt
> > > @@ -0,0 +1,112 @@
> > > +sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
> > > +
> > > +1. Overview
> > > +
> > > +The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas 
> > > sensors.
> > > +
> > > +Datasheets:
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
> 
> Put this stuff in the driver itself.

already there, so removed from doc in v2.

> > > +
> > > +2. Modes of Operation
> > > +
> > > +2.1. Driver Instantiation
> > > +
> > > +The sgpxx driver must be instantiated on the corresponding i2c bus with 
> > > the
> > > +product name (sgp30 or sgpc3) and i2c address (0x58).
> > > +
> > > +Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
> > > +
> > > +$ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
> > > +
> > > +Using the wrong product name results in an instantiation error. Check 
> > > dmesg.  
> > 
> > I'd rather drop this section, the only specific information is the I2C 
> > address

dropped in v2

> Which belongs in the device tree bindings not here.
> 
> > 
> > > +
> > > +2.2. Indoor Air Quality (IAQ) concentrations
> > > +
> > > +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> > > +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 
> > > only
> > > +
> > > +2.2.1. IAQ Initialization
> > > +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
> > > +initialized by writing a non-empty value to out_iaq_init:
> > > +
> > > +$ echo init > out_iaq_init  
> > 
> > can't this be done on probe()?

Yes, v2 now uses this approach and removes iaq_init entirely.
A semi-replacement is provided in 

Re: [PATCH] ARM: BCM5301X: Fix NAND ECC parameters for Linksys Panamera

[Rafał Miłecki]

On 10 March 2018 at 18:12, Vivek Unune  wrote:
> Using BCH8 gives ecc errors and makes the router unsuable.
> Switching to BCH1 fixes these errors.

Can you provide CFE's log messages starting with
"Decompressing...done" and up to the "Press Ctrl+C to stop in CFE"
please? I'd like to see what NAND info CFE prints there.

Re: [PATCH] ARM: BCM5301X: Fix NAND ECC parameters for Linksys Panamera

[Rafał Miłecki]

On 10 March 2018 at 18:12, Vivek Unune  wrote:
> Using BCH8 gives ecc errors and makes the router unsuable.
> Switching to BCH1 fixes these errors.

Can you provide CFE's log messages starting with
"Decompressing...done" and up to the "Press Ctrl+C to stop in CFE"
please? I'd like to see what NAND info CFE prints there.

[PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:18:45 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 1cc5bba670e1..7d37a417c756 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -757,10 +757,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
/* configure one AP supported rate class */
acx->rate_policy_idx = cpu_to_le32(wlvif->sta.ap_rate_idx);
@@ -773,10 +771,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
/*
 * configure one rate class for basic p2p operations.
@@ -791,14 +787,16 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, 
struct wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
-out:
+free_acx:
kfree(acx);
return ret;
+
+report_failure:
+   wl1271_warning("Setting of rate policies failed: %d", ret);
+   goto free_acx;
 }
 
 int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,
-- 
2.16.2

[PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:18:45 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 1cc5bba670e1..7d37a417c756 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -757,10 +757,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
/* configure one AP supported rate class */
acx->rate_policy_idx = cpu_to_le32(wlvif->sta.ap_rate_idx);
@@ -773,10 +771,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
/*
 * configure one rate class for basic p2p operations.
@@ -791,14 +787,16 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, 
struct wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
 
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
-   if (ret < 0) {
-   wl1271_warning("Setting of rate policies failed: %d", ret);
-   goto out;
-   }
+   if (ret < 0)
+   goto report_failure;
 
-out:
+free_acx:
kfree(acx);
return ret;
+
+report_failure:
+   wl1271_warning("Setting of rate policies failed: %d", ret);
+   goto free_acx;
 }
 
 int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,
-- 
2.16.2

[PATCH 2/3] wlcore: Return directly after a failed kzalloc() in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:00:31 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 6991fee8fe61..1cc5bba670e1 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -743,11 +743,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
wl1271_debug(DEBUG_ACX, "acx rate policies");
 
acx = kzalloc(sizeof(*acx), GFP_KERNEL);
-
-   if (!acx) {
-   ret = -ENOMEM;
-   goto out;
-   }
+   if (!acx)
+   return -ENOMEM;
 
wl1271_debug(DEBUG_ACX, "basic_rate: 0x%x, full_rate: 0x%x",
wlvif->basic_rate, wlvif->rate_set);
-- 
2.16.2

[PATCH 2/3] wlcore: Return directly after a failed kzalloc() in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:00:31 +0100

Return directly after a call of the function "kzalloc" failed
at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 6991fee8fe61..1cc5bba670e1 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -743,11 +743,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
wl1271_debug(DEBUG_ACX, "acx rate policies");
 
acx = kzalloc(sizeof(*acx), GFP_KERNEL);
-
-   if (!acx) {
-   ret = -ENOMEM;
-   goto out;
-   }
+   if (!acx)
+   return -ENOMEM;
 
wl1271_debug(DEBUG_ACX, "basic_rate: 0x%x, full_rate: 0x%x",
wlvif->basic_rate, wlvif->rate_set);
-- 
2.16.2

[PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 21:51:17 +0100

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 3ca9167d6146..6991fee8fe61 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -738,7 +738,7 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
 {
struct acx_rate_policy *acx;
struct conf_tx_rate_class *c = >conf.tx.sta_rc_conf;
-   int ret = 0;
+   int ret;
 
wl1271_debug(DEBUG_ACX, "acx rate policies");
 
-- 
2.16.2

[PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 21:51:17 +0100

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/net/wireless/ti/wlcore/acx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c 
b/drivers/net/wireless/ti/wlcore/acx.c
index 3ca9167d6146..6991fee8fe61 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -738,7 +738,7 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct 
wl12xx_vif *wlvif)
 {
struct acx_rate_policy *acx;
struct conf_tx_rate_class *c = >conf.tx.sta_rc_conf;
-   int ret = 0;
+   int ret;
 
wl1271_debug(DEBUG_ACX, "acx rate policies");
 
-- 
2.16.2

[PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:25:45 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete an unnecessary variable initialisation
  Return directly after a failed kzalloc()
  Use common error handling code

 drivers/net/wireless/ti/wlcore/acx.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

-- 
2.16.2

[PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 10 Mar 2018 22:25:45 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete an unnecessary variable initialisation
  Return directly after a failed kzalloc()
  Use common error handling code

 drivers/net/wireless/ti/wlcore/acx.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

-- 
2.16.2

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Douglas Gilbert]

On 2018-03-10 03:49 PM, James Bottomley wrote:

On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:

Hi Bart,

On Fri, 9 Mar 2018 22:47:12 +, Bart Van Assche 
wrote:


On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:


+/*
+ * SCSI command sizes are as follows, in bytes, for fixed size
commands,
per
+ * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
an opcode
+ * determine its group.
+ * The size table is encoded into a 32-bit value by subtracting
each
value
+ * from 16, resulting in a value of 1715488362
+ * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
<< 4 +
10).
+ * Command group 3 is reserved and should never be used.
+ */
+#define COMMAND_SIZE(opcode) \
+   (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
7)


To me this seems hard to read and hard to verify. Could this have
been
written as a combination of ternary expressions, e.g. using a gcc
statement
expression to ensure that opcode is evaluated once?


That’s what I’d tried initially, e.g.

#define COMMAND_SIZE(opcode) ({ \
int index = ((opcode) >> 5) & 7; \
index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
10); \
})

But gcc still reckons that results in a VLA, defeating the initial
purpose of
the exercise.

Does it help if I make the magic value construction clearer?

#define SCSI_COMMAND_SIZE_TBL ( \
   (16 -  6)\
+ ((16 - 10) <<  4)   \
+ ((16 - 10) <<  8)   \
+ ((16 - 12) << 12)   \
+ ((16 - 16) << 16)   \
+ ((16 - 12) << 20)   \
+ ((16 - 10) << 24)   \
+ ((16 - 10) << 28))

#define COMMAND_SIZE(opcode)
\
   (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
7)


Couldn't we do the less clever thing of making the array a static const
and moving it to a header?  That way the compiler should be able to
work it out at compile time.


And maybe add a comment that as of now (SPC-5 rev 19), COMMAND_SIZE is not
valid for opcodes 0x7e and 0x7f plus everything above and including 0xc0.
The latter ones are vendor specific and are loosely constrained, probably
all even numbered lengths in the closed range: [6,260].


If the SCSI command sets want to keep up with NVMe, they may want to think
about how they can gainfully use cdb_s that are > 64 bytes long. WRITE
SCATTERED got into SBC-4 but READ GATHERED didn't, due to lack of interest.
The READ GATHERED proposed was a bidi command, but it could have been a
a simpler data-in command with a looong cdb (holding LBA, number_of_blocks
pairs).

Doug Gilbert

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Douglas Gilbert]

On 2018-03-10 03:49 PM, James Bottomley wrote:

On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote:

Hi Bart,

On Fri, 9 Mar 2018 22:47:12 +, Bart Van Assche 
wrote:


On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:


+/*
+ * SCSI command sizes are as follows, in bytes, for fixed size
commands,
per
+ * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of
an opcode
+ * determine its group.
+ * The size table is encoded into a 32-bit value by subtracting
each
value
+ * from 16, resulting in a value of 1715488362
+ * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6
<< 4 +
10).
+ * Command group 3 is reserved and should never be used.
+ */
+#define COMMAND_SIZE(opcode) \
+   (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) &
7)


To me this seems hard to read and hard to verify. Could this have
been
written as a combination of ternary expressions, e.g. using a gcc
statement
expression to ensure that opcode is evaluated once?


That’s what I’d tried initially, e.g.

#define COMMAND_SIZE(opcode) ({ \
int index = ((opcode) >> 5) & 7; \
index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 :
10); \
})

But gcc still reckons that results in a VLA, defeating the initial
purpose of
the exercise.

Does it help if I make the magic value construction clearer?

#define SCSI_COMMAND_SIZE_TBL ( \
   (16 -  6)\
+ ((16 - 10) <<  4)   \
+ ((16 - 10) <<  8)   \
+ ((16 - 12) << 12)   \
+ ((16 - 16) << 16)   \
+ ((16 - 12) << 20)   \
+ ((16 - 10) << 24)   \
+ ((16 - 10) << 28))

#define COMMAND_SIZE(opcode)
\
   (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) &
7)


Couldn't we do the less clever thing of making the array a static const
and moving it to a header?  That way the compiler should be able to
work it out at compile time.


And maybe add a comment that as of now (SPC-5 rev 19), COMMAND_SIZE is not
valid for opcodes 0x7e and 0x7f plus everything above and including 0xc0.
The latter ones are vendor specific and are loosely constrained, probably
all even numbered lengths in the closed range: [6,260].


If the SCSI command sets want to keep up with NVMe, they may want to think
about how they can gainfully use cdb_s that are > 64 bytes long. WRITE
SCATTERED got into SBC-4 but READ GATHERED didn't, due to lack of interest.
The READ GATHERED proposed was a bidi command, but it could have been a
a simpler data-in command with a looong cdb (holding LBA, number_of_blocks
pairs).

Doug Gilbert

Re: [PATCH] leds: fix wrong dmi_match on PC Engines APU LEDs

[Jacek Anaszewski]

Hi Hans,

Thank you for the patch.

On 03/05/2018 06:09 PM, Hans Ulli Kroll wrote:
> APU has compared to APU2 no DMI_BOARD_NAME.
> Use DMI_PRODUCT_NAME instead.

Could we have the commit message more expressive?

Is it that now this driver doesn't work for APU board?

> Signed-off-by: Hans Ulli Kroll 
> ---
>  drivers/leds/leds-apu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-apu.c b/drivers/leds/leds-apu.c
> index 74820aab9497..5bbf5c31413e 100644
> --- a/drivers/leds/leds-apu.c
> +++ b/drivers/leds/leds-apu.c
> @@ -206,7 +206,7 @@ static int __init apu_led_probe(struct platform_device 
> *pdev)
>  
>   apu_led->pdev = pdev;
>  
> - if (dmi_match(DMI_BOARD_NAME, "APU")) {
> + if (dmi_match(DMI_PRODUCT_NAME, "APU")) {
>   apu_led->profile = apu1_led_profile;
>   apu_led->platform = APU1_LED_PLATFORM;
>   apu_led->num_led_instances = ARRAY_SIZE(apu1_led_profile);
> 

If it fails here, then how it is possible that it succeeds
in the apu_led_init() ?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: fix wrong dmi_match on PC Engines APU LEDs

[Jacek Anaszewski]

Hi Hans,

Thank you for the patch.

On 03/05/2018 06:09 PM, Hans Ulli Kroll wrote:
> APU has compared to APU2 no DMI_BOARD_NAME.
> Use DMI_PRODUCT_NAME instead.

Could we have the commit message more expressive?

Is it that now this driver doesn't work for APU board?

> Signed-off-by: Hans Ulli Kroll 
> ---
>  drivers/leds/leds-apu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-apu.c b/drivers/leds/leds-apu.c
> index 74820aab9497..5bbf5c31413e 100644
> --- a/drivers/leds/leds-apu.c
> +++ b/drivers/leds/leds-apu.c
> @@ -206,7 +206,7 @@ static int __init apu_led_probe(struct platform_device 
> *pdev)
>  
>   apu_led->pdev = pdev;
>  
> - if (dmi_match(DMI_BOARD_NAME, "APU")) {
> + if (dmi_match(DMI_PRODUCT_NAME, "APU")) {
>   apu_led->profile = apu1_led_profile;
>   apu_led->platform = APU1_LED_PLATFORM;
>   apu_led->num_led_instances = ARRAY_SIZE(apu1_led_profile);
> 

If it fails here, then how it is possible that it succeeds
in the apu_led_init() ?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] x86: always use SYSCALL_DEFINE*

[Tautschnig, Michael]

On 10 Mar 2018, at 20:55, Tautschnig, Michael  wrote:
> 
> All syscall arguments are passed in as types of the same byte size as
> unsigned long (width of full registers). Using a smaller type without a
> cast may result in losing bits of information. SYSCALL_DEFINE* introduce
> adequate type casts. All definitions of syscalls in x86 except for those
> patched here have already been using the appropriate SYSCALL_DEFINE*.
[...]

Additional context: I had previously made an attempt to ensure type
consistency of sys_ioperm as "Syscall arguments are unsigned long (full
registers)" (https://lkml.org/lkml/2016/7/4/336). I hope the new proposal
is more acceptable.

Best,
Michael



Amazon Web Services UK Limited. Registered in England and Wales with 
registration number 08650665 with its registered office at 1 Principal Place, 
Worship Street, London, EC2A 2FA, United Kingdom.

Re: [PATCH] x86: always use SYSCALL_DEFINE*

[Tautschnig, Michael]

On 10 Mar 2018, at 20:55, Tautschnig, Michael  wrote:
> 
> All syscall arguments are passed in as types of the same byte size as
> unsigned long (width of full registers). Using a smaller type without a
> cast may result in losing bits of information. SYSCALL_DEFINE* introduce
> adequate type casts. All definitions of syscalls in x86 except for those
> patched here have already been using the appropriate SYSCALL_DEFINE*.
[...]

Additional context: I had previously made an attempt to ensure type
consistency of sys_ioperm as "Syscall arguments are unsigned long (full
registers)" (https://lkml.org/lkml/2016/7/4/336). I hope the new proposal
is more acceptable.

Best,
Michael



Amazon Web Services UK Limited. Registered in England and Wales with 
registration number 08650665 with its registered office at 1 Principal Place, 
Worship Street, London, EC2A 2FA, United Kingdom.