Re: [PATCH RFC 2/5] sched/fair: Skip frequency update if CPU about to idle

[Joel Fernandes]

On Wed, Nov 1, 2017 at 12:35 PM, Steve Muckle  wrote:
> On 10/30/2017 12:02 PM, Joel Fernandes wrote:
>>>
>>> Also, this more looks like a policy decision. Will it be better to
>>> put that directly into schedutil? Like this:
>>>
>>>  if (cpu_idle())
>>>  "Don't change the freq";
>>>
>>> Will something like that work?
>>
>>
>> I thought about this and I think it wont work very well. In the
>> dequeue path we're still running the task being dequeued so the CPU is
>> not yet idle. What is needed here IMO is a notion that the CPU is
>> possibly about to idle and we can get predict that from the dequeue
>> path of the CFS class.
>>
>> Also just looking at whether the CPU is currently idle or not in the
>> governor doesn't help to differentiate between say the dequeue path /
>> tick path. Both of which can occur when the CPU is not idle.
>>
>> Any thoughts about this?
>
>
> Also if it really is the case that this bit of policy is universally
> desirable, I'd think it is better to do this in the scheduler and avoid a
> needless trip through a fn pointer out to schedutil for performance reasons.

I agree.

Peter, what do you think? Are you Ok with the approach of this patch
(preventing of the cpufreq update call during dequeue)?

thanks,

- Joel

Re: [PATCH RFC 2/5] sched/fair: Skip frequency update if CPU about to idle

[Joel Fernandes]

On Wed, Nov 1, 2017 at 12:35 PM, Steve Muckle  wrote:
> On 10/30/2017 12:02 PM, Joel Fernandes wrote:
>>>
>>> Also, this more looks like a policy decision. Will it be better to
>>> put that directly into schedutil? Like this:
>>>
>>>  if (cpu_idle())
>>>  "Don't change the freq";
>>>
>>> Will something like that work?
>>
>>
>> I thought about this and I think it wont work very well. In the
>> dequeue path we're still running the task being dequeued so the CPU is
>> not yet idle. What is needed here IMO is a notion that the CPU is
>> possibly about to idle and we can get predict that from the dequeue
>> path of the CFS class.
>>
>> Also just looking at whether the CPU is currently idle or not in the
>> governor doesn't help to differentiate between say the dequeue path /
>> tick path. Both of which can occur when the CPU is not idle.
>>
>> Any thoughts about this?
>
>
> Also if it really is the case that this bit of policy is universally
> desirable, I'd think it is better to do this in the scheduler and avoid a
> needless trip through a fn pointer out to schedutil for performance reasons.

I agree.

Peter, what do you think? Are you Ok with the approach of this patch
(preventing of the cpufreq update call during dequeue)?

thanks,

- Joel

Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property

[Brian Norris]

On Mon, Oct 30, 2017 at 8:03 PM, Lin Huang  wrote:
> Document a "reset" and "assert-reset-us", it can be used for
> driver control reset property. And reuse post-power-on-delay-ms
> for deassert reset delay.
>
> Signed-off-by: Lin Huang 
> ---
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt 
> b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index 28e8bd8..6ab0eed 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used 
> in addition to the
>
>  - vdd-supply: phandle of the regulator that provides the supply voltage.
>  - post-power-on-delay-ms: time required by the device after enabling its 
> regulators
> -  before it is ready for communication. Must be used with 'vdd-supply'.
> +  or deassert reset pin before it is ready for communication.
> +- reset: phandle of the gpio that provides for hid reset pin.
> +- assert-reset-us: the device require reset assert time.

