[PATCH] ext4: avoid unused variable warning
The two new variables are only used in an #ifdef, so they cause a
warning without CONFIG_QUOTA:
fs/ext4/super.c: In function 'parse_options':
fs/ext4/super.c:1977:26: error: unused variable 'grp_qf_name'
[-Werror=unused-variable]
char *p, *usr_qf_name, *grp_qf_name;
^~~
fs/ext4/super.c:1977:12: error: unused variable 'usr_qf_name'
[-Werror=unused-variable]
char *p, *usr_qf_name, *grp_qf_name;
Fixes: 20cefcdc2040 ("ext4: fix use-after-free race in ext4_remount()'s error
path")
Signed-off-by: Arnd Bergmann
---
fs/ext4/super.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e9ca8312457b..d60199510c94 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1974,7 +1974,10 @@ static int parse_options(char *options, struct
super_block *sb,
int is_remount)
{
struct ext4_sb_info *sbi = EXT4_SB(sb);
- char *p, *usr_qf_name, *grp_qf_name;
+ char *p;
+#ifdef CONFIG_QUOTA
+ char *usr_qf_name, *grp_qf_name;
+#endif
substring_t args[MAX_OPT_ARGS];
int token;
--
2.18.0
[PATCH] ext4: avoid unused variable warning
The two new variables are only used in an #ifdef, so they cause a
warning without CONFIG_QUOTA:
fs/ext4/super.c: In function 'parse_options':
fs/ext4/super.c:1977:26: error: unused variable 'grp_qf_name'
[-Werror=unused-variable]
char *p, *usr_qf_name, *grp_qf_name;
^~~
fs/ext4/super.c:1977:12: error: unused variable 'usr_qf_name'
[-Werror=unused-variable]
char *p, *usr_qf_name, *grp_qf_name;
Fixes: 20cefcdc2040 ("ext4: fix use-after-free race in ext4_remount()'s error
path")
Signed-off-by: Arnd Bergmann
---
fs/ext4/super.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e9ca8312457b..d60199510c94 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1974,7 +1974,10 @@ static int parse_options(char *options, struct
super_block *sb,
int is_remount)
{
struct ext4_sb_info *sbi = EXT4_SB(sb);
- char *p, *usr_qf_name, *grp_qf_name;
+ char *p;
+#ifdef CONFIG_QUOTA
+ char *usr_qf_name, *grp_qf_name;
+#endif
substring_t args[MAX_OPT_ARGS];
int token;
--
2.18.0
[PATCH v2 8/8] Input: olpc_apsp: allocate the GPIOs used
Take the GPIO lines are used by the SP. The driver doesn't touch the
lines -- this is done to disallow anything else from fiddling with
them because that would confuse the SP firmware.
Also, the lines are now nicely visible in /sys/kernel/debug/gpio.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index fe9e19014e70..287759c5f36d 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* The OLPC XO-1.75 and XO-4 laptops do not have a hardware PS/2 controller.
@@ -76,6 +77,8 @@ struct olpc_apsp {
struct serio *padio;
void __iomem *base;
struct clk *clk;
+ struct gpio_desc *clk_gpio;
+ struct gpio_desc *data_gpio;
int open_count;
int irq;
};
@@ -211,6 +214,16 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (IS_ERR(priv->clk))
return PTR_ERR(priv->clk);
+ priv->clk_gpio = devm_gpiod_get_optional(>dev, "clk",
+ GPIOD_ASIS);
+ if (IS_ERR(priv->clk_gpio))
+ return PTR_ERR(priv->clk_gpio);
+
+ priv->data_gpio = devm_gpiod_get_optional(>dev, "data",
+ GPIOD_ASIS);
+ if (IS_ERR(priv->data_gpio))
+ return PTR_ERR(priv->data_gpio);
+
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
[PATCH v2 8/8] Input: olpc_apsp: allocate the GPIOs used
Take the GPIO lines are used by the SP. The driver doesn't touch the
lines -- this is done to disallow anything else from fiddling with
them because that would confuse the SP firmware.
Also, the lines are now nicely visible in /sys/kernel/debug/gpio.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index fe9e19014e70..287759c5f36d 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* The OLPC XO-1.75 and XO-4 laptops do not have a hardware PS/2 controller.
@@ -76,6 +77,8 @@ struct olpc_apsp {
struct serio *padio;
void __iomem *base;
struct clk *clk;
+ struct gpio_desc *clk_gpio;
+ struct gpio_desc *data_gpio;
int open_count;
int irq;
};
@@ -211,6 +214,16 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (IS_ERR(priv->clk))
return PTR_ERR(priv->clk);
+ priv->clk_gpio = devm_gpiod_get_optional(>dev, "clk",
+ GPIOD_ASIS);
+ if (IS_ERR(priv->clk_gpio))
+ return PTR_ERR(priv->clk_gpio);
+
+ priv->data_gpio = devm_gpiod_get_optional(>dev, "data",
+ GPIOD_ASIS);
+ if (IS_ERR(priv->data_gpio))
+ return PTR_ERR(priv->data_gpio);
+
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
Re: [PATCH] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
On Wed, Oct 10, 2018 at 03:03:05PM +0100, Laurence Rochfort wrote:
> On Tue, Oct 09, 2018 at 03:13:03PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 04, 2018 at 06:52:25PM +0100, Laurence Rochfort wrote:
> > > Silence the below sparse warnings by casting betwen u8 __iomem *, and
> > > void *.
> > >
> > > warning: incorrect type in assignment (different address spaces)
> > >expected unsigned char [noderef] [usertype] *virt_base
> > >got void *[assigned] mem
> > > warning: incorrect type in argument 3 (different address spaces)
> > >expected void *cpu_addr
> > >got unsigned char [noderef] [usertype] *virt_base
> > >
> > > Signed-off-by: Laurence Rochfort
> > > ---
> > > drivers/staging/gasket/gasket_page_table.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/gasket/gasket_page_table.c
> > > b/drivers/staging/gasket/gasket_page_table.c
> > > index d4c5f8a..7f5e5b3 100644
> > > --- a/drivers/staging/gasket/gasket_page_table.c
> > > +++ b/drivers/staging/gasket/gasket_page_table.c
> > > @@ -1278,7 +1278,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > >dma_addr_t *dma_address, u64 index)
> > > {
> > > dma_addr_t handle;
> > > - void *mem;
> > > + u8 __iomem *mem;
> > > int j;
> > > unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> > > const struct gasket_driver_desc *driver_desc =
> > > @@ -1290,7 +1290,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > if (num_pages == 0)
> > > return -EINVAL;
> > >
> > > - mem = dma_alloc_coherent(gasket_get_device(gasket_dev),
> > > + mem = (u8 __iomem *)dma_alloc_coherent(gasket_get_device(gasket_dev),
> > >num_pages * PAGE_SIZE, , 0);
> > > if (!mem)
> > > goto nomem;
> > > @@ -1325,7 +1325,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > nomem:
> > > if (mem) {
> > > dma_free_coherent(gasket_get_device(gasket_dev),
> > > - num_pages * PAGE_SIZE, mem, handle);
> > > + num_pages * PAGE_SIZE, (void __force *)mem,
> > > handle);
> > > }
> > >
> > > if (gasket_dev->page_table[index]->coherent_pages) {
> > > @@ -1353,7 +1353,7 @@ int gasket_free_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > if (gasket_dev->coherent_buffer.length_bytes) {
> > > dma_free_coherent(gasket_get_device(gasket_dev),
> > > gasket_dev->coherent_buffer.length_bytes,
> > > - gasket_dev->coherent_buffer.virt_base,
> > > + (void __force
> > > *)gasket_dev->coherent_buffer.virt_base,
> >
> > We should not have to "force" anything here. Something feels wrong in
> > the definition of these variable types...
> >From bde80ff6bdab740d838210aa3e664c8901b2142a Mon Sep 17 00:00:00 2001
> From: Laurence Rochfort
> Date: Thu, 4 Oct 2018 18:42:26 +0100
> Subject: [PATCH v2] staging: gasket: Fix sparse "incorrect type in assignment"
> warnings.
>
> Correct the coherent buffer data type and memory space cookie.
Why have you attached a patch? I can't do anything with that.
And it seems wrong as well, are you sure about this change?
greg k-h
[PATCH v2 6/8] Input: olpc_apsp: check FIFO status on open(), not probe()
Let's defer the FIFO status checking until open().
When we'll get a clk handle, this will allow us to defer clock enablement
until the device is actually used.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index 8e9a4209fcad..8b19a47dfa46 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -145,8 +145,15 @@ static int olpc_apsp_open(struct serio *port)
{
struct olpc_apsp *priv = port->port_data;
unsigned int tmp;
+ unsigned long l;
if (priv->open_count++ == 0) {
+ l = readl(priv->base + COMMAND_FIFO_STATUS);
+ if (!(l & CMD_STS_MASK)) {
+ dev_err(priv->dev, "SP cannot accept commands.\n");
+ return -EIO;
+ }
+
/* Enable interrupt 0 by clearing its bit */
tmp = readl(priv->base + PJ_INTERRUPT_MASK);
writel(tmp & ~INT_0, priv->base + PJ_INTERRUPT_MASK);
@@ -173,7 +180,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
struct olpc_apsp *priv;
struct resource *res;
struct device_node *np;
- unsigned long l;
int error;
priv = devm_kzalloc(>dev, sizeof(struct olpc_apsp), GFP_KERNEL);
@@ -192,12 +198,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (priv->irq < 0)
return priv->irq;
- l = readl(priv->base + COMMAND_FIFO_STATUS);
- if (!(l & CMD_STS_MASK)) {
- dev_err(>dev, "SP cannot accept commands.\n");
- return -EIO;
- }
-
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
[PATCH v2 5/8] Input: olpc_apsp: depend on CONFIG_OF
This is a device-tree enabled driver. Moreover CONFIG_OLPC is specific to the x86 platform code, while the driver is for an ARM-based laptop. Signed-off-by: Lubomir Rintel --- drivers/input/serio/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig index d90d9f1098ff..a4f70021b53e 100644 --- a/drivers/input/serio/Kconfig +++ b/drivers/input/serio/Kconfig @@ -254,7 +254,7 @@ config SERIO_APBPS2 config SERIO_OLPC_APSP tristate "OLPC AP-SP input support" - depends on OLPC || COMPILE_TEST + depends on OF help Say Y here if you want support for the keyboard and touchpad included in the OLPC XO-1.75 and XO-4 laptops. -- 2.19.0
Re: [PATCH] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
On Wed, Oct 10, 2018 at 03:03:05PM +0100, Laurence Rochfort wrote:
> On Tue, Oct 09, 2018 at 03:13:03PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 04, 2018 at 06:52:25PM +0100, Laurence Rochfort wrote:
> > > Silence the below sparse warnings by casting betwen u8 __iomem *, and
> > > void *.
> > >
> > > warning: incorrect type in assignment (different address spaces)
> > >expected unsigned char [noderef] [usertype] *virt_base
> > >got void *[assigned] mem
> > > warning: incorrect type in argument 3 (different address spaces)
> > >expected void *cpu_addr
> > >got unsigned char [noderef] [usertype] *virt_base
> > >
> > > Signed-off-by: Laurence Rochfort
> > > ---
> > > drivers/staging/gasket/gasket_page_table.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/gasket/gasket_page_table.c
> > > b/drivers/staging/gasket/gasket_page_table.c
> > > index d4c5f8a..7f5e5b3 100644
> > > --- a/drivers/staging/gasket/gasket_page_table.c
> > > +++ b/drivers/staging/gasket/gasket_page_table.c
> > > @@ -1278,7 +1278,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > >dma_addr_t *dma_address, u64 index)
> > > {
> > > dma_addr_t handle;
> > > - void *mem;
> > > + u8 __iomem *mem;
> > > int j;
> > > unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> > > const struct gasket_driver_desc *driver_desc =
> > > @@ -1290,7 +1290,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > if (num_pages == 0)
> > > return -EINVAL;
> > >
> > > - mem = dma_alloc_coherent(gasket_get_device(gasket_dev),
> > > + mem = (u8 __iomem *)dma_alloc_coherent(gasket_get_device(gasket_dev),
> > >num_pages * PAGE_SIZE, , 0);
> > > if (!mem)
> > > goto nomem;
> > > @@ -1325,7 +1325,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > nomem:
> > > if (mem) {
> > > dma_free_coherent(gasket_get_device(gasket_dev),
> > > - num_pages * PAGE_SIZE, mem, handle);
> > > + num_pages * PAGE_SIZE, (void __force *)mem,
> > > handle);
> > > }
> > >
> > > if (gasket_dev->page_table[index]->coherent_pages) {
> > > @@ -1353,7 +1353,7 @@ int gasket_free_coherent_memory(struct gasket_dev
> > > *gasket_dev, u64 size,
> > > if (gasket_dev->coherent_buffer.length_bytes) {
> > > dma_free_coherent(gasket_get_device(gasket_dev),
> > > gasket_dev->coherent_buffer.length_bytes,
> > > - gasket_dev->coherent_buffer.virt_base,
> > > + (void __force
> > > *)gasket_dev->coherent_buffer.virt_base,
> >
> > We should not have to "force" anything here. Something feels wrong in
> > the definition of these variable types...
> >From bde80ff6bdab740d838210aa3e664c8901b2142a Mon Sep 17 00:00:00 2001
> From: Laurence Rochfort
> Date: Thu, 4 Oct 2018 18:42:26 +0100
> Subject: [PATCH v2] staging: gasket: Fix sparse "incorrect type in assignment"
> warnings.
>
> Correct the coherent buffer data type and memory space cookie.
Why have you attached a patch? I can't do anything with that.
And it seems wrong as well, are you sure about this change?
greg k-h
[PATCH v2 6/8] Input: olpc_apsp: check FIFO status on open(), not probe()
Let's defer the FIFO status checking until open().
When we'll get a clk handle, this will allow us to defer clock enablement
until the device is actually used.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index 8e9a4209fcad..8b19a47dfa46 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -145,8 +145,15 @@ static int olpc_apsp_open(struct serio *port)
{
struct olpc_apsp *priv = port->port_data;
unsigned int tmp;
+ unsigned long l;
if (priv->open_count++ == 0) {
+ l = readl(priv->base + COMMAND_FIFO_STATUS);
+ if (!(l & CMD_STS_MASK)) {
+ dev_err(priv->dev, "SP cannot accept commands.\n");
+ return -EIO;
+ }
+
/* Enable interrupt 0 by clearing its bit */
tmp = readl(priv->base + PJ_INTERRUPT_MASK);
writel(tmp & ~INT_0, priv->base + PJ_INTERRUPT_MASK);
@@ -173,7 +180,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
struct olpc_apsp *priv;
struct resource *res;
struct device_node *np;
- unsigned long l;
int error;
priv = devm_kzalloc(>dev, sizeof(struct olpc_apsp), GFP_KERNEL);
@@ -192,12 +198,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (priv->irq < 0)
return priv->irq;
- l = readl(priv->base + COMMAND_FIFO_STATUS);
- if (!(l & CMD_STS_MASK)) {
- dev_err(>dev, "SP cannot accept commands.\n");
- return -EIO;
- }
-
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
[PATCH v2 5/8] Input: olpc_apsp: depend on CONFIG_OF
This is a device-tree enabled driver. Moreover CONFIG_OLPC is specific to the x86 platform code, while the driver is for an ARM-based laptop. Signed-off-by: Lubomir Rintel --- drivers/input/serio/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig index d90d9f1098ff..a4f70021b53e 100644 --- a/drivers/input/serio/Kconfig +++ b/drivers/input/serio/Kconfig @@ -254,7 +254,7 @@ config SERIO_APBPS2 config SERIO_OLPC_APSP tristate "OLPC AP-SP input support" - depends on OLPC || COMPILE_TEST + depends on OF help Say Y here if you want support for the keyboard and touchpad included in the OLPC XO-1.75 and XO-4 laptops. -- 2.19.0
[PATCH v2 4/8] clk: mmp2: add SP clock
The "security processor", sometimes referred to as "wireless trusted
module" or "generic encrypt unit" is a low-power core present on MMP2,
that has nothing to do with security, wireless, trust or encryption.
On an OLPC machine it runs CForth and serves as a keyboard controller:
http://dev.laptop.org/git/users/wmb/cforth/tree/src/app/arm-xo-1.75/ps2.fth
The register address was obtained from the OLPC kernel, since the
datasheet seems to be the Marvell's most important business secret.
Acked-by: Stephen Boyd
Signed-off-by: Lubomir Rintel
---
drivers/clk/mmp/clk-of-mmp2.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
index 0fc75c395957..1da50f827a15 100644
--- a/drivers/clk/mmp/clk-of-mmp2.c
+++ b/drivers/clk/mmp/clk-of-mmp2.c
@@ -53,6 +53,7 @@
#define APMU_DISP1 0x110
#define APMU_CCIC0 0x50
#define APMU_CCIC1 0xf4
+#define APMU_SP0x68
#define MPMU_UART_PLL 0x14
struct mmp2_clk_unit {
@@ -209,6 +210,8 @@ static struct mmp_clk_mix_config ccic1_mix_config = {
.reg_info = DEFINE_MIX_REG_INFO(4, 16, 2, 6, 32),
};
+static DEFINE_SPINLOCK(sp_lock);
+
static struct mmp_param_mux_clk apmu_mux_clks[] = {
{MMP2_CLK_DISP0_MUX, "disp0_mux", disp_parent_names,
ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP0, 6, 2, 0,
_lock},
{MMP2_CLK_DISP1_MUX, "disp1_mux", disp_parent_names,
ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP1, 6, 2, 0,
_lock},
@@ -239,6 +242,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
{MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT,
APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, _lock},
{MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk",
CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, _lock},
{MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div",
CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, _lock},
+ {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b,
0x0, 0, _lock},
};
static void mmp2_axi_periph_clk_init(struct mmp2_clk_unit *pxa_unit)
--
2.19.0
[PATCH v2 7/8] Input: olpc_apsp: enable the SP clock
Without the clock, the keyboard controller won't operate.
Tested on an OLPC XO 1.75.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index 8b19a47dfa46..fe9e19014e70 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
/*
* The OLPC XO-1.75 and XO-4 laptops do not have a hardware PS/2 controller.
@@ -74,6 +75,7 @@ struct olpc_apsp {
struct serio *kbio;
struct serio *padio;
void __iomem *base;
+ struct clk *clk;
int open_count;
int irq;
};
@@ -146,8 +148,13 @@ static int olpc_apsp_open(struct serio *port)
struct olpc_apsp *priv = port->port_data;
unsigned int tmp;
unsigned long l;
+ int error;
if (priv->open_count++ == 0) {
+ error = clk_prepare_enable(priv->clk);
+ if (error)
+ return error;
+
l = readl(priv->base + COMMAND_FIFO_STATUS);
if (!(l & CMD_STS_MASK)) {
dev_err(priv->dev, "SP cannot accept commands.\n");
@@ -171,6 +178,8 @@ static void olpc_apsp_close(struct serio *port)
/* Disable interrupt 0 */
tmp = readl(priv->base + PJ_INTERRUPT_MASK);
writel(tmp | INT_0, priv->base + PJ_INTERRUPT_MASK);
+
+ clk_disable_unprepare(priv->clk);
}
}
@@ -198,6 +207,10 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (priv->irq < 0)
return priv->irq;
+ priv->clk = devm_clk_get(>dev, "sp");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
[PATCH v2 0/8] OLPC 1.75 Keyboard/Touchpad fixes
This makes keyboard/touchpad work on a DT MMP2 platform. I believe that it would be a good idea if this, once reviewed, went in via the input tree. The DT and CLK parts got reviews/acks. Changes from v1: - Basically none, just re-send, including Ack and Review tags in patches that received them. Plus an extra Cc. I'd be very thankful for reviews. Lubi
[PATCH v2 2/8] dt-bindings: olpc,ap-sp: add GPIO lines
Add properties describing the GPIO lines used by the keyboard controller.
The olpc-apsp driver will do happily without them, but they are still part
of the hardware description. The driver could still reserve the lines,
so that nothing else touches them.
This makes the device node almost compatible with "ps2-gpio". I'm not
adding a compatible property, because ps2-gpio would use a different
interrupt, so that we'd need to name it. And I haven't actually tried it.
Reviewed-by: Rob Herring
Signed-off-by: Lubomir Rintel
---
Documentation/devicetree/bindings/serio/olpc,ap-sp.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
index 36603419d6f8..2b1b1dbb54c1 100644
--- a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
+++ b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
@@ -7,6 +7,10 @@ Required properties:
- clocks : phandle + clock-specifier for the clock that drives the WTM
- clock-names: should be "sp"
+Optional properties:
+- data-gpios : GPIO line used for PS/2 interface data
+- clk-gpios : GPIO line used for PS/2 interface clock
+
Example:
ap-sp@d429 {
compatible = "olpc,ap-sp";
@@ -14,4 +18,6 @@ Example:
interrupts = <40>;
clocks = <_clocks MMP2_CLK_SP>;
clock-names = "sp";
+ data-gpios = < 72 GPIO_ACTIVE_HIGH>;
+ clk-gpios = < 71 GPIO_ACTIVE_HIGH>;
}
--
2.19.0
[PATCH v2 4/8] clk: mmp2: add SP clock
The "security processor", sometimes referred to as "wireless trusted
module" or "generic encrypt unit" is a low-power core present on MMP2,
that has nothing to do with security, wireless, trust or encryption.
On an OLPC machine it runs CForth and serves as a keyboard controller:
http://dev.laptop.org/git/users/wmb/cforth/tree/src/app/arm-xo-1.75/ps2.fth
The register address was obtained from the OLPC kernel, since the
datasheet seems to be the Marvell's most important business secret.
Acked-by: Stephen Boyd
Signed-off-by: Lubomir Rintel
---
drivers/clk/mmp/clk-of-mmp2.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
index 0fc75c395957..1da50f827a15 100644
--- a/drivers/clk/mmp/clk-of-mmp2.c
+++ b/drivers/clk/mmp/clk-of-mmp2.c
@@ -53,6 +53,7 @@
#define APMU_DISP1 0x110
#define APMU_CCIC0 0x50
#define APMU_CCIC1 0xf4
+#define APMU_SP0x68
#define MPMU_UART_PLL 0x14
struct mmp2_clk_unit {
@@ -209,6 +210,8 @@ static struct mmp_clk_mix_config ccic1_mix_config = {
.reg_info = DEFINE_MIX_REG_INFO(4, 16, 2, 6, 32),
};
+static DEFINE_SPINLOCK(sp_lock);
+
static struct mmp_param_mux_clk apmu_mux_clks[] = {
{MMP2_CLK_DISP0_MUX, "disp0_mux", disp_parent_names,
ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP0, 6, 2, 0,
_lock},
{MMP2_CLK_DISP1_MUX, "disp1_mux", disp_parent_names,
ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP1, 6, 2, 0,
_lock},
@@ -239,6 +242,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
{MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT,
APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, _lock},
{MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk",
CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, _lock},
{MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div",
CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, _lock},
+ {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b,
0x0, 0, _lock},
};
static void mmp2_axi_periph_clk_init(struct mmp2_clk_unit *pxa_unit)
--
2.19.0
[PATCH v2 7/8] Input: olpc_apsp: enable the SP clock
Without the clock, the keyboard controller won't operate.
Tested on an OLPC XO 1.75.
Signed-off-by: Lubomir Rintel
---
drivers/input/serio/olpc_apsp.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index 8b19a47dfa46..fe9e19014e70 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
/*
* The OLPC XO-1.75 and XO-4 laptops do not have a hardware PS/2 controller.
@@ -74,6 +75,7 @@ struct olpc_apsp {
struct serio *kbio;
struct serio *padio;
void __iomem *base;
+ struct clk *clk;
int open_count;
int irq;
};
@@ -146,8 +148,13 @@ static int olpc_apsp_open(struct serio *port)
struct olpc_apsp *priv = port->port_data;
unsigned int tmp;
unsigned long l;
+ int error;
if (priv->open_count++ == 0) {
+ error = clk_prepare_enable(priv->clk);
+ if (error)
+ return error;
+
l = readl(priv->base + COMMAND_FIFO_STATUS);
if (!(l & CMD_STS_MASK)) {
dev_err(priv->dev, "SP cannot accept commands.\n");
@@ -171,6 +178,8 @@ static void olpc_apsp_close(struct serio *port)
/* Disable interrupt 0 */
tmp = readl(priv->base + PJ_INTERRUPT_MASK);
writel(tmp | INT_0, priv->base + PJ_INTERRUPT_MASK);
+
+ clk_disable_unprepare(priv->clk);
}
}
@@ -198,6 +207,10 @@ static int olpc_apsp_probe(struct platform_device *pdev)
if (priv->irq < 0)
return priv->irq;
+ priv->clk = devm_clk_get(>dev, "sp");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
/* KEYBOARD */
kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
if (!kb_serio)
--
2.19.0
[PATCH v2 0/8] OLPC 1.75 Keyboard/Touchpad fixes
This makes keyboard/touchpad work on a DT MMP2 platform. I believe that it would be a good idea if this, once reviewed, went in via the input tree. The DT and CLK parts got reviews/acks. Changes from v1: - Basically none, just re-send, including Ack and Review tags in patches that received them. Plus an extra Cc. I'd be very thankful for reviews. Lubi
[PATCH v2 2/8] dt-bindings: olpc,ap-sp: add GPIO lines
Add properties describing the GPIO lines used by the keyboard controller.
The olpc-apsp driver will do happily without them, but they are still part
of the hardware description. The driver could still reserve the lines,
so that nothing else touches them.
This makes the device node almost compatible with "ps2-gpio". I'm not
adding a compatible property, because ps2-gpio would use a different
interrupt, so that we'd need to name it. And I haven't actually tried it.
Reviewed-by: Rob Herring
Signed-off-by: Lubomir Rintel
---
Documentation/devicetree/bindings/serio/olpc,ap-sp.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
index 36603419d6f8..2b1b1dbb54c1 100644
--- a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
+++ b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
@@ -7,6 +7,10 @@ Required properties:
- clocks : phandle + clock-specifier for the clock that drives the WTM
- clock-names: should be "sp"
+Optional properties:
+- data-gpios : GPIO line used for PS/2 interface data
+- clk-gpios : GPIO line used for PS/2 interface clock
+
Example:
ap-sp@d429 {
compatible = "olpc,ap-sp";
@@ -14,4 +18,6 @@ Example:
interrupts = <40>;
clocks = <_clocks MMP2_CLK_SP>;
clock-names = "sp";
+ data-gpios = < 72 GPIO_ACTIVE_HIGH>;
+ clk-gpios = < 71 GPIO_ACTIVE_HIGH>;
}
--
2.19.0
[PATCH v2 3/8] dt-bindings: marvell,mmp2: Add clock id for the SP clock
This is the clock for the "security processor" core. Reviewed-by: Rob Herring Acked-by: Stephen Boyd Signed-off-by: Lubomir Rintel --- include/dt-bindings/clock/marvell,mmp2.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/clock/marvell,mmp2.h b/include/dt-bindings/clock/marvell,mmp2.h index 228a5e234af0..7b24fc791146 100644 --- a/include/dt-bindings/clock/marvell,mmp2.h +++ b/include/dt-bindings/clock/marvell,mmp2.h @@ -71,6 +71,7 @@ #define MMP2_CLK_CCIC1_MIX 117 #define MMP2_CLK_CCIC1_PHY 118 #define MMP2_CLK_CCIC1_SPHY119 +#define MMP2_CLK_SP120 #define MMP2_NR_CLKS 200 #endif -- 2.19.0
[PATCH v2 3/8] dt-bindings: marvell,mmp2: Add clock id for the SP clock
This is the clock for the "security processor" core. Reviewed-by: Rob Herring Acked-by: Stephen Boyd Signed-off-by: Lubomir Rintel --- include/dt-bindings/clock/marvell,mmp2.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/clock/marvell,mmp2.h b/include/dt-bindings/clock/marvell,mmp2.h index 228a5e234af0..7b24fc791146 100644 --- a/include/dt-bindings/clock/marvell,mmp2.h +++ b/include/dt-bindings/clock/marvell,mmp2.h @@ -71,6 +71,7 @@ #define MMP2_CLK_CCIC1_MIX 117 #define MMP2_CLK_CCIC1_PHY 118 #define MMP2_CLK_CCIC1_SPHY119 +#define MMP2_CLK_SP120 #define MMP2_NR_CLKS 200 #endif -- 2.19.0
[PATCH v2 1/8] dt-bindings: olpc,ap-sp: add clock
The clock is necessary for the device operation, hence it's required.
Its name, "sp", stands for "Security Processor". It is one of several
names that are used for the processor that serves as the keyboard
controller on an OLPC and is consistent with the node name.
Reviewed-by: Rob Herring
Signed-off-by: Lubomir Rintel
---
Documentation/devicetree/bindings/serio/olpc,ap-sp.txt | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
index 0e72183f52bc..36603419d6f8 100644
--- a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
+++ b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
@@ -4,10 +4,14 @@ Required properties:
- compatible : "olpc,ap-sp"
- reg : base address and length of SoC's WTM registers
- interrupts : SP-AP interrupt
+- clocks : phandle + clock-specifier for the clock that drives the WTM
+- clock-names: should be "sp"
Example:
ap-sp@d429 {
compatible = "olpc,ap-sp";
reg = <0xd429 0x1000>;
interrupts = <40>;
+ clocks = <_clocks MMP2_CLK_SP>;
+ clock-names = "sp";
}
--
2.19.0
[PATCH v2 1/8] dt-bindings: olpc,ap-sp: add clock
The clock is necessary for the device operation, hence it's required.
Its name, "sp", stands for "Security Processor". It is one of several
names that are used for the processor that serves as the keyboard
controller on an OLPC and is consistent with the node name.
Reviewed-by: Rob Herring
Signed-off-by: Lubomir Rintel
---
Documentation/devicetree/bindings/serio/olpc,ap-sp.txt | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
index 0e72183f52bc..36603419d6f8 100644
--- a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
+++ b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
@@ -4,10 +4,14 @@ Required properties:
- compatible : "olpc,ap-sp"
- reg : base address and length of SoC's WTM registers
- interrupts : SP-AP interrupt
+- clocks : phandle + clock-specifier for the clock that drives the WTM
+- clock-names: should be "sp"
Example:
ap-sp@d429 {
compatible = "olpc,ap-sp";
reg = <0xd429 0x1000>;
interrupts = <40>;
+ clocks = <_clocks MMP2_CLK_SP>;
+ clock-names = "sp";
}
--
2.19.0
Re: [RFC PATCH 6/7] sched/fair: update cpu_capcity to reflect thermal pressure
On 10/10/2018 01:57 AM, Javi Merino wrote: > On Tue, Oct 09, 2018 at 12:25:01PM -0400, Thara Gopinath wrote: >> cpu_capacity relflects the maximum available capacity of a cpu. Thermal >> pressure on a cpu means this maximum available capacity is reduced. This >> patch reduces the average thermal pressure for a cpu from its maximum >> available capacity so that cpu_capacity reflects the actual >> available capacity. >> >> Signed-off-by: Thara Gopinath >> --- >> kernel/sched/fair.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 7deb1d0..8651e55 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -7497,6 +7497,7 @@ static unsigned long scale_rt_capacity(int cpu) >> >> used = READ_ONCE(rq->avg_rt.util_avg); >> used += READ_ONCE(rq->avg_dl.util_avg); >> +used += READ_ONCE(rq->avg_thermal.load_avg); > > IIUIC, you are treating thermal pressure as an artificial load on the > cpu. If so, this sounds like a hard to maintain hack. Thermal > pressure have different characteristics to utilization. What happens > if thermal sets the cpu cooling state back to 0 because there is > thermal headroom again? Do we keep adding this artificial load to the > cpu just because there was thermal pressure in the past and let it > decay as if it was cpu load? Setting cpu cooling state back to 0 will decay the thermal pressure back to 0 ? Yes, cpu will not register an instantaneous drop in the cap, it will be decayed down following the PELT signals. > > Cheers, > Javi > -- Regards Thara
Re: [RFC PATCH 6/7] sched/fair: update cpu_capcity to reflect thermal pressure
On 10/10/2018 01:57 AM, Javi Merino wrote: > On Tue, Oct 09, 2018 at 12:25:01PM -0400, Thara Gopinath wrote: >> cpu_capacity relflects the maximum available capacity of a cpu. Thermal >> pressure on a cpu means this maximum available capacity is reduced. This >> patch reduces the average thermal pressure for a cpu from its maximum >> available capacity so that cpu_capacity reflects the actual >> available capacity. >> >> Signed-off-by: Thara Gopinath >> --- >> kernel/sched/fair.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 7deb1d0..8651e55 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -7497,6 +7497,7 @@ static unsigned long scale_rt_capacity(int cpu) >> >> used = READ_ONCE(rq->avg_rt.util_avg); >> used += READ_ONCE(rq->avg_dl.util_avg); >> +used += READ_ONCE(rq->avg_thermal.load_avg); > > IIUIC, you are treating thermal pressure as an artificial load on the > cpu. If so, this sounds like a hard to maintain hack. Thermal > pressure have different characteristics to utilization. What happens > if thermal sets the cpu cooling state back to 0 because there is > thermal headroom again? Do we keep adding this artificial load to the > cpu just because there was thermal pressure in the past and let it > decay as if it was cpu load? Setting cpu cooling state back to 0 will decay the thermal pressure back to 0 ? Yes, cpu will not register an instantaneous drop in the cap, it will be decayed down following the PELT signals. > > Cheers, > Javi > -- Regards Thara
Re: INFO: rcu detected stall in shmem_fault
On 2018/10/10 20:35, Michal Hocko wrote:
What should we do if memcg-OOM found no killable task because the
allocating task
was oom_score_adj == -1000 ? Flooding printk() until RCU stall watchdog
fires
(which seems to be caused by commit 3100dab2aa09dc6e ("mm: memcontrol:
print proper
OOM header when no eligible victim left") because syzbot was terminating
the test
upon WARN(1) removed by that commit) is not a good behavior.
>>>
>>> We definitely want to inform about ineligible oom victim. We might
>>> consider some rate limiting for the memcg state but that is a valuable
>>> information to see under normal situation (when you do not have floods
>>> of these situations).
>>>
>>
>> But if the caller cannot be noticed by SIGKILL from the OOM killer,
>> allowing the caller to trigger the OOM killer again and again (until
>> global OOM killer triggers) is bad.
>
> There is simply no other option. Well, except for failing the charge
> which has been considered and refused because it could trigger
> unexpected error paths and that breaking the isolation on rare cases
> when of the misconfiguration is acceptable. We can reconsider that
> but you should bring really good arguments on the table. I was very
> successful doing that.
>
By the way, how do we avoid this flooding? Something like this?
include/linux/sched.h | 1 +
mm/oom_kill.c | 11 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 977cb57..58eff50 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -723,6 +723,7 @@ struct task_struct {
#endif
#ifdef CONFIG_MEMCG
unsignedin_user_fault:1;
+ unsignedmemcg_oom_no_eligible_warned:1;
#ifdef CONFIG_MEMCG_KMEM
unsignedmemcg_kmem_skip_account:1;
#endif
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f10aa53..ff0fa65 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -1106,6 +1106,13 @@ bool out_of_memory(struct oom_control *oc)
select_bad_process(oc);
/* Found nothing?!?! */
if (!oc->chosen) {
+#ifdef CONFIG_MEMCG
+ if (is_memcg_oom(oc)) {
+ if (current->memcg_oom_no_eligible_warned)
+ return false;
+ current->memcg_oom_no_eligible_warned = 1;
+ }
+#endif
dump_header(oc, NULL);
pr_warn("Out of memory and no killable processes...\n");
/*
@@ -1115,6 +1122,10 @@ bool out_of_memory(struct oom_control *oc)
*/
if (!is_sysrq_oom(oc) && !is_memcg_oom(oc))
panic("System is deadlocked on memory\n");
+#ifdef CONFIG_MEMCG
+ } else if (is_memcg_oom(oc)) {
+ current->memcg_oom_no_eligible_warned = 0;
+#endif
}
if (oc->chosen && oc->chosen != (void *)-1UL)
oom_kill_process(oc, !is_memcg_oom(oc) ? "Out of memory" :
--
1.8.3.1
Re: INFO: rcu detected stall in shmem_fault
On 2018/10/10 20:35, Michal Hocko wrote:
What should we do if memcg-OOM found no killable task because the
allocating task
was oom_score_adj == -1000 ? Flooding printk() until RCU stall watchdog
fires
(which seems to be caused by commit 3100dab2aa09dc6e ("mm: memcontrol:
print proper
OOM header when no eligible victim left") because syzbot was terminating
the test
upon WARN(1) removed by that commit) is not a good behavior.
>>>
>>> We definitely want to inform about ineligible oom victim. We might
>>> consider some rate limiting for the memcg state but that is a valuable
>>> information to see under normal situation (when you do not have floods
>>> of these situations).
>>>
>>
>> But if the caller cannot be noticed by SIGKILL from the OOM killer,
>> allowing the caller to trigger the OOM killer again and again (until
>> global OOM killer triggers) is bad.
>
> There is simply no other option. Well, except for failing the charge
> which has been considered and refused because it could trigger
> unexpected error paths and that breaking the isolation on rare cases
> when of the misconfiguration is acceptable. We can reconsider that
> but you should bring really good arguments on the table. I was very
> successful doing that.
>
By the way, how do we avoid this flooding? Something like this?
include/linux/sched.h | 1 +
mm/oom_kill.c | 11 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 977cb57..58eff50 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -723,6 +723,7 @@ struct task_struct {
#endif
#ifdef CONFIG_MEMCG
unsignedin_user_fault:1;
+ unsignedmemcg_oom_no_eligible_warned:1;
#ifdef CONFIG_MEMCG_KMEM
unsignedmemcg_kmem_skip_account:1;
#endif
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f10aa53..ff0fa65 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -1106,6 +1106,13 @@ bool out_of_memory(struct oom_control *oc)
select_bad_process(oc);
/* Found nothing?!?! */
if (!oc->chosen) {
+#ifdef CONFIG_MEMCG
+ if (is_memcg_oom(oc)) {
+ if (current->memcg_oom_no_eligible_warned)
+ return false;
+ current->memcg_oom_no_eligible_warned = 1;
+ }
+#endif
dump_header(oc, NULL);
pr_warn("Out of memory and no killable processes...\n");
/*
@@ -1115,6 +1122,10 @@ bool out_of_memory(struct oom_control *oc)
*/
if (!is_sysrq_oom(oc) && !is_memcg_oom(oc))
panic("System is deadlocked on memory\n");
+#ifdef CONFIG_MEMCG
+ } else if (is_memcg_oom(oc)) {
+ current->memcg_oom_no_eligible_warned = 0;
+#endif
}
if (oc->chosen && oc->chosen != (void *)-1UL)
oom_kill_process(oc, !is_memcg_oom(oc) ? "Out of memory" :
--
1.8.3.1
Re: [PATCH v2 01/11] arch/x86: Start renaming the rdt files to more generic names
Babu, On Wed, 10 Oct 2018, Moger, Babu wrote: > On 10/09/2018 05:01 PM, Reinette Chatre wrote: > > As far as all the code you touch is concerned it may be easier and cause > > less confusion for now to just follow the current naming conventions as > > you have done in patches 3 onwards and have it be included in the later > > larger restructuring. > > Yes. I am making sure first 3 patches are renamed to "resctrl" wherever > applicable. Will send the patches soon. Please make sure they apply against git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache which has all the latest updates and fixes to this code. Thanks, tglx
Re: [PATCH v2 01/11] arch/x86: Start renaming the rdt files to more generic names
Babu, On Wed, 10 Oct 2018, Moger, Babu wrote: > On 10/09/2018 05:01 PM, Reinette Chatre wrote: > > As far as all the code you touch is concerned it may be easier and cause > > less confusion for now to just follow the current naming conventions as > > you have done in patches 3 onwards and have it be included in the later > > larger restructuring. > > Yes. I am making sure first 3 patches are renamed to "resctrl" wherever > applicable. Will send the patches soon. Please make sure they apply against git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache which has all the latest updates and fixes to this code. Thanks, tglx
[PATCH] ALSA: cs4236: fix return check from call to pnp_port_start
From: Colin Ian King
Currently the return from pnp_port_start is being checked for validity
by checking if it is greater or equal to zero, however, the return
is unsigned and hence this check is always true. An 0 return from
pnp_port_start is actually an error, so fix the check by checking
for validity when it is non-zero.
Detected by CoverityScan, CID#1468334 ("Unsigned compared to 0")
Fixes: a9824c868a2c ("[ALSA] Add CS4232 PnP BIOS support")
Signed-off-by: Colin Ian King
---
sound/isa/cs423x/cs4236.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/isa/cs423x/cs4236.c b/sound/isa/cs423x/cs4236.c
index 70559e59d18f..2211c39d2c18 100644
--- a/sound/isa/cs423x/cs4236.c
+++ b/sound/isa/cs423x/cs4236.c
@@ -292,7 +292,7 @@ static int snd_cs423x_pnp_init_mpu(int dev, struct pnp_dev
*pdev)
mpu_irq[dev] = SNDRV_AUTO_IRQ;
} else {
mpu_port[dev] = pnp_port_start(pdev, 0);
- if (mpu_irq[dev] >= 0 &&
+ if (!mpu_irq[dev] &&
pnp_irq_valid(pdev, 0) && pnp_irq(pdev, 0) >= 0) {
mpu_irq[dev] = pnp_irq(pdev, 0);
} else {
--
2.17.1
[PATCH] ALSA: cs4236: fix return check from call to pnp_port_start
From: Colin Ian King
Currently the return from pnp_port_start is being checked for validity
by checking if it is greater or equal to zero, however, the return
is unsigned and hence this check is always true. An 0 return from
pnp_port_start is actually an error, so fix the check by checking
for validity when it is non-zero.
Detected by CoverityScan, CID#1468334 ("Unsigned compared to 0")
Fixes: a9824c868a2c ("[ALSA] Add CS4232 PnP BIOS support")
Signed-off-by: Colin Ian King
---
sound/isa/cs423x/cs4236.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/isa/cs423x/cs4236.c b/sound/isa/cs423x/cs4236.c
index 70559e59d18f..2211c39d2c18 100644
--- a/sound/isa/cs423x/cs4236.c
+++ b/sound/isa/cs423x/cs4236.c
@@ -292,7 +292,7 @@ static int snd_cs423x_pnp_init_mpu(int dev, struct pnp_dev
*pdev)
mpu_irq[dev] = SNDRV_AUTO_IRQ;
} else {
mpu_port[dev] = pnp_port_start(pdev, 0);
- if (mpu_irq[dev] >= 0 &&
+ if (!mpu_irq[dev] &&
pnp_irq_valid(pdev, 0) && pnp_irq(pdev, 0) >= 0) {
mpu_irq[dev] = pnp_irq(pdev, 0);
} else {
--
2.17.1
Re: [RFC PATCH 0/7] Introduce thermal pressure
Hello Javi, Thanks for the interest. On 10/10/2018 01:44 AM, Javi Merino wrote: > On Tue, Oct 09, 2018 at 12:24:55PM -0400, Thara Gopinath wrote: >> Thermal governors can respond to an overheat event for a cpu by >> capping the cpu's maximum possible frequency. This in turn >> means that the maximum available compute capacity of the >> cpu is restricted. But today in linux kernel, in event of maximum >> frequency capping of a cpu, the maximum available compute >> capacity of the cpu is not adjusted at all. In other words, scheduler >> is unware maximum cpu capacity restrictions placed due to thermal >> activity. > > Interesting, I would have sworn that I tested this years ago by > lowering the maximum frequency of a cpufreq domain, and the scheduler > reacted accordingly to the new maximum capacities of the cpus. > >> This patch series attempts to address this issue. >> The benefits identified are better task placement among available >> cpus in event of overheating which in turn leads to better >> performance numbers. >> >> The delta between the maximum possible capacity of a cpu and >> maximum available capacity of a cpu due to thermal event can >> be considered as thermal pressure. Instantaneous thermal pressure >> is hard to record and can sometime be erroneous as there can be mismatch >> between the actual capping of capacity and scheduler recording it. >> Thus solution is to have a weighted average per cpu value for thermal >> pressure over time. The weight reflects the amount of time the cpu has >> spent at a capped maximum frequency. To accumulate, average and >> appropriately decay thermal pressure, this patch series uses pelt >> signals and reuses the available framework that does a similar >> bookkeeping of rt/dl task utilization. >> >> Regarding testing, basic build, boot and sanity testing have been >> performed on hikey960 mainline kernel with debian file system. >> Further aobench (An occlusion renderer for benchmarking realworld >> floating point performance) showed the following results on hikey960 >> with debain. >> >> Result Standard >> Standard >> (Time secs) Error >> Deviation >> Hikey 960 - no thermal pressure applied 138.67 6.52 >> 11.52% >> Hikey 960 - thermal pressure applied 122.37 5.78 >> 11.57% >> >> Thara Gopinath (7): >> sched/pelt: Add option to make load and util calculations frequency >> invariant >> sched/pelt.c: Add support to track thermal pressure >> sched: Add infrastructure to store and update instantaneous thermal >> pressure >> sched: Initialize per cpu thermal pressure structure >> sched/fair: Enable CFS periodic tick to update thermal pressure >> sched/fair: update cpu_capcity to reflect thermal pressure >> thermal/cpu-cooling: Update thermal pressure in case of a maximum >> frequency capping >> >> drivers/base/arch_topology.c | 1 + >> drivers/thermal/cpu_cooling.c | 20 - > > thermal? There are other ways in which the maximum frequency of a cpu > can be limited, for example from userspace via scaling_max_freq. Yes there are other ways in which maximum frequency of a cpu can be capped. The difference probably is that in case of a user-space cap, the time duration the cpu remains in the capped state is significantly higher than capping due to a thermal event. So may be the response of the scheduler should be different in that scenario (like rebuilding the capacities of all cpus etc). This patch series does not rebuild the scheduler structures. This just tells the scheduler that some cpu capacity is stolen. > > When something (anything) changes the maximum frequency of a cpufreq > policy, the scheduler should be notified. I think this change should > be done in cpufreq instead to make it generic and not particular to > a given maximum frequency "capper". I am glad to hear you say this! So far, all I have heard whenever I bring up this topic is issues with such an update from cpufreq(delays, lost updates etc). Personally, I have not seen these issues enough to comment on them. I will really like to hear more about these issues for an update from cpufreq here on the list. For me, the patch series is a mechanism to let scheduler know that a thermal event has stolen some cpu capacity. The update itself can happen from any framework which can track the event and we all mutually agree on. Regards Thara > > Cheers, > Javi > -- Regards Thara
Re: [RFC PATCH 0/7] Introduce thermal pressure
Hello Javi, Thanks for the interest. On 10/10/2018 01:44 AM, Javi Merino wrote: > On Tue, Oct 09, 2018 at 12:24:55PM -0400, Thara Gopinath wrote: >> Thermal governors can respond to an overheat event for a cpu by >> capping the cpu's maximum possible frequency. This in turn >> means that the maximum available compute capacity of the >> cpu is restricted. But today in linux kernel, in event of maximum >> frequency capping of a cpu, the maximum available compute >> capacity of the cpu is not adjusted at all. In other words, scheduler >> is unware maximum cpu capacity restrictions placed due to thermal >> activity. > > Interesting, I would have sworn that I tested this years ago by > lowering the maximum frequency of a cpufreq domain, and the scheduler > reacted accordingly to the new maximum capacities of the cpus. > >> This patch series attempts to address this issue. >> The benefits identified are better task placement among available >> cpus in event of overheating which in turn leads to better >> performance numbers. >> >> The delta between the maximum possible capacity of a cpu and >> maximum available capacity of a cpu due to thermal event can >> be considered as thermal pressure. Instantaneous thermal pressure >> is hard to record and can sometime be erroneous as there can be mismatch >> between the actual capping of capacity and scheduler recording it. >> Thus solution is to have a weighted average per cpu value for thermal >> pressure over time. The weight reflects the amount of time the cpu has >> spent at a capped maximum frequency. To accumulate, average and >> appropriately decay thermal pressure, this patch series uses pelt >> signals and reuses the available framework that does a similar >> bookkeeping of rt/dl task utilization. >> >> Regarding testing, basic build, boot and sanity testing have been >> performed on hikey960 mainline kernel with debian file system. >> Further aobench (An occlusion renderer for benchmarking realworld >> floating point performance) showed the following results on hikey960 >> with debain. >> >> Result Standard >> Standard >> (Time secs) Error >> Deviation >> Hikey 960 - no thermal pressure applied 138.67 6.52 >> 11.52% >> Hikey 960 - thermal pressure applied 122.37 5.78 >> 11.57% >> >> Thara Gopinath (7): >> sched/pelt: Add option to make load and util calculations frequency >> invariant >> sched/pelt.c: Add support to track thermal pressure >> sched: Add infrastructure to store and update instantaneous thermal >> pressure >> sched: Initialize per cpu thermal pressure structure >> sched/fair: Enable CFS periodic tick to update thermal pressure >> sched/fair: update cpu_capcity to reflect thermal pressure >> thermal/cpu-cooling: Update thermal pressure in case of a maximum >> frequency capping >> >> drivers/base/arch_topology.c | 1 + >> drivers/thermal/cpu_cooling.c | 20 - > > thermal? There are other ways in which the maximum frequency of a cpu > can be limited, for example from userspace via scaling_max_freq. Yes there are other ways in which maximum frequency of a cpu can be capped. The difference probably is that in case of a user-space cap, the time duration the cpu remains in the capped state is significantly higher than capping due to a thermal event. So may be the response of the scheduler should be different in that scenario (like rebuilding the capacities of all cpus etc). This patch series does not rebuild the scheduler structures. This just tells the scheduler that some cpu capacity is stolen. > > When something (anything) changes the maximum frequency of a cpufreq > policy, the scheduler should be notified. I think this change should > be done in cpufreq instead to make it generic and not particular to > a given maximum frequency "capper". I am glad to hear you say this! So far, all I have heard whenever I bring up this topic is issues with such an update from cpufreq(delays, lost updates etc). Personally, I have not seen these issues enough to comment on them. I will really like to hear more about these issues for an update from cpufreq here on the list. For me, the patch series is a mechanism to let scheduler know that a thermal event has stolen some cpu capacity. The update itself can happen from any framework which can track the event and we all mutually agree on. Regards Thara > > Cheers, > Javi > -- Regards Thara
[PATCH] pinctrl: madera: Fix uninitialized variable issue in madera_mux_set_mux
There is a potential execution path in which variable *ret* is checked
in an IF statement, and then its value is used to report an error at
line 659 without being properly initialized previously:
659 if (ret)
660 dev_err(priv->dev, "Failed to write to 0x%x (%d)\n", reg, ret);
Fix this by initializing variable *ret* to -1.
Addresses-Coverity-ID: 1471969 ("Uninitialized scalar variable")
Fixes: 218d72a77b0b ("pinctrl: madera: Add driver for Cirrus Logic Madera
codecs")
Signed-off-by: Gustavo A. R. Silva
---
drivers/pinctrl/cirrus/pinctrl-madera-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c
b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
index 0266302..a99ace8 100644
--- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c
+++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
@@ -608,7 +608,7 @@ static int madera_mux_set_mux(struct pinctrl_dev *pctldev,
unsigned int n_chip_groups = priv->chip->n_pin_groups;
const char *func_name = madera_mux_funcs[selector].name;
unsigned int reg;
- int i, ret;
+ int i, ret = -1;
dev_dbg(priv->dev, "%s selecting %u (%s) for group %u (%s)\n",
__func__, selector, func_name, group,
--
2.7.4
[PATCH] pinctrl: madera: Fix uninitialized variable issue in madera_mux_set_mux
There is a potential execution path in which variable *ret* is checked
in an IF statement, and then its value is used to report an error at
line 659 without being properly initialized previously:
659 if (ret)
660 dev_err(priv->dev, "Failed to write to 0x%x (%d)\n", reg, ret);
Fix this by initializing variable *ret* to -1.
Addresses-Coverity-ID: 1471969 ("Uninitialized scalar variable")
Fixes: 218d72a77b0b ("pinctrl: madera: Add driver for Cirrus Logic Madera
codecs")
Signed-off-by: Gustavo A. R. Silva
---
drivers/pinctrl/cirrus/pinctrl-madera-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c
b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
index 0266302..a99ace8 100644
--- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c
+++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
@@ -608,7 +608,7 @@ static int madera_mux_set_mux(struct pinctrl_dev *pctldev,
unsigned int n_chip_groups = priv->chip->n_pin_groups;
const char *func_name = madera_mux_funcs[selector].name;
unsigned int reg;
- int i, ret;
+ int i, ret = -1;
dev_dbg(priv->dev, "%s selecting %u (%s) for group %u (%s)\n",
__func__, selector, func_name, group,
--
2.7.4
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
> From: "Wesley W. Terpstra"
>
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 240
> +++
> 3 files changed, 251 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d2527..dd12144d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -378,6 +378,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + help
> + Generic PWM framework driver for SiFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sifive.
> +
> config PWM_SPEAR
> tristate "STMicroelectronics SPEAr PWM support"
> depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0d..30089ca6 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index ..99580025
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + */
> +#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Keep these in alphabetical order, please.
> +
> +#define MAX_PWM 4
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG24
> +#define BIT_PWM0_IP 28
> +
> +#define SIZE_PWMCMP 4
> +#define MASK_PWM_SCALE 0xf
> +
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem*regs;
> + unsigned intapprox_period;
> + unsigned intreal_period;
> +};
No need to align these. A single space is enough to separate variable
type and name.
> +
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip
> *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
> +
> + frac = ((u64)duty_cycle << 16) / state->period;
> + frac = min(frac, 0xU);
> +
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
> +
> + if (state->enabled) {
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + state->duty_cycle = state->period - state->duty_cycle;
> + }
> +
> + return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
> From: "Wesley W. Terpstra"
>
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 240
> +++
> 3 files changed, 251 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d2527..dd12144d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -378,6 +378,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + help
> + Generic PWM framework driver for SiFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sifive.
> +
> config PWM_SPEAR
> tristate "STMicroelectronics SPEAr PWM support"
> depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0d..30089ca6 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index ..99580025
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + */
> +#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Keep these in alphabetical order, please.
> +
> +#define MAX_PWM 4
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG24
> +#define BIT_PWM0_IP 28
> +
> +#define SIZE_PWMCMP 4
> +#define MASK_PWM_SCALE 0xf
> +
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem*regs;
> + unsigned intapprox_period;
> + unsigned intreal_period;
> +};
No need to align these. A single space is enough to separate variable
type and name.
> +
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip
> *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
> +
> + frac = ((u64)duty_cycle << 16) / state->period;
> + frac = min(frac, 0xU);
> +
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
> +
> + if (state->enabled) {
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + state->duty_cycle = state->period - state->duty_cycle;
> + }
> +
> + return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct
Re: [PATCH v2 01/11] arch/x86: Start renaming the rdt files to more generic names
Hi Reinette,
On 10/09/2018 05:01 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/9/2018 2:17 PM, Moger, Babu wrote:
>> On 10/09/2018 11:39 AM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 10/5/2018 1:55 PM, Moger, Babu wrote:
New generation of AMD processors start support RDT(or QOS) features.
With more than one vendors supporting these features, it seems more
appropriate to rename these files.
Signed-off-by: Babu Moger
---
arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h} | 0
arch/x86/kernel/cpu/Makefile| 6 +++---
arch/x86/kernel/cpu/{intel_rdt.c => rdt.c} | 4 ++--
arch/x86/kernel/cpu/{intel_rdt.h => rdt.h} | 0
.../cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c} | 2 +-
arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c} | 2 +-
.../cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c} | 6 +++---
...ntel_rdt_pseudo_lock_event.h => rdt_pseudo_lock_event.h} | 2 +-
.../x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} | 4 ++--
arch/x86/kernel/process_32.c| 2 +-
arch/x86/kernel/process_64.c| 2 +-
11 files changed, 15 insertions(+), 15 deletions(-)
rename arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h} (100%)
rename arch/x86/kernel/cpu/{intel_rdt.c => rdt.c} (99%)
rename arch/x86/kernel/cpu/{intel_rdt.h => rdt.h} (100%)
rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c}
(99%)
rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c} (99%)
rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c}
(99%)
rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h =>
rdt_pseudo_lock_event.h} (95%)
rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} (99%)
>>>
>>> During the RFC it was agreed that "resctrl" will be the neutral name and
>>> "intel_rdt", "amd_qos", or "arm mpam" would be the vendor specific names.
>>>
>>> It is ok to delay that renaming but I think any renaming done from this
>>> point should respect this agreement.
>>>
>>> For example, if you want to rename intel_rdt.c then please rename it to
>>> resctrl.c instead of just rdt.c which does not represent a generic name
>>> as expressed as a goal in the subject of this patch.
>>
>> I knew this was going to bit tricky. I can change all the places where I
>> am touching the code to generic names(change from intel_rdt to "resctrl").
>
> Yes, "intel_rdt" can be changed to the generic "resctrl" when it is not
> vendor specific.
Ok. sure.
>
> As far as all the code you touch is concerned it may be easier and cause
> less confusion for now to just follow the current naming conventions as
> you have done in patches 3 onwards and have it be included in the later
> larger restructuring.
Yes. I am making sure first 3 patches are renamed to "resctrl" wherever
applicable. Will send the patches soon.
But I am confused about what you meant by "have it be included in the
later larger restructuring". Can you please elaborate?
>
>> Also lets change the "texts" which are visible to user to make it more
>> generic.
>
> Could you please elaborate what you mean with "texts" here? Are you
> referring to the pr_info() found in intel_rdt_late_init()? Here it may
> be good to also change to print "RESCTRL %s allocation
> detected"/"RESCTRL %s monitoring detected" - the resource names printed
> are already generic.
Yes. I meant pr_info text. Will change it to print "RESCTRL"
>
>> But "rdt" has been used generously in multiple files(like rdt_resource,
>> rdt_domain etc). Changing those definitions and functions will be messier.
>> I will not worry about it now. Thoughts?
>
> I agree. My comments were specific to the first two patches of this
> series that started doing the renaming but not using the agreed upon
> naming - especially since both those patches claim to transition to
> generic names. Could just these two patches be modified to change
> "intel_rdt" to "resctrl" instead of "intel_rdt" to "rdt" as it currently
> does?
Sure. Will take care of that.
>
> Reinette
>
>
Re: [PATCH v2 01/11] arch/x86: Start renaming the rdt files to more generic names
Hi Reinette,
On 10/09/2018 05:01 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/9/2018 2:17 PM, Moger, Babu wrote:
>> On 10/09/2018 11:39 AM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 10/5/2018 1:55 PM, Moger, Babu wrote:
New generation of AMD processors start support RDT(or QOS) features.
With more than one vendors supporting these features, it seems more
appropriate to rename these files.
Signed-off-by: Babu Moger
---
arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h} | 0
arch/x86/kernel/cpu/Makefile| 6 +++---
arch/x86/kernel/cpu/{intel_rdt.c => rdt.c} | 4 ++--
arch/x86/kernel/cpu/{intel_rdt.h => rdt.h} | 0
.../cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c} | 2 +-
arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c} | 2 +-
.../cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c} | 6 +++---
...ntel_rdt_pseudo_lock_event.h => rdt_pseudo_lock_event.h} | 2 +-
.../x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} | 4 ++--
arch/x86/kernel/process_32.c| 2 +-
arch/x86/kernel/process_64.c| 2 +-
11 files changed, 15 insertions(+), 15 deletions(-)
rename arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h} (100%)
rename arch/x86/kernel/cpu/{intel_rdt.c => rdt.c} (99%)
rename arch/x86/kernel/cpu/{intel_rdt.h => rdt.h} (100%)
rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c}
(99%)
rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c} (99%)
rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c}
(99%)
rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h =>
rdt_pseudo_lock_event.h} (95%)
rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} (99%)
>>>
>>> During the RFC it was agreed that "resctrl" will be the neutral name and
>>> "intel_rdt", "amd_qos", or "arm mpam" would be the vendor specific names.
>>>
>>> It is ok to delay that renaming but I think any renaming done from this
>>> point should respect this agreement.
>>>
>>> For example, if you want to rename intel_rdt.c then please rename it to
>>> resctrl.c instead of just rdt.c which does not represent a generic name
>>> as expressed as a goal in the subject of this patch.
>>
>> I knew this was going to bit tricky. I can change all the places where I
>> am touching the code to generic names(change from intel_rdt to "resctrl").
>
> Yes, "intel_rdt" can be changed to the generic "resctrl" when it is not
> vendor specific.
Ok. sure.
>
> As far as all the code you touch is concerned it may be easier and cause
> less confusion for now to just follow the current naming conventions as
> you have done in patches 3 onwards and have it be included in the later
> larger restructuring.
Yes. I am making sure first 3 patches are renamed to "resctrl" wherever
applicable. Will send the patches soon.
But I am confused about what you meant by "have it be included in the
later larger restructuring". Can you please elaborate?
>
>> Also lets change the "texts" which are visible to user to make it more
>> generic.
>
> Could you please elaborate what you mean with "texts" here? Are you
> referring to the pr_info() found in intel_rdt_late_init()? Here it may
> be good to also change to print "RESCTRL %s allocation
> detected"/"RESCTRL %s monitoring detected" - the resource names printed
> are already generic.
Yes. I meant pr_info text. Will change it to print "RESCTRL"
>
>> But "rdt" has been used generously in multiple files(like rdt_resource,
>> rdt_domain etc). Changing those definitions and functions will be messier.
>> I will not worry about it now. Thoughts?
>
> I agree. My comments were specific to the first two patches of this
> series that started doing the renaming but not using the agreed upon
> naming - especially since both those patches claim to transition to
> generic names. Could just these two patches be modified to change
> "intel_rdt" to "resctrl" instead of "intel_rdt" to "rdt" as it currently
> does?
Sure. Will take care of that.
>
> Reinette
>
>
Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote: > Currently, there are some values assigned to variable *rc*, which > are never actually used in any computation, because such variable > is updated at line 550, before they can be used: > > 549out: > 550rc = tpm_go_idle(chip, flags); > 551if (rc) > 552goto out; > > Fix this by removing such assignments. Should this be done by not quashing rc during the error unwind rather than dropping the errors? Jason
Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote: > Currently, there are some values assigned to variable *rc*, which > are never actually used in any computation, because such variable > is updated at line 550, before they can be used: > > 549out: > 550rc = tpm_go_idle(chip, flags); > 551if (rc) > 552goto out; > > Fix this by removing such assignments. Should this be done by not quashing rc during the error unwind rather than dropping the errors? Jason
Re: [PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
On 10/10/18, Geert Uytterhoeven wrote: > Currently EEPROM writes are implemented using a single SPI transfer, > which contains all of command, address, and payload data bytes. > As some SPI controllers impose limitations on transfers with respect to > the use of DMA, they may have to fall back to PIO. E.g. DMA may require > the transfer length to be a multiple of 4 bytes. > > Optimize writes for DMA by splitting writes in two SPI transfers: > - The first transfer contains command and address bytes, > - The second transfer contains the actual payload data, now stored at > the start of the (kmalloc() aligned) buffer, to improve payload > alignment. > > E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write > aligned to the page size was transferred using an 18-byte write. > After this change, the write is split in a 2-byte and an aligned 16-byte > write. > > Note that EEPROM reads already use a similar scheme, due to the > different data directions for command and address bytes versus payload > data. > > Signed-off-by: Geert Uytterhoeven Acked-by: Arnd Bergmann
Re: [PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
On 10/10/18, Geert Uytterhoeven wrote: > Currently EEPROM writes are implemented using a single SPI transfer, > which contains all of command, address, and payload data bytes. > As some SPI controllers impose limitations on transfers with respect to > the use of DMA, they may have to fall back to PIO. E.g. DMA may require > the transfer length to be a multiple of 4 bytes. > > Optimize writes for DMA by splitting writes in two SPI transfers: > - The first transfer contains command and address bytes, > - The second transfer contains the actual payload data, now stored at > the start of the (kmalloc() aligned) buffer, to improve payload > alignment. > > E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write > aligned to the page size was transferred using an 18-byte write. > After this change, the write is split in a 2-byte and an aligned 16-byte > write. > > Note that EEPROM reads already use a similar scheme, due to the > different data directions for command and address bytes versus payload > data. > > Signed-off-by: Geert Uytterhoeven Acked-by: Arnd Bergmann
Re: [PATCH 3/4] mm: workingset: add vmstat counter for shadow nodes
On Tue, Oct 09, 2018 at 03:04:01PM -0700, Andrew Morton wrote:
> On Tue, 9 Oct 2018 14:47:32 -0400 Johannes Weiner wrote:
>
> > Make it easier to catch bugs in the shadow node shrinker by adding a
> > counter for the shadow nodes in circulation.
> >
> > Signed-off-by: Johannes Weiner
> > ---
> > include/linux/mmzone.h | 1 +
> > mm/vmstat.c| 1 +
> > mm/workingset.c| 12 ++--
> > 3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 4179e67add3d..d82e80d82aa6 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -161,6 +161,7 @@ enum node_stat_item {
> > NR_SLAB_UNRECLAIMABLE,
> > NR_ISOLATED_ANON, /* Temporary isolated pages from anon lru */
> > NR_ISOLATED_FILE, /* Temporary isolated pages from file lru */
> > + WORKINGSET_NODES,
>
> Documentation/admin-guide/cgroup-v2.rst, please. And please check for
> any other missing items while in there?
The new counter isn't being added to the per-cgroup memory.stat,
actually, it just shows in /proc/vmstat.
It seemed a bit too low-level for the cgroup interface, and the other
stats in there are in bytes, which isn't straight-forward to calculate
with sl*b packing.
Not that I'm against adding a cgroup breakdown in general, but the
global counter was enough to see if things were working right or not,
so I'd cross that bridge when somebody needs it per cgroup.
But I checked cgroup-v2.rst anyway: all the exported items are
documented. Only the reclaim vs. refault stats were in different
orders: the doc has the refault stats first, the interface leads with
the reclaim stats. The refault stats go better with the page fault
stats, and are probably of more interest (since they have higher
impact on performance) than the LRU shuffling, so maybe this?
---
Subject: [PATCH] mm: memcontrol: fix memory.stat item ordering
The refault stats go better with the page fault stats, and are of
higher interest than the stats on LRU operations. In fact they used to
be grouped together; when the LRU operation stats were added later on,
they were wedged in between.
Move them back together. Documentation/admin-guide/cgroup-v2.rst
already lists them in the right order.
Signed-off-by: Johannes Weiner
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 81b47d0b14d7..ed15f233d31d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5575,6 +5575,13 @@ static int memory_stat_show(struct seq_file *m, void *v)
seq_printf(m, "pgfault %lu\n", acc.events[PGFAULT]);
seq_printf(m, "pgmajfault %lu\n", acc.events[PGMAJFAULT]);
+ seq_printf(m, "workingset_refault %lu\n",
+ acc.stat[WORKINGSET_REFAULT]);
+ seq_printf(m, "workingset_activate %lu\n",
+ acc.stat[WORKINGSET_ACTIVATE]);
+ seq_printf(m, "workingset_nodereclaim %lu\n",
+ acc.stat[WORKINGSET_NODERECLAIM]);
+
seq_printf(m, "pgrefill %lu\n", acc.events[PGREFILL]);
seq_printf(m, "pgscan %lu\n", acc.events[PGSCAN_KSWAPD] +
acc.events[PGSCAN_DIRECT]);
@@ -5585,13 +5592,6 @@ static int memory_stat_show(struct seq_file *m, void *v)
seq_printf(m, "pglazyfree %lu\n", acc.events[PGLAZYFREE]);
seq_printf(m, "pglazyfreed %lu\n", acc.events[PGLAZYFREED]);
- seq_printf(m, "workingset_refault %lu\n",
- acc.stat[WORKINGSET_REFAULT]);
- seq_printf(m, "workingset_activate %lu\n",
- acc.stat[WORKINGSET_ACTIVATE]);
- seq_printf(m, "workingset_nodereclaim %lu\n",
- acc.stat[WORKINGSET_NODERECLAIM]);
-
return 0;
}
Re: [PATCH 3/4] mm: workingset: add vmstat counter for shadow nodes
On Tue, Oct 09, 2018 at 03:04:01PM -0700, Andrew Morton wrote:
> On Tue, 9 Oct 2018 14:47:32 -0400 Johannes Weiner wrote:
>
> > Make it easier to catch bugs in the shadow node shrinker by adding a
> > counter for the shadow nodes in circulation.
> >
> > Signed-off-by: Johannes Weiner
> > ---
> > include/linux/mmzone.h | 1 +
> > mm/vmstat.c| 1 +
> > mm/workingset.c| 12 ++--
> > 3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 4179e67add3d..d82e80d82aa6 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -161,6 +161,7 @@ enum node_stat_item {
> > NR_SLAB_UNRECLAIMABLE,
> > NR_ISOLATED_ANON, /* Temporary isolated pages from anon lru */
> > NR_ISOLATED_FILE, /* Temporary isolated pages from file lru */
> > + WORKINGSET_NODES,
>
> Documentation/admin-guide/cgroup-v2.rst, please. And please check for
> any other missing items while in there?
The new counter isn't being added to the per-cgroup memory.stat,
actually, it just shows in /proc/vmstat.
It seemed a bit too low-level for the cgroup interface, and the other
stats in there are in bytes, which isn't straight-forward to calculate
with sl*b packing.
Not that I'm against adding a cgroup breakdown in general, but the
global counter was enough to see if things were working right or not,
so I'd cross that bridge when somebody needs it per cgroup.
But I checked cgroup-v2.rst anyway: all the exported items are
documented. Only the reclaim vs. refault stats were in different
orders: the doc has the refault stats first, the interface leads with
the reclaim stats. The refault stats go better with the page fault
stats, and are probably of more interest (since they have higher
impact on performance) than the LRU shuffling, so maybe this?
---
Subject: [PATCH] mm: memcontrol: fix memory.stat item ordering
The refault stats go better with the page fault stats, and are of
higher interest than the stats on LRU operations. In fact they used to
be grouped together; when the LRU operation stats were added later on,
they were wedged in between.
Move them back together. Documentation/admin-guide/cgroup-v2.rst
already lists them in the right order.
Signed-off-by: Johannes Weiner
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 81b47d0b14d7..ed15f233d31d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5575,6 +5575,13 @@ static int memory_stat_show(struct seq_file *m, void *v)
seq_printf(m, "pgfault %lu\n", acc.events[PGFAULT]);
seq_printf(m, "pgmajfault %lu\n", acc.events[PGMAJFAULT]);
+ seq_printf(m, "workingset_refault %lu\n",
+ acc.stat[WORKINGSET_REFAULT]);
+ seq_printf(m, "workingset_activate %lu\n",
+ acc.stat[WORKINGSET_ACTIVATE]);
+ seq_printf(m, "workingset_nodereclaim %lu\n",
+ acc.stat[WORKINGSET_NODERECLAIM]);
+
seq_printf(m, "pgrefill %lu\n", acc.events[PGREFILL]);
seq_printf(m, "pgscan %lu\n", acc.events[PGSCAN_KSWAPD] +
acc.events[PGSCAN_DIRECT]);
@@ -5585,13 +5592,6 @@ static int memory_stat_show(struct seq_file *m, void *v)
seq_printf(m, "pglazyfree %lu\n", acc.events[PGLAZYFREE]);
seq_printf(m, "pglazyfreed %lu\n", acc.events[PGLAZYFREED]);
- seq_printf(m, "workingset_refault %lu\n",
- acc.stat[WORKINGSET_REFAULT]);
- seq_printf(m, "workingset_activate %lu\n",
- acc.stat[WORKINGSET_ACTIVATE]);
- seq_printf(m, "workingset_nodereclaim %lu\n",
- acc.stat[WORKINGSET_NODERECLAIM]);
-
return 0;
}
Re: [PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
On 10/10/18, Geert Uytterhoeven wrote: > Reduce code duplication in at25_ee_read() by using the > spi_message_init_with_transfers() helper. > > Signed-off-by: Geert Uytterhoeven Acked-by: Arnd Bergmann
Re: [PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
On 10/10/18, Geert Uytterhoeven wrote: > Reduce code duplication in at25_ee_read() by using the > spi_message_init_with_transfers() helper. > > Signed-off-by: Geert Uytterhoeven Acked-by: Arnd Bergmann
Re: [PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
On 10/10/18, Geert Uytterhoeven wrote:
> Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
> dependency") changed the type of "off" from "loff_t" to "unsigned int",
> there is no longer a need to cast it to "unsigned".
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/misc/eeprom/at25.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 840afb398f9e4c88..7707d3fb49b526d9 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off,
> void *val, size_t count)
> do {
> unsigned long timeout, retries;
> unsignedsegment;
> - unsignedoffset = (unsigned) off;
> + unsignedoffset = off;
> u8 *cp = bounce;
> int sr;
> u8 instr;
> --
This looks correct, but why not remove the extra variable completely
and rename 'off' to 'offset'?
Arnd
Re: [PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
On 10/10/18, Geert Uytterhoeven wrote:
> Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
> dependency") changed the type of "off" from "loff_t" to "unsigned int",
> there is no longer a need to cast it to "unsigned".
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/misc/eeprom/at25.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 840afb398f9e4c88..7707d3fb49b526d9 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off,
> void *val, size_t count)
> do {
> unsigned long timeout, retries;
> unsignedsegment;
> - unsignedoffset = (unsigned) off;
> + unsignedoffset = off;
> u8 *cp = bounce;
> int sr;
> u8 instr;
> --
This looks correct, but why not remove the extra variable completely
and rename 'off' to 'offset'?
Arnd
RE: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
>
> On Wed, Oct 10, 2018 at 4:43 AM Avri Altman wrote:
> >
> >
> > > +++ b/mmc_cmds.c
> > > @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> > > }
> > >
> > > if (ext_csd_rev >= 7) {
> > > - printf("eMMC Firmware Version: %s\n",
> > > - (char*)_csd[EXT_CSD_FIRMWARE_VERSION]);
> > > + printf("Firmware Version:
> > > 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
> > ExtCSD[261:254] is an ASCII string, just add a terminating null.
>
> Unfortunately I found two different manufacturers which put
> non-printable characters in this 8-byte field. So I don't think it can
> be treated as ASCII in all cases. Printing out the hex value seemed
> liked the most comprehensive solution.
NAK with prejudice.
This interfere with the output that we/our clients expects.
RE: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
>
> On Wed, Oct 10, 2018 at 4:43 AM Avri Altman wrote:
> >
> >
> > > +++ b/mmc_cmds.c
> > > @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> > > }
> > >
> > > if (ext_csd_rev >= 7) {
> > > - printf("eMMC Firmware Version: %s\n",
> > > - (char*)_csd[EXT_CSD_FIRMWARE_VERSION]);
> > > + printf("Firmware Version:
> > > 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
> > ExtCSD[261:254] is an ASCII string, just add a terminating null.
>
> Unfortunately I found two different manufacturers which put
> non-printable characters in this 8-byte field. So I don't think it can
> be treated as ASCII in all cases. Printing out the hex value seemed
> liked the most comprehensive solution.
NAK with prejudice.
This interfere with the output that we/our clients expects.
Re: [PATCH] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
On Tue, Oct 09, 2018 at 03:13:03PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 04, 2018 at 06:52:25PM +0100, Laurence Rochfort wrote:
> > Silence the below sparse warnings by casting betwen u8 __iomem *, and
> > void *.
> >
> > warning: incorrect type in assignment (different address spaces)
> >expected unsigned char [noderef] [usertype] *virt_base
> >got void *[assigned] mem
> > warning: incorrect type in argument 3 (different address spaces)
> >expected void *cpu_addr
> >got unsigned char [noderef] [usertype] *virt_base
> >
> > Signed-off-by: Laurence Rochfort
> > ---
> > drivers/staging/gasket/gasket_page_table.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/gasket_page_table.c
> > b/drivers/staging/gasket/gasket_page_table.c
> > index d4c5f8a..7f5e5b3 100644
> > --- a/drivers/staging/gasket/gasket_page_table.c
> > +++ b/drivers/staging/gasket/gasket_page_table.c
> > @@ -1278,7 +1278,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > dma_addr_t *dma_address, u64 index)
> > {
> > dma_addr_t handle;
> > - void *mem;
> > + u8 __iomem *mem;
> > int j;
> > unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> > const struct gasket_driver_desc *driver_desc =
> > @@ -1290,7 +1290,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > if (num_pages == 0)
> > return -EINVAL;
> >
> > - mem = dma_alloc_coherent(gasket_get_device(gasket_dev),
> > + mem = (u8 __iomem *)dma_alloc_coherent(gasket_get_device(gasket_dev),
> > num_pages * PAGE_SIZE, , 0);
> > if (!mem)
> > goto nomem;
> > @@ -1325,7 +1325,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > nomem:
> > if (mem) {
> > dma_free_coherent(gasket_get_device(gasket_dev),
> > - num_pages * PAGE_SIZE, mem, handle);
> > + num_pages * PAGE_SIZE, (void __force *)mem,
> > handle);
> > }
> >
> > if (gasket_dev->page_table[index]->coherent_pages) {
> > @@ -1353,7 +1353,7 @@ int gasket_free_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > if (gasket_dev->coherent_buffer.length_bytes) {
> > dma_free_coherent(gasket_get_device(gasket_dev),
> > gasket_dev->coherent_buffer.length_bytes,
> > - gasket_dev->coherent_buffer.virt_base,
> > + (void __force
> > *)gasket_dev->coherent_buffer.virt_base,
>
> We should not have to "force" anything here. Something feels wrong in
> the definition of these variable types...
>From bde80ff6bdab740d838210aa3e664c8901b2142a Mon Sep 17 00:00:00 2001
From: Laurence Rochfort
Date: Thu, 4 Oct 2018 18:42:26 +0100
Subject: [PATCH v2] staging: gasket: Fix sparse "incorrect type in assignment"
warnings.
Correct the coherent buffer data type and memory space cookie.
sparse errors were being generated when passing virt_base to
dma_alloc_coherent().
warning: incorrect type in assignment (different address spaces)
expected unsigned char [noderef] [usertype] *virt_base
got void *[assigned] mem
warning: incorrect type in argument 3 (different address spaces)
expected void *cpu_addr
got unsigned char [noderef] [usertype] *virt_base
Signed-off-by: Laurence Rochfort
---
Changes in v2:
- Correct data type rather than forcing type coersion, as per gkh
drivers/staging/gasket/gasket_core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/gasket/gasket_core.h
b/drivers/staging/gasket/gasket_core.h
index 275fd0b..9d09678 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -231,7 +231,7 @@ struct gasket_coherent_buffer_desc {
/* Coherent buffer structure. */
struct gasket_coherent_buffer {
/* Virtual base address. */
- u8 __iomem *virt_base;
+ void *virt_base;
/* Physical base address. */
ulong phys_base;
--
2.9.5
Re: [PATCH] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
On Tue, Oct 09, 2018 at 03:13:03PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 04, 2018 at 06:52:25PM +0100, Laurence Rochfort wrote:
> > Silence the below sparse warnings by casting betwen u8 __iomem *, and
> > void *.
> >
> > warning: incorrect type in assignment (different address spaces)
> >expected unsigned char [noderef] [usertype] *virt_base
> >got void *[assigned] mem
> > warning: incorrect type in argument 3 (different address spaces)
> >expected void *cpu_addr
> >got unsigned char [noderef] [usertype] *virt_base
> >
> > Signed-off-by: Laurence Rochfort
> > ---
> > drivers/staging/gasket/gasket_page_table.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/gasket_page_table.c
> > b/drivers/staging/gasket/gasket_page_table.c
> > index d4c5f8a..7f5e5b3 100644
> > --- a/drivers/staging/gasket/gasket_page_table.c
> > +++ b/drivers/staging/gasket/gasket_page_table.c
> > @@ -1278,7 +1278,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > dma_addr_t *dma_address, u64 index)
> > {
> > dma_addr_t handle;
> > - void *mem;
> > + u8 __iomem *mem;
> > int j;
> > unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> > const struct gasket_driver_desc *driver_desc =
> > @@ -1290,7 +1290,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > if (num_pages == 0)
> > return -EINVAL;
> >
> > - mem = dma_alloc_coherent(gasket_get_device(gasket_dev),
> > + mem = (u8 __iomem *)dma_alloc_coherent(gasket_get_device(gasket_dev),
> > num_pages * PAGE_SIZE, , 0);
> > if (!mem)
> > goto nomem;
> > @@ -1325,7 +1325,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > nomem:
> > if (mem) {
> > dma_free_coherent(gasket_get_device(gasket_dev),
> > - num_pages * PAGE_SIZE, mem, handle);
> > + num_pages * PAGE_SIZE, (void __force *)mem,
> > handle);
> > }
> >
> > if (gasket_dev->page_table[index]->coherent_pages) {
> > @@ -1353,7 +1353,7 @@ int gasket_free_coherent_memory(struct gasket_dev
> > *gasket_dev, u64 size,
> > if (gasket_dev->coherent_buffer.length_bytes) {
> > dma_free_coherent(gasket_get_device(gasket_dev),
> > gasket_dev->coherent_buffer.length_bytes,
> > - gasket_dev->coherent_buffer.virt_base,
> > + (void __force
> > *)gasket_dev->coherent_buffer.virt_base,
>
> We should not have to "force" anything here. Something feels wrong in
> the definition of these variable types...
>From bde80ff6bdab740d838210aa3e664c8901b2142a Mon Sep 17 00:00:00 2001
From: Laurence Rochfort
Date: Thu, 4 Oct 2018 18:42:26 +0100
Subject: [PATCH v2] staging: gasket: Fix sparse "incorrect type in assignment"
warnings.
Correct the coherent buffer data type and memory space cookie.
sparse errors were being generated when passing virt_base to
dma_alloc_coherent().
warning: incorrect type in assignment (different address spaces)
expected unsigned char [noderef] [usertype] *virt_base
got void *[assigned] mem
warning: incorrect type in argument 3 (different address spaces)
expected void *cpu_addr
got unsigned char [noderef] [usertype] *virt_base
Signed-off-by: Laurence Rochfort
---
Changes in v2:
- Correct data type rather than forcing type coersion, as per gkh
drivers/staging/gasket/gasket_core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/gasket/gasket_core.h
b/drivers/staging/gasket/gasket_core.h
index 275fd0b..9d09678 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -231,7 +231,7 @@ struct gasket_coherent_buffer_desc {
/* Coherent buffer structure. */
struct gasket_coherent_buffer {
/* Virtual base address. */
- u8 __iomem *virt_base;
+ void *virt_base;
/* Physical base address. */
ulong phys_base;
--
2.9.5
Re: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
On Wed, Oct 10, 2018 at 4:43 AM Avri Altman wrote:
>
>
> > +++ b/mmc_cmds.c
> > @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> > }
> >
> > if (ext_csd_rev >= 7) {
> > - printf("eMMC Firmware Version: %s\n",
> > - (char*)_csd[EXT_CSD_FIRMWARE_VERSION]);
> > + printf("Firmware Version:
> > 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
> ExtCSD[261:254] is an ASCII string, just add a terminating null.
Unfortunately I found two different manufacturers which put
non-printable characters in this 8-byte field. So I don't think it can
be treated as ASCII in all cases. Printing out the hex value seemed
liked the most comprehensive solution.
>
> Thanks,
> Avri
>
--
--
This message is intended exclusively for the individual or entity to which
it is addressed. This communication may contain information that is
proprietary, privileged, confidential or otherwise legally exempt from
disclosure. If you are not the named addressee, or have been inadvertently
and erroneously referenced in the address line, you are not authorized to
read, print, retain, copy or disseminate this message or any part of it. If
you have received this message in error, please notify the sender
immediately by e-mail and delete all copies of the message.
Re: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
On Wed, Oct 10, 2018 at 4:43 AM Avri Altman wrote:
>
>
> > +++ b/mmc_cmds.c
> > @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> > }
> >
> > if (ext_csd_rev >= 7) {
> > - printf("eMMC Firmware Version: %s\n",
> > - (char*)_csd[EXT_CSD_FIRMWARE_VERSION]);
> > + printf("Firmware Version:
> > 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> > + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
> ExtCSD[261:254] is an ASCII string, just add a terminating null.
Unfortunately I found two different manufacturers which put
non-printable characters in this 8-byte field. So I don't think it can
be treated as ASCII in all cases. Printing out the hex value seemed
liked the most comprehensive solution.
>
> Thanks,
> Avri
>
--
--
This message is intended exclusively for the individual or entity to which
it is addressed. This communication may contain information that is
proprietary, privileged, confidential or otherwise legally exempt from
disclosure. If you are not the named addressee, or have been inadvertently
and erroneously referenced in the address line, you are not authorized to
read, print, retain, copy or disseminate this message or any part of it. If
you have received this message in error, please notify the sender
immediately by e-mail and delete all copies of the message.
Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote: [...] > +- interrupts: one interrupt per PWM channel (currently unused in the driver) This should probably say what the interrupt is used for. And once you have that, remove the comment about it being unused in the driver. DT is OS agnostic, so "driver" is very unspecific and your claim may actually be false. Thierry signature.asc Description: PGP signature
Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote: [...] > +- interrupts: one interrupt per PWM channel (currently unused in the driver) This should probably say what the interrupt is used for. And once you have that, remove the comment about it being unused in the driver. DT is OS agnostic, so "driver" is very unspecific and your claim may actually be false. Thierry signature.asc Description: PGP signature
Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote: > From: "Wesley W. Terpstra" > > DT documentation for PWM controller added with updated compatible > string. > > Signed-off-by: Wesley W. Terpstra > [Atish: Compatible string update] > Signed-off-by: Atish Patra > --- > .../devicetree/bindings/pwm/pwm-sifive.txt | 32 > ++ > 1 file changed, 32 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > new file mode 100644 > index ..532b10fc > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > @@ -0,0 +1,32 @@ > +SiFive PWM controller > + > +Unlike most other PWM controllers, the SiFive PWM controller currently only > +supports one period for all channels in the PWM. This is set globally in DTS. > +The period also has significant restrictions on the values it can achieve, > +which the driver rounds to the nearest achievable frequency. What restrictions are these? If "nearest achievable" is too far off the target period it might be preferable to error out. > +Required properties: > +- compatible: should be one of > + "sifive,fu540-c000-pwm0","sifive,pwm0". What's the '0' in here? A version number? > + PWM controller is HiFive Unleashed specific chip which warrants a > + specific compatible string. The second string is kept for backward > + compatibility until a firmware update with latest compatible string. > +- reg: physical base address and length of the controller's registers > +- clocks: The frequency the controller runs at > +- #pwm-cells: Should be 2. > + The first cell is the PWM channel number > + The second cell is the PWM polarity > +- sifive,approx-period: the driver will get as close to this period as it can Given the above comment, maybe "sifive,period"? Thierry signature.asc Description: PGP signature
Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote: > From: "Wesley W. Terpstra" > > DT documentation for PWM controller added with updated compatible > string. > > Signed-off-by: Wesley W. Terpstra > [Atish: Compatible string update] > Signed-off-by: Atish Patra > --- > .../devicetree/bindings/pwm/pwm-sifive.txt | 32 > ++ > 1 file changed, 32 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > new file mode 100644 > index ..532b10fc > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > @@ -0,0 +1,32 @@ > +SiFive PWM controller > + > +Unlike most other PWM controllers, the SiFive PWM controller currently only > +supports one period for all channels in the PWM. This is set globally in DTS. > +The period also has significant restrictions on the values it can achieve, > +which the driver rounds to the nearest achievable frequency. What restrictions are these? If "nearest achievable" is too far off the target period it might be preferable to error out. > +Required properties: > +- compatible: should be one of > + "sifive,fu540-c000-pwm0","sifive,pwm0". What's the '0' in here? A version number? > + PWM controller is HiFive Unleashed specific chip which warrants a > + specific compatible string. The second string is kept for backward > + compatibility until a firmware update with latest compatible string. > +- reg: physical base address and length of the controller's registers > +- clocks: The frequency the controller runs at > +- #pwm-cells: Should be 2. > + The first cell is the PWM channel number > + The second cell is the PWM polarity > +- sifive,approx-period: the driver will get as close to this period as it can Given the above comment, maybe "sifive,period"? Thierry signature.asc Description: PGP signature
Re: [RFD/RFC PATCH 0/8] Towards implementing proxy execution
On 10/10/18 2:24 PM, Peter Zijlstra wrote: >> I believe there were some papers circulated last year that looked at >> something similar to this when you had overlapping or completely disjoint >> CPUsets I think it would be nice to drag into the discussion. Has this been >> considered? (if so, sorry for adding line-noise!) > Hurm, was that one of Bjorn's papers? Didn't that deal with AC of > disjoint/overlapping sets? > This paper: https://people.mpi-sws.org/~bbb/papers/pdf/rtsj14.pdf But, unless I am wrong, there were findings after this paper that shows some imprecision on it. Anyway, it does not analyse the locking properties, only scheduler of independent tasks - it is a start, but far from what we do here. (btw this paper is really complex...) The locking problem for such case: APA with the nesting of different locks in the locking implementation (we use raw spin lock on this, and this method could also be used in the rw lock/sem in the future, nesting rw_lock(mutex_proxy(raw_spinlock())) is an open problem from the academic point of view. I explained these things (nested lock and the need of APA for locking) as "Open Problems" at the RTSOPs (part of the ECRTS) earlier this year: http://rtsops2018.loria.fr/wp-content/uploads/2018/07/RTSOPS18_proceedings_final.pdf Bjorn were there, but not only him... Baruah, Davis, Alan Burns were there. There are some works being done for more complex locking in academy, like: https://www.cs.unc.edu/~jarretc/papers/ecrts18b_long.pdf But still, the task model used on these implementations are not the Linux one. -- Daniel
Re: [RFD/RFC PATCH 0/8] Towards implementing proxy execution
On 10/10/18 2:24 PM, Peter Zijlstra wrote: >> I believe there were some papers circulated last year that looked at >> something similar to this when you had overlapping or completely disjoint >> CPUsets I think it would be nice to drag into the discussion. Has this been >> considered? (if so, sorry for adding line-noise!) > Hurm, was that one of Bjorn's papers? Didn't that deal with AC of > disjoint/overlapping sets? > This paper: https://people.mpi-sws.org/~bbb/papers/pdf/rtsj14.pdf But, unless I am wrong, there were findings after this paper that shows some imprecision on it. Anyway, it does not analyse the locking properties, only scheduler of independent tasks - it is a start, but far from what we do here. (btw this paper is really complex...) The locking problem for such case: APA with the nesting of different locks in the locking implementation (we use raw spin lock on this, and this method could also be used in the rw lock/sem in the future, nesting rw_lock(mutex_proxy(raw_spinlock())) is an open problem from the academic point of view. I explained these things (nested lock and the need of APA for locking) as "Open Problems" at the RTSOPs (part of the ECRTS) earlier this year: http://rtsops2018.loria.fr/wp-content/uploads/2018/07/RTSOPS18_proceedings_final.pdf Bjorn were there, but not only him... Baruah, Davis, Alan Burns were there. There are some works being done for more complex locking in academy, like: https://www.cs.unc.edu/~jarretc/papers/ecrts18b_long.pdf But still, the task model used on these implementations are not the Linux one. -- Daniel
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wednesday 10 Oct 2018 at 15:27:57 (+0200), Vincent Guittot wrote: > On Wed, 10 Oct 2018 at 15:05, Quentin Perret wrote: > > > > On Wednesday 10 Oct 2018 at 14:04:40 (+0200), Vincent Guittot wrote: > > > This patchset doesn't touch cpu_capacity_orig and doesn't need to as > > > it assume that the max capacity is unchanged but some capacity is > > > momentary stolen by thermal. > > > If you want to reflect immediately all thermal capping change, you > > > have to update this field and all related fields and struct around > > > > I don't follow you here. I never said I wanted to change > > cpu_capacity_orig. I don't think we should do that actually. Changing > > capacity_of (which is updated during LB IIRC) is just fine. The question > > is about what you want to do there: reflect an averaged value or the > > instantaneous one. > > Sorry I though your were speaking about updating this cpu_capacity_orig. N/p, communication via email can easily become confusing :-) > With using instantaneous max value in capacity_of(), we are back to > the problem raised by Thara that the value will most probably not > reflect the current capping value when it is used in LB, because LB > period can quite long on busy CPU (default max value is 32*sd_weight > ms) But averaging the capping value over time doesn't make LB happen more often ... That will help you account for capping that happened in the past, but it's not obvious this is actually a good thing. Probably not all the time anyway. Say a CPU was capped at 50% of it's capacity, then the cap is removed. At that point it'll take 100ms+ for the thermal signal to decay and let the scheduler know about the newly available capacity. That can probably be a performance hit in some use cases ... And the other way around, it can also take forever for the scheduler to notice that a CPU has a reduced capacity before reacting to it. If you want to filter out very short transient capping events to avoid over-reacting in the scheduler (is this actually happening ?), then maybe the average should be done on the cooling device side or something like that ? Thanks, Quentin
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wednesday 10 Oct 2018 at 15:27:57 (+0200), Vincent Guittot wrote: > On Wed, 10 Oct 2018 at 15:05, Quentin Perret wrote: > > > > On Wednesday 10 Oct 2018 at 14:04:40 (+0200), Vincent Guittot wrote: > > > This patchset doesn't touch cpu_capacity_orig and doesn't need to as > > > it assume that the max capacity is unchanged but some capacity is > > > momentary stolen by thermal. > > > If you want to reflect immediately all thermal capping change, you > > > have to update this field and all related fields and struct around > > > > I don't follow you here. I never said I wanted to change > > cpu_capacity_orig. I don't think we should do that actually. Changing > > capacity_of (which is updated during LB IIRC) is just fine. The question > > is about what you want to do there: reflect an averaged value or the > > instantaneous one. > > Sorry I though your were speaking about updating this cpu_capacity_orig. N/p, communication via email can easily become confusing :-) > With using instantaneous max value in capacity_of(), we are back to > the problem raised by Thara that the value will most probably not > reflect the current capping value when it is used in LB, because LB > period can quite long on busy CPU (default max value is 32*sd_weight > ms) But averaging the capping value over time doesn't make LB happen more often ... That will help you account for capping that happened in the past, but it's not obvious this is actually a good thing. Probably not all the time anyway. Say a CPU was capped at 50% of it's capacity, then the cap is removed. At that point it'll take 100ms+ for the thermal signal to decay and let the scheduler know about the newly available capacity. That can probably be a performance hit in some use cases ... And the other way around, it can also take forever for the scheduler to notice that a CPU has a reduced capacity before reacting to it. If you want to filter out very short transient capping events to avoid over-reacting in the scheduler (is this actually happening ?), then maybe the average should be done on the cooling device side or something like that ? Thanks, Quentin
[PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
dependency") changed the type of "off" from "loff_t" to "unsigned int",
there is no longer a need to cast it to "unsigned".
Signed-off-by: Geert Uytterhoeven
---
drivers/misc/eeprom/at25.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 840afb398f9e4c88..7707d3fb49b526d9 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off, void
*val, size_t count)
do {
unsigned long timeout, retries;
unsignedsegment;
- unsignedoffset = (unsigned) off;
+ unsignedoffset = off;
u8 *cp = bounce;
int sr;
u8 instr;
--
2.17.1
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Wed, Oct 10, 2018 at 06:11:29AM -0700, Christoph Hellwig wrote:
[...]
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifivem",
> > + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> > + },
> > +};
>
> What about using tabs to align this a little more nicely?
>
> static struct platform_driver sifive_pwm_driver = {
> .probe = sifive_pwm_probe,
> .remove = sifive_pwm_remove,
> .driver = {
> .name = "pwm-sifivem",
> .of_match_table = of_match_ptr(sifive_pwm_of_match),
> },
> };
I discourage people from doing that because down the road somebody might
add a field here that's longer than the alignment tabs and then either
it becomes ugly or they either have to realign everything to keep it
pretty. Single spaces around '=' don't have that problem if used
consistently.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional
Hi Doug,
Really thank you for your review.
On 2018-10-10 05:56, Doug Anderson wrote:
Hi,
On Sun, Oct 7, 2018 at 9:34 PM Can Guo wrote:
From: Venkat Gopalakrishnan
The UFS Tx lane1 clock could be muxed, hence keep it optional by
ignoring
it if it is not provided in device tree.
Thanks for the updated commit message. This makes much more sense now!
:-)
Yeah, sorry for making you guys confused. My bad previously.
@@ -113,10 +110,10 @@ static void ufs_qcom_disable_lane_clks(struct
ufs_qcom_host *host)
if (!host->is_lane_clks_enabled)
return;
- if (host->hba->lanes_per_direction > 1)
+ if (host->tx_l1_sync_clk)
clk_disable_unprepare(host->tx_l1_sync_clk);
I don't believe you need the "if". A NULL clock is by definition OK
to enable / disable which is designed to make optional clocks easier
to deal with.
You are right, I checked the implemention, clock enable/disable func
would
bail out if clk is NULL just as your comments.
clk_disable_unprepare(host->tx_l0_sync_clk);
- if (host->hba->lanes_per_direction > 1)
+ if (host->rx_l1_sync_clk)
clk_disable_unprepare(host->rx_l1_sync_clk);
optional: Technically this part of the patch wasn't actually needed,
right? "rx_l1_sync_clk" is not optional so that means that
"rx_l1_sync_clk" should be non-NULL exactly when lanes_per_direction >
1.
...but actually I'm fine with keeping this part of your patch for
symmetry. Especially since you can leverage the
clk_disable_unprepare(NULL) to simplify your code. Please mention in
your commit message that this is a cleanup and just for symmetry.
Probably also remove the "if" tests that are guarding
ufs_qcom_host_clk_enable().
Sure, got it.
clk_disable_unprepare(host->rx_l0_sync_clk);
@@ -141,24 +138,21 @@ static int ufs_qcom_enable_lane_clks(struct
ufs_qcom_host *host)
if (err)
goto disable_rx_l0;
- if (host->hba->lanes_per_direction > 1) {
+ if (host->rx_l1_sync_clk) {
Similar: the above change isn't required but I'm OK if you want to
make this change for symmetry / to cleanup clock handling. Please
mention in your commit message.
Sure, shall do so.
+ /* The tx lane1 clk could be muxed, hence keep this optional
*/
+ if (host->tx_l1_sync_clk)
+ ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
+host->tx_l1_sync_clk);
If "host->tx_l1_sync_clk" is provided then you should still check the
return value of ufs_qcom_host_clk_enable(), right? ...so please leave
the error path here.
Yeah... you are right.
@@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct
ufs_qcom_host *host)
err = ufs_qcom_host_clk_get(dev,
"rx_lane0_sync_clk", >rx_l0_sync_clk);
- if (err)
+ if (err) {
+ dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err
%d",
+ __func__, err);
nit: including "__func__" in dev_xxx() calls is discouraged. The
"dev_xxx" calls already print the device name and the string within a
given device driver should be unique enough so __func__ just adds crap
to the logs. If you really feel that __func__ adds something for you,
try posting up a patch to make all "dev_err" functions include
__func__. ...but I think you'd probably be rejected.
suggestion: you'd save a few lines of code and simplify your probe if
you just passed an "optional" bool to the ufs_qcom_host_clk_get() that
controlled the printout.
Good idea!
- err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
- >tx_l1_sync_clk);
+ /* The tx lane1 clk could be muxed, hence keep this
optional */
+ ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
+ >tx_l1_sync_clk);
To be technically correct you should actually check the error value
returned by ufs_qcom_host_clk_get(). Specifically figure out what the
error value is when "tx_lane1_sync_clk" isn't specified and check for
that.
...one reason this matters is -EPROBE_DEFER. In theory devm_clk_get()
could return -EPROBE_DEFER. In such a case you'd want to exit the
probe, not continue on. It's also just good coding practice to handle
just the error you want though so that if the function returned some
other failing case you'd propagate it down instead of eating it.
If you passed "optional" to ufs_qcom_host_clk_get() as suggested above
you could put this error-code checking in ufs_qcom_host_clk_get() and
return 0 from that function if an optional clock was found to not
exist.
-Doug
I think I understand it. Thanks a lot for the thorough review and
suggestions.
-Can Guo
[PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
dependency") changed the type of "off" from "loff_t" to "unsigned int",
there is no longer a need to cast it to "unsigned".
Signed-off-by: Geert Uytterhoeven
---
drivers/misc/eeprom/at25.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 840afb398f9e4c88..7707d3fb49b526d9 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off, void
*val, size_t count)
do {
unsigned long timeout, retries;
unsignedsegment;
- unsignedoffset = (unsigned) off;
+ unsignedoffset = off;
u8 *cp = bounce;
int sr;
u8 instr;
--
2.17.1
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Wed, Oct 10, 2018 at 06:11:29AM -0700, Christoph Hellwig wrote:
[...]
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifivem",
> > + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> > + },
> > +};
>
> What about using tabs to align this a little more nicely?
>
> static struct platform_driver sifive_pwm_driver = {
> .probe = sifive_pwm_probe,
> .remove = sifive_pwm_remove,
> .driver = {
> .name = "pwm-sifivem",
> .of_match_table = of_match_ptr(sifive_pwm_of_match),
> },
> };
I discourage people from doing that because down the road somebody might
add a field here that's longer than the alignment tabs and then either
it becomes ugly or they either have to realign everything to keep it
pretty. Single spaces around '=' don't have that problem if used
consistently.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional
Hi Doug,
Really thank you for your review.
On 2018-10-10 05:56, Doug Anderson wrote:
Hi,
On Sun, Oct 7, 2018 at 9:34 PM Can Guo wrote:
From: Venkat Gopalakrishnan
The UFS Tx lane1 clock could be muxed, hence keep it optional by
ignoring
it if it is not provided in device tree.
Thanks for the updated commit message. This makes much more sense now!
:-)
Yeah, sorry for making you guys confused. My bad previously.
@@ -113,10 +110,10 @@ static void ufs_qcom_disable_lane_clks(struct
ufs_qcom_host *host)
if (!host->is_lane_clks_enabled)
return;
- if (host->hba->lanes_per_direction > 1)
+ if (host->tx_l1_sync_clk)
clk_disable_unprepare(host->tx_l1_sync_clk);
I don't believe you need the "if". A NULL clock is by definition OK
to enable / disable which is designed to make optional clocks easier
to deal with.
You are right, I checked the implemention, clock enable/disable func
would
bail out if clk is NULL just as your comments.
clk_disable_unprepare(host->tx_l0_sync_clk);
- if (host->hba->lanes_per_direction > 1)
+ if (host->rx_l1_sync_clk)
clk_disable_unprepare(host->rx_l1_sync_clk);
optional: Technically this part of the patch wasn't actually needed,
right? "rx_l1_sync_clk" is not optional so that means that
"rx_l1_sync_clk" should be non-NULL exactly when lanes_per_direction >
1.
...but actually I'm fine with keeping this part of your patch for
symmetry. Especially since you can leverage the
clk_disable_unprepare(NULL) to simplify your code. Please mention in
your commit message that this is a cleanup and just for symmetry.
Probably also remove the "if" tests that are guarding
ufs_qcom_host_clk_enable().
Sure, got it.
clk_disable_unprepare(host->rx_l0_sync_clk);
@@ -141,24 +138,21 @@ static int ufs_qcom_enable_lane_clks(struct
ufs_qcom_host *host)
if (err)
goto disable_rx_l0;
- if (host->hba->lanes_per_direction > 1) {
+ if (host->rx_l1_sync_clk) {
Similar: the above change isn't required but I'm OK if you want to
make this change for symmetry / to cleanup clock handling. Please
mention in your commit message.
Sure, shall do so.
+ /* The tx lane1 clk could be muxed, hence keep this optional
*/
+ if (host->tx_l1_sync_clk)
+ ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
+host->tx_l1_sync_clk);
If "host->tx_l1_sync_clk" is provided then you should still check the
return value of ufs_qcom_host_clk_enable(), right? ...so please leave
the error path here.
Yeah... you are right.
@@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct
ufs_qcom_host *host)
err = ufs_qcom_host_clk_get(dev,
"rx_lane0_sync_clk", >rx_l0_sync_clk);
- if (err)
+ if (err) {
+ dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err
%d",
+ __func__, err);
nit: including "__func__" in dev_xxx() calls is discouraged. The
"dev_xxx" calls already print the device name and the string within a
given device driver should be unique enough so __func__ just adds crap
to the logs. If you really feel that __func__ adds something for you,
try posting up a patch to make all "dev_err" functions include
__func__. ...but I think you'd probably be rejected.
suggestion: you'd save a few lines of code and simplify your probe if
you just passed an "optional" bool to the ufs_qcom_host_clk_get() that
controlled the printout.
Good idea!
- err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
- >tx_l1_sync_clk);
+ /* The tx lane1 clk could be muxed, hence keep this
optional */
+ ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
+ >tx_l1_sync_clk);
To be technically correct you should actually check the error value
returned by ufs_qcom_host_clk_get(). Specifically figure out what the
error value is when "tx_lane1_sync_clk" isn't specified and check for
that.
...one reason this matters is -EPROBE_DEFER. In theory devm_clk_get()
could return -EPROBE_DEFER. In such a case you'd want to exit the
probe, not continue on. It's also just good coding practice to handle
just the error you want though so that if the function returned some
other failing case you'd propagate it down instead of eating it.
If you passed "optional" to ufs_qcom_host_clk_get() as suggested above
you could put this error-code checking in ufs_qcom_host_clk_get() and
return 0 from that function if an optional clock was found to not
exist.
-Doug
I think I understand it. Thanks a lot for the thorough review and
suggestions.
-Can Guo
[PATCH 0/3] eeprom: at25: SPI transfer improvements
[Resend with correct linux-mtd address]
Hi all,
This patch series contains various improvements for the AT25 SPI EEPROM
driver, related to SPI transfers.
Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
and a 25LC040 EEPROM. The MSIOF SPI Controller can only do DMA for
transfers with a length that is a multiple of 4 bytes.
Thanks!
Geert Uytterhoeven (3):
eeprom: at25: Drop obsolete cast in at25_ee_write()
eeprom: at25: Use spi_message_init_with_transfers() instead of open
coding
eeprom: at25: Split writes in two SPI transfers to optimize DMA
drivers/misc/eeprom/at25.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 0/3] eeprom: at25: SPI transfer improvements
[Resend with correct linux-mtd address]
Hi all,
This patch series contains various improvements for the AT25 SPI EEPROM
driver, related to SPI transfers.
Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
and a 25LC040 EEPROM. The MSIOF SPI Controller can only do DMA for
transfers with a length that is a multiple of 4 bytes.
Thanks!
Geert Uytterhoeven (3):
eeprom: at25: Drop obsolete cast in at25_ee_write()
eeprom: at25: Use spi_message_init_with_transfers() instead of open
coding
eeprom: at25: Split writes in two SPI transfers to optimize DMA
drivers/misc/eeprom/at25.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
dependency") changed the type of "off" from "loff_t" to "unsigned int",
there is no longer a need to cast it to "unsigned".
Signed-off-by: Geert Uytterhoeven
---
drivers/misc/eeprom/at25.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 840afb398f9e4c88..7707d3fb49b526d9 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off, void
*val, size_t count)
do {
unsigned long timeout, retries;
unsignedsegment;
- unsignedoffset = (unsigned) off;
+ unsignedoffset = off;
u8 *cp = bounce;
int sr;
u8 instr;
--
2.17.1
[PATCH 1/3] eeprom: at25: Drop obsolete cast in at25_ee_write()
Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
dependency") changed the type of "off" from "loff_t" to "unsigned int",
there is no longer a need to cast it to "unsigned".
Signed-off-by: Geert Uytterhoeven
---
drivers/misc/eeprom/at25.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 840afb398f9e4c88..7707d3fb49b526d9 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -160,7 +160,7 @@ static int at25_ee_write(void *priv, unsigned int off, void
*val, size_t count)
do {
unsigned long timeout, retries;
unsignedsegment;
- unsignedoffset = (unsigned) off;
+ unsignedoffset = off;
u8 *cp = bounce;
int sr;
u8 instr;
--
2.17.1
[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
Currently EEPROM writes are implemented using a single SPI transfer, which contains all of command, address, and payload data bytes. As some SPI controllers impose limitations on transfers with respect to the use of DMA, they may have to fall back to PIO. E.g. DMA may require the transfer length to be a multiple of 4 bytes. Optimize writes for DMA by splitting writes in two SPI transfers: - The first transfer contains command and address bytes, - The second transfer contains the actual payload data, now stored at the start of the (kmalloc() aligned) buffer, to improve payload alignment. E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write aligned to the page size was transferred using an 18-byte write. After this change, the write is split in a 2-byte and an aligned 16-byte write. Note that EEPROM reads already use a similar scheme, due to the different data directions for command and address bytes versus payload data. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 5c8dc7ad391435f7..f84d1681835b4ded 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -136,6 +136,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) int status = 0; unsignedbuf_size; u8 *bounce; + struct spi_transfer t[2]; if (unlikely(off >= at25->chip.byte_len)) return -EFBIG; @@ -160,7 +161,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) unsigned long timeout, retries; unsignedsegment; unsignedoffset = off; - u8 *cp = bounce; + u8 *cp = bounce + buf_size; int sr; u8 instr; @@ -194,9 +195,17 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); + memcpy(bounce, buf, segment); + + memset(t, 0, sizeof(t)); + + t[0].tx_buf = bounce + buf_size; + t[0].len = at25->addrlen + 1; + + t[1].tx_buf = bounce; + t[1].len = segment; + + status = spi_sync_transfer(at25->spi, t, ARRAY_SIZE(t)); dev_dbg(>spi->dev, "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) -- 2.17.1
[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
Currently EEPROM writes are implemented using a single SPI transfer, which contains all of command, address, and payload data bytes. As some SPI controllers impose limitations on transfers with respect to the use of DMA, they may have to fall back to PIO. E.g. DMA may require the transfer length to be a multiple of 4 bytes. Optimize writes for DMA by splitting writes in two SPI transfers: - The first transfer contains command and address bytes, - The second transfer contains the actual payload data, now stored at the start of the (kmalloc() aligned) buffer, to improve payload alignment. E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write aligned to the page size was transferred using an 18-byte write. After this change, the write is split in a 2-byte and an aligned 16-byte write. Note that EEPROM reads already use a similar scheme, due to the different data directions for command and address bytes versus payload data. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 5c8dc7ad391435f7..f84d1681835b4ded 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -136,6 +136,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) int status = 0; unsignedbuf_size; u8 *bounce; + struct spi_transfer t[2]; if (unlikely(off >= at25->chip.byte_len)) return -EFBIG; @@ -160,7 +161,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) unsigned long timeout, retries; unsignedsegment; unsignedoffset = off; - u8 *cp = bounce; + u8 *cp = bounce + buf_size; int sr; u8 instr; @@ -194,9 +195,17 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); + memcpy(bounce, buf, segment); + + memset(t, 0, sizeof(t)); + + t[0].tx_buf = bounce + buf_size; + t[0].len = at25->addrlen + 1; + + t[1].tx_buf = bounce; + t[1].len = segment; + + status = spi_sync_transfer(at25->spi, t, ARRAY_SIZE(t)); dev_dbg(>spi->dev, "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) -- 2.17.1
[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
Currently EEPROM writes are implemented using a single SPI transfer, which contains all of command, address, and payload data bytes. As some SPI controllers impose limitations on transfers with respect to the use of DMA, they may have to fall back to PIO. E.g. DMA may require the transfer length to be a multiple of 4 bytes. Optimize writes for DMA by splitting writes in two SPI transfers: - The first transfer contains command and address bytes, - The second transfer contains the actual payload data, now stored at the start of the (kmalloc() aligned) buffer, to improve payload alignment. E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write aligned to the page size was transferred using an 18-byte write. After this change, the write is split in a 2-byte and an aligned 16-byte write. Note that EEPROM reads already use a similar scheme, due to the different data directions for command and address bytes versus payload data. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 5c8dc7ad391435f7..f84d1681835b4ded 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -136,6 +136,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) int status = 0; unsignedbuf_size; u8 *bounce; + struct spi_transfer t[2]; if (unlikely(off >= at25->chip.byte_len)) return -EFBIG; @@ -160,7 +161,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) unsigned long timeout, retries; unsignedsegment; unsignedoffset = off; - u8 *cp = bounce; + u8 *cp = bounce + buf_size; int sr; u8 instr; @@ -194,9 +195,17 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); + memcpy(bounce, buf, segment); + + memset(t, 0, sizeof(t)); + + t[0].tx_buf = bounce + buf_size; + t[0].len = at25->addrlen + 1; + + t[1].tx_buf = bounce; + t[1].len = segment; + + status = spi_sync_transfer(at25->spi, t, ARRAY_SIZE(t)); dev_dbg(>spi->dev, "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) -- 2.17.1
[PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
Reduce code duplication in at25_ee_read() by using the spi_message_init_with_transfers() helper. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 7707d3fb49b526d9..5c8dc7ad391435f7 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset, *cp++ = offset >> 0; } - spi_message_init(); memset(t, 0, sizeof(t)); t[0].tx_buf = command; t[0].len = at25->addrlen + 1; - spi_message_add_tail([0], ); t[1].rx_buf = buf; t[1].len = count; - spi_message_add_tail([1], ); + + spi_message_init_with_transfers(, t, ARRAY_SIZE(t)); mutex_lock(>lock); -- 2.17.1
[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers to optimize DMA
Currently EEPROM writes are implemented using a single SPI transfer, which contains all of command, address, and payload data bytes. As some SPI controllers impose limitations on transfers with respect to the use of DMA, they may have to fall back to PIO. E.g. DMA may require the transfer length to be a multiple of 4 bytes. Optimize writes for DMA by splitting writes in two SPI transfers: - The first transfer contains command and address bytes, - The second transfer contains the actual payload data, now stored at the start of the (kmalloc() aligned) buffer, to improve payload alignment. E.g. for a 25LC040 EEPROM with a page size 16 bytes, a 16-byte write aligned to the page size was transferred using an 18-byte write. After this change, the write is split in a 2-byte and an aligned 16-byte write. Note that EEPROM reads already use a similar scheme, due to the different data directions for command and address bytes versus payload data. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 5c8dc7ad391435f7..f84d1681835b4ded 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -136,6 +136,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) int status = 0; unsignedbuf_size; u8 *bounce; + struct spi_transfer t[2]; if (unlikely(off >= at25->chip.byte_len)) return -EFBIG; @@ -160,7 +161,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) unsigned long timeout, retries; unsignedsegment; unsignedoffset = off; - u8 *cp = bounce; + u8 *cp = bounce + buf_size; int sr; u8 instr; @@ -194,9 +195,17 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); + memcpy(bounce, buf, segment); + + memset(t, 0, sizeof(t)); + + t[0].tx_buf = bounce + buf_size; + t[0].len = at25->addrlen + 1; + + t[1].tx_buf = bounce; + t[1].len = segment; + + status = spi_sync_transfer(at25->spi, t, ARRAY_SIZE(t)); dev_dbg(>spi->dev, "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) -- 2.17.1
[PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
Reduce code duplication in at25_ee_read() by using the spi_message_init_with_transfers() helper. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 7707d3fb49b526d9..5c8dc7ad391435f7 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset, *cp++ = offset >> 0; } - spi_message_init(); memset(t, 0, sizeof(t)); t[0].tx_buf = command; t[0].len = at25->addrlen + 1; - spi_message_add_tail([0], ); t[1].rx_buf = buf; t[1].len = count; - spi_message_add_tail([1], ); + + spi_message_init_with_transfers(, t, ARRAY_SIZE(t)); mutex_lock(>lock); -- 2.17.1
[PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
Reduce code duplication in at25_ee_read() by using the spi_message_init_with_transfers() helper. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 7707d3fb49b526d9..5c8dc7ad391435f7 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset, *cp++ = offset >> 0; } - spi_message_init(); memset(t, 0, sizeof(t)); t[0].tx_buf = command; t[0].len = at25->addrlen + 1; - spi_message_add_tail([0], ); t[1].rx_buf = buf; t[1].len = count; - spi_message_add_tail([1], ); + + spi_message_init_with_transfers(, t, ARRAY_SIZE(t)); mutex_lock(>lock); -- 2.17.1
[PATCH 0/3] eeprom: at25: SPI transfer improvements
Hi all,
This patch series contains various improvements for the AT25 SPI EEPROM
driver, related to SPI transfers.
Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
and a 25LC040 EEPROM. The MSIOF SPI Controller can only do DMA for
transfers with a length that is a multiple of 4 bytes.
Thanks!
Geert Uytterhoeven (3):
eeprom: at25: Drop obsolete cast in at25_ee_write()
eeprom: at25: Use spi_message_init_with_transfers() instead of open
coding
eeprom: at25: Split writes in two SPI transfers to optimize DMA
drivers/misc/eeprom/at25.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 2/3] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding
Reduce code duplication in at25_ee_read() by using the spi_message_init_with_transfers() helper. Signed-off-by: Geert Uytterhoeven --- drivers/misc/eeprom/at25.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 7707d3fb49b526d9..5c8dc7ad391435f7 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset, *cp++ = offset >> 0; } - spi_message_init(); memset(t, 0, sizeof(t)); t[0].tx_buf = command; t[0].len = at25->addrlen + 1; - spi_message_add_tail([0], ); t[1].rx_buf = buf; t[1].len = count; - spi_message_add_tail([1], ); + + spi_message_init_with_transfers(, t, ARRAY_SIZE(t)); mutex_lock(>lock); -- 2.17.1
[PATCH 0/3] eeprom: at25: SPI transfer improvements
Hi all,
This patch series contains various improvements for the AT25 SPI EEPROM
driver, related to SPI transfers.
Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
and a 25LC040 EEPROM. The MSIOF SPI Controller can only do DMA for
transfers with a length that is a multiple of 4 bytes.
Thanks!
Geert Uytterhoeven (3):
eeprom: at25: Drop obsolete cast in at25_ee_write()
eeprom: at25: Use spi_message_init_with_transfers() instead of open
coding
eeprom: at25: Split writes in two SPI transfers to optimize DMA
drivers/misc/eeprom/at25.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wed, 10 Oct 2018 at 15:35, Juri Lelli wrote: > > On 10/10/18 15:08, Vincent Guittot wrote: > > On Wed, 10 Oct 2018 at 14:50, Juri Lelli wrote: > > > > > > On 10/10/18 14:34, Vincent Guittot wrote: > > > > Hi Juri, > > > > > > > > On Wed, 10 Oct 2018 at 14:23, Juri Lelli wrote: > > > > > > > > > > On 10/10/18 14:04, Vincent Guittot wrote: > > > > > > > > > > [...] > > > > > > > > > > > The problem was the same with RT, the cfs utilization was lower than > > > > > > reality because RT steals soem cycle to CFS > > > > > > So schedutil was selecting a lower frequency when cfs was running > > > > > > whereas the CPU was fully used. > > > > > > The same can happen with thermal: > > > > > > cap the max freq because of thermal > > > > > > the utilization with decrease. > > > > > > remove the cap > > > > > > the utilization is still low and you will select a low OPP because > > > > > > you > > > > > > don't take into account cycle stolen by thermal like with RT > > > > > > > > > > What if we scale frequency component considering the capped temporary > > > > > max? > > > > > > > > Do you mean using a kind of scale_thermal_capacity in accumulate_sum > > > > when computing utilization ? > > > > > > Yeah, something like that I guess. So that we account for temporary > > > "fake" 1024.. > > > > But the utilization will not be invariant anymore across the system > > Mmm, I guess I might be wrong, but I was thinking we should be able to > deal with this similarly to what we do with cpus with different max > capacities. So, another factor? Because then, how do we handle other > ways in which max freq can be restricted (e.g. from userspace as Javi > was also mentioning)? IMHO, userspace capping is a different story because it is not expected to happen so often but it should stay for a while and in this case, a solution is probably to rebuild the sched_domain and update all the cpu_capacity struct and fields
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wed, 10 Oct 2018 at 15:35, Juri Lelli wrote: > > On 10/10/18 15:08, Vincent Guittot wrote: > > On Wed, 10 Oct 2018 at 14:50, Juri Lelli wrote: > > > > > > On 10/10/18 14:34, Vincent Guittot wrote: > > > > Hi Juri, > > > > > > > > On Wed, 10 Oct 2018 at 14:23, Juri Lelli wrote: > > > > > > > > > > On 10/10/18 14:04, Vincent Guittot wrote: > > > > > > > > > > [...] > > > > > > > > > > > The problem was the same with RT, the cfs utilization was lower than > > > > > > reality because RT steals soem cycle to CFS > > > > > > So schedutil was selecting a lower frequency when cfs was running > > > > > > whereas the CPU was fully used. > > > > > > The same can happen with thermal: > > > > > > cap the max freq because of thermal > > > > > > the utilization with decrease. > > > > > > remove the cap > > > > > > the utilization is still low and you will select a low OPP because > > > > > > you > > > > > > don't take into account cycle stolen by thermal like with RT > > > > > > > > > > What if we scale frequency component considering the capped temporary > > > > > max? > > > > > > > > Do you mean using a kind of scale_thermal_capacity in accumulate_sum > > > > when computing utilization ? > > > > > > Yeah, something like that I guess. So that we account for temporary > > > "fake" 1024.. > > > > But the utilization will not be invariant anymore across the system > > Mmm, I guess I might be wrong, but I was thinking we should be able to > deal with this similarly to what we do with cpus with different max > capacities. So, another factor? Because then, how do we handle other > ways in which max freq can be restricted (e.g. from userspace as Javi > was also mentioning)? IMHO, userspace capping is a different story because it is not expected to happen so often but it should stay for a while and in this case, a solution is probably to rebuild the sched_domain and update all the cpu_capacity struct and fields
Re: [RCF PATCH v2 1/2] dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO
On Wed, Oct 10, 2018 at 09:33:25AM +, Vokáč Michal wrote: > Output of the PWM block on i.MX SoCs is always low when the block is > disabled. This can cause issues when inverted PWM polarity is needed. > With inverted polarity a duty cycle = 0% corresponds to high level on > the output. Now, when PWM is disabled its output instantly goes low > which corresponds to duty cycle = 100%. > > To get a truly inverted PWM output two pinctrl states of the PWM pin > can be used. Configure the pin to GPIO function when PWM is disabled > and switch back to PWM function whenever non-zero duty cycle is needed. > > Signed-off-by: Michal Vokáč > --- > Changes in v2: > - Do not use the "default" pinctrl state for GPIO. > - Use two new "pwm" and "gpio" pinctrl states. > - Add a new pwm-gpios signal. > > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 51 > +++ > 1 file changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt > index c61bdf8..bd5b6bd 100644 > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt > @@ -14,6 +14,17 @@ See the clock consumer binding, > Documentation/devicetree/bindings/clock/clock-bindings.txt > - interrupts: The interrupt for the pwm controller > > +Optional properties: > +- pinctrl: For i.MX27 and newer SoCs. Use "pwm" and "gpio" specific pinctrls > + instead of the "default" to configure the PWM pin to GPIO and PWM function. > + It allows control over the pin output level when the PWM block is disabled. > + This is useful if you use the PWM for single purpose and you need inverted > + polarity of the PWM signal. See "Inverted PWM output" section bellow. > +- pwm-gpios: Specify the GPIO pin that will act as the PWM output. This > should > + be the same pin as is used for normal PWM output. Define the pin as > + GPIO_ACTIVE_LOW to get HIGH level on the output when PWM is disabled. See > + "Inverted PWM output" section bellow. It's somewhat unfortunate that we have to specify this in DT. For one thing, we don't really want to use the pin as GPIO, we really only care about whether it is "active" or "inactive". We also already specify the GPIO in the pinctrl nodes, albeit via a different name. So we're effectively duplicating information here. It'd be nice to avoid that. Two possibilities that I could think about are: 1) Do not explicitly rely on driving the GPIO as output: I know this was discussed before and it sounds like this is not an option for PWM because the GPIO may be configured as output by the firmware, and hence switching to GPIO mode may not give the expected result. I suppose one way to solve this is by using a gpio-hog entry for the PWM GPIO so that it will automatically get configured as an input and at the same time marked as busy so that nobody can go and just request it again (via sysfs for example). 2) Derive the GPIO from the pin. I'm not sure there's anything in the pinctrl framework to do that. The reverse (GPIO -> pin) can be done, so perhaps this is something that could be added? Other than than I think this looks very nice. Thierry signature.asc Description: PGP signature
Re: [RCF PATCH v2 1/2] dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO
On Wed, Oct 10, 2018 at 09:33:25AM +, Vokáč Michal wrote: > Output of the PWM block on i.MX SoCs is always low when the block is > disabled. This can cause issues when inverted PWM polarity is needed. > With inverted polarity a duty cycle = 0% corresponds to high level on > the output. Now, when PWM is disabled its output instantly goes low > which corresponds to duty cycle = 100%. > > To get a truly inverted PWM output two pinctrl states of the PWM pin > can be used. Configure the pin to GPIO function when PWM is disabled > and switch back to PWM function whenever non-zero duty cycle is needed. > > Signed-off-by: Michal Vokáč > --- > Changes in v2: > - Do not use the "default" pinctrl state for GPIO. > - Use two new "pwm" and "gpio" pinctrl states. > - Add a new pwm-gpios signal. > > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 51 > +++ > 1 file changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt > index c61bdf8..bd5b6bd 100644 > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt > @@ -14,6 +14,17 @@ See the clock consumer binding, > Documentation/devicetree/bindings/clock/clock-bindings.txt > - interrupts: The interrupt for the pwm controller > > +Optional properties: > +- pinctrl: For i.MX27 and newer SoCs. Use "pwm" and "gpio" specific pinctrls > + instead of the "default" to configure the PWM pin to GPIO and PWM function. > + It allows control over the pin output level when the PWM block is disabled. > + This is useful if you use the PWM for single purpose and you need inverted > + polarity of the PWM signal. See "Inverted PWM output" section bellow. > +- pwm-gpios: Specify the GPIO pin that will act as the PWM output. This > should > + be the same pin as is used for normal PWM output. Define the pin as > + GPIO_ACTIVE_LOW to get HIGH level on the output when PWM is disabled. See > + "Inverted PWM output" section bellow. It's somewhat unfortunate that we have to specify this in DT. For one thing, we don't really want to use the pin as GPIO, we really only care about whether it is "active" or "inactive". We also already specify the GPIO in the pinctrl nodes, albeit via a different name. So we're effectively duplicating information here. It'd be nice to avoid that. Two possibilities that I could think about are: 1) Do not explicitly rely on driving the GPIO as output: I know this was discussed before and it sounds like this is not an option for PWM because the GPIO may be configured as output by the firmware, and hence switching to GPIO mode may not give the expected result. I suppose one way to solve this is by using a gpio-hog entry for the PWM GPIO so that it will automatically get configured as an input and at the same time marked as busy so that nobody can go and just request it again (via sysfs for example). 2) Derive the GPIO from the pin. I'm not sure there's anything in the pinctrl framework to do that. The reverse (GPIO -> pin) can be done, so perhaps this is something that could be added? Other than than I think this looks very nice. Thierry signature.asc Description: PGP signature
[PATCH] tpm: fix unused-value issues in tpm_try_transmit
Currently, there are some values assigned to variable *rc*, which
are never actually used in any computation, because such variable
is updated at line 550, before they can be used:
549out:
550rc = tpm_go_idle(chip, flags);
551if (rc)
552goto out;
Fix this by removing such assignments.
Addresses-Coverity-ID: 1470245 ("Unused value")
Addresses-Coverity-ID: 1470250 ("Unused value")
Addresses-Coverity-ID: 1470251 ("Unused value")
Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
Signed-off-by: Gustavo A. R. Silva
---
drivers/char/tpm/tpm-interface.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640..8062736 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -512,7 +512,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
if (chip->ops->req_canceled(chip, status)) {
dev_err(>dev, "Operation Canceled\n");
- rc = -ECANCELED;
goto out;
}
@@ -522,7 +521,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
chip->ops->cancel(chip);
dev_err(>dev, "Operation Timed out\n");
- rc = -ETIME;
goto out;
out_recv:
@@ -533,14 +531,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
"tpm_transmit: tpm_recv: error %d\n", rc);
goto out;
} else if (len < TPM_HEADER_SIZE) {
- rc = -EFAULT;
goto out;
}
- if (len != be32_to_cpu(header->length)) {
- rc = -EFAULT;
+ if (len != be32_to_cpu(header->length))
goto out;
- }
+
rc = tpm2_commit_space(chip, space, ordinal, buf, );
if (rc)
--
2.7.4
[PATCH] tpm: fix unused-value issues in tpm_try_transmit
Currently, there are some values assigned to variable *rc*, which
are never actually used in any computation, because such variable
is updated at line 550, before they can be used:
549out:
550rc = tpm_go_idle(chip, flags);
551if (rc)
552goto out;
Fix this by removing such assignments.
Addresses-Coverity-ID: 1470245 ("Unused value")
Addresses-Coverity-ID: 1470250 ("Unused value")
Addresses-Coverity-ID: 1470251 ("Unused value")
Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
Signed-off-by: Gustavo A. R. Silva
---
drivers/char/tpm/tpm-interface.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640..8062736 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -512,7 +512,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
if (chip->ops->req_canceled(chip, status)) {
dev_err(>dev, "Operation Canceled\n");
- rc = -ECANCELED;
goto out;
}
@@ -522,7 +521,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
chip->ops->cancel(chip);
dev_err(>dev, "Operation Timed out\n");
- rc = -ETIME;
goto out;
out_recv:
@@ -533,14 +531,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
"tpm_transmit: tpm_recv: error %d\n", rc);
goto out;
} else if (len < TPM_HEADER_SIZE) {
- rc = -EFAULT;
goto out;
}
- if (len != be32_to_cpu(header->length)) {
- rc = -EFAULT;
+ if (len != be32_to_cpu(header->length))
goto out;
- }
+
rc = tpm2_commit_space(chip, space, ordinal, buf, );
if (rc)
--
2.7.4
Re: linux-next: Tree for Oct 10
On Wed, Oct 10, 2018 at 04:46:27PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20181009:
>
> The ext4 tree lost its build failure.
>
> The kvm-ppc tree gained a conflict against the kvm-arm tree.
>
> Non-merge commits (relative to Linus' tree): 9423
> 8942 files changed, 420582 insertions(+), 194157 deletions(-)
>
>
Another day, another (different) set of failures.
Build results:
total: 135 pass: 122 fail: 13
Failed builds:
alpha:allmodconfig
arm64:allmodconfig
m68k:defconfig
m68k:allmodconfig
mips:allmodconfig
parisc:allmodconfig
powerpc:allmodconfig
powerpc:ppc6xx_defconfig
riscv:defconfig
s390:allmodconfig
s390:allnoconfig
sparc64:allmodconfig
xtensa:allmodconfig
Qemu test results:
total: 318 pass: 280 fail: 38
Failed tests:
riscv:virt:defconfig:initrd
riscv:virt:defconfig:virtio-blk:rootfs
sparc32:SPARCClassic:nosmp:sparc32_defconfig
sparc32:SPARCbook:nosmp:sparc32_defconfig
sparc32:SS-4:nosmp:sparc32_defconfig
sparc32:SS-5:nosmp:sparc32_defconfig
sparc32:SS-10:nosmp:sparc32_defconfig
sparc32:SS-20:nosmp:sparc32_defconfig
sparc32:SS-600MP:nosmp:sparc32_defconfig
sparc32:LX:nosmp:sparc32_defconfig
sparc32:Voyager:nosmp:sparc32_defconfig
sparc32:SPARCClassic:smp:sparc32_defconfig
sparc32:SPARCbook:smp:sparc32_defconfig
sparc32:SS-4:smp:sparc32_defconfig
sparc32:SS-5:smp:sparc32_defconfig
sparc32:SS-10:smp:sparc32_defconfig
sparc32:SS-20:smp:sparc32_defconfig
sparc32:SS-600MP:smp:sparc32_defconfig
sparc32:LX:smp:sparc32_defconfig
sparc32:Voyager:smp:sparc32_defconfig
sparc64:sun4u:smp:initrd
sparc64:sun4u:smp:ata:rootfs
sparc64:sun4u:smp:mmc:rootfs
sparc64:sun4u:smp:nvme:rootfs
sparc64:sun4u:smp:scsi[DC395]:rootfs
sparc64:sun4u:smp:scsi[MEGASAS]:rootfs
sparc64:sun4u:smp:scsi[AM53C974]:rootfs
sparc64:sun4u:smp:usb-xhci:rootfs
sparc64:sun4u:smp:usb-uas-xhci:rootfs
sparc64:sun4u:smp:virtio-pci:rootfs
sparc64:sun4v:smp:ata:rootfs
sparc64:sun4v:smp:nvme:rootfs
sparc64:sun4u:nosmp:ata:rootfs
sparc64:sun4v:nosmp:ata:rootfs
i386:pc:Penryn:defconfig:pae:smp:initrd
i386:pc:Westmere:defconfig:pae:smp:efi32:usb:rootfs
i386:q35:pentium3:defconfig:pae:nosmp:nvme:rootfs
i386:q35:coreduo:defconfig:pae:nosmp:efi32:mmc:rootfs
Details are available at https://kerneltests.org/builders/.
Guenter
---
Compile errors:
alpha, arm64, parisc, powerpc, s390, sparc64, xtensa:
drivers/net/ethernet/chelsio/cxgb4/cxgb4_thermal.c: In function
'cxgb4_thermal_get_trip_type':
drivers/net/ethernet/chelsio/cxgb4/cxgb4_thermal.c:48:11: error: 'struct
adapter' has no member named 'ch_thermal'
mips:
net/sunrpc/auth_gss/gss_krb5_seal.c: In function 'gss_seq_send64_fetch_and_inc':
net/sunrpc/auth_gss/gss_krb5_seal.c:144:3: error: implicit declaration of
function 'cmpxchg64'
riscv:
Building riscv:defconfig ... failed
--
Error log:
cc1: error: -march=c: ISA string must begin with rv32 or rv64
cc1: error: ABI requires -march=rv64
Crashes:
sparc32, sparc64:
(log is from sparc32)
Unable to handle kernel NULL pointer dereference
tsk->{mm,active_mm}->context =
tsk->{mm,active_mm}->pgd = fc00
swapper(0): Oops [#1]
CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc7-next-20181009 #1
PSR: 41801fc2 PC: f03aee08 NPC: f03aee0c Y: Not tainted
PC:
%G: f04d2968 f03b23a4 0001 0066 f048 f0481b40
RPC:
%L: f044c0a8 0020 f0459000 f03afd8c f0444928 f0481d08
%I: f0481d08 f0481d48 f04f5ba0 f0481ba8 f044e72c f0481b28 f0481b48 f03b3e58
Disabling lock debugging due to kernel taint
Caller[f03b3e58]: pointer+0x338/0x5f8
Caller[f03b4338]: vsnprintf+0x220/0x5d0
Caller[f03b4764]: snprintf+0x20/0x30
Caller[f04abddc]: build_path_component+0x1e8/0x214
Caller[f04bbeb8]: of_pdt_build_tree+0x54/0x138
Caller[f04bc020]: of_pdt_build_devicetree+0x84/0x98
Caller[f04abb40]: prom_build_devicetree+0x10/0x30
Caller[f04ad61c]: paging_init+0xc/0x24
Caller[f04ab570]: setup_arch+0x45c/0x494
Caller[f04a7b40]: start_kernel+0x48/0x458
Caller[f04a743c]: continue_boot+0x324/0x334
Caller[]: (null)
x86:
[6.632088] Kernel panic - not syncing: stack-protector: Kernel stack is
corrupted in: pgd_alloc+0x2be/0x2c0
[6.632088]
[6.632712] CPU: 0 PID: 717 Comm: kworker/u2:3 Not tainted
4.19.0-rc7-next-20181009 #1
[6.632712] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.11.2-0-gf9626ccb91-prebuilt.qemu-project.org 04/01/2014
[6.632712] Call Trace:
[6.632712]
Re: linux-next: Tree for Oct 10
On Wed, Oct 10, 2018 at 04:46:27PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20181009:
>
> The ext4 tree lost its build failure.
>
> The kvm-ppc tree gained a conflict against the kvm-arm tree.
>
> Non-merge commits (relative to Linus' tree): 9423
> 8942 files changed, 420582 insertions(+), 194157 deletions(-)
>
>
Another day, another (different) set of failures.
Build results:
total: 135 pass: 122 fail: 13
Failed builds:
alpha:allmodconfig
arm64:allmodconfig
m68k:defconfig
m68k:allmodconfig
mips:allmodconfig
parisc:allmodconfig
powerpc:allmodconfig
powerpc:ppc6xx_defconfig
riscv:defconfig
s390:allmodconfig
s390:allnoconfig
sparc64:allmodconfig
xtensa:allmodconfig
Qemu test results:
total: 318 pass: 280 fail: 38
Failed tests:
riscv:virt:defconfig:initrd
riscv:virt:defconfig:virtio-blk:rootfs
sparc32:SPARCClassic:nosmp:sparc32_defconfig
sparc32:SPARCbook:nosmp:sparc32_defconfig
sparc32:SS-4:nosmp:sparc32_defconfig
sparc32:SS-5:nosmp:sparc32_defconfig
sparc32:SS-10:nosmp:sparc32_defconfig
sparc32:SS-20:nosmp:sparc32_defconfig
sparc32:SS-600MP:nosmp:sparc32_defconfig
sparc32:LX:nosmp:sparc32_defconfig
sparc32:Voyager:nosmp:sparc32_defconfig
sparc32:SPARCClassic:smp:sparc32_defconfig
sparc32:SPARCbook:smp:sparc32_defconfig
sparc32:SS-4:smp:sparc32_defconfig
sparc32:SS-5:smp:sparc32_defconfig
sparc32:SS-10:smp:sparc32_defconfig
sparc32:SS-20:smp:sparc32_defconfig
sparc32:SS-600MP:smp:sparc32_defconfig
sparc32:LX:smp:sparc32_defconfig
sparc32:Voyager:smp:sparc32_defconfig
sparc64:sun4u:smp:initrd
sparc64:sun4u:smp:ata:rootfs
sparc64:sun4u:smp:mmc:rootfs
sparc64:sun4u:smp:nvme:rootfs
sparc64:sun4u:smp:scsi[DC395]:rootfs
sparc64:sun4u:smp:scsi[MEGASAS]:rootfs
sparc64:sun4u:smp:scsi[AM53C974]:rootfs
sparc64:sun4u:smp:usb-xhci:rootfs
sparc64:sun4u:smp:usb-uas-xhci:rootfs
sparc64:sun4u:smp:virtio-pci:rootfs
sparc64:sun4v:smp:ata:rootfs
sparc64:sun4v:smp:nvme:rootfs
sparc64:sun4u:nosmp:ata:rootfs
sparc64:sun4v:nosmp:ata:rootfs
i386:pc:Penryn:defconfig:pae:smp:initrd
i386:pc:Westmere:defconfig:pae:smp:efi32:usb:rootfs
i386:q35:pentium3:defconfig:pae:nosmp:nvme:rootfs
i386:q35:coreduo:defconfig:pae:nosmp:efi32:mmc:rootfs
Details are available at https://kerneltests.org/builders/.
Guenter
---
Compile errors:
alpha, arm64, parisc, powerpc, s390, sparc64, xtensa:
drivers/net/ethernet/chelsio/cxgb4/cxgb4_thermal.c: In function
'cxgb4_thermal_get_trip_type':
drivers/net/ethernet/chelsio/cxgb4/cxgb4_thermal.c:48:11: error: 'struct
adapter' has no member named 'ch_thermal'
mips:
net/sunrpc/auth_gss/gss_krb5_seal.c: In function 'gss_seq_send64_fetch_and_inc':
net/sunrpc/auth_gss/gss_krb5_seal.c:144:3: error: implicit declaration of
function 'cmpxchg64'
riscv:
Building riscv:defconfig ... failed
--
Error log:
cc1: error: -march=c: ISA string must begin with rv32 or rv64
cc1: error: ABI requires -march=rv64
Crashes:
sparc32, sparc64:
(log is from sparc32)
Unable to handle kernel NULL pointer dereference
tsk->{mm,active_mm}->context =
tsk->{mm,active_mm}->pgd = fc00
swapper(0): Oops [#1]
CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc7-next-20181009 #1
PSR: 41801fc2 PC: f03aee08 NPC: f03aee0c Y: Not tainted
PC:
%G: f04d2968 f03b23a4 0001 0066 f048 f0481b40
RPC:
%L: f044c0a8 0020 f0459000 f03afd8c f0444928 f0481d08
%I: f0481d08 f0481d48 f04f5ba0 f0481ba8 f044e72c f0481b28 f0481b48 f03b3e58
Disabling lock debugging due to kernel taint
Caller[f03b3e58]: pointer+0x338/0x5f8
Caller[f03b4338]: vsnprintf+0x220/0x5d0
Caller[f03b4764]: snprintf+0x20/0x30
Caller[f04abddc]: build_path_component+0x1e8/0x214
Caller[f04bbeb8]: of_pdt_build_tree+0x54/0x138
Caller[f04bc020]: of_pdt_build_devicetree+0x84/0x98
Caller[f04abb40]: prom_build_devicetree+0x10/0x30
Caller[f04ad61c]: paging_init+0xc/0x24
Caller[f04ab570]: setup_arch+0x45c/0x494
Caller[f04a7b40]: start_kernel+0x48/0x458
Caller[f04a743c]: continue_boot+0x324/0x334
Caller[]: (null)
x86:
[6.632088] Kernel panic - not syncing: stack-protector: Kernel stack is
corrupted in: pgd_alloc+0x2be/0x2c0
[6.632088]
[6.632712] CPU: 0 PID: 717 Comm: kworker/u2:3 Not tainted
4.19.0-rc7-next-20181009 #1
[6.632712] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.11.2-0-gf9626ccb91-prebuilt.qemu-project.org 04/01/2014
[6.632712] Call Trace:
[6.632712]
Re: [RFC PATCH 0/7] Introduce thermal pressure
On 10/10/18 15:08, Vincent Guittot wrote: > On Wed, 10 Oct 2018 at 14:50, Juri Lelli wrote: > > > > On 10/10/18 14:34, Vincent Guittot wrote: > > > Hi Juri, > > > > > > On Wed, 10 Oct 2018 at 14:23, Juri Lelli wrote: > > > > > > > > On 10/10/18 14:04, Vincent Guittot wrote: > > > > > > > > [...] > > > > > > > > > The problem was the same with RT, the cfs utilization was lower than > > > > > reality because RT steals soem cycle to CFS > > > > > So schedutil was selecting a lower frequency when cfs was running > > > > > whereas the CPU was fully used. > > > > > The same can happen with thermal: > > > > > cap the max freq because of thermal > > > > > the utilization with decrease. > > > > > remove the cap > > > > > the utilization is still low and you will select a low OPP because you > > > > > don't take into account cycle stolen by thermal like with RT > > > > > > > > What if we scale frequency component considering the capped temporary > > > > max? > > > > > > Do you mean using a kind of scale_thermal_capacity in accumulate_sum > > > when computing utilization ? > > > > Yeah, something like that I guess. So that we account for temporary > > "fake" 1024.. > > But the utilization will not be invariant anymore across the system Mmm, I guess I might be wrong, but I was thinking we should be able to deal with this similarly to what we do with cpus with different max capacities. So, another factor? Because then, how do we handle other ways in which max freq can be restricted (e.g. from userspace as Javi was also mentioning)?
Re: [RFC PATCH 0/7] Introduce thermal pressure
On 10/10/18 15:08, Vincent Guittot wrote: > On Wed, 10 Oct 2018 at 14:50, Juri Lelli wrote: > > > > On 10/10/18 14:34, Vincent Guittot wrote: > > > Hi Juri, > > > > > > On Wed, 10 Oct 2018 at 14:23, Juri Lelli wrote: > > > > > > > > On 10/10/18 14:04, Vincent Guittot wrote: > > > > > > > > [...] > > > > > > > > > The problem was the same with RT, the cfs utilization was lower than > > > > > reality because RT steals soem cycle to CFS > > > > > So schedutil was selecting a lower frequency when cfs was running > > > > > whereas the CPU was fully used. > > > > > The same can happen with thermal: > > > > > cap the max freq because of thermal > > > > > the utilization with decrease. > > > > > remove the cap > > > > > the utilization is still low and you will select a low OPP because you > > > > > don't take into account cycle stolen by thermal like with RT > > > > > > > > What if we scale frequency component considering the capped temporary > > > > max? > > > > > > Do you mean using a kind of scale_thermal_capacity in accumulate_sum > > > when computing utilization ? > > > > Yeah, something like that I guess. So that we account for temporary > > "fake" 1024.. > > But the utilization will not be invariant anymore across the system Mmm, I guess I might be wrong, but I was thinking we should be able to deal with this similarly to what we do with cpus with different max capacities. So, another factor? Because then, how do we handle other ways in which max freq can be restricted (e.g. from userspace as Javi was also mentioning)?
Re: [RFC 4/4] gpio: sifive: Add GPIO driver for SiFive SoCs
On Okt 10 2018, Christoph Hellwig wrote:
> On Wed, Oct 10, 2018 at 03:01:29PM +0200, Andreas Schwab wrote:
>> On Okt 09 2018, Atish Patra wrote:
>>
>> > +static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
>> > +{
>> > + unsigned long flags;
>> > + unsigned int trigger;
>> > +
>> > + raw_spin_lock_irqsave(>lock, flags);
>> > + trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
>>
>> This should use test_bit instead.
>
> Given that this apparently needs the spinlock for atomciy with more than
> just the bitmap test_bit would be rather pointless.
BIT and test_bit/assign_bit are not compatible.
Andreas.
--
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Re: [RFC 4/4] gpio: sifive: Add GPIO driver for SiFive SoCs
On Okt 10 2018, Christoph Hellwig wrote:
> On Wed, Oct 10, 2018 at 03:01:29PM +0200, Andreas Schwab wrote:
>> On Okt 09 2018, Atish Patra wrote:
>>
>> > +static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
>> > +{
>> > + unsigned long flags;
>> > + unsigned int trigger;
>> > +
>> > + raw_spin_lock_irqsave(>lock, flags);
>> > + trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
>>
>> This should use test_bit instead.
>
> Given that this apparently needs the spinlock for atomciy with more than
> just the bitmap test_bit would be rather pointless.
BIT and test_bit/assign_bit are not compatible.
Andreas.
--
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wed, 10 Oct 2018 at 15:05, Quentin Perret wrote: > > On Wednesday 10 Oct 2018 at 14:04:40 (+0200), Vincent Guittot wrote: > > This patchset doesn't touch cpu_capacity_orig and doesn't need to as > > it assume that the max capacity is unchanged but some capacity is > > momentary stolen by thermal. > > If you want to reflect immediately all thermal capping change, you > > have to update this field and all related fields and struct around > > I don't follow you here. I never said I wanted to change > cpu_capacity_orig. I don't think we should do that actually. Changing > capacity_of (which is updated during LB IIRC) is just fine. The question > is about what you want to do there: reflect an averaged value or the > instantaneous one. Sorry I though your were speaking about updating this cpu_capacity_orig. With using instantaneous max value in capacity_of(), we are back to the problem raised by Thara that the value will most probably not reflect the current capping value when it is used in LB, because LB period can quite long on busy CPU (default max value is 32*sd_weight ms) > > It's not obvious (to me) that the complex one (the averaged value) is > better than the other, simpler, one. All I'm saying from the beginning > is that it'd be nice to have numbers and use cases to discuss the pros > and cons of both approaches. > > > > > > Hmm, let me have a closer look at the patches, I must have missed > > > > > something ... > > > > > > > > > > > The pace of changing the capping is to fast to reflect that in the > > > > > > whole scheduler topology > > > > > [snip] > > > > > > Well, that wasn't the problem with rt tasks. The problem with RT tasks > > > was that the time they spend on the CPU wasn't accounted _at all_ when > > > selecting frequency for CFS, not that the accounting was at a different > > > pace ... > > > > The problem was the same with RT, the cfs utilization was lower than > > reality because RT steals soem cycle to CFS > > So schedutil was selecting a lower frequency when cfs was running > > whereas the CPU was fully used. > > The same can happen with thermal: > > cap the max freq because of thermal > > the utilization with decrease. > > remove the cap > > the utilization is still low and you will select a low OPP because you > > don't take into account cycle stolen by thermal like with RT > > I'm not arguing with the fact that we need to reflect the thermal > pressure in the scheduler to see that a CPU is fully busy. I agree with > that. > > I'm saying you don't necessarily have to update the thermal stuff and > the existing PELT signals *at the same pace*, because different > platforms have different thermal characteristics. But you also need to take into account how fast other metrics in the scheduler are updated otherwise a metric will reflect a change not already reflected in the other metrics and you might take wrong decision as my example above where utilization is still low but thermal pressure is nul and you assume that you have lot of spare capacity Having metrics that use same responsiveness and are synced, help to get a consolidated view of the system. Vincent > > Thanks,* > Quentin
Re: [RFC PATCH 0/7] Introduce thermal pressure
On Wed, 10 Oct 2018 at 15:05, Quentin Perret wrote: > > On Wednesday 10 Oct 2018 at 14:04:40 (+0200), Vincent Guittot wrote: > > This patchset doesn't touch cpu_capacity_orig and doesn't need to as > > it assume that the max capacity is unchanged but some capacity is > > momentary stolen by thermal. > > If you want to reflect immediately all thermal capping change, you > > have to update this field and all related fields and struct around > > I don't follow you here. I never said I wanted to change > cpu_capacity_orig. I don't think we should do that actually. Changing > capacity_of (which is updated during LB IIRC) is just fine. The question > is about what you want to do there: reflect an averaged value or the > instantaneous one. Sorry I though your were speaking about updating this cpu_capacity_orig. With using instantaneous max value in capacity_of(), we are back to the problem raised by Thara that the value will most probably not reflect the current capping value when it is used in LB, because LB period can quite long on busy CPU (default max value is 32*sd_weight ms) > > It's not obvious (to me) that the complex one (the averaged value) is > better than the other, simpler, one. All I'm saying from the beginning > is that it'd be nice to have numbers and use cases to discuss the pros > and cons of both approaches. > > > > > > Hmm, let me have a closer look at the patches, I must have missed > > > > > something ... > > > > > > > > > > > The pace of changing the capping is to fast to reflect that in the > > > > > > whole scheduler topology > > > > > [snip] > > > > > > Well, that wasn't the problem with rt tasks. The problem with RT tasks > > > was that the time they spend on the CPU wasn't accounted _at all_ when > > > selecting frequency for CFS, not that the accounting was at a different > > > pace ... > > > > The problem was the same with RT, the cfs utilization was lower than > > reality because RT steals soem cycle to CFS > > So schedutil was selecting a lower frequency when cfs was running > > whereas the CPU was fully used. > > The same can happen with thermal: > > cap the max freq because of thermal > > the utilization with decrease. > > remove the cap > > the utilization is still low and you will select a low OPP because you > > don't take into account cycle stolen by thermal like with RT > > I'm not arguing with the fact that we need to reflect the thermal > pressure in the scheduler to see that a CPU is fully busy. I agree with > that. > > I'm saying you don't necessarily have to update the thermal stuff and > the existing PELT signals *at the same pace*, because different > platforms have different thermal characteristics. But you also need to take into account how fast other metrics in the scheduler are updated otherwise a metric will reflect a change not already reflected in the other metrics and you might take wrong decision as my example above where utilization is still low but thermal pressure is nul and you assume that you have lot of spare capacity Having metrics that use same responsiveness and are synced, help to get a consolidated view of the system. Vincent > > Thanks,* > Quentin
Re: [PATCH 1/2] LICENSES: Add note to CDDL-1.0 license that it should not be used
On Wed, 10 Oct 2018, Hans de Goede wrote: > The only reason we have the CDDL-1.0 license text around is for some > dual-licensed files from virtualbox. New code should not use this license. > > Add a note about this and change the example tag to be dual-licensed. > > Signed-off-by: Hans de Goede > --- > LICENSES/other/CDDL-1.0 | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/LICENSES/other/CDDL-1.0 b/LICENSES/other/CDDL-1.0 > index 195a1687930a..164401ea9e62 100644 > --- a/LICENSES/other/CDDL-1.0 > +++ b/LICENSES/other/CDDL-1.0 > @@ -1,10 +1,12 @@ > Valid-License-Identifier: CDDL-1.0 > SPDX-URL: https://spdx.org/licenses/CDDL-1.0.html > Usage-Guide: > + Do NOT use. The CDDL-1.0 is not GPL compatible. It may only be used for > + dual-licensed files where the other license is GPL compatible. >To use the Common Development and Distribution License 1.0 put the >following SPDX tag/value pair into a comment according to the placement >guidelines in the licensing rules documentation: > -SPDX-License-Identifier: CDDL-1.0 > +SPDX-License-Identifier: (GPL-2.0 OR CDDL-1.0) I'd rather replace the 'GPL-2.0' with something like '$GPL-COMPATIBLE-ID' and say explicitely in the prosa text that CCDL can only be used with OR and not with AND. Thanks, tglx
Re: [PATCH 1/2] LICENSES: Add note to CDDL-1.0 license that it should not be used
On Wed, 10 Oct 2018, Hans de Goede wrote: > The only reason we have the CDDL-1.0 license text around is for some > dual-licensed files from virtualbox. New code should not use this license. > > Add a note about this and change the example tag to be dual-licensed. > > Signed-off-by: Hans de Goede > --- > LICENSES/other/CDDL-1.0 | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/LICENSES/other/CDDL-1.0 b/LICENSES/other/CDDL-1.0 > index 195a1687930a..164401ea9e62 100644 > --- a/LICENSES/other/CDDL-1.0 > +++ b/LICENSES/other/CDDL-1.0 > @@ -1,10 +1,12 @@ > Valid-License-Identifier: CDDL-1.0 > SPDX-URL: https://spdx.org/licenses/CDDL-1.0.html > Usage-Guide: > + Do NOT use. The CDDL-1.0 is not GPL compatible. It may only be used for > + dual-licensed files where the other license is GPL compatible. >To use the Common Development and Distribution License 1.0 put the >following SPDX tag/value pair into a comment according to the placement >guidelines in the licensing rules documentation: > -SPDX-License-Identifier: CDDL-1.0 > +SPDX-License-Identifier: (GPL-2.0 OR CDDL-1.0) I'd rather replace the 'GPL-2.0' with something like '$GPL-COMPATIBLE-ID' and say explicitely in the prosa text that CCDL can only be used with OR and not with AND. Thanks, tglx
[tip:x86/mm] x86/mm: Do not warn about PCI BIOS W+X mappings
Commit-ID: c200dac78fec66d87ef262cac38cfe4feabdf737
Gitweb: https://git.kernel.org/tip/c200dac78fec66d87ef262cac38cfe4feabdf737
Author: Thomas Gleixner
AuthorDate: Mon, 8 Oct 2018 21:53:48 +0200
Committer: Thomas Gleixner
CommitDate: Wed, 10 Oct 2018 15:16:13 +0200
x86/mm: Do not warn about PCI BIOS W+X mappings
PCI BIOS requires the BIOS area 0x0A-0x0FF to be mapped W+X for
various legacy reasons. When CONFIG_DEBUG_WX is enabled, this triggers the
WX warning, but this is misleading because the mapping is required and is
not a result of an accidental oversight.
Prevent the full warning when PCI BIOS is enabled and the detected WX
mapping is in the BIOS area. Just emit a pr_warn() which denotes the
fact. This is partially duplicating the info which the PCI BIOS code emits
when it maps the area as executable, but that info is not in the context of
the WX checking output.
Remove the extra %p printout in the WARN_ONCE() while at it. %pS is enough.
Reported-by: Paul Menzel
Signed-off-by: Thomas Gleixner
Tested-by: Borislav Petkov
Cc: Joerg Roedel
Cc: Kees Cook
Cc: Bjorn Helgaas
Link:
https://lkml.kernel.org/r/alpine.deb.2.21.1810082151160.2...@nanos.tec.linutronix.de
---
arch/x86/mm/dump_pagetables.c | 35 +++
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index a12afff146d1..fc37bbd23eb8 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -19,7 +19,9 @@
#include
#include
#include
+#include
+#include
#include
/*
@@ -241,6 +243,29 @@ static unsigned long normalize_addr(unsigned long u)
return (signed long)(u << shift) >> shift;
}
+static void note_wx(struct pg_state *st)
+{
+ unsigned long npages;
+
+ npages = (st->current_address - st->start_address) / PAGE_SIZE;
+
+#ifdef CONFIG_PCI_BIOS
+ /*
+* If PCI BIOS is enabled, the PCI BIOS area is forced to WX.
+* Inform about it, but avoid the warning.
+*/
+ if (pcibios_enabled && st->start_address >= PAGE_OFFSET + BIOS_BEGIN &&
+ st->current_address <= PAGE_OFFSET + BIOS_END) {
+ pr_warn_once("x86/mm: PCI BIOS W+X mapping %lu pages\n",
npages);
+ return;
+ }
+#endif
+ /* Account the WX pages */
+ st->wx_pages += npages;
+ WARN_ONCE(1, "x86/mm: Found insecure W+X mapping at address %pS\n",
+ (void *)st->start_address);
+}
+
/*
* This function gets called on a break in a continuous series
* of PTE entries; the next one is different so we need to
@@ -276,14 +301,8 @@ static void note_page(struct seq_file *m, struct pg_state
*st,
unsigned long delta;
int width = sizeof(unsigned long) * 2;
- if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX)) {
- WARN_ONCE(1,
- "x86/mm: Found insecure W+X mapping at
address %p/%pS\n",
- (void *)st->start_address,
- (void *)st->start_address);
- st->wx_pages += (st->current_address -
-st->start_address) / PAGE_SIZE;
- }
+ if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX))
+ note_wx(st);
/*
* Now print the actual finished series
[tip:x86/mm] x86/mm: Do not warn about PCI BIOS W+X mappings
Commit-ID: c200dac78fec66d87ef262cac38cfe4feabdf737
Gitweb: https://git.kernel.org/tip/c200dac78fec66d87ef262cac38cfe4feabdf737
Author: Thomas Gleixner
AuthorDate: Mon, 8 Oct 2018 21:53:48 +0200
Committer: Thomas Gleixner
CommitDate: Wed, 10 Oct 2018 15:16:13 +0200
x86/mm: Do not warn about PCI BIOS W+X mappings
PCI BIOS requires the BIOS area 0x0A-0x0FF to be mapped W+X for
various legacy reasons. When CONFIG_DEBUG_WX is enabled, this triggers the
WX warning, but this is misleading because the mapping is required and is
not a result of an accidental oversight.
Prevent the full warning when PCI BIOS is enabled and the detected WX
mapping is in the BIOS area. Just emit a pr_warn() which denotes the
fact. This is partially duplicating the info which the PCI BIOS code emits
when it maps the area as executable, but that info is not in the context of
the WX checking output.
Remove the extra %p printout in the WARN_ONCE() while at it. %pS is enough.
Reported-by: Paul Menzel
Signed-off-by: Thomas Gleixner
Tested-by: Borislav Petkov
Cc: Joerg Roedel
Cc: Kees Cook
Cc: Bjorn Helgaas
Link:
https://lkml.kernel.org/r/alpine.deb.2.21.1810082151160.2...@nanos.tec.linutronix.de
---
arch/x86/mm/dump_pagetables.c | 35 +++
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index a12afff146d1..fc37bbd23eb8 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -19,7 +19,9 @@
#include
#include
#include
+#include
+#include
#include
/*
@@ -241,6 +243,29 @@ static unsigned long normalize_addr(unsigned long u)
return (signed long)(u << shift) >> shift;
}
+static void note_wx(struct pg_state *st)
+{
+ unsigned long npages;
+
+ npages = (st->current_address - st->start_address) / PAGE_SIZE;
+
+#ifdef CONFIG_PCI_BIOS
+ /*
+* If PCI BIOS is enabled, the PCI BIOS area is forced to WX.
+* Inform about it, but avoid the warning.
+*/
+ if (pcibios_enabled && st->start_address >= PAGE_OFFSET + BIOS_BEGIN &&
+ st->current_address <= PAGE_OFFSET + BIOS_END) {
+ pr_warn_once("x86/mm: PCI BIOS W+X mapping %lu pages\n",
npages);
+ return;
+ }
+#endif
+ /* Account the WX pages */
+ st->wx_pages += npages;
+ WARN_ONCE(1, "x86/mm: Found insecure W+X mapping at address %pS\n",
+ (void *)st->start_address);
+}
+
/*
* This function gets called on a break in a continuous series
* of PTE entries; the next one is different so we need to
@@ -276,14 +301,8 @@ static void note_page(struct seq_file *m, struct pg_state
*st,
unsigned long delta;
int width = sizeof(unsigned long) * 2;
- if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX)) {
- WARN_ONCE(1,
- "x86/mm: Found insecure W+X mapping at
address %p/%pS\n",
- (void *)st->start_address,
- (void *)st->start_address);
- st->wx_pages += (st->current_address -
-st->start_address) / PAGE_SIZE;
- }
+ if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX))
+ note_wx(st);
/*
* Now print the actual finished series
