Re: [PATCH] tracing: do not leak kernel addresses
On Wed, 25 Jul 2018 13:22:36 -0700 Mark Salyzyn wrote: > From: Nick Desaulniers > > Switch from 0x%lx to 0x%pK to print the kernel addresses. > > Fixes: CVE-2017-0630 Wait This breaks perf and trace-cmd! They require this to be able to print various strings in trace events. This file is root read only, as the CVE says. NAK for this fix. Come up with something that doesn't break perf and trace-cmd. That will not be trivial, as the format is stored in the ring buffer with an address, then referenced directly. It also handles trace_printk() functions that simply point to the string format itself. A fix would require having a pointer be the same that is referenced inside the kernel as well as in this file. Maybe make the format string placed in a location that doesn't leak where the rest of the kernel exists? -- Steve > Signed-off-by: Mark Salyzyn > Cc: Nick Desaulniers > Cc: Steven Rostedt > Cc: Ingo Molnar > Cc: > Cc: # 3.18, 4.4, 4.9, 4.14 > Cc: > > --- > kernel/trace/trace_printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c > index ad1d6164e946..93698023baf1 100644 > --- a/kernel/trace/trace_printk.c > +++ b/kernel/trace/trace_printk.c > @@ -304,7 +304,7 @@ static int t_show(struct seq_file *m, void *v) > if (!*fmt) > return 0; > > - seq_printf(m, "0x%lx : \"", *(unsigned long *)fmt); > + seq_printf(m, "0x%pK : \"", *(unsigned long *)fmt); > > /* >* Tabs and new lines need to be converted.
Re: [PATCH] tracing: do not leak kernel addresses
On Wed, 25 Jul 2018 13:22:36 -0700 Mark Salyzyn wrote: > From: Nick Desaulniers > > Switch from 0x%lx to 0x%pK to print the kernel addresses. > > Fixes: CVE-2017-0630 Wait This breaks perf and trace-cmd! They require this to be able to print various strings in trace events. This file is root read only, as the CVE says. NAK for this fix. Come up with something that doesn't break perf and trace-cmd. That will not be trivial, as the format is stored in the ring buffer with an address, then referenced directly. It also handles trace_printk() functions that simply point to the string format itself. A fix would require having a pointer be the same that is referenced inside the kernel as well as in this file. Maybe make the format string placed in a location that doesn't leak where the rest of the kernel exists? -- Steve > Signed-off-by: Mark Salyzyn > Cc: Nick Desaulniers > Cc: Steven Rostedt > Cc: Ingo Molnar > Cc: > Cc: # 3.18, 4.4, 4.9, 4.14 > Cc: > > --- > kernel/trace/trace_printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c > index ad1d6164e946..93698023baf1 100644 > --- a/kernel/trace/trace_printk.c > +++ b/kernel/trace/trace_printk.c > @@ -304,7 +304,7 @@ static int t_show(struct seq_file *m, void *v) > if (!*fmt) > return 0; > > - seq_printf(m, "0x%lx : \"", *(unsigned long *)fmt); > + seq_printf(m, "0x%pK : \"", *(unsigned long *)fmt); > > /* >* Tabs and new lines need to be converted.
linux-next: build warning after merge of the rdma tree
Hi all,
After merging the rdma tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:
net/sunrpc/xprtrdma/svc_rdma_rw.c: In function 'svc_rdma_post_chunk_ctxt':
net/sunrpc/xprtrdma/svc_rdma_rw.c:350:5: warning: 'bad_wr' may be used
uninitialized in this function [-Wmaybe-uninitialized]
if (bad_wr != first_wr)
^
Introduced by commit
ed288d74a9e5 ("net/xprtrdma: Simplify ib_post_(send|recv|srq_recv)() calls")
This is an actual problem.
--
Cheers,
Stephen Rothwell
pgpBpaF3FlVqx.pgp
Description: OpenPGP digital signature
linux-next: build warning after merge of the rdma tree
Hi all,
After merging the rdma tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:
net/sunrpc/xprtrdma/svc_rdma_rw.c: In function 'svc_rdma_post_chunk_ctxt':
net/sunrpc/xprtrdma/svc_rdma_rw.c:350:5: warning: 'bad_wr' may be used
uninitialized in this function [-Wmaybe-uninitialized]
if (bad_wr != first_wr)
^
Introduced by commit
ed288d74a9e5 ("net/xprtrdma: Simplify ib_post_(send|recv|srq_recv)() calls")
This is an actual problem.
--
Cheers,
Stephen Rothwell
pgpBpaF3FlVqx.pgp
Description: OpenPGP digital signature
Re: Showing /sys/fs/cgroup/memory/memory.stat very slow on some machines
On 7/19/18 3:40 AM, Bruce Merry wrote: > On 18 July 2018 at 17:49, Shakeel Butt wrote: >> On Wed, Jul 18, 2018 at 8:37 AM Bruce Merry wrote: >>> That sounds promising. Is there any way to tell how many zombies there >>> are, and is there any way to deliberately create zombies? If I can >>> produce zombies that might give me a reliable way to reproduce the >>> problem, which could then sensibly be tested against newer kernel >>> versions. >>> >> >> Yes, very easy to produce zombies, though I don't think kernel >> provides any way to tell how many zombies exist on the system. >> >> To create a zombie, first create a memcg node, enter that memcg, >> create a tmpfs file of few KiBs, exit the memcg and rmdir the memcg. >> That memcg will be a zombie until you delete that tmpfs file. > > Thanks, that makes sense. I'll see if I can reproduce the issue. Do > you expect the same thing to happen with normal (non-tmpfs) files that > are sitting in the page cache, and/or dentries? > Do you by any chance have use_hierarch=1? memcg_stat_show should just rely on counters inside the memory cgroup and the the LRU sizes for each node. Balbir Singh.
Re: Showing /sys/fs/cgroup/memory/memory.stat very slow on some machines
On 7/19/18 3:40 AM, Bruce Merry wrote: > On 18 July 2018 at 17:49, Shakeel Butt wrote: >> On Wed, Jul 18, 2018 at 8:37 AM Bruce Merry wrote: >>> That sounds promising. Is there any way to tell how many zombies there >>> are, and is there any way to deliberately create zombies? If I can >>> produce zombies that might give me a reliable way to reproduce the >>> problem, which could then sensibly be tested against newer kernel >>> versions. >>> >> >> Yes, very easy to produce zombies, though I don't think kernel >> provides any way to tell how many zombies exist on the system. >> >> To create a zombie, first create a memcg node, enter that memcg, >> create a tmpfs file of few KiBs, exit the memcg and rmdir the memcg. >> That memcg will be a zombie until you delete that tmpfs file. > > Thanks, that makes sense. I'll see if I can reproduce the issue. Do > you expect the same thing to happen with normal (non-tmpfs) files that > are sitting in the page cache, and/or dentries? > Do you by any chance have use_hierarch=1? memcg_stat_show should just rely on counters inside the memory cgroup and the the LRU sizes for each node. Balbir Singh.
Re: [PATCH v4 2/3] Input: edt-ft5x06 - Set wake/reset values on resume/suspend
Hi Mylène, On Wed, Jul 25, 2018 at 09:34:09AM +0200, Mylène Josserand wrote: > On resume and suspend, set the value of wake and reset gpios > to be sure that we are in a know state after suspending/resuming. > > Signed-off-by: Mylène Josserand > --- > drivers/input/touchscreen/edt-ft5x06.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c > b/drivers/input/touchscreen/edt-ft5x06.c > index dcde719094f7..dad2f1f8bf89 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -1158,6 +1158,12 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct > device *dev) > else > regulator_disable(tsdata->vcc); > > + if (tsdata->wake_gpio) > + gpiod_set_value(tsdata->wake_gpio, 0); > + > + if (tsdata->reset_gpio) > + gpiod_set_value(tsdata->reset_gpio, 1); Ondřej mentioned in previous review that if you power off the controller it will not be able to wake up the system, and you had to move call to regulator_disable() into "else" branch of check whether the controller is a wakeup device. Guess what happens if you unconditionally put the device into reset state? Thanks. -- Dmitry
Re: [PATCH v4 2/3] Input: edt-ft5x06 - Set wake/reset values on resume/suspend
Hi Mylène, On Wed, Jul 25, 2018 at 09:34:09AM +0200, Mylène Josserand wrote: > On resume and suspend, set the value of wake and reset gpios > to be sure that we are in a know state after suspending/resuming. > > Signed-off-by: Mylène Josserand > --- > drivers/input/touchscreen/edt-ft5x06.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c > b/drivers/input/touchscreen/edt-ft5x06.c > index dcde719094f7..dad2f1f8bf89 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -1158,6 +1158,12 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct > device *dev) > else > regulator_disable(tsdata->vcc); > > + if (tsdata->wake_gpio) > + gpiod_set_value(tsdata->wake_gpio, 0); > + > + if (tsdata->reset_gpio) > + gpiod_set_value(tsdata->reset_gpio, 1); Ondřej mentioned in previous review that if you power off the controller it will not be able to wake up the system, and you had to move call to regulator_disable() into "else" branch of check whether the controller is a wakeup device. Guess what happens if you unconditionally put the device into reset state? Thanks. -- Dmitry
[PATCH v2] reset: hisilicon: fix potential NULL pointer dereference
There is a potential execution path in which function
platform_get_resource() returns NULL. If this happens,
we will end up having a NULL pointer dereference.
Fix this by replacing devm_ioremap with devm_ioremap_resource,
which has the NULL check and the memory region request.
This code was detected with the help of Coccinelle.
Cc: sta...@vger.kernel.org
Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
hisi_reset_init")
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Use devm_ioremap_resource. Thanks to Stephen Boyd for
pointing it out.
drivers/clk/hisilicon/reset.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
index 2a5015c..43e82fa 100644
--- a/drivers/clk/hisilicon/reset.c
+++ b/drivers/clk/hisilicon/reset.c
@@ -109,9 +109,8 @@ struct hisi_reset_controller *hisi_reset_init(struct
platform_device *pdev)
return NULL;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- rstc->membase = devm_ioremap(>dev,
- res->start, resource_size(res));
- if (!rstc->membase)
+ rstc->membase = devm_ioremap_resource(>dev, res);
+ if (IS_ERR(rstc->membase))
return NULL;
spin_lock_init(>lock);
--
2.7.4
Re: [PATCH v4 1/3] Input: edt-ft5x06 - Add support for regulator
Hi Mylène,
On Wed, Jul 25, 2018 at 09:34:08AM +0200, Mylène Josserand wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Mylène Josserand
> Reviewed-by: Rob Herring
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> drivers/input/touchscreen/edt-ft5x06.c | 43
> ++
> 2 files changed, 44 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 025cf8c9324a..48e975b9c1aa 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -30,6 +30,7 @@ Required properties:
> Optional properties:
> - reset-gpios: GPIO specification for the RESET input
> - wake-gpios: GPIO specification for the WAKE input
> + - vcc-supply: Regulator that supplies the touchscreen
>
> - pinctrl-names: should be "default"
> - pinctrl-0: a phandle pointing to the pin settings for the
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c
> b/drivers/input/touchscreen/edt-ft5x06.c
> index 1e18ca0d1b4e..dcde719094f7 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
>
> #define WORK_REGISTER_THRESHOLD 0x00
> #define WORK_REGISTER_REPORT_RATE0x08
> @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> struct touchscreen_properties prop;
> u16 num_x;
> u16 num_y;
> + struct regulator *vcc;
>
> struct gpio_desc *reset_gpio;
> struct gpio_desc *wake_gpio;
> @@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
> }
> }
>
> +static void edt_ft5x06_disable_regulator(void *arg)
> +{
> + struct edt_ft5x06_ts_data *data = arg;
> +
> + regulator_disable(data->vcc);
> +}
> +
> static int edt_ft5x06_ts_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
> {
> @@ -991,6 +1000,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client
> *client,
>
> tsdata->max_support_points = chip_data->max_support_points;
>
> + tsdata->vcc = devm_regulator_get(>dev, "vcc");
> + if (IS_ERR(tsdata->vcc)) {
> + error = PTR_ERR(tsdata->vcc);
> + if (error != -EPROBE_DEFER)
> + dev_err(>dev, "failed to request regulator:
> %d\n",
> + error);
> + return error;
> + }
> +
> + error = regulator_enable(tsdata->vcc);
> + if (error < 0) {
> + dev_err(>dev, "failed to enable vcc: %d\n",
> + error);
> + return error;
> + }
It is better to put the chip into reset and then power up the regulatori
and take it out of the reset, rather than power up and then toggle reset
on and off.
> +
> + error = devm_add_action_or_reset(>dev,
> + edt_ft5x06_disable_regulator,
> + tsdata);
> + if (error)
> + return error;
> +
> tsdata->reset_gpio = devm_gpiod_get_optional(>dev,
>"reset", GPIOD_OUT_HIGH);
> if (IS_ERR(tsdata->reset_gpio)) {
> @@ -1120,9 +1151,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client
> *client)
> static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> + struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
>
> if (device_may_wakeup(dev))
> enable_irq_wake(client->irq);
> + else
> + regulator_disable(tsdata->vcc);
>
> return 0;
> }
> @@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct
> device *dev)
> static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> + struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> + int ret;
>
> if (device_may_wakeup(dev))
> disable_irq_wake(client->irq);
> + else {
> + ret = regulator_enable(tsdata->vcc);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable vcc: %d\n", ret);
> + return ret;
> + }
> + }
I believe I mentioned in other review that once you powered up the
device, you need to restore its settings, include switching to factory
mode, if it was in factory mode, and restoring threshold/gain/offset
settings.
Thanks.
--
Dmitry
Re: [PATCH v1] arm64: dts: sdm845: enable tsens thermal zones
Hi Amit,
On Wed, Jul 18, 2018 at 01:19:17PM +0530, Amit Kucheria wrote:
> One thermal zone per cpu is defined
>
> Signed-off-by: Amit Kucheria
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 170
> +++
> 1 file changed, 170 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 01ff146..a75be7c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -340,4 +340,174 @@
> };
> };
> };
> +
> + thermal-zones {
> + cpu0-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
In the context of the TSENS patches you mentioned that you are working
on interrupt support. Can the polling delays be removed once that is merged?
> + thermal-sensors = < 1>;
> +
> + trips {
> + cpu_alert0: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit0: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu1-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 2>;
> +
> + trips {
> + cpu_alert1: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit1: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu2-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 3>;
> +
> + trips {
> + cpu_alert2: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit2: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu3-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 4>;
> +
> + trips {
> + cpu_alert3: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit3: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu4-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 7>;
> +
> + trips {
> + cpu_alert4: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit4: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu5-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> +
[PATCH v2] reset: hisilicon: fix potential NULL pointer dereference
There is a potential execution path in which function
platform_get_resource() returns NULL. If this happens,
we will end up having a NULL pointer dereference.
Fix this by replacing devm_ioremap with devm_ioremap_resource,
which has the NULL check and the memory region request.
This code was detected with the help of Coccinelle.
Cc: sta...@vger.kernel.org
Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
hisi_reset_init")
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Use devm_ioremap_resource. Thanks to Stephen Boyd for
pointing it out.
drivers/clk/hisilicon/reset.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
index 2a5015c..43e82fa 100644
--- a/drivers/clk/hisilicon/reset.c
+++ b/drivers/clk/hisilicon/reset.c
@@ -109,9 +109,8 @@ struct hisi_reset_controller *hisi_reset_init(struct
platform_device *pdev)
return NULL;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- rstc->membase = devm_ioremap(>dev,
- res->start, resource_size(res));
- if (!rstc->membase)
+ rstc->membase = devm_ioremap_resource(>dev, res);
+ if (IS_ERR(rstc->membase))
return NULL;
spin_lock_init(>lock);
--
2.7.4
Re: [PATCH v4 1/3] Input: edt-ft5x06 - Add support for regulator
Hi Mylène,
On Wed, Jul 25, 2018 at 09:34:08AM +0200, Mylène Josserand wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Mylène Josserand
> Reviewed-by: Rob Herring
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> drivers/input/touchscreen/edt-ft5x06.c | 43
> ++
> 2 files changed, 44 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 025cf8c9324a..48e975b9c1aa 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -30,6 +30,7 @@ Required properties:
> Optional properties:
> - reset-gpios: GPIO specification for the RESET input
> - wake-gpios: GPIO specification for the WAKE input
> + - vcc-supply: Regulator that supplies the touchscreen
>
> - pinctrl-names: should be "default"
> - pinctrl-0: a phandle pointing to the pin settings for the
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c
> b/drivers/input/touchscreen/edt-ft5x06.c
> index 1e18ca0d1b4e..dcde719094f7 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
>
> #define WORK_REGISTER_THRESHOLD 0x00
> #define WORK_REGISTER_REPORT_RATE0x08
> @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data {
> struct touchscreen_properties prop;
> u16 num_x;
> u16 num_y;
> + struct regulator *vcc;
>
> struct gpio_desc *reset_gpio;
> struct gpio_desc *wake_gpio;
> @@ -963,6 +965,13 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
> }
> }
>
> +static void edt_ft5x06_disable_regulator(void *arg)
> +{
> + struct edt_ft5x06_ts_data *data = arg;
> +
> + regulator_disable(data->vcc);
> +}
> +
> static int edt_ft5x06_ts_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
> {
> @@ -991,6 +1000,28 @@ static int edt_ft5x06_ts_probe(struct i2c_client
> *client,
>
> tsdata->max_support_points = chip_data->max_support_points;
>
> + tsdata->vcc = devm_regulator_get(>dev, "vcc");
> + if (IS_ERR(tsdata->vcc)) {
> + error = PTR_ERR(tsdata->vcc);
> + if (error != -EPROBE_DEFER)
> + dev_err(>dev, "failed to request regulator:
> %d\n",
> + error);
> + return error;
> + }
> +
> + error = regulator_enable(tsdata->vcc);
> + if (error < 0) {
> + dev_err(>dev, "failed to enable vcc: %d\n",
> + error);
> + return error;
> + }
It is better to put the chip into reset and then power up the regulatori
and take it out of the reset, rather than power up and then toggle reset
on and off.
> +
> + error = devm_add_action_or_reset(>dev,
> + edt_ft5x06_disable_regulator,
> + tsdata);
> + if (error)
> + return error;
> +
> tsdata->reset_gpio = devm_gpiod_get_optional(>dev,
>"reset", GPIOD_OUT_HIGH);
> if (IS_ERR(tsdata->reset_gpio)) {
> @@ -1120,9 +1151,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client
> *client)
> static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> + struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
>
> if (device_may_wakeup(dev))
> enable_irq_wake(client->irq);
> + else
> + regulator_disable(tsdata->vcc);
>
> return 0;
> }
> @@ -1130,9 +1164,18 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct
> device *dev)
> static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> + struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> + int ret;
>
> if (device_may_wakeup(dev))
> disable_irq_wake(client->irq);
> + else {
> + ret = regulator_enable(tsdata->vcc);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable vcc: %d\n", ret);
> + return ret;
> + }
> + }
I believe I mentioned in other review that once you powered up the
device, you need to restore its settings, include switching to factory
mode, if it was in factory mode, and restoring threshold/gain/offset
settings.
Thanks.
--
Dmitry
Re: [PATCH v1] arm64: dts: sdm845: enable tsens thermal zones
Hi Amit,
On Wed, Jul 18, 2018 at 01:19:17PM +0530, Amit Kucheria wrote:
> One thermal zone per cpu is defined
>
> Signed-off-by: Amit Kucheria
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 170
> +++
> 1 file changed, 170 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 01ff146..a75be7c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -340,4 +340,174 @@
> };
> };
> };
> +
> + thermal-zones {
> + cpu0-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
In the context of the TSENS patches you mentioned that you are working
on interrupt support. Can the polling delays be removed once that is merged?
> + thermal-sensors = < 1>;
> +
> + trips {
> + cpu_alert0: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit0: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu1-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 2>;
> +
> + trips {
> + cpu_alert1: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit1: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu2-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 3>;
> +
> + trips {
> + cpu_alert2: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit2: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu3-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 4>;
> +
> + trips {
> + cpu_alert3: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit3: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu4-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = < 7>;
> +
> + trips {
> + cpu_alert4: trip0 {
> + temperature = <75000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit4: trip1 {
> + temperature = <11>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + cpu5-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> +
Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions
On Fri, 13 Jul 2018 08:18:03 -0400 Steven Rostedt wrote: > On Fri, 13 Jul 2018 11:53:01 +0900 > Masami Hiramatsu wrote: > > > On Thu, 12 Jul 2018 13:54:12 -0400 > > Francis Deslauriers wrote: > > > > > From: Masami Hiramatsu > > > > > > Prohibit kprobe-events probing on notrace function. > > > Since probing on the notrace function can cause recursive > > > event call. In most case those are just skipped, but > > > in some case it falls into infinite recursive call. > > > > BTW, I'm considering to add an option to allow putting > > kprobes on notrace function - just for debugging > > ftrace by kprobes. That is "developer only" option > > so generally it should be disabled, but for debugging > > the ftrace, we still need it. Or should I introduce > > another kprobes module for debugging it? > > No, I think the former is better (to add an option to allow putting > kprobes on notrace functions). By default we let people protect > themselves. But if then provide a switch that lets you do things that > might let you shoot yourself in the foot. I'm adding CONFIG_KPROBE_EVENTS_ON_NOTRACE kconfig which allows kprobes on notrace function. I think we don't need to make it online switchable, since it is only good for ftrace developers. Thank you, > > BTW, I'm now leaving on vacation. I'll be back on the 23rd and will be > looking for patches that I should be pulling in then. > > Thanks! > > -- Steve -- Masami Hiramatsu
Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions
On Fri, 13 Jul 2018 08:18:03 -0400 Steven Rostedt wrote: > On Fri, 13 Jul 2018 11:53:01 +0900 > Masami Hiramatsu wrote: > > > On Thu, 12 Jul 2018 13:54:12 -0400 > > Francis Deslauriers wrote: > > > > > From: Masami Hiramatsu > > > > > > Prohibit kprobe-events probing on notrace function. > > > Since probing on the notrace function can cause recursive > > > event call. In most case those are just skipped, but > > > in some case it falls into infinite recursive call. > > > > BTW, I'm considering to add an option to allow putting > > kprobes on notrace function - just for debugging > > ftrace by kprobes. That is "developer only" option > > so generally it should be disabled, but for debugging > > the ftrace, we still need it. Or should I introduce > > another kprobes module for debugging it? > > No, I think the former is better (to add an option to allow putting > kprobes on notrace functions). By default we let people protect > themselves. But if then provide a switch that lets you do things that > might let you shoot yourself in the foot. I'm adding CONFIG_KPROBE_EVENTS_ON_NOTRACE kconfig which allows kprobes on notrace function. I think we don't need to make it online switchable, since it is only good for ftrace developers. Thank you, > > BTW, I'm now leaving on vacation. I'll be back on the 23rd and will be > looking for patches that I should be pulling in then. > > Thanks! > > -- Steve -- Masami Hiramatsu
Re: Reminder to review a few patches sent two weeks ago
On 2018-07-24 17:33, Joe Perches wrote:
On Tue, 2018-07-24 at 14:56 -0700, pher...@codeaurora.org wrote:
A reminder to review a few patches I had sent last week. Below are the
links for the patches.
https://lkml.org/lkml/2018/7/5/798
I have no fundamental object to this one, but
the 80 column use is unnecessary and should be
coalesced before it can be applied.
Perhaps:
# warn about #if 1
if ($line =~ /^.\s*\#\s*if\s+1\b/) {
WARN("IF_1",
"Consider removing the #if 1 and its #endif\n" .
$herecurr);
}
http://lists-archives.com/linux-kernel/29168320-checkpatch-check-for-invalid-return-codes.html
This one has I think too many existing uses of
things like "return -1;"
$ git grep -P "return\s*\-\d+\s*;" | wc -l
9929
How many of these are actually appropriate?
I did go through a few of the files which return -1 in their functions,
I observed that most of them were inappropriate and there was a case
where actually the use of return -1 was
incorrect(kernel/arch/ia64/mm/contig.c
in the function find_bootmap_location()). We could actually catch such
errors
from now on if we use this patch.
Also, no space is required between return and -1
by c90 and this should use $Int so it should be:
if ($line =~ /\breturn\s*\-\$Int\s*;/) {
etc...
Re: Reminder to review a few patches sent two weeks ago
On 2018-07-24 17:33, Joe Perches wrote:
On Tue, 2018-07-24 at 14:56 -0700, pher...@codeaurora.org wrote:
A reminder to review a few patches I had sent last week. Below are the
links for the patches.
https://lkml.org/lkml/2018/7/5/798
I have no fundamental object to this one, but
the 80 column use is unnecessary and should be
coalesced before it can be applied.
Perhaps:
# warn about #if 1
if ($line =~ /^.\s*\#\s*if\s+1\b/) {
WARN("IF_1",
"Consider removing the #if 1 and its #endif\n" .
$herecurr);
}
http://lists-archives.com/linux-kernel/29168320-checkpatch-check-for-invalid-return-codes.html
This one has I think too many existing uses of
things like "return -1;"
$ git grep -P "return\s*\-\d+\s*;" | wc -l
9929
How many of these are actually appropriate?
I did go through a few of the files which return -1 in their functions,
I observed that most of them were inappropriate and there was a case
where actually the use of return -1 was
incorrect(kernel/arch/ia64/mm/contig.c
in the function find_bootmap_location()). We could actually catch such
errors
from now on if we use this patch.
Also, no space is required between return and -1
by c90 and this should use $Int so it should be:
if ($line =~ /\breturn\s*\-\$Int\s*;/) {
etc...
Re: [PATCH] firmware: Avoid caching firmware when FW_OPT_NOCACHE is set
On Thu, Jul 19, 2018 at 02:25:21PM -0700, Rishabh Bhatnagar wrote:
> When calling request_firmware_into_buf(), we pass the FW_OPT_NOCACHE
> flag with the intent of skipping the caching mechanism of the
> firmware loader. Unfortunately, that doesn't work, because
> alloc_lookup_fw_priv() isn't told to _not_ add the struct
> firmware_buf to the firmware cache (fwc) list. So when we call
> request_firmware_into_buf() the second time, we find the buffer in
> the cache and return it immediately without reloading.
The code in question is not dealing with the firmware cache though, it is
batched requests. Unfortunately the code is not very clear given the
structure used is the struct firmware_cache, however if you look
carefully you will see there are two struct list_head used:
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
struct list_head head;
...
#ifdef CONFIG_PM_SLEEP
/*
* Names of firmware images which have been cached successfully
* will be added into the below list so that device uncache
* helper can trace which firmware images have been cached
* before.
*/
spinlock_t name_lock;
struct list_head fw_names;
...
};
So the first struct list_head is used for batched firmware requests. It
was the first patch of the work, it was introduced via commit one
commit, while the actual firmware cache used for suspend/resume in
another. Respectively they are commits:
1f2b79599ee8f ("firmware loader: always let firmware_buf own the pages buffer")
37276a51f803e ("firmware: introduce device_cache/uncache_fw_images")
The ifdefery above clarifies this is a bit, however it is rather easy
to fall into the trap to easily review this code and think they are the
same thing.
> This may break users of request_firmware_into_buf that are expecting
> a fresh copy of the firmware to be reloaded into memory.
No, this is not the reason why this will break users of
request_firmware_into_buf(), its because the call is by design
using device driver specific memory, so as a matter of fact, it would
actually be a security issue:
*Any* kernel code which makes a request for the same firmware name, if
the call is carefully timed, could end up getting a pointer to the device's
own memory area used for this firmware, and since
request_firmware_into_buf() was used, and since today's only driver
using this is with IO memory this is a bigger issue.
What you state above alludes that the issue is that we want a fresh
copy, this has nothing to do with a copy, this is device driver specific
memory.
> The existing
> copy may either be modified by drivers, remote processors or even
> freed.
This is *also* true, specially since the only single Qualcomm driver
using this *also* uses IO memory. And this is a small example of why
also LSMs *should* be abe to have knowledge when this type of memory is
used.
> Fix fw_lookup_and_allocate_buf to not add to the fwc list
> if FW_OPT_NOCACHE is set, and also don't do the lookup in the list.
>
> Fixes: 0e742e9271 ("firmware: provide infrastructure to make fw caching
> optional")
This commit does not exist, you made a typo, the last 1 should be a 5:
0e742e9275
NACK for a few reasons:
a) The commit ID identified as it fixing is wrong
b) The commit is not accurate as to the reason why this is an issue
c) The commit does not ackowledge the severity of the issue
d) I was hoping we can make the fix simpler
As for c) and d) -- come to think of it, the fact that we didn't
implement batched firmware requests with a const pointer instead
means we have similar potential issues possible with othe kernel
code also requesting the same firmware and potentially modifying
it on the fly prior to a device processing it, therefore causing
possibly unintended behaviour.
So, initially I was inclined to just rip the batched firmware
implementation completely for all callers. Unfortunately trying to do
this revealed to me just how broken this was. For instance, the private
buf kept around for the firmware when caching it is kept track by the
using the above mentioned struct list head. This means that any firmware
which intends to be used for firmware caching *must* also be added to
the fw cache head list. And unfortunately the search for this entry is
indexed by just the name. So uncache_firmnware() for instance uses
lookup_fw_priv() and that in turn which just look for the first private
buf with the associated firmware name. This means firmware which is
cached assumes only one copy is kept around during suspend/resume cycle,
and the batched firmware feature was collateral.
So I don't think we can easily disable batched firmware requests
without breaking caching, as such I think your patch in turn is
what we need for now. Can you re-spin, fix the commit log with a
proper explanation and commit ID it fixes?
Luis
> Signed-off-by: Vikram Mulukutla
>
Re: [PATCH] firmware: Avoid caching firmware when FW_OPT_NOCACHE is set
On Thu, Jul 19, 2018 at 02:25:21PM -0700, Rishabh Bhatnagar wrote:
> When calling request_firmware_into_buf(), we pass the FW_OPT_NOCACHE
> flag with the intent of skipping the caching mechanism of the
> firmware loader. Unfortunately, that doesn't work, because
> alloc_lookup_fw_priv() isn't told to _not_ add the struct
> firmware_buf to the firmware cache (fwc) list. So when we call
> request_firmware_into_buf() the second time, we find the buffer in
> the cache and return it immediately without reloading.
The code in question is not dealing with the firmware cache though, it is
batched requests. Unfortunately the code is not very clear given the
structure used is the struct firmware_cache, however if you look
carefully you will see there are two struct list_head used:
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
struct list_head head;
...
#ifdef CONFIG_PM_SLEEP
/*
* Names of firmware images which have been cached successfully
* will be added into the below list so that device uncache
* helper can trace which firmware images have been cached
* before.
*/
spinlock_t name_lock;
struct list_head fw_names;
...
};
So the first struct list_head is used for batched firmware requests. It
was the first patch of the work, it was introduced via commit one
commit, while the actual firmware cache used for suspend/resume in
another. Respectively they are commits:
1f2b79599ee8f ("firmware loader: always let firmware_buf own the pages buffer")
37276a51f803e ("firmware: introduce device_cache/uncache_fw_images")
The ifdefery above clarifies this is a bit, however it is rather easy
to fall into the trap to easily review this code and think they are the
same thing.
> This may break users of request_firmware_into_buf that are expecting
> a fresh copy of the firmware to be reloaded into memory.
No, this is not the reason why this will break users of
request_firmware_into_buf(), its because the call is by design
using device driver specific memory, so as a matter of fact, it would
actually be a security issue:
*Any* kernel code which makes a request for the same firmware name, if
the call is carefully timed, could end up getting a pointer to the device's
own memory area used for this firmware, and since
request_firmware_into_buf() was used, and since today's only driver
using this is with IO memory this is a bigger issue.
What you state above alludes that the issue is that we want a fresh
copy, this has nothing to do with a copy, this is device driver specific
memory.
> The existing
> copy may either be modified by drivers, remote processors or even
> freed.
This is *also* true, specially since the only single Qualcomm driver
using this *also* uses IO memory. And this is a small example of why
also LSMs *should* be abe to have knowledge when this type of memory is
used.
> Fix fw_lookup_and_allocate_buf to not add to the fwc list
> if FW_OPT_NOCACHE is set, and also don't do the lookup in the list.
>
> Fixes: 0e742e9271 ("firmware: provide infrastructure to make fw caching
> optional")
This commit does not exist, you made a typo, the last 1 should be a 5:
0e742e9275
NACK for a few reasons:
a) The commit ID identified as it fixing is wrong
b) The commit is not accurate as to the reason why this is an issue
c) The commit does not ackowledge the severity of the issue
d) I was hoping we can make the fix simpler
As for c) and d) -- come to think of it, the fact that we didn't
implement batched firmware requests with a const pointer instead
means we have similar potential issues possible with othe kernel
code also requesting the same firmware and potentially modifying
it on the fly prior to a device processing it, therefore causing
possibly unintended behaviour.
So, initially I was inclined to just rip the batched firmware
implementation completely for all callers. Unfortunately trying to do
this revealed to me just how broken this was. For instance, the private
buf kept around for the firmware when caching it is kept track by the
using the above mentioned struct list head. This means that any firmware
which intends to be used for firmware caching *must* also be added to
the fw cache head list. And unfortunately the search for this entry is
indexed by just the name. So uncache_firmnware() for instance uses
lookup_fw_priv() and that in turn which just look for the first private
buf with the associated firmware name. This means firmware which is
cached assumes only one copy is kept around during suspend/resume cycle,
and the batched firmware feature was collateral.
So I don't think we can easily disable batched firmware requests
without breaking caching, as such I think your patch in turn is
what we need for now. Can you re-spin, fix the commit log with a
proper explanation and commit ID it fixes?
Luis
> Signed-off-by: Vikram Mulukutla
>
Re: [PATCH v2] EDAC, sb_edac: Add support for systems with segmented PCI buses
Hi Boris,
On 07/25/2018 05:22 AM, Borislav Petkov wrote:
> On Tue, Jul 24, 2018 at 03:02:13PM -0400, Masayoshi Mizuma wrote:
>> [*] KASAN report is as follows.
>
> That KASAN report is an arbitrary side-effect from the missing segmented
> support so I ripped it out from the commit message and ended up
> committing this:
Thank you so much!
- Masa
>
> ---
> From: Masayoshi Mizuma
> Date: Tue, 24 Jul 2018 15:02:13 -0400
> Subject: [PATCH] EDAC, sb_edac: Add support for systems with segmented PCI
> buses
>
> Extend the driver to check whether segment number and bus number matches
> when deciding how to group memory controller PCI devices to CPU sockets.
>
> Signed-off-by: Masayoshi Mizuma
> Reviewed-by: Tony Luck
> Cc: Mauro Carvalho Chehab
> Cc: linux-edac
> Link: http://lkml.kernel.org/r/20180724190213.26359-1-msys.miz...@gmail.com
> [ Cleanup commit message. ]
> Signed-off-by: Borislav Petkov
> ---
> drivers/edac/sb_edac.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
> index 4a89c8093307..07726fb00321 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -352,6 +352,7 @@ struct pci_id_table {
>
> struct sbridge_dev {
> struct list_headlist;
> + int seg;
> u8 bus, mc;
> u8 node_id, source_id;
> struct pci_dev **pdev;
> @@ -729,7 +730,8 @@ static inline int numcol(u32 mtr)
> return 1 << cols;
> }
>
> -static struct sbridge_dev *get_sbridge_dev(u8 bus, enum domain dom, int
> multi_bus,
> +static struct sbridge_dev *get_sbridge_dev(int seg, u8 bus, enum domain dom,
> +int multi_bus,
> struct sbridge_dev *prev)
> {
> struct sbridge_dev *sbridge_dev;
> @@ -747,14 +749,15 @@ static struct sbridge_dev *get_sbridge_dev(u8 bus, enum
> domain dom, int multi_bu
> : sbridge_edac_list.next, struct
> sbridge_dev, list);
>
> list_for_each_entry_from(sbridge_dev, _edac_list, list) {
> - if (sbridge_dev->bus == bus && (dom == SOCK || dom ==
> sbridge_dev->dom))
> + if ((sbridge_dev->seg == seg) && (sbridge_dev->bus == bus) &&
> + (dom == SOCK || dom == sbridge_dev->dom))
> return sbridge_dev;
> }
>
> return NULL;
> }
>
> -static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum domain dom,
> +static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain
> dom,
>const struct pci_id_table *table)
> {
> struct sbridge_dev *sbridge_dev;
> @@ -771,6 +774,7 @@ static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum
> domain dom,
> return NULL;
> }
>
> + sbridge_dev->seg = seg;
> sbridge_dev->bus = bus;
> sbridge_dev->dom = dom;
> sbridge_dev->n_devs = table->n_devs_per_imc;
> @@ -2246,6 +2250,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
> struct sbridge_dev *sbridge_dev = NULL;
> const struct pci_id_descr *dev_descr = >descr[devno];
> struct pci_dev *pdev = NULL;
> + int seg = 0;
> u8 bus = 0;
> int i = 0;
>
> @@ -2276,10 +2281,12 @@ static int sbridge_get_onedevice(struct pci_dev
> **prev,
> /* End of list, leave */
> return -ENODEV;
> }
> + seg = pci_domain_nr(pdev->bus);
> bus = pdev->bus->number;
>
> next_imc:
> - sbridge_dev = get_sbridge_dev(bus, dev_descr->dom, multi_bus,
> sbridge_dev);
> + sbridge_dev = get_sbridge_dev(seg, bus, dev_descr->dom,
> + multi_bus, sbridge_dev);
> if (!sbridge_dev) {
> /* If the HA1 wasn't found, don't create EDAC second memory
> controller */
> if (dev_descr->dom == IMC1 && devno != 1) {
> @@ -2292,7 +2299,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
> if (dev_descr->dom == SOCK)
> goto out_imc;
>
> - sbridge_dev = alloc_sbridge_dev(bus, dev_descr->dom, table);
> + sbridge_dev = alloc_sbridge_dev(seg, bus, dev_descr->dom,
> table);
> if (!sbridge_dev) {
> pci_dev_put(pdev);
> return -ENOMEM;
>
Re: [PATCH v2] EDAC, sb_edac: Add support for systems with segmented PCI buses
Hi Boris,
On 07/25/2018 05:22 AM, Borislav Petkov wrote:
> On Tue, Jul 24, 2018 at 03:02:13PM -0400, Masayoshi Mizuma wrote:
>> [*] KASAN report is as follows.
>
> That KASAN report is an arbitrary side-effect from the missing segmented
> support so I ripped it out from the commit message and ended up
> committing this:
Thank you so much!
- Masa
>
> ---
> From: Masayoshi Mizuma
> Date: Tue, 24 Jul 2018 15:02:13 -0400
> Subject: [PATCH] EDAC, sb_edac: Add support for systems with segmented PCI
> buses
>
> Extend the driver to check whether segment number and bus number matches
> when deciding how to group memory controller PCI devices to CPU sockets.
>
> Signed-off-by: Masayoshi Mizuma
> Reviewed-by: Tony Luck
> Cc: Mauro Carvalho Chehab
> Cc: linux-edac
> Link: http://lkml.kernel.org/r/20180724190213.26359-1-msys.miz...@gmail.com
> [ Cleanup commit message. ]
> Signed-off-by: Borislav Petkov
> ---
> drivers/edac/sb_edac.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
> index 4a89c8093307..07726fb00321 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -352,6 +352,7 @@ struct pci_id_table {
>
> struct sbridge_dev {
> struct list_headlist;
> + int seg;
> u8 bus, mc;
> u8 node_id, source_id;
> struct pci_dev **pdev;
> @@ -729,7 +730,8 @@ static inline int numcol(u32 mtr)
> return 1 << cols;
> }
>
> -static struct sbridge_dev *get_sbridge_dev(u8 bus, enum domain dom, int
> multi_bus,
> +static struct sbridge_dev *get_sbridge_dev(int seg, u8 bus, enum domain dom,
> +int multi_bus,
> struct sbridge_dev *prev)
> {
> struct sbridge_dev *sbridge_dev;
> @@ -747,14 +749,15 @@ static struct sbridge_dev *get_sbridge_dev(u8 bus, enum
> domain dom, int multi_bu
> : sbridge_edac_list.next, struct
> sbridge_dev, list);
>
> list_for_each_entry_from(sbridge_dev, _edac_list, list) {
> - if (sbridge_dev->bus == bus && (dom == SOCK || dom ==
> sbridge_dev->dom))
> + if ((sbridge_dev->seg == seg) && (sbridge_dev->bus == bus) &&
> + (dom == SOCK || dom == sbridge_dev->dom))
> return sbridge_dev;
> }
>
> return NULL;
> }
>
> -static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum domain dom,
> +static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain
> dom,
>const struct pci_id_table *table)
> {
> struct sbridge_dev *sbridge_dev;
> @@ -771,6 +774,7 @@ static struct sbridge_dev *alloc_sbridge_dev(u8 bus, enum
> domain dom,
> return NULL;
> }
>
> + sbridge_dev->seg = seg;
> sbridge_dev->bus = bus;
> sbridge_dev->dom = dom;
> sbridge_dev->n_devs = table->n_devs_per_imc;
> @@ -2246,6 +2250,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
> struct sbridge_dev *sbridge_dev = NULL;
> const struct pci_id_descr *dev_descr = >descr[devno];
> struct pci_dev *pdev = NULL;
> + int seg = 0;
> u8 bus = 0;
> int i = 0;
>
> @@ -2276,10 +2281,12 @@ static int sbridge_get_onedevice(struct pci_dev
> **prev,
> /* End of list, leave */
> return -ENODEV;
> }
> + seg = pci_domain_nr(pdev->bus);
> bus = pdev->bus->number;
>
> next_imc:
> - sbridge_dev = get_sbridge_dev(bus, dev_descr->dom, multi_bus,
> sbridge_dev);
> + sbridge_dev = get_sbridge_dev(seg, bus, dev_descr->dom,
> + multi_bus, sbridge_dev);
> if (!sbridge_dev) {
> /* If the HA1 wasn't found, don't create EDAC second memory
> controller */
> if (dev_descr->dom == IMC1 && devno != 1) {
> @@ -2292,7 +2299,7 @@ static int sbridge_get_onedevice(struct pci_dev **prev,
> if (dev_descr->dom == SOCK)
> goto out_imc;
>
> - sbridge_dev = alloc_sbridge_dev(bus, dev_descr->dom, table);
> + sbridge_dev = alloc_sbridge_dev(seg, bus, dev_descr->dom,
> table);
> if (!sbridge_dev) {
> pci_dev_put(pdev);
> return -ENOMEM;
>
Re: [PATCH] reset: hisilicon: fix potential NULL pointer dereference
Hi Stephen,
On 07/25/2018 06:45 PM, Stephen Boyd wrote:
> Quoting Gustavo A. R. Silva (2018-07-18 18:58:45)
>> There is a potential execution path in which function
>> platform_get_resource() returns NULL. If this happens,
>> we will end up having a NULL pointer dereference.
>>
>> Fix this by adding asanity check in order to avoid a
>> NULL pointer dereference.
>>
>> This code was detected with the help of Coccinelle.
>>
>> Cc: sta...@vger.kernel.org
>> Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
>> hisi_reset_init")
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/clk/hisilicon/reset.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
>> index 2a5015c..5dfb48b 100644
>> --- a/drivers/clk/hisilicon/reset.c
>> +++ b/drivers/clk/hisilicon/reset.c
>> @@ -109,6 +109,9 @@ struct hisi_reset_controller *hisi_reset_init(struct
>> platform_device *pdev)
>> return NULL;
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return NULL;
>> +
>> rstc->membase = devm_ioremap(>dev,
>> res->start, resource_size(res));
>
> Why can't we use devm_ioremap_resource() here instead?
>
You're right. I think we can perfectly use devm_ioremap_resource here and
remove the null check.
I'll send patch for this shortly.
Thanks
--
Gustavo
Re: [PATCH] reset: hisilicon: fix potential NULL pointer dereference
Hi Stephen,
On 07/25/2018 06:45 PM, Stephen Boyd wrote:
> Quoting Gustavo A. R. Silva (2018-07-18 18:58:45)
>> There is a potential execution path in which function
>> platform_get_resource() returns NULL. If this happens,
>> we will end up having a NULL pointer dereference.
>>
>> Fix this by adding asanity check in order to avoid a
>> NULL pointer dereference.
>>
>> This code was detected with the help of Coccinelle.
>>
>> Cc: sta...@vger.kernel.org
>> Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
>> hisi_reset_init")
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/clk/hisilicon/reset.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
>> index 2a5015c..5dfb48b 100644
>> --- a/drivers/clk/hisilicon/reset.c
>> +++ b/drivers/clk/hisilicon/reset.c
>> @@ -109,6 +109,9 @@ struct hisi_reset_controller *hisi_reset_init(struct
>> platform_device *pdev)
>> return NULL;
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return NULL;
>> +
>> rstc->membase = devm_ioremap(>dev,
>> res->start, resource_size(res));
>
> Why can't we use devm_ioremap_resource() here instead?
>
You're right. I think we can perfectly use devm_ioremap_resource here and
remove the null check.
I'll send patch for this shortly.
Thanks
--
Gustavo
Re: [PATCH] fsi: master-ast-cf: Fix memory leak
On 07/25/2018 06:45 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2018-07-25 at 08:38 -0500, Gustavo A. R. Silva wrote:
>> In case memory resources for *fw* were allocated, release them
>> before return.
>>
>> Addresses-Coverity-ID: 1472044 ("Resource leak")
>> Fixes: 6a794a27daca ("fsi: master-ast-cf: Add new FSI master using Aspeed
>> ColdFire")
>> Signed-off-by: Gustavo A. R. Silva
>
> Thanks Gustavo !
>
Glad to help. :)
> I'll apply before I send my next pull request to Greg !
>
Thanks
--
Gustavo
Re: [PATCH] fsi: master-ast-cf: Fix memory leak
On 07/25/2018 06:45 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2018-07-25 at 08:38 -0500, Gustavo A. R. Silva wrote:
>> In case memory resources for *fw* were allocated, release them
>> before return.
>>
>> Addresses-Coverity-ID: 1472044 ("Resource leak")
>> Fixes: 6a794a27daca ("fsi: master-ast-cf: Add new FSI master using Aspeed
>> ColdFire")
>> Signed-off-by: Gustavo A. R. Silva
>
> Thanks Gustavo !
>
Glad to help. :)
> I'll apply before I send my next pull request to Greg !
>
Thanks
--
Gustavo
[LKP] [ovl] 24c944dd64: BUG:kernel_reboot-without-warning_in_boot_stage
FYI, we noticed the following commit (built with gcc-7):
commit: 24c944dd64f807542a2ec72744c81f064d1a60da ("ovl: Modify ovl_lookup() and
friends to lookup metacopy dentry")
https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git overlayfs-next
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -smp 2 -m 512M
caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
+-+++
| | 48b3292dcd | 24c944dd64 |
+-+++
| boot_successes | 34 | 0 |
| boot_failures | 10 | 48 |
| BUG:kernel_hang_in_test_stage | 8 ||
| invoked_oom-killer:gfp_mask=0x | 2 ||
| Mem-Info| 2 ||
| Out_of_memory:Kill_process | 2 ||
| BUG:kernel_reboot-without-warning_in_boot_stage | 0 | 48 |
+-+++
[0.00] BRK [0x1d3ee000, 0x1d3eefff] PGTABLE
[0.00] RAMDISK: [mem 0x1e73c000-0x1ffd]
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI: RSDP 0x000F6870 14 (v00 BOCHS )
[0.00] ACPI: RSDT 0x1FFE1628 30 (v01 BOCHS BXPCRSDT
0001 BXPC 0001)
BUG: kernel reboot-without-warning in boot stage
Elapsed time: 10
#!/bin/bash
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k job-script # job-script is attached in this
email
Thanks,
Rong, Chen
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86_64 4.18.0-rc1 Kernel Configuration
#
#
# Compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
#
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_INSTRUCTION_DECODER=y
CONFIG_OUTPUT_FORMAT="elf64-x86-64"
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_MMU=y
CONFIG_ARCH_MMAP_RND_BITS_MIN=28
CONFIG_ARCH_MMAP_RND_BITS_MAX=32
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MIN=8
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX=16
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_ARCH_HAS_CPU_RELAX=y
CONFIG_ARCH_HAS_CACHE_LINE_SIZE=y
CONFIG_ARCH_HAS_FILTER_PGPROT=y
CONFIG_HAVE_SETUP_PER_CPU_AREA=y
CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK=y
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
CONFIG_ARCH_WANT_HUGE_PMD_SHARE=y
CONFIG_ARCH_WANT_GENERAL_HUGETLB=y
CONFIG_ZONE_DMA32=y
CONFIG_AUDIT_ARCH=y
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING=y
CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y
CONFIG_KASAN_SHADOW_OFFSET=0xdc00
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_PGTABLE_LEVELS=4
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=70300
CONFIG_CLANG_VERSION=0
CONFIG_CONSTRUCTORS=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y
#
# General setup
#
CONFIG_BROKEN_ON_SMP=y
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
# CONFIG_KERNEL_GZIP is not set
# CONFIG_KERNEL_BZIP2 is not set
CONFIG_KERNEL_LZMA=y
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
CONFIG_DEFAULT_HOSTNAME="(none)"
# CONFIG_SYSVIPC is not set
# CONFIG_POSIX_MQUEUE is not set
CONFIG_CROSS_MEMORY_ATTACH=y
# CONFIG_USELIB is not set
# CONFIG_AUDIT is not set
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_CHIP=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_SIM=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_GENERIC_IRQ_DEBUGFS=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
#
# CPU/Task time and stats accounting
#
CONFIG_TICK_CPU_ACCOUNTING=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
#
[LKP] [ovl] 24c944dd64: BUG:kernel_reboot-without-warning_in_boot_stage
FYI, we noticed the following commit (built with gcc-7):
commit: 24c944dd64f807542a2ec72744c81f064d1a60da ("ovl: Modify ovl_lookup() and
friends to lookup metacopy dentry")
https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git overlayfs-next
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -smp 2 -m 512M
caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
+-+++
| | 48b3292dcd | 24c944dd64 |
+-+++
| boot_successes | 34 | 0 |
| boot_failures | 10 | 48 |
| BUG:kernel_hang_in_test_stage | 8 ||
| invoked_oom-killer:gfp_mask=0x | 2 ||
| Mem-Info| 2 ||
| Out_of_memory:Kill_process | 2 ||
| BUG:kernel_reboot-without-warning_in_boot_stage | 0 | 48 |
+-+++
[0.00] BRK [0x1d3ee000, 0x1d3eefff] PGTABLE
[0.00] RAMDISK: [mem 0x1e73c000-0x1ffd]
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI: RSDP 0x000F6870 14 (v00 BOCHS )
[0.00] ACPI: RSDT 0x1FFE1628 30 (v01 BOCHS BXPCRSDT
0001 BXPC 0001)
BUG: kernel reboot-without-warning in boot stage
Elapsed time: 10
#!/bin/bash
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k job-script # job-script is attached in this
email
Thanks,
Rong, Chen
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86_64 4.18.0-rc1 Kernel Configuration
#
#
# Compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
#
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_INSTRUCTION_DECODER=y
CONFIG_OUTPUT_FORMAT="elf64-x86-64"
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_MMU=y
CONFIG_ARCH_MMAP_RND_BITS_MIN=28
CONFIG_ARCH_MMAP_RND_BITS_MAX=32
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MIN=8
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX=16
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_ARCH_HAS_CPU_RELAX=y
CONFIG_ARCH_HAS_CACHE_LINE_SIZE=y
CONFIG_ARCH_HAS_FILTER_PGPROT=y
CONFIG_HAVE_SETUP_PER_CPU_AREA=y
CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK=y
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
CONFIG_ARCH_WANT_HUGE_PMD_SHARE=y
CONFIG_ARCH_WANT_GENERAL_HUGETLB=y
CONFIG_ZONE_DMA32=y
CONFIG_AUDIT_ARCH=y
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING=y
CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y
CONFIG_KASAN_SHADOW_OFFSET=0xdc00
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_PGTABLE_LEVELS=4
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=70300
CONFIG_CLANG_VERSION=0
CONFIG_CONSTRUCTORS=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y
#
# General setup
#
CONFIG_BROKEN_ON_SMP=y
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
# CONFIG_KERNEL_GZIP is not set
# CONFIG_KERNEL_BZIP2 is not set
CONFIG_KERNEL_LZMA=y
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
CONFIG_DEFAULT_HOSTNAME="(none)"
# CONFIG_SYSVIPC is not set
# CONFIG_POSIX_MQUEUE is not set
CONFIG_CROSS_MEMORY_ATTACH=y
# CONFIG_USELIB is not set
# CONFIG_AUDIT is not set
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_CHIP=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_SIM=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_GENERIC_IRQ_DEBUGFS=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
#
# CPU/Task time and stats accounting
#
CONFIG_TICK_CPU_ACCOUNTING=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
#
Re: linux-next: Signed-off-by missing for commit in the xarray tree
On 07/25/2018 05:03 PM, Stephen Rothwell wrote:
> Hi Matthew,
>
> On Wed, 25 Jul 2018 16:36:21 -0700 Matthew Wilcox wrote:
>>
>> On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
>>>
>>> Commits
>>>
>>> 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
>>> aaf149902c79 ("filesystem-dax: Set page->index")
>>>
>>> are missing a Signed-off-by from their committers.
>>
>> Oh, hah. I assume this is an automated email?
>
> Well, semi-automatic :-)
>
>> These two commits I cherry-picked from the nvdimm tree so that XArray can
>> be rebased on top of it. Is there some other way I should be doing this,
>> like rebasing on top of the nvdimm tree?
>
> Ideally, the nvdimm tree would have just those two commits in a branch
> that you could merge (so that you both have the same commits (as
> opposed to patches)) that way these changes cannot cause conflicts when
> the files are further modifed in either tree. Alternatively, if you do
> have to cherry-pick them, then you need to add your Signed-off-by to
> the copy that you commit.
>
> As things are now, you could merge commit
>
> c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
>
> from the nvdimm tree into your tree before the conflicting commits in
> your tree (or just rebase your tree on top of that commit). You need
> to make sure that Dan and/or Dave (cc'd) will never rebase that part of
> their tree. Also, you will pick up some other commits (which may not
> be a problem for you).
>
I have all the acks I need for that branch for Dan's patches. So that
branch shouldn't change anymore AFAIK.
Re: linux-next: Signed-off-by missing for commit in the xarray tree
On 07/25/2018 05:03 PM, Stephen Rothwell wrote:
> Hi Matthew,
>
> On Wed, 25 Jul 2018 16:36:21 -0700 Matthew Wilcox wrote:
>>
>> On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
>>>
>>> Commits
>>>
>>> 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
>>> aaf149902c79 ("filesystem-dax: Set page->index")
>>>
>>> are missing a Signed-off-by from their committers.
>>
>> Oh, hah. I assume this is an automated email?
>
> Well, semi-automatic :-)
>
>> These two commits I cherry-picked from the nvdimm tree so that XArray can
>> be rebased on top of it. Is there some other way I should be doing this,
>> like rebasing on top of the nvdimm tree?
>
> Ideally, the nvdimm tree would have just those two commits in a branch
> that you could merge (so that you both have the same commits (as
> opposed to patches)) that way these changes cannot cause conflicts when
> the files are further modifed in either tree. Alternatively, if you do
> have to cherry-pick them, then you need to add your Signed-off-by to
> the copy that you commit.
>
> As things are now, you could merge commit
>
> c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
>
> from the nvdimm tree into your tree before the conflicting commits in
> your tree (or just rebase your tree on top of that commit). You need
> to make sure that Dan and/or Dave (cc'd) will never rebase that part of
> their tree. Also, you will pick up some other commits (which may not
> be a problem for you).
>
I have all the acks I need for that branch for Dan's patches. So that
branch shouldn't change anymore AFAIK.
Re: linux-next: Signed-off-by missing for commit in the xarray tree
Hi Matthew,
On Wed, 25 Jul 2018 16:36:21 -0700 Matthew Wilcox wrote:
>
> On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
> >
> > Commits
> >
> > 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
> > aaf149902c79 ("filesystem-dax: Set page->index")
> >
> > are missing a Signed-off-by from their committers.
>
> Oh, hah. I assume this is an automated email?
Well, semi-automatic :-)
> These two commits I cherry-picked from the nvdimm tree so that XArray can
> be rebased on top of it. Is there some other way I should be doing this,
> like rebasing on top of the nvdimm tree?
Ideally, the nvdimm tree would have just those two commits in a branch
that you could merge (so that you both have the same commits (as
opposed to patches)) that way these changes cannot cause conflicts when
the files are further modifed in either tree. Alternatively, if you do
have to cherry-pick them, then you need to add your Signed-off-by to
the copy that you commit.
As things are now, you could merge commit
c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
from the nvdimm tree into your tree before the conflicting commits in
your tree (or just rebase your tree on top of that commit). You need
to make sure that Dan and/or Dave (cc'd) will never rebase that part of
their tree. Also, you will pick up some other commits (which may not
be a problem for you).
--
Cheers,
Stephen Rothwell
pgpfefLdYxXfn.pgp
Description: OpenPGP digital signature
Re: linux-next: Signed-off-by missing for commit in the xarray tree
Hi Matthew,
On Wed, 25 Jul 2018 16:36:21 -0700 Matthew Wilcox wrote:
>
> On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
> >
> > Commits
> >
> > 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
> > aaf149902c79 ("filesystem-dax: Set page->index")
> >
> > are missing a Signed-off-by from their committers.
>
> Oh, hah. I assume this is an automated email?
Well, semi-automatic :-)
> These two commits I cherry-picked from the nvdimm tree so that XArray can
> be rebased on top of it. Is there some other way I should be doing this,
> like rebasing on top of the nvdimm tree?
Ideally, the nvdimm tree would have just those two commits in a branch
that you could merge (so that you both have the same commits (as
opposed to patches)) that way these changes cannot cause conflicts when
the files are further modifed in either tree. Alternatively, if you do
have to cherry-pick them, then you need to add your Signed-off-by to
the copy that you commit.
As things are now, you could merge commit
c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
from the nvdimm tree into your tree before the conflicting commits in
your tree (or just rebase your tree on top of that commit). You need
to make sure that Dan and/or Dave (cc'd) will never rebase that part of
their tree. Also, you will pick up some other commits (which may not
be a problem for you).
--
Cheers,
Stephen Rothwell
pgpfefLdYxXfn.pgp
Description: OpenPGP digital signature
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
Thanks a lot for your comments.
Sorry for my late reply, I was on vacation.
The last days I have been working to move NPCM pinctrl GPIO to GENERIC
GPIO, most of the work have been done but I had the some issue.
I initialize bgpio as follow:
ret = bgpio_init(>gpio_bank[id].gc,
pctrl->dev, 4,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DIN,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DOUT,
NULL,
NULL,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_IEM,
BGPIOF_READ_OUTPUT_REG_SET);
After doing it, the directions functions I used are:
bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv
and the I/O get function is bgpio_get_set
By using inv directions:
direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio))
direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio))
The problem occur when reading the GPIO value from bgpio_get_set
function, because the directions value are inverse it reading the
wrong I/O registers
For direction out it reading dat register (instead of set register)
For direction in it calling set register (instead of dat register)
if (gc->bgpio_dir & pinmask)
return !!(gc->read_reg(gc->reg_set) & pinmask);
else
return !!(gc->read_reg(gc->reg_dat) & pinmask);
the same issue occur at bgpio_get_set_multiple function.
Maybe in bgpio_dir parameter direction out should be in both cases 1
and direction in = 0.
for now i did a local fix in the npcm pinctrl driver by setting
bgpio_dir parameters as direction out = 1 and direction in = 0.
Thanks a lot,
Tomer
On 13 July 2018 at 11:51, Linus Walleij wrote:
> On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
>
> > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> > GPIO controller driver.
> >
> > Signed-off-by: Tomer Maimon
>
> (...)
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> > @@ -0,0 +1,2089 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> > +// Copyright (c) 2016, Dell Inc
> > +
> > +#include
> > +#include
>
> As this is a driver you should only include
>
> > +#include
> > +#include
> > +#include
>
> If you need syscon then the driver should select or depend
> on MFD_SYSCON in Kconfig.
>
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do you really need this include?
>
> > +/* Structure for register banks */
> > +struct NPCM7XX_GPIO {
>
> Can we have this lowercase? Please?
>
> > + void __iomem*base;
> > + struct gpio_chipgc;
> > + int irqbase;
> > + int irq;
> > + spinlock_t lock;
> > + void*priv;
> > + struct irq_chip irq_chip;
> > + u32 pinctrl_id;
> > +};
>
> So each GPIO bank has its own gpio chip and register
> base, that is NICE! Because then it looks like you can
> select GPIO_GENERIC and use the MMIO GPIO helper
> library to handle the registers. Let's look into that
> option!
>
> > +struct NPCM7xx_pinctrl {
> > + struct pinctrl_dev *pctldev;
> > + struct device *dev;
> > + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> > + struct irq_domain *domain;
>
> I wonder why the pin controller needs and IRQ domain but
> I keep reading the code and I might find out...
>
> > +enum operand {
> > + op_set,
> > + op_getbit,
> > + op_setbit,
> > + op_clrbit,
> > +};
>
> This looks complicated. I suspect you can use GPIO_GENERIC
> to set/get and clear bits in the register(s).
>
> > +/* Perform locked bit operations on GPIO registers */
> > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int
> offset,
> > + int reg)
> > +{
> > + unsigned long flags;
> > + u32 mask, val;
> > +
> > + mask = (1L << offset);
> > + spin_lock_irqsave(>lock, flags);
> > + switch (op) {
> > + case op_set:
> > + iowrite32(mask, bank->base + reg);
> > + break;
> > + case op_getbit:
> > + mask &= ioread32(bank->base + reg);
> > + break;
> > + case op_setbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val | mask, bank->base + reg);
> > + break;
> > + case op_clrbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val & (~mask), bank->base + reg);
> > + break;
> > + }
> > +
Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
Hi Linus,
Thanks a lot for your comments.
Sorry for my late reply, I was on vacation.
The last days I have been working to move NPCM pinctrl GPIO to GENERIC
GPIO, most of the work have been done but I had the some issue.
I initialize bgpio as follow:
ret = bgpio_init(>gpio_bank[id].gc,
pctrl->dev, 4,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DIN,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_DOUT,
NULL,
NULL,
pctrl->gpio_bank[id].base +
NPCM7XX_GP_N_IEM,
BGPIOF_READ_OUTPUT_REG_SET);
After doing it, the directions functions I used are:
bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv
and the I/O get function is bgpio_get_set
By using inv directions:
direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio))
direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio))
The problem occur when reading the GPIO value from bgpio_get_set
function, because the directions value are inverse it reading the
wrong I/O registers
For direction out it reading dat register (instead of set register)
For direction in it calling set register (instead of dat register)
if (gc->bgpio_dir & pinmask)
return !!(gc->read_reg(gc->reg_set) & pinmask);
else
return !!(gc->read_reg(gc->reg_dat) & pinmask);
the same issue occur at bgpio_get_set_multiple function.
Maybe in bgpio_dir parameter direction out should be in both cases 1
and direction in = 0.
for now i did a local fix in the npcm pinctrl driver by setting
bgpio_dir parameters as direction out = 1 and direction in = 0.
Thanks a lot,
Tomer
On 13 July 2018 at 11:51, Linus Walleij wrote:
> On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon wrote:
>
> > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> > GPIO controller driver.
> >
> > Signed-off-by: Tomer Maimon
>
> (...)
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> > @@ -0,0 +1,2089 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> > +// Copyright (c) 2016, Dell Inc
> > +
> > +#include
> > +#include
>
> As this is a driver you should only include
>
> > +#include
> > +#include
> > +#include
>
> If you need syscon then the driver should select or depend
> on MFD_SYSCON in Kconfig.
>
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do you really need this include?
>
> > +/* Structure for register banks */
> > +struct NPCM7XX_GPIO {
>
> Can we have this lowercase? Please?
>
> > + void __iomem*base;
> > + struct gpio_chipgc;
> > + int irqbase;
> > + int irq;
> > + spinlock_t lock;
> > + void*priv;
> > + struct irq_chip irq_chip;
> > + u32 pinctrl_id;
> > +};
>
> So each GPIO bank has its own gpio chip and register
> base, that is NICE! Because then it looks like you can
> select GPIO_GENERIC and use the MMIO GPIO helper
> library to handle the registers. Let's look into that
> option!
>
> > +struct NPCM7xx_pinctrl {
> > + struct pinctrl_dev *pctldev;
> > + struct device *dev;
> > + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> > + struct irq_domain *domain;
>
> I wonder why the pin controller needs and IRQ domain but
> I keep reading the code and I might find out...
>
> > +enum operand {
> > + op_set,
> > + op_getbit,
> > + op_setbit,
> > + op_clrbit,
> > +};
>
> This looks complicated. I suspect you can use GPIO_GENERIC
> to set/get and clear bits in the register(s).
>
> > +/* Perform locked bit operations on GPIO registers */
> > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int
> offset,
> > + int reg)
> > +{
> > + unsigned long flags;
> > + u32 mask, val;
> > +
> > + mask = (1L << offset);
> > + spin_lock_irqsave(>lock, flags);
> > + switch (op) {
> > + case op_set:
> > + iowrite32(mask, bank->base + reg);
> > + break;
> > + case op_getbit:
> > + mask &= ioread32(bank->base + reg);
> > + break;
> > + case op_setbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val | mask, bank->base + reg);
> > + break;
> > + case op_clrbit:
> > + val = ioread32(bank->base + reg);
> > + iowrite32(val & (~mask), bank->base + reg);
> > + break;
> > + }
> > +
[PATCH v4 1/9] proc/kcore: don't grab lock for kclist_add()
From: Omar Sandoval
kclist_add() is only called at init time, so there's no point in
grabbing any locks. We're also going to replace the rwlock with a rwsem,
which we don't want to try grabbing during early boot.
While we're here, mark kclist_add() with __init so that we'll get a
warning if it's called from non-init code.
Reviewed-by: Andrew Morton
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 7 +++
include/linux/kcore.h | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 66c373230e60..b0b9a76f28d6 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -62,16 +62,15 @@ static LIST_HEAD(kclist_head);
static DEFINE_RWLOCK(kclist_lock);
static int kcore_need_update = 1;
-void
-kclist_add(struct kcore_list *new, void *addr, size_t size, int type)
+/* This doesn't grab kclist_lock, so it should only be used at init time. */
+void __init kclist_add(struct kcore_list *new, void *addr, size_t size,
+ int type)
{
new->addr = (unsigned long)addr;
new->size = size;
new->type = type;
- write_lock(_lock);
list_add_tail(>list, _head);
- write_unlock(_lock);
}
static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
diff --git a/include/linux/kcore.h b/include/linux/kcore.h
index 8de55e4b5ee9..c20f296438fb 100644
--- a/include/linux/kcore.h
+++ b/include/linux/kcore.h
@@ -35,7 +35,7 @@ struct vmcoredd_node {
};
#ifdef CONFIG_PROC_KCORE
-extern void kclist_add(struct kcore_list *, void *, size_t, int type);
+void __init kclist_add(struct kcore_list *, void *, size_t, int type);
#else
static inline
void kclist_add(struct kcore_list *new, void *addr, size_t size, int type)
--
2.18.0
[PATCH v4 1/9] proc/kcore: don't grab lock for kclist_add()
From: Omar Sandoval
kclist_add() is only called at init time, so there's no point in
grabbing any locks. We're also going to replace the rwlock with a rwsem,
which we don't want to try grabbing during early boot.
While we're here, mark kclist_add() with __init so that we'll get a
warning if it's called from non-init code.
Reviewed-by: Andrew Morton
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 7 +++
include/linux/kcore.h | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 66c373230e60..b0b9a76f28d6 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -62,16 +62,15 @@ static LIST_HEAD(kclist_head);
static DEFINE_RWLOCK(kclist_lock);
static int kcore_need_update = 1;
-void
-kclist_add(struct kcore_list *new, void *addr, size_t size, int type)
+/* This doesn't grab kclist_lock, so it should only be used at init time. */
+void __init kclist_add(struct kcore_list *new, void *addr, size_t size,
+ int type)
{
new->addr = (unsigned long)addr;
new->size = size;
new->type = type;
- write_lock(_lock);
list_add_tail(>list, _head);
- write_unlock(_lock);
}
static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
diff --git a/include/linux/kcore.h b/include/linux/kcore.h
index 8de55e4b5ee9..c20f296438fb 100644
--- a/include/linux/kcore.h
+++ b/include/linux/kcore.h
@@ -35,7 +35,7 @@ struct vmcoredd_node {
};
#ifdef CONFIG_PROC_KCORE
-extern void kclist_add(struct kcore_list *, void *, size_t, int type);
+void __init kclist_add(struct kcore_list *, void *, size_t, int type);
#else
static inline
void kclist_add(struct kcore_list *new, void *addr, size_t size, int type)
--
2.18.0
[PATCH v4 3/9] proc/kcore: replace kclist_lock rwlock with rwsem
From: Omar Sandoval
Now we only need kclist_lock from user context and at fs init time, and
the following changes need to sleep while holding the kclist_lock.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index e83f15a4f66d..ae43a97d511d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -59,7 +59,7 @@ struct memelfnote
};
static LIST_HEAD(kclist_head);
-static DEFINE_RWLOCK(kclist_lock);
+static DECLARE_RWSEM(kclist_lock);
static int kcore_need_update = 1;
/* This doesn't grab kclist_lock, so it should only be used at init time. */
@@ -117,7 +117,7 @@ static void __kcore_update_ram(struct list_head *list)
struct kcore_list *tmp, *pos;
LIST_HEAD(garbage);
- write_lock(_lock);
+ down_write(_lock);
if (xchg(_need_update, 0)) {
list_for_each_entry_safe(pos, tmp, _head, list) {
if (pos->type == KCORE_RAM
@@ -128,7 +128,7 @@ static void __kcore_update_ram(struct list_head *list)
} else
list_splice(list, );
proc_root_kcore->size = get_kcore_size(, );
- write_unlock(_lock);
+ up_write(_lock);
free_kclist_ents();
}
@@ -451,11 +451,11 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
int nphdr;
unsigned long start;
- read_lock(_lock);
+ down_read(_lock);
size = get_kcore_size(, _buflen);
if (buflen == 0 || *fpos >= size) {
- read_unlock(_lock);
+ up_read(_lock);
return 0;
}
@@ -472,11 +472,11 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
tsz = buflen;
elf_buf = kzalloc(elf_buflen, GFP_ATOMIC);
if (!elf_buf) {
- read_unlock(_lock);
+ up_read(_lock);
return -ENOMEM;
}
elf_kcore_store_hdr(elf_buf, nphdr, elf_buflen);
- read_unlock(_lock);
+ up_read(_lock);
if (copy_to_user(buffer, elf_buf + *fpos, tsz)) {
kfree(elf_buf);
return -EFAULT;
@@ -491,7 +491,7 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
if (buflen == 0)
return acc;
} else
- read_unlock(_lock);
+ up_read(_lock);
/*
* Check to see if our file offset matches with any of
@@ -504,12 +504,12 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
while (buflen) {
struct kcore_list *m;
- read_lock(_lock);
+ down_read(_lock);
list_for_each_entry(m, _head, list) {
if (start >= m->addr && start < (m->addr+m->size))
break;
}
- read_unlock(_lock);
+ up_read(_lock);
if (>list == _head) {
if (clear_user(buffer, tsz))
--
2.18.0
[PATCH v4 3/9] proc/kcore: replace kclist_lock rwlock with rwsem
From: Omar Sandoval
Now we only need kclist_lock from user context and at fs init time, and
the following changes need to sleep while holding the kclist_lock.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index e83f15a4f66d..ae43a97d511d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -59,7 +59,7 @@ struct memelfnote
};
static LIST_HEAD(kclist_head);
-static DEFINE_RWLOCK(kclist_lock);
+static DECLARE_RWSEM(kclist_lock);
static int kcore_need_update = 1;
/* This doesn't grab kclist_lock, so it should only be used at init time. */
@@ -117,7 +117,7 @@ static void __kcore_update_ram(struct list_head *list)
struct kcore_list *tmp, *pos;
LIST_HEAD(garbage);
- write_lock(_lock);
+ down_write(_lock);
if (xchg(_need_update, 0)) {
list_for_each_entry_safe(pos, tmp, _head, list) {
if (pos->type == KCORE_RAM
@@ -128,7 +128,7 @@ static void __kcore_update_ram(struct list_head *list)
} else
list_splice(list, );
proc_root_kcore->size = get_kcore_size(, );
- write_unlock(_lock);
+ up_write(_lock);
free_kclist_ents();
}
@@ -451,11 +451,11 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
int nphdr;
unsigned long start;
- read_lock(_lock);
+ down_read(_lock);
size = get_kcore_size(, _buflen);
if (buflen == 0 || *fpos >= size) {
- read_unlock(_lock);
+ up_read(_lock);
return 0;
}
@@ -472,11 +472,11 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
tsz = buflen;
elf_buf = kzalloc(elf_buflen, GFP_ATOMIC);
if (!elf_buf) {
- read_unlock(_lock);
+ up_read(_lock);
return -ENOMEM;
}
elf_kcore_store_hdr(elf_buf, nphdr, elf_buflen);
- read_unlock(_lock);
+ up_read(_lock);
if (copy_to_user(buffer, elf_buf + *fpos, tsz)) {
kfree(elf_buf);
return -EFAULT;
@@ -491,7 +491,7 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
if (buflen == 0)
return acc;
} else
- read_unlock(_lock);
+ up_read(_lock);
/*
* Check to see if our file offset matches with any of
@@ -504,12 +504,12 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
while (buflen) {
struct kcore_list *m;
- read_lock(_lock);
+ down_read(_lock);
list_for_each_entry(m, _head, list) {
if (start >= m->addr && start < (m->addr+m->size))
break;
}
- read_unlock(_lock);
+ up_read(_lock);
if (>list == _head) {
if (clear_user(buffer, tsz))
--
2.18.0
[PATCH v4 5/9] proc/kcore: hold lock during read
From: Omar Sandoval
Now that we're using an rwsem, we can hold it during the entirety of
read_kcore() and have a common return path. This is preparation for the
next change.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 70 -
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 95aa988c5b5d..dc34642bbdb7 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -440,19 +440,18 @@ static ssize_t
read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
{
char *buf = file->private_data;
- ssize_t acc = 0;
size_t size, tsz;
size_t elf_buflen;
int nphdr;
unsigned long start;
+ size_t orig_buflen = buflen;
+ int ret = 0;
down_read(_lock);
size = get_kcore_size(, _buflen);
- if (buflen == 0 || *fpos >= size) {
- up_read(_lock);
- return 0;
- }
+ if (buflen == 0 || *fpos >= size)
+ goto out;
/* trim buflen to not go beyond EOF */
if (buflen > size - *fpos)
@@ -465,28 +464,26 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
tsz = elf_buflen - *fpos;
if (buflen < tsz)
tsz = buflen;
- elf_buf = kzalloc(elf_buflen, GFP_ATOMIC);
+ elf_buf = kzalloc(elf_buflen, GFP_KERNEL);
if (!elf_buf) {
- up_read(_lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
elf_kcore_store_hdr(elf_buf, nphdr, elf_buflen);
- up_read(_lock);
if (copy_to_user(buffer, elf_buf + *fpos, tsz)) {
kfree(elf_buf);
- return -EFAULT;
+ ret = -EFAULT;
+ goto out;
}
kfree(elf_buf);
buflen -= tsz;
*fpos += tsz;
buffer += tsz;
- acc += tsz;
/* leave now if filled buffer already */
if (buflen == 0)
- return acc;
- } else
- up_read(_lock);
+ goto out;
+ }
/*
* Check to see if our file offset matches with any of
@@ -499,25 +496,29 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
while (buflen) {
struct kcore_list *m;
- down_read(_lock);
list_for_each_entry(m, _head, list) {
if (start >= m->addr && start < (m->addr+m->size))
break;
}
- up_read(_lock);
if (>list == _head) {
- if (clear_user(buffer, tsz))
- return -EFAULT;
+ if (clear_user(buffer, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else if (m->type == KCORE_VMALLOC) {
vread(buf, (char *)start, tsz);
/* we have to zero-fill user buffer even if no read */
- if (copy_to_user(buffer, buf, tsz))
- return -EFAULT;
+ if (copy_to_user(buffer, buf, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else if (m->type == KCORE_USER) {
/* User page is handled prior to normal kernel page: */
- if (copy_to_user(buffer, (char *)start, tsz))
- return -EFAULT;
+ if (copy_to_user(buffer, (char *)start, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else {
if (kern_addr_valid(start)) {
/*
@@ -525,26 +526,35 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
* hardened user copy kernel text checks.
*/
if (probe_kernel_read(buf, (void *) start,
tsz)) {
- if (clear_user(buffer, tsz))
- return -EFAULT;
+ if (clear_user(buffer, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else {
- if (copy_to_user(buffer, buf,
[PATCH v4 5/9] proc/kcore: hold lock during read
From: Omar Sandoval
Now that we're using an rwsem, we can hold it during the entirety of
read_kcore() and have a common return path. This is preparation for the
next change.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 70 -
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 95aa988c5b5d..dc34642bbdb7 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -440,19 +440,18 @@ static ssize_t
read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
{
char *buf = file->private_data;
- ssize_t acc = 0;
size_t size, tsz;
size_t elf_buflen;
int nphdr;
unsigned long start;
+ size_t orig_buflen = buflen;
+ int ret = 0;
down_read(_lock);
size = get_kcore_size(, _buflen);
- if (buflen == 0 || *fpos >= size) {
- up_read(_lock);
- return 0;
- }
+ if (buflen == 0 || *fpos >= size)
+ goto out;
/* trim buflen to not go beyond EOF */
if (buflen > size - *fpos)
@@ -465,28 +464,26 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
tsz = elf_buflen - *fpos;
if (buflen < tsz)
tsz = buflen;
- elf_buf = kzalloc(elf_buflen, GFP_ATOMIC);
+ elf_buf = kzalloc(elf_buflen, GFP_KERNEL);
if (!elf_buf) {
- up_read(_lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
elf_kcore_store_hdr(elf_buf, nphdr, elf_buflen);
- up_read(_lock);
if (copy_to_user(buffer, elf_buf + *fpos, tsz)) {
kfree(elf_buf);
- return -EFAULT;
+ ret = -EFAULT;
+ goto out;
}
kfree(elf_buf);
buflen -= tsz;
*fpos += tsz;
buffer += tsz;
- acc += tsz;
/* leave now if filled buffer already */
if (buflen == 0)
- return acc;
- } else
- up_read(_lock);
+ goto out;
+ }
/*
* Check to see if our file offset matches with any of
@@ -499,25 +496,29 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
while (buflen) {
struct kcore_list *m;
- down_read(_lock);
list_for_each_entry(m, _head, list) {
if (start >= m->addr && start < (m->addr+m->size))
break;
}
- up_read(_lock);
if (>list == _head) {
- if (clear_user(buffer, tsz))
- return -EFAULT;
+ if (clear_user(buffer, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else if (m->type == KCORE_VMALLOC) {
vread(buf, (char *)start, tsz);
/* we have to zero-fill user buffer even if no read */
- if (copy_to_user(buffer, buf, tsz))
- return -EFAULT;
+ if (copy_to_user(buffer, buf, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else if (m->type == KCORE_USER) {
/* User page is handled prior to normal kernel page: */
- if (copy_to_user(buffer, (char *)start, tsz))
- return -EFAULT;
+ if (copy_to_user(buffer, (char *)start, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else {
if (kern_addr_valid(start)) {
/*
@@ -525,26 +526,35 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
* hardened user copy kernel text checks.
*/
if (probe_kernel_read(buf, (void *) start,
tsz)) {
- if (clear_user(buffer, tsz))
- return -EFAULT;
+ if (clear_user(buffer, tsz)) {
+ ret = -EFAULT;
+ goto out;
+ }
} else {
- if (copy_to_user(buffer, buf,
[PATCH v4 7/9] proc/kcore: optimize multiple page reads
From: Omar Sandoval
The current code does a full search of the segment list every time for
every page. This is wasteful, since it's almost certain that the next
page will be in the same segment. Instead, check if the previous segment
covers the current page before doing the list search.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 808ef9afd084..758c14e46a44 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -428,10 +428,18 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
if ((tsz = (PAGE_SIZE - (start & ~PAGE_MASK))) > buflen)
tsz = buflen;
+ m = NULL;
while (buflen) {
- list_for_each_entry(m, _head, list) {
- if (start >= m->addr && start < (m->addr+m->size))
- break;
+ /*
+* If this is the first iteration or the address is not within
+* the previous entry, search for a matching entry.
+*/
+ if (!m || start < m->addr || start >= m->addr + m->size) {
+ list_for_each_entry(m, _head, list) {
+ if (start >= m->addr &&
+ start < m->addr + m->size)
+ break;
+ }
}
if (>list == _head) {
--
2.18.0
[PATCH v4 7/9] proc/kcore: optimize multiple page reads
From: Omar Sandoval
The current code does a full search of the segment list every time for
every page. This is wasteful, since it's almost certain that the next
page will be in the same segment. Instead, check if the previous segment
covers the current page before doing the list search.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 808ef9afd084..758c14e46a44 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -428,10 +428,18 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
if ((tsz = (PAGE_SIZE - (start & ~PAGE_MASK))) > buflen)
tsz = buflen;
+ m = NULL;
while (buflen) {
- list_for_each_entry(m, _head, list) {
- if (start >= m->addr && start < (m->addr+m->size))
- break;
+ /*
+* If this is the first iteration or the address is not within
+* the previous entry, search for a matching entry.
+*/
+ if (!m || start < m->addr || start >= m->addr + m->size) {
+ list_for_each_entry(m, _head, list) {
+ if (start >= m->addr &&
+ start < m->addr + m->size)
+ break;
+ }
}
if (>list == _head) {
--
2.18.0
[PATCH v4 2/9] proc/kcore: don't grab lock for memory hotplug notifier
From: Omar Sandoval
The memory hotplug notifier kcore_callback() only needs kclist_lock to
prevent races with __kcore_update_ram(), but we can easily eliminate
that race by using an atomic xchg() in __kcore_update_ram(). This is
preparation for converting kclist_lock to an rwsem.
Reviewed-by: Andrew Morton
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index b0b9a76f28d6..e83f15a4f66d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -118,7 +118,7 @@ static void __kcore_update_ram(struct list_head *list)
LIST_HEAD(garbage);
write_lock(_lock);
- if (kcore_need_update) {
+ if (xchg(_need_update, 0)) {
list_for_each_entry_safe(pos, tmp, _head, list) {
if (pos->type == KCORE_RAM
|| pos->type == KCORE_VMEMMAP)
@@ -127,7 +127,6 @@ static void __kcore_update_ram(struct list_head *list)
list_splice_tail(list, _head);
} else
list_splice(list, );
- kcore_need_update = 0;
proc_root_kcore->size = get_kcore_size(, );
write_unlock(_lock);
@@ -593,9 +592,8 @@ static int __meminit kcore_callback(struct notifier_block
*self,
switch (action) {
case MEM_ONLINE:
case MEM_OFFLINE:
- write_lock(_lock);
kcore_need_update = 1;
- write_unlock(_lock);
+ break;
}
return NOTIFY_OK;
}
--
2.18.0
[PATCH v4 6/9] proc/kcore: clean up ELF header generation
From: Omar Sandoval
Currently, the ELF file header, program headers, and note segment are
allocated all at once, in some icky code dating back to 2.3. Programs
tend to read the file header, then the program headers, then the note
segment, all separately, so this is a waste of effort. It's cleaner and
more efficient to handle the three separately.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 350 +++-
1 file changed, 141 insertions(+), 209 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index dc34642bbdb7..808ef9afd084 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -49,15 +49,6 @@ static struct proc_dir_entry *proc_root_kcore;
#definekc_offset_to_vaddr(o) ((o) + PAGE_OFFSET)
#endif
-/* An ELF note in memory */
-struct memelfnote
-{
- const char *name;
- int type;
- unsigned int datasz;
- void *data;
-};
-
static LIST_HEAD(kclist_head);
static DECLARE_RWSEM(kclist_lock);
static int kcore_need_update = 1;
@@ -73,7 +64,8 @@ void __init kclist_add(struct kcore_list *new, void *addr,
size_t size,
list_add_tail(>list, _head);
}
-static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
+static size_t get_kcore_size(int *nphdr, size_t *phdrs_len, size_t *notes_len,
+size_t *data_offset)
{
size_t try, size;
struct kcore_list *m;
@@ -87,15 +79,15 @@ static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
size = try;
*nphdr = *nphdr + 1;
}
- *elf_buflen = sizeof(struct elfhdr) +
- (*nphdr + 2)*sizeof(struct elf_phdr) +
- 3 * ((sizeof(struct elf_note)) +
-roundup(sizeof(CORE_STR), 4)) +
- roundup(sizeof(struct elf_prstatus), 4) +
- roundup(sizeof(struct elf_prpsinfo), 4) +
- roundup(arch_task_struct_size, 4);
- *elf_buflen = PAGE_ALIGN(*elf_buflen);
- return size + *elf_buflen;
+
+ *phdrs_len = *nphdr * sizeof(struct elf_phdr);
+ *notes_len = (3 * (sizeof(struct elf_note) + ALIGN(sizeof(CORE_STR),
4)) +
+ ALIGN(sizeof(struct elf_prstatus), 4) +
+ ALIGN(sizeof(struct elf_prpsinfo), 4) +
+ ALIGN(arch_task_struct_size, 4));
+ *data_offset = PAGE_ALIGN(sizeof(struct elfhdr) + *phdrs_len +
+ *notes_len);
+ return *data_offset + size;
}
#ifdef CONFIG_HIGHMEM
@@ -241,7 +233,7 @@ static int kcore_update_ram(void)
LIST_HEAD(list);
LIST_HEAD(garbage);
int nphdr;
- size_t size;
+ size_t phdrs_len, notes_len, data_offset;
struct kcore_list *tmp, *pos;
int ret = 0;
@@ -263,7 +255,8 @@ static int kcore_update_ram(void)
}
list_splice_tail(, _head);
- proc_root_kcore->size = get_kcore_size(, );
+ proc_root_kcore->size = get_kcore_size(, _len, _len,
+ _offset);
out:
up_write(_lock);
@@ -274,228 +267,168 @@ static int kcore_update_ram(void)
return ret;
}
-/*/
-/*
- * determine size of ELF note
- */
-static int notesize(struct memelfnote *en)
+static void append_kcore_note(char *notes, size_t *i, const char *name,
+ unsigned int type, const void *desc,
+ size_t descsz)
{
- int sz;
-
- sz = sizeof(struct elf_note);
- sz += roundup((strlen(en->name) + 1), 4);
- sz += roundup(en->datasz, 4);
-
- return sz;
-} /* end notesize() */
-
-/*/
-/*
- * store a note in the header buffer
- */
-static char *storenote(struct memelfnote *men, char *bufp)
-{
- struct elf_note en;
-
-#define DUMP_WRITE(addr,nr) do { memcpy(bufp,addr,nr); bufp += nr; } while(0)
-
- en.n_namesz = strlen(men->name) + 1;
- en.n_descsz = men->datasz;
- en.n_type = men->type;
-
- DUMP_WRITE(, sizeof(en));
- DUMP_WRITE(men->name, en.n_namesz);
-
- /* XXX - cast from long long to long to avoid need for libgcc.a */
- bufp = (char*) roundup((unsigned long)bufp,4);
- DUMP_WRITE(men->data, men->datasz);
- bufp = (char*) roundup((unsigned long)bufp,4);
-
-#undef DUMP_WRITE
-
- return bufp;
-} /* end storenote() */
-
-/*
- * store an ELF coredump header in the supplied buffer
- * nphdr is the number of elf_phdr to insert
- */
-static void elf_kcore_store_hdr(char *bufp, int nphdr, int dataoff)
-{
- struct elf_prstatus prstatus; /* NT_PRSTATUS */
- struct elf_prpsinfo prpsinfo; /* NT_PRPSINFO */
- struct elf_phdr *nhdr, *phdr;
- struct elfhdr *elf;
- struct memelfnote notes[3];
- off_t offset = 0;
-
[PATCH v4 2/9] proc/kcore: don't grab lock for memory hotplug notifier
From: Omar Sandoval
The memory hotplug notifier kcore_callback() only needs kclist_lock to
prevent races with __kcore_update_ram(), but we can easily eliminate
that race by using an atomic xchg() in __kcore_update_ram(). This is
preparation for converting kclist_lock to an rwsem.
Reviewed-by: Andrew Morton
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index b0b9a76f28d6..e83f15a4f66d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -118,7 +118,7 @@ static void __kcore_update_ram(struct list_head *list)
LIST_HEAD(garbage);
write_lock(_lock);
- if (kcore_need_update) {
+ if (xchg(_need_update, 0)) {
list_for_each_entry_safe(pos, tmp, _head, list) {
if (pos->type == KCORE_RAM
|| pos->type == KCORE_VMEMMAP)
@@ -127,7 +127,6 @@ static void __kcore_update_ram(struct list_head *list)
list_splice_tail(list, _head);
} else
list_splice(list, );
- kcore_need_update = 0;
proc_root_kcore->size = get_kcore_size(, );
write_unlock(_lock);
@@ -593,9 +592,8 @@ static int __meminit kcore_callback(struct notifier_block
*self,
switch (action) {
case MEM_ONLINE:
case MEM_OFFLINE:
- write_lock(_lock);
kcore_need_update = 1;
- write_unlock(_lock);
+ break;
}
return NOTIFY_OK;
}
--
2.18.0
[PATCH v4 6/9] proc/kcore: clean up ELF header generation
From: Omar Sandoval
Currently, the ELF file header, program headers, and note segment are
allocated all at once, in some icky code dating back to 2.3. Programs
tend to read the file header, then the program headers, then the note
segment, all separately, so this is a waste of effort. It's cleaner and
more efficient to handle the three separately.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 350 +++-
1 file changed, 141 insertions(+), 209 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index dc34642bbdb7..808ef9afd084 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -49,15 +49,6 @@ static struct proc_dir_entry *proc_root_kcore;
#definekc_offset_to_vaddr(o) ((o) + PAGE_OFFSET)
#endif
-/* An ELF note in memory */
-struct memelfnote
-{
- const char *name;
- int type;
- unsigned int datasz;
- void *data;
-};
-
static LIST_HEAD(kclist_head);
static DECLARE_RWSEM(kclist_lock);
static int kcore_need_update = 1;
@@ -73,7 +64,8 @@ void __init kclist_add(struct kcore_list *new, void *addr,
size_t size,
list_add_tail(>list, _head);
}
-static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
+static size_t get_kcore_size(int *nphdr, size_t *phdrs_len, size_t *notes_len,
+size_t *data_offset)
{
size_t try, size;
struct kcore_list *m;
@@ -87,15 +79,15 @@ static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
size = try;
*nphdr = *nphdr + 1;
}
- *elf_buflen = sizeof(struct elfhdr) +
- (*nphdr + 2)*sizeof(struct elf_phdr) +
- 3 * ((sizeof(struct elf_note)) +
-roundup(sizeof(CORE_STR), 4)) +
- roundup(sizeof(struct elf_prstatus), 4) +
- roundup(sizeof(struct elf_prpsinfo), 4) +
- roundup(arch_task_struct_size, 4);
- *elf_buflen = PAGE_ALIGN(*elf_buflen);
- return size + *elf_buflen;
+
+ *phdrs_len = *nphdr * sizeof(struct elf_phdr);
+ *notes_len = (3 * (sizeof(struct elf_note) + ALIGN(sizeof(CORE_STR),
4)) +
+ ALIGN(sizeof(struct elf_prstatus), 4) +
+ ALIGN(sizeof(struct elf_prpsinfo), 4) +
+ ALIGN(arch_task_struct_size, 4));
+ *data_offset = PAGE_ALIGN(sizeof(struct elfhdr) + *phdrs_len +
+ *notes_len);
+ return *data_offset + size;
}
#ifdef CONFIG_HIGHMEM
@@ -241,7 +233,7 @@ static int kcore_update_ram(void)
LIST_HEAD(list);
LIST_HEAD(garbage);
int nphdr;
- size_t size;
+ size_t phdrs_len, notes_len, data_offset;
struct kcore_list *tmp, *pos;
int ret = 0;
@@ -263,7 +255,8 @@ static int kcore_update_ram(void)
}
list_splice_tail(, _head);
- proc_root_kcore->size = get_kcore_size(, );
+ proc_root_kcore->size = get_kcore_size(, _len, _len,
+ _offset);
out:
up_write(_lock);
@@ -274,228 +267,168 @@ static int kcore_update_ram(void)
return ret;
}
-/*/
-/*
- * determine size of ELF note
- */
-static int notesize(struct memelfnote *en)
+static void append_kcore_note(char *notes, size_t *i, const char *name,
+ unsigned int type, const void *desc,
+ size_t descsz)
{
- int sz;
-
- sz = sizeof(struct elf_note);
- sz += roundup((strlen(en->name) + 1), 4);
- sz += roundup(en->datasz, 4);
-
- return sz;
-} /* end notesize() */
-
-/*/
-/*
- * store a note in the header buffer
- */
-static char *storenote(struct memelfnote *men, char *bufp)
-{
- struct elf_note en;
-
-#define DUMP_WRITE(addr,nr) do { memcpy(bufp,addr,nr); bufp += nr; } while(0)
-
- en.n_namesz = strlen(men->name) + 1;
- en.n_descsz = men->datasz;
- en.n_type = men->type;
-
- DUMP_WRITE(, sizeof(en));
- DUMP_WRITE(men->name, en.n_namesz);
-
- /* XXX - cast from long long to long to avoid need for libgcc.a */
- bufp = (char*) roundup((unsigned long)bufp,4);
- DUMP_WRITE(men->data, men->datasz);
- bufp = (char*) roundup((unsigned long)bufp,4);
-
-#undef DUMP_WRITE
-
- return bufp;
-} /* end storenote() */
-
-/*
- * store an ELF coredump header in the supplied buffer
- * nphdr is the number of elf_phdr to insert
- */
-static void elf_kcore_store_hdr(char *bufp, int nphdr, int dataoff)
-{
- struct elf_prstatus prstatus; /* NT_PRSTATUS */
- struct elf_prpsinfo prpsinfo; /* NT_PRPSINFO */
- struct elf_phdr *nhdr, *phdr;
- struct elfhdr *elf;
- struct memelfnote notes[3];
- off_t offset = 0;
-
[PATCH v4 9/9] proc/kcore: add vmcoreinfo note to /proc/kcore
From: Omar Sandoval
The vmcoreinfo information is useful for runtime debugging tools, not
just for crash dumps. A lot of this information can be determined by
other means, but this is much more convenient, and it only adds a page
at most to the file.
Signed-off-by: Omar Sandoval
---
fs/proc/Kconfig| 1 +
fs/proc/kcore.c| 18 --
include/linux/crash_core.h | 2 ++
kernel/crash_core.c| 4 ++--
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 0eaeb41453f5..817c02b13b1d 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -31,6 +31,7 @@ config PROC_FS
config PROC_KCORE
bool "/proc/kcore support" if !ARM
depends on PROC_FS && MMU
+ select CRASH_CORE
help
Provides a virtual ELF core file of the live kernel. This can
be read with gdb and other ELF tools. No modifications can be
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 758c14e46a44..80464432dfe6 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -10,6 +10,7 @@
* Safe accesses to vmalloc/direct-mapped discontiguous areas, Kanoj
Sarcar
*/
+#include
#include
#include
#include
@@ -81,10 +82,13 @@ static size_t get_kcore_size(int *nphdr, size_t *phdrs_len,
size_t *notes_len,
}
*phdrs_len = *nphdr * sizeof(struct elf_phdr);
- *notes_len = (3 * (sizeof(struct elf_note) + ALIGN(sizeof(CORE_STR),
4)) +
+ *notes_len = (4 * sizeof(struct elf_note) +
+ 3 * ALIGN(sizeof(CORE_STR), 4) +
+ VMCOREINFO_NOTE_NAME_BYTES +
ALIGN(sizeof(struct elf_prstatus), 4) +
ALIGN(sizeof(struct elf_prpsinfo), 4) +
- ALIGN(arch_task_struct_size, 4));
+ ALIGN(arch_task_struct_size, 4) +
+ ALIGN(vmcoreinfo_size, 4));
*data_offset = PAGE_ALIGN(sizeof(struct elfhdr) + *phdrs_len +
*notes_len);
return *data_offset + size;
@@ -406,6 +410,16 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
sizeof(prpsinfo));
append_kcore_note(notes, , CORE_STR, NT_TASKSTRUCT, current,
arch_task_struct_size);
+ /*
+* vmcoreinfo_size is mostly constant after init time, but it
+* can be changed by crash_save_vmcoreinfo(). Racing here with a
+* panic on another CPU before the machine goes down is insanely
+* unlikely, but it's better to not leave potential buffer
+* overflows lying around, regardless.
+*/
+ append_kcore_note(notes, , VMCOREINFO_NOTE_NAME, 0,
+ vmcoreinfo_data,
+ min(vmcoreinfo_size, notes_len - i));
tsz = min_t(size_t, buflen, notes_offset + notes_len - *fpos);
if (copy_to_user(buffer, notes + *fpos - notes_offset, tsz)) {
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index b511f6d24b42..525510a9f965 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -60,6 +60,8 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_CONFIG(name) \
vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
+extern unsigned char *vmcoreinfo_data;
+extern size_t vmcoreinfo_size;
extern u32 *vmcoreinfo_note;
Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index e7b4025c7b24..a683bfb0530f 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -14,8 +14,8 @@
#include
/* vmcoreinfo stuff */
-static unsigned char *vmcoreinfo_data;
-static size_t vmcoreinfo_size;
+unsigned char *vmcoreinfo_data;
+size_t vmcoreinfo_size;
u32 *vmcoreinfo_note;
/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
--
2.18.0
[PATCH v4 9/9] proc/kcore: add vmcoreinfo note to /proc/kcore
From: Omar Sandoval
The vmcoreinfo information is useful for runtime debugging tools, not
just for crash dumps. A lot of this information can be determined by
other means, but this is much more convenient, and it only adds a page
at most to the file.
Signed-off-by: Omar Sandoval
---
fs/proc/Kconfig| 1 +
fs/proc/kcore.c| 18 --
include/linux/crash_core.h | 2 ++
kernel/crash_core.c| 4 ++--
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 0eaeb41453f5..817c02b13b1d 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -31,6 +31,7 @@ config PROC_FS
config PROC_KCORE
bool "/proc/kcore support" if !ARM
depends on PROC_FS && MMU
+ select CRASH_CORE
help
Provides a virtual ELF core file of the live kernel. This can
be read with gdb and other ELF tools. No modifications can be
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 758c14e46a44..80464432dfe6 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -10,6 +10,7 @@
* Safe accesses to vmalloc/direct-mapped discontiguous areas, Kanoj
Sarcar
*/
+#include
#include
#include
#include
@@ -81,10 +82,13 @@ static size_t get_kcore_size(int *nphdr, size_t *phdrs_len,
size_t *notes_len,
}
*phdrs_len = *nphdr * sizeof(struct elf_phdr);
- *notes_len = (3 * (sizeof(struct elf_note) + ALIGN(sizeof(CORE_STR),
4)) +
+ *notes_len = (4 * sizeof(struct elf_note) +
+ 3 * ALIGN(sizeof(CORE_STR), 4) +
+ VMCOREINFO_NOTE_NAME_BYTES +
ALIGN(sizeof(struct elf_prstatus), 4) +
ALIGN(sizeof(struct elf_prpsinfo), 4) +
- ALIGN(arch_task_struct_size, 4));
+ ALIGN(arch_task_struct_size, 4) +
+ ALIGN(vmcoreinfo_size, 4));
*data_offset = PAGE_ALIGN(sizeof(struct elfhdr) + *phdrs_len +
*notes_len);
return *data_offset + size;
@@ -406,6 +410,16 @@ read_kcore(struct file *file, char __user *buffer, size_t
buflen, loff_t *fpos)
sizeof(prpsinfo));
append_kcore_note(notes, , CORE_STR, NT_TASKSTRUCT, current,
arch_task_struct_size);
+ /*
+* vmcoreinfo_size is mostly constant after init time, but it
+* can be changed by crash_save_vmcoreinfo(). Racing here with a
+* panic on another CPU before the machine goes down is insanely
+* unlikely, but it's better to not leave potential buffer
+* overflows lying around, regardless.
+*/
+ append_kcore_note(notes, , VMCOREINFO_NOTE_NAME, 0,
+ vmcoreinfo_data,
+ min(vmcoreinfo_size, notes_len - i));
tsz = min_t(size_t, buflen, notes_offset + notes_len - *fpos);
if (copy_to_user(buffer, notes + *fpos - notes_offset, tsz)) {
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index b511f6d24b42..525510a9f965 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -60,6 +60,8 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_CONFIG(name) \
vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
+extern unsigned char *vmcoreinfo_data;
+extern size_t vmcoreinfo_size;
extern u32 *vmcoreinfo_note;
Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index e7b4025c7b24..a683bfb0530f 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -14,8 +14,8 @@
#include
/* vmcoreinfo stuff */
-static unsigned char *vmcoreinfo_data;
-static size_t vmcoreinfo_size;
+unsigned char *vmcoreinfo_data;
+size_t vmcoreinfo_size;
u32 *vmcoreinfo_note;
/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
--
2.18.0
[PATCH v4 0/9] /proc/kcore improvements
From: Omar Sandoval Hi, This series makes a few improvements to /proc/kcore. It fixes a couple of small issues in v3 but is otherwise the same. Patches 1, 2, and 3 are prep patches. Patch 4 is a fix/cleanup. Patch 5 is another prep patch. Patches 6 and 7 are optimizations to ->read(). Patch 8 makes it possible to enable CRASH_CORE on any architecture, which is needed for patch 9. Patch 9 adds vmcoreinfo to /proc/kcore. Based on v4.18-rc6 + James' patch in the mm tree, and tested with crash and readelf. Thanks! Changes from v3: - Fixes a mixed up up_write() instead of up_read() in patch 5 reported by Tetsuo Handa - Added patch 8 to fix a build failure reported by Stephen Rothwell Changes from v2: - Add __init to kclist_add() as per Andrew - Got rid of conversion of kcore_need_update from int to atomic_t and just used xchg() instead of atomic_cmpxchg() (split out into a new patch instead of combining it with the rwlock -> rwsem conversion) - Add comment about the increase in file size to patch 8 Changes from v1: - Rebased onto v4.18-rc4 + James' patch (https://patchwork.kernel.org/patch/10519739/) in the mm tree - Fix spurious sparse warning (see the report and response in https://patchwork.kernel.org/patch/10512431/) Omar Sandoval (9): proc/kcore: don't grab lock for kclist_add() proc/kcore: don't grab lock for memory hotplug notifier proc/kcore: replace kclist_lock rwlock with rwsem proc/kcore: fix memory hotplug vs multiple opens race proc/kcore: hold lock during read proc/kcore: clean up ELF header generation proc/kcore: optimize multiple page reads crash_core: use VMCOREINFO_SYMBOL_ARRAY() for swapper_pg_dir proc/kcore: add vmcoreinfo note to /proc/kcore fs/proc/Kconfig| 1 + fs/proc/kcore.c| 534 + include/linux/crash_core.h | 2 + include/linux/kcore.h | 2 +- kernel/crash_core.c| 6 +- 5 files changed, 252 insertions(+), 293 deletions(-) -- 2.18.0
[PATCH v4 8/9] crash_core: use VMCOREINFO_SYMBOL_ARRAY() for swapper_pg_dir
From: Omar Sandoval
This is preparation for allowing CRASH_CORE to be enabled for any
architecture.
swapper_pg_dir is always either an array or a macro expanding to NULL.
In the latter case, VMCOREINFO_SYMBOL() won't work, as it tries to take
the address of the given symbol:
#define VMCOREINFO_SYMBOL(name) \
vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
long))
Instead, use VMCOREINFO_SYMBOL_ARRAY(), which uses the value:
#define VMCOREINFO_SYMBOL_ARRAY(name) \
vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
long)name)
This is the same thing for the array case but isn't an error for the
macro case.
Signed-off-by: Omar Sandoval
---
kernel/crash_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index b66aced5e8c2..e7b4025c7b24 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -401,7 +401,7 @@ static int __init crash_save_vmcoreinfo_init(void)
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_SYMBOL(node_online_map);
#ifdef CONFIG_MMU
- VMCOREINFO_SYMBOL(swapper_pg_dir);
+ VMCOREINFO_SYMBOL_ARRAY(swapper_pg_dir);
#endif
VMCOREINFO_SYMBOL(_stext);
VMCOREINFO_SYMBOL(vmap_area_list);
--
2.18.0
[PATCH v4 4/9] proc/kcore: fix memory hotplug vs multiple opens race
From: Omar Sandoval
There's a theoretical race condition that will cause /proc/kcore to miss
a memory hotplug event:
CPU0 CPU1
// hotplug event 1
kcore_need_update = 1
open_kcore() open_kcore()
kcore_update_ram()kcore_update_ram()
// Walk RAM // Walk RAM
__kcore_update_ram() __kcore_update_ram()
kcore_need_update = 0
// hotplug event 2
kcore_need_update = 1
kcore_need_update = 0
Note that CPU1 set up the RAM kcore entries with the state after hotplug
event 1 but cleared the flag for hotplug event 2. The RAM entries will
therefore be stale until there is another hotplug event.
This is an extremely unlikely sequence of events, but the fix makes the
synchronization saner, anyways: we serialize the entire update sequence,
which means that whoever clears the flag will always succeed in
replacing the kcore list.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 93 +++--
1 file changed, 44 insertions(+), 49 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index ae43a97d511d..95aa988c5b5d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -98,53 +98,15 @@ static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
return size + *elf_buflen;
}
-static void free_kclist_ents(struct list_head *head)
-{
- struct kcore_list *tmp, *pos;
-
- list_for_each_entry_safe(pos, tmp, head, list) {
- list_del(>list);
- kfree(pos);
- }
-}
-/*
- * Replace all KCORE_RAM/KCORE_VMEMMAP information with passed list.
- */
-static void __kcore_update_ram(struct list_head *list)
-{
- int nphdr;
- size_t size;
- struct kcore_list *tmp, *pos;
- LIST_HEAD(garbage);
-
- down_write(_lock);
- if (xchg(_need_update, 0)) {
- list_for_each_entry_safe(pos, tmp, _head, list) {
- if (pos->type == KCORE_RAM
- || pos->type == KCORE_VMEMMAP)
- list_move(>list, );
- }
- list_splice_tail(list, _head);
- } else
- list_splice(list, );
- proc_root_kcore->size = get_kcore_size(, );
- up_write(_lock);
-
- free_kclist_ents();
-}
-
-
#ifdef CONFIG_HIGHMEM
/*
* If no highmem, we can assume [0...max_low_pfn) continuous range of memory
* because memory hole is not as big as !HIGHMEM case.
* (HIGHMEM is special because part of memory is _invisible_ from the kernel.)
*/
-static int kcore_update_ram(void)
+static int kcore_ram_list(struct list_head *head)
{
- LIST_HEAD(head);
struct kcore_list *ent;
- int ret = 0;
ent = kmalloc(sizeof(*ent), GFP_KERNEL);
if (!ent)
@@ -152,9 +114,8 @@ static int kcore_update_ram(void)
ent->addr = (unsigned long)__va(0);
ent->size = max_low_pfn << PAGE_SHIFT;
ent->type = KCORE_RAM;
- list_add(>list, );
- __kcore_update_ram();
- return ret;
+ list_add(>list, head);
+ return 0;
}
#else /* !CONFIG_HIGHMEM */
@@ -253,11 +214,10 @@ kclist_add_private(unsigned long pfn, unsigned long
nr_pages, void *arg)
return 1;
}
-static int kcore_update_ram(void)
+static int kcore_ram_list(struct list_head *list)
{
int nid, ret;
unsigned long end_pfn;
- LIST_HEAD(head);
/* Not inializedupdate now */
/* find out "max pfn" */
@@ -269,15 +229,50 @@ static int kcore_update_ram(void)
end_pfn = node_end;
}
/* scan 0 to max_pfn */
- ret = walk_system_ram_range(0, end_pfn, , kclist_add_private);
- if (ret) {
- free_kclist_ents();
+ ret = walk_system_ram_range(0, end_pfn, list, kclist_add_private);
+ if (ret)
return -ENOMEM;
+ return 0;
+}
+#endif /* CONFIG_HIGHMEM */
+
+static int kcore_update_ram(void)
+{
+ LIST_HEAD(list);
+ LIST_HEAD(garbage);
+ int nphdr;
+ size_t size;
+ struct kcore_list *tmp, *pos;
+ int ret = 0;
+
+ down_write(_lock);
+ if (!xchg(_need_update, 0))
+ goto out;
+
+ ret = kcore_ram_list();
+ if (ret) {
+ /* Couldn't get the RAM list, try again next time. */
+ WRITE_ONCE(kcore_need_update, 1);
+ list_splice_tail(, );
+ goto out;
+ }
+
+ list_for_each_entry_safe(pos, tmp, _head, list) {
+ if (pos->type == KCORE_RAM || pos->type == KCORE_VMEMMAP)
+ list_move(>list, );
+ }
+ list_splice_tail(, _head);
+
+ proc_root_kcore->size = get_kcore_size(, );
+
+out:
+ up_write(_lock);
+ list_for_each_entry_safe(pos, tmp, , list) {
+ list_del(>list);
+ kfree(pos);
[PATCH v4 0/9] /proc/kcore improvements
From: Omar Sandoval Hi, This series makes a few improvements to /proc/kcore. It fixes a couple of small issues in v3 but is otherwise the same. Patches 1, 2, and 3 are prep patches. Patch 4 is a fix/cleanup. Patch 5 is another prep patch. Patches 6 and 7 are optimizations to ->read(). Patch 8 makes it possible to enable CRASH_CORE on any architecture, which is needed for patch 9. Patch 9 adds vmcoreinfo to /proc/kcore. Based on v4.18-rc6 + James' patch in the mm tree, and tested with crash and readelf. Thanks! Changes from v3: - Fixes a mixed up up_write() instead of up_read() in patch 5 reported by Tetsuo Handa - Added patch 8 to fix a build failure reported by Stephen Rothwell Changes from v2: - Add __init to kclist_add() as per Andrew - Got rid of conversion of kcore_need_update from int to atomic_t and just used xchg() instead of atomic_cmpxchg() (split out into a new patch instead of combining it with the rwlock -> rwsem conversion) - Add comment about the increase in file size to patch 8 Changes from v1: - Rebased onto v4.18-rc4 + James' patch (https://patchwork.kernel.org/patch/10519739/) in the mm tree - Fix spurious sparse warning (see the report and response in https://patchwork.kernel.org/patch/10512431/) Omar Sandoval (9): proc/kcore: don't grab lock for kclist_add() proc/kcore: don't grab lock for memory hotplug notifier proc/kcore: replace kclist_lock rwlock with rwsem proc/kcore: fix memory hotplug vs multiple opens race proc/kcore: hold lock during read proc/kcore: clean up ELF header generation proc/kcore: optimize multiple page reads crash_core: use VMCOREINFO_SYMBOL_ARRAY() for swapper_pg_dir proc/kcore: add vmcoreinfo note to /proc/kcore fs/proc/Kconfig| 1 + fs/proc/kcore.c| 534 + include/linux/crash_core.h | 2 + include/linux/kcore.h | 2 +- kernel/crash_core.c| 6 +- 5 files changed, 252 insertions(+), 293 deletions(-) -- 2.18.0
[PATCH v4 8/9] crash_core: use VMCOREINFO_SYMBOL_ARRAY() for swapper_pg_dir
From: Omar Sandoval
This is preparation for allowing CRASH_CORE to be enabled for any
architecture.
swapper_pg_dir is always either an array or a macro expanding to NULL.
In the latter case, VMCOREINFO_SYMBOL() won't work, as it tries to take
the address of the given symbol:
#define VMCOREINFO_SYMBOL(name) \
vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
long))
Instead, use VMCOREINFO_SYMBOL_ARRAY(), which uses the value:
#define VMCOREINFO_SYMBOL_ARRAY(name) \
vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
long)name)
This is the same thing for the array case but isn't an error for the
macro case.
Signed-off-by: Omar Sandoval
---
kernel/crash_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index b66aced5e8c2..e7b4025c7b24 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -401,7 +401,7 @@ static int __init crash_save_vmcoreinfo_init(void)
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_SYMBOL(node_online_map);
#ifdef CONFIG_MMU
- VMCOREINFO_SYMBOL(swapper_pg_dir);
+ VMCOREINFO_SYMBOL_ARRAY(swapper_pg_dir);
#endif
VMCOREINFO_SYMBOL(_stext);
VMCOREINFO_SYMBOL(vmap_area_list);
--
2.18.0
[PATCH v4 4/9] proc/kcore: fix memory hotplug vs multiple opens race
From: Omar Sandoval
There's a theoretical race condition that will cause /proc/kcore to miss
a memory hotplug event:
CPU0 CPU1
// hotplug event 1
kcore_need_update = 1
open_kcore() open_kcore()
kcore_update_ram()kcore_update_ram()
// Walk RAM // Walk RAM
__kcore_update_ram() __kcore_update_ram()
kcore_need_update = 0
// hotplug event 2
kcore_need_update = 1
kcore_need_update = 0
Note that CPU1 set up the RAM kcore entries with the state after hotplug
event 1 but cleared the flag for hotplug event 2. The RAM entries will
therefore be stale until there is another hotplug event.
This is an extremely unlikely sequence of events, but the fix makes the
synchronization saner, anyways: we serialize the entire update sequence,
which means that whoever clears the flag will always succeed in
replacing the kcore list.
Signed-off-by: Omar Sandoval
---
fs/proc/kcore.c | 93 +++--
1 file changed, 44 insertions(+), 49 deletions(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index ae43a97d511d..95aa988c5b5d 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -98,53 +98,15 @@ static size_t get_kcore_size(int *nphdr, size_t *elf_buflen)
return size + *elf_buflen;
}
-static void free_kclist_ents(struct list_head *head)
-{
- struct kcore_list *tmp, *pos;
-
- list_for_each_entry_safe(pos, tmp, head, list) {
- list_del(>list);
- kfree(pos);
- }
-}
-/*
- * Replace all KCORE_RAM/KCORE_VMEMMAP information with passed list.
- */
-static void __kcore_update_ram(struct list_head *list)
-{
- int nphdr;
- size_t size;
- struct kcore_list *tmp, *pos;
- LIST_HEAD(garbage);
-
- down_write(_lock);
- if (xchg(_need_update, 0)) {
- list_for_each_entry_safe(pos, tmp, _head, list) {
- if (pos->type == KCORE_RAM
- || pos->type == KCORE_VMEMMAP)
- list_move(>list, );
- }
- list_splice_tail(list, _head);
- } else
- list_splice(list, );
- proc_root_kcore->size = get_kcore_size(, );
- up_write(_lock);
-
- free_kclist_ents();
-}
-
-
#ifdef CONFIG_HIGHMEM
/*
* If no highmem, we can assume [0...max_low_pfn) continuous range of memory
* because memory hole is not as big as !HIGHMEM case.
* (HIGHMEM is special because part of memory is _invisible_ from the kernel.)
*/
-static int kcore_update_ram(void)
+static int kcore_ram_list(struct list_head *head)
{
- LIST_HEAD(head);
struct kcore_list *ent;
- int ret = 0;
ent = kmalloc(sizeof(*ent), GFP_KERNEL);
if (!ent)
@@ -152,9 +114,8 @@ static int kcore_update_ram(void)
ent->addr = (unsigned long)__va(0);
ent->size = max_low_pfn << PAGE_SHIFT;
ent->type = KCORE_RAM;
- list_add(>list, );
- __kcore_update_ram();
- return ret;
+ list_add(>list, head);
+ return 0;
}
#else /* !CONFIG_HIGHMEM */
@@ -253,11 +214,10 @@ kclist_add_private(unsigned long pfn, unsigned long
nr_pages, void *arg)
return 1;
}
-static int kcore_update_ram(void)
+static int kcore_ram_list(struct list_head *list)
{
int nid, ret;
unsigned long end_pfn;
- LIST_HEAD(head);
/* Not inializedupdate now */
/* find out "max pfn" */
@@ -269,15 +229,50 @@ static int kcore_update_ram(void)
end_pfn = node_end;
}
/* scan 0 to max_pfn */
- ret = walk_system_ram_range(0, end_pfn, , kclist_add_private);
- if (ret) {
- free_kclist_ents();
+ ret = walk_system_ram_range(0, end_pfn, list, kclist_add_private);
+ if (ret)
return -ENOMEM;
+ return 0;
+}
+#endif /* CONFIG_HIGHMEM */
+
+static int kcore_update_ram(void)
+{
+ LIST_HEAD(list);
+ LIST_HEAD(garbage);
+ int nphdr;
+ size_t size;
+ struct kcore_list *tmp, *pos;
+ int ret = 0;
+
+ down_write(_lock);
+ if (!xchg(_need_update, 0))
+ goto out;
+
+ ret = kcore_ram_list();
+ if (ret) {
+ /* Couldn't get the RAM list, try again next time. */
+ WRITE_ONCE(kcore_need_update, 1);
+ list_splice_tail(, );
+ goto out;
+ }
+
+ list_for_each_entry_safe(pos, tmp, _head, list) {
+ if (pos->type == KCORE_RAM || pos->type == KCORE_VMEMMAP)
+ list_move(>list, );
+ }
+ list_splice_tail(, _head);
+
+ proc_root_kcore->size = get_kcore_size(, );
+
+out:
+ up_write(_lock);
+ list_for_each_entry_safe(pos, tmp, , list) {
+ list_del(>list);
+ kfree(pos);
Re: [PATCH v1 3/3] thermal: tsens: Fix negative temperature reporting
On Wed, Jul 18, 2018 at 12:55:03PM +0530, Amit Kucheria wrote:
> The current code will always return 0x in case of negative
> temperatures due to a bug in how the binary sign extension is being done.
>
> Use sign_extend32() instead.
>
> Signed-off-by: Amit Kucheria
> ---
> drivers/thermal/qcom/tsens-v2.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 908e3dc..46abc21 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -5,19 +5,20 @@
> */
>
> #include
> +#include
> #include "tsens.h"
>
> #define STATUS_OFFSET0xa0
> #define LAST_TEMP_MASK 0xfff
> #define STATUS_VALID_BIT BIT(21)
> -#define CODE_SIGN_BITBIT(11)
>
> static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
> {
> struct tsens_sensor *s = >sensor[id];
> u32 code;
> unsigned int status_reg;
> - int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
> + u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
> + int ret;
>
> status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> ret = regmap_read(tmdev->map, status_reg, );
> @@ -54,12 +55,8 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev,
> int id, int *temp)
> else if (last_temp2 == last_temp3)
> last_temp = last_temp3;
> done:
> - /* Code sign bit is the sign extension for a negative value */
> - if (last_temp & CODE_SIGN_BIT)
> - last_temp |= ~CODE_SIGN_BIT;
> -
> - /* Temperatures are in deciCelicius */
> - *temp = last_temp * 100;
> + /* Convert temperatures to milliCelicius */
nits:
s/temperatures/temperature/
s/milliCelicius/milliCelsius/
> + *temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
>
> return 0;
> }
Reviewed-by: Matthias Kaehlcke
Re: [PATCH v1 3/3] thermal: tsens: Fix negative temperature reporting
On Wed, Jul 18, 2018 at 12:55:03PM +0530, Amit Kucheria wrote:
> The current code will always return 0x in case of negative
> temperatures due to a bug in how the binary sign extension is being done.
>
> Use sign_extend32() instead.
>
> Signed-off-by: Amit Kucheria
> ---
> drivers/thermal/qcom/tsens-v2.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 908e3dc..46abc21 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -5,19 +5,20 @@
> */
>
> #include
> +#include
> #include "tsens.h"
>
> #define STATUS_OFFSET0xa0
> #define LAST_TEMP_MASK 0xfff
> #define STATUS_VALID_BIT BIT(21)
> -#define CODE_SIGN_BITBIT(11)
>
> static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
> {
> struct tsens_sensor *s = >sensor[id];
> u32 code;
> unsigned int status_reg;
> - int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
> + u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
> + int ret;
>
> status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> ret = regmap_read(tmdev->map, status_reg, );
> @@ -54,12 +55,8 @@ static int get_temp_tsens_v2(struct tsens_device *tmdev,
> int id, int *temp)
> else if (last_temp2 == last_temp3)
> last_temp = last_temp3;
> done:
> - /* Code sign bit is the sign extension for a negative value */
> - if (last_temp & CODE_SIGN_BIT)
> - last_temp |= ~CODE_SIGN_BIT;
> -
> - /* Temperatures are in deciCelicius */
> - *temp = last_temp * 100;
> + /* Convert temperatures to milliCelicius */
nits:
s/temperatures/temperature/
s/milliCelicius/milliCelsius/
> + *temp = sign_extend32(last_temp, fls(LAST_TEMP_MASK) - 1) * 100;
>
> return 0;
> }
Reviewed-by: Matthias Kaehlcke
Re: [PATCH 1/3] clk: s2mps11: Fix matching when built as module and DT node contains compatible
Quoting Krzysztof Kozlowski (2018-07-25 08:55:15)
> When driver is built as module and DT node contains clocks compatible
> (e.g. "samsung,s2mps11-clk"), the module will not be autoloaded because
> module aliases won't match.
>
> The modalias from uevent: of:NclocksTCsamsung,s2mps11-clk
> The modalias from driver: platform:s2mps11-clk
>
> The devices are instantiated by parent's MFD. However both Device Tree
> bindings and parent define the compatible for clocks devices. In case
> of module matching this DT compatible will be used.
>
> The issue will not happen if this is built-in (no need for module
> matching) or when clocks DT node does not contain compatible (not
> correct from bindings perspective but working for driver).
>
> Note when backporting to stable kernels: adjust the list of device ID
> entries.
>
> Cc:
> Fixes: 53c31b3437a6 ("mfd: sec-core: Add of_compatible strings for clock MFD
> cells")
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Stephen Boyd
Re: [PATCH 1/3] clk: s2mps11: Fix matching when built as module and DT node contains compatible
Quoting Krzysztof Kozlowski (2018-07-25 08:55:15)
> When driver is built as module and DT node contains clocks compatible
> (e.g. "samsung,s2mps11-clk"), the module will not be autoloaded because
> module aliases won't match.
>
> The modalias from uevent: of:NclocksTCsamsung,s2mps11-clk
> The modalias from driver: platform:s2mps11-clk
>
> The devices are instantiated by parent's MFD. However both Device Tree
> bindings and parent define the compatible for clocks devices. In case
> of module matching this DT compatible will be used.
>
> The issue will not happen if this is built-in (no need for module
> matching) or when clocks DT node does not contain compatible (not
> correct from bindings perspective but working for driver).
>
> Note when backporting to stable kernels: adjust the list of device ID
> entries.
>
> Cc:
> Fixes: 53c31b3437a6 ("mfd: sec-core: Add of_compatible strings for clock MFD
> cells")
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Stephen Boyd
Re: [PATCH] fsi: master-ast-cf: Fix memory leak
On Wed, 2018-07-25 at 08:38 -0500, Gustavo A. R. Silva wrote:
> In case memory resources for *fw* were allocated, release them
> before return.
>
> Addresses-Coverity-ID: 1472044 ("Resource leak")
> Fixes: 6a794a27daca ("fsi: master-ast-cf: Add new FSI master using Aspeed
> ColdFire")
> Signed-off-by: Gustavo A. R. Silva
Thanks Gustavo !
I'll apply before I send my next pull request to Greg !
Cheers,
Ben.
> ---
> drivers/fsi/fsi-master-ast-cf.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-ast-cf.c b/drivers/fsi/fsi-master-ast-cf.c
> index c4a3557..e39a4630 100644
> --- a/drivers/fsi/fsi-master-ast-cf.c
> +++ b/drivers/fsi/fsi-master-ast-cf.c
> @@ -861,7 +861,8 @@ static int load_copro_firmware(struct fsi_master_acf
> *master)
> if (sig != wanted_sig) {
> dev_err(master->dev, "Failed to locate image sig %04x in FW
> blob\n",
> wanted_sig);
> - return -ENODEV;
> + rc = -ENODEV;
> + goto release_fw;
> }
> if (size > master->cf_mem_size) {
> dev_err(master->dev, "FW size (%zd) bigger than memory reserve
> (%zd)\n",
> @@ -870,8 +871,9 @@ static int load_copro_firmware(struct fsi_master_acf
> *master)
> } else {
> memcpy_toio(master->cf_mem, data, size);
> }
> - release_firmware(fw);
>
> +release_fw:
> + release_firmware(fw);
> return rc;
> }
>
Re: [PATCH] fsi: master-ast-cf: Fix memory leak
On Wed, 2018-07-25 at 08:38 -0500, Gustavo A. R. Silva wrote:
> In case memory resources for *fw* were allocated, release them
> before return.
>
> Addresses-Coverity-ID: 1472044 ("Resource leak")
> Fixes: 6a794a27daca ("fsi: master-ast-cf: Add new FSI master using Aspeed
> ColdFire")
> Signed-off-by: Gustavo A. R. Silva
Thanks Gustavo !
I'll apply before I send my next pull request to Greg !
Cheers,
Ben.
> ---
> drivers/fsi/fsi-master-ast-cf.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-ast-cf.c b/drivers/fsi/fsi-master-ast-cf.c
> index c4a3557..e39a4630 100644
> --- a/drivers/fsi/fsi-master-ast-cf.c
> +++ b/drivers/fsi/fsi-master-ast-cf.c
> @@ -861,7 +861,8 @@ static int load_copro_firmware(struct fsi_master_acf
> *master)
> if (sig != wanted_sig) {
> dev_err(master->dev, "Failed to locate image sig %04x in FW
> blob\n",
> wanted_sig);
> - return -ENODEV;
> + rc = -ENODEV;
> + goto release_fw;
> }
> if (size > master->cf_mem_size) {
> dev_err(master->dev, "FW size (%zd) bigger than memory reserve
> (%zd)\n",
> @@ -870,8 +871,9 @@ static int load_copro_firmware(struct fsi_master_acf
> *master)
> } else {
> memcpy_toio(master->cf_mem, data, size);
> }
> - release_firmware(fw);
>
> +release_fw:
> + release_firmware(fw);
> return rc;
> }
>
Re: [PATCH] reset: hisilicon: fix potential NULL pointer dereference
Quoting Gustavo A. R. Silva (2018-07-18 18:58:45)
> There is a potential execution path in which function
> platform_get_resource() returns NULL. If this happens,
> we will end up having a NULL pointer dereference.
>
> Fix this by adding asanity check in order to avoid a
> NULL pointer dereference.
>
> This code was detected with the help of Coccinelle.
>
> Cc: sta...@vger.kernel.org
> Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
> hisi_reset_init")
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/clk/hisilicon/reset.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 2a5015c..5dfb48b 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -109,6 +109,9 @@ struct hisi_reset_controller *hisi_reset_init(struct
> platform_device *pdev)
> return NULL;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return NULL;
> +
> rstc->membase = devm_ioremap(>dev,
> res->start, resource_size(res));
Why can't we use devm_ioremap_resource() here instead?
Re: [PATCH] reset: hisilicon: fix potential NULL pointer dereference
Quoting Gustavo A. R. Silva (2018-07-18 18:58:45)
> There is a potential execution path in which function
> platform_get_resource() returns NULL. If this happens,
> we will end up having a NULL pointer dereference.
>
> Fix this by adding asanity check in order to avoid a
> NULL pointer dereference.
>
> This code was detected with the help of Coccinelle.
>
> Cc: sta...@vger.kernel.org
> Fixes: 97b7129cd2af ("reset: hisilicon: change the definition of
> hisi_reset_init")
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/clk/hisilicon/reset.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 2a5015c..5dfb48b 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -109,6 +109,9 @@ struct hisi_reset_controller *hisi_reset_init(struct
> platform_device *pdev)
> return NULL;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return NULL;
> +
> rstc->membase = devm_ioremap(>dev,
> res->start, resource_size(res));
Why can't we use devm_ioremap_resource() here instead?
Re: [PATCH 15/15] clk: pistachio: Fix wrong SDHost card speed
Quoting Andreas Färber (2018-07-22 14:20:10) > From: Govindraj Raja > > The SDHost currently clocks the card 4x slower than it > should do, because there is a fixed divide by 4 in the > sdhost wrapper that is not present in the clock tree. > To model this, add a fixed divide by 4 clock node in > the SDHost clock path. > > This will ensure the right clock frequency is selected when > the mmc driver tries to configure frequency on card insert. > > Signed-off-by: Govindraj Raja > Signed-off-by: Andreas Färber > --- Acked-by: Stephen Boyd
Re: [PATCH 15/15] clk: pistachio: Fix wrong SDHost card speed
Quoting Andreas Färber (2018-07-22 14:20:10) > From: Govindraj Raja > > The SDHost currently clocks the card 4x slower than it > should do, because there is a fixed divide by 4 in the > sdhost wrapper that is not present in the clock tree. > To model this, add a fixed divide by 4 clock node in > the SDHost clock path. > > This will ensure the right clock frequency is selected when > the mmc driver tries to configure frequency on card insert. > > Signed-off-by: Govindraj Raja > Signed-off-by: Andreas Färber > --- Acked-by: Stephen Boyd
Re: [PATCH] clk: tegra: probe deferral error reporting
On 7/25/2018 7:24 PM, Stephen Boyd wrote:
Quoting Marcel Ziswiler (2018-07-20 00:54:22)
From: Marcel Ziswiler
Actually report the error code from devm_regulator_get() which may as
well just be a probe deferral.
Signed-off-by: Marcel Ziswiler
---
drivers/clk/tegra/clk-dfll.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
index 48ee43734e05..b2123084e175 100644
--- a/drivers/clk/tegra/clk-dfll.c
+++ b/drivers/clk/tegra/clk-dfll.c
@@ -1609,8 +1609,9 @@ int tegra_dfll_register(struct platform_device *pdev,
td->vdd_reg = devm_regulator_get(td->dev, "vdd-cpu");
if (IS_ERR(td->vdd_reg)) {
- dev_err(td->dev, "couldn't get vdd_cpu regulator\n");
- return PTR_ERR(td->vdd_reg);
+ ret = PTR_ERR(td->vdd_reg);
+ dev_err(td->dev, "couldn't get vdd_cpu regulator: %d\n", ret);
Do you want to know that a probe defer is happening? Usually patches are
sent to make that error path silent.
Just asking as the newbie here, but shouldn't probe deferral be
regulated to dev_debug?
Then pass any other error code as dev_err.
Re: [PATCH] clk: tegra: probe deferral error reporting
On 7/25/2018 7:24 PM, Stephen Boyd wrote:
Quoting Marcel Ziswiler (2018-07-20 00:54:22)
From: Marcel Ziswiler
Actually report the error code from devm_regulator_get() which may as
well just be a probe deferral.
Signed-off-by: Marcel Ziswiler
---
drivers/clk/tegra/clk-dfll.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
index 48ee43734e05..b2123084e175 100644
--- a/drivers/clk/tegra/clk-dfll.c
+++ b/drivers/clk/tegra/clk-dfll.c
@@ -1609,8 +1609,9 @@ int tegra_dfll_register(struct platform_device *pdev,
td->vdd_reg = devm_regulator_get(td->dev, "vdd-cpu");
if (IS_ERR(td->vdd_reg)) {
- dev_err(td->dev, "couldn't get vdd_cpu regulator\n");
- return PTR_ERR(td->vdd_reg);
+ ret = PTR_ERR(td->vdd_reg);
+ dev_err(td->dev, "couldn't get vdd_cpu regulator: %d\n", ret);
Do you want to know that a probe defer is happening? Usually patches are
sent to make that error path silent.
Just asking as the newbie here, but shouldn't probe deferral be
regulated to dev_debug?
Then pass any other error code as dev_err.
Re: [PATCH v7 3/5] clk: actions: Add S700 SoC clock support
Quoting Saravanan Sekar (2018-07-19 02:06:47) > Add Actions Semi S700 SoC clock support > > Signed-off-by: Parthiban Nallathambi > Signed-off-by: Saravanan Sekar > Reviewed-by: Manivannan Sadhasivam > --- Applied to clk-next
Re: [PATCH v7 3/5] clk: actions: Add S700 SoC clock support
Quoting Saravanan Sekar (2018-07-19 02:06:47) > Add Actions Semi S700 SoC clock support > > Signed-off-by: Parthiban Nallathambi > Signed-off-by: Saravanan Sekar > Reviewed-by: Manivannan Sadhasivam > --- Applied to clk-next
Re: [PATCH v7 1/5] clk: actions: Add missing REGMAP_MMIO dependency
Quoting Saravanan Sekar (2018-07-19 02:06:45)
> Add REGMAP_MMIO as dependency to avoid undefined
> reference to regmap symbols.
>
> Fixes: d85d20053e19 ("clk: actions: Add S900 SoC clock support")
> Signed-off-by: Saravanan Sekar
> Reviewed-by: Andreas Färber
> Reviewed-by: Manivannan Sadhasivam
> ---
Applied to clk-next
Re: [PATCH v7 2/5] dt-bindings: clock: Add S700 support for Actions Semi Soc's
Quoting Saravanan Sekar (2018-07-19 02:06:46) > Add clock bindings constants for action S700 > Maintain common clock dt-bindings for Actions Semi SoC's > S700 and S900. > > Signed-off-by: Parthiban Nallathambi > Signed-off-by: Saravanan Sekar > Reviewed-by: Rob Herring > --- Applied to clk-next
Re: [PATCH v7 1/5] clk: actions: Add missing REGMAP_MMIO dependency
Quoting Saravanan Sekar (2018-07-19 02:06:45)
> Add REGMAP_MMIO as dependency to avoid undefined
> reference to regmap symbols.
>
> Fixes: d85d20053e19 ("clk: actions: Add S900 SoC clock support")
> Signed-off-by: Saravanan Sekar
> Reviewed-by: Andreas Färber
> Reviewed-by: Manivannan Sadhasivam
> ---
Applied to clk-next
Re: [PATCH v7 2/5] dt-bindings: clock: Add S700 support for Actions Semi Soc's
Quoting Saravanan Sekar (2018-07-19 02:06:46) > Add clock bindings constants for action S700 > Maintain common clock dt-bindings for Actions Semi SoC's > S700 and S900. > > Signed-off-by: Parthiban Nallathambi > Signed-off-by: Saravanan Sekar > Reviewed-by: Rob Herring > --- Applied to clk-next
Re: [PATCH] input: pxrc - do not store USB device in private struct
On Tue, Jul 24, 2018 at 08:09:59PM +0200, Marcus Folkesson wrote: > Hello Dmitry, > > On Tue, Jul 24, 2018 at 02:38:04AM +, Dmitry Torokhov wrote: > > Hi Marcus, > > > > On Mon, Jul 16, 2018 at 04:40:14PM +0200, Marcus Folkesson wrote: > > > The USB device is only needed during setup, so put it back after > > > initialization and do not store it in our private struct. > > > > > > Also, the USB device is a parent of USB interface so our driver > > > model rules ensure that USB device should not disappear while > > > interface device is still there. > > > So not keep a refcount on the device is safe. > > > > > > Reported-by: Alexey Khoroshilov > > > Signed-off-by: Marcus Folkesson > > > --- > > > drivers/input/joystick/pxrc.c | 22 -- > > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c > > > index 07a0dbd3ced2..46a7acb747bf 100644 > > > --- a/drivers/input/joystick/pxrc.c > > > +++ b/drivers/input/joystick/pxrc.c > > ... > > > > > @@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf, > > > return -ENOMEM; > > > > > > mutex_init(>pm_mutex); > > > - pxrc->udev = usb_get_dev(interface_to_usbdev(intf)); > > > + udev = usb_get_dev(interface_to_usbdev(intf)); > > > > There is really no need to "get" device for the probe duration, or in > > general, when you are not storing the reference to it. > > > > I posted series with an updated version of this patch plus couple more > > cleanups/fixes, and would appreciate if you could give it a spin. > > Thank you for doing this. > > I have reviewed the patchset and tested on real hardware, and it looks good > to me. > > For what it's worth: > > Reviewed-by: Marcus Folkesson > Tested-by: Marcus Folkesson > > On the whole patchset. Excellent, thank you. -- Dmitry
Re: [PATCH] input: pxrc - do not store USB device in private struct
On Tue, Jul 24, 2018 at 08:09:59PM +0200, Marcus Folkesson wrote: > Hello Dmitry, > > On Tue, Jul 24, 2018 at 02:38:04AM +, Dmitry Torokhov wrote: > > Hi Marcus, > > > > On Mon, Jul 16, 2018 at 04:40:14PM +0200, Marcus Folkesson wrote: > > > The USB device is only needed during setup, so put it back after > > > initialization and do not store it in our private struct. > > > > > > Also, the USB device is a parent of USB interface so our driver > > > model rules ensure that USB device should not disappear while > > > interface device is still there. > > > So not keep a refcount on the device is safe. > > > > > > Reported-by: Alexey Khoroshilov > > > Signed-off-by: Marcus Folkesson > > > --- > > > drivers/input/joystick/pxrc.c | 22 -- > > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c > > > index 07a0dbd3ced2..46a7acb747bf 100644 > > > --- a/drivers/input/joystick/pxrc.c > > > +++ b/drivers/input/joystick/pxrc.c > > ... > > > > > @@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf, > > > return -ENOMEM; > > > > > > mutex_init(>pm_mutex); > > > - pxrc->udev = usb_get_dev(interface_to_usbdev(intf)); > > > + udev = usb_get_dev(interface_to_usbdev(intf)); > > > > There is really no need to "get" device for the probe duration, or in > > general, when you are not storing the reference to it. > > > > I posted series with an updated version of this patch plus couple more > > cleanups/fixes, and would appreciate if you could give it a spin. > > Thank you for doing this. > > I have reviewed the patchset and tested on real hardware, and it looks good > to me. > > For what it's worth: > > Reviewed-by: Marcus Folkesson > Tested-by: Marcus Folkesson > > On the whole patchset. Excellent, thank you. -- Dmitry
Re: linux-next: Signed-off-by missing for commit in the xarray tree
On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
> Hi Matthew,
>
> Commits
>
> 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
> aaf149902c79 ("filesystem-dax: Set page->index")
>
> are missing a Signed-off-by from their committers.
Oh, hah. I assume this is an automated email?
These two commits I cherry-picked from the nvdimm tree so that XArray can
be rebased on top of it. Is there some other way I should be doing this,
like rebasing on top of the nvdimm tree?
Re: linux-next: Signed-off-by missing for commit in the xarray tree
On Thu, Jul 26, 2018 at 07:28:14AM +1000, Stephen Rothwell wrote:
> Hi Matthew,
>
> Commits
>
> 890e537e2b42 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
> aaf149902c79 ("filesystem-dax: Set page->index")
>
> are missing a Signed-off-by from their committers.
Oh, hah. I assume this is an automated email?
These two commits I cherry-picked from the nvdimm tree so that XArray can
be rebased on top of it. Is there some other way I should be doing this,
like rebasing on top of the nvdimm tree?
[PATCH] mm/vmscan: fix page_freeze_refs in comment.
page_freeze_refs has already been relplaced by page_ref_freeze, but
it is not modified in the comment.
Signed-off-by: Jiang Biao
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 03822f8..d29e207 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -744,7 +744,7 @@ static int __remove_mapping(struct address_space *mapping,
struct page *page,
refcount = 2;
if (!page_ref_freeze(page, refcount))
goto cannot_free;
- /* note: atomic_cmpxchg in page_freeze_refs provides the smp_rmb */
+ /* note: atomic_cmpxchg in page_refs_freeze provides the smp_rmb */
if (unlikely(PageDirty(page))) {
page_ref_unfreeze(page, refcount);
goto cannot_free;
--
2.7.4
[PATCH] mm/vmscan: fix page_freeze_refs in comment.
page_freeze_refs has already been relplaced by page_ref_freeze, but
it is not modified in the comment.
Signed-off-by: Jiang Biao
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 03822f8..d29e207 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -744,7 +744,7 @@ static int __remove_mapping(struct address_space *mapping,
struct page *page,
refcount = 2;
if (!page_ref_freeze(page, refcount))
goto cannot_free;
- /* note: atomic_cmpxchg in page_freeze_refs provides the smp_rmb */
+ /* note: atomic_cmpxchg in page_refs_freeze provides the smp_rmb */
if (unlikely(PageDirty(page))) {
page_ref_unfreeze(page, refcount);
goto cannot_free;
--
2.7.4
Re: [PATCH v3 5/8] proc/kcore: hold lock during read
On Wed, Jul 25, 2018 at 12:11:26AM +0900, Tetsuo Handa wrote:
> On 2018/07/19 7:58, Omar Sandoval wrote:
> > From: Omar Sandoval
> >
> > Now that we're using an rwsem, we can hold it during the entirety of
> > read_kcore() and have a common return path. This is preparation for the
> > next change.
> >
> > Signed-off-by: Omar Sandoval
> > ---
> > fs/proc/kcore.c | 70 -
> > 1 file changed, 40 insertions(+), 30 deletions(-)
> >
> > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> > index 95aa988c5b5d..e317ac890871 100644
> > --- a/fs/proc/kcore.c
> > +++ b/fs/proc/kcore.c
> > @@ -440,19 +440,18 @@ static ssize_t
> > read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t
> > *fpos)
> > {
> > char *buf = file->private_data;
> > - ssize_t acc = 0;
> > size_t size, tsz;
> > size_t elf_buflen;
> > int nphdr;
> > unsigned long start;
> > + size_t orig_buflen = buflen;
> > + int ret = 0;
> >
> > down_read(_lock);
>
> (...snipped...)
>
> > +out:
> > + up_write(_lock);
>
> Oops. This needs to be up_read().
Yeah, thanks, I'll fix this and rerun my tests with lockdep.
Re: [PATCH v3 5/8] proc/kcore: hold lock during read
On Wed, Jul 25, 2018 at 12:11:26AM +0900, Tetsuo Handa wrote:
> On 2018/07/19 7:58, Omar Sandoval wrote:
> > From: Omar Sandoval
> >
> > Now that we're using an rwsem, we can hold it during the entirety of
> > read_kcore() and have a common return path. This is preparation for the
> > next change.
> >
> > Signed-off-by: Omar Sandoval
> > ---
> > fs/proc/kcore.c | 70 -
> > 1 file changed, 40 insertions(+), 30 deletions(-)
> >
> > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> > index 95aa988c5b5d..e317ac890871 100644
> > --- a/fs/proc/kcore.c
> > +++ b/fs/proc/kcore.c
> > @@ -440,19 +440,18 @@ static ssize_t
> > read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t
> > *fpos)
> > {
> > char *buf = file->private_data;
> > - ssize_t acc = 0;
> > size_t size, tsz;
> > size_t elf_buflen;
> > int nphdr;
> > unsigned long start;
> > + size_t orig_buflen = buflen;
> > + int ret = 0;
> >
> > down_read(_lock);
>
> (...snipped...)
>
> > +out:
> > + up_write(_lock);
>
> Oops. This needs to be up_read().
Yeah, thanks, I'll fix this and rerun my tests with lockdep.
Re: [PATCH] x86/bugs: protect against userspace-userspace spectreRSB
On Thu, Jul 26, 2018 at 01:11:01AM +0200, Jiri Kosina wrote: > On Wed, 25 Jul 2018, Linus Torvalds wrote: > > > > Mitigate userspace-userspace attacks by always unconditionally filling > > > RSB on > > > context switch when generic spectrev2 mitigation has been enabled. > > > > Shouldn't this also do something like > > > > x86_spec_ctrl_base |= x86_spec_ctrl_mask & SPEC_CTRL_STIBP; > > > > if we have HT enabled? > > So IIUC your comment is not really tightly related to spectreRSB, is it? > > If you're making a more general remark about things that we'd also have to > do in order to improve userspace-userspace spectrev2 prevention, then I > agree. > > It probably wouldn't be as simple as adding it to x86_spec_ctrl_base I > think though, as the VM switching also has to save/restore it properly > (the same way we handle SSBD). So I'd rather handle this separately, as it > really is in principle a completely different protection. > > STIBP is plugging much smaller hole than spectreRSB (as the bigger part is > already plugged by IBPB) Just to clarify, the IBPB hole is *not* plugged for context switches. It's only enabled for non-dumpable processes (which are basically non-existent in practice). > so I'd rather have that one in first, and look at improving STIBP > later if noone beats me to it. It would be interesting to see performance measurements for STIBP, but based on what we've seen with IBRS in user space, I'd guess that it's not pretty. -- Josh
Re: [PATCH] x86/bugs: protect against userspace-userspace spectreRSB
On Thu, Jul 26, 2018 at 01:11:01AM +0200, Jiri Kosina wrote: > On Wed, 25 Jul 2018, Linus Torvalds wrote: > > > > Mitigate userspace-userspace attacks by always unconditionally filling > > > RSB on > > > context switch when generic spectrev2 mitigation has been enabled. > > > > Shouldn't this also do something like > > > > x86_spec_ctrl_base |= x86_spec_ctrl_mask & SPEC_CTRL_STIBP; > > > > if we have HT enabled? > > So IIUC your comment is not really tightly related to spectreRSB, is it? > > If you're making a more general remark about things that we'd also have to > do in order to improve userspace-userspace spectrev2 prevention, then I > agree. > > It probably wouldn't be as simple as adding it to x86_spec_ctrl_base I > think though, as the VM switching also has to save/restore it properly > (the same way we handle SSBD). So I'd rather handle this separately, as it > really is in principle a completely different protection. > > STIBP is plugging much smaller hole than spectreRSB (as the bigger part is > already plugged by IBPB) Just to clarify, the IBPB hole is *not* plugged for context switches. It's only enabled for non-dumpable processes (which are basically non-existent in practice). > so I'd rather have that one in first, and look at improving STIBP > later if noone beats me to it. It would be interesting to see performance measurements for STIBP, but based on what we've seen with IBRS in user space, I'd guess that it's not pretty. -- Josh
Re: [PATCH] clk: uniphier: add clock frequency support for SPI
Quoting Keiji Hayashibara (2018-07-18 22:23:48) > From: Kunihiko Hayashi > > Add clock control for SPI controller on UniPhier SoCs. > > Signed-off-by: Kunihiko Hayashi > Signed-off-by: Masahiro Yamada > --- Signed-off-by chain is a little weird, but I'll go with it. Applied to clk-next
Re: [PATCH] clk: uniphier: add clock frequency support for SPI
Quoting Keiji Hayashibara (2018-07-18 22:23:48) > From: Kunihiko Hayashi > > Add clock control for SPI controller on UniPhier SoCs. > > Signed-off-by: Kunihiko Hayashi > Signed-off-by: Masahiro Yamada > --- Signed-off-by chain is a little weird, but I'll go with it. Applied to clk-next
Re: [PATCH] thermal: qcom-spmi: Delete unused field 'prev_stage'
Hi, On Tue, Jul 24, 2018 at 4:56 PM, Matthias Kaehlcke wrote: > The field 'prev_stage' in struct qpnp_tm_chip is not used, remove it. > > Signed-off-by: Matthias Kaehlcke > --- > drivers/thermal/qcom-spmi-temp-alarm.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Douglas Anderson
Re: [PATCH] thermal: qcom-spmi: Delete unused field 'prev_stage'
Hi, On Tue, Jul 24, 2018 at 4:56 PM, Matthias Kaehlcke wrote: > The field 'prev_stage' in struct qpnp_tm_chip is not used, remove it. > > Signed-off-by: Matthias Kaehlcke > --- > drivers/thermal/qcom-spmi-temp-alarm.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Douglas Anderson
Re: [PATCH] clk: tegra: probe deferral error reporting
Quoting Marcel Ziswiler (2018-07-20 00:54:22)
> From: Marcel Ziswiler
>
> Actually report the error code from devm_regulator_get() which may as
> well just be a probe deferral.
>
> Signed-off-by: Marcel Ziswiler
>
> ---
>
> drivers/clk/tegra/clk-dfll.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index 48ee43734e05..b2123084e175 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1609,8 +1609,9 @@ int tegra_dfll_register(struct platform_device *pdev,
>
> td->vdd_reg = devm_regulator_get(td->dev, "vdd-cpu");
> if (IS_ERR(td->vdd_reg)) {
> - dev_err(td->dev, "couldn't get vdd_cpu regulator\n");
> - return PTR_ERR(td->vdd_reg);
> + ret = PTR_ERR(td->vdd_reg);
> + dev_err(td->dev, "couldn't get vdd_cpu regulator: %d\n", ret);
Do you want to know that a probe defer is happening? Usually patches are
sent to make that error path silent.
Re: [PATCH] clk: tegra: probe deferral error reporting
Quoting Marcel Ziswiler (2018-07-20 00:54:22)
> From: Marcel Ziswiler
>
> Actually report the error code from devm_regulator_get() which may as
> well just be a probe deferral.
>
> Signed-off-by: Marcel Ziswiler
>
> ---
>
> drivers/clk/tegra/clk-dfll.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index 48ee43734e05..b2123084e175 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -1609,8 +1609,9 @@ int tegra_dfll_register(struct platform_device *pdev,
>
> td->vdd_reg = devm_regulator_get(td->dev, "vdd-cpu");
> if (IS_ERR(td->vdd_reg)) {
> - dev_err(td->dev, "couldn't get vdd_cpu regulator\n");
> - return PTR_ERR(td->vdd_reg);
> + ret = PTR_ERR(td->vdd_reg);
> + dev_err(td->dev, "couldn't get vdd_cpu regulator: %d\n", ret);
Do you want to know that a probe defer is happening? Usually patches are
sent to make that error path silent.
Re: [PATCH] dt-bindings: thermal: qcom-spmi-temp-alarm: Improve thermal zone in example
Hi, On Tue, Jul 24, 2018 at 4:50 PM, Matthias Kaehlcke wrote: > The current example for a thermal zone isn't very useful as reference > since it would result in a hardware shutdown at 145°C, instead of > allowing the system to try to shutdown gracefully. Without an ADC > channel a maximum of two trip points is useful in practice for this > sensor, with temperatures corresponding to the stage 1 and stage 2 > 'hardware trip points'. A critical trip point at stage 2 may allow the > system to shutdown before a hardware shutdown at stage 3 kicks in. It > should be noted though that by default the chip performs a 'partial > shutdown' when the temperature reaches stage 2, which may prevent an > orderly shutdown. The 'partial shutdown' can be disabled by software. > > Signed-off-by: Matthias Kaehlcke > --- > .../bindings/thermal/qcom-spmi-temp-alarm.txt | 11 +++ > 1 file changed, 3 insertions(+), 8 deletions(-) Assuming your patch to disable the partial shutdown lands, then: Reviewed-by: Douglas Anderson ...if that patch doesn't land (or if someone disagrees with me and thinks we need an extra property to disable the partial shutdown) then we would need the example to be different. Because of that, maybe this should be appended to the end of your series which includes the patch to disable partial shutdown? -Doug
Re: [PATCH] dt-bindings: thermal: qcom-spmi-temp-alarm: Improve thermal zone in example
Hi, On Tue, Jul 24, 2018 at 4:50 PM, Matthias Kaehlcke wrote: > The current example for a thermal zone isn't very useful as reference > since it would result in a hardware shutdown at 145°C, instead of > allowing the system to try to shutdown gracefully. Without an ADC > channel a maximum of two trip points is useful in practice for this > sensor, with temperatures corresponding to the stage 1 and stage 2 > 'hardware trip points'. A critical trip point at stage 2 may allow the > system to shutdown before a hardware shutdown at stage 3 kicks in. It > should be noted though that by default the chip performs a 'partial > shutdown' when the temperature reaches stage 2, which may prevent an > orderly shutdown. The 'partial shutdown' can be disabled by software. > > Signed-off-by: Matthias Kaehlcke > --- > .../bindings/thermal/qcom-spmi-temp-alarm.txt | 11 +++ > 1 file changed, 3 insertions(+), 8 deletions(-) Assuming your patch to disable the partial shutdown lands, then: Reviewed-by: Douglas Anderson ...if that patch doesn't land (or if someone disagrees with me and thinks we need an extra property to disable the partial shutdown) then we would need the example to be different. Because of that, maybe this should be appended to the end of your series which includes the patch to disable partial shutdown? -Doug
Re: [PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
Hi,
On Wed, Jul 25, 2018 at 07:48:21AM +1000, NeilBrown wrote:
> On Tue, Jul 24 2018, Brian Norris wrote:
> > On Tue, Jul 24, 2018 at 11:51:49AM +1000, NeilBrown wrote:
> >> On Tue, Jul 24 2018, Boris Brezillon wrote:
> >> > On Tue, 24 Jul 2018 08:46:33 +1000
> >> > NeilBrown wrote:
> >> >> One possibility that occurred to me when I was exploring this issue is
> >> >> to revert to 3-byte mode whenever 4-byte was not actively in use.
> >> >> So any access beyond 16Meg is:
> >> >> switch-to-4-byte ; perform IO ; switch to 3-byte
> >> >> or similar. On my hardware it would be more efficient to
> >> >> use the 4-byte opcode to perform the IO, then reset the cached
> >> >> 4th address byte that the NOR chip transparently remembered.
> >
> > Do these chips cache the last 4th-byte used? I don't recall reading
> > that, but that would be interesting. It also sounds like that would make
> > things even more difficult to do robustly.
>
> That is how the Winbond W25Q256FV appears to behave in my experiments.
> Any time you use a 4-byte address, the high byte is saved. Any time you
> use a 3-byte address, the saved high-byte is used.
> The data sheet seems to support this understanding, as detailed in
>
> Commit: f134fbbb4ff8 ("mtd: spi-nor: clear Winbond Extended Address Reg on
> switch to 3-byte addressing.")
Thanks for the notes. I never really paid attention to that section of
the datasheet, since I always relied on a hardware reset.
By the way, doesn't this part support the dedicated (non-stateful)
4-byte address commands? Why not use those and avoid the problem
entirely? Although, it's not actually clear to me from the datasheet
whether, e.g., Fast Read with 4-byte Address (0Ch) will set the Extended
Address Register.
> >> >> This adds a little overhead, but should be fairly robust.
> >> >> It doesn't help if something goes terribly wrong while IO is happening,
> >> >> but I don't think any other software solution does either.
> >> >>
> >> >> How would you see that approach?
> >> >
> >> > I think the problem stands: people that have proper HW mitigation for
> >> > this problem (NOR chip is reset when the Processor is reset) don't want
> >> > to pay the overhead. So, even if we go for this approach, we probably
> >> > want to only do that for broken HW.
> >
> > If it actually saves bytes on the wire to stay in 3-byte mode more
> > often, then that could be helpful to all systems. But otherwise, yes, it
> > doesn't really belong on a properly-designed system.
>
> I'm not sure exactly what you encompass by the term "properly-designed",
I consider all systems with >16MiB SPI flash that have stateful
addressing modes and don't have a useful HW RESET signal to be
improperly designed. These SPI flash never had a standardized (or
easily-determined) software reset, so it's nearly impossible to write
robust software for them that is compatible with a relatively generic
boot ROM and/or bootloader.
> but with the SOC that I have (mt7621) and the flash chip (Winbond
> W25Q256FV) it is not possible to wire them up in any way that will work
> reliably without software support for leaving 3-byte mode.
> The SOC does not have an out-going reset line that signals a general
> system reset - it only has one that signals watchdog reset.
> The flash chip has an incoming reset line, but it shares a pin with
> "hold", and that is the default use. So we would need to program the
(BTW, you could probably choose to reprogram the HOLD# line to RESET#
before switching to 4-byte mode. That still doesn't resolve the SoC
reset line issue on its own.)
> flash to listen for reset, and it would only catch a watchdog reset.
> For any other reset, the CPU is (should be) in complete control (even
> after a panic!) and it needs to reprogram the flash chip before
> resetting the system.
"Even after a panic" is a bit dubious. That presumes that the cause of
the panic didn't also corrupt your exception code, driver code, driver
data structures, etc., that would be needed to reset the SPI chip
without double-faulting. And what about a panic in your SPI flash
driver?
Overall, my point is that the existence of such non-hardware-resettable
flash guarantees the possibility of a hard enough crash that you cannot
possibly reset it to 3-byte addressing before system reset.
> But maybe you think either the SOC and/or the flash chip is not
> "properly-designed" - and you may be correct..
Yes, I believe I do. If your watchdog reset pin was routed to the flash
HOLD/RESET pin, then I suppose it would be *possible* to have robust
software. But otherwise, I believe it's impossible.
Brian
Re: [PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
Hi,
On Wed, Jul 25, 2018 at 07:48:21AM +1000, NeilBrown wrote:
> On Tue, Jul 24 2018, Brian Norris wrote:
> > On Tue, Jul 24, 2018 at 11:51:49AM +1000, NeilBrown wrote:
> >> On Tue, Jul 24 2018, Boris Brezillon wrote:
> >> > On Tue, 24 Jul 2018 08:46:33 +1000
> >> > NeilBrown wrote:
> >> >> One possibility that occurred to me when I was exploring this issue is
> >> >> to revert to 3-byte mode whenever 4-byte was not actively in use.
> >> >> So any access beyond 16Meg is:
> >> >> switch-to-4-byte ; perform IO ; switch to 3-byte
> >> >> or similar. On my hardware it would be more efficient to
> >> >> use the 4-byte opcode to perform the IO, then reset the cached
> >> >> 4th address byte that the NOR chip transparently remembered.
> >
> > Do these chips cache the last 4th-byte used? I don't recall reading
> > that, but that would be interesting. It also sounds like that would make
> > things even more difficult to do robustly.
>
> That is how the Winbond W25Q256FV appears to behave in my experiments.
> Any time you use a 4-byte address, the high byte is saved. Any time you
> use a 3-byte address, the saved high-byte is used.
> The data sheet seems to support this understanding, as detailed in
>
> Commit: f134fbbb4ff8 ("mtd: spi-nor: clear Winbond Extended Address Reg on
> switch to 3-byte addressing.")
Thanks for the notes. I never really paid attention to that section of
the datasheet, since I always relied on a hardware reset.
By the way, doesn't this part support the dedicated (non-stateful)
4-byte address commands? Why not use those and avoid the problem
entirely? Although, it's not actually clear to me from the datasheet
whether, e.g., Fast Read with 4-byte Address (0Ch) will set the Extended
Address Register.
> >> >> This adds a little overhead, but should be fairly robust.
> >> >> It doesn't help if something goes terribly wrong while IO is happening,
> >> >> but I don't think any other software solution does either.
> >> >>
> >> >> How would you see that approach?
> >> >
> >> > I think the problem stands: people that have proper HW mitigation for
> >> > this problem (NOR chip is reset when the Processor is reset) don't want
> >> > to pay the overhead. So, even if we go for this approach, we probably
> >> > want to only do that for broken HW.
> >
> > If it actually saves bytes on the wire to stay in 3-byte mode more
> > often, then that could be helpful to all systems. But otherwise, yes, it
> > doesn't really belong on a properly-designed system.
>
> I'm not sure exactly what you encompass by the term "properly-designed",
I consider all systems with >16MiB SPI flash that have stateful
addressing modes and don't have a useful HW RESET signal to be
improperly designed. These SPI flash never had a standardized (or
easily-determined) software reset, so it's nearly impossible to write
robust software for them that is compatible with a relatively generic
boot ROM and/or bootloader.
> but with the SOC that I have (mt7621) and the flash chip (Winbond
> W25Q256FV) it is not possible to wire them up in any way that will work
> reliably without software support for leaving 3-byte mode.
> The SOC does not have an out-going reset line that signals a general
> system reset - it only has one that signals watchdog reset.
> The flash chip has an incoming reset line, but it shares a pin with
> "hold", and that is the default use. So we would need to program the
(BTW, you could probably choose to reprogram the HOLD# line to RESET#
before switching to 4-byte mode. That still doesn't resolve the SoC
reset line issue on its own.)
> flash to listen for reset, and it would only catch a watchdog reset.
> For any other reset, the CPU is (should be) in complete control (even
> after a panic!) and it needs to reprogram the flash chip before
> resetting the system.
"Even after a panic" is a bit dubious. That presumes that the cause of
the panic didn't also corrupt your exception code, driver code, driver
data structures, etc., that would be needed to reset the SPI chip
without double-faulting. And what about a panic in your SPI flash
driver?
Overall, my point is that the existence of such non-hardware-resettable
flash guarantees the possibility of a hard enough crash that you cannot
possibly reset it to 3-byte addressing before system reset.
> But maybe you think either the SOC and/or the flash chip is not
> "properly-designed" - and you may be correct..
Yes, I believe I do. If your watchdog reset pin was routed to the flash
HOLD/RESET pin, then I suppose it would be *possible* to have robust
software. But otherwise, I believe it's impossible.
Brian
Re: [PATCH 1/2] clk: uniphier: add NAND 200MHz clock
Quoting Masahiro Yamada (2018-07-20 01:37:35) > The Denali NAND controller IP needs three clocks: > > - clk: controller core clock > > - clk_x: bus interface clock > > - ecc_clk: clock at which ECC circuitry is run > > Currently, only the first one (50MHz) is provided. The rest of the > two clock ports must be connected to the 200MHz clock line. Add this. > > Signed-off-by: Masahiro Yamada > --- Applied to clk-next
Re: [PATCH 2/2] clk: uniphier: add more USB3 PHY clocks
Quoting Masahiro Yamada (2018-07-20 01:37:36) > Add USB3 PHY clocks where missing. Use fixed-factor clocks for those > without gating. > > For clarification, prefix clock names with 'ss' or 'hs'. > > Signed-off-by: Masahiro Yamada > --- Applied to clk-next
Re: [PATCH 1/2] clk: uniphier: add NAND 200MHz clock
Quoting Masahiro Yamada (2018-07-20 01:37:35) > The Denali NAND controller IP needs three clocks: > > - clk: controller core clock > > - clk_x: bus interface clock > > - ecc_clk: clock at which ECC circuitry is run > > Currently, only the first one (50MHz) is provided. The rest of the > two clock ports must be connected to the 200MHz clock line. Add this. > > Signed-off-by: Masahiro Yamada > --- Applied to clk-next
Re: [PATCH 2/2] clk: uniphier: add more USB3 PHY clocks
Quoting Masahiro Yamada (2018-07-20 01:37:36) > Add USB3 PHY clocks where missing. Use fixed-factor clocks for those > without gating. > > For clarification, prefix clock names with 'ss' or 'hs'. > > Signed-off-by: Masahiro Yamada > --- Applied to clk-next
Re: [PATCH] dt-bindings: thermal: qcom-spmi-temp-alarm: Fix documentation of 'reg'
Hi, On Tue, Jul 24, 2018 at 4:51 PM, Matthias Kaehlcke wrote: > The documentation claims that the 'reg' property consists of two values, > the SPMI address and the length of the controller's registers. However > the SPMI bus to which it is added specifies "#size-cells = <0>;". Remove > the controller register length from the documentation of the field and the > example. > > Signed-off-by: Matthias Kaehlcke > --- > .../devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH] dt-bindings: thermal: qcom-spmi-temp-alarm: Fix documentation of 'reg'
Hi, On Tue, Jul 24, 2018 at 4:51 PM, Matthias Kaehlcke wrote: > The documentation claims that the 'reg' property consists of two values, > the SPMI address and the length of the controller's registers. However > the SPMI bus to which it is added specifies "#size-cells = <0>;". Remove > the controller register length from the documentation of the field and the > example. > > Signed-off-by: Matthias Kaehlcke > --- > .../devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH v5 3/3] arm64: dts: qcom: pm8998: Add pm8998 thermal zone
Hi, On Tue, Jul 24, 2018 at 4:46 PM, Matthias Kaehlcke wrote: > The thermal zone uses spmi-temp-alarm as sensor, the trip points > correspond to the PMIC thermal stages 1 and 2. The critical trip > point at 125°C disables the partial PMIC shutdown at stage 2. > > Without an IIO input the sensor only reports a limited number of > temperatures: > > - 37°C for temperatures below 105°C > - 107°C for temperatures >= 105°C and < 125°C > - 127°C for temperatures >= 125°C > > (the numbers correspond to a stage 1 threshold of 105°C) > > Signed-off-by: Matthias Kaehlcke > --- > Changes in v5: > - removed 'stage2-shutdown-disabled' property from spmi-temp-alarm > - updated commit message > > Changes in v4: > - updated trip point temperatures to match stage 1 and 2 ones > - disabled stage 2 shutdown > - updated commit message > > Changes in v3: > - moved 'thermal-zones' node to the beginning of the .dtsi > > Changes in v2: > - defined 'thermal-zones' node in pm8998.dtsi instead of using a label > to refer to it > - use 105°C hardware trip point as critical trip point > - reduced number of trip points to 2 > - lowered temperature of passive trip point > - updated trip point names and added labels > - updated commit message > --- > arch/arm64/boot/dts/qcom/pm8998.dtsi | 25 + > 1 file changed, 25 insertions(+) Looks great! Reviewed-by: Douglas Anderson
Re: [PATCH v5 3/3] arm64: dts: qcom: pm8998: Add pm8998 thermal zone
Hi, On Tue, Jul 24, 2018 at 4:46 PM, Matthias Kaehlcke wrote: > The thermal zone uses spmi-temp-alarm as sensor, the trip points > correspond to the PMIC thermal stages 1 and 2. The critical trip > point at 125°C disables the partial PMIC shutdown at stage 2. > > Without an IIO input the sensor only reports a limited number of > temperatures: > > - 37°C for temperatures below 105°C > - 107°C for temperatures >= 105°C and < 125°C > - 127°C for temperatures >= 125°C > > (the numbers correspond to a stage 1 threshold of 105°C) > > Signed-off-by: Matthias Kaehlcke > --- > Changes in v5: > - removed 'stage2-shutdown-disabled' property from spmi-temp-alarm > - updated commit message > > Changes in v4: > - updated trip point temperatures to match stage 1 and 2 ones > - disabled stage 2 shutdown > - updated commit message > > Changes in v3: > - moved 'thermal-zones' node to the beginning of the .dtsi > > Changes in v2: > - defined 'thermal-zones' node in pm8998.dtsi instead of using a label > to refer to it > - use 105°C hardware trip point as critical trip point > - reduced number of trip points to 2 > - lowered temperature of passive trip point > - updated trip point names and added labels > - updated commit message > --- > arch/arm64/boot/dts/qcom/pm8998.dtsi | 25 + > 1 file changed, 25 insertions(+) Looks great! Reviewed-by: Douglas Anderson