If there was any point in adding the device-specific description
around "wacom,w9013"...then you should probably mention these
properties there too. The idea was to document possible properties
here (where you're adding them already), and to note the property
names under the devices (or so far, just 1 device) that support them.
Or IOW, you need an addition like this:

 - compatible:
   * "wacom,w9013" (Wacom W9013 digitizer). Supports:
 - vdd-supply
 - post-power-on-delay-ms
+- reset-gpios
+- assert-reset-us

Brian

Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property

[Brian Norris]

On Mon, Oct 30, 2017 at 8:03 PM, Lin Huang  wrote:
> Document a "reset" and "assert-reset-us", it can be used for
> driver control reset property. And reuse post-power-on-delay-ms
> for deassert reset delay.
>
> Signed-off-by: Lin Huang 
> ---
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt 
> b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index 28e8bd8..6ab0eed 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used 
> in addition to the
>
>  - vdd-supply: phandle of the regulator that provides the supply voltage.
>  - post-power-on-delay-ms: time required by the device after enabling its 
> regulators
> -  before it is ready for communication. Must be used with 'vdd-supply'.
> +  or deassert reset pin before it is ready for communication.
> +- reset: phandle of the gpio that provides for hid reset pin.
> +- assert-reset-us: the device require reset assert time.

If there was any point in adding the device-specific description
around "wacom,w9013"...then you should probably mention these
properties there too. The idea was to document possible properties
here (where you're adding them already), and to note the property
names under the devices (or so far, just 1 device) that support them.
Or IOW, you need an addition like this:

 - compatible:
   * "wacom,w9013" (Wacom W9013 digitizer). Supports:
 - vdd-supply
 - post-power-on-delay-ms
+- reset-gpios
+- assert-reset-us

Brian

[PATCH -next 2/2] arm: kprobes: Remove jprobe test case

[Masami Hiramatsu]

Remove jprobe test case because jprobe is deprecated
feature. We must not use it.

Signed-off-by: Masami Hiramatsu 
---
 arch/arm/probes/kprobes/test-core.c |   57 ---
 1 file changed, 57 deletions(-)

diff --git a/arch/arm/probes/kprobes/test-core.c 
b/arch/arm/probes/kprobes/test-core.c
index 9c3ceba69015..9ed0129bed3c 100644
--- a/arch/arm/probes/kprobes/test-core.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -227,7 +227,6 @@ static bool test_regs_ok;
 static int test_func_instance;
 static int pre_handler_called;
 static int post_handler_called;
-static int jprobe_func_called;
 static int kretprobe_handler_called;
 static int tests_failed;
 
@@ -370,50 +369,6 @@ static int test_kprobe(long (*func)(long, long))
return 0;
 }
 
-static void __kprobes jprobe_func(long r0, long r1)
-{
-   jprobe_func_called = test_func_instance;
-   if (r0 == FUNC_ARG1 && r1 == FUNC_ARG2)
-   test_regs_ok = true;
-   jprobe_return();
-}
-
-static struct jprobe the_jprobe = {
-   .entry  = jprobe_func,
-};
-
-static int test_jprobe(long (*func)(long, long))
-{
-   int ret;
-
-   the_jprobe.kp.addr = (kprobe_opcode_t *)func;
-   ret = register_jprobe(_jprobe);
-   if (ret < 0) {
-   pr_err("FAIL: register_jprobe failed with %d\n", ret);
-   return ret;
-   }
-
-   ret = call_test_func(func, true);
-
-   unregister_jprobe(_jprobe);
-   the_jprobe.kp.flags = 0; /* Clear disable flag to allow reuse */
-
-   if (!ret)
-   return -EINVAL;
-   if (jprobe_func_called != test_func_instance) {
-   pr_err("FAIL: jprobe handler function not called\n");
-   return -EINVAL;
-   }
-   if (!call_test_func(func, false))
-   return -EINVAL;
-   if (jprobe_func_called == test_func_instance) {
-   pr_err("FAIL: probe called after unregistering\n");
-   return -EINVAL;
-   }
-
-   return 0;
-}
-
 static int __kprobes
 kretprobe_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
@@ -468,18 +423,6 @@ static int run_api_tests(long (*func)(long, long))
if (ret < 0)
return ret;
 
-   pr_info("jprobe\n");
-   ret = test_jprobe(func);
-#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
-   if (ret == -EINVAL) {
-   pr_err("FAIL: Known longtime bug with jprobe on Thumb 
kernels\n");
-   tests_failed = ret;
-   ret = 0;
-   }
-#endif
-   if (ret < 0)
-   return ret;
-
pr_info("kretprobe\n");
ret = test_kretprobe(func);
if (ret < 0)

[PATCH -next 2/2] arm: kprobes: Remove jprobe test case

[Masami Hiramatsu]

Remove jprobe test case because jprobe is deprecated
feature. We must not use it.

Signed-off-by: Masami Hiramatsu 
---
 arch/arm/probes/kprobes/test-core.c |   57 ---
 1 file changed, 57 deletions(-)

diff --git a/arch/arm/probes/kprobes/test-core.c 
b/arch/arm/probes/kprobes/test-core.c
index 9c3ceba69015..9ed0129bed3c 100644
--- a/arch/arm/probes/kprobes/test-core.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -227,7 +227,6 @@ static bool test_regs_ok;
 static int test_func_instance;
 static int pre_handler_called;
 static int post_handler_called;
-static int jprobe_func_called;
 static int kretprobe_handler_called;
 static int tests_failed;
 
@@ -370,50 +369,6 @@ static int test_kprobe(long (*func)(long, long))
return 0;
 }
 
-static void __kprobes jprobe_func(long r0, long r1)
-{
-   jprobe_func_called = test_func_instance;
-   if (r0 == FUNC_ARG1 && r1 == FUNC_ARG2)
-   test_regs_ok = true;
-   jprobe_return();
-}
-
-static struct jprobe the_jprobe = {
-   .entry  = jprobe_func,
-};
-
-static int test_jprobe(long (*func)(long, long))
-{
-   int ret;
-
-   the_jprobe.kp.addr = (kprobe_opcode_t *)func;
-   ret = register_jprobe(_jprobe);
-   if (ret < 0) {
-   pr_err("FAIL: register_jprobe failed with %d\n", ret);
-   return ret;
-   }
-
-   ret = call_test_func(func, true);
-
-   unregister_jprobe(_jprobe);
-   the_jprobe.kp.flags = 0; /* Clear disable flag to allow reuse */
-
-   if (!ret)
-   return -EINVAL;
-   if (jprobe_func_called != test_func_instance) {
-   pr_err("FAIL: jprobe handler function not called\n");
-   return -EINVAL;
-   }
-   if (!call_test_func(func, false))
-   return -EINVAL;
-   if (jprobe_func_called == test_func_instance) {
-   pr_err("FAIL: probe called after unregistering\n");
-   return -EINVAL;
-   }
-
-   return 0;
-}
-
 static int __kprobes
 kretprobe_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
@@ -468,18 +423,6 @@ static int run_api_tests(long (*func)(long, long))
if (ret < 0)
return ret;
 
-   pr_info("jprobe\n");
-   ret = test_jprobe(func);
-#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
-   if (ret == -EINVAL) {
-   pr_err("FAIL: Known longtime bug with jprobe on Thumb 
kernels\n");
-   tests_failed = ret;
-   ret = 0;
-   }
-#endif
-   if (ret < 0)
-   return ret;
-
pr_info("kretprobe\n");
ret = test_kretprobe(func);
if (ret < 0)

[PATCH -next 0/2] arm: kprobes: Remove jprobe test case

[Masami Hiramatsu]

Hi,

This series fixes a typo and remove jprobe test case
because jprobe is deprecated feature. The typo is
related to jprobe so need to be fixed in this series.

Thank you,

---

Masami Hiramatsu (2):
  arm: kprobes: Fix kretprobe test to check correct counter
  arm: kprobes: Remove jprobe test case


 arch/arm/probes/kprobes/test-core.c |   59 +--
 1 file changed, 1 insertion(+), 58 deletions(-)

--
Masami Hiramatsu (Linaro) 

[PATCH -next 1/2] arm: kprobes: Fix kretprobe test to check correct counter

[Masami Hiramatsu]

test_kretprobe() uses jprobe_func_called at the
last test, but it must check kretprobe_handler_called.

Signed-off-by: Masami Hiramatsu 
---
 arch/arm/probes/kprobes/test-core.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/probes/kprobes/test-core.c 
b/arch/arm/probes/kprobes/test-core.c
index 1c98a87786ca..9c3ceba69015 100644
--- a/arch/arm/probes/kprobes/test-core.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -451,7 +451,7 @@ static int test_kretprobe(long (*func)(long, long))
}
if (!call_test_func(func, false))
return -EINVAL;
-   if (jprobe_func_called == test_func_instance) {
+   if (kretprobe_handler_called == test_func_instance) {
pr_err("FAIL: kretprobe called after unregistering\n");
return -EINVAL;
}

[PATCH -next 0/2] arm: kprobes: Remove jprobe test case

[Masami Hiramatsu]

Hi,

This series fixes a typo and remove jprobe test case
because jprobe is deprecated feature. The typo is
related to jprobe so need to be fixed in this series.

Thank you,

---

Masami Hiramatsu (2):
  arm: kprobes: Fix kretprobe test to check correct counter
  arm: kprobes: Remove jprobe test case


 arch/arm/probes/kprobes/test-core.c |   59 +--
 1 file changed, 1 insertion(+), 58 deletions(-)

--
Masami Hiramatsu (Linaro) 

[PATCH -next 1/2] arm: kprobes: Fix kretprobe test to check correct counter

[Masami Hiramatsu]

test_kretprobe() uses jprobe_func_called at the
last test, but it must check kretprobe_handler_called.

Signed-off-by: Masami Hiramatsu 
---
 arch/arm/probes/kprobes/test-core.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/probes/kprobes/test-core.c 
b/arch/arm/probes/kprobes/test-core.c
index 1c98a87786ca..9c3ceba69015 100644
--- a/arch/arm/probes/kprobes/test-core.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -451,7 +451,7 @@ static int test_kretprobe(long (*func)(long, long))
}
if (!call_test_func(func, false))
return -EINVAL;
-   if (jprobe_func_called == test_func_instance) {
+   if (kretprobe_handler_called == test_func_instance) {
pr_err("FAIL: kretprobe called after unregistering\n");
return -EINVAL;
}

Re: [PATCH 2/2] Subject: printk: Don't trap random context in infinite log_buf flush

[Sergey Senozhatsky]

On (11/02/17 06:52), Tejun Heo wrote:
> 
> When printk flushing isn't contended, whoever creates messages get to
> flush them, which is great in both fairness and keeping log delivery
> synchronous.  However, when console drivers can't keep up with the
> rate of new messages, which happens a lot, who ends up with the
> flushing duty is determined by timing and everyone else's messages
> become asynchronous.


Hello Tejun,

thanks for the patch set. we are currently looking at another approach:
lkml.kernel.org/r/20171102134515.6eef1...@gandalf.local.home

would you be interested in taking a look?

there are some concerns, like a huge number of printk-s happening while
console_sem is locked. e.g. console_lock()/console_unlock() on one of the
CPUs, or console_lock(); printk(); ... printk(); console_unlock();

>
> the problem of "the last printk()", which will take
> over and do the flush.
> 
> CPU0CPU1  ~  CPU99
> console_lock();
>printk(); ... printk();
> console_unlock();
>IRQ on CPU2
> printk()
>  // take over console_sem
>  console_unlock()
>

-ss

Re: [PATCH 2/2] Subject: printk: Don't trap random context in infinite log_buf flush

[Sergey Senozhatsky]

On (11/02/17 06:52), Tejun Heo wrote:
> 
> When printk flushing isn't contended, whoever creates messages get to
> flush them, which is great in both fairness and keeping log delivery
> synchronous.  However, when console drivers can't keep up with the
> rate of new messages, which happens a lot, who ends up with the
> flushing duty is determined by timing and everyone else's messages
> become asynchronous.


Hello Tejun,

thanks for the patch set. we are currently looking at another approach:
lkml.kernel.org/r/20171102134515.6eef1...@gandalf.local.home

would you be interested in taking a look?

there are some concerns, like a huge number of printk-s happening while
console_sem is locked. e.g. console_lock()/console_unlock() on one of the
CPUs, or console_lock(); printk(); ... printk(); console_unlock();

>
> the problem of "the last printk()", which will take
> over and do the flush.
> 
> CPU0CPU1  ~  CPU99
> console_lock();
>printk(); ... printk();
> console_unlock();
>IRQ on CPU2
> printk()
>  // take over console_sem
>  console_unlock()
>

-ss

Re: [PATCH V3 0/7] blk-mq: don't allocate driver tag beforehand for flush rq

[Ming Lei]

On Thu, Nov 02, 2017 at 11:24:31PM +0800, Ming Lei wrote:
> Hi Jens,
> 
> This patchset avoids to allocate driver tag beforehand for flush rq
> in case of I/O scheduler, then flush rq isn't treated specially
> wrt. get/put driver tag, code gets cleanup much, such as,
> reorder_tags_to_front() is removed, and we needn't to worry
> about request order in dispatch list for avoiding I/O deadlock.
> 
> 'dbench -t 30 -s 64' has been run on different devices(shared tag,
> multi-queue, singele queue, ...), and no issues are observed,
> even very low queue depth test are run, debench still works well.
> 
> Please consider it for V4.15, thanks!

Hi Jens,

As we discussed before, this patch is a good cleanup on handling flush
request, could you share your opinion on V3?

thanks,
Ming

Re: [PATCH V3 0/7] blk-mq: don't allocate driver tag beforehand for flush rq

[Ming Lei]

On Thu, Nov 02, 2017 at 11:24:31PM +0800, Ming Lei wrote:
> Hi Jens,
> 
> This patchset avoids to allocate driver tag beforehand for flush rq
> in case of I/O scheduler, then flush rq isn't treated specially
> wrt. get/put driver tag, code gets cleanup much, such as,
> reorder_tags_to_front() is removed, and we needn't to worry
> about request order in dispatch list for avoiding I/O deadlock.
> 
> 'dbench -t 30 -s 64' has been run on different devices(shared tag,
> multi-queue, singele queue, ...), and no issues are observed,
> even very low queue depth test are run, debench still works well.
> 
> Please consider it for V4.15, thanks!

Hi Jens,

As we discussed before, this patch is a good cleanup on handling flush
request, could you share your opinion on V3?

thanks,
Ming

[PATCH] PM / devfreq: Propagate error from devfreq_add_device()

[Bjorn Andersson]

Propagate the error of devfreq_add_device() in devm_devfreq_add_device()
rather than statically returning ENOMEM. This makes it slightly faster
to pinpoint the cause of a returned error.

Signed-off-by: Bjorn Andersson 
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 78fb496ecb4e..99c4021fc33b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -737,7 +737,7 @@ struct devfreq *devm_devfreq_add_device(struct device *dev,
devfreq = devfreq_add_device(dev, profile, governor_name, data);
if (IS_ERR(devfreq)) {
devres_free(ptr);
-   return ERR_PTR(-ENOMEM);
+   return devfreq;
}
 
*ptr = devfreq;
-- 
2.12.0

[PATCH] PM / devfreq: Propagate error from devfreq_add_device()

[Bjorn Andersson]

Propagate the error of devfreq_add_device() in devm_devfreq_add_device()
rather than statically returning ENOMEM. This makes it slightly faster
to pinpoint the cause of a returned error.

Signed-off-by: Bjorn Andersson 
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 78fb496ecb4e..99c4021fc33b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -737,7 +737,7 @@ struct devfreq *devm_devfreq_add_device(struct device *dev,
devfreq = devfreq_add_device(dev, profile, governor_name, data);
if (IS_ERR(devfreq)) {
devres_free(ptr);
-   return ERR_PTR(-ENOMEM);
+   return devfreq;
}
 
*ptr = devfreq;
-- 
2.12.0

RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Mario.Limonciello]

> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Friday, November 3, 2017 7:53 PM
> To: Limonciello, Mario 
> Cc: Andy Shevchenko ; LKML  ker...@vger.kernel.org>; platform-driver-...@vger.kernel.org;
> pali.ro...@gmail.com
> Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe 
> to
> dependent drivers
> 
> On Fri, Nov 03, 2017 at 11:27:22AM -0500, Mario Limonciello wrote:
> > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> > finishing probe successfully to probe themselves.
> >
> > Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> > (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> > try to redo probing due to deferred probing.
> >
> > To solve this have the dependent drivers query the dell-wmi-descriptor
> > driver whether the descriptor has been determined valid. The possible
> > results are:
> > -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
> >  and use deferred probing
> > < 0: Descriptor probed, invalid.  Dependent driver should return an
> >  error.
> > 0: Successful descriptor probe, dependent driver can continue
> >
> > Successful descriptor probe still doesn't mean that the descriptor driver
> > is necessarily bound at the time of initialization of dependent driver.
> > Userspace can unbind the driver, so all methods used from driver
> 
> Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a
> dependent driver is loaded. It isn't clear to me in which scenario we 
> encounter
> this problem ??

Userspace can however unbind a bound GUID.  When this happens getting
the interface version and size will both fail.

> 
> 
> > should still be verified to return success values otherwise deferred
> > probing be used.
> 
> The part after "otherwise" is breaking my English parser...
> 
> Should this read: "Userspace can unbind the driver, so all methods used from 
> the
> driver should still be verified to return successful values, falling back to
> deferred probing in case of failure." ??

Yeah that's clearer and correct.

> 
> > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h
> b/drivers/platform/x86/dell-wmi-descriptor.h
> > index 5f7b69c2c83a..776cddd5e135 100644
> > --- a/drivers/platform/x86/dell-wmi-descriptor.h
> > +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> > @@ -15,6 +15,13 @@
> >
> >  #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
> B622A1EF5492"
> >
> > +/* possible return values:
> 
> This should trigger a checkpatch error, but doesn't. Huh. For everything but
> "net", comment blocks should start with /* and not following text.
> 

OK.

> /*
>  * First line.
>  * Second line.
>  */
> 
> A nit, and I can clean up if no changes are deemed necessary here.
> 
> > + *  -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> > + *  0: valid descriptor, successfully probed
> > + *  < 0: invalid descriptor, don't probe dependent devices
> > + */
> 
> --
> Darren Hart
> VMware Open Source Technology Center

RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Mario.Limonciello]

> -Original Message-
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Friday, November 3, 2017 7:53 PM
> To: Limonciello, Mario 
> Cc: Andy Shevchenko ; LKML  ker...@vger.kernel.org>; platform-driver-...@vger.kernel.org;
> pali.ro...@gmail.com
> Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe 
> to
> dependent drivers
> 
> On Fri, Nov 03, 2017 at 11:27:22AM -0500, Mario Limonciello wrote:
> > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> > finishing probe successfully to probe themselves.
> >
> > Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> > (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> > try to redo probing due to deferred probing.
> >
> > To solve this have the dependent drivers query the dell-wmi-descriptor
> > driver whether the descriptor has been determined valid. The possible
> > results are:
> > -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
> >  and use deferred probing
> > < 0: Descriptor probed, invalid.  Dependent driver should return an
> >  error.
> > 0: Successful descriptor probe, dependent driver can continue
> >
> > Successful descriptor probe still doesn't mean that the descriptor driver
> > is necessarily bound at the time of initialization of dependent driver.
> > Userspace can unbind the driver, so all methods used from driver
> 
> Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a
> dependent driver is loaded. It isn't clear to me in which scenario we 
> encounter
> this problem ??

Userspace can however unbind a bound GUID.  When this happens getting
the interface version and size will both fail.

> 
> 
> > should still be verified to return success values otherwise deferred
> > probing be used.
> 
> The part after "otherwise" is breaking my English parser...
> 
> Should this read: "Userspace can unbind the driver, so all methods used from 
> the
> driver should still be verified to return successful values, falling back to
> deferred probing in case of failure." ??

Yeah that's clearer and correct.

> 
> > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h
> b/drivers/platform/x86/dell-wmi-descriptor.h
> > index 5f7b69c2c83a..776cddd5e135 100644
> > --- a/drivers/platform/x86/dell-wmi-descriptor.h
> > +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> > @@ -15,6 +15,13 @@
> >
> >  #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
> B622A1EF5492"
> >
> > +/* possible return values:
> 
> This should trigger a checkpatch error, but doesn't. Huh. For everything but
> "net", comment blocks should start with /* and not following text.
> 

OK.

> /*
>  * First line.
>  * Second line.
>  */
> 
> A nit, and I can clean up if no changes are deemed necessary here.
> 
> > + *  -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> > + *  0: valid descriptor, successfully probed
> > + *  < 0: invalid descriptor, don't probe dependent devices
> > + */
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Re: [PATCH v3] printk: Add console owner and waiter logic to load balance console writes

[Sergey Senozhatsky]

On (11/02/17 23:15), Steven Rostedt wrote:
> On Thu, 2 Nov 2017 23:16:16 +0100
> Vlastimil Babka  wrote:
> 
> > > + if (spin) {
> > > + /* We spin waiting for the owner to release us 
> > > */
> > > + spin_acquire(_owner_dep_map, 0, 0, 
> > > _THIS_IP_);
> > > + /* Owner will clear console_waiter on hand off 
> > > */
> > > + while (!READ_ONCE(console_waiter))  
> > 
> > This should not be negated, right? We should spin while it's true, not
> > false.
> 
> Ug, yes. How did that not crash in my tests.

Ah, right... Good catch, Vlastimil. The V1 didn't work as expected
on my tests.

-ss

Re: [PATCH v3] printk: Add console owner and waiter logic to load balance console writes

[Sergey Senozhatsky]

On (11/02/17 23:15), Steven Rostedt wrote:
> On Thu, 2 Nov 2017 23:16:16 +0100
> Vlastimil Babka  wrote:
> 
> > > + if (spin) {
> > > + /* We spin waiting for the owner to release us 
> > > */
> > > + spin_acquire(_owner_dep_map, 0, 0, 
> > > _THIS_IP_);
> > > + /* Owner will clear console_waiter on hand off 
> > > */
> > > + while (!READ_ONCE(console_waiter))  
> > 
> > This should not be negated, right? We should spin while it's true, not
> > false.
> 
> Ug, yes. How did that not crash in my tests.

Ah, right... Good catch, Vlastimil. The V1 didn't work as expected
on my tests.

-ss

Re: [PATCH v2] cpuidle: ladder: Add per CPU PM QoS resume latency support

[Ramesh Thomas]

On 2017-10-27 at 09:59:38 +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 27, 2017 at 4:01 AM, Ramesh Thomas  
> wrote:
> > Individual CPUs may have special requirements to not enter
> > deep idle states. For example, a CPU running real time
> > applications would not want to enter deep idle states to
> > avoid latency impacts. At the same time other CPUs that
> > do not have such a requirement could allow deep idle
> > states to save power.
> >
> > This was already implemented in the menu governor.
> > Implementing similar changes in the ladder governor which
> > gets selected when CONFIG_NO_HZ and CONFIG_NO_HZ_IDLE are not
> > set. Refer following commits for the menu governor changes.
> >
> > commit 9908859acaa9 ("cpuidle/menu: add per CPU PM QoS resume
> > latency consideration")
> > commit 6dbf5cea05a7 ("cpuidle: menu: Avoid taking spinlock for
> > accessing QoS values")
> >
> > Signed-off-by: Ramesh Thomas 
> > ---
> >
> > v2:
> >   - use PM_QOS_RESUME_LATENCY_NO_CONSTRAINT for "no constraint" value
> >   Should be applied over https://patchwork.kernel.org/patch/10024157/
> >
> >  drivers/cpuidle/governors/ladder.c | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/cpuidle/governors/ladder.c 
> > b/drivers/cpuidle/governors/ladder.c
> > index ce1a2ff..1ad8745 100644
> > --- a/drivers/cpuidle/governors/ladder.c
> > +++ b/drivers/cpuidle/governors/ladder.c
> > @@ -17,6 +17,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -67,10 +68,16 @@ static int ladder_select_state(struct cpuidle_driver 
> > *drv,
> > struct cpuidle_device *dev)
> >  {
> > struct ladder_device *ldev = this_cpu_ptr(_devices);
> > +   struct device *device = get_cpu_device(dev->cpu);
> > struct ladder_device_state *last_state;
> > int last_residency, last_idx = ldev->last_state_idx;
> > int first_idx = drv->states[0].flags & CPUIDLE_FLAG_POLLING ? 1 : 0;
> > int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
> > +   int resume_latency = dev_pm_qos_raw_read_value(device);
> > +
> > +   if (resume_latency < latency_req &&
> > +   resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> > +   latency_req = resume_latency;
> >
> > /* Special case when user has set very strict latency requirement */
> > if (unlikely(latency_req == 0)) {
> > --
> > 2.7.4
> >
> 
> Looks good to me.
> 
> I'll queue it up if nobody objects.
> 

Hi Rafael,

This patch would apply as is over your new PM QoS changes. If you need me
to repost please let me know.

Thanks,
Ramesh

Re: [PATCH v2] cpuidle: ladder: Add per CPU PM QoS resume latency support

[Ramesh Thomas]

On 2017-10-27 at 09:59:38 +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 27, 2017 at 4:01 AM, Ramesh Thomas  
> wrote:
> > Individual CPUs may have special requirements to not enter
> > deep idle states. For example, a CPU running real time
> > applications would not want to enter deep idle states to
> > avoid latency impacts. At the same time other CPUs that
> > do not have such a requirement could allow deep idle
> > states to save power.
> >
> > This was already implemented in the menu governor.
> > Implementing similar changes in the ladder governor which
> > gets selected when CONFIG_NO_HZ and CONFIG_NO_HZ_IDLE are not
> > set. Refer following commits for the menu governor changes.
> >
> > commit 9908859acaa9 ("cpuidle/menu: add per CPU PM QoS resume
> > latency consideration")
> > commit 6dbf5cea05a7 ("cpuidle: menu: Avoid taking spinlock for
> > accessing QoS values")
> >
> > Signed-off-by: Ramesh Thomas 
> > ---
> >
> > v2:
> >   - use PM_QOS_RESUME_LATENCY_NO_CONSTRAINT for "no constraint" value
> >   Should be applied over https://patchwork.kernel.org/patch/10024157/
> >
> >  drivers/cpuidle/governors/ladder.c | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/cpuidle/governors/ladder.c 
> > b/drivers/cpuidle/governors/ladder.c
> > index ce1a2ff..1ad8745 100644
> > --- a/drivers/cpuidle/governors/ladder.c
> > +++ b/drivers/cpuidle/governors/ladder.c
> > @@ -17,6 +17,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -67,10 +68,16 @@ static int ladder_select_state(struct cpuidle_driver 
> > *drv,
> > struct cpuidle_device *dev)
> >  {
> > struct ladder_device *ldev = this_cpu_ptr(_devices);
> > +   struct device *device = get_cpu_device(dev->cpu);
> > struct ladder_device_state *last_state;
> > int last_residency, last_idx = ldev->last_state_idx;
> > int first_idx = drv->states[0].flags & CPUIDLE_FLAG_POLLING ? 1 : 0;
> > int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
> > +   int resume_latency = dev_pm_qos_raw_read_value(device);
> > +
> > +   if (resume_latency < latency_req &&
> > +   resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> > +   latency_req = resume_latency;
> >
> > /* Special case when user has set very strict latency requirement */
> > if (unlikely(latency_req == 0)) {
> > --
> > 2.7.4
> >
> 
> Looks good to me.
> 
> I'll queue it up if nobody objects.
> 

Hi Rafael,

This patch would apply as is over your new PM QoS changes. If you need me
to repost please let me know.

Thanks,
Ramesh

Re: [RFT][PATCH v2 2/2] PM / QoS: Fix device resume latency framework

[Ramesh Thomas]

On 2017-11-03 at 12:50:15 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The special value of 0 for device resume latency PM QoS means
> "no restriction", but there are two problems with that.
> 
> First, device resume latency PM QoS requests with 0 as the
> value are always put in front of requests with positive
> values in the priority lists used internally by the PM QoS
> framework, causing 0 to be chosen as an effective constraint
> value.  However, that 0 is then interpreted as "no restriction"
> effectively overriding the other requests with specific
> restrictions which is incorrect.
> 
> Second, the users of device resume latency PM QoS have no
> way to specify that *any* resume latency at all should be
> avoided, which is an artificial limitation in general.
> 
> To address these issues, modify device resume latency PM QoS to
> use S32_MAX as the "no constraint" value and 0 as the "no
> latency at all" one and rework its users (the cpuidle menu
> governor, the genpd QoS governor and the runtime PM framework)
> to follow these changes.
> 
> Also add a special "n/a" value to the corresponding user space I/F
> to allow user space to indicate that it cannot accept any resume
> latencies at all for the given device.
> 
> Fixes: 85dc0b8a4019 (PM / QoS: Make it possible to expose PM QoS latency 
> constraints)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
> Reported-by: Reinette Chatre 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  Documentation/ABI/testing/sysfs-devices-power |4 +++-
>  drivers/base/cpu.c|3 ++-
>  drivers/base/power/domain.c   |2 +-
>  drivers/base/power/domain_governor.c  |   26 
> ++
>  drivers/base/power/qos.c  |5 -
>  drivers/base/power/runtime.c  |2 +-
>  drivers/base/power/sysfs.c|   25 
> +
>  drivers/cpuidle/governors/menu.c  |4 ++--
>  include/linux/pm_qos.h|   26 
> ++
>  9 files changed, 62 insertions(+), 35 deletions(-)
> 
> Index: linux-pm/drivers/base/power/sysfs.c
> ===
> --- linux-pm.orig/drivers/base/power/sysfs.c
> +++ linux-pm/drivers/base/power/sysfs.c
> @@ -218,7 +218,14 @@ static ssize_t pm_qos_resume_latency_sho
> struct device_attribute *attr,
> char *buf)
>  {
> - return sprintf(buf, "%d\n", dev_pm_qos_requested_resume_latency(dev));
> + s32 value = dev_pm_qos_requested_resume_latency(dev);
> +
> + if (value == 0)
> + return sprintf(buf, "n/a\n");
> + else if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> + value = 0;
> +
> + return sprintf(buf, "%d\n", value);
>  }
>  
>  static ssize_t pm_qos_resume_latency_store(struct device *dev,
> @@ -228,11 +235,21 @@ static ssize_t pm_qos_resume_latency_sto
>   s32 value;
>   int ret;
>  
> - if (kstrtos32(buf, 0, ))
> - return -EINVAL;
> + if (!kstrtos32(buf, 0, )) {
> + /*
> +  * Prevent users from writing negative or "no constraint" values
> +  * directly.
> +  */
> + if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> + return -EINVAL;
>  
> - if (value < 0)
> + if (value == 0)
> + value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> + } else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
> + value = 0;
> + } else {
>   return -EINVAL;
> + }
>  
>   ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>   value);
> Index: linux-pm/include/linux/pm_qos.h
> ===
> --- linux-pm.orig/include/linux/pm_qos.h
> +++ linux-pm/include/linux/pm_qos.h
> @@ -28,16 +28,19 @@ enum pm_qos_flags_status {
>   PM_QOS_FLAGS_ALL,
>  };
>  
> -#define PM_QOS_DEFAULT_VALUE -1
> +#define PM_QOS_DEFAULT_VALUE (-1)
> +#define PM_QOS_LATENCY_ANY   S32_MAX
> +#define PM_QOS_LATENCY_ANY_NS((s64)PM_QOS_LATENCY_ANY * 
> NSEC_PER_USEC)
>  
>  #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE  0
>  #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE0
> -#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE  0
> +#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE  PM_QOS_LATENCY_ANY
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT  PM_QOS_LATENCY_ANY
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS   PM_QOS_LATENCY_ANY_NS
>  #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 

Re: [RFT][PATCH v2 2/2] PM / QoS: Fix device resume latency framework

[Ramesh Thomas]

On 2017-11-03 at 12:50:15 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The special value of 0 for device resume latency PM QoS means
> "no restriction", but there are two problems with that.
> 
> First, device resume latency PM QoS requests with 0 as the
> value are always put in front of requests with positive
> values in the priority lists used internally by the PM QoS
> framework, causing 0 to be chosen as an effective constraint
> value.  However, that 0 is then interpreted as "no restriction"
> effectively overriding the other requests with specific
> restrictions which is incorrect.
> 
> Second, the users of device resume latency PM QoS have no
> way to specify that *any* resume latency at all should be
> avoided, which is an artificial limitation in general.
> 
> To address these issues, modify device resume latency PM QoS to
> use S32_MAX as the "no constraint" value and 0 as the "no
> latency at all" one and rework its users (the cpuidle menu
> governor, the genpd QoS governor and the runtime PM framework)
> to follow these changes.
> 
> Also add a special "n/a" value to the corresponding user space I/F
> to allow user space to indicate that it cannot accept any resume
> latencies at all for the given device.
> 
> Fixes: 85dc0b8a4019 (PM / QoS: Make it possible to expose PM QoS latency 
> constraints)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
> Reported-by: Reinette Chatre 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  Documentation/ABI/testing/sysfs-devices-power |4 +++-
>  drivers/base/cpu.c|3 ++-
>  drivers/base/power/domain.c   |2 +-
>  drivers/base/power/domain_governor.c  |   26 
> ++
>  drivers/base/power/qos.c  |5 -
>  drivers/base/power/runtime.c  |2 +-
>  drivers/base/power/sysfs.c|   25 
> +
>  drivers/cpuidle/governors/menu.c  |4 ++--
>  include/linux/pm_qos.h|   26 
> ++
>  9 files changed, 62 insertions(+), 35 deletions(-)
> 
> Index: linux-pm/drivers/base/power/sysfs.c
> ===
> --- linux-pm.orig/drivers/base/power/sysfs.c
> +++ linux-pm/drivers/base/power/sysfs.c
> @@ -218,7 +218,14 @@ static ssize_t pm_qos_resume_latency_sho
> struct device_attribute *attr,
> char *buf)
>  {
> - return sprintf(buf, "%d\n", dev_pm_qos_requested_resume_latency(dev));
> + s32 value = dev_pm_qos_requested_resume_latency(dev);
> +
> + if (value == 0)
> + return sprintf(buf, "n/a\n");
> + else if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> + value = 0;
> +
> + return sprintf(buf, "%d\n", value);
>  }
>  
>  static ssize_t pm_qos_resume_latency_store(struct device *dev,
> @@ -228,11 +235,21 @@ static ssize_t pm_qos_resume_latency_sto
>   s32 value;
>   int ret;
>  
> - if (kstrtos32(buf, 0, ))
> - return -EINVAL;
> + if (!kstrtos32(buf, 0, )) {
> + /*
> +  * Prevent users from writing negative or "no constraint" values
> +  * directly.
> +  */
> + if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> + return -EINVAL;
>  
> - if (value < 0)
> + if (value == 0)
> + value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> + } else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
> + value = 0;
> + } else {
>   return -EINVAL;
> + }
>  
>   ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>   value);
> Index: linux-pm/include/linux/pm_qos.h
> ===
> --- linux-pm.orig/include/linux/pm_qos.h
> +++ linux-pm/include/linux/pm_qos.h
> @@ -28,16 +28,19 @@ enum pm_qos_flags_status {
>   PM_QOS_FLAGS_ALL,
>  };
>  
> -#define PM_QOS_DEFAULT_VALUE -1
> +#define PM_QOS_DEFAULT_VALUE (-1)
> +#define PM_QOS_LATENCY_ANY   S32_MAX
> +#define PM_QOS_LATENCY_ANY_NS((s64)PM_QOS_LATENCY_ANY * 
> NSEC_PER_USEC)
>  
>  #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>  #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE  0
>  #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE0
> -#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE  0
> +#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE  PM_QOS_LATENCY_ANY
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT  PM_QOS_LATENCY_ANY
> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS   PM_QOS_LATENCY_ANY_NS
>  #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE   0
>  #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT   (-1)
> -#define 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
> 
> For the above reasons, rework it to be a bit more consistent.
> 
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
> 
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
> 
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>  
>   gpd_data->base.dev = dev;
>   gpd_data->td.constraint_changed = true;
> - gpd_data->td.effective_constraint_ns = -1;
> + gpd_data->td.effective_constraint_ns = 0;
>   gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>  
>   spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>   s64 *constraint_ns_p = data;
> - s32 constraint_ns = -1;
> -
> - if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> - constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + s64 constraint_ns;
>  
> - if (constraint_ns < 0) {
> - constraint_ns = dev_pm_qos_read_value(dev);
> - constraint_ns *= NSEC_PER_USEC;
> - }
> - if (constraint_ns == 0)
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>   return 0;
>  
>   /*
> -  * constraint_ns cannot be negative here, because the device has been
> -  * suspended.
> +  * Only take suspend-time QoS constraints of devices into account,
> +  * because constraints updated after the device has been suspended are
> +  * not guaranteed to be taken into account anyway.  In order for them
> +  * to take effect, the device has to be resumed and suspended again.
>*/
> + constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + /* 0 means "no constraint" */
> + if (constraint_ns == 0)
> + return 0;
> +
>   if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
>   *constraint_ns_p = constraint_ns;
>  
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
>   device_for_each_child(dev, _ns,
> dev_update_qos_constraint);
>  
> - if (constraint_ns > 0) {
> + if (constraint_ns == 0) {
> + /* "No restriction", so the device is allowed to suspend. */
> + td->effective_constraint_ns = 0;
> + td->cached_suspend_ok = true;
> + } else {
> + /*
> +  * constraint_ns must be positive here, because the children
> +  * walked above are all suspended, so effective_constraint_ns
> +  * cannot be negative for them.
> +  */
>   constraint_ns -= td->suspend_latency_ns +
>   td->resume_latency_ns;
> - if (constraint_ns == 0)
> + /*
> +  * effective_constraint_ns is 

Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ramesh Thomas]

On 2017-11-03 at 12:47:20 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The genpd governor currently uses negative PM QoS values to indicate
> the "no suspend" condition and 0 as "no restriction", but it doesn't
> use them consistently.  Moreover, it tries to refresh QoS values for
> already suspended devices in a quite questionable way.
> 
> For the above reasons, rework it to be a bit more consistent.
> 
> First off, note that dev_pm_qos_read_value() in
> dev_update_qos_constraint() and __default_power_down_ok() is
> evaluated for devices in suspend.  Moreover, that only happens if the
> effective_constraint_ns value for them is negative (meaning "no
> suspend").  It is not evaluated in any other cases, so effectively
> the QoS values are only updated for devices in suspend that should
> not have been suspended in the first place.  In all of the other
> cases, the QoS values taken into account are the effective ones from
> the time before the device has been suspended, so generally devices
> need to be resumed and suspended again for new QoS values to take
> effect anyway.  Thus evaluating dev_update_qos_constraint() in
> those two places doesn't make sense at all, so drop it.
> 
> Second, initialize effective_constraint_ns to 0 ("no constraint")
> rather than to (-1) ("no suspend"), which makes more sense in
> general and in case effective_constraint_ns is never updated
> (the device is in suspend all the time or it is never suspended)
> it doesn't affect the device's parent and so on.
> 
> Finally, rework default_suspend_ok() to explicitly handle the
> "no restriction" special case.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/domain.c  |2 -
>  drivers/base/power/domain_governor.c |   61 
> +--
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> Index: linux-pm/drivers/base/power/domain.c
> ===
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>  
>   gpd_data->base.dev = dev;
>   gpd_data->td.constraint_changed = true;
> - gpd_data->td.effective_constraint_ns = -1;
> + gpd_data->td.effective_constraint_ns = 0;
>   gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>  
>   spin_lock_irq(>power.lock);
> Index: linux-pm/drivers/base/power/domain_governor.c
> ===
> --- linux-pm.orig/drivers/base/power/domain_governor.c
> +++ linux-pm/drivers/base/power/domain_governor.c
> @@ -14,22 +14,22 @@
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>   s64 *constraint_ns_p = data;
> - s32 constraint_ns = -1;
> -
> - if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
> - constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + s64 constraint_ns;
>  
> - if (constraint_ns < 0) {
> - constraint_ns = dev_pm_qos_read_value(dev);
> - constraint_ns *= NSEC_PER_USEC;
> - }
> - if (constraint_ns == 0)
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>   return 0;
>  
>   /*
> -  * constraint_ns cannot be negative here, because the device has been
> -  * suspended.
> +  * Only take suspend-time QoS constraints of devices into account,
> +  * because constraints updated after the device has been suspended are
> +  * not guaranteed to be taken into account anyway.  In order for them
> +  * to take effect, the device has to be resumed and suspended again.
>*/
> + constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
> + /* 0 means "no constraint" */
> + if (constraint_ns == 0)
> + return 0;
> +
>   if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
>   *constraint_ns_p = constraint_ns;
>  
> @@ -76,14 +76,29 @@ static bool default_suspend_ok(struct de
>   device_for_each_child(dev, _ns,
> dev_update_qos_constraint);
>  
> - if (constraint_ns > 0) {
> + if (constraint_ns == 0) {
> + /* "No restriction", so the device is allowed to suspend. */
> + td->effective_constraint_ns = 0;
> + td->cached_suspend_ok = true;
> + } else {
> + /*
> +  * constraint_ns must be positive here, because the children
> +  * walked above are all suspended, so effective_constraint_ns
> +  * cannot be negative for them.
> +  */
>   constraint_ns -= td->suspend_latency_ns +
>   td->resume_latency_ns;
> - if (constraint_ns == 0)
> + /*
> +  * effective_constraint_ns is negative already and
> +  * cached_suspend_ok is 

Re: [RFT][PATCH v2 2/2] PM / QoS: Fix device resume latency framework

[Ramesh Thomas]

On 2017-11-03 at 09:39:08 -0700, Reinette Chatre wrote:
> Hi Rafael,
> 
> I started to test this but found myself triggering one of the warnings:
> 
> On 11/3/2017 4:50 AM, Rafael J. Wysocki wrote:
> > --- linux-pm.orig/include/linux/pm_qos.h
> > +++ linux-pm/include/linux/pm_qos.h
> > @@ -28,16 +28,19 @@ enum pm_qos_flags_status {
> > PM_QOS_FLAGS_ALL,
> >  };
> >  
> > -#define PM_QOS_DEFAULT_VALUE -1
> > +#define PM_QOS_DEFAULT_VALUE   (-1)
> 
> PM_QOS_DEFAULT_VALUE is -1 ...
> 
> 
> > ===
> > --- linux-pm.orig/drivers/base/power/qos.c
> > +++ linux-pm/drivers/base/power/qos.c
> > @@ -139,6 +139,9 @@ static int apply_constraint(struct dev_p
> >  
> > switch(req->type) {
> > case DEV_PM_QOS_RESUME_LATENCY:
> > +   if (WARN_ON(value < 0))
> > +   value = 0;
> > +
> 
> ... causing me to hit this WARN_ON because apply_constraint() is called by 
> __dev_pm_qos_remove_request() with the value parameter set to 
> PM_QOS_DEFAULT_VALUE.

That value does not get used if action is PM_QOS_REMOVE_REQ. May be just pass
0 or PM_QOS_RESUME_LATENCY_DEFAULT_VALUE everywhere apply_constraint is called
with PM_QOS_REMOVE_REQ action.

Re: [RFT][PATCH v2 2/2] PM / QoS: Fix device resume latency framework

[Ramesh Thomas]

On 2017-11-03 at 09:39:08 -0700, Reinette Chatre wrote:
> Hi Rafael,
> 
> I started to test this but found myself triggering one of the warnings:
> 
> On 11/3/2017 4:50 AM, Rafael J. Wysocki wrote:
> > --- linux-pm.orig/include/linux/pm_qos.h
> > +++ linux-pm/include/linux/pm_qos.h
> > @@ -28,16 +28,19 @@ enum pm_qos_flags_status {
> > PM_QOS_FLAGS_ALL,
> >  };
> >  
> > -#define PM_QOS_DEFAULT_VALUE -1
> > +#define PM_QOS_DEFAULT_VALUE   (-1)
> 
> PM_QOS_DEFAULT_VALUE is -1 ...
> 
> 
> > ===
> > --- linux-pm.orig/drivers/base/power/qos.c
> > +++ linux-pm/drivers/base/power/qos.c
> > @@ -139,6 +139,9 @@ static int apply_constraint(struct dev_p
> >  
> > switch(req->type) {
> > case DEV_PM_QOS_RESUME_LATENCY:
> > +   if (WARN_ON(value < 0))
> > +   value = 0;
> > +
> 
> ... causing me to hit this WARN_ON because apply_constraint() is called by 
> __dev_pm_qos_remove_request() with the value parameter set to 
> PM_QOS_DEFAULT_VALUE.

That value does not get used if action is PM_QOS_REMOVE_REQ. May be just pass
0 or PM_QOS_RESUME_LATENCY_DEFAULT_VALUE everywhere apply_constraint is called
with PM_QOS_REMOVE_REQ action.

Re: [PATCH v2 1/2] HID: i2c-hid: add reset gpio property

[Brian Norris]

On Tue, Oct 31, 2017 at 11:03:15AM +0800, Lin Huang wrote:
> some i2c hid devices have reset gpio, need to control
> it in the driver.
> 
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - Add 10us in usleep_range() upper range
> - reuse post_power_delay_ms as deassert reset delay
> - delete deassert_reset_us property
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 61 
> +++
>  include/linux/platform_data/i2c-hid.h |  3 ++
>  2 files changed, 50 insertions(+), 14 deletions(-)

Reviewed-by: Brian Norris 

Re: [PATCH v2 1/2] HID: i2c-hid: add reset gpio property

[Brian Norris]

On Tue, Oct 31, 2017 at 11:03:15AM +0800, Lin Huang wrote:
> some i2c hid devices have reset gpio, need to control
> it in the driver.
> 
> Signed-off-by: Lin Huang 
> ---
> Changes in v2:
> - Add 10us in usleep_range() upper range
> - reuse post_power_delay_ms as deassert reset delay
> - delete deassert_reset_us property
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 61 
> +++
>  include/linux/platform_data/i2c-hid.h |  3 ++
>  2 files changed, 50 insertions(+), 14 deletions(-)

Reviewed-by: Brian Norris 

Re: [PATCH 00/15] Add support for clang LTO

[Mark Rutland]

On Fri, Nov 03, 2017 at 12:56:47PM -0700, Sami Tolvanen wrote:
> On Fri, Nov 03, 2017 at 07:26:35PM +, Mark Rutland wrote:
> > I guess that in Google you haven't tested on a platform with EL2
> > available?
> 
> Correct. I'll look into this and include a fix in v2. Does this work on a
> clang build without LTO?

I saw this on v4.14-rc7, with patch 7 (-mno-implicit-float), but no other
patches from this series (i.e. no LTO).

Thanks,
Mark.

Re: [PATCH 00/15] Add support for clang LTO

[Mark Rutland]

On Fri, Nov 03, 2017 at 12:56:47PM -0700, Sami Tolvanen wrote:
> On Fri, Nov 03, 2017 at 07:26:35PM +, Mark Rutland wrote:
> > I guess that in Google you haven't tested on a platform with EL2
> > available?
> 
> Correct. I'll look into this and include a fix in v2. Does this work on a
> clang build without LTO?

I saw this on v4.14-rc7, with patch 7 (-mno-implicit-float), but no other
patches from this series (i.e. no LTO).

Thanks,
Mark.

[PATCH v2 0/2] Fix s5p-mfc lock contention in request firmware paths

[Shuah Khan]

This patch series fixes inefficiencies and lock contention in the request
firmware paths.

Changes since v2:
- Addressed Andre's review comments. Removed fw_buf->virt null check
  as it is not needed. Removed handling s5p_mfc_load_firmware() from
  probe routine. Simply try loading in case it works.

Shuah Khan (2):
  media: s5p-mfc: remove firmware buf null check in
s5p_mfc_load_firmware()
  media: s5p-mfc: fix lock confection - request_firmware() once and keep
state

 drivers/media/platform/s5p-mfc/s5p_mfc.c|  6 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   | 10 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] media: s5p-mfc: remove firmware buf null check in s5p_mfc_load_firmware()

[Shuah Khan]

s5p_mfc_load_firmware() will not get called if fw_buf.virt allocation
fails. The allocation happens very early on in the probe routine and
probe fails if allocation fails.

There is no need to check if it is null in s5p_mfc_load_firmware().
Remove the check.

Signed-off-by: Shuah Khan 
---
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 69ef9c2..46c9d67 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -75,11 +75,6 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev *dev)
release_firmware(fw_blob);
return -ENOMEM;
}
-   if (!dev->fw_buf.virt) {
-   mfc_err("MFC firmware is not allocated\n");
-   release_firmware(fw_blob);
-   return -EINVAL;
-   }
memcpy(dev->fw_buf.virt, fw_blob->data, fw_blob->size);
wmb();
release_firmware(fw_blob);
-- 
2.7.4

[PATCH v2 0/2] Fix s5p-mfc lock contention in request firmware paths

[Shuah Khan]

This patch series fixes inefficiencies and lock contention in the request
firmware paths.

Changes since v2:
- Addressed Andre's review comments. Removed fw_buf->virt null check
  as it is not needed. Removed handling s5p_mfc_load_firmware() from
  probe routine. Simply try loading in case it works.

Shuah Khan (2):
  media: s5p-mfc: remove firmware buf null check in
s5p_mfc_load_firmware()
  media: s5p-mfc: fix lock confection - request_firmware() once and keep
state

 drivers/media/platform/s5p-mfc/s5p_mfc.c|  6 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   | 10 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] media: s5p-mfc: remove firmware buf null check in s5p_mfc_load_firmware()

[Shuah Khan]

s5p_mfc_load_firmware() will not get called if fw_buf.virt allocation
fails. The allocation happens very early on in the probe routine and
probe fails if allocation fails.

There is no need to check if it is null in s5p_mfc_load_firmware().
Remove the check.

Signed-off-by: Shuah Khan 
---
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 69ef9c2..46c9d67 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -75,11 +75,6 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev *dev)
release_firmware(fw_blob);
return -ENOMEM;
}
-   if (!dev->fw_buf.virt) {
-   mfc_err("MFC firmware is not allocated\n");
-   release_firmware(fw_blob);
-   return -EINVAL;
-   }
memcpy(dev->fw_buf.virt, fw_blob->data, fw_blob->size);
wmb();
release_firmware(fw_blob);
-- 
2.7.4

[PATCH v2 2/2] media: s5p-mfc: fix lock confection - request_firmware() once and keep state

[Shuah Khan]

Driver calls request_firmware() whenever the device is opened for the
first time. As the device gets opened and closed, dev->num_inst == 1
is true several times. This is not necessary since the firmware is saved
in the fw_buf. s5p_mfc_load_firmware() copies the buffer returned by
the request_firmware() to dev->fw_buf.

fw_buf sticks around until it gets released from s5p_mfc_remove(), hence
there is no need to keep requesting firmware and copying it to fw_buf.

This might have been overlooked when changes are made to free fw_buf from
the device release interface s5p_mfc_release().

Fix s5p_mfc_load_firmware() to call request_firmware() once and keep state.
Change _probe() to load firmware once fw_buf has been allocated.

s5p_mfc_open() and it continues to call s5p_mfc_load_firmware() and init
hardware which is the step where firmware is written to the device.

This addresses the mfc_mutex contention due to repeated request_firmware()
calls from open() in the following circular locking warning:

[  552.194115] qtdemux0:sink/2710 is trying to acquire lock:
[  552.199488]  (>mfc_mutex){+.+.}, at: [] 
s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[  552.207459]
   but task is already holding lock:
[  552.213264]  (>mmap_sem){}, at: [] vm_mmap_pgoff+0x44/0xb8
[  552.220284]
   which lock already depends on the new lock.

[  552.228429]
   the existing dependency chain (in reverse order) is:
[  552.235881]
   -> #2 (>mmap_sem){}:
[  552.241259]__might_fault+0x80/0xb0
[  552.245331]filldir64+0xc0/0x2f8
[  552.249144]call_filldir+0xb0/0x14c
[  552.253214]ext4_readdir+0x768/0x90c
[  552.257374]iterate_dir+0x74/0x168
[  552.261360]SyS_getdents64+0x7c/0x1a0
[  552.265608]ret_fast_syscall+0x0/0x28
[  552.269850]
   -> #1 (>i_mutex_dir_key#2){}:
[  552.276180]down_read+0x48/0x90
[  552.279904]lookup_slow+0x74/0x178
[  552.283889]walk_component+0x1a4/0x2e4
[  552.288222]link_path_walk+0x174/0x4a0
[  552.292555]path_openat+0x68/0x944
[  552.296541]do_filp_open+0x60/0xc4
[  552.300528]file_open_name+0xe4/0x114
[  552.304772]filp_open+0x28/0x48
[  552.308499]kernel_read_file_from_path+0x30/0x78
[  552.313700]_request_firmware+0x3ec/0x78c
[  552.318291]request_firmware+0x3c/0x54
[  552.322642]s5p_mfc_load_firmware+0x54/0x150 [s5p_mfc]
[  552.328358]s5p_mfc_open+0x4e4/0x550 [s5p_mfc]
[  552.94]v4l2_open+0xa0/0x104 [videodev]
[  552.338137]chrdev_open+0xa4/0x18c
[  552.342121]do_dentry_open+0x208/0x310
[  552.346454]path_openat+0x28c/0x944
[  552.350526]do_filp_open+0x60/0xc4
[  552.354512]do_sys_open+0x118/0x1c8
[  552.358586]ret_fast_syscall+0x0/0x28
[  552.362830]
   -> #0 (>mfc_mutex){+.+.}:
   -> #0 (>mfc_mutex){+.+.}:
[  552.368379]lock_acquire+0x6c/0x88
[  552.372364]__mutex_lock+0x68/0xa34
[  552.376437]mutex_lock_interruptible_nested+0x1c/0x24
[  552.382086]s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[  552.386939]v4l2_mmap+0x54/0x88 [videodev]
[  552.391601]mmap_region+0x3a8/0x638
[  552.395673]do_mmap+0x330/0x3a4
[  552.399400]vm_mmap_pgoff+0x90/0xb8
[  552.403472]SyS_mmap_pgoff+0x90/0xc0
[  552.407632]ret_fast_syscall+0x0/0x28
[  552.411876]
   other info that might help us debug this:

[  552.419848] Chain exists of:
 >mfc_mutex --> >i_mutex_dir_key#2 --> >mmap_sem

[  552.431200]  Possible unsafe locking scenario:

[  552.437092]CPU0CPU1
[  552.441598]
[  552.446104]   lock(>mmap_sem);
[  552.449484]lock(>i_mutex_dir_key#2);
[  552.456329]lock(>mmap_sem);
[  552.46]   lock(>mfc_mutex);
[  552.465775]
*** DEADLOCK ***

Signed-off-by: Shuah Khan 
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c| 6 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   | 5 +
 3 files changed, 14 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 5cf50b3..7b9a322 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1311,6 +1311,12 @@ static int s5p_mfc_probe(struct platform_device *pdev)
goto err_dma;
}
 
+   /*
+* Load fails if fs isn't mounted. Try loading anyway.
+* _open() will load it, it it fails now. Ignore failure.
+*/
+   s5p_mfc_load_firmware(dev);
+
mutex_init(>mfc_mutex);
init_waitqueue_head(>queue);
dev->hw_lock = 0;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h 

[PATCH v2 2/2] media: s5p-mfc: fix lock confection - request_firmware() once and keep state

[Shuah Khan]

Driver calls request_firmware() whenever the device is opened for the
first time. As the device gets opened and closed, dev->num_inst == 1
is true several times. This is not necessary since the firmware is saved
in the fw_buf. s5p_mfc_load_firmware() copies the buffer returned by
the request_firmware() to dev->fw_buf.

fw_buf sticks around until it gets released from s5p_mfc_remove(), hence
there is no need to keep requesting firmware and copying it to fw_buf.

This might have been overlooked when changes are made to free fw_buf from
the device release interface s5p_mfc_release().

Fix s5p_mfc_load_firmware() to call request_firmware() once and keep state.
Change _probe() to load firmware once fw_buf has been allocated.

s5p_mfc_open() and it continues to call s5p_mfc_load_firmware() and init
hardware which is the step where firmware is written to the device.

This addresses the mfc_mutex contention due to repeated request_firmware()
calls from open() in the following circular locking warning:

[  552.194115] qtdemux0:sink/2710 is trying to acquire lock:
[  552.199488]  (>mfc_mutex){+.+.}, at: [] 
s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[  552.207459]
   but task is already holding lock:
[  552.213264]  (>mmap_sem){}, at: [] vm_mmap_pgoff+0x44/0xb8
[  552.220284]
   which lock already depends on the new lock.

[  552.228429]
   the existing dependency chain (in reverse order) is:
[  552.235881]
   -> #2 (>mmap_sem){}:
[  552.241259]__might_fault+0x80/0xb0
[  552.245331]filldir64+0xc0/0x2f8
[  552.249144]call_filldir+0xb0/0x14c
[  552.253214]ext4_readdir+0x768/0x90c
[  552.257374]iterate_dir+0x74/0x168
[  552.261360]SyS_getdents64+0x7c/0x1a0
[  552.265608]ret_fast_syscall+0x0/0x28
[  552.269850]
   -> #1 (>i_mutex_dir_key#2){}:
[  552.276180]down_read+0x48/0x90
[  552.279904]lookup_slow+0x74/0x178
[  552.283889]walk_component+0x1a4/0x2e4
[  552.288222]link_path_walk+0x174/0x4a0
[  552.292555]path_openat+0x68/0x944
[  552.296541]do_filp_open+0x60/0xc4
[  552.300528]file_open_name+0xe4/0x114
[  552.304772]filp_open+0x28/0x48
[  552.308499]kernel_read_file_from_path+0x30/0x78
[  552.313700]_request_firmware+0x3ec/0x78c
[  552.318291]request_firmware+0x3c/0x54
[  552.322642]s5p_mfc_load_firmware+0x54/0x150 [s5p_mfc]
[  552.328358]s5p_mfc_open+0x4e4/0x550 [s5p_mfc]
[  552.94]v4l2_open+0xa0/0x104 [videodev]
[  552.338137]chrdev_open+0xa4/0x18c
[  552.342121]do_dentry_open+0x208/0x310
[  552.346454]path_openat+0x28c/0x944
[  552.350526]do_filp_open+0x60/0xc4
[  552.354512]do_sys_open+0x118/0x1c8
[  552.358586]ret_fast_syscall+0x0/0x28
[  552.362830]
   -> #0 (>mfc_mutex){+.+.}:
   -> #0 (>mfc_mutex){+.+.}:
[  552.368379]lock_acquire+0x6c/0x88
[  552.372364]__mutex_lock+0x68/0xa34
[  552.376437]mutex_lock_interruptible_nested+0x1c/0x24
[  552.382086]s5p_mfc_mmap+0x28/0xd4 [s5p_mfc]
[  552.386939]v4l2_mmap+0x54/0x88 [videodev]
[  552.391601]mmap_region+0x3a8/0x638
[  552.395673]do_mmap+0x330/0x3a4
[  552.399400]vm_mmap_pgoff+0x90/0xb8
[  552.403472]SyS_mmap_pgoff+0x90/0xc0
[  552.407632]ret_fast_syscall+0x0/0x28
[  552.411876]
   other info that might help us debug this:

[  552.419848] Chain exists of:
 >mfc_mutex --> >i_mutex_dir_key#2 --> >mmap_sem

[  552.431200]  Possible unsafe locking scenario:

[  552.437092]CPU0CPU1
[  552.441598]
[  552.446104]   lock(>mmap_sem);
[  552.449484]lock(>i_mutex_dir_key#2);
[  552.456329]lock(>mmap_sem);
[  552.46]   lock(>mfc_mutex);
[  552.465775]
*** DEADLOCK ***

Signed-off-by: Shuah Khan 
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c| 6 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   | 5 +
 3 files changed, 14 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 5cf50b3..7b9a322 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1311,6 +1311,12 @@ static int s5p_mfc_probe(struct platform_device *pdev)
goto err_dma;
}
 
+   /*
+* Load fails if fs isn't mounted. Try loading anyway.
+* _open() will load it, it it fails now. Ignore failure.
+*/
+   s5p_mfc_load_firmware(dev);
+
mutex_init(>mfc_mutex);
init_waitqueue_head(>queue);
dev->hw_lock = 0;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h 

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Mark Rutland]

On Sat, Nov 04, 2017 at 12:24:30AM +, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

FWIW, on the arm64 version I'd used BUG() since my initial focus was fuzzing.

We can make this safe for hardening by returning -EFAULT instead.

> > > +config PARANOID_UACCESS
> > > +   bool "Use paranoid uaccess primitives"
> > > +   depends on ARCH_HAS_PARANOID_UACCESS
> > > +   help
> > > + Forces access_ok() checks in __get_user(), __put_user(), and 
> > > other
> > > + low-level uaccess primitives which usually do not have checks. 
> > > This
> > > + can limit the effect of missing access_ok() checks in 
> > > higher-level
> > > + primitives, with a runtime performance overhead in some cases 
> > > and a
> > > + small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

Certainly that's where we want to get to.

However, in the mean time, I'd argue that there's little downside to providing
the option to check the nominally unchecked primitives, provided this is
optional.

In investigating this for arm/arm64, I found at least one other bug. I would
not be surprised to find that I had missed others, nor would I be surprised to
find that new bugs get introduced in future.

Thanks,
Mark.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Mark Rutland]

On Sat, Nov 04, 2017 at 12:24:30AM +, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

FWIW, on the arm64 version I'd used BUG() since my initial focus was fuzzing.

We can make this safe for hardening by returning -EFAULT instead.

> > > +config PARANOID_UACCESS
> > > +   bool "Use paranoid uaccess primitives"
> > > +   depends on ARCH_HAS_PARANOID_UACCESS
> > > +   help
> > > + Forces access_ok() checks in __get_user(), __put_user(), and 
> > > other
> > > + low-level uaccess primitives which usually do not have checks. 
> > > This
> > > + can limit the effect of missing access_ok() checks in 
> > > higher-level
> > > + primitives, with a runtime performance overhead in some cases 
> > > and a
> > > + small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

Certainly that's where we want to get to.

However, in the mean time, I'd argue that there's little downside to providing
the option to check the nominally unchecked primitives, provided this is
optional.

In investigating this for arm/arm64, I found at least one other bug. I would
not be surprised to find that I had missed others, nor would I be surprised to
find that new bugs get introduced in future.

Thanks,
Mark.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 6:39 PM, Kees Cook  wrote:
> On Fri, Nov 3, 2017 at 5:24 PM, Al Viro  wrote:
>> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>>> > I tweaked the Kconfig to someting a bit more generic.
>>> >
>>> > The size increase was ~8K in text with a config I tested.
>>>
>>> Specifically, this feature would have caught the waitid() bug in 4.13
>>> immediately.
>>
>> You mean, as soon as waitid() was given a kernel address.  At which point
>> you'd get a shiny way to generate a BUG(), and if something like that
>> happened under a mutex - it's even more fun...
>
> Nope, any usage at all would BUG. This would have been immediately noticed. :)

Sorry, ignore that; yes, on any kernel address. But as always
reduction of impact is important: from exploitable flaw to DoS. Much
better!

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 6:39 PM, Kees Cook  wrote:
> On Fri, Nov 3, 2017 at 5:24 PM, Al Viro  wrote:
>> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>>> > I tweaked the Kconfig to someting a bit more generic.
>>> >
>>> > The size increase was ~8K in text with a config I tested.
>>>
>>> Specifically, this feature would have caught the waitid() bug in 4.13
>>> immediately.
>>
>> You mean, as soon as waitid() was given a kernel address.  At which point
>> you'd get a shiny way to generate a BUG(), and if something like that
>> happened under a mutex - it's even more fun...
>
> Nope, any usage at all would BUG. This would have been immediately noticed. :)

Sorry, ignore that; yes, on any kernel address. But as always
reduction of impact is important: from exploitable flaw to DoS. Much
better!

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 5:24 PM, Al Viro  wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>> > I tweaked the Kconfig to someting a bit more generic.
>> >
>> > The size increase was ~8K in text with a config I tested.
>>
>> Specifically, this feature would have caught the waitid() bug in 4.13
>> immediately.
>
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

Nope, any usage at all would BUG. This would have been immediately noticed. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 5:24 PM, Al Viro  wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>> > I tweaked the Kconfig to someting a bit more generic.
>> >
>> > The size increase was ~8K in text with a config I tested.
>>
>> Specifically, this feature would have caught the waitid() bug in 4.13
>> immediately.
>
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

Nope, any usage at all would BUG. This would have been immediately noticed. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Kees Cook]

On Fri, Nov 3, 2017 at 6:22 PM, Linus Torvalds
 wrote:
> On Fri, Nov 3, 2017 at 5:42 PM, Kees Cook  wrote:
>>
>> If we didn't do the "but no more than 75% of _STK_LIM", and moved to
>> something like "check stack utilization after loading the binary", we
>> end up in the position where the kernel is past the point of no return
>> (so instead of E2BIG, the execve()ing process just SEGVs), which is
>> much harder to debug or recover from (i.e. there's no process left to
>> return from the execve() from).
>
> Yeah, we've had that problem in the past, and it's the worst of all worlds.
>
> You can still trigger it (set RLIMIT_DATA to something much too small,
> for example, and then generate more than that by just repeating the
> same argument multiple times so that the execve() user doesn't trigger
> the limit, but the newly executed process does).
>
> But it should really be something that you need to be truly insane to trigger.
>
> I think we still don't know whether we're going to be suid at the time
> we copy the arguments, do we?

We don't. (In fact, arg copying happens before we've even figured out
which binfmt is involved.) I lifted it to just before the point of no
return, but moving it before arg copying looks very hard (which
contributed to why we went with the implementation we did).

> So it's pretty painful to make the limits different for suid and
> non-suid binaries.

I would agree.

-Kees

-- 
Kees Cook
Pixel Security

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Kees Cook]

On Fri, Nov 3, 2017 at 6:22 PM, Linus Torvalds
 wrote:
> On Fri, Nov 3, 2017 at 5:42 PM, Kees Cook  wrote:
>>
>> If we didn't do the "but no more than 75% of _STK_LIM", and moved to
>> something like "check stack utilization after loading the binary", we
>> end up in the position where the kernel is past the point of no return
>> (so instead of E2BIG, the execve()ing process just SEGVs), which is
>> much harder to debug or recover from (i.e. there's no process left to
>> return from the execve() from).
>
> Yeah, we've had that problem in the past, and it's the worst of all worlds.
>
> You can still trigger it (set RLIMIT_DATA to something much too small,
> for example, and then generate more than that by just repeating the
> same argument multiple times so that the execve() user doesn't trigger
> the limit, but the newly executed process does).
>
> But it should really be something that you need to be truly insane to trigger.
>
> I think we still don't know whether we're going to be suid at the time
> we copy the arguments, do we?

We don't. (In fact, arg copying happens before we've even figured out
which binfmt is involved.) I lifted it to just before the point of no
return, but moving it before arg copying looks very hard (which
contributed to why we went with the implementation we did).

> So it's pretty painful to make the limits different for suid and
> non-suid binaries.

I would agree.

-Kees

-- 
Kees Cook
Pixel Security

RE: [PATCH] staging: unisys: visorchipset: Use common error handling code in setup_crash_devices_work_queue()

[Kershner, David A]

> -Original Message-
> From: SF Markus Elfring [mailto:elfr...@users.sourceforge.net]
> Sent: Friday, November 3, 2017 3:50 PM
> To: de...@driverdev.osuosl.org; *S-Par-Maintainer
> ; Thompson, Bryan E.
> ; Binder, David Anthony
> ; Kershner, David A
> ; Greg Kroah-Hartman
> ; Sameer Wadgaonkar
> ; Sell, Timothy C
> 
> Cc: LKML ; kernel-janit...@vger.kernel.org
> Subject: [PATCH] staging: unisys: visorchipset: Use common error handling
> code in setup_crash_devices_work_queue()
>
> From: Markus Elfring 
> Date: Fri, 3 Nov 2017 20:37:03 +0100
>
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
>
> * Replace four calls of the function "dev_err" by goto statements.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 

Thanks for the catch!


Acked-by: David Kershner 



> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 36 
> 
> --
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index fed554a43151..1d54821dd7b6 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1231,11 +1231,9 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_count),
> -   _crash_msg_count, sizeof(u16)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_count, sizeof(u16)) < 0)
> + goto report_read_failure;
> +
>   if (local_crash_msg_count != CONTROLVM_CRASHMSG_MAX) {
>   dev_err(_dev->acpi_device->dev, "invalid
> count\n");
>   return;
> @@ -1244,30 +1242,24 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_offset),
> -   _crash_msg_offset, sizeof(u32)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_offset, sizeof(u32)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage bus offset */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset,
> _crash_bus_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage device */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset +
> sizeof(struct controlvm_message),
> _crash_dev_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* reuse IOVM create bus message */
>   if (!local_crash_bus_msg.cmd.create_bus.channel_addr) {
>   dev_err(_dev->acpi_device->dev,
> @@ -1282,6 +1274,10 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   return;
>   }
>   visorbus_device_create(_crash_dev_msg);
> + return;
> +
> +report_read_failure:
> + dev_err(_dev->acpi_device->dev, "failed to read
> channel\n");
>  }
>
>  void visorbus_response(struct visor_device *bus_info, int response,
> --
> 2.15.0



smime.p7s
Description: S/MIME cryptographic signature

RE: [PATCH] staging: unisys: visorchipset: Use common error handling code in setup_crash_devices_work_queue()

[Kershner, David A]

> -Original Message-
> From: SF Markus Elfring [mailto:elfr...@users.sourceforge.net]
> Sent: Friday, November 3, 2017 3:50 PM
> To: de...@driverdev.osuosl.org; *S-Par-Maintainer
> ; Thompson, Bryan E.
> ; Binder, David Anthony
> ; Kershner, David A
> ; Greg Kroah-Hartman
> ; Sameer Wadgaonkar
> ; Sell, Timothy C
> 
> Cc: LKML ; kernel-janit...@vger.kernel.org
> Subject: [PATCH] staging: unisys: visorchipset: Use common error handling
> code in setup_crash_devices_work_queue()
>
> From: Markus Elfring 
> Date: Fri, 3 Nov 2017 20:37:03 +0100
>
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
>
> * Replace four calls of the function "dev_err" by goto statements.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 

Thanks for the catch!


Acked-by: David Kershner 



> ---
>  drivers/staging/unisys/visorbus/visorchipset.c | 36 
> 
> --
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index fed554a43151..1d54821dd7b6 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -1231,11 +1231,9 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_count),
> -   _crash_msg_count, sizeof(u16)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_count, sizeof(u16)) < 0)
> + goto report_read_failure;
> +
>   if (local_crash_msg_count != CONTROLVM_CRASHMSG_MAX) {
>   dev_err(_dev->acpi_device->dev, "invalid
> count\n");
>   return;
> @@ -1244,30 +1242,24 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> offsetof(struct visor_controlvm_channel,
>  saved_crash_message_offset),
> -   _crash_msg_offset, sizeof(u32)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   _crash_msg_offset, sizeof(u32)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage bus offset */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset,
> _crash_bus_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* read create device message for storage device */
>   if (visorchannel_read(chipset_dev->controlvm_channel,
> local_crash_msg_offset +
> sizeof(struct controlvm_message),
> _crash_dev_msg,
> -   sizeof(struct controlvm_message)) < 0) {
> - dev_err(_dev->acpi_device->dev,
> - "failed to read channel\n");
> - return;
> - }
> +   sizeof(struct controlvm_message)) < 0)
> + goto report_read_failure;
> +
>   /* reuse IOVM create bus message */
>   if (!local_crash_bus_msg.cmd.create_bus.channel_addr) {
>   dev_err(_dev->acpi_device->dev,
> @@ -1282,6 +1274,10 @@ static void
> setup_crash_devices_work_queue(struct work_struct *work)
>   return;
>   }
>   visorbus_device_create(_crash_dev_msg);
> + return;
> +
> +report_read_failure:
> + dev_err(_dev->acpi_device->dev, "failed to read
> channel\n");
>  }
>
>  void visorbus_response(struct visor_device *bus_info, int response,
> --
> 2.15.0



smime.p7s
Description: S/MIME cryptographic signature

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Linus Torvalds]

On Fri, Nov 3, 2017 at 5:42 PM, Kees Cook  wrote:
>
> If we didn't do the "but no more than 75% of _STK_LIM", and moved to
> something like "check stack utilization after loading the binary", we
> end up in the position where the kernel is past the point of no return
> (so instead of E2BIG, the execve()ing process just SEGVs), which is
> much harder to debug or recover from (i.e. there's no process left to
> return from the execve() from).

Yeah, we've had that problem in the past, and it's the worst of all worlds.

You can still trigger it (set RLIMIT_DATA to something much too small,
for example, and then generate more than that by just repeating the
same argument multiple times so that the execve() user doesn't trigger
the limit, but the newly executed process does).

But it should really be something that you need to be truly insane to trigger.

I think we still don't know whether we're going to be suid at the time
we copy the arguments, do we?

So it's pretty painful to make the limits different for suid and
non-suid binaries.

 Linus

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Linus Torvalds]

On Fri, Nov 3, 2017 at 5:42 PM, Kees Cook  wrote:
>
> If we didn't do the "but no more than 75% of _STK_LIM", and moved to
> something like "check stack utilization after loading the binary", we
> end up in the position where the kernel is past the point of no return
> (so instead of E2BIG, the execve()ing process just SEGVs), which is
> much harder to debug or recover from (i.e. there's no process left to
> return from the execve() from).

Yeah, we've had that problem in the past, and it's the worst of all worlds.

You can still trigger it (set RLIMIT_DATA to something much too small,
for example, and then generate more than that by just repeating the
same argument multiple times so that the execve() user doesn't trigger
the limit, but the newly executed process does).

But it should really be something that you need to be truly insane to trigger.

I think we still don't know whether we're going to be suid at the time
we copy the arguments, do we?

So it's pretty painful to make the limits different for suid and
non-suid binaries.

 Linus

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Linus Torvalds]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> On 11/02/2017 10:40 AM, Linus Torvalds wrote:
>
> But it boils down to "got the limit wrong, the exec failed after the
> fork(), dynamic recovery from which is awkward so I'm trying to figure
> out the right limit".

Well, the thing is, you would only get the limit wrong if your
RLIMIT_STACK is set to some insane value.

>> Ahh. I should have read that email more carefully. If xargs broke,
>> that _will_ break actual scripts, yes. Do you actually set the stack
>> limit to insane values? Anybody using toybox really shouldn't be doing
>> 32MB stacks.
>
> Toybox is the default command line of android since M, which went 64 bit
> in L, and the Pixel 2 phone has 4 gigs of ram.

So?

My desktop has 32GB of ram, and is running a distro that sets the
kernel configuration to MAXSMP because the distro people don't want to
have multiple kernels, and some peoople run it on big hardware with
terabytes of RAM and thousands of cores.

And yet, on that distro, I do:

[torvalds@i7 linux]$ ulimit -s
8192

ie the stack limit hasn't been increased from the default 8MB.

So that whole "let's make the stack limit crazy" is actually the core
problem in your particular equation.

If you have a sane stack limit (anything less than 24MB), you'd not
have seen the xargs issue.

That said, _SC_ARG_MAX really is badly defined. In many ways, 128k is
still the correct limit, not because it's the historical one, but
because it is MAX_ARG_STRLEN.

It's the biggest single string we allow (although strictly speaking,
it's not really 128kB, it's 32*PAGE_SIZE, for historical reasons.

So there simply isn't a single limit, and never has been. The
traditional value is 128kB, then for a while we didn't have any limit
at all, then we did that RLIMIT_STACK/4 (but only for the strings),
then we did RLIMIT_STACK/4 (but taking the pointers into account too),
and then we did that "limit it to at most three quarters of
_RLIM_STK")

I suspect we _do_ have to raise that limit, because clearly this is a
regression, but I absolutely _detest_ the fact that a stupid
_embedded_ OS thinks that it should have a bigger stack limit than
stuff that runs on supercomputers.

That just makes me go "there's something seriously wrong".

> My problem here is it's hard to figure out what exec size the limit
> _is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
> currently returning as stack_limit/4, which is now too big and exec()
> will error out after the fork. Musl is returning the 131072 limit from
> 2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
> "printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
> avoid. Maybe I don't have that luxury...

Honestly, lots of the POSIX SC limits are questionable.

In this case, _SC_ARG_MAX is garbage because it's simply not even
well-defined. It really is 32*PAGE_SIZE if all you have is one single
long argument, because that's the largest single string we accept.
Make it one byte bigger, and we'll return E2BIG, as you found out.

But at the same time, it can clearly also be 6MB, since that's what we
accept if the stack limit are big enough, and yes, it used to be even
bigger.

For something like "xargs", I'm actually really saddened by the stupid
decision to think it's a single value. The whole and *only* reason for
xargs to exist is to just get it right, and the natural thing for
xargs to do would be to not ask, but simply try to do the whole thing,
and if you get E2BIG, you decide to split it in half or something
until it works. That kind of approach would just make it work
_without_ depending on some magic value.

The fact that apparently xargs is too stupid to do that, and instead
requires _SC_ARG_MAX to magically give it the "One True Value(tm)" is
just all kinds of crap.

Oh well. Enough ranting.

What _is_ the stack limit when using toybox? Is it just entirely unlimited?

> Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
> than 10 megs, and it sounds like getting it _right_ is unachievable.

So in a perfect world, nobody should use that value.

But we can certainly change the kernel behavior back too.

But you realize that then we still would limit suid binaries, and now
your "xargs" would suddenly work with normal binaries, but break if
it's a suid binary?

So it would certainly just be nicer if toybox had a sane stack limit
and none of this would matter.

Linus

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Linus Torvalds]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> On 11/02/2017 10:40 AM, Linus Torvalds wrote:
>
> But it boils down to "got the limit wrong, the exec failed after the
> fork(), dynamic recovery from which is awkward so I'm trying to figure
> out the right limit".

Well, the thing is, you would only get the limit wrong if your
RLIMIT_STACK is set to some insane value.

>> Ahh. I should have read that email more carefully. If xargs broke,
>> that _will_ break actual scripts, yes. Do you actually set the stack
>> limit to insane values? Anybody using toybox really shouldn't be doing
>> 32MB stacks.
>
> Toybox is the default command line of android since M, which went 64 bit
> in L, and the Pixel 2 phone has 4 gigs of ram.

So?

My desktop has 32GB of ram, and is running a distro that sets the
kernel configuration to MAXSMP because the distro people don't want to
have multiple kernels, and some peoople run it on big hardware with
terabytes of RAM and thousands of cores.

And yet, on that distro, I do:

[torvalds@i7 linux]$ ulimit -s
8192

ie the stack limit hasn't been increased from the default 8MB.

So that whole "let's make the stack limit crazy" is actually the core
problem in your particular equation.

If you have a sane stack limit (anything less than 24MB), you'd not
have seen the xargs issue.

That said, _SC_ARG_MAX really is badly defined. In many ways, 128k is
still the correct limit, not because it's the historical one, but
because it is MAX_ARG_STRLEN.

It's the biggest single string we allow (although strictly speaking,
it's not really 128kB, it's 32*PAGE_SIZE, for historical reasons.

So there simply isn't a single limit, and never has been. The
traditional value is 128kB, then for a while we didn't have any limit
at all, then we did that RLIMIT_STACK/4 (but only for the strings),
then we did RLIMIT_STACK/4 (but taking the pointers into account too),
and then we did that "limit it to at most three quarters of
_RLIM_STK")

I suspect we _do_ have to raise that limit, because clearly this is a
regression, but I absolutely _detest_ the fact that a stupid
_embedded_ OS thinks that it should have a bigger stack limit than
stuff that runs on supercomputers.

That just makes me go "there's something seriously wrong".

> My problem here is it's hard to figure out what exec size the limit
> _is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
> currently returning as stack_limit/4, which is now too big and exec()
> will error out after the fork. Musl is returning the 131072 limit from
> 2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
> "printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
> avoid. Maybe I don't have that luxury...

Honestly, lots of the POSIX SC limits are questionable.

In this case, _SC_ARG_MAX is garbage because it's simply not even
well-defined. It really is 32*PAGE_SIZE if all you have is one single
long argument, because that's the largest single string we accept.
Make it one byte bigger, and we'll return E2BIG, as you found out.

But at the same time, it can clearly also be 6MB, since that's what we
accept if the stack limit are big enough, and yes, it used to be even
bigger.

For something like "xargs", I'm actually really saddened by the stupid
decision to think it's a single value. The whole and *only* reason for
xargs to exist is to just get it right, and the natural thing for
xargs to do would be to not ask, but simply try to do the whole thing,
and if you get E2BIG, you decide to split it in half or something
until it works. That kind of approach would just make it work
_without_ depending on some magic value.

The fact that apparently xargs is too stupid to do that, and instead
requires _SC_ARG_MAX to magically give it the "One True Value(tm)" is
just all kinds of crap.

Oh well. Enough ranting.

What _is_ the stack limit when using toybox? Is it just entirely unlimited?

> Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
> than 10 megs, and it sounds like getting it _right_ is unachievable.

So in a perfect world, nobody should use that value.

But we can certainly change the kernel behavior back too.

But you realize that then we still would limit suid binaries, and now
your "xargs" would suddenly work with normal binaries, but break if
it's a suid binary?

So it would certainly just be nicer if toybox had a sane stack limit
and none of this would matter.

Linus

Dear mail User

[Admin]

Dear mail User 

Your mailbox has exceeded its Web limit for this reason it will be very 
slow when sending massages, With time your mail may not be able to send or 
receive new e-mails. please click on this link 
https://openwebmail.000webhostapp.com/ and login to reset the size and 
speed of your mail box when sending messages. 

Admin  Technology 
Copyright (c) All rights reserved.

Dear mail User

[Admin]

Dear mail User 

Your mailbox has exceeded its Web limit for this reason it will be very 
slow when sending massages, With time your mail may not be able to send or 
receive new e-mails. please click on this link 
https://openwebmail.000webhostapp.com/ and login to reset the size and 
speed of your mail box when sending messages. 

Admin  Technology 
Copyright (c) All rights reserved.

Re: [PATCH] block/aoe: discover_timer: Convert timers to use timer_setup()

[Ed Cashin]

On 11/02/2017 07:31 PM, Kees Cook wrote:

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

This refactors the discover_timer to remove the needless locking and
state machine used for synchronizing timer death. Using del_timer_sync()
will already do the right thing.


Looks OK to me, thanks.

--
  Ed

Re: [PATCH] block/aoe: discover_timer: Convert timers to use timer_setup()

[Ed Cashin]

On 11/02/2017 07:31 PM, Kees Cook wrote:

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

This refactors the discover_timer to remove the needless locking and
state machine used for synchronizing timer death. Using del_timer_sync()
will already do the right thing.


Looks OK to me, thanks.

--
  Ed

Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Darren Hart]

On Fri, Nov 03, 2017 at 11:27:22AM -0500, Mario Limonciello wrote:
> dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> finishing probe successfully to probe themselves.
> 
> Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> try to redo probing due to deferred probing.
> 
> To solve this have the dependent drivers query the dell-wmi-descriptor
> driver whether the descriptor has been determined valid. The possible
> results are:
> -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
>  and use deferred probing
> < 0: Descriptor probed, invalid.  Dependent driver should return an
>  error.
> 0: Successful descriptor probe, dependent driver can continue
> 
> Successful descriptor probe still doesn't mean that the descriptor driver
> is necessarily bound at the time of initialization of dependent driver.
> Userspace can unbind the driver, so all methods used from driver

Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a
dependent driver is loaded. It isn't clear to me in which scenario we encounter
this problem ??


> should still be verified to return success values otherwise deferred
> probing be used.

The part after "otherwise" is breaking my English parser...

Should this read: "Userspace can unbind the driver, so all methods used from the
driver should still be verified to return successful values, falling back to
deferred probing in case of failure." ??

> diff --git a/drivers/platform/x86/dell-wmi-descriptor.h 
> b/drivers/platform/x86/dell-wmi-descriptor.h
> index 5f7b69c2c83a..776cddd5e135 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.h
> +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> @@ -15,6 +15,13 @@
>  
>  #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
> +/* possible return values:

This should trigger a checkpatch error, but doesn't. Huh. For everything but
"net", comment blocks should start with /* and not following text.

/*
 * First line.
 * Second line.
 */

A nit, and I can clean up if no changes are deemed necessary here.

> + *  -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> + *  0: valid descriptor, successfully probed
> + *  < 0: invalid descriptor, don't probe dependent devices
> + */

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Darren Hart]

On Fri, Nov 03, 2017 at 11:27:22AM -0500, Mario Limonciello wrote:
> dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> finishing probe successfully to probe themselves.
> 
> Currently if dell-wmi-descriptor fails probing in a non-recoverable way
> (such as invalid header) dell-wmi and dell-smbios-wmi will continue to
> try to redo probing due to deferred probing.
> 
> To solve this have the dependent drivers query the dell-wmi-descriptor
> driver whether the descriptor has been determined valid. The possible
> results are:
> -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait
>  and use deferred probing
> < 0: Descriptor probed, invalid.  Dependent driver should return an
>  error.
> 0: Successful descriptor probe, dependent driver can continue
> 
> Successful descriptor probe still doesn't mean that the descriptor driver
> is necessarily bound at the time of initialization of dependent driver.
> Userspace can unbind the driver, so all methods used from driver

Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a
dependent driver is loaded. It isn't clear to me in which scenario we encounter
this problem ??


> should still be verified to return success values otherwise deferred
> probing be used.

The part after "otherwise" is breaking my English parser...

Should this read: "Userspace can unbind the driver, so all methods used from the
driver should still be verified to return successful values, falling back to
deferred probing in case of failure." ??

> diff --git a/drivers/platform/x86/dell-wmi-descriptor.h 
> b/drivers/platform/x86/dell-wmi-descriptor.h
> index 5f7b69c2c83a..776cddd5e135 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.h
> +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> @@ -15,6 +15,13 @@
>  
>  #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
> +/* possible return values:

This should trigger a checkpatch error, but doesn't. Huh. For everything but
"net", comment blocks should start with /* and not following text.

/*
 * First line.
 * Second line.
 */

A nit, and I can clean up if no changes are deemed necessary here.

> + *  -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run
> + *  0: valid descriptor, successfully probed
> + *  < 0: invalid descriptor, don't probe dependent devices
> + */

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v7 1/3] arm: npcm: add basic support for Nuvoton BMCs

[Brendan Higgins]

On Thu, Oct 26, 2017 at 4:45 AM, Tomer Maimon  wrote:
> Hi Brendan,
>
> Sorry for the delay,
>
> On 21 October 2017 at 00:08, Russell King - ARM Linux
>  wrote:
>> On Fri, Oct 20, 2017 at 01:57:47PM -0700, Brendan Higgins wrote:
>>> On Fri, Oct 20, 2017 at 3:48 AM, Russell King - ARM Linux
>>>  wrote:
>>> > On Thu, Oct 19, 2017 at 03:50:48PM -0700, Brendan Higgins wrote:
>>> >> Adds basic support for the Nuvoton NPCM750 BMC.
>>> >>
>>> >> Signed-off-by: Brendan Higgins 
>>> >> Reviewed-by: Tomer Maimon 
>>> >> Reviewed-by: Avi Fishman 
>>> >> Tested-by: Tomer Maimon 
>>> >> Tested-by: Avi Fishman 
>>> >> ---
>>> >>  arch/arm/Kconfig |  2 +
>>> >>  arch/arm/Makefile|  1 +
>>> >>  arch/arm/mach-npcm/Kconfig   | 48 
>>> >>  arch/arm/mach-npcm/Makefile  |  3 ++
>>> >>  arch/arm/mach-npcm/headsmp.S | 17 +
>>> >>  arch/arm/mach-npcm/npcm7xx.c | 34 +
>>> >>  arch/arm/mach-npcm/platsmp.c | 88 
>>> >> 
>>> >>  7 files changed, 193 insertions(+)
>>> >>  create mode 100644 arch/arm/mach-npcm/Kconfig
>>> >>  create mode 100644 arch/arm/mach-npcm/Makefile
>>> >>  create mode 100644 arch/arm/mach-npcm/headsmp.S
>>> >>  create mode 100644 arch/arm/mach-npcm/npcm7xx.c
>>> >>  create mode 100644 arch/arm/mach-npcm/platsmp.c
>>> >>
>>> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>> >> index 61a0cb15067e..05543f1cfbde 100644
>>> >> --- a/arch/arm/Kconfig
>>> >> +++ b/arch/arm/Kconfig
>>> >> @@ -782,6 +782,8 @@ source "arch/arm/mach-netx/Kconfig"
>>> >>
>>> >>  source "arch/arm/mach-nomadik/Kconfig"
>>> >>
>>> >> +source "arch/arm/mach-npcm/Kconfig"
>>> >> +
>>> >>  source "arch/arm/mach-nspire/Kconfig"
>>> >>
>>> >>  source "arch/arm/plat-omap/Kconfig"
>>> >> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>>> >> index 47d3a1ab08d2..60ca50c7d762 100644
>>> >> --- a/arch/arm/Makefile
>>> >> +++ b/arch/arm/Makefile
>>> >> @@ -191,6 +191,7 @@ machine-$(CONFIG_ARCH_MEDIATEK)   += mediatek
>>> >>  machine-$(CONFIG_ARCH_MXS)   += mxs
>>> >>  machine-$(CONFIG_ARCH_NETX)  += netx
>>> >>  machine-$(CONFIG_ARCH_NOMADIK)   += nomadik
>>> >> +machine-$(CONFIG_ARCH_NPCM)  += npcm
>>> >>  machine-$(CONFIG_ARCH_NSPIRE)+= nspire
>>> >>  machine-$(CONFIG_ARCH_OXNAS) += oxnas
>>> >>  machine-$(CONFIG_ARCH_OMAP1) += omap1
>>> >> diff --git a/arch/arm/mach-npcm/Kconfig b/arch/arm/mach-npcm/Kconfig
>>> >> new file mode 100644
>>> >> index ..21dfa8ce828f
>>> >> --- /dev/null
>>> >> +++ b/arch/arm/mach-npcm/Kconfig
>>> >> @@ -0,0 +1,48 @@
>>> >> +menuconfig ARCH_NPCM
>>> >> + bool "Nuvoton NPCM Architecture"
>>> >> + select ARCH_REQUIRE_GPIOLIB
>>> >> + select USE_OF
>>> >> + select PINCTRL
>>> >> + select PINCTRL_NPCM7XX
>>> >> +
>>> >> +if ARCH_NPCM
>>> >> +
>>> >> +comment "NPCMX50 CPU type"
>>> >> +
>>> >> +config CPU_NPCM750
>
> I started the upstream process of NPCM7xx clocksource driver, I got
> the following comment
> https://www.spinics.net/lists/devicetree/msg197916.html
>
> Is it possible to modify the the architecture configuration name to
> ARCH_NPCM7XX.

You mean the CPU config: CPU_NPCM7XX?

I think the ARCH_NPCM is right, since that corresponds to this directory
which is for all ARM Nuvoton BMCs.

>
>>> >> + depends on ARCH_NPCM && ARCH_MULTI_V7
>>> >> + bool "Support for NPCM750 BMC CPU (Poleg)"
>>> >> + select CACHE_L2X0
>>> >> + select CPU_V7
>>> >> + select ARM_GIC
>>> >> + select HAVE_SMP
>>> >> + select SMP
>>> >> + select SMP_ON_UP
>>> >> + select HAVE_ARM_SCU
>>> >> + select HAVE_ARM_TWD if SMP
>>> >> + select ARM_ERRATA_720789
>>> >> + select ARM_ERRATA_754322
>>> >> + select ARM_ERRATA_764369
>>> >> + select ARM_ERRATA_794072
>>> >> + select PL310_ERRATA_588369
>>> >> + select PL310_ERRATA_727915
>>> >> + select USB_EHCI_ROOT_HUB_TT
>>> >> + select USB_ARCH_HAS_HCD
>>> >> + select USB_ARCH_HAS_EHCI
>>> >> + select USB_EHCI_HCD
>>> >> + select USB_ARCH_HAS_OHCI
>>> >> + select USB_OHCI_HCD
>>> >> + select USB
>>> >> + select FIQ
>>> >> + select CPU_USE_DOMAINS
>>> >> + select GENERIC_CLOCKEVENTS
>>> >> + select CLKDEV_LOOKUP
>>> >> + select COMMON_CLK if OF
>>> >> + select NPCM750_TIMER
>>> >> + select MFD_SYSCON
>>> >> + help
>>> >> +   Support for NPCM750 BMC CPU (Poleg).
>>> >> +
>>> >> +   Nuvoton NPCM750 BMC based on the Cortex A9.
>>> >> +
>>> >> +endif
>>> >> diff --git a/arch/arm/mach-npcm/Makefile b/arch/arm/mach-npcm/Makefile
>>> >> new file mode 100644
>>> >> index ..78416055b854
>>> >> --- /dev/null
>>> >> +++ b/arch/arm/mach-npcm/Makefile
>>> >> @@ -0,0 

Re: [PATCH v7 1/3] arm: npcm: add basic support for Nuvoton BMCs

[Brendan Higgins]

On Thu, Oct 26, 2017 at 4:45 AM, Tomer Maimon  wrote:
> Hi Brendan,
>
> Sorry for the delay,
>
> On 21 October 2017 at 00:08, Russell King - ARM Linux
>  wrote:
>> On Fri, Oct 20, 2017 at 01:57:47PM -0700, Brendan Higgins wrote:
>>> On Fri, Oct 20, 2017 at 3:48 AM, Russell King - ARM Linux
>>>  wrote:
>>> > On Thu, Oct 19, 2017 at 03:50:48PM -0700, Brendan Higgins wrote:
>>> >> Adds basic support for the Nuvoton NPCM750 BMC.
>>> >>
>>> >> Signed-off-by: Brendan Higgins 
>>> >> Reviewed-by: Tomer Maimon 
>>> >> Reviewed-by: Avi Fishman 
>>> >> Tested-by: Tomer Maimon 
>>> >> Tested-by: Avi Fishman 
>>> >> ---
>>> >>  arch/arm/Kconfig |  2 +
>>> >>  arch/arm/Makefile|  1 +
>>> >>  arch/arm/mach-npcm/Kconfig   | 48 
>>> >>  arch/arm/mach-npcm/Makefile  |  3 ++
>>> >>  arch/arm/mach-npcm/headsmp.S | 17 +
>>> >>  arch/arm/mach-npcm/npcm7xx.c | 34 +
>>> >>  arch/arm/mach-npcm/platsmp.c | 88 
>>> >> 
>>> >>  7 files changed, 193 insertions(+)
>>> >>  create mode 100644 arch/arm/mach-npcm/Kconfig
>>> >>  create mode 100644 arch/arm/mach-npcm/Makefile
>>> >>  create mode 100644 arch/arm/mach-npcm/headsmp.S
>>> >>  create mode 100644 arch/arm/mach-npcm/npcm7xx.c
>>> >>  create mode 100644 arch/arm/mach-npcm/platsmp.c
>>> >>
>>> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>> >> index 61a0cb15067e..05543f1cfbde 100644
>>> >> --- a/arch/arm/Kconfig
>>> >> +++ b/arch/arm/Kconfig
>>> >> @@ -782,6 +782,8 @@ source "arch/arm/mach-netx/Kconfig"
>>> >>
>>> >>  source "arch/arm/mach-nomadik/Kconfig"
>>> >>
>>> >> +source "arch/arm/mach-npcm/Kconfig"
>>> >> +
>>> >>  source "arch/arm/mach-nspire/Kconfig"
>>> >>
>>> >>  source "arch/arm/plat-omap/Kconfig"
>>> >> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>>> >> index 47d3a1ab08d2..60ca50c7d762 100644
>>> >> --- a/arch/arm/Makefile
>>> >> +++ b/arch/arm/Makefile
>>> >> @@ -191,6 +191,7 @@ machine-$(CONFIG_ARCH_MEDIATEK)   += mediatek
>>> >>  machine-$(CONFIG_ARCH_MXS)   += mxs
>>> >>  machine-$(CONFIG_ARCH_NETX)  += netx
>>> >>  machine-$(CONFIG_ARCH_NOMADIK)   += nomadik
>>> >> +machine-$(CONFIG_ARCH_NPCM)  += npcm
>>> >>  machine-$(CONFIG_ARCH_NSPIRE)+= nspire
>>> >>  machine-$(CONFIG_ARCH_OXNAS) += oxnas
>>> >>  machine-$(CONFIG_ARCH_OMAP1) += omap1
>>> >> diff --git a/arch/arm/mach-npcm/Kconfig b/arch/arm/mach-npcm/Kconfig
>>> >> new file mode 100644
>>> >> index ..21dfa8ce828f
>>> >> --- /dev/null
>>> >> +++ b/arch/arm/mach-npcm/Kconfig
>>> >> @@ -0,0 +1,48 @@
>>> >> +menuconfig ARCH_NPCM
>>> >> + bool "Nuvoton NPCM Architecture"
>>> >> + select ARCH_REQUIRE_GPIOLIB
>>> >> + select USE_OF
>>> >> + select PINCTRL
>>> >> + select PINCTRL_NPCM7XX
>>> >> +
>>> >> +if ARCH_NPCM
>>> >> +
>>> >> +comment "NPCMX50 CPU type"
>>> >> +
>>> >> +config CPU_NPCM750
>
> I started the upstream process of NPCM7xx clocksource driver, I got
> the following comment
> https://www.spinics.net/lists/devicetree/msg197916.html
>
> Is it possible to modify the the architecture configuration name to
> ARCH_NPCM7XX.

You mean the CPU config: CPU_NPCM7XX?

I think the ARCH_NPCM is right, since that corresponds to this directory
which is for all ARM Nuvoton BMCs.

>
>>> >> + depends on ARCH_NPCM && ARCH_MULTI_V7
>>> >> + bool "Support for NPCM750 BMC CPU (Poleg)"
>>> >> + select CACHE_L2X0
>>> >> + select CPU_V7
>>> >> + select ARM_GIC
>>> >> + select HAVE_SMP
>>> >> + select SMP
>>> >> + select SMP_ON_UP
>>> >> + select HAVE_ARM_SCU
>>> >> + select HAVE_ARM_TWD if SMP
>>> >> + select ARM_ERRATA_720789
>>> >> + select ARM_ERRATA_754322
>>> >> + select ARM_ERRATA_764369
>>> >> + select ARM_ERRATA_794072
>>> >> + select PL310_ERRATA_588369
>>> >> + select PL310_ERRATA_727915
>>> >> + select USB_EHCI_ROOT_HUB_TT
>>> >> + select USB_ARCH_HAS_HCD
>>> >> + select USB_ARCH_HAS_EHCI
>>> >> + select USB_EHCI_HCD
>>> >> + select USB_ARCH_HAS_OHCI
>>> >> + select USB_OHCI_HCD
>>> >> + select USB
>>> >> + select FIQ
>>> >> + select CPU_USE_DOMAINS
>>> >> + select GENERIC_CLOCKEVENTS
>>> >> + select CLKDEV_LOOKUP
>>> >> + select COMMON_CLK if OF
>>> >> + select NPCM750_TIMER
>>> >> + select MFD_SYSCON
>>> >> + help
>>> >> +   Support for NPCM750 BMC CPU (Poleg).
>>> >> +
>>> >> +   Nuvoton NPCM750 BMC based on the Cortex A9.
>>> >> +
>>> >> +endif
>>> >> diff --git a/arch/arm/mach-npcm/Makefile b/arch/arm/mach-npcm/Makefile
>>> >> new file mode 100644
>>> >> index ..78416055b854
>>> >> --- /dev/null
>>> >> +++ b/arch/arm/mach-npcm/Makefile
>>> >> @@ -0,0 +1,3 @@
>>> >> +AFLAGS_headsmp.o += -march=armv7-a
>>> >> +
>>> >> +obj-$(CONFIG_CPU_NPCM750)+= npcm7xx.o platsmp.o headsmp.o
>>> >> diff --git 

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Sat, Nov 04, 2017 at 12:24:30AM +, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...
> 
> > > +config PARANOID_UACCESS
> > > +   bool "Use paranoid uaccess primitives"
> > > +   depends on ARCH_HAS_PARANOID_UACCESS
> > > +   help
> > > + Forces access_ok() checks in __get_user(), __put_user(), and 
> > > other
> > > + low-level uaccess primitives which usually do not have checks. 
> > > This
> > > + can limit the effect of missing access_ok() checks in 
> > > higher-level
> > > + primitives, with a runtime performance overhead in some cases 
> > > and a
> > > + small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

FWIW, unsafe variants ought to be encapsulated in as few places as possible.
And that includes both unsafe_... and __... stuff.  waitid() had been a dumb
fuckup (by me) and it should've been done as

static int waitid_put_siginfo(struct siginfo __user *si, struct waitid_info 
*info,
int signo)
{
if (!si)
return 0;
if (!access_ok(VERIFY_WRITE, si, sizeof(struct siginfo)))
return -EFAULT;
user_access_begin();
unsafe_put_user(signo, >si_signo, Efault);
unsafe_put_user(0, >si_errno, Efault);
unsafe_put_user(info->cause, >si_code, Efault);
unsafe_put_user(info->pid, >si_pid, Efault);
unsafe_put_user(info->uid, >si_uid, Efault);
unsafe_put_user(info->status, >si_status, Efault);
user_access_end();
return 0;
Efault:
user_access_end();
return -EFAULT;
}

instead, rather than mixing it with the rest.  Basically, any unsafe... or __...
should be
* used as little as possible
* accompanied by access_ok() in the same function
* not mixed with other stuff within the same function

We are obviously not there yet, but __get_user()/__put_user() *are* getting 
killed
off.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Sat, Nov 04, 2017 at 12:24:30AM +, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...
> 
> > > +config PARANOID_UACCESS
> > > +   bool "Use paranoid uaccess primitives"
> > > +   depends on ARCH_HAS_PARANOID_UACCESS
> > > +   help
> > > + Forces access_ok() checks in __get_user(), __put_user(), and 
> > > other
> > > + low-level uaccess primitives which usually do not have checks. 
> > > This
> > > + can limit the effect of missing access_ok() checks in 
> > > higher-level
> > > + primitives, with a runtime performance overhead in some cases 
> > > and a
> > > + small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

FWIW, unsafe variants ought to be encapsulated in as few places as possible.
And that includes both unsafe_... and __... stuff.  waitid() had been a dumb
fuckup (by me) and it should've been done as

static int waitid_put_siginfo(struct siginfo __user *si, struct waitid_info 
*info,
int signo)
{
if (!si)
return 0;
if (!access_ok(VERIFY_WRITE, si, sizeof(struct siginfo)))
return -EFAULT;
user_access_begin();
unsafe_put_user(signo, >si_signo, Efault);
unsafe_put_user(0, >si_errno, Efault);
unsafe_put_user(info->cause, >si_code, Efault);
unsafe_put_user(info->pid, >si_pid, Efault);
unsafe_put_user(info->uid, >si_uid, Efault);
unsafe_put_user(info->status, >si_status, Efault);
user_access_end();
return 0;
Efault:
user_access_end();
return -EFAULT;
}

instead, rather than mixing it with the rest.  Basically, any unsafe... or __...
should be
* used as little as possible
* accompanied by access_ok() in the same function
* not mixed with other stuff within the same function

We are obviously not there yet, but __get_user()/__put_user() *are* getting 
killed
off.

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Kees Cook]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> But this just broke my _fix_, not the earlier deployed stuff. I removed
> the size measuring code when the 131072 limit went away, the bug was
> there's a new limit I need to not hit, I tried to figure out what the
> limit is now, confirmed that the various libc implementations don't
> agree, then the actual kernel limit changed again while I was looking at it.

In the fix you landed (to include env in size calculations -- which
didn't change recently), you also included the pointer size itself in
the calculation, so

commit 98da7d08850f ("fs/exec.c: account for argv/envp pointers")

should be handled.

If we didn't do the "but no more than 75% of _STK_LIM", and moved to
something like "check stack utilization after loading the binary", we
end up in the position where the kernel is past the point of no return
(so instead of E2BIG, the execve()ing process just SEGVs), which is
much harder to debug or recover from (i.e. there's no process left to
return from the execve() from). We could, however, limit that behavior
to setuid processes? I'm open to whatever Linus says here.

FWIW, I have a lightly tested alternative here (should be visible shortly):
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/stack-size

-Kees

-- 
Kees Cook
Pixel Security

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Kees Cook]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> But this just broke my _fix_, not the earlier deployed stuff. I removed
> the size measuring code when the 131072 limit went away, the bug was
> there's a new limit I need to not hit, I tried to figure out what the
> limit is now, confirmed that the various libc implementations don't
> agree, then the actual kernel limit changed again while I was looking at it.

In the fix you landed (to include env in size calculations -- which
didn't change recently), you also included the pointer size itself in
the calculation, so

commit 98da7d08850f ("fs/exec.c: account for argv/envp pointers")

should be handled.

If we didn't do the "but no more than 75% of _STK_LIM", and moved to
something like "check stack utilization after loading the binary", we
end up in the position where the kernel is past the point of no return
(so instead of E2BIG, the execve()ing process just SEGVs), which is
much harder to debug or recover from (i.e. there's no process left to
return from the execve() from). We could, however, limit that behavior
to setuid processes? I'm open to whatever Linus says here.

FWIW, I have a lightly tested alternative here (should be visible shortly):
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/stack-size

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v10 02/13] x86/insn-eval: Compute linear address in several utility functions

[Ricardo Neri]

On Fri, Nov 03, 2017 at 11:17:49AM +0100, Ingo Molnar wrote:
> 
> * Ricardo Neri  wrote:
> 
> > On Thu, Nov 02, 2017 at 09:51:08AM +0100, Ingo Molnar wrote:
> > > 
> > > * Ricardo Neri  wrote:
> > > 
> > > > +   /*
> > > > +* -EDOM means that we must ignore the address_offset. In such 
> > > > a case,
> > > > +* in 64-bit mode the effective address relative to the RIP of 
> > > > the
> > > > +* following instruction.
> > > > +*/
> > > > +   if (*regoff == -EDOM) {
> > > > +   if (user_64bit_mode(regs))
> > > > +   tmp = (long)regs->ip + insn->length;
> > > > +   else
> > > > +   tmp = 0;
> > > > +   } else if (*regoff < 0) {
> > > > +   return -EINVAL;
> > > > +   } else {
> > > > +   tmp = (long)regs_get_register(regs, *regoff);
> > > > +   }
> > > 
> > > > +   else
> > > > +   indx = (long)regs_get_register(regs, indx_offset);
> > > 
> > > This and subsequent patches include a disgustly insane amount of type 
> > > casts - why?
> > > 
> > > For example here 'tmp' is 'long', while regs_get_register() returns
> > > 'unsigned long', but no type cast is necessary for that.
> > > 
> > > > +   ret = get_eff_addr_modrm(insn, regs, 
> > > > _offset,
> > > > +_addr);
> > 
> > One of the goals of this series is to have the ability to compute 16-bit, 
> > 32-bit
> > and 64-bit addresses. I put lost of casts, between signed and unsigned 
> > types,
> > between 64-bit and 32-bit and 16-bit casts. After seeing your comment I 
> > have gone
> > through the code and I have removed most of the casts. Instead I will use 
> > masks.
> > I will also inspect the resulting assembly code to make sure the arithmetic 
> > is
> > performed in the address size pertinent to each case.
> 
> Well, casts are probably fine when the goal is to zero out high bits

I was able to remove the majority of casts and used masks. I see many other 
parts
of Linux doing similarly. For instance, in arch/x86/kernel/kexec-bzimage64.c I 
see

params->hdr.ramdisk_image = initrd_load_addr & 0xUL;

ramdisk_image is of type __u32 while initrd_load_addr is of type unsigned long.

I guess that in this example doing

params->hdr.ramdisk_image = (__u32)(initrd_load_addr & 0xUL);

would be redundant? The mask would indicate better what is going on.

> but the ones I quoted converted types of the same with.

I made sure I removed these.

> 
> For register values it would also probably be cleaner to use the u8, u16, u32 
> and 
> u64 types instead of char/short/int/long - this goes hand in hand with how 
> the 
> instructions are documented in the SDMs.

In the rest of the functions I have used char/short/int/long. Would it be OK to 
have
a mixture of type styles? Perhaps I can rewrite only the functions that deal 
with
variables of different width.

Plus, one more advantage of using char/short/int/long is that when building a 
32-bit
kernel long will be a 32-bit type. Thus, all the aritmetic would be naturally 
done
with variables of the appropriate width. Perhaps I could use u8/u16/u32/long? It
looks white odd, though.

Thanks and BR,
Ricardo

Re: [PATCH v10 02/13] x86/insn-eval: Compute linear address in several utility functions

[Ricardo Neri]

On Fri, Nov 03, 2017 at 11:17:49AM +0100, Ingo Molnar wrote:
> 
> * Ricardo Neri  wrote:
> 
> > On Thu, Nov 02, 2017 at 09:51:08AM +0100, Ingo Molnar wrote:
> > > 
> > > * Ricardo Neri  wrote:
> > > 
> > > > +   /*
> > > > +* -EDOM means that we must ignore the address_offset. In such 
> > > > a case,
> > > > +* in 64-bit mode the effective address relative to the RIP of 
> > > > the
> > > > +* following instruction.
> > > > +*/
> > > > +   if (*regoff == -EDOM) {
> > > > +   if (user_64bit_mode(regs))
> > > > +   tmp = (long)regs->ip + insn->length;
> > > > +   else
> > > > +   tmp = 0;
> > > > +   } else if (*regoff < 0) {
> > > > +   return -EINVAL;
> > > > +   } else {
> > > > +   tmp = (long)regs_get_register(regs, *regoff);
> > > > +   }
> > > 
> > > > +   else
> > > > +   indx = (long)regs_get_register(regs, indx_offset);
> > > 
> > > This and subsequent patches include a disgustly insane amount of type 
> > > casts - why?
> > > 
> > > For example here 'tmp' is 'long', while regs_get_register() returns
> > > 'unsigned long', but no type cast is necessary for that.
> > > 
> > > > +   ret = get_eff_addr_modrm(insn, regs, 
> > > > _offset,
> > > > +_addr);
> > 
> > One of the goals of this series is to have the ability to compute 16-bit, 
> > 32-bit
> > and 64-bit addresses. I put lost of casts, between signed and unsigned 
> > types,
> > between 64-bit and 32-bit and 16-bit casts. After seeing your comment I 
> > have gone
> > through the code and I have removed most of the casts. Instead I will use 
> > masks.
> > I will also inspect the resulting assembly code to make sure the arithmetic 
> > is
> > performed in the address size pertinent to each case.
> 
> Well, casts are probably fine when the goal is to zero out high bits

I was able to remove the majority of casts and used masks. I see many other 
parts
of Linux doing similarly. For instance, in arch/x86/kernel/kexec-bzimage64.c I 
see

params->hdr.ramdisk_image = initrd_load_addr & 0xUL;

ramdisk_image is of type __u32 while initrd_load_addr is of type unsigned long.

I guess that in this example doing

params->hdr.ramdisk_image = (__u32)(initrd_load_addr & 0xUL);

would be redundant? The mask would indicate better what is going on.

> but the ones I quoted converted types of the same with.

I made sure I removed these.

> 
> For register values it would also probably be cleaner to use the u8, u16, u32 
> and 
> u64 types instead of char/short/int/long - this goes hand in hand with how 
> the 
> instructions are documented in the SDMs.

In the rest of the functions I have used char/short/int/long. Would it be OK to 
have
a mixture of type styles? Perhaps I can rewrite only the functions that deal 
with
variables of different width.

Plus, one more advantage of using char/short/int/long is that when building a 
32-bit
kernel long will be a 32-bit type. Thus, all the aritmetic would be naturally 
done
with variables of the appropriate width. Perhaps I could use u8/u16/u32/long? It
looks white odd, though.

Thanks and BR,
Ricardo

Re: [PATCH] kbuild: Set KBUILD_CFLAGS before incl. arch Makefile

[Matthias Kaehlcke]

El Thu, Nov 02, 2017 at 02:26:48PM -0700 Nick Desaulniers ha dit:

> From: Chris Fries 
> 
> Set the clang KBUILD_CFLAGS up before including arch/ Makefiles,
> so that ld-options (etc.) can work correctly.
> 
> This fixes errors with clang such as ld-options trying to CC
> against your host architecture, but LD trying to link against
> your target architecture.
> 
> We didn't notice this problem on Android, because we took the original
> LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
> ran into this taking the proper upstream patch on newer kernel versions.
> The original LLVMLinux patch can be seen at:
> 
> http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6
> 
> It seems that when the patch was re-upstreamed, a V2 was requested that
> moved the definition of Clang's target triple to be later in the top
> level Makefile than the inclusion of the arch specific Makefile,
> breaking macros like ld-option when cross compiling. V2 was requested
> at:
> 
> https://lkml.org/lkml/2017/4/21/116
> 
> Signed-off-by: Chris Fries 
> Signed-off-by: Nick Desaulniers 
> ---
>  Makefile | 64 
> 
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5f91a28a3cea..72ea86157114 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -512,6 +512,38 @@ ifneq ($(filter install,$(MAKECMDGOALS)),)
>  endif
>  endif
>  
> +ifeq ($(cc-name),clang)
> +ifneq ($(CROSS_COMPILE),)
> +CLANG_TARGET := --target=$(notdir $(CROSS_COMPILE:%-=%))
> +GCC_TOOLCHAIN:= $(realpath $(dir $(shell which $(LD)))/..)
> +endif
> +ifneq ($(GCC_TOOLCHAIN),)
> +CLANG_GCC_TC := --gcc-toolchain=$(GCC_TOOLCHAIN)
> +endif
> +KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as 
> the
> +# source of a reference will be _MergedGlobals and not on of the whitelisted 
> names.
> +# See modpost pattern 2
> +KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
> +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
> +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
> +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
> +else
> +
> +# These warnings generated too much noise in a regular build.
> +# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> +endif
> +
>  ifeq ($(mixed-targets),1)
>  # ===
>  # We're called with mixed targets (*config and build targets).
> @@ -695,38 +727,6 @@ ifdef CONFIG_CC_STACKPROTECTOR
>  endif
>  KBUILD_CFLAGS += $(stackp-flag)
>  
> -ifeq ($(cc-name),clang)
> -ifneq ($(CROSS_COMPILE),)
> -CLANG_TARGET := --target=$(notdir $(CROSS_COMPILE:%-=%))
> -GCC_TOOLCHAIN:= $(realpath $(dir $(shell which $(LD)))/..)
> -endif
> -ifneq ($(GCC_TOOLCHAIN),)
> -CLANG_GCC_TC := --gcc-toolchain=$(GCC_TOOLCHAIN)
> -endif
> -KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> -KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> -KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> -KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> -KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> -KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> -KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> -# Quiet clang warning: comparison of unsigned expression < 0 is always false
> -KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> -# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as 
> the
> -# source of a reference will be _MergedGlobals and not on of the whitelisted 
> names.
> -# See modpost pattern 2
> -KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
> -KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
> -KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
> -KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
> -else
> -
> -# These warnings generated too much noise in a regular build.
> -# Use make W=1 to enable them 

Re: [PATCH] kbuild: Set KBUILD_CFLAGS before incl. arch Makefile

[Matthias Kaehlcke]

El Thu, Nov 02, 2017 at 02:26:48PM -0700 Nick Desaulniers ha dit:

> From: Chris Fries 
> 
> Set the clang KBUILD_CFLAGS up before including arch/ Makefiles,
> so that ld-options (etc.) can work correctly.
> 
> This fixes errors with clang such as ld-options trying to CC
> against your host architecture, but LD trying to link against
> your target architecture.
> 
> We didn't notice this problem on Android, because we took the original
> LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
> ran into this taking the proper upstream patch on newer kernel versions.
> The original LLVMLinux patch can be seen at:
> 
> http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6
> 
> It seems that when the patch was re-upstreamed, a V2 was requested that
> moved the definition of Clang's target triple to be later in the top
> level Makefile than the inclusion of the arch specific Makefile,
> breaking macros like ld-option when cross compiling. V2 was requested
> at:
> 
> https://lkml.org/lkml/2017/4/21/116
> 
> Signed-off-by: Chris Fries 
> Signed-off-by: Nick Desaulniers 
> ---
>  Makefile | 64 
> 
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5f91a28a3cea..72ea86157114 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -512,6 +512,38 @@ ifneq ($(filter install,$(MAKECMDGOALS)),)
>  endif
>  endif
>  
> +ifeq ($(cc-name),clang)
> +ifneq ($(CROSS_COMPILE),)
> +CLANG_TARGET := --target=$(notdir $(CROSS_COMPILE:%-=%))
> +GCC_TOOLCHAIN:= $(realpath $(dir $(shell which $(LD)))/..)
> +endif
> +ifneq ($(GCC_TOOLCHAIN),)
> +CLANG_GCC_TC := --gcc-toolchain=$(GCC_TOOLCHAIN)
> +endif
> +KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as 
> the
> +# source of a reference will be _MergedGlobals and not on of the whitelisted 
> names.
> +# See modpost pattern 2
> +KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
> +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
> +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
> +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
> +else
> +
> +# These warnings generated too much noise in a regular build.
> +# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> +endif
> +
>  ifeq ($(mixed-targets),1)
>  # ===
>  # We're called with mixed targets (*config and build targets).
> @@ -695,38 +727,6 @@ ifdef CONFIG_CC_STACKPROTECTOR
>  endif
>  KBUILD_CFLAGS += $(stackp-flag)
>  
> -ifeq ($(cc-name),clang)
> -ifneq ($(CROSS_COMPILE),)
> -CLANG_TARGET := --target=$(notdir $(CROSS_COMPILE:%-=%))
> -GCC_TOOLCHAIN:= $(realpath $(dir $(shell which $(LD)))/..)
> -endif
> -ifneq ($(GCC_TOOLCHAIN),)
> -CLANG_GCC_TC := --gcc-toolchain=$(GCC_TOOLCHAIN)
> -endif
> -KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> -KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> -KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> -KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> -KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> -KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> -KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> -# Quiet clang warning: comparison of unsigned expression < 0 is always false
> -KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> -# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as 
> the
> -# source of a reference will be _MergedGlobals and not on of the whitelisted 
> names.
> -# See modpost pattern 2
> -KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
> -KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
> -KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
> -KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
> -else
> -
> -# These warnings generated too much noise in a regular build.
> -# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> -KBUILD_CFLAGS += $(call 

Re: [PATCH v2 resend 0/1] Input: add support HiDeep touchscreen.

[Dmitry Torokhov]

Hi Anthony,

On Tue, Oct 31, 2017 at 07:17:56PM +0900, Anthony Kim wrote:
> Hi,
> 
> I remake patch to base on dmitry's modified code.
> Please refer as follow.
> http://www.spinics.net/lists/linux-input/msg53724.html
> 
> That code is working well almost, but needed to some modified.
> 
> - v2
>   - Our fw binary data is little endian, but our IC protocol
> for fw update use big endian. So it need to convert to big endian data
> and I added about it.

I understand that _currently_ your firmware binary is little endian,
however it is easily remedied on your side. I do not understand why you
would want to encumber the kernel running on many devices to do that
conversion on fly.

Thanks.

-- 
Dmitry

Re: [PATCH v2 resend 0/1] Input: add support HiDeep touchscreen.

[Dmitry Torokhov]

Hi Anthony,

On Tue, Oct 31, 2017 at 07:17:56PM +0900, Anthony Kim wrote:
> Hi,
> 
> I remake patch to base on dmitry's modified code.
> Please refer as follow.
> http://www.spinics.net/lists/linux-input/msg53724.html
> 
> That code is working well almost, but needed to some modified.
> 
> - v2
>   - Our fw binary data is little endian, but our IC protocol
> for fw update use big endian. So it need to convert to big endian data
> and I added about it.

I understand that _currently_ your firmware binary is little endian,
however it is easily remedied on your side. I do not understand why you
would want to encumber the kernel running on many devices to do that
conversion on fly.

Thanks.

-- 
Dmitry

Re: [PATCHSET] 2.6.15-rc7-bird1

[Al Viro]

On Sat, Nov 04, 2017 at 12:29:54AM +, Al Viro wrote:
[snip]

Bloody hell...  My apologies - that was a postponed mail from *way*
back that got sent now by dumb mistake.  Sorry about that

Re: [PATCHSET] 2.6.15-rc7-bird1

[Al Viro]

On Sat, Nov 04, 2017 at 12:29:54AM +, Al Viro wrote:
[snip]

Bloody hell...  My apologies - that was a postponed mail from *way*
back that got sent now by dumb mistake.  Sorry about that

Re: [PATCHSET] 2.6.15-rc7-bird1

[Al Viro]

BTW, here's the state of toolchains, by architecture:

alpha   alpha-linux works
arm arm-linux   works
arm26   support is gone in gcc4, tree is not well either
criscris-linux  broken [*3][*4], but gets through arch/cris, so
there's some use for it...
frv frv-linux   4.1 mostly works [*2], but "U" constraint
support is still missing in mainline
h8300   h8300-elf   broken [*5][*3]
i386x86_64-linuxworks
ia64ia64-linux  works
m32rm32r-linux  works [*2]
m68km68k-linux  works [*2]
mipsmips64-linuxworks
mips64  mips64-linuxworks [*1]
parisc  hppa-linux  works [*4]; gcc-4.1 gives an ICE [*7]
parisc64hppa64-linuxLinus' tree is broken; gcc builds, but doesn't
get far in kernel build; might die later.
powerpc powerpc64-linux works
ppc powerpc64-linux works
s390s390x-linux works
sh  sh-linuxworks [*4]
sh64sh64-linux  broken [*6]
sparc   sparc64-linux   works
sparc64 sparc64-linux   works
um  toolchain of corresponding sub-arch
v850v850e-linux works [*2][*5]
x86_64  x86_64-linuxworks
xtensa, m68knommu: have not tried lately

[*1] except that we get bogus flags passed to as(1) if we are building
for 64bit cpu without CONFIG_BUILD_ELF64.
[*2] 64bit bugs in binutils, fixes exist
[*3] ICE during kernel build
[*4] needs a patch to get rid of dependency on target libc headers; fix exists
[*5] 64bit bugs in gcc, fixes exist
[*6] ICE during libgcc build
[*7] 4.0 works, 4.1 gives an ICE when you build on 64bit host with -Os;
trimmed-down reproducer is
void foo(int sig, int *p)
{
if (!((1ULL << ((sig)-1)) & 0x30edcULL))
*p = 0;
}

Re: [PATCHSET] 2.6.15-rc7-bird1

[Al Viro]

BTW, here's the state of toolchains, by architecture:

alpha   alpha-linux works
arm arm-linux   works
arm26   support is gone in gcc4, tree is not well either
criscris-linux  broken [*3][*4], but gets through arch/cris, so
there's some use for it...
frv frv-linux   4.1 mostly works [*2], but "U" constraint
support is still missing in mainline
h8300   h8300-elf   broken [*5][*3]
i386x86_64-linuxworks
ia64ia64-linux  works
m32rm32r-linux  works [*2]
m68km68k-linux  works [*2]
mipsmips64-linuxworks
mips64  mips64-linuxworks [*1]
parisc  hppa-linux  works [*4]; gcc-4.1 gives an ICE [*7]
parisc64hppa64-linuxLinus' tree is broken; gcc builds, but doesn't
get far in kernel build; might die later.
powerpc powerpc64-linux works
ppc powerpc64-linux works
s390s390x-linux works
sh  sh-linuxworks [*4]
sh64sh64-linux  broken [*6]
sparc   sparc64-linux   works
sparc64 sparc64-linux   works
um  toolchain of corresponding sub-arch
v850v850e-linux works [*2][*5]
x86_64  x86_64-linuxworks
xtensa, m68knommu: have not tried lately

[*1] except that we get bogus flags passed to as(1) if we are building
for 64bit cpu without CONFIG_BUILD_ELF64.
[*2] 64bit bugs in binutils, fixes exist
[*3] ICE during kernel build
[*4] needs a patch to get rid of dependency on target libc headers; fix exists
[*5] 64bit bugs in gcc, fixes exist
[*6] ICE during libgcc build
[*7] 4.0 works, 4.1 gives an ICE when you build on 64bit host with -Os;
trimmed-down reproducer is
void foo(int sig, int *p)
{
if (!((1ULL << ((sig)-1)) & 0x30edcULL))
*p = 0;
}

Re: [PATCH v2 1/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

Looks like you missed a couple of comments from Daniel:
https://www.spinics.net/lists/devicetree/msg196683.html

Also, I think the binding documentation is supposed to come before the driver.

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon  wrote:
> Add Nuvoton BMC NPCM7xx timer driver.
>
> the clocksource Enable 24-bit TIMER0 and TIMER1 counters,
> while TIMER0 serves as clockevent and TIMER1 serves as clocksource.
>
> Signed-off-by: Tomer Maimon 
> ---
>  drivers/clocksource/Kconfig |   8 ++
>  drivers/clocksource/Makefile|   1 +
>  drivers/clocksource/npcm7xx-timer.c | 224 
> 
>  3 files changed, 233 insertions(+)
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c

Looks like you forgot to rename the driver.

>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cc6062049170..46184af75d6c 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -140,6 +140,14 @@ config VT8500_TIMER
> help
>   Enables support for the VT8500 driver.
>
> +config NPCM7XX_TIMER
> +   bool "NPCM7xx timer driver" if COMPILE_TEST
> +   depends on HAS_IOMEM
> +   select CLKSRC_MMIO
> +   help
> + Enable 24-bit TIMER0 and TIMER1 counters in the NPCM7xx arcithcture,
> + While TIMER0 serves as clockevent and TIMER1 serves as clocksource.
> +
>  config CADENCE_TTC_TIMER
> bool "Cadence TTC timer driver" if COMPILE_TEST
> depends on COMMON_CLK
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dbc1ad14515e..f2a9318a1eec 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)   += timer-ti-32k.o
>  obj-$(CONFIG_CLKSRC_NPS)   += timer-nps.o
>  obj-$(CONFIG_OXNAS_RPS_TIMER)  += timer-oxnas-rps.o
>  obj-$(CONFIG_OWL_TIMER)+= owl-timer.o
> +obj-$(CONFIG_NPCM7XX_TIMER)+= npcm7xx-timer.o
>
>  obj-$(CONFIG_ARC_TIMERS)   += arc_timer.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)   += arm_arch_timer.o
> diff --git a/drivers/clocksource/npcm7xx-timer.c 
> b/drivers/clocksource/npcm7xx-timer.c
> new file mode 100644
> index ..d1ea5c183a69
> --- /dev/null
> +++ b/drivers/clocksource/npcm7xx-timer.c
> @@ -0,0 +1,224 @@
> +
> +/*
> + * Copyright (c) 2017 Nuvoton Technology corporation.
> + * Copyright 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;version 2 of the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct npcm7xx_clockevent_data {
> +   struct clock_event_device cvd;
> +   void __iomem *timer_base;
> +   unsigned int rate;
> +};
> +
> +/* Timers registers */
> +#define NPCM7XX_REG_TCSR0  0x0 /* Timer 0 Control and Status Register */
> +#define NPCM7XX_REG_TICR0  0x8 /* Timer 0 Initial Count Register */
> +#define NPCM7XX_REG_TCSR1  0x4 /* Timer 1 Control and Status Register */
> +#define NPCM7XX_REG_TICR1  0xc /* Timer 1 Initial Count Register */
> +#define NPCM7XX_REG_TDR1   0x14 /* Timer 1 Data Register */
> +#define NPCM7XX_REG_TISR   0x18 /* Timer Interrupt Status Register */
> +
> +/* Timers control */
> +#define NPCM7XX_Tx_RESETINT0x1f
> +#define NPCM7XX_Tx_PERIOD  BIT(27)
> +#define NPCM7XX_Tx_INTEN   BIT(29)
> +#define NPCM7XX_Tx_COUNTEN BIT(30)
> +#define NPCM7XX_Tx_ONESHOT 0x0
> +#define NPCM7XX_Tx_OPERGENMASK(3, 27)
> +#define NPCM7XX_Tx_MIN_PRESCALE0x1
> +#define NPCM7XX_Tx_TDR_MASK_BITS   24
> +#define NPCM7XX_Tx_MAX_CNT 0xFF
> +#define NPCM7XX_T0_CLR_INT 0x1
> +#define NPCM7XX_Tx_CLR_CSR 0x0
> +
> +/* Timers operating mode */
> +#define NPCM7XX_START_PERIODIC_Tx (NPCM7XX_Tx_PERIOD | NPCM7XX_Tx_COUNTEN | \
> +   NPCM7XX_Tx_INTEN | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +
> +#define NPCM7XX_START_ONESHOT_Tx (NPCM7XX_Tx_ONESHOT | NPCM7XX_Tx_COUNTEN | \
> +   NPCM7XX_Tx_INTEN | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +#define NPCM7XX_START_Tx (NPCM7XX_Tx_COUNTEN | NPCM7XX_Tx_PERIOD | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +
> +static int npcm7xx_timer_oneshot(struct clock_event_device *evt)
> +{
> +   u32 val;
> +   struct npcm7xx_clockevent_data *cevtd =
> +   container_of(evt, struct npcm7xx_clockevent_data, cvd);
> +
> +   val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +   val &= ~NPCM7XX_Tx_OPER;
> +
> +   val = 

Re: [PATCH v2 1/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

Looks like you missed a couple of comments from Daniel:
https://www.spinics.net/lists/devicetree/msg196683.html

Also, I think the binding documentation is supposed to come before the driver.

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon  wrote:
> Add Nuvoton BMC NPCM7xx timer driver.
>
> the clocksource Enable 24-bit TIMER0 and TIMER1 counters,
> while TIMER0 serves as clockevent and TIMER1 serves as clocksource.
>
> Signed-off-by: Tomer Maimon 
> ---
>  drivers/clocksource/Kconfig |   8 ++
>  drivers/clocksource/Makefile|   1 +
>  drivers/clocksource/npcm7xx-timer.c | 224 
> 
>  3 files changed, 233 insertions(+)
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c

Looks like you forgot to rename the driver.

>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cc6062049170..46184af75d6c 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -140,6 +140,14 @@ config VT8500_TIMER
> help
>   Enables support for the VT8500 driver.
>
> +config NPCM7XX_TIMER
> +   bool "NPCM7xx timer driver" if COMPILE_TEST
> +   depends on HAS_IOMEM
> +   select CLKSRC_MMIO
> +   help
> + Enable 24-bit TIMER0 and TIMER1 counters in the NPCM7xx arcithcture,
> + While TIMER0 serves as clockevent and TIMER1 serves as clocksource.
> +
>  config CADENCE_TTC_TIMER
> bool "Cadence TTC timer driver" if COMPILE_TEST
> depends on COMMON_CLK
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dbc1ad14515e..f2a9318a1eec 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)   += timer-ti-32k.o
>  obj-$(CONFIG_CLKSRC_NPS)   += timer-nps.o
>  obj-$(CONFIG_OXNAS_RPS_TIMER)  += timer-oxnas-rps.o
>  obj-$(CONFIG_OWL_TIMER)+= owl-timer.o
> +obj-$(CONFIG_NPCM7XX_TIMER)+= npcm7xx-timer.o
>
>  obj-$(CONFIG_ARC_TIMERS)   += arc_timer.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)   += arm_arch_timer.o
> diff --git a/drivers/clocksource/npcm7xx-timer.c 
> b/drivers/clocksource/npcm7xx-timer.c
> new file mode 100644
> index ..d1ea5c183a69
> --- /dev/null
> +++ b/drivers/clocksource/npcm7xx-timer.c
> @@ -0,0 +1,224 @@
> +
> +/*
> + * Copyright (c) 2017 Nuvoton Technology corporation.
> + * Copyright 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;version 2 of the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct npcm7xx_clockevent_data {
> +   struct clock_event_device cvd;
> +   void __iomem *timer_base;
> +   unsigned int rate;
> +};
> +
> +/* Timers registers */
> +#define NPCM7XX_REG_TCSR0  0x0 /* Timer 0 Control and Status Register */
> +#define NPCM7XX_REG_TICR0  0x8 /* Timer 0 Initial Count Register */
> +#define NPCM7XX_REG_TCSR1  0x4 /* Timer 1 Control and Status Register */
> +#define NPCM7XX_REG_TICR1  0xc /* Timer 1 Initial Count Register */
> +#define NPCM7XX_REG_TDR1   0x14 /* Timer 1 Data Register */
> +#define NPCM7XX_REG_TISR   0x18 /* Timer Interrupt Status Register */
> +
> +/* Timers control */
> +#define NPCM7XX_Tx_RESETINT0x1f
> +#define NPCM7XX_Tx_PERIOD  BIT(27)
> +#define NPCM7XX_Tx_INTEN   BIT(29)
> +#define NPCM7XX_Tx_COUNTEN BIT(30)
> +#define NPCM7XX_Tx_ONESHOT 0x0
> +#define NPCM7XX_Tx_OPERGENMASK(3, 27)
> +#define NPCM7XX_Tx_MIN_PRESCALE0x1
> +#define NPCM7XX_Tx_TDR_MASK_BITS   24
> +#define NPCM7XX_Tx_MAX_CNT 0xFF
> +#define NPCM7XX_T0_CLR_INT 0x1
> +#define NPCM7XX_Tx_CLR_CSR 0x0
> +
> +/* Timers operating mode */
> +#define NPCM7XX_START_PERIODIC_Tx (NPCM7XX_Tx_PERIOD | NPCM7XX_Tx_COUNTEN | \
> +   NPCM7XX_Tx_INTEN | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +
> +#define NPCM7XX_START_ONESHOT_Tx (NPCM7XX_Tx_ONESHOT | NPCM7XX_Tx_COUNTEN | \
> +   NPCM7XX_Tx_INTEN | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +#define NPCM7XX_START_Tx (NPCM7XX_Tx_COUNTEN | NPCM7XX_Tx_PERIOD | \
> +   NPCM7XX_Tx_MIN_PRESCALE)
> +
> +static int npcm7xx_timer_oneshot(struct clock_event_device *evt)
> +{
> +   u32 val;
> +   struct npcm7xx_clockevent_data *cevtd =
> +   container_of(evt, struct npcm7xx_clockevent_data, cvd);
> +
> +   val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> +   val &= ~NPCM7XX_Tx_OPER;
> +
> +   val = readl(cevtd->timer_base + NPCM7XX_REG_TCSR0);
> + 

Re: [PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon  wrote:
> Addressed comments from:
>  - Daniel Lezcano: https://www.spinics.net/lists/devicetree/msg196683.html
>
> Changes since version 1:
>  - Rename driver name
>  - Removing unnecessary dependencies in configuration
>  - Adding prefix to the macros
>  - No changes to dt-binding documentation since v1
>
> Asked Brendon to modify NPCM7xx device tree for support NPCM7xx timer driver
> https://www.spinics.net/lists/arm-kernel/msg614424.html

Rob Herring asks that we keep the device tree binding the same as I have it.
Unless, there is some part of the conversation that I am missing.

>
> Changes have been tested on the NPCM750 evaluation board.
>
> Tomer Maimon (2):
>   clocksource: npcm: add NPCM7xx timer driver
>   dt-binding: timer: document NPCM7xx timer DT bindings
>
>  .../bindings/timer/nuvoton,npcm7xx-timer.txt   |  25 +++
>  drivers/clocksource/Kconfig|   8 +
>  drivers/clocksource/Makefile   |   1 +
>  drivers/clocksource/npcm7xx-timer.c| 224 
> +
>  4 files changed, 258 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c
>
> --
> 2.14.1
>

Re: [PATCH v2 0/2] clocksource: npcm: add NPCM7xx timer driver

[Brendan Higgins]

On Wed, Nov 1, 2017 at 5:16 AM, Tomer Maimon  wrote:
> Addressed comments from:
>  - Daniel Lezcano: https://www.spinics.net/lists/devicetree/msg196683.html
>
> Changes since version 1:
>  - Rename driver name
>  - Removing unnecessary dependencies in configuration
>  - Adding prefix to the macros
>  - No changes to dt-binding documentation since v1
>
> Asked Brendon to modify NPCM7xx device tree for support NPCM7xx timer driver
> https://www.spinics.net/lists/arm-kernel/msg614424.html

Rob Herring asks that we keep the device tree binding the same as I have it.
Unless, there is some part of the conversation that I am missing.

>
> Changes have been tested on the NPCM750 evaluation board.
>
> Tomer Maimon (2):
>   clocksource: npcm: add NPCM7xx timer driver
>   dt-binding: timer: document NPCM7xx timer DT bindings
>
>  .../bindings/timer/nuvoton,npcm7xx-timer.txt   |  25 +++
>  drivers/clocksource/Kconfig|   8 +
>  drivers/clocksource/Makefile   |   1 +
>  drivers/clocksource/npcm7xx-timer.c| 224 
> +
>  4 files changed, 258 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/timer/nuvoton,npcm7xx-timer.txt
>  create mode 100644 drivers/clocksource/npcm7xx-timer.c
>
> --
> 2.14.1
>

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > x86 turns out to be easier since the safe and unsafe paths are mostly
> > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > I tweaked the Kconfig to someting a bit more generic.
> >
> > The size increase was ~8K in text with a config I tested.
> 
> Specifically, this feature would have caught the waitid() bug in 4.13
> immediately.

You mean, as soon as waitid() was given a kernel address.  At which point
you'd get a shiny way to generate a BUG(), and if something like that
happened under a mutex - it's even more fun...

> > +config PARANOID_UACCESS
> > +   bool "Use paranoid uaccess primitives"
> > +   depends on ARCH_HAS_PARANOID_UACCESS
> > +   help
> > + Forces access_ok() checks in __get_user(), __put_user(), and other
> > + low-level uaccess primitives which usually do not have checks. 
> > This
> > + can limit the effect of missing access_ok() checks in higher-level
> > + primitives, with a runtime performance overhead in some cases and 
> > a
> > + small code size overhead.

IMO that's the wrong way to go - what we need is to reduce the amount of
__get_user()/__put_user(), rather than "instrumenting" them that way.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > x86 turns out to be easier since the safe and unsafe paths are mostly
> > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > I tweaked the Kconfig to someting a bit more generic.
> >
> > The size increase was ~8K in text with a config I tested.
> 
> Specifically, this feature would have caught the waitid() bug in 4.13
> immediately.

You mean, as soon as waitid() was given a kernel address.  At which point
you'd get a shiny way to generate a BUG(), and if something like that
happened under a mutex - it's even more fun...

> > +config PARANOID_UACCESS
> > +   bool "Use paranoid uaccess primitives"
> > +   depends on ARCH_HAS_PARANOID_UACCESS
> > +   help
> > + Forces access_ok() checks in __get_user(), __put_user(), and other
> > + low-level uaccess primitives which usually do not have checks. 
> > This
> > + can limit the effect of missing access_ok() checks in higher-level
> > + primitives, with a runtime performance overhead in some cases and 
> > a
> > + small code size overhead.

IMO that's the wrong way to go - what we need is to reduce the amount of
__get_user()/__put_user(), rather than "instrumenting" them that way.

[git pull] Input updates for v4.14-rc7

[Dmitry Torokhov]

Hi Linus,

Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus

to receive updates for the input subsystem. Just a couple of fixups to
sparse-keymap module and Microchip AR1021 touchscreen driver.

Changelog:
-

Martin Kepplinger (1):
  Input: ar1021_i2c - set INPUT_PROP_DIRECT

Stefan Brüns (1):
  Input: sparse-keymap - send sync event for KE_SW/KE_VSW

Diffstat:


 drivers/input/sparse-keymap.c  | 1 +
 drivers/input/touchscreen/ar1021_i2c.c | 1 +
 2 files changed, 2 insertions(+)

Thanks.


-- 
Dmitry

[git pull] Input updates for v4.14-rc7

[Dmitry Torokhov]

Hi Linus,

Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus

to receive updates for the input subsystem. Just a couple of fixups to
sparse-keymap module and Microchip AR1021 touchscreen driver.

Changelog:
-

Martin Kepplinger (1):
  Input: ar1021_i2c - set INPUT_PROP_DIRECT

Stefan Brüns (1):
  Input: sparse-keymap - send sync event for KE_SW/KE_VSW

Diffstat:


 drivers/input/sparse-keymap.c  | 1 +
 drivers/input/touchscreen/ar1021_i2c.c | 1 +
 2 files changed, 2 insertions(+)

Thanks.


-- 
Dmitry

Re: [PATCH] Net: netfilter: vmalloc/vfree to kvmalloc/kvfree

[Florian Westphal]

Charlie Sale  wrote:
> + hinfo = kvmalloc(sizeof(*hinfo) + sizeof(struct hlist_head) * size,
> +  GPT_KERNEL);

Looks like you did not even compile test this.  Again. :-(

Re: [PATCH] Net: netfilter: vmalloc/vfree to kvmalloc/kvfree

[Florian Westphal]

Charlie Sale  wrote:
> + hinfo = kvmalloc(sizeof(*hinfo) + sizeof(struct hlist_head) * size,
> +  GPT_KERNEL);

Looks like you did not even compile test this.  Again. :-(

Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>
> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>
> >>
> >> I see the AVI infoframe has hdmi_quantization_range and
> >> hdmi_ycc_quantization_range along with vid_code.
> >>
> >> I'm not at all clear what to do with this information. Is there
> >> anything you see in the datasheet [1] that points to something I need
> >> to be doing?
> >
> > You can ignore hdmi_ycc_quantization_range, it is the 
> > hdmi_quantization_range
> > that you need to read out.
> >
> > The TDA can receive the following formats:
> >
> > RGB Full Range
> > RGB Limited Range
> > YUV Bt.601 (aka SMPTE 170M)
> > YUV Rec.709
> >
> > The YUV formats are always limited range.
> >
> > The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
> > and
> > YUV to be limited range. YUV can be either 601 or 709.
> >
> > So if the TDA transmits RGB then you need to support the following 
> > conversions:
> >
> > RGB Full -> RGB Full
> > RGB Limited -> RGB Full
> > YUV 601 -> RGB Full
> > YUV 709 -> RGB Full
> >
> > And if the TDA transmits YUV then you need these conversions:
> >
> > RGB Full -> YUV601 or YUV709
> > RGB Limited -> YUV601 or YUV709
> > YUV601 -> YUV601
> > YUV709 -> YUV709
> >
> > For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> > 709.
> > I recommend to either always convert to YUV601 or to let it depend on the 
> > resolution
> > (SDTV YUV601, HDTV YUV709).
> >
>
> Ok - this is a good explanation that I should be able to follow. I
> will make sure to take into account hdmi_quantization_range when I
> setup the colorspace conversion matrix for v3.

Hans,

I'm having trouble figuring out the conversion matrix to use between
limited and full.

Currently I have the following conversion matrices, the values which
came from some old vendor code:

/* Colorspace conversion matrix coefficients and offsets */
struct color_matrix_coefs {
/* Input offsets */
s16 offint1;
s16 offint2;
s16 offint3;
/* Coeficients */
s16 p11coef;
s16 p12coef;
s16 p13coef;
s16 p21coef;
s16 p22coef;
s16 p23coef;
s16 p31coef;
s16 p32coef;
s16 p33coef;
/* Output offsets */
s16 offout1;
s16 offout2;
s16 offout3;
};
/* Conversion matrixes */
enum {
ITU709_RGBLIMITED,
ITU601_RGBLIMITED,
RGBLIMITED_ITU601,
   };
   static const struct color_matrix_coefs conv_matrix[] = {
/* ITU709 -> RGBLimited */
{
-256, -2048,  -2048,
4096, -1875,   -750,
4096,  6307,  0,
4096, 0,   7431,
 256,   256,256,
},
/* YUV601 limited -> RGB limited */
{
-256, -2048,  -2048,
4096, -2860,  -1378,
4096,  5615,  0,
4096, 0,   7097,
256,256,256,
},
/* RGB limited -> ITU601 */
{
-256,  -256,   -256,
2404,  1225,467,
-1754, 2095,   -341,
-1388, -707,   2095,
256,   2048,   2048,
},
};

Assuming the above are correct this leaves me missing RGB limitted ->
RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
YUV601.

I don't have documentation for the registers but I'm assuming the
input offset is applied first, then the multiplication by the coef,
then the output offset is applied. I'm looking over
https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
but I'm unable to figure out how to apply those to the above. Any
ideas?

Thanks,

Tim

Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Mon, Oct 23, 2017 at 10:05 AM, Tim Harvey  wrote:
>
> On Fri, Oct 20, 2017 at 9:23 AM, Hans Verkuil  wrote:
>
> >>
> >> I see the AVI infoframe has hdmi_quantization_range and
> >> hdmi_ycc_quantization_range along with vid_code.
> >>
> >> I'm not at all clear what to do with this information. Is there
> >> anything you see in the datasheet [1] that points to something I need
> >> to be doing?
> >
> > You can ignore hdmi_ycc_quantization_range, it is the 
> > hdmi_quantization_range
> > that you need to read out.
> >
> > The TDA can receive the following formats:
> >
> > RGB Full Range
> > RGB Limited Range
> > YUV Bt.601 (aka SMPTE 170M)
> > YUV Rec.709
> >
> > The YUV formats are always limited range.
> >
> > The TDA can transmit RGB and YUV to the SoC. You want RGB to be full range 
> > and
> > YUV to be limited range. YUV can be either 601 or 709.
> >
> > So if the TDA transmits RGB then you need to support the following 
> > conversions:
> >
> > RGB Full -> RGB Full
> > RGB Limited -> RGB Full
> > YUV 601 -> RGB Full
> > YUV 709 -> RGB Full
> >
> > And if the TDA transmits YUV then you need these conversions:
> >
> > RGB Full -> YUV601 or YUV709
> > RGB Limited -> YUV601 or YUV709
> > YUV601 -> YUV601
> > YUV709 -> YUV709
> >
> > For the RGB to YUV conversion you have a choice of converting to YUV601 or 
> > 709.
> > I recommend to either always convert to YUV601 or to let it depend on the 
> > resolution
> > (SDTV YUV601, HDTV YUV709).
> >
>
> Ok - this is a good explanation that I should be able to follow. I
> will make sure to take into account hdmi_quantization_range when I
> setup the colorspace conversion matrix for v3.

Hans,

I'm having trouble figuring out the conversion matrix to use between
limited and full.

Currently I have the following conversion matrices, the values which
came from some old vendor code:

/* Colorspace conversion matrix coefficients and offsets */
struct color_matrix_coefs {
/* Input offsets */
s16 offint1;
s16 offint2;
s16 offint3;
/* Coeficients */
s16 p11coef;
s16 p12coef;
s16 p13coef;
s16 p21coef;
s16 p22coef;
s16 p23coef;
s16 p31coef;
s16 p32coef;
s16 p33coef;
/* Output offsets */
s16 offout1;
s16 offout2;
s16 offout3;
};
/* Conversion matrixes */
enum {
ITU709_RGBLIMITED,
ITU601_RGBLIMITED,
RGBLIMITED_ITU601,
   };
   static const struct color_matrix_coefs conv_matrix[] = {
/* ITU709 -> RGBLimited */
{
-256, -2048,  -2048,
4096, -1875,   -750,
4096,  6307,  0,
4096, 0,   7431,
 256,   256,256,
},
/* YUV601 limited -> RGB limited */
{
-256, -2048,  -2048,
4096, -2860,  -1378,
4096,  5615,  0,
4096, 0,   7097,
256,256,256,
},
/* RGB limited -> ITU601 */
{
-256,  -256,   -256,
2404,  1225,467,
-1754, 2095,   -341,
-1388, -707,   2095,
256,   2048,   2048,
},
};

Assuming the above are correct this leaves me missing RGB limitted ->
RGB full, YUV601 -> RGB full, YUV709 -> RGB full, and RGB Full ->
YUV601.

I don't have documentation for the registers but I'm assuming the
input offset is applied first, then the multiplication by the coef,
then the output offset is applied. I'm looking over
https://en.wikipedia.org/wiki/YUV for colorspace conversion matrices
but I'm unable to figure out how to apply those to the above. Any
ideas?

Thanks,

Tim

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 4:04 PM, Laura Abbott  wrote:
> __{get,put}_user calls are designed to be fast and have no checks,
> relying on the caller to have made the appropriate calls previously.
> It's very easy to forget a check though, leaving the kernel vulnerable
> to exploits. Add an option to do the checks and kill the kernel if it
> catches something bad.
>
> Signed-off-by: Laura Abbott 
> ---
> This is the actual implemtation for __{get,put}_user on x86 based on
> Mark Rutland's work for arm66
> lkml.kernel.org/r/<20171026090942.7041-1-mark.rutl...@arm.com>
>
> x86 turns out to be easier since the safe and unsafe paths are mostly
> disjoint so we don't have to worry about gcc optimizing out access_ok.
> I tweaked the Kconfig to someting a bit more generic.
>
> The size increase was ~8K in text with a config I tested.

Specifically, this feature would have caught the waitid() bug in 4.13
immediately.

> ---
>  arch/x86/Kconfig   |  3 +++
>  arch/x86/include/asm/uaccess.h | 11 ++-
>  security/Kconfig   | 11 +++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2fdb23313dd5..10c6e150a91e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -261,6 +261,9 @@ config RWSEM_XCHGADD_ALGORITHM
>  config GENERIC_CALIBRATE_DELAY
> def_bool y
>
> +config ARCH_HAS_PARANOID_UACCESS
> +   def_bool y
> +
>  config ARCH_HAS_CPU_RELAX
> def_bool y
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index d23fb5844404..767febe1c720 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -132,6 +132,13 @@ extern int __get_user_bad(void);
>  #define __inttype(x) \
>  __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>
> +
> +#define verify_uaccess(dir, ptr)\
> +({  \
> +if (IS_ENABLED(CONFIG_PARANOID_UACCESS))  \
> +BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr; \
> +})
> +
>  /**
>   * get_user: - Get a simple variable from user space.
>   * @x:   Variable to store result.
> @@ -278,6 +285,7 @@ do {  
>   \
> typeof(ptr) __pu_ptr = (ptr);   \
> retval = 0; \
> __chk_user_ptr(__pu_ptr);   \
> +   verify_uaccess(VERIFY_WRITE, __pu_ptr); \
> switch (size) { \
> case 1: \
> __put_user_asm(x, __pu_ptr, retval, "b", "b", "iq", \
> @@ -293,7 +301,7 @@ do {  
>   \
> break;  \
> case 8: \
> __put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr, \
> -   retval, \ errret);  \
> +   retval, errret);\
> break;  \
> default:\
> __put_user_bad();   \

Which tree is this against? I don't see the weird line break in my tree?

> @@ -359,6 +367,7 @@ do {  
>   \
> typeof(ptr) __gu_ptr = (ptr);   \
> retval = 0; \
> __chk_user_ptr(__gu_ptr);   \
> +   verify_uaccess(VERIFY_READ, __gu_ptr);  \
> switch (size) { \
> case 1: \
> __get_user_asm(x, __gu_ptr, retval, "b", "b", "=q", \

Does __put/get_user_size_ex() need additions too? (And does
put/get_user_ex() lack access_ok() checks as-is? Looks like the users
are have access_ok() checks, but that naming really shouldn't be
aliased to "put/get_user_ex" -- otherwise it gives the impression it's
doing access_ok() checks...)

> diff --git a/security/Kconfig b/security/Kconfig
> index e8e449444e65..0a9ec1a4e86b 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -205,6 +205,17 @@ config STATIC_USERMODEHELPER_PATH
>   If you wish for all usermode helper programs to be disabled,
>   specify an empty string here (i.e. "").
>
> +config 

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 4:04 PM, Laura Abbott  wrote:
> __{get,put}_user calls are designed to be fast and have no checks,
> relying on the caller to have made the appropriate calls previously.
> It's very easy to forget a check though, leaving the kernel vulnerable
> to exploits. Add an option to do the checks and kill the kernel if it
> catches something bad.
>
> Signed-off-by: Laura Abbott 
> ---
> This is the actual implemtation for __{get,put}_user on x86 based on
> Mark Rutland's work for arm66
> lkml.kernel.org/r/<20171026090942.7041-1-mark.rutl...@arm.com>
>
> x86 turns out to be easier since the safe and unsafe paths are mostly
> disjoint so we don't have to worry about gcc optimizing out access_ok.
> I tweaked the Kconfig to someting a bit more generic.
>
> The size increase was ~8K in text with a config I tested.

Specifically, this feature would have caught the waitid() bug in 4.13
immediately.

> ---
>  arch/x86/Kconfig   |  3 +++
>  arch/x86/include/asm/uaccess.h | 11 ++-
>  security/Kconfig   | 11 +++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2fdb23313dd5..10c6e150a91e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -261,6 +261,9 @@ config RWSEM_XCHGADD_ALGORITHM
>  config GENERIC_CALIBRATE_DELAY
> def_bool y
>
> +config ARCH_HAS_PARANOID_UACCESS
> +   def_bool y
> +
>  config ARCH_HAS_CPU_RELAX
> def_bool y
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index d23fb5844404..767febe1c720 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -132,6 +132,13 @@ extern int __get_user_bad(void);
>  #define __inttype(x) \
>  __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>
> +
> +#define verify_uaccess(dir, ptr)\
> +({  \
> +if (IS_ENABLED(CONFIG_PARANOID_UACCESS))  \
> +BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr; \
> +})
> +
>  /**
>   * get_user: - Get a simple variable from user space.
>   * @x:   Variable to store result.
> @@ -278,6 +285,7 @@ do {  
>   \
> typeof(ptr) __pu_ptr = (ptr);   \
> retval = 0; \
> __chk_user_ptr(__pu_ptr);   \
> +   verify_uaccess(VERIFY_WRITE, __pu_ptr); \
> switch (size) { \
> case 1: \
> __put_user_asm(x, __pu_ptr, retval, "b", "b", "iq", \
> @@ -293,7 +301,7 @@ do {  
>   \
> break;  \
> case 8: \
> __put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr, \
> -   retval, \ errret);  \
> +   retval, errret);\
> break;  \
> default:\
> __put_user_bad();   \

Which tree is this against? I don't see the weird line break in my tree?

> @@ -359,6 +367,7 @@ do {  
>   \
> typeof(ptr) __gu_ptr = (ptr);   \
> retval = 0; \
> __chk_user_ptr(__gu_ptr);   \
> +   verify_uaccess(VERIFY_READ, __gu_ptr);  \
> switch (size) { \
> case 1: \
> __get_user_asm(x, __gu_ptr, retval, "b", "b", "=q", \

Does __put/get_user_size_ex() need additions too? (And does
put/get_user_ex() lack access_ok() checks as-is? Looks like the users
are have access_ok() checks, but that naming really shouldn't be
aliased to "put/get_user_ex" -- otherwise it gives the impression it's
doing access_ok() checks...)

> diff --git a/security/Kconfig b/security/Kconfig
> index e8e449444e65..0a9ec1a4e86b 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -205,6 +205,17 @@ config STATIC_USERMODEHELPER_PATH
>   If you wish for all usermode helper programs to be disabled,
>   specify an empty string here (i.e. "").
>
> +config PARANOID_UACCESS
> +   bool "Use 

Re: [PATCH] HID: Wacom: switch Dell canvas into highres mode

[Jason Gerecke]

On November 3, 2017 10:29:47 AM PDT, Benjamin Tissoires 
 wrote:
>The Dell Canvas exports 2 collections for the Pen part. The only
>difference between the 2 is that the default one has half the
>resolution
>of the second one.
>
>The Windows driver switches the tablet into the second mode, so we
>should
>behave the same.
>
>Signed-off-by: Benjamin Tissoires 
>---
>
>Hi,
>
>well, this is not the cleanest way of handling this mode set, but it is
>the
>less intrusive AFAICT.
>
>I was thinking that we might want to add a new field in struct
>wacom_feature,
>but then it wouldn't make sense to keep the set mode specifics in
>_wacom_query_tablet_data().
>
>So I am open to any better suggestion, but this one works and the
>impact on
>other HID generic tablets is null.
>

Hmm. Agreed that this isn't ideal. I'd like to take a look at the descriptor, 
if you wouldn't mind sharing it. The hardware should be similar to other 
devices of ours and it might be possible to piggyback on existing code.

Jason

>Cheers,
>Benjamin
>
> drivers/hid/wacom_sys.c | 7 +++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>index 906e654fb0ba..ee71ad9b6cc1 100644
>--- a/drivers/hid/wacom_sys.c
>+++ b/drivers/hid/wacom_sys.c
>@@ -196,6 +196,13 @@ static void wacom_feature_mapping(struct
>hid_device *hdev,
>   kfree(data);
>   break;
>   }
>+
>+  if (hdev->vendor == USB_VENDOR_ID_WACOM &&
>+  hdev->product == 0x4200 /* Dell Canvas 27 */ &&
>+  field->application == HID_UP_MSVENDOR) {
>+  wacom->wacom_wac.mode_report = field->report->id;
>+  wacom->wacom_wac.mode_value = 2;
>+  }
> }
> 
> /*


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] HID: Wacom: switch Dell canvas into highres mode

[Jason Gerecke]

On November 3, 2017 10:29:47 AM PDT, Benjamin Tissoires 
 wrote:
>The Dell Canvas exports 2 collections for the Pen part. The only
>difference between the 2 is that the default one has half the
>resolution
>of the second one.
>
>The Windows driver switches the tablet into the second mode, so we
>should
>behave the same.
>
>Signed-off-by: Benjamin Tissoires 
>---
>
>Hi,
>
>well, this is not the cleanest way of handling this mode set, but it is
>the
>less intrusive AFAICT.
>
>I was thinking that we might want to add a new field in struct
>wacom_feature,
>but then it wouldn't make sense to keep the set mode specifics in
>_wacom_query_tablet_data().
>
>So I am open to any better suggestion, but this one works and the
>impact on
>other HID generic tablets is null.
>

Hmm. Agreed that this isn't ideal. I'd like to take a look at the descriptor, 
if you wouldn't mind sharing it. The hardware should be similar to other 
devices of ours and it might be possible to piggyback on existing code.

Jason

>Cheers,
>Benjamin
>
> drivers/hid/wacom_sys.c | 7 +++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>index 906e654fb0ba..ee71ad9b6cc1 100644
>--- a/drivers/hid/wacom_sys.c
>+++ b/drivers/hid/wacom_sys.c
>@@ -196,6 +196,13 @@ static void wacom_feature_mapping(struct
>hid_device *hdev,
>   kfree(data);
>   break;
>   }
>+
>+  if (hdev->vendor == USB_VENDOR_ID_WACOM &&
>+  hdev->product == 0x4200 /* Dell Canvas 27 */ &&
>+  field->application == HID_UP_MSVENDOR) {
>+  wacom->wacom_wac.mode_report = field->report->id;
>+  wacom->wacom_wac.mode_value = 2;
>+  }
> }
> 
> /*


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v5 3/3] KVM: nVMX: Fix mmu context after VMLAUNCH/VMRESUME failure

[Krish Sadhukhan]

On 11/02/2017 05:50 PM, Wanpeng Li wrote:


From: Wanpeng Li 

Commit 4f350c6dbcb (kvm: nVMX: Handle deferred early VMLAUNCH/VMRESUME failure
properly) can result in L1(run kvm-unit-tests/run_tests.sh vmx_controls in L1)
null pointer deference and also L0 calltrace when EPT=0 on both L0 and L1.

In L1:

BUG: unable to handle kernel paging request at c015bf8f
  IP: vmx_vcpu_run+0x202/0x510 [kvm_intel]
  PGD 146e13067 P4D 146e13067 PUD 146e15067 PMD 3d2686067 PTE 3d4af9161
  Oops: 0003 [#1] PREEMPT SMP
  CPU: 2 PID: 1798 Comm: qemu-system-x86 Not tainted 4.14.0-rc4+ #6
  RIP: 0010:vmx_vcpu_run+0x202/0x510 [kvm_intel]
  Call Trace:
  WARNING: kernel stack frame pointer at b86f4988bc18 in 
qemu-system-x86:1798 has bad value 0002

In L0:

---[ cut here ]
  WARNING: CPU: 6 PID: 4460 at /home/kernel/linux/arch/x86/kvm//vmx.c:9845 
vmx_inject_page_fault_nested+0x130/0x140 [kvm_intel]
  CPU: 6 PID: 4460 Comm: qemu-system-x86 Tainted: G   OE   4.14.0-rc7+ 
#25
  RIP: 0010:vmx_inject_page_fault_nested+0x130/0x140 [kvm_intel]
  Call Trace:
   paging64_page_fault+0x500/0xde0 [kvm]
   ? paging32_gva_to_gpa_nested+0x120/0x120 [kvm]
   ? nonpaging_page_fault+0x3b0/0x3b0 [kvm]
   ? __asan_storeN+0x12/0x20
   ? paging64_gva_to_gpa+0xb0/0x120 [kvm]
   ? paging64_walk_addr_generic+0x11a0/0x11a0 [kvm]
   ? lock_acquire+0x2c0/0x2c0
   ? vmx_read_guest_seg_ar+0x97/0x100 [kvm_intel]
   ? vmx_get_segment+0x2a6/0x310 [kvm_intel]
   ? sched_clock+0x1f/0x30
   ? check_chain_key+0x137/0x1e0
   ? __lock_acquire+0x83c/0x2420
   ? kvm_multiple_exception+0xf2/0x220 [kvm]
   ? debug_check_no_locks_freed+0x240/0x240
   ? debug_smp_processor_id+0x17/0x20
   ? __lock_is_held+0x9e/0x100
   kvm_mmu_page_fault+0x90/0x180 [kvm]
   kvm_handle_page_fault+0x15c/0x310 [kvm]
   ? __lock_is_held+0x9e/0x100
   handle_exception+0x3c7/0x4d0 [kvm_intel]
   vmx_handle_exit+0x103/0x1010 [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run+0x1628/0x2e20 [kvm]

The commit avoids to load host state of vmcs12 as vmcs01's guest state
since vmcs12 is not modified (except for the VM-instruction error field)
if the checking of vmcs control area fails. However, the mmu context is
switched to nested mmu in prepare_vmcs02() and it will not be reloaded
since load_vmcs12_host_state() is skipped when nested VMLAUNCH/VMRESUME
fails. This patch fixes it by reloading mmu context when nested
VMLAUNCH/VMRESUME fails.

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Jim Mattson 
Signed-off-by: Wanpeng Li 
---
v3 -> v4:
  * move it to a new function load_vmcs12_mmu_host_state

  arch/x86/kvm/vmx.c | 34 ++
  1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6cf3972..8aefb91 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11259,6 +11259,24 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, 
struct vmcs12 *vmcs12,
kvm_clear_interrupt_queue(vcpu);
  }
  
+static void load_vmcs12_mmu_host_state(struct kvm_vcpu *vcpu,

+   struct vmcs12 *vmcs12)
+{
+   u32 entry_failure_code;
+
+   nested_ept_uninit_mmu_context(vcpu);
+
+   /*
+* Only PDPTE load can fail as the value of cr3 was checked on entry and
+* couldn't have changed.
+*/
+   if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, 
_failure_code))
+   nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
+
+   if (!enable_ept)
+   vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault;
+}
+
  /*
   * A part of what we need to when the nested L2 guest exits and we want to
   * run its L1 parent, is to reset L1's guest state to the host state specified
@@ -11272,7 +11290,6 @@ static void load_vmcs12_host_state(struct kvm_vcpu 
*vcpu,
   struct vmcs12 *vmcs12)
  {
struct kvm_segment seg;
-   u32 entry_failure_code;
  
  	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)

vcpu->arch.efer = vmcs12->host_ia32_efer;
@@ -11299,17 +11316,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu 
*vcpu,
vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK);
vmx_set_cr4(vcpu, vmcs12->host_cr4);
  
-	nested_ept_uninit_mmu_context(vcpu);

-
-   /*
-* Only PDPTE load can fail as the value of cr3 was checked on entry and
-* couldn't have changed.
-*/
-   if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, 
_failure_code))
-   nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
-
-   if (!enable_ept)
-   vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault;
+   load_vmcs12_mmu_host_state(vcpu, vmcs12);
  
  	if (enable_vpid) {

/*
@@ -11539,6 +11546,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, 

Re: [PATCH v5 3/3] KVM: nVMX: Fix mmu context after VMLAUNCH/VMRESUME failure

[Krish Sadhukhan]

On 11/02/2017 05:50 PM, Wanpeng Li wrote:


From: Wanpeng Li 

Commit 4f350c6dbcb (kvm: nVMX: Handle deferred early VMLAUNCH/VMRESUME failure
properly) can result in L1(run kvm-unit-tests/run_tests.sh vmx_controls in L1)
null pointer deference and also L0 calltrace when EPT=0 on both L0 and L1.

In L1:

BUG: unable to handle kernel paging request at c015bf8f
  IP: vmx_vcpu_run+0x202/0x510 [kvm_intel]
  PGD 146e13067 P4D 146e13067 PUD 146e15067 PMD 3d2686067 PTE 3d4af9161
  Oops: 0003 [#1] PREEMPT SMP
  CPU: 2 PID: 1798 Comm: qemu-system-x86 Not tainted 4.14.0-rc4+ #6
  RIP: 0010:vmx_vcpu_run+0x202/0x510 [kvm_intel]
  Call Trace:
  WARNING: kernel stack frame pointer at b86f4988bc18 in 
qemu-system-x86:1798 has bad value 0002

In L0:

---[ cut here ]
  WARNING: CPU: 6 PID: 4460 at /home/kernel/linux/arch/x86/kvm//vmx.c:9845 
vmx_inject_page_fault_nested+0x130/0x140 [kvm_intel]
  CPU: 6 PID: 4460 Comm: qemu-system-x86 Tainted: G   OE   4.14.0-rc7+ 
#25
  RIP: 0010:vmx_inject_page_fault_nested+0x130/0x140 [kvm_intel]
  Call Trace:
   paging64_page_fault+0x500/0xde0 [kvm]
   ? paging32_gva_to_gpa_nested+0x120/0x120 [kvm]
   ? nonpaging_page_fault+0x3b0/0x3b0 [kvm]
   ? __asan_storeN+0x12/0x20
   ? paging64_gva_to_gpa+0xb0/0x120 [kvm]
   ? paging64_walk_addr_generic+0x11a0/0x11a0 [kvm]
   ? lock_acquire+0x2c0/0x2c0
   ? vmx_read_guest_seg_ar+0x97/0x100 [kvm_intel]
   ? vmx_get_segment+0x2a6/0x310 [kvm_intel]
   ? sched_clock+0x1f/0x30
   ? check_chain_key+0x137/0x1e0
   ? __lock_acquire+0x83c/0x2420
   ? kvm_multiple_exception+0xf2/0x220 [kvm]
   ? debug_check_no_locks_freed+0x240/0x240
   ? debug_smp_processor_id+0x17/0x20
   ? __lock_is_held+0x9e/0x100
   kvm_mmu_page_fault+0x90/0x180 [kvm]
   kvm_handle_page_fault+0x15c/0x310 [kvm]
   ? __lock_is_held+0x9e/0x100
   handle_exception+0x3c7/0x4d0 [kvm_intel]
   vmx_handle_exit+0x103/0x1010 [kvm_intel]
   ? kvm_arch_vcpu_ioctl_run+0x1628/0x2e20 [kvm]

The commit avoids to load host state of vmcs12 as vmcs01's guest state
since vmcs12 is not modified (except for the VM-instruction error field)
if the checking of vmcs control area fails. However, the mmu context is
switched to nested mmu in prepare_vmcs02() and it will not be reloaded
since load_vmcs12_host_state() is skipped when nested VMLAUNCH/VMRESUME
fails. This patch fixes it by reloading mmu context when nested
VMLAUNCH/VMRESUME fails.

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Jim Mattson 
Signed-off-by: Wanpeng Li 
---
v3 -> v4:
  * move it to a new function load_vmcs12_mmu_host_state

  arch/x86/kvm/vmx.c | 34 ++
  1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6cf3972..8aefb91 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11259,6 +11259,24 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, 
struct vmcs12 *vmcs12,
kvm_clear_interrupt_queue(vcpu);
  }
  
+static void load_vmcs12_mmu_host_state(struct kvm_vcpu *vcpu,

+   struct vmcs12 *vmcs12)
+{
+   u32 entry_failure_code;
+
+   nested_ept_uninit_mmu_context(vcpu);
+
+   /*
+* Only PDPTE load can fail as the value of cr3 was checked on entry and
+* couldn't have changed.
+*/
+   if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, 
_failure_code))
+   nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
+
+   if (!enable_ept)
+   vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault;
+}
+
  /*
   * A part of what we need to when the nested L2 guest exits and we want to
   * run its L1 parent, is to reset L1's guest state to the host state specified
@@ -11272,7 +11290,6 @@ static void load_vmcs12_host_state(struct kvm_vcpu 
*vcpu,
   struct vmcs12 *vmcs12)
  {
struct kvm_segment seg;
-   u32 entry_failure_code;
  
  	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)

vcpu->arch.efer = vmcs12->host_ia32_efer;
@@ -11299,17 +11316,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu 
*vcpu,
vcpu->arch.cr4_guest_owned_bits = ~vmcs_readl(CR4_GUEST_HOST_MASK);
vmx_set_cr4(vcpu, vmcs12->host_cr4);
  
-	nested_ept_uninit_mmu_context(vcpu);

-
-   /*
-* Only PDPTE load can fail as the value of cr3 was checked on entry and
-* couldn't have changed.
-*/
-   if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, 
_failure_code))
-   nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
-
-   if (!enable_ept)
-   vcpu->arch.walk_mmu->inject_page_fault = kvm_inject_page_fault;
+   load_vmcs12_mmu_host_state(vcpu, vmcs12);
  
  	if (enable_vpid) {

/*
@@ -11539,6 +11546,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, 
u32 exit_reason,
 * accordingly.
 */
nested_vmx_failValid(vcpu, 

Re: [Toybox] Regression: commit da029c11e6b1 broke toybox xargs.

[enh]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> On 11/02/2017 10:40 AM, Linus Torvalds wrote:
>> On Wed, Nov 1, 2017 at 9:28 PM, Linus Torvalds
>>  wrote:
>>>
>>> Behavior changed. Things that test particular limits will get different
>>> results. That's not breakage.
>>>
>>> Did an actual user application or script break?
>
> Only due to getting the limit wrong. The actual failure's in the android
> internal bugzilla I've never been able to read:
>
> http://lists.landley.net/pipermail/toybox-landley.net/2017-September/009167.html

there was nothing secret or interesting in the original bug report.
just someone trying to use find/xargs on the whole file system. here's
a copy & paste:

marlin:/ # find / /data -xdev -type f ! -name \*.jpg -exec grep -Fl
belkin.a {} +
find: exec grep: Argument list too long

marlin:/ # find / /data -xdev -type f ! -name \*.jpg -print | xargs
grep -Fl belkin.a
xargs: exec grep: Argument list too long

> But it boils down to "got the limit wrong, the exec failed after the
> fork(), dynamic recovery from which is awkward so I'm trying to figure
> out the right limit".
>
>> Ahh. I should have read that email more carefully. If xargs broke,
>> that _will_ break actual scripts, yes. Do you actually set the stack
>> limit to insane values? Anybody using toybox really shouldn't be doing
>> 32MB stacks.
>
> Toybox is the default command line of android since M, which went 64 bit
> in L, and the Pixel 2 phone has 4 gigs of ram. My goal with toybox is to
> turn android into a self-hosting development environment no longer
> cross-compiled from a PC (http://landley.net/talks/celf-2013.txt) so I'm
> trying to implement a command line that can run the entire AOSP build.
>
> I.E. I have no idea what people will do with it, and try not to get in
> their way.
>
> My problem here is it's hard to figure out what exec size the limit
> _is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
> currently returning as stack_limit/4, which is now too big and exec()
> will error out after the fork. Musl is returning the 131072 limit from
> 2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
> "printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
> avoid. Maybe I don't have that luxury...
>
> Each argument has its own limit separate from the argv+envp total limit,
> but there's only one "size" you can query through sysconf, so the
> querying API is insufficient at the design level.
>
> Meanwhile under bash you can allocate and dirty 256 megabytes from the
> command line with:
>
>   echo $(printf '%0*d' $((1<<28)))
>
> Because it's a shell builtin so there's no actual exec. (And if
> https://sourceware.org/bugzilla/show_bug.cgi?id=17829 ever gets fixed
> it'll go back to allowing INT_MAX.)
>
> Posix is its usual helpful self, read conservatively
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html
> says to break the line at 2048 bytes.
>
>> So I still do wonder if this actually breaks anything real, or just a
>> test-suite or something?
>
> I've cc'd Elliott, who would know. (He's the Android base os userspace
> maintainer, he knows everything. Or can at least decode
> http://b/65818597 .)
>
> But this just broke my _fix_, not the earlier deployed stuff. I removed
> the size measuring code when the 131072 limit went away, the bug was
> there's a new limit I need to not hit, I tried to figure out what the
> limit is now, confirmed that the various libc implementations don't
> agree, then the actual kernel limit changed again while I was looking at it.
>
>>Linus
>
> Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
> than 10 megs, and it sounds like getting it _right_ is unachievable.
>
> Thanks,
>
> Rob
>
>
> ___
> Toybox mailing list
> toy...@lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [Toybox] Regression: commit da029c11e6b1 broke toybox xargs.

[enh]

On Fri, Nov 3, 2017 at 4:58 PM, Rob Landley  wrote:
> On 11/02/2017 10:40 AM, Linus Torvalds wrote:
>> On Wed, Nov 1, 2017 at 9:28 PM, Linus Torvalds
>>  wrote:
>>>
>>> Behavior changed. Things that test particular limits will get different
>>> results. That's not breakage.
>>>
>>> Did an actual user application or script break?
>
> Only due to getting the limit wrong. The actual failure's in the android
> internal bugzilla I've never been able to read:
>
> http://lists.landley.net/pipermail/toybox-landley.net/2017-September/009167.html

there was nothing secret or interesting in the original bug report.
just someone trying to use find/xargs on the whole file system. here's
a copy & paste:

marlin:/ # find / /data -xdev -type f ! -name \*.jpg -exec grep -Fl
belkin.a {} +
find: exec grep: Argument list too long

marlin:/ # find / /data -xdev -type f ! -name \*.jpg -print | xargs
grep -Fl belkin.a
xargs: exec grep: Argument list too long

> But it boils down to "got the limit wrong, the exec failed after the
> fork(), dynamic recovery from which is awkward so I'm trying to figure
> out the right limit".
>
>> Ahh. I should have read that email more carefully. If xargs broke,
>> that _will_ break actual scripts, yes. Do you actually set the stack
>> limit to insane values? Anybody using toybox really shouldn't be doing
>> 32MB stacks.
>
> Toybox is the default command line of android since M, which went 64 bit
> in L, and the Pixel 2 phone has 4 gigs of ram. My goal with toybox is to
> turn android into a self-hosting development environment no longer
> cross-compiled from a PC (http://landley.net/talks/celf-2013.txt) so I'm
> trying to implement a command line that can run the entire AOSP build.
>
> I.E. I have no idea what people will do with it, and try not to get in
> their way.
>
> My problem here is it's hard to figure out what exec size the limit
> _is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
> currently returning as stack_limit/4, which is now too big and exec()
> will error out after the fork. Musl is returning the 131072 limit from
> 2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
> "printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
> avoid. Maybe I don't have that luxury...
>
> Each argument has its own limit separate from the argv+envp total limit,
> but there's only one "size" you can query through sysconf, so the
> querying API is insufficient at the design level.
>
> Meanwhile under bash you can allocate and dirty 256 megabytes from the
> command line with:
>
>   echo $(printf '%0*d' $((1<<28)))
>
> Because it's a shell builtin so there's no actual exec. (And if
> https://sourceware.org/bugzilla/show_bug.cgi?id=17829 ever gets fixed
> it'll go back to allowing INT_MAX.)
>
> Posix is its usual helpful self, read conservatively
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html
> says to break the line at 2048 bytes.
>
>> So I still do wonder if this actually breaks anything real, or just a
>> test-suite or something?
>
> I've cc'd Elliott, who would know. (He's the Android base os userspace
> maintainer, he knows everything. Or can at least decode
> http://b/65818597 .)
>
> But this just broke my _fix_, not the earlier deployed stuff. I removed
> the size measuring code when the 131072 limit went away, the bug was
> there's a new limit I need to not hit, I tried to figure out what the
> limit is now, confirmed that the various libc implementations don't
> agree, then the actual kernel limit changed again while I was looking at it.
>
>>Linus
>
> Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
> than 10 megs, and it sounds like getting it _right_ is unachievable.
>
> Thanks,
>
> Rob
>
>
> ___
> Toybox mailing list
> toy...@lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [PATCH 6/6] memfd-tests: test hugetlbfs sealing

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Remove most of the special-casing of hugetlbfs now that sealing
> is supported.

The changes below look fine.  Just a couple issues.

While discussing patch 4 with David, I realized that we should modify/expand
the fuse seals test to also verify proper functionality with hugetlbfs.  I
think this is a requirement.

The output of run_tests.sh looks something like:
opening: ./mnt/memfd
fuse: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE

It might be nice to distinguish testing of tmpfs and hugetlbfs.  The
string '#define MEMFD_STR "memfd:"' is prepended to the output lines.
Perhaps we could do something like:
#define MEMFD_STR   "memfd:"
#define MEMFD_HUGE_STR  "memfd-hugetlb"
char *memfd_str;

and then in main,
if (hugetlbfs_test)
memfd_str = MEMFD_HUGE_STR;
else
memfd_str = MEMFD_STR;

then prepend output strings with memfd_str.

This is just a suggestion and optional.

-- 
Mike Kravetz

> 
> Signed-off-by: Marc-André Lureau 
> ---
>  tools/testing/selftests/memfd/memfd_test.c | 150 
> +++--
>  1 file changed, 15 insertions(+), 135 deletions(-)
> 
> diff --git a/tools/testing/selftests/memfd/memfd_test.c 
> b/tools/testing/selftests/memfd/memfd_test.c
> index f94c6d1fb46f..f5028f800107 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -512,6 +512,10 @@ static void mfd_assert_grow_write(int fd)
>   static char *buf;
>   ssize_t l;
>  
> + /* hugetlbfs does not support write */
> + if (hugetlbfs_test)
> + return;
> +
>   buf = malloc(mfd_def_size * 8);
>   if (!buf) {
>   printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -532,6 +536,10 @@ static void mfd_fail_grow_write(int fd)
>   static char *buf;
>   ssize_t l;
>  
> + /* hugetlbfs does not support write */
> + if (hugetlbfs_test)
> + return;
> +
>   buf = malloc(mfd_def_size * 8);
>   if (!buf) {
>   printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -626,18 +634,13 @@ static void test_create(void)
>   fd = mfd_assert_new("", 0, MFD_CLOEXEC);
>   close(fd);
>  
> - if (!hugetlbfs_test) {
> - /* verify MFD_ALLOW_SEALING is allowed */
> - fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> - close(fd);
> -
> - /* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> - fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> - close(fd);
> - } else {
> - /* sealing is not supported on hugetlbfs */
> - mfd_fail_new("", MFD_ALLOW_SEALING);
> - }
> + /* verify MFD_ALLOW_SEALING is allowed */
> + fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> + close(fd);
> +
> + /* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> + fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> + close(fd);
>  }
>  
>  /*
> @@ -648,10 +651,6 @@ static void test_basic(void)
>  {
>   int fd;
>  
> - /* hugetlbfs does not contain sealing support */
> - if (hugetlbfs_test)
> - return;
> -
>   printf("%s BASIC\n", MEMFD_STR);
>  
>   fd = mfd_assert_new("kern_memfd_basic",
> @@ -696,28 +695,6 @@ static void test_basic(void)
>   close(fd);
>  }
>  
> -/*
> - * hugetlbfs doesn't support seals or write, so just verify grow and shrink
> - * on a hugetlbfs file created via memfd_create.
> - */
> -static void test_hugetlbfs_grow_shrink(void)
> -{
> - int fd;
> -
> - printf("%s HUGETLBFS-GROW-SHRINK\n", MEMFD_STR);
> -
> - fd = mfd_assert_new("kern_memfd_seal_write",
> - mfd_def_size,
> - MFD_CLOEXEC);
> -
> - mfd_assert_read(fd);
> - mfd_assert_write(fd);
> - mfd_assert_shrink(fd);
> - mfd_assert_grow(fd);
> -
> - close(fd);
> -}
> -
>  /*
>   * Test SEAL_WRITE
>   * Test whether SEAL_WRITE actually prevents modifications.
> @@ -726,13 +703,6 @@ static void test_seal_write(void)
>  {
>   int fd;
>  
> - /*
> -  * hugetlbfs does not contain sealing or write support.  Just test
> -  * basic grow and shrink via test_hugetlbfs_grow_shrink.
> -  */
> - if (hugetlbfs_test)
> - 

Re: [PATCH 6/6] memfd-tests: test hugetlbfs sealing

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Remove most of the special-casing of hugetlbfs now that sealing
> is supported.

The changes below look fine.  Just a couple issues.

While discussing patch 4 with David, I realized that we should modify/expand
the fuse seals test to also verify proper functionality with hugetlbfs.  I
think this is a requirement.

The output of run_tests.sh looks something like:
opening: ./mnt/memfd
fuse: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE

It might be nice to distinguish testing of tmpfs and hugetlbfs.  The
string '#define MEMFD_STR "memfd:"' is prepended to the output lines.
Perhaps we could do something like:
#define MEMFD_STR   "memfd:"
#define MEMFD_HUGE_STR  "memfd-hugetlb"
char *memfd_str;

and then in main,
if (hugetlbfs_test)
memfd_str = MEMFD_HUGE_STR;
else
memfd_str = MEMFD_STR;

then prepend output strings with memfd_str.

This is just a suggestion and optional.

-- 
Mike Kravetz

> 
> Signed-off-by: Marc-André Lureau 
> ---
>  tools/testing/selftests/memfd/memfd_test.c | 150 
> +++--
>  1 file changed, 15 insertions(+), 135 deletions(-)
> 
> diff --git a/tools/testing/selftests/memfd/memfd_test.c 
> b/tools/testing/selftests/memfd/memfd_test.c
> index f94c6d1fb46f..f5028f800107 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -512,6 +512,10 @@ static void mfd_assert_grow_write(int fd)
>   static char *buf;
>   ssize_t l;
>  
> + /* hugetlbfs does not support write */
> + if (hugetlbfs_test)
> + return;
> +
>   buf = malloc(mfd_def_size * 8);
>   if (!buf) {
>   printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -532,6 +536,10 @@ static void mfd_fail_grow_write(int fd)
>   static char *buf;
>   ssize_t l;
>  
> + /* hugetlbfs does not support write */
> + if (hugetlbfs_test)
> + return;
> +
>   buf = malloc(mfd_def_size * 8);
>   if (!buf) {
>   printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -626,18 +634,13 @@ static void test_create(void)
>   fd = mfd_assert_new("", 0, MFD_CLOEXEC);
>   close(fd);
>  
> - if (!hugetlbfs_test) {
> - /* verify MFD_ALLOW_SEALING is allowed */
> - fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> - close(fd);
> -
> - /* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> - fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> - close(fd);
> - } else {
> - /* sealing is not supported on hugetlbfs */
> - mfd_fail_new("", MFD_ALLOW_SEALING);
> - }
> + /* verify MFD_ALLOW_SEALING is allowed */
> + fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> + close(fd);
> +
> + /* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> + fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> + close(fd);
>  }
>  
>  /*
> @@ -648,10 +651,6 @@ static void test_basic(void)
>  {
>   int fd;
>  
> - /* hugetlbfs does not contain sealing support */
> - if (hugetlbfs_test)
> - return;
> -
>   printf("%s BASIC\n", MEMFD_STR);
>  
>   fd = mfd_assert_new("kern_memfd_basic",
> @@ -696,28 +695,6 @@ static void test_basic(void)
>   close(fd);
>  }
>  
> -/*
> - * hugetlbfs doesn't support seals or write, so just verify grow and shrink
> - * on a hugetlbfs file created via memfd_create.
> - */
> -static void test_hugetlbfs_grow_shrink(void)
> -{
> - int fd;
> -
> - printf("%s HUGETLBFS-GROW-SHRINK\n", MEMFD_STR);
> -
> - fd = mfd_assert_new("kern_memfd_seal_write",
> - mfd_def_size,
> - MFD_CLOEXEC);
> -
> - mfd_assert_read(fd);
> - mfd_assert_write(fd);
> - mfd_assert_shrink(fd);
> - mfd_assert_grow(fd);
> -
> - close(fd);
> -}
> -
>  /*
>   * Test SEAL_WRITE
>   * Test whether SEAL_WRITE actually prevents modifications.
> @@ -726,13 +703,6 @@ static void test_seal_write(void)
>  {
>   int fd;
>  
> - /*
> -  * hugetlbfs does not contain sealing or write support.  Just test
> -  * basic grow and shrink via test_hugetlbfs_grow_shrink.
> -  */
> - if (hugetlbfs_test)
> - return 

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Rob Landley]

On 11/02/2017 10:40 AM, Linus Torvalds wrote:
> On Wed, Nov 1, 2017 at 9:28 PM, Linus Torvalds
>  wrote:
>>
>> Behavior changed. Things that test particular limits will get different
>> results. That's not breakage.
>>
>> Did an actual user application or script break?

Only due to getting the limit wrong. The actual failure's in the android
internal bugzilla I've never been able to read:

http://lists.landley.net/pipermail/toybox-landley.net/2017-September/009167.html

But it boils down to "got the limit wrong, the exec failed after the
fork(), dynamic recovery from which is awkward so I'm trying to figure
out the right limit".

> Ahh. I should have read that email more carefully. If xargs broke,
> that _will_ break actual scripts, yes. Do you actually set the stack
> limit to insane values? Anybody using toybox really shouldn't be doing
> 32MB stacks.

Toybox is the default command line of android since M, which went 64 bit
in L, and the Pixel 2 phone has 4 gigs of ram. My goal with toybox is to
turn android into a self-hosting development environment no longer
cross-compiled from a PC (http://landley.net/talks/celf-2013.txt) so I'm
trying to implement a command line that can run the entire AOSP build.

I.E. I have no idea what people will do with it, and try not to get in
their way.

My problem here is it's hard to figure out what exec size the limit
_is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
currently returning as stack_limit/4, which is now too big and exec()
will error out after the fork. Musl is returning the 131072 limit from
2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
"printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
avoid. Maybe I don't have that luxury...

Each argument has its own limit separate from the argv+envp total limit,
but there's only one "size" you can query through sysconf, so the
querying API is insufficient at the design level.

Meanwhile under bash you can allocate and dirty 256 megabytes from the
command line with:

  echo $(printf '%0*d' $((1<<28)))

Because it's a shell builtin so there's no actual exec. (And if
https://sourceware.org/bugzilla/show_bug.cgi?id=17829 ever gets fixed
it'll go back to allowing INT_MAX.)

Posix is its usual helpful self, read conservatively
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html
says to break the line at 2048 bytes.

> So I still do wonder if this actually breaks anything real, or just a
> test-suite or something?

I've cc'd Elliott, who would know. (He's the Android base os userspace
maintainer, he knows everything. Or can at least decode
http://b/65818597 .)

But this just broke my _fix_, not the earlier deployed stuff. I removed
the size measuring code when the 131072 limit went away, the bug was
there's a new limit I need to not hit, I tried to figure out what the
limit is now, confirmed that the various libc implementations don't
agree, then the actual kernel limit changed again while I was looking at it.

>Linus

Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
than 10 megs, and it sounds like getting it _right_ is unachievable.

Thanks,

Rob

Re: Regression: commit da029c11e6b1 broke toybox xargs.

[Rob Landley]

On 11/02/2017 10:40 AM, Linus Torvalds wrote:
> On Wed, Nov 1, 2017 at 9:28 PM, Linus Torvalds
>  wrote:
>>
>> Behavior changed. Things that test particular limits will get different
>> results. That's not breakage.
>>
>> Did an actual user application or script break?

Only due to getting the limit wrong. The actual failure's in the android
internal bugzilla I've never been able to read:

http://lists.landley.net/pipermail/toybox-landley.net/2017-September/009167.html

But it boils down to "got the limit wrong, the exec failed after the
fork(), dynamic recovery from which is awkward so I'm trying to figure
out the right limit".

> Ahh. I should have read that email more carefully. If xargs broke,
> that _will_ break actual scripts, yes. Do you actually set the stack
> limit to insane values? Anybody using toybox really shouldn't be doing
> 32MB stacks.

Toybox is the default command line of android since M, which went 64 bit
in L, and the Pixel 2 phone has 4 gigs of ram. My goal with toybox is to
turn android into a self-hosting development environment no longer
cross-compiled from a PC (http://landley.net/talks/celf-2013.txt) so I'm
trying to implement a command line that can run the entire AOSP build.

I.E. I have no idea what people will do with it, and try not to get in
their way.

My problem here is it's hard to figure out what exec size the limit
_is_. There's a sysconf(_SC_ARG_MAX) which bionic and glibc are
currently returning as stack_limit/4, which is now too big and exec()
will error out after the fork. Musl is returning the 131072 limit from
2011-ish, meaning "/bin/echo $(printf '%0*d' 131071)" works but
"printf '%0*d' 131071 | xargs" fails, an inconsistency I was trying to
avoid. Maybe I don't have that luxury...

Each argument has its own limit separate from the argv+envp total limit,
but there's only one "size" you can query through sysconf, so the
querying API is insufficient at the design level.

Meanwhile under bash you can allocate and dirty 256 megabytes from the
command line with:

  echo $(printf '%0*d' $((1<<28)))

Because it's a shell builtin so there's no actual exec. (And if
https://sourceware.org/bugzilla/show_bug.cgi?id=17829 ever gets fixed
it'll go back to allowing INT_MAX.)

Posix is its usual helpful self, read conservatively
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html
says to break the line at 2048 bytes.

> So I still do wonder if this actually breaks anything real, or just a
> test-suite or something?

I've cc'd Elliott, who would know. (He's the Android base os userspace
maintainer, he knows everything. Or can at least decode
http://b/65818597 .)

But this just broke my _fix_, not the earlier deployed stuff. I removed
the size measuring code when the 131072 limit went away, the bug was
there's a new limit I need to not hit, I tried to figure out what the
limit is now, confirmed that the various libc implementations don't
agree, then the actual kernel limit changed again while I was looking at it.

>Linus

Should I just go back to hardwiring in 131072? It's no _less_ arbitrary
than 10 megs, and it sounds like getting it _right_ is unachievable.

Thanks,

Rob

Re: [PATCH v5 2/2] Input: add support for the Samsung S6SY761 touchscreen

[Dmitry Torokhov]

Hi Andi,

On Thu, Nov 02, 2017 at 03:07:26PM +0900, Andi Shyti wrote:
> +
> + err = s6sy761_hw_init(sdata);
> + if (err)
> + return err;
> +
> + sdata->input = devm_input_allocate_device(>dev);
> + if (!sdata->input)
> + return -ENOMEM;
> +
> + sdata->input->name = S6SY761_DEV_NAME;
> + sdata->input->id.bustype = BUS_I2C;
> + sdata->input->open = s6sy761_input_open;
> + sdata->input->close = s6sy761_input_close;
> +
> + /* the range has been previously read in hw_init */
> + if (sdata->prop.max_x && sdata->prop.max_y) {

You do not need make this a conditional, if sdata->prop.max_x or
sdata->prop.max_y is 0 is it OK.

However, there is a slight problem: you call hw_init() on resume,
potentially updating prop.max_x and prop.max_y and taking them out of
sync if they have been overridden by the DT data and
touchscreen_parse_properties(). I think it would be best if
s6sy761_hw_init() had 2 additional arguments for max_x and max_y and you
passed them to input_set_abs_params() and left sdata->prop to be managed
solely by touchscreen_parse_properties().

Also, is everything in s6sy761_hw_init() needed at resume time?

Thanks.

-- 
Dmitry

Re: [PATCH v5 2/2] Input: add support for the Samsung S6SY761 touchscreen

[Dmitry Torokhov]

Hi Andi,

On Thu, Nov 02, 2017 at 03:07:26PM +0900, Andi Shyti wrote:
> +
> + err = s6sy761_hw_init(sdata);
> + if (err)
> + return err;
> +
> + sdata->input = devm_input_allocate_device(>dev);
> + if (!sdata->input)
> + return -ENOMEM;
> +
> + sdata->input->name = S6SY761_DEV_NAME;
> + sdata->input->id.bustype = BUS_I2C;
> + sdata->input->open = s6sy761_input_open;
> + sdata->input->close = s6sy761_input_close;
> +
> + /* the range has been previously read in hw_init */
> + if (sdata->prop.max_x && sdata->prop.max_y) {

You do not need make this a conditional, if sdata->prop.max_x or
sdata->prop.max_y is 0 is it OK.

However, there is a slight problem: you call hw_init() on resume,
potentially updating prop.max_x and prop.max_y and taking them out of
sync if they have been overridden by the DT data and
touchscreen_parse_properties(). I think it would be best if
s6sy761_hw_init() had 2 additional arguments for max_x and max_y and you
passed them to input_set_abs_params() and left sdata->prop to be managed
solely by touchscreen_parse_properties().

Also, is everything in s6sy761_hw_init() needed at resume time?

Thanks.

-- 
Dmitry