Re: [PATCH 7/8] LinuxPPS: pps_gen_parport: Add polarity parameter for inverted signal.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

On some devices it may be necessary to transmit inverted data. This commit
simply adds polarity parameter to define which state represents presence of
signal: it equals 0 if signal is on the low level (default), or 1 if signal
is on the high level (inverted signal).

Signed-off-by: Alexander GQ Gerasiov 
---
 drivers/pps/generators/pps_gen_parport.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pps/generators/pps_gen_parport.c 
b/drivers/pps/generators/pps_gen_parport.c
index dcd39fb..7739301 100644
--- a/drivers/pps/generators/pps_gen_parport.c
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -36,8 +36,8 @@

 #define DRVDESC "parallel port PPS signal generator"

-#define SIGNAL 0
-#define NO_SIGNAL  PARPORT_CONTROL_STROBE
+#define SIGNAL (polarity?PARPORT_CONTROL_STROBE:0)
+#define NO_SIGNAL  (polarity?0:PARPORT_CONTROL_STROBE)


Add spaces:


+#define SIGNAL (polarity ? PARPORT_CONTROL_STROBE : 0)
+#define NO_SIGNAL  (polarity ? 0 : PARPORT_CONTROL_STROBE)



 /* module parameters */

@@ -48,6 +48,10 @@ MODULE_PARM_DESC(delay,
"Delay between setting and dropping the signal (ns)");
 module_param_named(delay, send_delay, uint, 0);

+static unsigned int polarity;
+MODULE_PARM_DESC(polarity,
+   "Signal is on the low level (0 - default) or on the high level (1).");
+module_param(polarity, uint, 0);

 #define SAFETY_INTERVAL3000/* set the hrtimer earlier for safety 
(ns) */


Acked-by: Rodolfo Giometti 

Re: [PATCH 7/8] LinuxPPS: pps_gen_parport: Add polarity parameter for inverted signal.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

On some devices it may be necessary to transmit inverted data. This commit
simply adds polarity parameter to define which state represents presence of
signal: it equals 0 if signal is on the low level (default), or 1 if signal
is on the high level (inverted signal).

Signed-off-by: Alexander GQ Gerasiov 
---
 drivers/pps/generators/pps_gen_parport.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pps/generators/pps_gen_parport.c 
b/drivers/pps/generators/pps_gen_parport.c
index dcd39fb..7739301 100644
--- a/drivers/pps/generators/pps_gen_parport.c
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -36,8 +36,8 @@

 #define DRVDESC "parallel port PPS signal generator"

-#define SIGNAL 0
-#define NO_SIGNAL  PARPORT_CONTROL_STROBE
+#define SIGNAL (polarity?PARPORT_CONTROL_STROBE:0)
+#define NO_SIGNAL  (polarity?0:PARPORT_CONTROL_STROBE)


Add spaces:


+#define SIGNAL (polarity ? PARPORT_CONTROL_STROBE : 0)
+#define NO_SIGNAL  (polarity ? 0 : PARPORT_CONTROL_STROBE)



 /* module parameters */

@@ -48,6 +48,10 @@ MODULE_PARM_DESC(delay,
"Delay between setting and dropping the signal (ns)");
 module_param_named(delay, send_delay, uint, 0);

+static unsigned int polarity;
+MODULE_PARM_DESC(polarity,
+   "Signal is on the low level (0 - default) or on the high level (1).");
+module_param(polarity, uint, 0);

 #define SAFETY_INTERVAL3000/* set the hrtimer earlier for safety 
(ns) */


Acked-by: Rodolfo Giometti 

Re: [PATCH 6/8] LinuxPPS: pps_parport: Ignore interrupt invoked less than 0.5sec after previous.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

On some devices interrupt may be invoked by parasitic assert event produced
while switching from high to low. Such interrupt should be ignored.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Crypto Fixes for 4.11

[Herbert Xu]

Hi Linus:

This push fixes the following issues:

- vmalloc stack regression in CCM.
- Build problem in CRC32 on ARM.
- Memory leak in cavium.
- Missing Kconfig dependencies in atmel and mediatek.
- XTS Regression on some platforms (s390 and ppc).
- Memory overrun in CCM test vector.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Ard Biesheuvel (3):
  crypto: ccm - move cbcmac input off the stack
  crypto: arm/crc32 - fix build error with outdated binutils
  crypto: arm/crc32 - add build time test for CRC instruction support

Colin Ian King (1):
  crypto: cavium - fix leak on curr if curr->head fails to be allocated

Geert Uytterhoeven (2):
  crypto: atmel - CRYPTO_DEV_ATMEL_TDES and CRYPTO_DEV_ATMEL_SHA should 
depend on HAS_DMA
  crypto: atmel - CRYPTO_DEV_MEDIATEK should depend on HAS_DMA

George Cherian (1):
  crypto: cavium - Fix couple of static checker errors

Herbert Xu (2):
  crypto: api - Add crypto_requires_off helper
  crypto: xts - Propagate NEED_FALLBACK bit

Laura Abbott (1):
  crypto: testmgr - Pad aes_ccm_enc_tv_template vector

Paulo Flabiano Smorigo (2):
  crypto: vmx - Use skcipher for cbc fallback
  crypto: vmx - Use skcipher for xts fallback

 arch/arm/crypto/Makefile |   12 ++-
 arch/arm/crypto/crc32-ce-core.S  |2 +-
 crypto/ccm.c |5 +--
 crypto/testmgr.h |2 +-
 crypto/xts.c |   14 
 drivers/crypto/Kconfig   |3 ++
 drivers/crypto/cavium/cpt/cptvf_main.c   |5 ++-
 drivers/crypto/cavium/cpt/cptvf_reqmanager.c |4 +--
 drivers/crypto/vmx/aes_cbc.c |   47 +-
 drivers/crypto/vmx/aes_xts.c |   32 +-
 include/crypto/algapi.h  |7 +++-
 11 files changed, 79 insertions(+), 54 deletions(-)

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] pps: add ioctl_compat function to correct ioctl definitions

[Rodolfo Giometti]

On 02/24/17 21:23, Matt Ranostay wrote:

ioctl definitions use the pointer size of the architecture which
is fine when userspace and kernel are the same bitsize. This
patchset workarounds an issue with mixed bitsize kernel + userspace
by rewriting the cmd to the kernelspace architecture pointer size.

Cc: Rodolfo Giometti 
Cc: Moritz Fischer 
Cc: George McCollister 
Signed-off-by: Matt Ranostay 
---
 drivers/pps/pps.c | 13 +
 1 file changed, 13 insertions(+)


Acked-by: Rodolfo Giometti 

--

HCE Engineering  e-mail: giome...@hce-engineering.com
GNU/Linux Solutions  giome...@enneenne.com
Linux Device Driver  giome...@linux.it
Embedded Systems phone:  +39 349 2432127
UNIX programming skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.io
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH 6/8] LinuxPPS: pps_parport: Ignore interrupt invoked less than 0.5sec after previous.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

On some devices interrupt may be invoked by parasitic assert event produced
while switching from high to low. Such interrupt should be ignored.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Crypto Fixes for 4.11

[Herbert Xu]

Hi Linus:

This push fixes the following issues:

- vmalloc stack regression in CCM.
- Build problem in CRC32 on ARM.
- Memory leak in cavium.
- Missing Kconfig dependencies in atmel and mediatek.
- XTS Regression on some platforms (s390 and ppc).
- Memory overrun in CCM test vector.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Ard Biesheuvel (3):
  crypto: ccm - move cbcmac input off the stack
  crypto: arm/crc32 - fix build error with outdated binutils
  crypto: arm/crc32 - add build time test for CRC instruction support

Colin Ian King (1):
  crypto: cavium - fix leak on curr if curr->head fails to be allocated

Geert Uytterhoeven (2):
  crypto: atmel - CRYPTO_DEV_ATMEL_TDES and CRYPTO_DEV_ATMEL_SHA should 
depend on HAS_DMA
  crypto: atmel - CRYPTO_DEV_MEDIATEK should depend on HAS_DMA

George Cherian (1):
  crypto: cavium - Fix couple of static checker errors

Herbert Xu (2):
  crypto: api - Add crypto_requires_off helper
  crypto: xts - Propagate NEED_FALLBACK bit

Laura Abbott (1):
  crypto: testmgr - Pad aes_ccm_enc_tv_template vector

Paulo Flabiano Smorigo (2):
  crypto: vmx - Use skcipher for cbc fallback
  crypto: vmx - Use skcipher for xts fallback

 arch/arm/crypto/Makefile |   12 ++-
 arch/arm/crypto/crc32-ce-core.S  |2 +-
 crypto/ccm.c |5 +--
 crypto/testmgr.h |2 +-
 crypto/xts.c |   14 
 drivers/crypto/Kconfig   |3 ++
 drivers/crypto/cavium/cpt/cptvf_main.c   |5 ++-
 drivers/crypto/cavium/cpt/cptvf_reqmanager.c |4 +--
 drivers/crypto/vmx/aes_cbc.c |   47 +-
 drivers/crypto/vmx/aes_xts.c |   32 +-
 include/crypto/algapi.h  |7 +++-
 11 files changed, 79 insertions(+), 54 deletions(-)

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] pps: add ioctl_compat function to correct ioctl definitions

[Rodolfo Giometti]

On 02/24/17 21:23, Matt Ranostay wrote:

ioctl definitions use the pointer size of the architecture which
is fine when userspace and kernel are the same bitsize. This
patchset workarounds an issue with mixed bitsize kernel + userspace
by rewriting the cmd to the kernelspace architecture pointer size.

Cc: Rodolfo Giometti 
Cc: Moritz Fischer 
Cc: George McCollister 
Signed-off-by: Matt Ranostay 
---
 drivers/pps/pps.c | 13 +
 1 file changed, 13 insertions(+)


Acked-by: Rodolfo Giometti 

--

HCE Engineering  e-mail: giome...@hce-engineering.com
GNU/Linux Solutions  giome...@enneenne.com
Linux Device Driver  giome...@linux.it
Embedded Systems phone:  +39 349 2432127
UNIX programming skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.io
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH 5/8] LinuxPPS: pps_parport: Do not generate assert in case of lost signal.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Since clear timeout is non-zero, some clear event capture is requested.
Therefore, if signal is lost we shouldn't generate assert event alone.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 5/8] LinuxPPS: pps_parport: Do not generate assert in case of lost signal.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Since clear timeout is non-zero, some clear event capture is requested.
Therefore, if signal is lost we shouldn't generate assert event alone.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 4/8] LinuxPPS: kapi: Unlock before waking up events queue in pps_event().

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Otherwise we get "scheduling while atomic" problem.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 4/8] LinuxPPS: kapi: Unlock before waking up events queue in pps_event().

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Otherwise we get "scheduling while atomic" problem.

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 3/8] hardpps: fix some pps_jitter issues.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

Handle possible overflow, implementation-defined result of signed right shift
and replace unsuitable constant.

Signed-off-by: Andrey Drobyshev 
Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 3/8] hardpps: fix some pps_jitter issues.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

Handle possible overflow, implementation-defined result of signed right shift
and replace unsuitable constant.

Signed-off-by: Andrey Drobyshev 
Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 1/8] hardpps: simple fixes replacing clumsy code with abs() macro.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Here are some trivial fixes, including:
  * simplify comparisons by using abs() macro
  * remove unnecessary variables
  * remove duplicate headers
  * fix typos

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [PATCH 1/8] hardpps: simple fixes replacing clumsy code with abs() macro.

[Rodolfo Giometti]

On 02/15/17 15:31, Andrey Drobyshev wrote:

From: Alexander GQ Gerasiov 

Here are some trivial fixes, including:
  * simplify comparisons by using abs() macro
  * remove unnecessary variables
  * remove duplicate headers
  * fix typos

Signed-off-by: Alexander GQ Gerasiov 


Acked-by: Rodolfo Giometti 

Re: [GIT PULL] sched.h split-up

[Ingo Molnar]

* Linus Torvalds  wrote:

> Anyway, I fixed the semantic merge error by just including thar
>  file, but this is just not acceptable.
> 
> Having to have files know that if they use the wait-event functins
> (well, _some_ of them), they not only need to include ,
> they need to completely illogically also include
>  is just not maintainable.

Absolutely and agreed, will fix this ASAP!

wait.h generates too large functions anyway, so uninlining parts of it will be 
an 
improvement anyway.

> And who knows what similar semantic cases I _won't_ notice, because
> they occur in code I don't build even with my allmodconfig builds?

I did a fair amount of allyes/allno/allmod plus randconfig testing on x86, and 
caught and fixed a number of cases, so that angle should be covered to a fair 
degree.

On non-x86 I did allyes/allno/allmod testing as well, but not randconfig 
testing - 
plus some of the architectures have a large amount of defconfigs which I 
couldn't 
all test through.

So I'd expect build breakages to be concentrated into newly upstreamed code 
(such 
as this one) - which risk I tried to reduce by re-testing them on the 
almost-last 
day of the merge window.

Thanks,

Ingo

Re: [GIT PULL] sched.h split-up

[Ingo Molnar]

* Linus Torvalds  wrote:

> Anyway, I fixed the semantic merge error by just including thar
>  file, but this is just not acceptable.
> 
> Having to have files know that if they use the wait-event functins
> (well, _some_ of them), they not only need to include ,
> they need to completely illogically also include
>  is just not maintainable.

Absolutely and agreed, will fix this ASAP!

wait.h generates too large functions anyway, so uninlining parts of it will be 
an 
improvement anyway.

> And who knows what similar semantic cases I _won't_ notice, because
> they occur in code I don't build even with my allmodconfig builds?

I did a fair amount of allyes/allno/allmod plus randconfig testing on x86, and 
caught and fixed a number of cases, so that angle should be covered to a fair 
degree.

On non-x86 I did allyes/allno/allmod testing as well, but not randconfig 
testing - 
plus some of the architectures have a large amount of defconfigs which I 
couldn't 
all test through.

So I'd expect build breakages to be concentrated into newly upstreamed code 
(such 
as this one) - which risk I tried to reduce by re-testing them on the 
almost-last 
day of the merge window.

Thanks,

Ingo

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Sat, 2017-03-04 at 08:49 +0200, Ernestas Kulik wrote:
> On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote:
> > 
> > struct hostif_hdr.event is declared at uint16_t
> > and not as __le16 so I believe this is incorrect
> > and actually introduces a sparse error.
> 
> Sure, but I change that in the patch. :)

More stuff the changelog doesn't show :(

On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> This fixes type warnings generated by sparse, replaces instances of
> ntohs() with be16_to_cpu() and removes unused fields in structs.

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Sat, 2017-03-04 at 08:49 +0200, Ernestas Kulik wrote:
> On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote:
> > 
> > struct hostif_hdr.event is declared at uint16_t
> > and not as __le16 so I believe this is incorrect
> > and actually introduces a sparse error.
> 
> Sure, but I change that in the patch. :)

More stuff the changelog doesn't show :(

On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> This fixes type warnings generated by sparse, replaces instances of
> ntohs() with be16_to_cpu() and removes unused fields in structs.

[PATCH v2] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

yong mao (1):
  mmc: mediatek: Fixed bug where clock frequency could be set wrong

 drivers/mmc/host/mtk-sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.1.1.dirty

[PATCH v2] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

yong mao (1):
  mmc: mediatek: Fixed bug where clock frequency could be set wrong

 drivers/mmc/host/mtk-sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.1.1.dirty

[PATCH] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

From: yong mao 

This patch can fix two issues:

Issue 1:
In previous code, div may be overflow when setting clock frequency
as f_min. We can use DIV_ROUND_UP to fix this boundary related
issue.

Issue 2:
In previous code, we can not set the correct clock frequency when
div equals 0xff.

Signed-off-by: Yong Mao 
Signed-off-by: Chaotian Jing 
---
 drivers/mmc/host/mtk-sd.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 07f3236..3ad5228 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -591,7 +591,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned 
char timing, u32 hz)
}
}
sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD | MSDC_CFG_CKDIV,
-   (mode << 8) | (div % 0xff));
+ (mode << 8) | div);
sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_CKPDN);
while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
cpu_relax();
@@ -1692,7 +1692,7 @@ static int msdc_drv_probe(struct platform_device *pdev)
host->src_clk_freq = clk_get_rate(host->src_clk);
/* Set host parameters to mmc */
mmc->ops = _msdc_ops;
-   mmc->f_min = host->src_clk_freq / (4 * 255);
+   mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 255);
 
mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
/* MMC core transfer sizes tunable parameters */
-- 
1.7.9.5

[PATCH] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

From: yong mao 

This patch can fix two issues:

Issue 1:
In previous code, div may be overflow when setting clock frequency
as f_min. We can use DIV_ROUND_UP to fix this boundary related
issue.

Issue 2:
In previous code, we can not set the correct clock frequency when
div equals 0xff.

Signed-off-by: Yong Mao 
Signed-off-by: Chaotian Jing 
---
 drivers/mmc/host/mtk-sd.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 07f3236..3ad5228 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -591,7 +591,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned 
char timing, u32 hz)
}
}
sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD | MSDC_CFG_CKDIV,
-   (mode << 8) | (div % 0xff));
+ (mode << 8) | div);
sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_CKPDN);
while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
cpu_relax();
@@ -1692,7 +1692,7 @@ static int msdc_drv_probe(struct platform_device *pdev)
host->src_clk_freq = clk_get_rate(host->src_clk);
/* Set host parameters to mmc */
mmc->ops = _msdc_ops;
-   mmc->f_min = host->src_clk_freq / (4 * 255);
+   mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 255);
 
mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
/* MMC core transfer sizes tunable parameters */
-- 
1.7.9.5

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

On Thu, 2017-03-02 at 20:20 +0800, Daniel Kurtz wrote:
> On Wed, Mar 1, 2017 at 5:56 PM, Yong Mao  wrote:
> > On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:
> >> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao  wrote:
> >> > From:   Yong Mao 
> >> > To: Daniel Kurtz 
> >> > Subject:Re: [PATCH v1] mmc: mediatek: Fixed bug where clock 
> >> > frequency
> >> > could be set wrong
> >> > Date:   Fri, 24 Feb 2017 17:33:37 +0800
> >> >
> >> >
> >> > On Fri, 2017-02-24 at 17:52 +0900, Daniel Kurtz wrote:
> >> >> On Fri, Feb 24, 2017 at 5:22 PM, Yong Mao 
> >> > wrote:
> >> >> >
> >> >> > From: yong mao 
> >> >> >
> >> >> > This patch can fix two issues:
> >> >> >
> >> >> > Issue 1:
> >> >> > The maximum value of clock divider is 0xff.
> >> >> > Because the type of div is u32, div may be larger than max_div.
> >> >> > In this case, we should use max_div to set the clock frequency.
> >> >> >
> >> >> > Issue 2:
> >> >> > In previous code, we can not set the correct clock frequency when
> >> >> > div equals 0xff.
> >> >> >
> >> >> > Signed-off-by: Yong Mao 
> >> >> > Signed-off-by: Chaotian Jing 
> >> >> > ---
> >> >> >  drivers/mmc/host/mtk-sd.c |   13 -
> >> >> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> >> >> > index 07f3236..3174445 100644
> >> >> > --- a/drivers/mmc/host/mtk-sd.c
> >> >> > +++ b/drivers/mmc/host/mtk-sd.c
> >> >> > @@ -540,6 +540,7 @@ static void msdc_set_mclk(struct msdc_host
> >> > *host, unsigned char timing, u32 hz)
> >> >> > u32 mode;
> >> >> > u32 flags;
> >> >> > u32 div;
> >> >> > +   u32 max_div;
> >> >>
> >> >> There's really no need for this variable.  Just use 0xff below.
> >> > For all of our IC, max_div is not a constant.
> >> > We will upstream another patch which max_div will get the different
> >> > value depending on the IC.
> >> > Therefore, we keep the max_div as a variable here.
> >>
> >> Please add the variable in the patch that uses it as a variable.
> >>
> >> >
> >> >>
> >> >> > u32 sclk;
> >> >> >
> >> >> > if (!hz) {
> >> >> > @@ -590,8 +591,18 @@ static void msdc_set_mclk(struct msdc_host
> >> > *host, unsigned char timing, u32 hz)
> >> >> > sclk = (host->src_clk_freq >> 2) / div;
> >> >> > }
> >> >> > }
> >> >> > +
> >> >> > +   /**
> >> >> > +* The maximum value of div is 0xff.
> >> >> > +* Check if the div is larger than max_div.
> >> >> > +*/
> >> >> > +   max_div = 0xff;
> >> >> > +   if (div > max_div) {
> >> >> > +   div = max_div;
> >> >> > +   sclk = (host->src_clk_freq >> 2) / div;
> >> >> > +   }
> >> >> > sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD |
> >> > MSDC_CFG_CKDIV,
> >> >> > -   (mode << 8) | (div % 0xff));
> >> >> > + (mode << 8) | div);
> >> >>
> >> >> Hmm, I don't know much about this sub-system, but should we even be
> >> >> allowing requests to set a frequency that we can't actually achieve
> >> >> with the divider?
> >> >>
> >> >
> >> > No. We can not get a frequency that we can't actually achieve with the
> >> > divider. This patch is to solve this kind of issue.
> >>
> >> Sorry, I am trying to understand why we need this patch.
> >>
> >> AFAICT, it looks like sometimes msdc_set_mclk() is being called with
> >> hz that cannot be generated by your hardware.  In particular,
> >> sometimes the original code computes "div > 255".
> >> To work around this problem, this patch just caps the divider value to
> >> 255, which is the maximum divider provided by the hardware.  However,
> >> presumably this means that in this case we won't actually be
> >> generating the requested hz value.
> >>
> >> So, can you please explain in what exact scenario this patch is
> >> required, and justify why it is ok to generate a clock other than the
> >> requested in this case?
> >>
> >> -Dan
> >>
> >
> > This issue is hidden deeply. It is a boundary related issue.
> > Let me take the real value to explain how this issue occurs.
> >
> > 1. mmc->f_min = host->src_clk_freq / (4 * 255);
> >mmc->f_min = 4 / (4 * 255) = 392156;
> > 2. mmc core tries to initialize emmc by using 40hz.
> >If the first try failed, it will retry by using max(30hz,
> > mmc->f_min)= 392156hz
> > 3. msdc_set_mclk will be invoked by mmc core to set the clock.
> >and then following code will be executed.
> > div = (host->src_clk_freq + ((hz << 2) - 1)) / (hz << 2);
> > (div = (4 + (392156 << 2) - 1)) / (392156 << 2) = 256)
> >
> > Why do we use (host->src_clk_freq + ((hz << 2) - 1))?
> > ==> Because if we can not get a proper clock frequency, we should 

Re: [PATCH v1] mmc: mediatek: Fixed bug where clock frequency could be set wrong

[Yong Mao]

On Thu, 2017-03-02 at 20:20 +0800, Daniel Kurtz wrote:
> On Wed, Mar 1, 2017 at 5:56 PM, Yong Mao  wrote:
> > On Tue, 2017-02-28 at 14:56 +0800, Daniel Kurtz wrote:
> >> On Fri, Feb 24, 2017 at 5:38 PM, Yong Mao  wrote:
> >> > From:   Yong Mao 
> >> > To: Daniel Kurtz 
> >> > Subject:Re: [PATCH v1] mmc: mediatek: Fixed bug where clock 
> >> > frequency
> >> > could be set wrong
> >> > Date:   Fri, 24 Feb 2017 17:33:37 +0800
> >> >
> >> >
> >> > On Fri, 2017-02-24 at 17:52 +0900, Daniel Kurtz wrote:
> >> >> On Fri, Feb 24, 2017 at 5:22 PM, Yong Mao 
> >> > wrote:
> >> >> >
> >> >> > From: yong mao 
> >> >> >
> >> >> > This patch can fix two issues:
> >> >> >
> >> >> > Issue 1:
> >> >> > The maximum value of clock divider is 0xff.
> >> >> > Because the type of div is u32, div may be larger than max_div.
> >> >> > In this case, we should use max_div to set the clock frequency.
> >> >> >
> >> >> > Issue 2:
> >> >> > In previous code, we can not set the correct clock frequency when
> >> >> > div equals 0xff.
> >> >> >
> >> >> > Signed-off-by: Yong Mao 
> >> >> > Signed-off-by: Chaotian Jing 
> >> >> > ---
> >> >> >  drivers/mmc/host/mtk-sd.c |   13 -
> >> >> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> >> >> > index 07f3236..3174445 100644
> >> >> > --- a/drivers/mmc/host/mtk-sd.c
> >> >> > +++ b/drivers/mmc/host/mtk-sd.c
> >> >> > @@ -540,6 +540,7 @@ static void msdc_set_mclk(struct msdc_host
> >> > *host, unsigned char timing, u32 hz)
> >> >> > u32 mode;
> >> >> > u32 flags;
> >> >> > u32 div;
> >> >> > +   u32 max_div;
> >> >>
> >> >> There's really no need for this variable.  Just use 0xff below.
> >> > For all of our IC, max_div is not a constant.
> >> > We will upstream another patch which max_div will get the different
> >> > value depending on the IC.
> >> > Therefore, we keep the max_div as a variable here.
> >>
> >> Please add the variable in the patch that uses it as a variable.
> >>
> >> >
> >> >>
> >> >> > u32 sclk;
> >> >> >
> >> >> > if (!hz) {
> >> >> > @@ -590,8 +591,18 @@ static void msdc_set_mclk(struct msdc_host
> >> > *host, unsigned char timing, u32 hz)
> >> >> > sclk = (host->src_clk_freq >> 2) / div;
> >> >> > }
> >> >> > }
> >> >> > +
> >> >> > +   /**
> >> >> > +* The maximum value of div is 0xff.
> >> >> > +* Check if the div is larger than max_div.
> >> >> > +*/
> >> >> > +   max_div = 0xff;
> >> >> > +   if (div > max_div) {
> >> >> > +   div = max_div;
> >> >> > +   sclk = (host->src_clk_freq >> 2) / div;
> >> >> > +   }
> >> >> > sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD |
> >> > MSDC_CFG_CKDIV,
> >> >> > -   (mode << 8) | (div % 0xff));
> >> >> > + (mode << 8) | div);
> >> >>
> >> >> Hmm, I don't know much about this sub-system, but should we even be
> >> >> allowing requests to set a frequency that we can't actually achieve
> >> >> with the divider?
> >> >>
> >> >
> >> > No. We can not get a frequency that we can't actually achieve with the
> >> > divider. This patch is to solve this kind of issue.
> >>
> >> Sorry, I am trying to understand why we need this patch.
> >>
> >> AFAICT, it looks like sometimes msdc_set_mclk() is being called with
> >> hz that cannot be generated by your hardware.  In particular,
> >> sometimes the original code computes "div > 255".
> >> To work around this problem, this patch just caps the divider value to
> >> 255, which is the maximum divider provided by the hardware.  However,
> >> presumably this means that in this case we won't actually be
> >> generating the requested hz value.
> >>
> >> So, can you please explain in what exact scenario this patch is
> >> required, and justify why it is ok to generate a clock other than the
> >> requested in this case?
> >>
> >> -Dan
> >>
> >
> > This issue is hidden deeply. It is a boundary related issue.
> > Let me take the real value to explain how this issue occurs.
> >
> > 1. mmc->f_min = host->src_clk_freq / (4 * 255);
> >mmc->f_min = 4 / (4 * 255) = 392156;
> > 2. mmc core tries to initialize emmc by using 40hz.
> >If the first try failed, it will retry by using max(30hz,
> > mmc->f_min)= 392156hz
> > 3. msdc_set_mclk will be invoked by mmc core to set the clock.
> >and then following code will be executed.
> > div = (host->src_clk_freq + ((hz << 2) - 1)) / (hz << 2);
> > (div = (4 + (392156 << 2) - 1)) / (392156 << 2) = 256)
> >
> > Why do we use (host->src_clk_freq + ((hz << 2) - 1))?
> > ==> Because if we can not get a proper clock frequency, we should set a
> > clock frequency which is less than proper clock frequency.
> >
> > 4. In this IC, it only use 8 bits to indicate the value of clock
> > divider. Therefore, 256 is overflow, it is 

Re: [PATCH] staging: ks7010: clean up code

[Ernestas Kulik]

On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote:
>
> struct hostif_hdr.event is declared at uint16_t
> and not as __le16 so I believe this is incorrect
> and actually introduces a sparse error.

Sure, but I change that in the patch. :)

On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote:
>
> struct hostif_hdr {
>-  uint16_t size;
>-  uint16_t event;
>+  __le16 size;
>+  __le16 event;
> } __packed;

Re: [PATCH] staging: ks7010: clean up code

[Ernestas Kulik]

On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote:
>
> struct hostif_hdr.event is declared at uint16_t
> and not as __le16 so I believe this is incorrect
> and actually introduces a sparse error.

Sure, but I change that in the patch. :)

On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote:
>
> struct hostif_hdr {
>-  uint16_t size;
>-  uint16_t event;
>+  __le16 size;
>+  __le16 event;
> } __packed;

Re: Is it really safe to use workqueues to drive expedited grace periods?

[Paul E. McKenney]

On Fri, Mar 03, 2017 at 11:30:49AM -0800, Tejun Heo wrote:
> Hello, Paul.
> 
> Sorry about the long delay.  Travel + sickness.  Just starting to
> catch up with things now.

No problem, I have been distracted by other projects in any case.  ;-)

> On Mon, Feb 13, 2017 at 04:16:00PM -0800, Paul E. McKenney wrote:
> > Thank you for the information!  So if I am to continue using workqueues
> > for expedited RCU grace periods, I believe that need to do the following:
> > 
> > 1.  Use alloc_workqueue() to create my own WQ_MEM_RECLAIM
> > workqueue.
> 
> This is only necessary if RCU can be in a dependency chain in the
> memory reclaim path - e.g. somebody doing synchronize_expedited_rcu()
> in some obscure writeback path; however, given how wildly RCU is used,
> I don't think this is a bad idea.  The only extra overhead which comes
> from it is memory used for an extra workqueue and a rescuer thread
> after all.

I suspect that SLAB_DESTROY_BY_RCU qualifies -- such a slab allocator
could have a great many slabs waiting for an RCU grace period.

> > 2.  Rework my workqueue handler to avoid blocking waiting for
> > the expedited grace period to complete.  I should be able
> > to do a small number of timed wait, but if I actually
> > wait for the grace period to complete, I might end up
> > hogging the reserved items.  (Or does my workqueue supply
> > them for me?  If so, so much the better!)
> 
> So, what the dedicated workqueue w/ WQ_MEMRECLAIM guarantees is that
> there will always be at least one worker thread which is executing
> work items from the workqueue.
> 
> As long as your workqueue usage guarantees forward progress - that is,
> as long as one work item in the workqueue won't block indefinitely on
> another work item on the same workqueue, you shouldn't need to reword
> the workqueue handler.
> 
> If there can be dependency chain among work items of the same
> WQ_MEMRECLAIM workqueue, often the best approach is breaking up the
> chain and putting them into their own WQ_MEMRECLAIM workqueues.

Thank you, good to know!

> > 3.  Concurrency would not be a problem -- there can be no more
> > four work elements in flight across both possible flavors
> > of expedited grace periods.
> 
> You usually don't have to worry about concurrency all that much with
> workqueues.  They'll provide the maximum the system can as long as
> that's possible.
> 
> If the four work items can depend on each other, it'd be best to put
> them in separate workqueues.  If not, there's nothing to worry about.

For a given pair, one can be acquiring a mutext that the other holds.
Ah, so if only one task was running, that would fail to make forward
progress.  That said, splitting would be a bit weird in this case,
because alternating grace periods for a given RCU flavor would need
to schedule work on a different workqueue.  Might be easier to do
mutex_trylock() and reschedule...

And thank you for the info, very helpful!

Thanx, Paul

Re: Is it really safe to use workqueues to drive expedited grace periods?

[Paul E. McKenney]

On Fri, Mar 03, 2017 at 11:30:49AM -0800, Tejun Heo wrote:
> Hello, Paul.
> 
> Sorry about the long delay.  Travel + sickness.  Just starting to
> catch up with things now.

No problem, I have been distracted by other projects in any case.  ;-)

> On Mon, Feb 13, 2017 at 04:16:00PM -0800, Paul E. McKenney wrote:
> > Thank you for the information!  So if I am to continue using workqueues
> > for expedited RCU grace periods, I believe that need to do the following:
> > 
> > 1.  Use alloc_workqueue() to create my own WQ_MEM_RECLAIM
> > workqueue.
> 
> This is only necessary if RCU can be in a dependency chain in the
> memory reclaim path - e.g. somebody doing synchronize_expedited_rcu()
> in some obscure writeback path; however, given how wildly RCU is used,
> I don't think this is a bad idea.  The only extra overhead which comes
> from it is memory used for an extra workqueue and a rescuer thread
> after all.

I suspect that SLAB_DESTROY_BY_RCU qualifies -- such a slab allocator
could have a great many slabs waiting for an RCU grace period.

> > 2.  Rework my workqueue handler to avoid blocking waiting for
> > the expedited grace period to complete.  I should be able
> > to do a small number of timed wait, but if I actually
> > wait for the grace period to complete, I might end up
> > hogging the reserved items.  (Or does my workqueue supply
> > them for me?  If so, so much the better!)
> 
> So, what the dedicated workqueue w/ WQ_MEMRECLAIM guarantees is that
> there will always be at least one worker thread which is executing
> work items from the workqueue.
> 
> As long as your workqueue usage guarantees forward progress - that is,
> as long as one work item in the workqueue won't block indefinitely on
> another work item on the same workqueue, you shouldn't need to reword
> the workqueue handler.
> 
> If there can be dependency chain among work items of the same
> WQ_MEMRECLAIM workqueue, often the best approach is breaking up the
> chain and putting them into their own WQ_MEMRECLAIM workqueues.

Thank you, good to know!

> > 3.  Concurrency would not be a problem -- there can be no more
> > four work elements in flight across both possible flavors
> > of expedited grace periods.
> 
> You usually don't have to worry about concurrency all that much with
> workqueues.  They'll provide the maximum the system can as long as
> that's possible.
> 
> If the four work items can depend on each other, it'd be best to put
> them in separate workqueues.  If not, there's nothing to worry about.

For a given pair, one can be acquiring a mutext that the other holds.
Ah, so if only one task was running, that would fail to make forward
progress.  That said, splitting would be a bit weird in this case,
because alternating grace periods for a given RCU flavor would need
to schedule work on a different workqueue.  Might be easier to do
mutex_trylock() and reschedule...

And thank you for the info, very helpful!

Thanx, Paul

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote:
> On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote:
> 
> > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> > 
> > > This fixes type warnings generated by sparse, replaces instances of
> > > ntohs() with be16_to_cpu() and removes unused fields in structs.
> > 
> > There's no real need to convert ntohs to be16_to_cpu
> 
> I noticed a patch that was merged that replaces calls to these with the
> more “generic” be16_to_cpu(), but I can revert that.
> 
> This kind of conversion also happened in SeaBIOS, but it’s not exactly
> the same thing as here.
> 
> > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> > > b/drivers/staging/ks7010/ks7010_sdio.c
> > 
> > []
> > > 
> > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private 
> > > *priv, unsigned char *buffer,
> > >   hdr = (struct hostif_hdr *)buffer;
> > >  
> > >   DPRINTK(4, "size=%d\n", hdr->size);
> > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> > > 
> > 
> > This isn't mentioned and doesn't match the commit message
> 
> It is and it does, but the commit message is a bit vague. I’ll try to
> expand it.

Not really.

struct hostif_hdr.event is declared at uint16_t
and not as __le16 so I believe this is incorrect
and actually introduces a sparse error.

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote:
> On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote:
> 
> > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> > 
> > > This fixes type warnings generated by sparse, replaces instances of
> > > ntohs() with be16_to_cpu() and removes unused fields in structs.
> > 
> > There's no real need to convert ntohs to be16_to_cpu
> 
> I noticed a patch that was merged that replaces calls to these with the
> more “generic” be16_to_cpu(), but I can revert that.
> 
> This kind of conversion also happened in SeaBIOS, but it’s not exactly
> the same thing as here.
> 
> > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> > > b/drivers/staging/ks7010/ks7010_sdio.c
> > 
> > []
> > > 
> > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private 
> > > *priv, unsigned char *buffer,
> > >   hdr = (struct hostif_hdr *)buffer;
> > >  
> > >   DPRINTK(4, "size=%d\n", hdr->size);
> > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> > > 
> > 
> > This isn't mentioned and doesn't match the commit message
> 
> It is and it does, but the commit message is a bit vague. I’ll try to
> expand it.

Not really.

struct hostif_hdr.event is declared at uint16_t
and not as __le16 so I believe this is incorrect
and actually introduces a sparse error.

Re: [PATCH] staging: ks7010: clean up code

[Ernestas Kulik]

On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote:

> On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
>
> > This fixes type warnings generated by sparse, replaces instances of
> > ntohs() with be16_to_cpu() and removes unused fields in structs.
> 
> There's no real need to convert ntohs to be16_to_cpu

I noticed a patch that was merged that replaces calls to these with the
more “generic” be16_to_cpu(), but I can revert that.

This kind of conversion also happened in SeaBIOS, but it’s not exactly
the same thing as here.

> > diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> > b/drivers/staging/ks7010/ks7010_sdio.c
> []
> > 
> > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private 
> > *priv, unsigned char *buffer,
> > hdr = (struct hostif_hdr *)buffer;
> >  
> > DPRINTK(4, "size=%d\n", hdr->size);
> > -   if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> > +   if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> > +   HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> > 
> 
> This isn't mentioned and doesn't match the commit message

It is and it does, but the commit message is a bit vague. I’ll try to
expand it.

Re: [PATCH] staging: ks7010: clean up code

[Ernestas Kulik]

On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote:

> On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
>
> > This fixes type warnings generated by sparse, replaces instances of
> > ntohs() with be16_to_cpu() and removes unused fields in structs.
> 
> There's no real need to convert ntohs to be16_to_cpu

I noticed a patch that was merged that replaces calls to these with the
more “generic” be16_to_cpu(), but I can revert that.

This kind of conversion also happened in SeaBIOS, but it’s not exactly
the same thing as here.

> > diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> > b/drivers/staging/ks7010/ks7010_sdio.c
> []
> > 
> > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private 
> > *priv, unsigned char *buffer,
> > hdr = (struct hostif_hdr *)buffer;
> >  
> > DPRINTK(4, "size=%d\n", hdr->size);
> > -   if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> > +   if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> > +   HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> > 
> 
> This isn't mentioned and doesn't match the commit message

It is and it does, but the commit message is a bit vague. I’ll try to
expand it.

Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:51:28PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Move I2C-specific code into its own file and rely on regmap to access
> > registers. The core code provides access to x, y, z and scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_I2C
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> > Driver"
> 
> > depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> I'm wondering if it works in a form of
> 
> depends on INPUT_ADXL34X=n
> 

Yes it works. Will change it into this form.

> > +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> > +const char *name);
> > +int adxl345_common_remove(struct device *dev);
> 
> I think a "common" word is redundant.
> 

OK. I will use "core" instead.

> > - * IIO driver for ADXL345
> > - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> > - * 0x53 (ALT ADDRESS pin grounded)
> 
> > + * IIO core driver for ADXL345
> 
> Should not it be at the beginning of header comment?
> 

Ack.

> > +static int adxl345_i2c_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const char *name = NULL;
> 
> Reverse tree order, please.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing i2c regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> > +   return PTR_ERR(regmap);
> > +   }
> > +
> 
> > +   if (id)
> > +   name = id->name;
> > +
> > +   return adxl345_common_probe(>dev, regmap, name);
> 
> Do you need temporary variable?
> 
> return adxl345_probe(>dev, regmap, id ? id->name : NULL);
> 

Will remove it and use the form you suggest.

Thanks,
Eva

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:51:28PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Move I2C-specific code into its own file and rely on regmap to access
> > registers. The core code provides access to x, y, z and scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_I2C
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> > Driver"
> 
> > depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> I'm wondering if it works in a form of
> 
> depends on INPUT_ADXL34X=n
> 

Yes it works. Will change it into this form.

> > +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> > +const char *name);
> > +int adxl345_common_remove(struct device *dev);
> 
> I think a "common" word is redundant.
> 

OK. I will use "core" instead.

> > - * IIO driver for ADXL345
> > - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> > - * 0x53 (ALT ADDRESS pin grounded)
> 
> > + * IIO core driver for ADXL345
> 
> Should not it be at the beginning of header comment?
> 

Ack.

> > +static int adxl345_i2c_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const char *name = NULL;
> 
> Reverse tree order, please.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing i2c regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> > +   return PTR_ERR(regmap);
> > +   }
> > +
> 
> > +   if (id)
> > +   name = id->name;
> > +
> > +   return adxl345_common_probe(>dev, regmap, name);
> 
> Do you need temporary variable?
> 
> return adxl345_probe(>dev, regmap, id ? id->name : NULL);
> 

Will remove it and use the form you suggest.

Thanks,
Eva

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [RFC] arm64: support HAVE_ARCH_RARE_WRITE

[Hoeun Ryu]

> On Mar 4, 2017, at 5:50 AM, Andy Lutomirski  wrote:
> 
>> On Thu, Mar 2, 2017 at 7:00 AM, Hoeun Ryu  wrote:
>> +unsigned long __rare_write_rw_alias_start = TASK_SIZE_64 / 4;
>> +
>> +__always_inline unsigned long __arch_rare_write_map(void)
>> +{
>> +   struct mm_struct *mm = _write_mm;
>> +
>> +   preempt_disable();
>> +
>> +   __switch_mm(mm);
> 
> ...
> 
>> +__always_inline unsigned long __arch_rare_write_unmap(void)
>> +{
>> +   struct mm_struct *mm = current->active_mm;
>> +
>> +   __switch_mm(mm);
>> +
> 
> This reminds me: this code imposes constraints on the context in which
> it's called.  I'd advise making it very explicit, asserting
> correctness, and putting the onus on the caller to set things up.  For
> example:
> 
> DEBUG_LOCKS_WARN_ON(preemptible() || in_interrupt() || in_nmi());
> 

OK. I will add some onus in the next version.

> in both the map and unmap functions, along with getting rid of the
> preempt_disable().  I don't think we want the preempt-disabledness to
> depend on the arch.  The generic non-arch rare_write helpers can do
> the preempt_disable().
> 

I think I can fix this in the next version when Kees send the next version of 
RFC.

> This code also won't work if the mm is wacky when called.  On x86, we could 
> do:
> 
> DEBUG_LOCKS_WARN_ON(read_cr3() != current->active_mm->pgd);
> 
> or similar (since that surely doesn't compile as is).
> 
> --Andy

Thank you for the review.

Re: [RFC] arm64: support HAVE_ARCH_RARE_WRITE

[Hoeun Ryu]

> On Mar 4, 2017, at 5:50 AM, Andy Lutomirski  wrote:
> 
>> On Thu, Mar 2, 2017 at 7:00 AM, Hoeun Ryu  wrote:
>> +unsigned long __rare_write_rw_alias_start = TASK_SIZE_64 / 4;
>> +
>> +__always_inline unsigned long __arch_rare_write_map(void)
>> +{
>> +   struct mm_struct *mm = _write_mm;
>> +
>> +   preempt_disable();
>> +
>> +   __switch_mm(mm);
> 
> ...
> 
>> +__always_inline unsigned long __arch_rare_write_unmap(void)
>> +{
>> +   struct mm_struct *mm = current->active_mm;
>> +
>> +   __switch_mm(mm);
>> +
> 
> This reminds me: this code imposes constraints on the context in which
> it's called.  I'd advise making it very explicit, asserting
> correctness, and putting the onus on the caller to set things up.  For
> example:
> 
> DEBUG_LOCKS_WARN_ON(preemptible() || in_interrupt() || in_nmi());
> 

OK. I will add some onus in the next version.

> in both the map and unmap functions, along with getting rid of the
> preempt_disable().  I don't think we want the preempt-disabledness to
> depend on the arch.  The generic non-arch rare_write helpers can do
> the preempt_disable().
> 

I think I can fix this in the next version when Kees send the next version of 
RFC.

> This code also won't work if the mm is wacky when called.  On x86, we could 
> do:
> 
> DEBUG_LOCKS_WARN_ON(read_cr3() != current->active_mm->pgd);
> 
> or similar (since that surely doesn't compile as is).
> 
> --Andy

Thank you for the review.

Re: [PATCH v5 4/4] iio: accel: adxl345: Add SPI support

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:55:26PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Add SPI driver that initializes SPI regmap for the adxl345 core driver.
> > The driver supports the same functionality as I2C namely the x, y, z and
> > scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_SPI
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer SPI 
> > Driver"
> 
> > +   depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> Same question. Would it be just
> 
> depends on INPUT_ADXL34X=n
> 
> ?
> 
> > +/* Setting bits 7 and 6 enables multiple-byte read */
> > +   .read_flag_mask = BIT(7) | BIT(6),
> 
> GENMASK(7, 6) ?
> 

True, but I would like to keep this as is. It is more readable and
obvious than GENMASK().

> > +static int adxl345_spi_probe(struct spi_device *spi)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const struct spi_device_id *id = spi_get_device_id(spi);
> 
> Reverse order.
> 
> And usually we do assignments from function parameters first.

Ack.

Thanks,
Eva
> 
> > +   dev_err(>dev, "Error initializing spi regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> Ugly casting!
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 4/4] iio: accel: adxl345: Add SPI support

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:55:26PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Add SPI driver that initializes SPI regmap for the adxl345 core driver.
> > The driver supports the same functionality as I2C namely the x, y, z and
> > scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_SPI
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer SPI 
> > Driver"
> 
> > +   depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> Same question. Would it be just
> 
> depends on INPUT_ADXL34X=n
> 
> ?
> 
> > +/* Setting bits 7 and 6 enables multiple-byte read */
> > +   .read_flag_mask = BIT(7) | BIT(6),
> 
> GENMASK(7, 6) ?
> 

True, but I would like to keep this as is. It is more readable and
obvious than GENMASK().

> > +static int adxl345_spi_probe(struct spi_device *spi)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const struct spi_device_id *id = spi_get_device_id(spi);
> 
> Reverse order.
> 
> And usually we do assignments from function parameters first.

Ack.

Thanks,
Eva
> 
> > +   dev_err(>dev, "Error initializing spi regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> Ugly casting!
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/4] net: thunderx: Fix IOMMU translation faults

[Sunil Kovvuri]

On Fri, Mar 3, 2017 at 11:26 PM, David Miller  wrote:
> From: sunil.kovv...@gmail.com
> Date: Fri,  3 Mar 2017 16:17:47 +0530
>
>> @@ -1643,6 +1650,9 @@ static int nicvf_probe(struct pci_dev *pdev, const 
>> struct pci_device_id *ent)
>>   if (!pass1_silicon(nic->pdev))
>>   nic->hw_tso = true;
>>
>> + /* Check if we are attached to IOMMU */
>> + nic->iommu_domain = iommu_get_domain_for_dev(dev);
>
> This function is not universally available.

Even if CONFIG_IOMMU_API is not enabled, it will return NULL and will be okay.
http://lxr.free-electrons.com/source/include/linux/iommu.h#L400

>
> This looks very hackish to me anyways, how all of this stuff is supposed
> to work is that you simply use the DMA interfaces unconditionally and
> whatever is behind the operations takes care of everything.
>
> Doing it conditionally in the driver with all of this special IOMMU
> domain et al. knowledge makes no sense to me at all.
>
> I don't see other drivers doing stuff like this at all, so if you're
> going to handle this in a unique way like this you better write
> several paragraphs in your commit message explaining why this weird
> crap is necessary.

I already tried to explain in the commit message that HW anyway takes care
of data coherency, so calling DMA interfaces when there is no IOMMU will
only result in performance drop.

We are seeing a 0.75Mpps drop with IP forwarding rate due to that.
Hence I have restricted calling DMA interfaces to only when IOMMU is enabled.

Re: [PATCH 1/4] net: thunderx: Fix IOMMU translation faults

[Sunil Kovvuri]

On Fri, Mar 3, 2017 at 11:26 PM, David Miller  wrote:
> From: sunil.kovv...@gmail.com
> Date: Fri,  3 Mar 2017 16:17:47 +0530
>
>> @@ -1643,6 +1650,9 @@ static int nicvf_probe(struct pci_dev *pdev, const 
>> struct pci_device_id *ent)
>>   if (!pass1_silicon(nic->pdev))
>>   nic->hw_tso = true;
>>
>> + /* Check if we are attached to IOMMU */
>> + nic->iommu_domain = iommu_get_domain_for_dev(dev);
>
> This function is not universally available.

Even if CONFIG_IOMMU_API is not enabled, it will return NULL and will be okay.
http://lxr.free-electrons.com/source/include/linux/iommu.h#L400

>
> This looks very hackish to me anyways, how all of this stuff is supposed
> to work is that you simply use the DMA interfaces unconditionally and
> whatever is behind the operations takes care of everything.
>
> Doing it conditionally in the driver with all of this special IOMMU
> domain et al. knowledge makes no sense to me at all.
>
> I don't see other drivers doing stuff like this at all, so if you're
> going to handle this in a unique way like this you better write
> several paragraphs in your commit message explaining why this weird
> crap is necessary.

I already tried to explain in the commit message that HW anyway takes care
of data coherency, so calling DMA interfaces when there is no IOMMU will
only result in performance drop.

We are seeing a 0.75Mpps drop with IP forwarding rate due to that.
Hence I have restricted calling DMA interfaces to only when IOMMU is enabled.

Re: [RFC] arm64: support HAVE_ARCH_RARE_WRITE

[Hoeun Ryu]

> On Mar 3, 2017, at 1:02 PM, Kees Cook  wrote:
> 
>> On Thu, Mar 2, 2017 at 7:00 AM, Hoeun Ryu  wrote:
>> This RFC is a quick and dirty arm64 implementation for Kees Cook's RFC for
>> rare_write infrastructure [1].
> 
> Awesome! :)
> 
>> This implementation is based on Mark Rutland's suggestions, which is that
>> a special userspace mm that maps only __start/end_rodata as RW permission
>> is prepared during early boot time (paging_init) and __arch_rare_write_map()
>> switches to the mm [2].
>> 
>> Due to the limit of implementation (the mm having RW mapping is userspace
>> mm), we need a new arch-specific __arch_rare_write_ptr() to convert RO
>> address to RW address (CONFIG_HAVE_RARE_WRITE_PTR is added), which is
>> general for all architectures (__rare_write_ptr()) in Kees's RFC . So all
>> writes should be instrumented by __rare_write().
> 
> Cool, yeah, I'll get all this fixed up in my next version.

I'll send the next version of this when you send yours.

>> One caveat for arm64 is CONFIG_ARM64_SW_TTBR0_PAN.
>> Because __arch_rare_write_map() installes a special user mm to ttbr0,
>> usercopy inside  __arch_rare_write_map/unmap() pair will break rare_write.
>> (uaccess_enable() replaces the special mm and RW alias is no longer valid.)
> 
> That's totally fine constraint: this case should never happen for so
> many reasons. :)
> 

OK. Thank you for the review.

>> A similar problem could rise in general usercopy inside
>> __arch_rare_write_map/unmap(). __arch_rare_write_map() replaces current->mm,
>> so we loose the address space of the `current` process.
>> 
>> It passes LKDTM's rare write test.
>> 
>> [1] : http://www.openwall.com/lists/kernel-hardening/2017/02/27/5
>> [2] : https://lkml.org/lkml/2017/2/22/254
>> 
>> Signed-off-by: Hoeun Ryu 
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Re: [RFC] arm64: support HAVE_ARCH_RARE_WRITE

[Hoeun Ryu]

> On Mar 3, 2017, at 1:02 PM, Kees Cook  wrote:
> 
>> On Thu, Mar 2, 2017 at 7:00 AM, Hoeun Ryu  wrote:
>> This RFC is a quick and dirty arm64 implementation for Kees Cook's RFC for
>> rare_write infrastructure [1].
> 
> Awesome! :)
> 
>> This implementation is based on Mark Rutland's suggestions, which is that
>> a special userspace mm that maps only __start/end_rodata as RW permission
>> is prepared during early boot time (paging_init) and __arch_rare_write_map()
>> switches to the mm [2].
>> 
>> Due to the limit of implementation (the mm having RW mapping is userspace
>> mm), we need a new arch-specific __arch_rare_write_ptr() to convert RO
>> address to RW address (CONFIG_HAVE_RARE_WRITE_PTR is added), which is
>> general for all architectures (__rare_write_ptr()) in Kees's RFC . So all
>> writes should be instrumented by __rare_write().
> 
> Cool, yeah, I'll get all this fixed up in my next version.

I'll send the next version of this when you send yours.

>> One caveat for arm64 is CONFIG_ARM64_SW_TTBR0_PAN.
>> Because __arch_rare_write_map() installes a special user mm to ttbr0,
>> usercopy inside  __arch_rare_write_map/unmap() pair will break rare_write.
>> (uaccess_enable() replaces the special mm and RW alias is no longer valid.)
> 
> That's totally fine constraint: this case should never happen for so
> many reasons. :)
> 

OK. Thank you for the review.

>> A similar problem could rise in general usercopy inside
>> __arch_rare_write_map/unmap(). __arch_rare_write_map() replaces current->mm,
>> so we loose the address space of the `current` process.
>> 
>> It passes LKDTM's rare write test.
>> 
>> [1] : http://www.openwall.com/lists/kernel-hardening/2017/02/27/5
>> [2] : https://lkml.org/lkml/2017/2/22/254
>> 
>> Signed-off-by: Hoeun Ryu 
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:44:27PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Convert the driver to use regmap instead of I2C-specific functions. This
> > is done in preparation for splitting this driver into core and
> > I2C-specific code as well as introduction of SPI driver.
> >
> > Signed-off-by: Eva Rachel Retuya 
> > Reviewed-by: Andy Shevchenko 
> 
> Okay, my another set of comments.
> > @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  {
> > struct adxl345_data *data = iio_priv(indio_dev);
> 
> > int ret;
> > +   __le16 regval;
> 
> Reverse tree order, please.
> 
>  __le16 regval;
>  int ret;
> 

Ack.

> > @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
> >  {
> > struct adxl345_data *data;
> > struct iio_dev *indio_dev;
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > int ret;
> > +   u32 regval;
> 
> Ditto.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> I don't like explicit casting in such cases at all. Why not to use
> %ld? Same for the similar places in the series.
> 

OK. Will do that instead.

> > -   if (ret != ADXL345_DEVID) {
> > -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> > +   if (regval != ADXL345_DEVID) {
> > +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> > +   regval, ADXL345_DEVID);
> 
> So, originally it was in decimal form. Does hex looks better or what
> is behind the change?

Yes it looks better. I made the change to make it easily seen what it
read in hex compared to the known DEVID.

Thanks,
Eva

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:44:27PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Convert the driver to use regmap instead of I2C-specific functions. This
> > is done in preparation for splitting this driver into core and
> > I2C-specific code as well as introduction of SPI driver.
> >
> > Signed-off-by: Eva Rachel Retuya 
> > Reviewed-by: Andy Shevchenko 
> 
> Okay, my another set of comments.
> > @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  {
> > struct adxl345_data *data = iio_priv(indio_dev);
> 
> > int ret;
> > +   __le16 regval;
> 
> Reverse tree order, please.
> 
>  __le16 regval;
>  int ret;
> 

Ack.

> > @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
> >  {
> > struct adxl345_data *data;
> > struct iio_dev *indio_dev;
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > int ret;
> > +   u32 regval;
> 
> Ditto.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> I don't like explicit casting in such cases at all. Why not to use
> %ld? Same for the similar places in the series.
> 

OK. Will do that instead.

> > -   if (ret != ADXL345_DEVID) {
> > -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> > +   if (regval != ADXL345_DEVID) {
> > +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> > +   regval, ADXL345_DEVID);
> 
> So, originally it was in decimal form. Does hex looks better or what
> is behind the change?

Yes it looks better. I made the change to make it easily seen what it
read in hex compared to the known DEVID.

Thanks,
Eva

> 
> -- 
> With Best Regards,
> Andy Shevchenko

[PATCH] KVM: nVMX: Reset nested_run_pending if the CPU is going to be reset

[Wanpeng Li]

Reported by syzkaller:

WARNING: CPU: 1 PID: 27742 at arch/x86/kvm/vmx.c:11029
nested_vmx_vmexit+0x5c35/0x74d0 arch/x86/kvm/vmx.c:11029
CPU: 1 PID: 27742 Comm: a.out Not tainted 4.10.0+ #229
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:15 [inline]
 dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
 panic+0x1fb/0x412 kernel/panic.c:179
 __warn+0x1c4/0x1e0 kernel/panic.c:540
 warn_slowpath_null+0x2c/0x40 kernel/panic.c:583
 nested_vmx_vmexit+0x5c35/0x74d0 arch/x86/kvm/vmx.c:11029
 vmx_leave_nested arch/x86/kvm/vmx.c:11136 [inline]
 vmx_set_msr+0x1565/0x1910 arch/x86/kvm/vmx.c:3324
 kvm_set_msr+0xd4/0x170 arch/x86/kvm/x86.c:1099
 do_set_msr+0x11e/0x190 arch/x86/kvm/x86.c:1128
 __msr_io arch/x86/kvm/x86.c:2577 [inline]
 msr_io+0x24b/0x450 arch/x86/kvm/x86.c:2614
 kvm_arch_vcpu_ioctl+0x35b/0x46a0 arch/x86/kvm/x86.c:3497
 kvm_vcpu_ioctl+0x232/0x1120 
arch/x86/kvm/../../../virt/kvm/kvm_main.c:2721
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
 SYSC_ioctl fs/ioctl.c:698 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
 entry_SYSCALL_64_fastpath+0x1f/0xc2

The syzkaller folks reported a nested_run_pending warning during userspace 
clear VMX 
capability which is exposed to L1 before. 

The warning gets thrown while doing

(*(uint32_t*)0x20aecfe8 = (uint32_t)0x1);
(*(uint32_t*)0x20aecfec = (uint32_t)0x0);
(*(uint32_t*)0x20aecff0 = (uint32_t)0x3a);
(*(uint32_t*)0x20aecff4 = (uint32_t)0x0);
(*(uint64_t*)0x20aecff8 = (uint64_t)0x0);
r[29] = syscall(__NR_ioctl, r[4], 0x4008ae89ul,
0x20aecfe8ul, 0, 0, 0, 0, 0, 0);

i.e. KVM_SET_MSR ioctl with

struct kvm_msrs {
.nmsrs = 1,
.pad = 0,
.entries = {
{.index = MSR_IA32_FEATURE_CONTROL,
.reserved = 0,
.data = 0}
}
}

The VMLANCH/VMRESUME emulation should be stopped since the CPU is going to 
reset here. 
This patch resets the nested_run_pending since the CPU is going to be reset 
hence there 
should be nothing pending.

Reported-by: Dmitry Vyukov 
Suggested-by: Radim Krčmář 
Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Dmitry Vyukov 
Signed-off-by: Wanpeng Li 
---
 arch/x86/kvm/vmx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 283aa86..b4a757369 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3310,8 +3310,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
return 1;
vmx->msr_ia32_feature_control = data;
-   if (msr_info->host_initiated && data == 0)
+   if (msr_info->host_initiated && data == 0) {
+   vmx->nested.nested_run_pending = 0;
vmx_leave_nested(vcpu);
+   }
break;
case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
if (!msr_info->host_initiated)
-- 
2.7.4

[PATCH] KVM: nVMX: Reset nested_run_pending if the CPU is going to be reset

[Wanpeng Li]

Reported by syzkaller:

WARNING: CPU: 1 PID: 27742 at arch/x86/kvm/vmx.c:11029
nested_vmx_vmexit+0x5c35/0x74d0 arch/x86/kvm/vmx.c:11029
CPU: 1 PID: 27742 Comm: a.out Not tainted 4.10.0+ #229
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:15 [inline]
 dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
 panic+0x1fb/0x412 kernel/panic.c:179
 __warn+0x1c4/0x1e0 kernel/panic.c:540
 warn_slowpath_null+0x2c/0x40 kernel/panic.c:583
 nested_vmx_vmexit+0x5c35/0x74d0 arch/x86/kvm/vmx.c:11029
 vmx_leave_nested arch/x86/kvm/vmx.c:11136 [inline]
 vmx_set_msr+0x1565/0x1910 arch/x86/kvm/vmx.c:3324
 kvm_set_msr+0xd4/0x170 arch/x86/kvm/x86.c:1099
 do_set_msr+0x11e/0x190 arch/x86/kvm/x86.c:1128
 __msr_io arch/x86/kvm/x86.c:2577 [inline]
 msr_io+0x24b/0x450 arch/x86/kvm/x86.c:2614
 kvm_arch_vcpu_ioctl+0x35b/0x46a0 arch/x86/kvm/x86.c:3497
 kvm_vcpu_ioctl+0x232/0x1120 
arch/x86/kvm/../../../virt/kvm/kvm_main.c:2721
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
 SYSC_ioctl fs/ioctl.c:698 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
 entry_SYSCALL_64_fastpath+0x1f/0xc2

The syzkaller folks reported a nested_run_pending warning during userspace 
clear VMX 
capability which is exposed to L1 before. 

The warning gets thrown while doing

(*(uint32_t*)0x20aecfe8 = (uint32_t)0x1);
(*(uint32_t*)0x20aecfec = (uint32_t)0x0);
(*(uint32_t*)0x20aecff0 = (uint32_t)0x3a);
(*(uint32_t*)0x20aecff4 = (uint32_t)0x0);
(*(uint64_t*)0x20aecff8 = (uint64_t)0x0);
r[29] = syscall(__NR_ioctl, r[4], 0x4008ae89ul,
0x20aecfe8ul, 0, 0, 0, 0, 0, 0);

i.e. KVM_SET_MSR ioctl with

struct kvm_msrs {
.nmsrs = 1,
.pad = 0,
.entries = {
{.index = MSR_IA32_FEATURE_CONTROL,
.reserved = 0,
.data = 0}
}
}

The VMLANCH/VMRESUME emulation should be stopped since the CPU is going to 
reset here. 
This patch resets the nested_run_pending since the CPU is going to be reset 
hence there 
should be nothing pending.

Reported-by: Dmitry Vyukov 
Suggested-by: Radim Krčmář 
Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Dmitry Vyukov 
Signed-off-by: Wanpeng Li 
---
 arch/x86/kvm/vmx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 283aa86..b4a757369 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3310,8 +3310,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
return 1;
vmx->msr_ia32_feature_control = data;
-   if (msr_info->host_initiated && data == 0)
+   if (msr_info->host_initiated && data == 0) {
+   vmx->nested.nested_run_pending = 0;
vmx_leave_nested(vcpu);
+   }
break;
case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
if (!msr_info->host_initiated)
-- 
2.7.4

Re: [PATCH 1/2] xfs: allow kmem_zalloc_greedy to fail

[Dave Chinner]

On Fri, Mar 03, 2017 at 03:19:12PM -0800, Darrick J. Wong wrote:
> On Sat, Mar 04, 2017 at 09:54:44AM +1100, Dave Chinner wrote:
> > On Thu, Mar 02, 2017 at 04:45:40PM +0100, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > Even though kmem_zalloc_greedy is documented it might fail the current
> > > code doesn't really implement this properly and loops on the smallest
> > > allowed size for ever. This is a problem because vzalloc might fail
> > > permanently - we might run out of vmalloc space or since 5d17a73a2ebe
> > > ("vmalloc: back off when the current task is killed") when the current
> > > task is killed. The later one makes the failure scenario much more
> > > probable than it used to be because it makes vmalloc() failures
> > > permanent for tasks with fatal signals pending.. Fix this by bailing out
> > > if the minimum size request failed.
> > > 
> > > This has been noticed by a hung generic/269 xfstest by Xiong Zhou.
> > > 
> > > fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, 
> > > mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null)
> > > fsstress cpuset=/ mems_allowed=0-1
> > > CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
> > > Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 
> > > 10/05/2016
> > > Call Trace:
> > >  dump_stack+0x63/0x87
> > >  warn_alloc+0x114/0x1c0
> > >  ? alloc_pages_current+0x88/0x120
> > >  __vmalloc_node_range+0x250/0x2a0
> > >  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  ? free_hot_cold_page+0x21f/0x280
> > >  vzalloc+0x54/0x60
> > >  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  xfs_bulkstat+0x11b/0x730 [xfs]
> > >  ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
> > >  ? selinux_capable+0x20/0x30
> > >  ? security_capable+0x48/0x60
> > >  xfs_ioc_bulkstat+0xe4/0x190 [xfs]
> > >  xfs_file_ioctl+0x9dd/0xad0 [xfs]
> > >  ? do_filp_open+0xa5/0x100
> > >  do_vfs_ioctl+0xa7/0x5e0
> > >  SyS_ioctl+0x79/0x90
> > >  do_syscall_64+0x67/0x180
> > >  entry_SYSCALL64_slow_path+0x25/0x25
> > > 
> > > fsstress keeps looping inside kmem_zalloc_greedy without any way out
> > > because vmalloc keeps failing due to fatal_signal_pending.
> > > 
> > > Reported-by: Xiong Zhou 
> > > Analyzed-by: Tetsuo Handa 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  fs/xfs/kmem.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c
> > > index 339c696bbc01..ee95f5c6db45 100644
> > > --- a/fs/xfs/kmem.c
> > > +++ b/fs/xfs/kmem.c
> > > @@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t 
> > > maxsize)
> > >   size_t  kmsize = maxsize;
> > >  
> > >   while (!(ptr = vzalloc(kmsize))) {
> > > + if (kmsize == minsize)
> > > + break;
> > >   if ((kmsize >>= 1) <= minsize)
> > >   kmsize = minsize;
> > >   }
> > 
> > Seems wrong to me - this function used to have lots of callers and
> > over time we've slowly removed them or replaced them with something
> > else. I'd suggest removing it completely, replacing the call sites
> > with kmem_zalloc_large().
> 
> Heh.  I thought the reason why _greedy still exists (for its sole user
> bulkstat) is that bulkstat had the flexibility to deal with receiving
> 0, 1, or 4 pages.  So yeah, we could just kill it.

irbuf is sized to minimise AGI locking, but if memory is low
it just uses what it can get. Keep in mind the number of inodes we
need to process is determined by the userspace buffer size, which
can easily be sized to hold tens of thousands of struct
xfs_bulkstat.

> But thinking even more stingily about memory, are there applications
> that care about being able to bulkstat 16384 inodes at once?

IIRC, xfsdump can bulkstat up to 64k inodes per call

> How badly
> does bulkstat need to be able to bulk-process more than a page's worth
> of inobt records, anyway?

Benchmark it on a busy system doing lots of other AGI work (e.g. a
busy NFS server workload with a working set of tens of millions of
inodes so it doesn't fit in cache) and find out. That's generally
how I answer those sorts of questions...

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH 1/2] xfs: allow kmem_zalloc_greedy to fail

[Dave Chinner]

On Fri, Mar 03, 2017 at 03:19:12PM -0800, Darrick J. Wong wrote:
> On Sat, Mar 04, 2017 at 09:54:44AM +1100, Dave Chinner wrote:
> > On Thu, Mar 02, 2017 at 04:45:40PM +0100, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > Even though kmem_zalloc_greedy is documented it might fail the current
> > > code doesn't really implement this properly and loops on the smallest
> > > allowed size for ever. This is a problem because vzalloc might fail
> > > permanently - we might run out of vmalloc space or since 5d17a73a2ebe
> > > ("vmalloc: back off when the current task is killed") when the current
> > > task is killed. The later one makes the failure scenario much more
> > > probable than it used to be because it makes vmalloc() failures
> > > permanent for tasks with fatal signals pending.. Fix this by bailing out
> > > if the minimum size request failed.
> > > 
> > > This has been noticed by a hung generic/269 xfstest by Xiong Zhou.
> > > 
> > > fsstress: vmalloc: allocation failure, allocated 12288 of 20480 bytes, 
> > > mode:0x14080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO), nodemask=(null)
> > > fsstress cpuset=/ mems_allowed=0-1
> > > CPU: 1 PID: 23460 Comm: fsstress Not tainted 4.10.0-master-45554b2+ #21
> > > Hardware name: HP ProLiant DL380 Gen9/ProLiant DL380 Gen9, BIOS P89 
> > > 10/05/2016
> > > Call Trace:
> > >  dump_stack+0x63/0x87
> > >  warn_alloc+0x114/0x1c0
> > >  ? alloc_pages_current+0x88/0x120
> > >  __vmalloc_node_range+0x250/0x2a0
> > >  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  ? free_hot_cold_page+0x21f/0x280
> > >  vzalloc+0x54/0x60
> > >  ? kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  kmem_zalloc_greedy+0x2b/0x40 [xfs]
> > >  xfs_bulkstat+0x11b/0x730 [xfs]
> > >  ? xfs_bulkstat_one_int+0x340/0x340 [xfs]
> > >  ? selinux_capable+0x20/0x30
> > >  ? security_capable+0x48/0x60
> > >  xfs_ioc_bulkstat+0xe4/0x190 [xfs]
> > >  xfs_file_ioctl+0x9dd/0xad0 [xfs]
> > >  ? do_filp_open+0xa5/0x100
> > >  do_vfs_ioctl+0xa7/0x5e0
> > >  SyS_ioctl+0x79/0x90
> > >  do_syscall_64+0x67/0x180
> > >  entry_SYSCALL64_slow_path+0x25/0x25
> > > 
> > > fsstress keeps looping inside kmem_zalloc_greedy without any way out
> > > because vmalloc keeps failing due to fatal_signal_pending.
> > > 
> > > Reported-by: Xiong Zhou 
> > > Analyzed-by: Tetsuo Handa 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  fs/xfs/kmem.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c
> > > index 339c696bbc01..ee95f5c6db45 100644
> > > --- a/fs/xfs/kmem.c
> > > +++ b/fs/xfs/kmem.c
> > > @@ -34,6 +34,8 @@ kmem_zalloc_greedy(size_t *size, size_t minsize, size_t 
> > > maxsize)
> > >   size_t  kmsize = maxsize;
> > >  
> > >   while (!(ptr = vzalloc(kmsize))) {
> > > + if (kmsize == minsize)
> > > + break;
> > >   if ((kmsize >>= 1) <= minsize)
> > >   kmsize = minsize;
> > >   }
> > 
> > Seems wrong to me - this function used to have lots of callers and
> > over time we've slowly removed them or replaced them with something
> > else. I'd suggest removing it completely, replacing the call sites
> > with kmem_zalloc_large().
> 
> Heh.  I thought the reason why _greedy still exists (for its sole user
> bulkstat) is that bulkstat had the flexibility to deal with receiving
> 0, 1, or 4 pages.  So yeah, we could just kill it.

irbuf is sized to minimise AGI locking, but if memory is low
it just uses what it can get. Keep in mind the number of inodes we
need to process is determined by the userspace buffer size, which
can easily be sized to hold tens of thousands of struct
xfs_bulkstat.

> But thinking even more stingily about memory, are there applications
> that care about being able to bulkstat 16384 inodes at once?

IIRC, xfsdump can bulkstat up to 64k inodes per call

> How badly
> does bulkstat need to be able to bulk-process more than a page's worth
> of inobt records, anyway?

Benchmark it on a busy system doing lots of other AGI work (e.g. a
busy NFS server workload with a working set of tens of millions of
inodes so it doesn't fit in cache) and find out. That's generally
how I answer those sorts of questions...

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu  wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu  wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao"  wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
>  T user_read
>  t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..

Ah, I see.

static int create_trace_kprobe(int argc, char **argv)
...
} else {
/* a symbol specified */
symbol = argv[1];
/* TODO: support .init module functions */
ret = traceprobe_split_symbol_offset(symbol, );
if (ret) {
pr_info("Failed to parse symbol.\n");
return ret;
}
if (offset && is_return &&
!arch_function_offset_within_entry(offset)) {
pr_info("Given offset is not valid for return 
probe.\n");
return -EINVAL;
}
}

So, actually, traceprobe_split_symbol_offset() just split out symbol
and offset from symbol string (e.g. "_text+3539616").
So, you should use kallsyms_lookup_size_offset() here again to check
offset.

Please try attached patch (I've already tested on x86-64).

$ sudo ./perf probe -a user_read%return
Added new events:
  probe:user_read  (on user_read%return)
  probe:user_read_1(on user_read%return)

You can now use it in all perf tools, such as:

perf record -e probe:user_read_1 -aR sleep 1

$ sudo ./perf probe -l
  probe:user_read  (on user_read%return@security/keys/user_defined.c)
  probe:user_read_1(on user_read%return@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
9637bf70  r  user_read+0x0[DISABLED][FTRACE]
963602f0  r  user_read+0x0[DISABLED][FTRACE]

Thank you,


-- 
Masami Hiramatsu 


tracing-kprobe-check-kretprobe
Description: Binary data

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu  wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu  wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao"  wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
>  T user_read
>  t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..

Ah, I see.

static int create_trace_kprobe(int argc, char **argv)
...
} else {
/* a symbol specified */
symbol = argv[1];
/* TODO: support .init module functions */
ret = traceprobe_split_symbol_offset(symbol, );
if (ret) {
pr_info("Failed to parse symbol.\n");
return ret;
}
if (offset && is_return &&
!arch_function_offset_within_entry(offset)) {
pr_info("Given offset is not valid for return 
probe.\n");
return -EINVAL;
}
}

So, actually, traceprobe_split_symbol_offset() just split out symbol
and offset from symbol string (e.g. "_text+3539616").
So, you should use kallsyms_lookup_size_offset() here again to check
offset.

Please try attached patch (I've already tested on x86-64).

$ sudo ./perf probe -a user_read%return
Added new events:
  probe:user_read  (on user_read%return)
  probe:user_read_1(on user_read%return)

You can now use it in all perf tools, such as:

perf record -e probe:user_read_1 -aR sleep 1

$ sudo ./perf probe -l
  probe:user_read  (on user_read%return@security/keys/user_defined.c)
  probe:user_read_1(on user_read%return@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
9637bf70  r  user_read+0x0[DISABLED][FTRACE]
963602f0  r  user_read+0x0[DISABLED][FTRACE]

Thank you,


-- 
Masami Hiramatsu 


tracing-kprobe-check-kretprobe
Description: Binary data

Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

[Masahiro Yamada]

Hi Piotr,


2017-03-03 17:31 GMT+09:00 Piotr Sroka :
> Add support for HS400ES mode to Cadence SDHCI driver.
>
> Signed-off-by: Piotr Sroka 


Thanks for your patch.  It looks almost good to me.
My comments are about some coding style issues only.
Please see below.




> +static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
> +{
> +   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
> +   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
> +}
> +

Why doesn't this function simply return u32 value?





>
> +static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
> +struct mmc_ios *ios)
> +{
> +   struct sdhci_host *host = mmc_priv(mmc);
> +   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +   u32 timingMode;

Could you avoid CamelCase, please?
(perhaps does checkpatch.pl complain about this?).

I guess "mode" could be enough.



> +   priv->enhanced_strobe = ios->enhanced_strobe;
> +
> +   sdhci_cdns_get_emmc_mode(priv, );
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
> +   ios->enhanced_strobe)
> +   sdhci_cdns_set_emmc_mode(priv,
> +SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
> +   !ios->enhanced_strobe)

You could stretch this if-conditional within 80-col
by renaming "timingMode" into "mode".





> +   host->mmc_host_ops.hs400_enhanced_strobe =
> +   sdhci_cdns_hs400_enhanced_strobe;

I should not say this if this is the only matter,
but can you move the 2nd line to the right by inserting some more tabs?



FYI:
If you have not checked the coding-style guideline yet, and you are interested,
it is a good idea to read Documentation/process/coding-style.rst


Some of my comments are based on the following statements in that document:

  - "LOCAL variable names should be short, and to the point."
  - "Descendants are always substantially shorter than the parent and
 are placed substantially to the right."

We need not too be nervous about the style,
but generally speaking, it will be nice to keep the style consistent
unless there is a specific reason.

Thanks!

-- 
Best Regards
Masahiro Yamada

Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

[Masahiro Yamada]

Hi Piotr,


2017-03-03 17:31 GMT+09:00 Piotr Sroka :
> Add support for HS400ES mode to Cadence SDHCI driver.
>
> Signed-off-by: Piotr Sroka 


Thanks for your patch.  It looks almost good to me.
My comments are about some coding style issues only.
Please see below.




> +static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
> +{
> +   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
> +   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
> +}
> +

Why doesn't this function simply return u32 value?





>
> +static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
> +struct mmc_ios *ios)
> +{
> +   struct sdhci_host *host = mmc_priv(mmc);
> +   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +   u32 timingMode;

Could you avoid CamelCase, please?
(perhaps does checkpatch.pl complain about this?).

I guess "mode" could be enough.



> +   priv->enhanced_strobe = ios->enhanced_strobe;
> +
> +   sdhci_cdns_get_emmc_mode(priv, );
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
> +   ios->enhanced_strobe)
> +   sdhci_cdns_set_emmc_mode(priv,
> +SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
> +   !ios->enhanced_strobe)

You could stretch this if-conditional within 80-col
by renaming "timingMode" into "mode".





> +   host->mmc_host_ops.hs400_enhanced_strobe =
> +   sdhci_cdns_hs400_enhanced_strobe;

I should not say this if this is the only matter,
but can you move the 2nd line to the right by inserting some more tabs?



FYI:
If you have not checked the coding-style guideline yet, and you are interested,
it is a good idea to read Documentation/process/coding-style.rst


Some of my comments are based on the following statements in that document:

  - "LOCAL variable names should be short, and to the point."
  - "Descendants are always substantially shorter than the parent and
 are placed substantially to the right."

We need not too be nervous about the style,
but generally speaking, it will be nice to keep the style consistent
unless there is a specific reason.

Thanks!

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v10 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform

[ayaka]

What is wrong with this patch? I have not seen it is merged in 
next-20170303.



On 12/11/2016 11:36 PM, Randy Li wrote:

On the rk3288 USB host-only port (the one that's not the OTG-enabled
port) the PHY can get into a bad state when a wakeup is asserted (not
just a wakeup from full system suspend but also a wakeup from
autosuspend).

We can get the PHY out of its bad state by asserting its "port reset",
but unfortunately that seems to assert a reset onto the USB bus so it
could confuse things if we don't actually deenumerate / reenumerate the
device.

We can also get the PHY out of its bad state by fully resetting it using
the reset from the CRU (clock reset unit) in chip, which does a more full
reset.  The CRU-based reset appears to actually cause devices on the bus
to be removed and reinserted, which fixes the problem (albeit in a hacky
way).

It's unfortunate that we need to do a full re-enumeration of devices at
wakeup time, but this is better than alternative of letting the bus get
wedged.

Signed-off-by: Randy Li <ay...@soulik.info>
Acked-by: John Youn <johny...@synopsys.com>
---
  drivers/usb/dwc2/core.h  |  1 +
  drivers/usb/dwc2/core_intr.c | 11 +++
  drivers/usb/dwc2/platform.c  |  9 +
  3 files changed, 21 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9548d3e..0cd5896 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -919,6 +919,7 @@ struct dwc2_hsotg {
unsigned int ll_hw_enabled:1;
  
  	struct phy *phy;

+   struct work_struct phy_rst_work;
struct usb_phy *uphy;
struct dwc2_hsotg_plat *plat;
struct regulator_bulk_data 
supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 5b228ba..bf1c029 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg 
*hsotg)
  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
  {
int ret;
+   struct device_node *np = hsotg->dev->of_node;
  
  	/* Clear interrupt */

dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
@@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct 
dwc2_hsotg *hsotg)
/* Restart the Phy Clock */
pcgcctl &= ~PCGCTL_STOPPCLK;
dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
+
+   /*
+* It is a quirk in Rockchip RK3288, causing by
+* a hardware bug. This will propagate out and
+* eventually we'll re-enumerate the device.
+* Not great but the best we can do.
+*/
+   if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
+   schedule_work(>phy_rst_work);
+
mod_timer(>wkp_timer,
  jiffies + msecs_to_jiffies(71));
} else {
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 4fc8c60..8ef278e 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -207,6 +207,14 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
return ret;
  }
  
+/* Only used to reset usb phy at interrupter runtime */

+static void dwc2_reset_phy_work(struct work_struct *data)
+{
+   struct dwc2_hsotg *hsotg = container_of(data, struct dwc2_hsotg,
+   phy_rst_work);
+   phy_reset(hsotg->phy);
+}
+
  static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
  {
int i, ret;
@@ -251,6 +259,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
return ret;
}
}
+   INIT_WORK(>phy_rst_work, dwc2_reset_phy_work);
  
  	if (!hsotg->phy) {

hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);

Re: [PATCH v10 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform

[ayaka]

What is wrong with this patch? I have not seen it is merged in 
next-20170303.



On 12/11/2016 11:36 PM, Randy Li wrote:

On the rk3288 USB host-only port (the one that's not the OTG-enabled
port) the PHY can get into a bad state when a wakeup is asserted (not
just a wakeup from full system suspend but also a wakeup from
autosuspend).

We can get the PHY out of its bad state by asserting its "port reset",
but unfortunately that seems to assert a reset onto the USB bus so it
could confuse things if we don't actually deenumerate / reenumerate the
device.

We can also get the PHY out of its bad state by fully resetting it using
the reset from the CRU (clock reset unit) in chip, which does a more full
reset.  The CRU-based reset appears to actually cause devices on the bus
to be removed and reinserted, which fixes the problem (albeit in a hacky
way).

It's unfortunate that we need to do a full re-enumeration of devices at
wakeup time, but this is better than alternative of letting the bus get
wedged.

Signed-off-by: Randy Li 
Acked-by: John Youn 
---
  drivers/usb/dwc2/core.h  |  1 +
  drivers/usb/dwc2/core_intr.c | 11 +++
  drivers/usb/dwc2/platform.c  |  9 +
  3 files changed, 21 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9548d3e..0cd5896 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -919,6 +919,7 @@ struct dwc2_hsotg {
unsigned int ll_hw_enabled:1;
  
  	struct phy *phy;

+   struct work_struct phy_rst_work;
struct usb_phy *uphy;
struct dwc2_hsotg_plat *plat;
struct regulator_bulk_data 
supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 5b228ba..bf1c029 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg 
*hsotg)
  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
  {
int ret;
+   struct device_node *np = hsotg->dev->of_node;
  
  	/* Clear interrupt */

dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
@@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct 
dwc2_hsotg *hsotg)
/* Restart the Phy Clock */
pcgcctl &= ~PCGCTL_STOPPCLK;
dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
+
+   /*
+* It is a quirk in Rockchip RK3288, causing by
+* a hardware bug. This will propagate out and
+* eventually we'll re-enumerate the device.
+* Not great but the best we can do.
+*/
+   if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
+   schedule_work(>phy_rst_work);
+
mod_timer(>wkp_timer,
  jiffies + msecs_to_jiffies(71));
} else {
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 4fc8c60..8ef278e 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -207,6 +207,14 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
return ret;
  }
  
+/* Only used to reset usb phy at interrupter runtime */

+static void dwc2_reset_phy_work(struct work_struct *data)
+{
+   struct dwc2_hsotg *hsotg = container_of(data, struct dwc2_hsotg,
+   phy_rst_work);
+   phy_reset(hsotg->phy);
+}
+
  static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
  {
int i, ret;
@@ -251,6 +259,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
return ret;
}
}
+   INIT_WORK(>phy_rst_work, dwc2_reset_phy_work);
  
  	if (!hsotg->phy) {

hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);

[git pull] vfs.git final bits and pieces

[Al Viro]

A few unrelated patches that got beating in -next.  Everything else
will have to go into the next window ;-/

The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77:

  Linux 4.10-rc1 (2016-12-25 16:13:08 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.misc

for you to fetch changes up to eec11535ca3d3e2daa2c8f59fa8ce1963db98abd:

  hfs: fix hfs_readdir() (2017-02-18 22:11:15 -0500)


Al Viro (2):
  9p: constify ->d_name handling
  selftest for default_file_splice_read() infoleak

Dan Carpenter (1):
  hfs: fix hfs_readdir()

 fs/9p/fid.c  | 10 +-
 fs/9p/vfs_inode.c| 10 +-
 fs/9p/vfs_inode_dotl.c   | 20 ++--
 fs/hfs/dir.c |  2 +-
 include/net/9p/9p.h  |  8 
 include/net/9p/client.h  | 18 +-
 net/9p/client.c  | 18 +-
 tools/testing/selftests/Makefile |  1 +
 tools/testing/selftests/splice/Makefile  |  8 
 .../selftests/splice/default_file_splice_read.c  |  8 
 .../selftests/splice/default_file_splice_read.sh |  7 +++
 11 files changed, 67 insertions(+), 43 deletions(-)
 create mode 100644 tools/testing/selftests/splice/Makefile
 create mode 100644 tools/testing/selftests/splice/default_file_splice_read.c
 create mode 100755 tools/testing/selftests/splice/default_file_splice_read.sh

[git pull] vfs.git final bits and pieces

[Al Viro]

A few unrelated patches that got beating in -next.  Everything else
will have to go into the next window ;-/

The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77:

  Linux 4.10-rc1 (2016-12-25 16:13:08 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.misc

for you to fetch changes up to eec11535ca3d3e2daa2c8f59fa8ce1963db98abd:

  hfs: fix hfs_readdir() (2017-02-18 22:11:15 -0500)


Al Viro (2):
  9p: constify ->d_name handling
  selftest for default_file_splice_read() infoleak

Dan Carpenter (1):
  hfs: fix hfs_readdir()

 fs/9p/fid.c  | 10 +-
 fs/9p/vfs_inode.c| 10 +-
 fs/9p/vfs_inode_dotl.c   | 20 ++--
 fs/hfs/dir.c |  2 +-
 include/net/9p/9p.h  |  8 
 include/net/9p/client.h  | 18 +-
 net/9p/client.c  | 18 +-
 tools/testing/selftests/Makefile |  1 +
 tools/testing/selftests/splice/Makefile  |  8 
 .../selftests/splice/default_file_splice_read.c  |  8 
 .../selftests/splice/default_file_splice_read.sh |  7 +++
 11 files changed, 67 insertions(+), 43 deletions(-)
 create mode 100644 tools/testing/selftests/splice/Makefile
 create mode 100644 tools/testing/selftests/splice/default_file_splice_read.c
 create mode 100755 tools/testing/selftests/splice/default_file_splice_read.sh

[PATCH] mm: do not call mem_cgroup_free() from within mem_cgroup_alloc()

[Tahsin Erdogan]

mem_cgroup_free() indirectly calls wb_domain_exit() which is not
prepared to deal with a struct wb_domain object that hasn't executed
wb_domain_init(). For instance, the following warning message is
printed by lockdep if alloc_percpu() fails in mem_cgroup_alloc():

  INFO: trying to register non-static key.
  the code is fine but needs lockdep annotation.
  turning off the locking correctness validator.
  CPU: 1 PID: 1950 Comm: mkdir Not tainted 4.10.0+ #151
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Call Trace:
   dump_stack+0x67/0x99
   register_lock_class+0x36d/0x540
   __lock_acquire+0x7f/0x1a30
   ? irq_work_queue+0x73/0x90
   ? wake_up_klogd+0x36/0x40
   ? console_unlock+0x45d/0x540
   ? vprintk_emit+0x211/0x2e0
   lock_acquire+0xcc/0x200
   ? try_to_del_timer_sync+0x60/0x60
   del_timer_sync+0x3c/0xc0
   ? try_to_del_timer_sync+0x60/0x60
   wb_domain_exit+0x14/0x20
   mem_cgroup_free+0x14/0x40
   mem_cgroup_css_alloc+0x3f9/0x620
   cgroup_apply_control_enable+0x190/0x390
   cgroup_mkdir+0x290/0x3d0
   kernfs_iop_mkdir+0x58/0x80
   vfs_mkdir+0x10e/0x1a0
   SyS_mkdirat+0xa8/0xd0
   SyS_mkdir+0x14/0x20
   entry_SYSCALL_64_fastpath+0x18/0xad

Fix mem_cgroup_alloc() by doing more granular clean up in case of
failures.

Fixes: 0b8f73e104285 ("mm: memcontrol: clean up alloc, online, offline, free 
functions")
Signed-off-by: Tahsin Erdogan 
---
 mm/memcontrol.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c52ec893e241..9a9d5630df91 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4194,9 +4194,12 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
idr_replace(_cgroup_idr, memcg, memcg->id.id);
return memcg;
 fail:
+   for_each_node(node)
+   free_mem_cgroup_per_node_info(memcg, node);
+   free_percpu(memcg->stat);
if (memcg->id.id > 0)
idr_remove(_cgroup_idr, memcg->id.id);
-   mem_cgroup_free(memcg);
+   kfree(memcg);
return NULL;
 }
 
-- 
2.12.0.rc1.440.g5b76565f74-goog

[PATCH] mm: do not call mem_cgroup_free() from within mem_cgroup_alloc()

[Tahsin Erdogan]

mem_cgroup_free() indirectly calls wb_domain_exit() which is not
prepared to deal with a struct wb_domain object that hasn't executed
wb_domain_init(). For instance, the following warning message is
printed by lockdep if alloc_percpu() fails in mem_cgroup_alloc():

  INFO: trying to register non-static key.
  the code is fine but needs lockdep annotation.
  turning off the locking correctness validator.
  CPU: 1 PID: 1950 Comm: mkdir Not tainted 4.10.0+ #151
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Call Trace:
   dump_stack+0x67/0x99
   register_lock_class+0x36d/0x540
   __lock_acquire+0x7f/0x1a30
   ? irq_work_queue+0x73/0x90
   ? wake_up_klogd+0x36/0x40
   ? console_unlock+0x45d/0x540
   ? vprintk_emit+0x211/0x2e0
   lock_acquire+0xcc/0x200
   ? try_to_del_timer_sync+0x60/0x60
   del_timer_sync+0x3c/0xc0
   ? try_to_del_timer_sync+0x60/0x60
   wb_domain_exit+0x14/0x20
   mem_cgroup_free+0x14/0x40
   mem_cgroup_css_alloc+0x3f9/0x620
   cgroup_apply_control_enable+0x190/0x390
   cgroup_mkdir+0x290/0x3d0
   kernfs_iop_mkdir+0x58/0x80
   vfs_mkdir+0x10e/0x1a0
   SyS_mkdirat+0xa8/0xd0
   SyS_mkdir+0x14/0x20
   entry_SYSCALL_64_fastpath+0x18/0xad

Fix mem_cgroup_alloc() by doing more granular clean up in case of
failures.

Fixes: 0b8f73e104285 ("mm: memcontrol: clean up alloc, online, offline, free 
functions")
Signed-off-by: Tahsin Erdogan 
---
 mm/memcontrol.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c52ec893e241..9a9d5630df91 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4194,9 +4194,12 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
idr_replace(_cgroup_idr, memcg, memcg->id.id);
return memcg;
 fail:
+   for_each_node(node)
+   free_mem_cgroup_per_node_info(memcg, node);
+   free_percpu(memcg->stat);
if (memcg->id.id > 0)
idr_remove(_cgroup_idr, memcg->id.id);
-   mem_cgroup_free(memcg);
+   kfree(memcg);
return NULL;
 }
 
-- 
2.12.0.rc1.440.g5b76565f74-goog

Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used

[Andy Shevchenko]

On Sat, Mar 4, 2017 at 1:23 AM, James Hogan  wrote:
> On Fri, Mar 03, 2017 at 03:31:06PM +0200, Andy Shevchenko wrote:
>> On Fri, 2017-03-03 at 00:21 +, James Hogan wrote:
>> > The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
>> > seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
>> > add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:

>> Btw, I hope you also noticed this one:
>>
>> http://www.spinics.net/lists/linux-serial/msg25314.html
>
> Interesting.
>
> Following Russel's past advise[1], the following patch on top of Heiko's
> patch also fixes things for me on Octeon:
>
> [1] https://lists.gt.net/linux/kernel/2102623
>
> If thats an acceptable fix I'll post it properly. Thoughts?

Fine by me. Looks definitely better than IS_ERR_OR_NULL().

Please, submit as usual with your SoB tag.

> diff --git a/drivers/tty/serial/8250/8250_dw.c 
> b/drivers/tty/serial/8250/8250_dw.c
> index 223ac234ddb2..e65808c482f1 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, 
> struct ktermios *termios,
> rate = clk_round_rate(d->clk, baud * 16);
> if (rate < 0)
> ret = rate;
> +   else if (rate == 0)
> +   ret = -ENOENT;
> else
> ret = clk_set_rate(d->clk, rate);
> clk_prepare_enable(d->clk);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used

[Andy Shevchenko]

On Sat, Mar 4, 2017 at 1:23 AM, James Hogan  wrote:
> On Fri, Mar 03, 2017 at 03:31:06PM +0200, Andy Shevchenko wrote:
>> On Fri, 2017-03-03 at 00:21 +, James Hogan wrote:
>> > The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
>> > seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
>> > add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:

>> Btw, I hope you also noticed this one:
>>
>> http://www.spinics.net/lists/linux-serial/msg25314.html
>
> Interesting.
>
> Following Russel's past advise[1], the following patch on top of Heiko's
> patch also fixes things for me on Octeon:
>
> [1] https://lists.gt.net/linux/kernel/2102623
>
> If thats an acceptable fix I'll post it properly. Thoughts?

Fine by me. Looks definitely better than IS_ERR_OR_NULL().

Please, submit as usual with your SoB tag.

> diff --git a/drivers/tty/serial/8250/8250_dw.c 
> b/drivers/tty/serial/8250/8250_dw.c
> index 223ac234ddb2..e65808c482f1 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, 
> struct ktermios *termios,
> rate = clk_round_rate(d->clk, baud * 16);
> if (rate < 0)
> ret = rate;
> +   else if (rate == 0)
> +   ret = -ENOENT;
> else
> ret = clk_set_rate(d->clk, rate);
> clk_prepare_enable(d->clk);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 09:49:11 +0900
Masami Hiramatsu  wrote:

> On Thu,  2 Mar 2017 23:25:06 +0530
> "Naveen N. Rao"  wrote:
> 
> > We indicate support for accepting sym+offset with kretprobes through a
> > line in ftrace README. Parse the same to identify support and choose the
> > appropriate format for kprobe_events.
> 
> Could you give us an example of this change here? :)
> for example, comment of commit 613f050d68a8 .
> 
> I think the code is OK, but we need actual example of result.

Hi Naveen,

I've tried following commands

$ grep "[Tt] user_read$" /proc/kallsyms  
 T user_read
 t user_read
$ sudo ./perf probe -D user_read%return
r:probe/user_read _text+3539616
r:probe/user_read_1 _text+3653408

OK, looks good. However, when I set the retprobes, I got an error.

$ sudo ./perf probe -a user_read%return
Failed to write event: Invalid argument
  Error: Failed to add events.

And kernel rejected that.

$ dmesg -k | tail -n 1
[  850.315068] Given offset is not valid for return probe.

Hmm, curious..

I tried normal probes

$ sudo ./perf probe -D user_read
p:probe/user_read _text+3539616
p:probe/user_read_1 _text+3653408
$ sudo ./perf probe -a user_read
Added new events:
  probe:user_read  (on user_read)
  probe:user_read_1(on user_read)

You can now use it in all perf tools, such as:

perf record -e probe:user_read_1 -aR sleep 1

It works!

$ sudo ./perf probe -l
  probe:user_read  (on user_read@security/keys/user_defined.c)
  probe:user_read_1(on user_read@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
9237bf20  k  user_read+0x0[DISABLED][FTRACE]
923602a0  k  user_read+0x0[DISABLED][FTRACE]

So, the both "_text+3539616" and "_text+3653408" are correctly located
on the entry address of user_read functions. It seems kernel-side
symbol+offset check is wrong.

Thank you,


> 
> Thanks,
> 
> > 
> > Signed-off-by: Naveen N. Rao 
> > ---
> >  tools/perf/util/probe-event.c | 12 +---
> >  tools/perf/util/probe-file.c  |  7 +++
> >  tools/perf/util/probe-file.h  |  1 +
> >  3 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 35f5b7b7715c..faf5789902f5 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct 
> > probe_trace_event *tevs,
> > }
> >  
> > for (i = 0; i < ntevs; i++) {
> > -   if (!tevs[i].point.address || tevs[i].point.retprobe)
> > +   if (!tevs[i].point.address)
> > +   continue;
> > +   if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
> > continue;
> > /* If we found a wrong one, mark it by NULL symbol */
> > if (kprobe_warn_out_range(tevs[i].point.symbol,
> > @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct 
> > perf_probe_event *pev)
> > return -EINVAL;
> > }
> >  
> > -   if (pp->retprobe && !pp->function) {
> > -   semantic_error("Return probe requires an entry function.\n");
> > -   return -EINVAL;
> > -   }
> > -
> > if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
> > semantic_error("Offset/Line/Lazy pattern can't be used with "
> >"return probe.\n");
> > @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct 
> > perf_probe_event *pev,
> > }
> >  
> > /* Note that the symbols in the kmodule are not relocated */
> > -   if (!pev->uprobes && !pp->retprobe && !pev->target) {
> > +   if (!pev->uprobes && !pev->target &&
> > +   (!pp->retprobe || kretprobe_offset_is_supported())) {
> > reloc_sym = kernel_get_ref_reloc_sym();
> > if (!reloc_sym) {
> > pr_warning("Relocated base symbol is not found!\n");
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 8a219cd831b7..1542cd0d6799 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -879,6 +879,7 @@ int probe_cache__show_all_caches(struct strfilter 
> > *filter)
> >  
> >  enum ftrace_readme {
> > FTRACE_README_PROBE_TYPE_X = 0,
> > +   FTRACE_README_KRETPROBE_OFFSET,
> > FTRACE_README_END,
> >  };
> >  
> > @@ -889,6 +890,7 @@ static struct {
> >  #define DEFINE_TYPE(idx, pat)  \
> > [idx] = {.pattern = pat, .avail = false}
> > DEFINE_TYPE(FTRACE_README_PROBE_TYPE_X, "*type: * x8/16/32/64,*"),
> > +   DEFINE_TYPE(FTRACE_README_KRETPROBE_OFFSET, "*place (kretprobe): *"),
> >  };
> >  
> >  static bool scan_ftrace_readme(enum ftrace_readme type)
> > @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
> >  
> > return true;
> >  

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 09:49:11 +0900
Masami Hiramatsu  wrote:

> On Thu,  2 Mar 2017 23:25:06 +0530
> "Naveen N. Rao"  wrote:
> 
> > We indicate support for accepting sym+offset with kretprobes through a
> > line in ftrace README. Parse the same to identify support and choose the
> > appropriate format for kprobe_events.
> 
> Could you give us an example of this change here? :)
> for example, comment of commit 613f050d68a8 .
> 
> I think the code is OK, but we need actual example of result.

Hi Naveen,

I've tried following commands

$ grep "[Tt] user_read$" /proc/kallsyms  
 T user_read
 t user_read
$ sudo ./perf probe -D user_read%return
r:probe/user_read _text+3539616
r:probe/user_read_1 _text+3653408

OK, looks good. However, when I set the retprobes, I got an error.

$ sudo ./perf probe -a user_read%return
Failed to write event: Invalid argument
  Error: Failed to add events.

And kernel rejected that.

$ dmesg -k | tail -n 1
[  850.315068] Given offset is not valid for return probe.

Hmm, curious..

I tried normal probes

$ sudo ./perf probe -D user_read
p:probe/user_read _text+3539616
p:probe/user_read_1 _text+3653408
$ sudo ./perf probe -a user_read
Added new events:
  probe:user_read  (on user_read)
  probe:user_read_1(on user_read)

You can now use it in all perf tools, such as:

perf record -e probe:user_read_1 -aR sleep 1

It works!

$ sudo ./perf probe -l
  probe:user_read  (on user_read@security/keys/user_defined.c)
  probe:user_read_1(on user_read@selinux/ss/policydb.c)
$ sudo cat /sys/kernel/debug/kprobes/list
9237bf20  k  user_read+0x0[DISABLED][FTRACE]
923602a0  k  user_read+0x0[DISABLED][FTRACE]

So, the both "_text+3539616" and "_text+3653408" are correctly located
on the entry address of user_read functions. It seems kernel-side
symbol+offset check is wrong.

Thank you,


> 
> Thanks,
> 
> > 
> > Signed-off-by: Naveen N. Rao 
> > ---
> >  tools/perf/util/probe-event.c | 12 +---
> >  tools/perf/util/probe-file.c  |  7 +++
> >  tools/perf/util/probe-file.h  |  1 +
> >  3 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 35f5b7b7715c..faf5789902f5 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct 
> > probe_trace_event *tevs,
> > }
> >  
> > for (i = 0; i < ntevs; i++) {
> > -   if (!tevs[i].point.address || tevs[i].point.retprobe)
> > +   if (!tevs[i].point.address)
> > +   continue;
> > +   if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
> > continue;
> > /* If we found a wrong one, mark it by NULL symbol */
> > if (kprobe_warn_out_range(tevs[i].point.symbol,
> > @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct 
> > perf_probe_event *pev)
> > return -EINVAL;
> > }
> >  
> > -   if (pp->retprobe && !pp->function) {
> > -   semantic_error("Return probe requires an entry function.\n");
> > -   return -EINVAL;
> > -   }
> > -
> > if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
> > semantic_error("Offset/Line/Lazy pattern can't be used with "
> >"return probe.\n");
> > @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct 
> > perf_probe_event *pev,
> > }
> >  
> > /* Note that the symbols in the kmodule are not relocated */
> > -   if (!pev->uprobes && !pp->retprobe && !pev->target) {
> > +   if (!pev->uprobes && !pev->target &&
> > +   (!pp->retprobe || kretprobe_offset_is_supported())) {
> > reloc_sym = kernel_get_ref_reloc_sym();
> > if (!reloc_sym) {
> > pr_warning("Relocated base symbol is not found!\n");
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 8a219cd831b7..1542cd0d6799 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -879,6 +879,7 @@ int probe_cache__show_all_caches(struct strfilter 
> > *filter)
> >  
> >  enum ftrace_readme {
> > FTRACE_README_PROBE_TYPE_X = 0,
> > +   FTRACE_README_KRETPROBE_OFFSET,
> > FTRACE_README_END,
> >  };
> >  
> > @@ -889,6 +890,7 @@ static struct {
> >  #define DEFINE_TYPE(idx, pat)  \
> > [idx] = {.pattern = pat, .avail = false}
> > DEFINE_TYPE(FTRACE_README_PROBE_TYPE_X, "*type: * x8/16/32/64,*"),
> > +   DEFINE_TYPE(FTRACE_README_KRETPROBE_OFFSET, "*place (kretprobe): *"),
> >  };
> >  
> >  static bool scan_ftrace_readme(enum ftrace_readme type)
> > @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
> >  
> > return true;
> >  }
> > +
> > +bool kretprobe_offset_is_supported(void)
> > +{
> > +   return 

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu  wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu  wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao"  wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
>  T user_read
>  t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..
> 
> I tried normal probes
> 
> $ sudo ./perf probe -D user_read
> p:probe/user_read _text+3539616
> p:probe/user_read_1 _text+3653408
> $ sudo ./perf probe -a user_read
> Added new events:
>   probe:user_read  (on user_read)
>   probe:user_read_1(on user_read)
> 
> You can now use it in all perf tools, such as:
> 
>   perf record -e probe:user_read_1 -aR sleep 1
> 
> It works!
> 
> $ sudo ./perf probe -l
>   probe:user_read  (on user_read@security/keys/user_defined.c)
>   probe:user_read_1(on user_read@selinux/ss/policydb.c)
> $ sudo cat /sys/kernel/debug/kprobes/list
> 9237bf20  k  user_read+0x0[DISABLED][FTRACE]
> 923602a0  k  user_read+0x0[DISABLED][FTRACE]
> 
> So, the both "_text+3539616" and "_text+3653408" are correctly located
> on the entry address of user_read functions. It seems kernel-side
> symbol+offset check is wrong.

FYI, without this patch, perf probe returns same place for same-name
functions. So this patch itself looks good.

$ sudo ./perf probe -D user_read%return
r:probe/user_read user_read+0
r:probe/user_read_1 user_read+0


Thanks,


-- 
Masami Hiramatsu 

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Sat, 4 Mar 2017 11:35:51 +0900
Masami Hiramatsu  wrote:

> On Sat, 4 Mar 2017 09:49:11 +0900
> Masami Hiramatsu  wrote:
> 
> > On Thu,  2 Mar 2017 23:25:06 +0530
> > "Naveen N. Rao"  wrote:
> > 
> > > We indicate support for accepting sym+offset with kretprobes through a
> > > line in ftrace README. Parse the same to identify support and choose the
> > > appropriate format for kprobe_events.
> > 
> > Could you give us an example of this change here? :)
> > for example, comment of commit 613f050d68a8 .
> > 
> > I think the code is OK, but we need actual example of result.
> 
> Hi Naveen,
> 
> I've tried following commands
> 
> $ grep "[Tt] user_read$" /proc/kallsyms  
>  T user_read
>  t user_read
> $ sudo ./perf probe -D user_read%return
> r:probe/user_read _text+3539616
> r:probe/user_read_1 _text+3653408
> 
> OK, looks good. However, when I set the retprobes, I got an error.
> 
> $ sudo ./perf probe -a user_read%return
> Failed to write event: Invalid argument
>   Error: Failed to add events.
> 
> And kernel rejected that.
> 
> $ dmesg -k | tail -n 1
> [  850.315068] Given offset is not valid for return probe.
> 
> Hmm, curious..
> 
> I tried normal probes
> 
> $ sudo ./perf probe -D user_read
> p:probe/user_read _text+3539616
> p:probe/user_read_1 _text+3653408
> $ sudo ./perf probe -a user_read
> Added new events:
>   probe:user_read  (on user_read)
>   probe:user_read_1(on user_read)
> 
> You can now use it in all perf tools, such as:
> 
>   perf record -e probe:user_read_1 -aR sleep 1
> 
> It works!
> 
> $ sudo ./perf probe -l
>   probe:user_read  (on user_read@security/keys/user_defined.c)
>   probe:user_read_1(on user_read@selinux/ss/policydb.c)
> $ sudo cat /sys/kernel/debug/kprobes/list
> 9237bf20  k  user_read+0x0[DISABLED][FTRACE]
> 923602a0  k  user_read+0x0[DISABLED][FTRACE]
> 
> So, the both "_text+3539616" and "_text+3653408" are correctly located
> on the entry address of user_read functions. It seems kernel-side
> symbol+offset check is wrong.

FYI, without this patch, perf probe returns same place for same-name
functions. So this patch itself looks good.

$ sudo ./perf probe -D user_read%return
r:probe/user_read user_read+0
r:probe/user_read_1 user_read+0


Thanks,


-- 
Masami Hiramatsu 

[PATCH 1/3] USB: misc: ldusb: fixed decimal permission coding issue

[Milian Reichardt]

Fixed ERROR: Use 4 digit octal (0777) not decimal permissions to fulfill
the current coding-style.

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 3bc5356832db..e965ed24bb37 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -122,13 +122,13 @@ MODULE_SUPPORTED_DEVICE("LD USB Devices");
  * avoid racing conditions and get better performance of the driver.
  */
 static int ring_buffer_size = 128;
-module_param(ring_buffer_size, int, 0);
+module_param(ring_buffer_size, int, );
 MODULE_PARM_DESC(ring_buffer_size, "Read ring buffer size in reports");
 
 /* The write_buffer can contain more than one interrupt out transfer.
  */
 static int write_buffer_size = 10;
-module_param(write_buffer_size, int, 0);
+module_param(write_buffer_size, int, );
 MODULE_PARM_DESC(write_buffer_size, "Write buffer size in reports");
 
 /* As of kernel version 2.6.4 ehci-hcd uses an
@@ -141,11 +141,11 @@ MODULE_PARM_DESC(write_buffer_size, "Write buffer size in 
reports");
  * or set to 1 to use the standard interval from the endpoint descriptors.
  */
 static int min_interrupt_in_interval = 2;
-module_param(min_interrupt_in_interval, int, 0);
+module_param(min_interrupt_in_interval, int, );
 MODULE_PARM_DESC(min_interrupt_in_interval, "Minimum interrupt in interval in 
ms");
 
 static int min_interrupt_out_interval = 2;
-module_param(min_interrupt_out_interval, int, 0);
+module_param(min_interrupt_out_interval, int, );
 MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum interrupt out interval 
in ms");
 
 /* Structure to hold all of our device specific stuff */
-- 
2.12.0

[PATCH 1/3] USB: misc: ldusb: fixed decimal permission coding issue

[Milian Reichardt]

Fixed ERROR: Use 4 digit octal (0777) not decimal permissions to fulfill
the current coding-style.

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 3bc5356832db..e965ed24bb37 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -122,13 +122,13 @@ MODULE_SUPPORTED_DEVICE("LD USB Devices");
  * avoid racing conditions and get better performance of the driver.
  */
 static int ring_buffer_size = 128;
-module_param(ring_buffer_size, int, 0);
+module_param(ring_buffer_size, int, );
 MODULE_PARM_DESC(ring_buffer_size, "Read ring buffer size in reports");
 
 /* The write_buffer can contain more than one interrupt out transfer.
  */
 static int write_buffer_size = 10;
-module_param(write_buffer_size, int, 0);
+module_param(write_buffer_size, int, );
 MODULE_PARM_DESC(write_buffer_size, "Write buffer size in reports");
 
 /* As of kernel version 2.6.4 ehci-hcd uses an
@@ -141,11 +141,11 @@ MODULE_PARM_DESC(write_buffer_size, "Write buffer size in 
reports");
  * or set to 1 to use the standard interval from the endpoint descriptors.
  */
 static int min_interrupt_in_interval = 2;
-module_param(min_interrupt_in_interval, int, 0);
+module_param(min_interrupt_in_interval, int, );
 MODULE_PARM_DESC(min_interrupt_in_interval, "Minimum interrupt in interval in 
ms");
 
 static int min_interrupt_out_interval = 2;
-module_param(min_interrupt_out_interval, int, 0);
+module_param(min_interrupt_out_interval, int, );
 MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum interrupt out interval 
in ms");
 
 /* Structure to hold all of our device specific stuff */
-- 
2.12.0

[PATCH 3/3] USB: misc: ldusb: Added Space after ',' to fit the coding style

[Milian Reichardt]

Added a Space after ',' to get rid of an error message in checkpatch.pl
and improve readability

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 2c68e5153539..77b76b468307 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -561,7 +561,7 @@ static ssize_t ld_usb_write(struct file *file, const char 
__user *buffer,
/* write the data into interrupt_out_buffer from userspace */
bytes_to_write = min(count, 
write_buffer_size*dev->interrupt_out_endpoint_size);
if (bytes_to_write < count)
-   dev_warn(>intf->dev, "Write buffer overflow, %zd bytes 
dropped\n",count-bytes_to_write);
+   dev_warn(>intf->dev, "Write buffer overflow, %zd bytes 
dropped\n", count-bytes_to_write);
dev_dbg(>intf->dev, "%s: count = %zd, bytes_to_write = %zd\n",
__func__, count, bytes_to_write);
 
-- 
2.12.0

[PATCH 3/3] USB: misc: ldusb: Added Space after ',' to fit the coding style

[Milian Reichardt]

Added a Space after ',' to get rid of an error message in checkpatch.pl
and improve readability

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 2c68e5153539..77b76b468307 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -561,7 +561,7 @@ static ssize_t ld_usb_write(struct file *file, const char 
__user *buffer,
/* write the data into interrupt_out_buffer from userspace */
bytes_to_write = min(count, 
write_buffer_size*dev->interrupt_out_endpoint_size);
if (bytes_to_write < count)
-   dev_warn(>intf->dev, "Write buffer overflow, %zd bytes 
dropped\n",count-bytes_to_write);
+   dev_warn(>intf->dev, "Write buffer overflow, %zd bytes 
dropped\n", count-bytes_to_write);
dev_dbg(>intf->dev, "%s: count = %zd, bytes_to_write = %zd\n",
__func__, count, bytes_to_write);
 
-- 
2.12.0

[PATCH 0/3] fixed errors from checkpatch.pl

[Milian Reichardt]

Changed multiple errors output from checkpatch.pl to enhance readability

Milian Reichardt (3):
  USB: misc: ldusb: fixed decimal permission coding issue
  USB: misc: ldusb: changed '*' location to fit coding Style
  USB: misc: ldusb: Added Space after ',' to fit the coding style

 drivers/usb/misc/ldusb.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

-- 
2.12.0

[PATCH 2/3] USB: misc: ldusb: changed '*' location to fit coding Style

[Milian Reichardt]

Changed the location of '*' to fit the current coding style and easy
readability.

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index e965ed24bb37..2c68e5153539 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -151,20 +151,20 @@ MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum 
interrupt out interval in
 /* Structure to hold all of our device specific stuff */
 struct ld_usb {
struct mutexmutex;  /* locks this structure */
-   struct usb_interface*   intf;   /* save off the usb interface 
pointer */
+   struct usb_interface*intf;  /* save off the usb interface 
pointer */
 
int open_count; /* number of times this port 
has been opened */
 
-   char*   ring_buffer;
+   char*ring_buffer;
unsigned intring_head;
unsigned intring_tail;
 
wait_queue_head_t   read_wait;
wait_queue_head_t   write_wait;
 
-   char*   interrupt_in_buffer;
-   struct usb_endpoint_descriptor* interrupt_in_endpoint;
-   struct urb* interrupt_in_urb;
+   char*interrupt_in_buffer;
+   struct usb_endpoint_descriptor *interrupt_in_endpoint;
+   struct urb  *interrupt_in_urb;
int interrupt_in_interval;
size_t  interrupt_in_endpoint_size;
int interrupt_in_running;
@@ -172,9 +172,9 @@ struct ld_usb {
int buffer_overflow;
spinlock_t  rbsl;
 
-   char*   interrupt_out_buffer;
-   struct usb_endpoint_descriptor* interrupt_out_endpoint;
-   struct urb* interrupt_out_urb;
+   char*interrupt_out_buffer;
+   struct usb_endpoint_descriptor *interrupt_out_endpoint;
+   struct urb  *interrupt_out_urb;
int interrupt_out_interval;
size_t  interrupt_out_endpoint_size;
int interrupt_out_busy;
@@ -244,7 +244,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
if (urb->actual_length > 0) {
next_ring_head = (dev->ring_head+1) % ring_buffer_size;
if (next_ring_head != dev->ring_tail) {
-   actual_buffer = (size_t*)(dev->ring_buffer + 
dev->ring_head*(sizeof(size_t)+dev->interrupt_in_endpoint_size));
+   actual_buffer = (size_t *)(dev->ring_buffer + 
dev->ring_head * (sizeof(size_t)+dev->interrupt_in_endpoint_size));
/* actual_buffer gets urb->actual_length + 
interrupt_in_buffer */
*actual_buffer = urb->actual_length;
memcpy(actual_buffer+1, dev->interrupt_in_buffer, 
urb->actual_length);
@@ -483,7 +483,7 @@ static ssize_t ld_usb_read(struct file *file, char __user 
*buffer, size_t count,
}
 
/* actual_buffer contains actual_length + interrupt_in_buffer */
-   actual_buffer = (size_t*)(dev->ring_buffer + 
dev->ring_tail*(sizeof(size_t)+dev->interrupt_in_endpoint_size));
+   actual_buffer = (size_t *)(dev->ring_buffer + dev->ring_tail * 
(sizeof(size_t)+dev->interrupt_in_endpoint_size));
bytes_to_read = min(count, *actual_buffer);
if (bytes_to_read < *actual_buffer)
dev_warn(>intf->dev, "Read buffer overflow, %zd bytes 
dropped\n",
-- 
2.12.0

[PATCH 0/3] fixed errors from checkpatch.pl

[Milian Reichardt]

Changed multiple errors output from checkpatch.pl to enhance readability

Milian Reichardt (3):
  USB: misc: ldusb: fixed decimal permission coding issue
  USB: misc: ldusb: changed '*' location to fit coding Style
  USB: misc: ldusb: Added Space after ',' to fit the coding style

 drivers/usb/misc/ldusb.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

-- 
2.12.0

[PATCH 2/3] USB: misc: ldusb: changed '*' location to fit coding Style

[Milian Reichardt]

Changed the location of '*' to fit the current coding style and easy
readability.

Signed-of-by: Milian Reichardt 
---
 drivers/usb/misc/ldusb.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index e965ed24bb37..2c68e5153539 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -151,20 +151,20 @@ MODULE_PARM_DESC(min_interrupt_out_interval, "Minimum 
interrupt out interval in
 /* Structure to hold all of our device specific stuff */
 struct ld_usb {
struct mutexmutex;  /* locks this structure */
-   struct usb_interface*   intf;   /* save off the usb interface 
pointer */
+   struct usb_interface*intf;  /* save off the usb interface 
pointer */
 
int open_count; /* number of times this port 
has been opened */
 
-   char*   ring_buffer;
+   char*ring_buffer;
unsigned intring_head;
unsigned intring_tail;
 
wait_queue_head_t   read_wait;
wait_queue_head_t   write_wait;
 
-   char*   interrupt_in_buffer;
-   struct usb_endpoint_descriptor* interrupt_in_endpoint;
-   struct urb* interrupt_in_urb;
+   char*interrupt_in_buffer;
+   struct usb_endpoint_descriptor *interrupt_in_endpoint;
+   struct urb  *interrupt_in_urb;
int interrupt_in_interval;
size_t  interrupt_in_endpoint_size;
int interrupt_in_running;
@@ -172,9 +172,9 @@ struct ld_usb {
int buffer_overflow;
spinlock_t  rbsl;
 
-   char*   interrupt_out_buffer;
-   struct usb_endpoint_descriptor* interrupt_out_endpoint;
-   struct urb* interrupt_out_urb;
+   char*interrupt_out_buffer;
+   struct usb_endpoint_descriptor *interrupt_out_endpoint;
+   struct urb  *interrupt_out_urb;
int interrupt_out_interval;
size_t  interrupt_out_endpoint_size;
int interrupt_out_busy;
@@ -244,7 +244,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
if (urb->actual_length > 0) {
next_ring_head = (dev->ring_head+1) % ring_buffer_size;
if (next_ring_head != dev->ring_tail) {
-   actual_buffer = (size_t*)(dev->ring_buffer + 
dev->ring_head*(sizeof(size_t)+dev->interrupt_in_endpoint_size));
+   actual_buffer = (size_t *)(dev->ring_buffer + 
dev->ring_head * (sizeof(size_t)+dev->interrupt_in_endpoint_size));
/* actual_buffer gets urb->actual_length + 
interrupt_in_buffer */
*actual_buffer = urb->actual_length;
memcpy(actual_buffer+1, dev->interrupt_in_buffer, 
urb->actual_length);
@@ -483,7 +483,7 @@ static ssize_t ld_usb_read(struct file *file, char __user 
*buffer, size_t count,
}
 
/* actual_buffer contains actual_length + interrupt_in_buffer */
-   actual_buffer = (size_t*)(dev->ring_buffer + 
dev->ring_tail*(sizeof(size_t)+dev->interrupt_in_endpoint_size));
+   actual_buffer = (size_t *)(dev->ring_buffer + dev->ring_tail * 
(sizeof(size_t)+dev->interrupt_in_endpoint_size));
bytes_to_read = min(count, *actual_buffer);
if (bytes_to_read < *actual_buffer)
dev_warn(>intf->dev, "Read buffer overflow, %zd bytes 
dropped\n",
-- 
2.12.0

Re: [PATCH] staging, android: remove lowmemory killer from the tree

[Tim Murray]

Hi all,

I mentioned before that I had some ideas to overhaul lowmemorykiller,
which would have the side effect of getting it out of the kernel. I've
been working through some prototypes over the past few weeks (actually
started before Michal sent his patch out), and I'd appreciate some
feedback on what I'd like to do so I can start working on more
complete patches.

First, Michal has mentioned why the current lowmemorykiller
implementation is bad. However, the design and implementation of
lowmemorykiller is bad for Android users as well. Rather than fixing
lowmemorykiller in the kernel or enabling an equivalent
reimplementation of lowmemorykiller in userspace, I think we can solve
the Android problems and remove lowmemorykiller from the tree at the
same time.

What's wrong with lowmemorykiller from an Android user's POV?

1. lowmemorykiller can be way too aggressive when there are transient
spikes in memory consumption. LMK relies on hand-tuned thresholds to
determine when to kill a process, but hitting the threshold shouldn't
always imply a kill. For example, on some current high-end Android
devices, lowmemorykiller will start to kill oom_score_adj 200
processes once there is less than 112MB in the page cache and less
than 112MB of free pages. oom_score_adj 200 is used for processes that
are important and visible to the user but not the currently-used
foreground app; music playback or camera post-processing for some apps
usually runs as adj 200. This threshold means that even if the system
would quiesce at 110MB in the page cache and 110MB of free pages,
something important to the user may die. This is bad!

2. lowmemorykiller can be way too passive on lower memory devices.
Because lowmemorykiller has a shared threshold for the amount of free
pages and the size of the page cache before it will kill a process,
there is a scenario that we hit all the time that results in low
memory devices becoming unusable. Assume the current application and
supporting system software need X bytes in the page cache in order to
provide reasonable UI performance, and X is larger than the zone_high
watermark that stops kswapd. The number of free pages can drop below
zone_low and kswapd will start evicting pages from the page cache;
however, because the working set is actually of size X, those pages
will be paged back in about as quickly as they can be paged out. This
manifests as kswapd constantly evicting file pages and the foreground
UI workload constantly waiting on page faults. Meanwhile, even if
there are very unimportant processes to kill, lowmemorykiller won't do
anything to kill them.

#2 can be addressed somewhat by separating the limits for number of
free pages and the size of the page cache, but then lowmemorykiller
would have two sets of arbitrary hand-tuned values and still no
knowledge of kswapd/reclaim. It doesn't make sense to do that if we
can avoid it.

We have plenty of evidence for both of these on real Android devices.
I'm bringing up these issues to not only explain the problems that
we'd like to solve, but also to provide some evidence that we're
serious about fixing lowmemorykiller once and for all.

Here's where I'd like to go.

First of all, lowmemorykiller should not be in the kernel, and Android
should move to per-app mem cgroups and kill unnecessary background
tasks when under memory pressure from userspace, not the kernel.

Second, Android has good knowledge of what's important to the user and
what's not. I'd like the ability to use that information to drive
decisions about reclaiming memory, so kswapd can shrink the mem
cgroups associated with background tasks before moving on to
foreground tasks. As far as I can tell, what I'm suggesting isn't a
soft limit or something like that. We don't have specific limits on
memory consumption for particular processes, and there's no size we
definitely want to get background processes to via reclaim before we
start reclaiming from foreground or persistent processes. In practice,
I think this looks like a per-memory-cgroup reclaim priority. I have a
prototype of this where I've added a new knob called memory.priority
from 0 to 10 that serves two purposes:

- Skip reclaiming from higher-priority cgroups entirely until the
priority from shrink_zone is high enough.
- Reduce the number of pages scanned from higher-priority cgroups once
they are eligible for reclamation.

This would let kswapd reclaim from applications that aren't critical
to the user while still occasionally reclaiming from persistent
processes (evicting pages that are used very rarely from
always-running system processes). This would effectively reduce the
size of backgrounded applications without impacting UI performance--a
huge improvement over what Android can do today.

Third, assuming we can do this kind of prioritized reclaim, I'd like
more information available via vmpressure (or similar) about the
current state of kswapd in terms of what priority it's trying to
reclaim. If lmkd knew 

Re: [PATCH] staging, android: remove lowmemory killer from the tree

[Tim Murray]

Hi all,

I mentioned before that I had some ideas to overhaul lowmemorykiller,
which would have the side effect of getting it out of the kernel. I've
been working through some prototypes over the past few weeks (actually
started before Michal sent his patch out), and I'd appreciate some
feedback on what I'd like to do so I can start working on more
complete patches.

First, Michal has mentioned why the current lowmemorykiller
implementation is bad. However, the design and implementation of
lowmemorykiller is bad for Android users as well. Rather than fixing
lowmemorykiller in the kernel or enabling an equivalent
reimplementation of lowmemorykiller in userspace, I think we can solve
the Android problems and remove lowmemorykiller from the tree at the
same time.

What's wrong with lowmemorykiller from an Android user's POV?

1. lowmemorykiller can be way too aggressive when there are transient
spikes in memory consumption. LMK relies on hand-tuned thresholds to
determine when to kill a process, but hitting the threshold shouldn't
always imply a kill. For example, on some current high-end Android
devices, lowmemorykiller will start to kill oom_score_adj 200
processes once there is less than 112MB in the page cache and less
than 112MB of free pages. oom_score_adj 200 is used for processes that
are important and visible to the user but not the currently-used
foreground app; music playback or camera post-processing for some apps
usually runs as adj 200. This threshold means that even if the system
would quiesce at 110MB in the page cache and 110MB of free pages,
something important to the user may die. This is bad!

2. lowmemorykiller can be way too passive on lower memory devices.
Because lowmemorykiller has a shared threshold for the amount of free
pages and the size of the page cache before it will kill a process,
there is a scenario that we hit all the time that results in low
memory devices becoming unusable. Assume the current application and
supporting system software need X bytes in the page cache in order to
provide reasonable UI performance, and X is larger than the zone_high
watermark that stops kswapd. The number of free pages can drop below
zone_low and kswapd will start evicting pages from the page cache;
however, because the working set is actually of size X, those pages
will be paged back in about as quickly as they can be paged out. This
manifests as kswapd constantly evicting file pages and the foreground
UI workload constantly waiting on page faults. Meanwhile, even if
there are very unimportant processes to kill, lowmemorykiller won't do
anything to kill them.

#2 can be addressed somewhat by separating the limits for number of
free pages and the size of the page cache, but then lowmemorykiller
would have two sets of arbitrary hand-tuned values and still no
knowledge of kswapd/reclaim. It doesn't make sense to do that if we
can avoid it.

We have plenty of evidence for both of these on real Android devices.
I'm bringing up these issues to not only explain the problems that
we'd like to solve, but also to provide some evidence that we're
serious about fixing lowmemorykiller once and for all.

Here's where I'd like to go.

First of all, lowmemorykiller should not be in the kernel, and Android
should move to per-app mem cgroups and kill unnecessary background
tasks when under memory pressure from userspace, not the kernel.

Second, Android has good knowledge of what's important to the user and
what's not. I'd like the ability to use that information to drive
decisions about reclaiming memory, so kswapd can shrink the mem
cgroups associated with background tasks before moving on to
foreground tasks. As far as I can tell, what I'm suggesting isn't a
soft limit or something like that. We don't have specific limits on
memory consumption for particular processes, and there's no size we
definitely want to get background processes to via reclaim before we
start reclaiming from foreground or persistent processes. In practice,
I think this looks like a per-memory-cgroup reclaim priority. I have a
prototype of this where I've added a new knob called memory.priority
from 0 to 10 that serves two purposes:

- Skip reclaiming from higher-priority cgroups entirely until the
priority from shrink_zone is high enough.
- Reduce the number of pages scanned from higher-priority cgroups once
they are eligible for reclamation.

This would let kswapd reclaim from applications that aren't critical
to the user while still occasionally reclaiming from persistent
processes (evicting pages that are used very rarely from
always-running system processes). This would effectively reduce the
size of backgrounded applications without impacting UI performance--a
huge improvement over what Android can do today.

Third, assuming we can do this kind of prioritized reclaim, I'd like
more information available via vmpressure (or similar) about the
current state of kswapd in terms of what priority it's trying to
reclaim. If lmkd knew 

Re: [PATCH v2 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

Hi Michael

On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable.  No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.

I have a preliminary report.  The backlight functionality remains functional
on an S7020 across all four of the patches in this series and with the
additional 2-patch cleanup series applied.

With regard to patch 2/4 you wrote:
> Jonathan, this *really* needs testing on relevant hardware.  After
> applying this patch, you should be able to turn LCD backlight on and off
> using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> returned by that attribute upon read should be in sync with actual
> backlight state even right after loading the module (i.e. before writing
> anything to bl_power).  Please let me know if any of the above is not
> true and the module works correctly without this patch applied.

With patch 2/4 applied:

 * It is possible to read bl_power

 * It is possible to write a value to bl_power and read that value back

 * Writing values to bl_power does not appear to affect the LCD panel in 
   any way.  That is, the backlight remains unchanged regardless of the 
   value written.

 * Behaviour is the same both under X and from the terminal.

Backing out patch 2/4 but with all others still in place, resulted in no
change in behaviour.  So while bl_power had no effect with patch 2/4 in
place, it seems that patch 2/4 is *not* the cause of this.

I shall run some more bl_power tests and complete a review of the code later
this weekend.

Regards
  jonathan

Re: [PATCH v2 0/4] fujitsu_init() cleanup

[Jonathan Woithe]

Hi Michael

On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable.  No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.

I have a preliminary report.  The backlight functionality remains functional
on an S7020 across all four of the patches in this series and with the
additional 2-patch cleanup series applied.

With regard to patch 2/4 you wrote:
> Jonathan, this *really* needs testing on relevant hardware.  After
> applying this patch, you should be able to turn LCD backlight on and off
> using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> returned by that attribute upon read should be in sync with actual
> backlight state even right after loading the module (i.e. before writing
> anything to bl_power).  Please let me know if any of the above is not
> true and the module works correctly without this patch applied.

With patch 2/4 applied:

 * It is possible to read bl_power

 * It is possible to write a value to bl_power and read that value back

 * Writing values to bl_power does not appear to affect the LCD panel in 
   any way.  That is, the backlight remains unchanged regardless of the 
   value written.

 * Behaviour is the same both under X and from the terminal.

Backing out patch 2/4 but with all others still in place, resulted in no
change in behaviour.  So while bl_power had no effect with patch 2/4 in
place, it seems that patch 2/4 is *not* the cause of this.

I shall run some more bl_power tests and complete a review of the code later
this weekend.

Regards
  jonathan

Re: [RFC PATCH v1 11/30] fs: new API for handling i_version

[Jeff Layton]

On Sat, 2017-03-04 at 10:55 +1100, NeilBrown wrote:
> On Wed, Dec 21 2016, Jeff Layton wrote:
> 
> > We already have inode_inc_iversion. Add inode_set_iversion,
> > inode_get_iversion, inode_cmp_iversion and inode_iversion_need_inc.
> 
> This list of added interfaces is incomplete.
> And some of these interfaces could really use some justification up
> front.
> 
> You later add a "force" parameter to inode_inc_version.
> Why not do that up front here?
> 

First, thanks to you and Bruce for having a look.

Yes, it's clear from this and the earlier emails that I didn't do
enough documentation and explanation. I'll plan to respin this when I
get a chance, and lay out the justification and discussion a bit more.

I'll also make sure the not change the API midstream like this when I
respin.

> > +
> > +/**
> > + * inode_get_iversion - read i_version for later use
> > + * @inode: inode from which i_version should be read
> > + *
> > + * Read the inode i_version counter. This should be used by callers that 
> > wish
> > + * to store the returned i_version for later comparison.
> > + */
> > +static inline u64
> > +inode_get_iversion(const struct inode *inode)
> > +{
> > +   return inode_get_iversion_raw(inode);
> > +}
> 
> I don't understand why this can use the _raw version rather than the
> _read version.
> Surely you need to know about any changes after this read.
> 

The approach here was to convert everything to a new API and have it
work much like the code works today and then to morph it into something
that only conditionally bumps the counter.

In the later implementation, yes you do need to know about changes
after the read, but in the initial implementation it doesn't matter
since the counter is bumped on every change anyway.

I'll try to do a better job laying out this rationale in follow-on
postings.
-- 
Jeff Layton 

Re: [RFC PATCH v1 11/30] fs: new API for handling i_version

[Jeff Layton]

On Sat, 2017-03-04 at 10:55 +1100, NeilBrown wrote:
> On Wed, Dec 21 2016, Jeff Layton wrote:
> 
> > We already have inode_inc_iversion. Add inode_set_iversion,
> > inode_get_iversion, inode_cmp_iversion and inode_iversion_need_inc.
> 
> This list of added interfaces is incomplete.
> And some of these interfaces could really use some justification up
> front.
> 
> You later add a "force" parameter to inode_inc_version.
> Why not do that up front here?
> 

First, thanks to you and Bruce for having a look.

Yes, it's clear from this and the earlier emails that I didn't do
enough documentation and explanation. I'll plan to respin this when I
get a chance, and lay out the justification and discussion a bit more.

I'll also make sure the not change the API midstream like this when I
respin.

> > +
> > +/**
> > + * inode_get_iversion - read i_version for later use
> > + * @inode: inode from which i_version should be read
> > + *
> > + * Read the inode i_version counter. This should be used by callers that 
> > wish
> > + * to store the returned i_version for later comparison.
> > + */
> > +static inline u64
> > +inode_get_iversion(const struct inode *inode)
> > +{
> > +   return inode_get_iversion_raw(inode);
> > +}
> 
> I don't understand why this can use the _raw version rather than the
> _read version.
> Surely you need to know about any changes after this read.
> 

The approach here was to convert everything to a new API and have it
work much like the code works today and then to morph it into something
that only conditionally bumps the counter.

In the later implementation, yes you do need to know about changes
after the read, but in the initial implementation it doesn't matter
since the counter is bumped on every change anyway.

I'll try to do a better job laying out this rationale in follow-on
postings.
-- 
Jeff Layton 

[PATCH] Staging: media: platform: bcm2835 - Style fix

[Derek Robson]

Fixed style of block comments across whole driver
Found using checkpatch

Signed-off-by: Derek Robson 
---
 .../media/platform/bcm2835/bcm2835-camera.c| 24 ++
 drivers/staging/media/platform/bcm2835/controls.c  | 22 +++-
 .../staging/media/platform/bcm2835/mmal-msg-port.h |  6 --
 drivers/staging/media/platform/bcm2835/mmal-msg.h  | 18 
 .../staging/media/platform/bcm2835/mmal-vchiq.c|  3 ++-
 .../staging/media/platform/bcm2835/mmal-vchiq.h|  4 ++--
 6 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..25beca62a8a9 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -239,8 +239,9 @@ static struct mmal_fmt *get_format(struct v4l2_format *f)
 }
 
 /* --
-   Videobuf queue operations
-   --*/
+ * Videobuf queue operations
+ * --
+ */
 
 static int queue_setup(struct vb2_queue *vq,
   unsigned int *nbuffers, unsigned int *nplanes,
@@ -668,8 +669,9 @@ static struct vb2_ops bm2835_mmal_video_qops = {
 };
 
 /* --
-   IOCTL operations
-   --*/
+ * IOCTL operations
+ * --
+ */
 
 static int set_overlay_params(struct bm2835_mmal_dev *dev,
  struct vchiq_mmal_port *port)
@@ -834,7 +836,8 @@ static int vidioc_g_fbuf(struct file *file, void *fh,
 struct v4l2_framebuffer *a)
 {
/* The video overlay must stay within the framebuffer and can't be
-  positioned independently. */
+* positioned independently.
+*/
struct bm2835_mmal_dev *dev = video_drvdata(file);
struct vchiq_mmal_port *preview_port =
>component[MMAL_COMPONENT_CAMERA]->
@@ -1291,7 +1294,8 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void 
*priv,
}
 
/* If the format is unsupported v4l2 says we should switch to
-* a supported one and not return an error. */
+* a supported one and not return an error.
+*/
mfmt = get_format(f);
if (!mfmt) {
v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev,
@@ -1485,7 +1489,8 @@ static const struct v4l2_ioctl_ops 
camera0_ioctl_ops_gstreamer = {
.vidioc_qbuf = vb2_ioctl_qbuf,
.vidioc_dqbuf = vb2_ioctl_dqbuf,
/* Remove this function ptr to fix gstreamer bug
-   .vidioc_enum_framesizes = vidioc_enum_framesizes, */
+* .vidioc_enum_framesizes = vidioc_enum_framesizes,
+*/
.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
.vidioc_g_parm= vidioc_g_parm,
.vidioc_s_parm= vidioc_s_parm,
@@ -1498,8 +1503,9 @@ static const struct v4l2_ioctl_ops 
camera0_ioctl_ops_gstreamer = {
 };
 
 /* --
-   Driver init/finalise
-   --*/
+ * Driver init/finalise
+ * --
+ */
 
 static const struct v4l2_file_operations camera0_fops = {
.owner = THIS_MODULE,
diff --git a/drivers/staging/media/platform/bcm2835/controls.c 
b/drivers/staging/media/platform/bcm2835/controls.c
index a40987b2e75d..d96753fc761f 100644
--- a/drivers/staging/media/platform/bcm2835/controls.c
+++ b/drivers/staging/media/platform/bcm2835/controls.c
@@ -90,7 +90,8 @@ struct bm2835_mmal_v4l2_ctrl {
u32 id; /* v4l2 control identifier */
enum bm2835_mmal_ctrl_type type;
/* control minimum value or
-* mask for MMAL_CONTROL_TYPE_STD_MENU */
+* mask for MMAL_CONTROL_TYPE_STD_MENU
+*/
s32 min;
s32 max; /* maximum value of control */
s32 def;  /* default value of control */
@@ -398,10 +399,10 @@ static int ctrl_set_metering_mode(struct bm2835_mmal_dev 
*dev,
break;
 
/* todo matrix weighting not added to Linux API till 3.9
-   case V4L2_EXPOSURE_METERING_MATRIX:
-   dev->metering_mode = MMAL_PARAM_EXPOSUREMETERINGMODE_MATRIX;
-   break;
-   */
+* case V4L2_EXPOSURE_METERING_MATRIX:
+*  dev->metering_mode = MMAL_PARAM_EXPOSUREMETERINGMODE_MATRIX;
+*  break;
+*/
}
 
if (dev->scene_mode == V4L2_SCENE_MODE_NONE) {
@@ -982,8 +983,9 @@ static const struct bm2835_mmal_v4l2_ctrl 
v4l2_ctrls[V4L2_CTRL_COUNT] = 

[PATCH] Staging: media: platform: bcm2835 - Style fix

[Derek Robson]

Fixed style of block comments across whole driver
Found using checkpatch

Signed-off-by: Derek Robson 
---
 .../media/platform/bcm2835/bcm2835-camera.c| 24 ++
 drivers/staging/media/platform/bcm2835/controls.c  | 22 +++-
 .../staging/media/platform/bcm2835/mmal-msg-port.h |  6 --
 drivers/staging/media/platform/bcm2835/mmal-msg.h  | 18 
 .../staging/media/platform/bcm2835/mmal-vchiq.c|  3 ++-
 .../staging/media/platform/bcm2835/mmal-vchiq.h|  4 ++--
 6 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..25beca62a8a9 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -239,8 +239,9 @@ static struct mmal_fmt *get_format(struct v4l2_format *f)
 }
 
 /* --
-   Videobuf queue operations
-   --*/
+ * Videobuf queue operations
+ * --
+ */
 
 static int queue_setup(struct vb2_queue *vq,
   unsigned int *nbuffers, unsigned int *nplanes,
@@ -668,8 +669,9 @@ static struct vb2_ops bm2835_mmal_video_qops = {
 };
 
 /* --
-   IOCTL operations
-   --*/
+ * IOCTL operations
+ * --
+ */
 
 static int set_overlay_params(struct bm2835_mmal_dev *dev,
  struct vchiq_mmal_port *port)
@@ -834,7 +836,8 @@ static int vidioc_g_fbuf(struct file *file, void *fh,
 struct v4l2_framebuffer *a)
 {
/* The video overlay must stay within the framebuffer and can't be
-  positioned independently. */
+* positioned independently.
+*/
struct bm2835_mmal_dev *dev = video_drvdata(file);
struct vchiq_mmal_port *preview_port =
>component[MMAL_COMPONENT_CAMERA]->
@@ -1291,7 +1294,8 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void 
*priv,
}
 
/* If the format is unsupported v4l2 says we should switch to
-* a supported one and not return an error. */
+* a supported one and not return an error.
+*/
mfmt = get_format(f);
if (!mfmt) {
v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev,
@@ -1485,7 +1489,8 @@ static const struct v4l2_ioctl_ops 
camera0_ioctl_ops_gstreamer = {
.vidioc_qbuf = vb2_ioctl_qbuf,
.vidioc_dqbuf = vb2_ioctl_dqbuf,
/* Remove this function ptr to fix gstreamer bug
-   .vidioc_enum_framesizes = vidioc_enum_framesizes, */
+* .vidioc_enum_framesizes = vidioc_enum_framesizes,
+*/
.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
.vidioc_g_parm= vidioc_g_parm,
.vidioc_s_parm= vidioc_s_parm,
@@ -1498,8 +1503,9 @@ static const struct v4l2_ioctl_ops 
camera0_ioctl_ops_gstreamer = {
 };
 
 /* --
-   Driver init/finalise
-   --*/
+ * Driver init/finalise
+ * --
+ */
 
 static const struct v4l2_file_operations camera0_fops = {
.owner = THIS_MODULE,
diff --git a/drivers/staging/media/platform/bcm2835/controls.c 
b/drivers/staging/media/platform/bcm2835/controls.c
index a40987b2e75d..d96753fc761f 100644
--- a/drivers/staging/media/platform/bcm2835/controls.c
+++ b/drivers/staging/media/platform/bcm2835/controls.c
@@ -90,7 +90,8 @@ struct bm2835_mmal_v4l2_ctrl {
u32 id; /* v4l2 control identifier */
enum bm2835_mmal_ctrl_type type;
/* control minimum value or
-* mask for MMAL_CONTROL_TYPE_STD_MENU */
+* mask for MMAL_CONTROL_TYPE_STD_MENU
+*/
s32 min;
s32 max; /* maximum value of control */
s32 def;  /* default value of control */
@@ -398,10 +399,10 @@ static int ctrl_set_metering_mode(struct bm2835_mmal_dev 
*dev,
break;
 
/* todo matrix weighting not added to Linux API till 3.9
-   case V4L2_EXPOSURE_METERING_MATRIX:
-   dev->metering_mode = MMAL_PARAM_EXPOSUREMETERINGMODE_MATRIX;
-   break;
-   */
+* case V4L2_EXPOSURE_METERING_MATRIX:
+*  dev->metering_mode = MMAL_PARAM_EXPOSUREMETERINGMODE_MATRIX;
+*  break;
+*/
}
 
if (dev->scene_mode == V4L2_SCENE_MODE_NONE) {
@@ -982,8 +983,9 @@ static const struct bm2835_mmal_v4l2_ctrl 
v4l2_ctrls[V4L2_CTRL_COUNT] = {

[PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock

[Tahsin Erdogan]

blkg_conf_prep() currently calls blkg_lookup_create() while holding
request queue spinlock. This means allocating memory for struct
blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
failures in call paths like below:

  pcpu_alloc+0x68f/0x710
  __alloc_percpu_gfp+0xd/0x10
  __percpu_counter_init+0x55/0xc0
  cfq_pd_alloc+0x3b2/0x4e0
  blkg_alloc+0x187/0x230
  blkg_create+0x489/0x670
  blkg_lookup_create+0x9a/0x230
  blkg_conf_prep+0x1fb/0x240
  __cfqg_set_weight_device.isra.105+0x5c/0x180
  cfq_set_weight_on_dfl+0x69/0xc0
  cgroup_file_write+0x39/0x1c0
  kernfs_fop_write+0x13f/0x1d0
  __vfs_write+0x23/0x120
  vfs_write+0xc2/0x1f0
  SyS_write+0x44/0xb0
  entry_SYSCALL_64_fastpath+0x18/0xad

In the code path above, percpu allocator cannot call vmalloc() due to
queue spinlock.

A failure in this call path gives grief to tools which are trying to
configure io weights. We see occasional failures happen shortly after
reboots even when system is not under any memory pressure. Machines
with a lot of cpus are more vulnerable to this condition.

Update blkg_create() function to temporarily drop the rcu and queue
locks when it is allowed by gfp mask.

Suggested-by: Tejun Heo 
Signed-off-by: Tahsin Erdogan 
---
v3:
  Pushed down all blkg allocations into blkg_create()

v2:
  Moved blkg creation into blkg_lookup_create() to avoid duplicating
  blkg_lookup_create() logic.

 block/blk-cgroup.c | 96 +-
 include/linux/blk-cgroup.h | 20 --
 2 files changed, 76 insertions(+), 40 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 295e98c2c8cc..9bc2b10f3b5a 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -164,16 +164,17 @@ struct blkcg_gq *blkg_lookup_slowpath(struct blkcg *blkcg,
 EXPORT_SYMBOL_GPL(blkg_lookup_slowpath);
 
 /*
- * If @new_blkg is %NULL, this function tries to allocate a new one as
- * necessary using %GFP_NOWAIT.  @new_blkg is always consumed on return.
+ * If gfp mask allows blocking, this function temporarily drops rcu and queue
+ * locks to allocate memory.
  */
 static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
-   struct request_queue *q,
-   struct blkcg_gq *new_blkg)
+   struct request_queue *q, gfp_t gfp,
+   const struct blkcg_policy *pol)
 {
-   struct blkcg_gq *blkg;
+   struct blkcg_gq *blkg = NULL;
struct bdi_writeback_congested *wb_congested;
int i, ret;
+   const bool drop_locks = gfpflags_allow_blocking(gfp);
 
WARN_ON_ONCE(!rcu_read_lock_held());
lockdep_assert_held(q->queue_lock);
@@ -184,24 +185,48 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
goto err_free_blkg;
}
 
+   if (drop_locks) {
+   spin_unlock_irq(q->queue_lock);
+   rcu_read_unlock();
+   }
+
wb_congested = wb_congested_get_create(q->backing_dev_info,
-  blkcg->css.id,
-  GFP_NOWAIT | __GFP_NOWARN);
-   if (!wb_congested) {
+  blkcg->css.id, gfp);
+   blkg = blkg_alloc(blkcg, q, gfp);
+
+   if (drop_locks) {
+   rcu_read_lock();
+   spin_lock_irq(q->queue_lock);
+   }
+
+   if (unlikely(!wb_congested)) {
ret = -ENOMEM;
goto err_put_css;
+   } else if (unlikely(!blkg)) {
+   ret = -ENOMEM;
+   goto err_put_congested;
}
 
-   /* allocate */
-   if (!new_blkg) {
-   new_blkg = blkg_alloc(blkcg, q, GFP_NOWAIT | __GFP_NOWARN);
-   if (unlikely(!new_blkg)) {
-   ret = -ENOMEM;
+   blkg->wb_congested = wb_congested;
+
+   if (pol) {
+   WARN_ON(!drop_locks);
+
+   if (!blkcg_policy_enabled(q, pol)) {
+   ret = -EOPNOTSUPP;
+   goto err_put_congested;
+   }
+
+   /*
+* This could be the first entry point of blkcg implementation
+* and we shouldn't allow anything to go through for a bypassing
+* queue.
+*/
+   if (unlikely(blk_queue_bypass(q))) {
+   ret = blk_queue_dying(q) ? -ENODEV : -EBUSY;
goto err_put_congested;
}
}
-   blkg = new_blkg;
-   blkg->wb_congested = wb_congested;
 
/* link parent */
if (blkcg_parent(blkcg)) {
@@ -250,7 +275,7 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
 err_put_css:
css_put(>css);
 err_free_blkg:
-   blkg_free(new_blkg);
+   blkg_free(blkg);
return ERR_PTR(ret);
 }
 
@@ -258,31 +283,30 @@ static struct blkcg_gq 

[PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock

[Tahsin Erdogan]

blkg_conf_prep() currently calls blkg_lookup_create() while holding
request queue spinlock. This means allocating memory for struct
blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
failures in call paths like below:

  pcpu_alloc+0x68f/0x710
  __alloc_percpu_gfp+0xd/0x10
  __percpu_counter_init+0x55/0xc0
  cfq_pd_alloc+0x3b2/0x4e0
  blkg_alloc+0x187/0x230
  blkg_create+0x489/0x670
  blkg_lookup_create+0x9a/0x230
  blkg_conf_prep+0x1fb/0x240
  __cfqg_set_weight_device.isra.105+0x5c/0x180
  cfq_set_weight_on_dfl+0x69/0xc0
  cgroup_file_write+0x39/0x1c0
  kernfs_fop_write+0x13f/0x1d0
  __vfs_write+0x23/0x120
  vfs_write+0xc2/0x1f0
  SyS_write+0x44/0xb0
  entry_SYSCALL_64_fastpath+0x18/0xad

In the code path above, percpu allocator cannot call vmalloc() due to
queue spinlock.

A failure in this call path gives grief to tools which are trying to
configure io weights. We see occasional failures happen shortly after
reboots even when system is not under any memory pressure. Machines
with a lot of cpus are more vulnerable to this condition.

Update blkg_create() function to temporarily drop the rcu and queue
locks when it is allowed by gfp mask.

Suggested-by: Tejun Heo 
Signed-off-by: Tahsin Erdogan 
---
v3:
  Pushed down all blkg allocations into blkg_create()

v2:
  Moved blkg creation into blkg_lookup_create() to avoid duplicating
  blkg_lookup_create() logic.

 block/blk-cgroup.c | 96 +-
 include/linux/blk-cgroup.h | 20 --
 2 files changed, 76 insertions(+), 40 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 295e98c2c8cc..9bc2b10f3b5a 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -164,16 +164,17 @@ struct blkcg_gq *blkg_lookup_slowpath(struct blkcg *blkcg,
 EXPORT_SYMBOL_GPL(blkg_lookup_slowpath);
 
 /*
- * If @new_blkg is %NULL, this function tries to allocate a new one as
- * necessary using %GFP_NOWAIT.  @new_blkg is always consumed on return.
+ * If gfp mask allows blocking, this function temporarily drops rcu and queue
+ * locks to allocate memory.
  */
 static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
-   struct request_queue *q,
-   struct blkcg_gq *new_blkg)
+   struct request_queue *q, gfp_t gfp,
+   const struct blkcg_policy *pol)
 {
-   struct blkcg_gq *blkg;
+   struct blkcg_gq *blkg = NULL;
struct bdi_writeback_congested *wb_congested;
int i, ret;
+   const bool drop_locks = gfpflags_allow_blocking(gfp);
 
WARN_ON_ONCE(!rcu_read_lock_held());
lockdep_assert_held(q->queue_lock);
@@ -184,24 +185,48 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
goto err_free_blkg;
}
 
+   if (drop_locks) {
+   spin_unlock_irq(q->queue_lock);
+   rcu_read_unlock();
+   }
+
wb_congested = wb_congested_get_create(q->backing_dev_info,
-  blkcg->css.id,
-  GFP_NOWAIT | __GFP_NOWARN);
-   if (!wb_congested) {
+  blkcg->css.id, gfp);
+   blkg = blkg_alloc(blkcg, q, gfp);
+
+   if (drop_locks) {
+   rcu_read_lock();
+   spin_lock_irq(q->queue_lock);
+   }
+
+   if (unlikely(!wb_congested)) {
ret = -ENOMEM;
goto err_put_css;
+   } else if (unlikely(!blkg)) {
+   ret = -ENOMEM;
+   goto err_put_congested;
}
 
-   /* allocate */
-   if (!new_blkg) {
-   new_blkg = blkg_alloc(blkcg, q, GFP_NOWAIT | __GFP_NOWARN);
-   if (unlikely(!new_blkg)) {
-   ret = -ENOMEM;
+   blkg->wb_congested = wb_congested;
+
+   if (pol) {
+   WARN_ON(!drop_locks);
+
+   if (!blkcg_policy_enabled(q, pol)) {
+   ret = -EOPNOTSUPP;
+   goto err_put_congested;
+   }
+
+   /*
+* This could be the first entry point of blkcg implementation
+* and we shouldn't allow anything to go through for a bypassing
+* queue.
+*/
+   if (unlikely(blk_queue_bypass(q))) {
+   ret = blk_queue_dying(q) ? -ENODEV : -EBUSY;
goto err_put_congested;
}
}
-   blkg = new_blkg;
-   blkg->wb_congested = wb_congested;
 
/* link parent */
if (blkcg_parent(blkcg)) {
@@ -250,7 +275,7 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
 err_put_css:
css_put(>css);
 err_free_blkg:
-   blkg_free(new_blkg);
+   blkg_free(blkg);
return ERR_PTR(ret);
 }
 
@@ -258,31 +283,30 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
  * 

Re: [PATCH v3 0/3] Locality support for tpm_crb

[Jerry Snitselaar]

Jarkko Sakkinen @ 2017-02-08 11:11 GMT:

> The tpm_crb driver should follow the policy of reserving and
> relinquishing the locality it uses when multiple localities are used,
> like when TXT is another locality.
>
> [This is a resend of v3 as this has been asked from me for a few times
>  now and there wasn't anything obvious to fix.]
>
> Jarkko Sakkinen (3):
>   tpm_crb: map locality registers
>   tpm_crb: encapsulate crb_wait_for_reg_32
>   tpm_crb: request and relinquish locality 0
>
>  drivers/char/tpm/tpm_crb.c | 185 
> +
>  1 file changed, 137 insertions(+), 48 deletions(-)

Reviewed-by: Jerry Snitselaar 

Re: [PATCH v3 0/3] Locality support for tpm_crb

[Jerry Snitselaar]

Jarkko Sakkinen @ 2017-02-08 11:11 GMT:

> The tpm_crb driver should follow the policy of reserving and
> relinquishing the locality it uses when multiple localities are used,
> like when TXT is another locality.
>
> [This is a resend of v3 as this has been asked from me for a few times
>  now and there wasn't anything obvious to fix.]
>
> Jarkko Sakkinen (3):
>   tpm_crb: map locality registers
>   tpm_crb: encapsulate crb_wait_for_reg_32
>   tpm_crb: request and relinquish locality 0
>
>  drivers/char/tpm/tpm_crb.c | 185 
> +
>  1 file changed, 137 insertions(+), 48 deletions(-)

Reviewed-by: Jerry Snitselaar 

Re: [PATCH v3 0/3] Add support for MyGica T230C DVB-T2 stick

[Antti Palosaari]

On 03/03/2017 08:35 PM, Brüns, Stefan wrote:

On Fr, 2017-02-17 at 01:55 +0100, Stefan Brüns wrote:

The required command sequence for the new tuner (Si2141) was traced
from the
current Windows driver and verified with a small python
script/libusb.
The changes to the Si2168 and dvbsky driver are mostly additions of
the
required IDs and some glue code.

Stefan Brüns (3):
  [media] si2157: Add support for Si2141-A10
  [media] si2168: add support for Si2168-D60
  [media] dvbsky: MyGica T230C support

 drivers/media/dvb-core/dvb-usb-ids.h  |  1 +
 drivers/media/dvb-frontends/si2168.c  |  4 ++
 drivers/media/dvb-frontends/si2168_priv.h |  2 +
 drivers/media/tuners/si2157.c | 23 +++-
 drivers/media/tuners/si2157_priv.h|  2 +
 drivers/media/usb/dvb-usb-v2/dvbsky.c | 88
+++
 6 files changed, 118 insertions(+), 2 deletions(-)


Instead of this series, a different patchset was accepted, although
Antti raised concerns about at least 2 of the 3 patches accpeted, more
specifically the si2157 patch contains some bogus initialization code,
and the T230C support were better added to the dvbsky driver instead of
 cxusb.


Patch set looks good. I ordered that device and it arrived yesterday. I 
will handle that during 2 weeks - it is now skiing holiday and I am at 
France alps whole next week. So just wait :)


regards
Antti

--
http://palosaari.fi/

Re: [PATCH v3 0/3] Add support for MyGica T230C DVB-T2 stick

[Antti Palosaari]

On 03/03/2017 08:35 PM, Brüns, Stefan wrote:

On Fr, 2017-02-17 at 01:55 +0100, Stefan Brüns wrote:

The required command sequence for the new tuner (Si2141) was traced
from the
current Windows driver and verified with a small python
script/libusb.
The changes to the Si2168 and dvbsky driver are mostly additions of
the
required IDs and some glue code.

Stefan Brüns (3):
  [media] si2157: Add support for Si2141-A10
  [media] si2168: add support for Si2168-D60
  [media] dvbsky: MyGica T230C support

 drivers/media/dvb-core/dvb-usb-ids.h  |  1 +
 drivers/media/dvb-frontends/si2168.c  |  4 ++
 drivers/media/dvb-frontends/si2168_priv.h |  2 +
 drivers/media/tuners/si2157.c | 23 +++-
 drivers/media/tuners/si2157_priv.h|  2 +
 drivers/media/usb/dvb-usb-v2/dvbsky.c | 88
+++
 6 files changed, 118 insertions(+), 2 deletions(-)


Instead of this series, a different patchset was accepted, although
Antti raised concerns about at least 2 of the 3 patches accpeted, more
specifically the si2157 patch contains some bogus initialization code,
and the T230C support were better added to the dvbsky driver instead of
 cxusb.


Patch set looks good. I ordered that device and it arrived yesterday. I 
will handle that during 2 weeks - it is now skiing holiday and I am at 
France alps whole next week. So just wait :)


regards
Antti

--
http://palosaari.fi/

Re: [PATCH] uapi: fix asm/signal.h userspace compilation errors

[Carlos O'Donell]

On Thu, Mar 2, 2017 at 10:48 AM, Dmitry V. Levin  wrote:
> On Thu, Mar 02, 2017 at 10:22:18AM -0500, Carlos O'Donell wrote:
>> On Wed, Mar 1, 2017 at 11:20 AM, Arnd Bergmann  wrote:
>> > On Sun, Feb 26, 2017 at 2:01 AM, Dmitry V. Levin  wrote:
>> >> Include  (guarded by #ifndef __KERNEL__) to fix asm/signal.h
>> >> userspace compilation errors like this:
>> >>
>> >> /usr/include/asm/signal.h:126:2: error: unknown type name 'size_t'
>> >>   size_t ss_size;
>> >>
>> >> As no uapi header provides a definition of size_t, inclusion
>> >> of  seems to be the most conservative fix available.
> [...]
>> > I'm not sure if this is the best fix. We generally should not include one
>> > standard header from another standard header. Would it be possible
>> > to use __kernel_size_t instead of size_t?
>>
>> In glibc we handle this with special use of __need_size_t with GCC's
>> provided stddef.h.
>>
>> For example glibc's signal.h does this:
>>
>> # define __need_size_t
>> # include 
>
> Just to make it clear, do you suggest this approach for asm/signal.h as well?

The kernel is duplicating userspace headers in the UAPI implementation
and running into exactly the same problems we have already solved in
userspace. We currently have no better solution than the "__need_*"
interface for avoiding the duplication of the type definitions and the
problems that come with that.

I am taking this up with senior glibc developers on libc-alpha to see
if we have a better suggestion. If you give me 72 hours I'll either
have a better suggestion or the acknowledgement that my suggestion is
the best practical solution we have.

Note that in a GNU userspace stddef.h here comes from gcc, which
completes the implementation of the C development environment that
provides the types you need. The use of "__need_size_t" is a collusion
between the components of the implementation and use by the Linux
kernel would mean expecting something specific to a GNU
implementation.

I might suggest you use include/uapi/linux/libc-compat.h in an attempt
to abstract away the GNU-specific magic for getting just size_t from
stddef.h. That way you have documented the places that other runtime
authors need to fill out for things to work.

Cheers,
Carlos.

Re: [PATCH] uapi: fix asm/signal.h userspace compilation errors

[Carlos O'Donell]

On Thu, Mar 2, 2017 at 10:48 AM, Dmitry V. Levin  wrote:
> On Thu, Mar 02, 2017 at 10:22:18AM -0500, Carlos O'Donell wrote:
>> On Wed, Mar 1, 2017 at 11:20 AM, Arnd Bergmann  wrote:
>> > On Sun, Feb 26, 2017 at 2:01 AM, Dmitry V. Levin  wrote:
>> >> Include  (guarded by #ifndef __KERNEL__) to fix asm/signal.h
>> >> userspace compilation errors like this:
>> >>
>> >> /usr/include/asm/signal.h:126:2: error: unknown type name 'size_t'
>> >>   size_t ss_size;
>> >>
>> >> As no uapi header provides a definition of size_t, inclusion
>> >> of  seems to be the most conservative fix available.
> [...]
>> > I'm not sure if this is the best fix. We generally should not include one
>> > standard header from another standard header. Would it be possible
>> > to use __kernel_size_t instead of size_t?
>>
>> In glibc we handle this with special use of __need_size_t with GCC's
>> provided stddef.h.
>>
>> For example glibc's signal.h does this:
>>
>> # define __need_size_t
>> # include 
>
> Just to make it clear, do you suggest this approach for asm/signal.h as well?

The kernel is duplicating userspace headers in the UAPI implementation
and running into exactly the same problems we have already solved in
userspace. We currently have no better solution than the "__need_*"
interface for avoiding the duplication of the type definitions and the
problems that come with that.

I am taking this up with senior glibc developers on libc-alpha to see
if we have a better suggestion. If you give me 72 hours I'll either
have a better suggestion or the acknowledgement that my suggestion is
the best practical solution we have.

Note that in a GNU userspace stddef.h here comes from gcc, which
completes the implementation of the C development environment that
provides the types you need. The use of "__need_size_t" is a collusion
between the components of the implementation and use by the Linux
kernel would mean expecting something specific to a GNU
implementation.

I might suggest you use include/uapi/linux/libc-compat.h in an attempt
to abstract away the GNU-specific magic for getting just size_t from
stddef.h. That way you have documented the places that other runtime
authors need to fill out for things to work.

Cheers,
Carlos.

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> This fixes type warnings generated by sparse, replaces instances of
> ntohs() with be16_to_cpu() and removes unused fields in structs.

There's no real need to convert ntohs to be16_to_cpu

> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
[]
> 
> @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, 
> unsigned char *buffer,
>   hdr = (struct hostif_hdr *)buffer;
>  
>   DPRINTK(4, "size=%d\n", hdr->size);
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> 

This isn't mentioned and doesn't match the commit message

Re: [PATCH] staging: ks7010: clean up code

[Joe Perches]

On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote:
> This fixes type warnings generated by sparse, replaces instances of
> ntohs() with be16_to_cpu() and removes unused fields in structs.

There's no real need to convert ntohs to be16_to_cpu

> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
[]
> 
> @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, 
> unsigned char *buffer,
>   hdr = (struct hostif_hdr *)buffer;
>  
>   DPRINTK(4, "size=%d\n", hdr->size);
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + HIF_REQ_MAX < le16_to_cpu(hdr->event)) {
> 

This isn't mentioned and doesn't match the commit message

[GIT PULL] final round of SCSI updates for the 4.10+ merge window

[James Bottomley]

This is the set of stuff that didn't quite make the initial pull and a
set of fixes for stuff which did.  The new stuff is basically lpfc
(nvme), qedi and aacraid.  The fixes cover a lot of previously
submitted stuff, the most important of which probably covers some of
the failing irq vectors allocation and other fallout from having the
SCSI command allocated as part of the block allocation functions.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-misc

The short changelog is:

Arnd Bergmann (3):
  scsi: lpfc: use proper format string for dma_addr_t
  scsi: lpfc: use div_u64 for 64-bit division
  scsi: smartpqi: fix time handling

Christoph Hellwig (7):
  scsi: remove scsi_execute_req_flags
  scsi: merge __scsi_execute into scsi_execute
  scsi: simplify scsi_execute_req_flags
  scsi: make the sense header argument to scsi_test_unit_ready mandatory
  scsi: sd: improve TUR handling in sd_check_events
  scsi: always zero sshdr in scsi_normalize_sense
  scsi: lpfc: use pci_irq_alloc_vectors and pci_irq_free_vectors

Colin Ian King (3):
  scsi: aacraid: remove redundant zero check on ret
  scsi: qla2xxx: fix spelling mistake: "seperator" -> "separator"
  scsi: fix memory leak of sdpk on when gd fails to allocate

Dan Carpenter (1):
  scsi: scsi_dh_emc: return success in clariion_std_inquiry()

Don Brace (1):
  scsi: cciss: correct check map error.

Dupuis, Chad (3):
  scsi: qedi: Fix memory leak in tmf response processing.
  scsi: qedf: fixup compilation warning about atomic_t usage
  scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.

Finn Thain (1):
  scsi: mac_scsi: Fix MAC_SCSI=m option when SCSI=m

Hannes Reinecke (2):
  scsi: mpt3sas: switch to pci_alloc_irq_vectors
  scsi: use 'scsi_device_from_queue()' for scsi_dh

James Smart (16):
  scsi: lpfc: add missing Kconfig NVME dependencies
  scsi: lpfc: Update lpfc version to 11.2.0.7
  scsi: lpfc: Update copyrights
  scsi: lpfc: NVME Target: Add debugfs support
  scsi: lpfc: NVME Target: bind to nvmet_fc api
  scsi: lpfc: NVME Target: Merge into FC discovery
  scsi: lpfc: NVME Target: Receive buffer updates
  scsi: lpfc: NVME Target: Base modifications
  scsi: lpfc: NVME Initiator: Add debugfs support
  scsi: lpfc: NVME Initiator: bind to nvme_fc api
  scsi: lpfc: NVME Initiator: Merge into FC discovery
  scsi: lpfc: NVME Initiator: Base modifications
  scsi: lpfc: refactor debugfs queue dump routines
  scsi: lpfc: refactor debugfs queue prints
  scsi: lpfc: minor code cleanups
  scsi: lpfc: Correct WQ creation for pagesize

Matthew R. Ochs (1):
  scsi: cxlflash: Enable PCI device ID for future IBM CXL Flash AFU

Michael Hernandez (3):
  scsi: qla2xxx: Fix Regression introduced by 
pci_alloc_irq_vectors_affinity call.
  scsi: qla2xxx: Fix response queue count for Target mode.
  scsi: qla2xxx: Cleaned up queue configuration code.

Raghava Aditya Renukunta (16):
  scsi: aacraid: Fixed expander hotplug for SMART family
  scsi: aacraid: Update driver version
  scsi: aacraid: Fix a potential spinlock double unlock bug
  scsi: aacraid: Save adapter fib log before an IOP reset
  scsi: aacraid: Reorder Adapter status check
  scsi: aacraid: Skip IOP reset on controller panic(SMART Family)
  scsi: aacraid: Decrease adapter health check interval
  scsi: aacraid: Reload offlined drives after controller reset
  scsi: aacraid: Skip wellness sync on controller failure
  scsi: aacraid: Fix sync fibs time out on controller reset
  scsi: aacraid: Added sysfs for driver version
  scsi: aacraid: Fix memory leak in fib init path
  scsi: aacraid: Prevent E3 lockup when deleting units
  scsi: aacraid: Fix for excessive prints on EEH
  scsi: aacraid: Use correct channel number for raw srb
  scsi: aacraid: Fix camel case

Subhash Jadavani (1):
  scsi: ufs-qcom: remove redundant condition check

Wei Yongjun (1):
  scsi: sd: make sd_devt_release() static

And the diffstat

 MAINTAINERS |6 +
 drivers/ata/libata-scsi.c   |   12 +-
 drivers/block/cciss.c   |   32 +-
 drivers/scsi/Kconfig|4 +-
 drivers/scsi/Makefile   |1 +
 drivers/scsi/aacraid/aachba.c   |   59 +-
 drivers/scsi/aacraid/aacraid.h  |  107 +-
 drivers/scsi/aacraid/commctrl.c |2 +-
 drivers/scsi/aacraid/comminit.c |2 +-
 drivers/scsi/aacraid/commsup.c  |  118 +-
 drivers/scsi/aacraid/linit.c|   47 +-
 drivers/scsi/aacraid/rx.c   |2 +-
 drivers/scsi/aacraid/src.c  |   48 +-
 drivers/scsi/cxlflash/main.c|4 +
 drivers/scsi/cxlflash/main.h|1 +
 

[GIT PULL] final round of SCSI updates for the 4.10+ merge window

[James Bottomley]

This is the set of stuff that didn't quite make the initial pull and a
set of fixes for stuff which did.  The new stuff is basically lpfc
(nvme), qedi and aacraid.  The fixes cover a lot of previously
submitted stuff, the most important of which probably covers some of
the failing irq vectors allocation and other fallout from having the
SCSI command allocated as part of the block allocation functions.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-misc

The short changelog is:

Arnd Bergmann (3):
  scsi: lpfc: use proper format string for dma_addr_t
  scsi: lpfc: use div_u64 for 64-bit division
  scsi: smartpqi: fix time handling

Christoph Hellwig (7):
  scsi: remove scsi_execute_req_flags
  scsi: merge __scsi_execute into scsi_execute
  scsi: simplify scsi_execute_req_flags
  scsi: make the sense header argument to scsi_test_unit_ready mandatory
  scsi: sd: improve TUR handling in sd_check_events
  scsi: always zero sshdr in scsi_normalize_sense
  scsi: lpfc: use pci_irq_alloc_vectors and pci_irq_free_vectors

Colin Ian King (3):
  scsi: aacraid: remove redundant zero check on ret
  scsi: qla2xxx: fix spelling mistake: "seperator" -> "separator"
  scsi: fix memory leak of sdpk on when gd fails to allocate

Dan Carpenter (1):
  scsi: scsi_dh_emc: return success in clariion_std_inquiry()

Don Brace (1):
  scsi: cciss: correct check map error.

Dupuis, Chad (3):
  scsi: qedi: Fix memory leak in tmf response processing.
  scsi: qedf: fixup compilation warning about atomic_t usage
  scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.

Finn Thain (1):
  scsi: mac_scsi: Fix MAC_SCSI=m option when SCSI=m

Hannes Reinecke (2):
  scsi: mpt3sas: switch to pci_alloc_irq_vectors
  scsi: use 'scsi_device_from_queue()' for scsi_dh

James Smart (16):
  scsi: lpfc: add missing Kconfig NVME dependencies
  scsi: lpfc: Update lpfc version to 11.2.0.7
  scsi: lpfc: Update copyrights
  scsi: lpfc: NVME Target: Add debugfs support
  scsi: lpfc: NVME Target: bind to nvmet_fc api
  scsi: lpfc: NVME Target: Merge into FC discovery
  scsi: lpfc: NVME Target: Receive buffer updates
  scsi: lpfc: NVME Target: Base modifications
  scsi: lpfc: NVME Initiator: Add debugfs support
  scsi: lpfc: NVME Initiator: bind to nvme_fc api
  scsi: lpfc: NVME Initiator: Merge into FC discovery
  scsi: lpfc: NVME Initiator: Base modifications
  scsi: lpfc: refactor debugfs queue dump routines
  scsi: lpfc: refactor debugfs queue prints
  scsi: lpfc: minor code cleanups
  scsi: lpfc: Correct WQ creation for pagesize

Matthew R. Ochs (1):
  scsi: cxlflash: Enable PCI device ID for future IBM CXL Flash AFU

Michael Hernandez (3):
  scsi: qla2xxx: Fix Regression introduced by 
pci_alloc_irq_vectors_affinity call.
  scsi: qla2xxx: Fix response queue count for Target mode.
  scsi: qla2xxx: Cleaned up queue configuration code.

Raghava Aditya Renukunta (16):
  scsi: aacraid: Fixed expander hotplug for SMART family
  scsi: aacraid: Update driver version
  scsi: aacraid: Fix a potential spinlock double unlock bug
  scsi: aacraid: Save adapter fib log before an IOP reset
  scsi: aacraid: Reorder Adapter status check
  scsi: aacraid: Skip IOP reset on controller panic(SMART Family)
  scsi: aacraid: Decrease adapter health check interval
  scsi: aacraid: Reload offlined drives after controller reset
  scsi: aacraid: Skip wellness sync on controller failure
  scsi: aacraid: Fix sync fibs time out on controller reset
  scsi: aacraid: Added sysfs for driver version
  scsi: aacraid: Fix memory leak in fib init path
  scsi: aacraid: Prevent E3 lockup when deleting units
  scsi: aacraid: Fix for excessive prints on EEH
  scsi: aacraid: Use correct channel number for raw srb
  scsi: aacraid: Fix camel case

Subhash Jadavani (1):
  scsi: ufs-qcom: remove redundant condition check

Wei Yongjun (1):
  scsi: sd: make sd_devt_release() static

And the diffstat

 MAINTAINERS |6 +
 drivers/ata/libata-scsi.c   |   12 +-
 drivers/block/cciss.c   |   32 +-
 drivers/scsi/Kconfig|4 +-
 drivers/scsi/Makefile   |1 +
 drivers/scsi/aacraid/aachba.c   |   59 +-
 drivers/scsi/aacraid/aacraid.h  |  107 +-
 drivers/scsi/aacraid/commctrl.c |2 +-
 drivers/scsi/aacraid/comminit.c |2 +-
 drivers/scsi/aacraid/commsup.c  |  118 +-
 drivers/scsi/aacraid/linit.c|   47 +-
 drivers/scsi/aacraid/rx.c   |2 +-
 drivers/scsi/aacraid/src.c  |   48 +-
 drivers/scsi/cxlflash/main.c|4 +
 drivers/scsi/cxlflash/main.h|1 +
 

Re: [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization

[Jeff Layton]

On Fri, 2017-03-03 at 18:00 -0500, J. Bruce Fields wrote:
> On Wed, Dec 21, 2016 at 12:03:17PM -0500, Jeff Layton wrote:
> > tl;dr: I think we can greatly reduce the cost of the inode->i_version
> > counter, by exploiting the fact that we don't need to increment it
> > if no one is looking at it. We can also clean up the code to prepare
> > to eventually expose this value via statx().
> > 
> > The inode->i_version field is supposed to be a value that changes
> > whenever there is any data or metadata change to the inode. Some
> > filesystems use it internally to detect directory changes during
> > readdir. knfsd will use it if the filesystem has MS_I_VERSION
> > set. IMA will also use it (though it's not clear to me that that
> > works 100% -- no check for MS_I_VERSION there).
> 
> I'm a little confused about the internal uses for directories.  Is it
> necessarily kept on disk?

No, they aren't necessarily stored on disk, and in fact they aren't on
most (maybe all?) of those filesystems. It's just a convenient place to
store a dir change value that is subsequently checked in readdir
operations.

That's part of the "fun" of untangling this mess. ;)

> In cases it's not, then there's not the same
> performance issue, right? 

Right, I don't think it's really a big deal for most of those and I'm
not terribly concerned about the i_version counter on directory change
operations. An extra atomic op on a directory change seems unlikely to
hurt anything.

The main purpose of this is to improve the situation with small writes.
This should also help pave the way for fs' like NFS and Ceph that
implement a version counter but may not necessarily bump it on every
change.

I think once we have things more consistent, we'll be able to consider
exposing the i_version counter to userland via statx.

> Is there any risk these patches make
> performance slightly worse in that case?
> 

Maybe, but I think that risk is pretty low. Note that I haven't measured
that specifically here, so I could be completely wrong.

If it is a problem, we could consider unioning this thing with a non-
atomic field for the directory change cases, but that would add some
complexity and I'm not sure it's worth it. I'd want to measure it first.

> > Only btrfs, ext4, and xfs implement it for data changes. Because of
> > this, these filesystems must log the inode to disk whenever the
> > i_version counter changes.
> 
> On those filesystems that's done for both files and directories, right?
> 

Yes.

> > That has a non-zero performance impact,
> > especially on write-heavy workloads, because we end up dirtying the
> > inode metadata on every write, not just when the times change. [1]
> > 
> > It turns out though that none of these users of i_version require that
> > i_version change on every change to the file. The only real requirement
> > is that it be different if _something_ changed since the last time we
> > queried for it. [2]
> > 
> > So, if we simply keep track of when something queries the value, we
> > can avoid bumping the counter and that metadata update.
> > 
> > This patchset implements this:
> > 
> > It starts with some small cleanup patches to just remove any mention of
> > the i_version counter in filesystems that don't actually use it.
> > 
> > Then, we add a new set of static inlines for managing the counter. The
> > initial version should work identically to what we have now. Then, all
> > of the remaining filesystems that use i_version are converted to the new
> > inlines.
> > 
> > Once that's in place, we switch to a new implementation that allows us
> > to track readers of i_version counter, and only bump it when it's
> > necessary or convenient (i.e. we're going to disk anyway).
> > 
> > The final patch switches from a scheme that uses the i_lock to serialize
> > the counter updates during write to an atomic64_t. That's a wash
> > performance-wise in my testing, but I like not having to take the i_lock
> > down where it could end up nested inside other locks.
> > 
> > With this, we reduce inode metadata updates across all 3 filesystems
> > down to roughly the frequency of the timestamp granularity, particularly
> > when it's not being queried (the vastly common case).
> > 
> > The pessimal workload here is 1 byte writes, and it helps that
> > significantly. Of course, that's not a real-world workload.
> > 
> > A tiobench-example.fio workload also shows some modest performance
> > gains, and I've gotten mails from the kernel test robot that show some
> > significant performance gains on some microbenchmarks (case-msync-mt in
> > the vm-scalability testsuite to be specific).
> > 
> > I'm happy to run other workloads if anyone can suggest them.
> > 
> > At this point, the patchset works and does what it's expected to do in
> > my own testing. It seems like it's at least a modest performance win
> > across all 3 major disk-based filesystems. It may also encourage others
> > to implement i_version as well since it reduces that 

Re: [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization

[Jeff Layton]

On Fri, 2017-03-03 at 18:00 -0500, J. Bruce Fields wrote:
> On Wed, Dec 21, 2016 at 12:03:17PM -0500, Jeff Layton wrote:
> > tl;dr: I think we can greatly reduce the cost of the inode->i_version
> > counter, by exploiting the fact that we don't need to increment it
> > if no one is looking at it. We can also clean up the code to prepare
> > to eventually expose this value via statx().
> > 
> > The inode->i_version field is supposed to be a value that changes
> > whenever there is any data or metadata change to the inode. Some
> > filesystems use it internally to detect directory changes during
> > readdir. knfsd will use it if the filesystem has MS_I_VERSION
> > set. IMA will also use it (though it's not clear to me that that
> > works 100% -- no check for MS_I_VERSION there).
> 
> I'm a little confused about the internal uses for directories.  Is it
> necessarily kept on disk?

No, they aren't necessarily stored on disk, and in fact they aren't on
most (maybe all?) of those filesystems. It's just a convenient place to
store a dir change value that is subsequently checked in readdir
operations.

That's part of the "fun" of untangling this mess. ;)

> In cases it's not, then there's not the same
> performance issue, right? 

Right, I don't think it's really a big deal for most of those and I'm
not terribly concerned about the i_version counter on directory change
operations. An extra atomic op on a directory change seems unlikely to
hurt anything.

The main purpose of this is to improve the situation with small writes.
This should also help pave the way for fs' like NFS and Ceph that
implement a version counter but may not necessarily bump it on every
change.

I think once we have things more consistent, we'll be able to consider
exposing the i_version counter to userland via statx.

> Is there any risk these patches make
> performance slightly worse in that case?
> 

Maybe, but I think that risk is pretty low. Note that I haven't measured
that specifically here, so I could be completely wrong.

If it is a problem, we could consider unioning this thing with a non-
atomic field for the directory change cases, but that would add some
complexity and I'm not sure it's worth it. I'd want to measure it first.

> > Only btrfs, ext4, and xfs implement it for data changes. Because of
> > this, these filesystems must log the inode to disk whenever the
> > i_version counter changes.
> 
> On those filesystems that's done for both files and directories, right?
> 

Yes.

> > That has a non-zero performance impact,
> > especially on write-heavy workloads, because we end up dirtying the
> > inode metadata on every write, not just when the times change. [1]
> > 
> > It turns out though that none of these users of i_version require that
> > i_version change on every change to the file. The only real requirement
> > is that it be different if _something_ changed since the last time we
> > queried for it. [2]
> > 
> > So, if we simply keep track of when something queries the value, we
> > can avoid bumping the counter and that metadata update.
> > 
> > This patchset implements this:
> > 
> > It starts with some small cleanup patches to just remove any mention of
> > the i_version counter in filesystems that don't actually use it.
> > 
> > Then, we add a new set of static inlines for managing the counter. The
> > initial version should work identically to what we have now. Then, all
> > of the remaining filesystems that use i_version are converted to the new
> > inlines.
> > 
> > Once that's in place, we switch to a new implementation that allows us
> > to track readers of i_version counter, and only bump it when it's
> > necessary or convenient (i.e. we're going to disk anyway).
> > 
> > The final patch switches from a scheme that uses the i_lock to serialize
> > the counter updates during write to an atomic64_t. That's a wash
> > performance-wise in my testing, but I like not having to take the i_lock
> > down where it could end up nested inside other locks.
> > 
> > With this, we reduce inode metadata updates across all 3 filesystems
> > down to roughly the frequency of the timestamp granularity, particularly
> > when it's not being queried (the vastly common case).
> > 
> > The pessimal workload here is 1 byte writes, and it helps that
> > significantly. Of course, that's not a real-world workload.
> > 
> > A tiobench-example.fio workload also shows some modest performance
> > gains, and I've gotten mails from the kernel test robot that show some
> > significant performance gains on some microbenchmarks (case-msync-mt in
> > the vm-scalability testsuite to be specific).
> > 
> > I'm happy to run other workloads if anyone can suggest them.
> > 
> > At this point, the patchset works and does what it's expected to do in
> > my own testing. It seems like it's at least a modest performance win
> > across all 3 major disk-based filesystems. It may also encourage others
> > to implement i_version as well since it reduces that 

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Thu,  2 Mar 2017 23:25:06 +0530
"Naveen N. Rao"  wrote:

> We indicate support for accepting sym+offset with kretprobes through a
> line in ftrace README. Parse the same to identify support and choose the
> appropriate format for kprobe_events.

Could you give us an example of this change here? :)
for example, comment of commit 613f050d68a8 .

I think the code is OK, but we need actual example of result.

Thanks,

> 
> Signed-off-by: Naveen N. Rao 
> ---
>  tools/perf/util/probe-event.c | 12 +---
>  tools/perf/util/probe-file.c  |  7 +++
>  tools/perf/util/probe-file.h  |  1 +
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 35f5b7b7715c..faf5789902f5 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct 
> probe_trace_event *tevs,
>   }
>  
>   for (i = 0; i < ntevs; i++) {
> - if (!tevs[i].point.address || tevs[i].point.retprobe)
> + if (!tevs[i].point.address)
> + continue;
> + if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
>   continue;
>   /* If we found a wrong one, mark it by NULL symbol */
>   if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct 
> perf_probe_event *pev)
>   return -EINVAL;
>   }
>  
> - if (pp->retprobe && !pp->function) {
> - semantic_error("Return probe requires an entry function.\n");
> - return -EINVAL;
> - }
> -
>   if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
>   semantic_error("Offset/Line/Lazy pattern can't be used with "
>  "return probe.\n");
> @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct 
> perf_probe_event *pev,
>   }
>  
>   /* Note that the symbols in the kmodule are not relocated */
> - if (!pev->uprobes && !pp->retprobe && !pev->target) {
> + if (!pev->uprobes && !pev->target &&
> + (!pp->retprobe || kretprobe_offset_is_supported())) {
>   reloc_sym = kernel_get_ref_reloc_sym();
>   if (!reloc_sym) {
>   pr_warning("Relocated base symbol is not found!\n");
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 8a219cd831b7..1542cd0d6799 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -879,6 +879,7 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  
>  enum ftrace_readme {
>   FTRACE_README_PROBE_TYPE_X = 0,
> + FTRACE_README_KRETPROBE_OFFSET,
>   FTRACE_README_END,
>  };
>  
> @@ -889,6 +890,7 @@ static struct {
>  #define DEFINE_TYPE(idx, pat)\
>   [idx] = {.pattern = pat, .avail = false}
>   DEFINE_TYPE(FTRACE_README_PROBE_TYPE_X, "*type: * x8/16/32/64,*"),
> + DEFINE_TYPE(FTRACE_README_KRETPROBE_OFFSET, "*place (kretprobe): *"),
>  };
>  
>  static bool scan_ftrace_readme(enum ftrace_readme type)
> @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
>  
>   return true;
>  }
> +
> +bool kretprobe_offset_is_supported(void)
> +{
> + return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82eff8a0..dbf95a00864a 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -65,6 +65,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct 
> probe_cache *pcache,
>   const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> +bool kretprobe_offset_is_supported(void);
>  #else/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt 
> __maybe_unused)
>  {
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu 

Re: [PATCH v4 2/3] perf: kretprobes: offset from reloc_sym if kernel supports it

[Masami Hiramatsu]

On Thu,  2 Mar 2017 23:25:06 +0530
"Naveen N. Rao"  wrote:

> We indicate support for accepting sym+offset with kretprobes through a
> line in ftrace README. Parse the same to identify support and choose the
> appropriate format for kprobe_events.

Could you give us an example of this change here? :)
for example, comment of commit 613f050d68a8 .

I think the code is OK, but we need actual example of result.

Thanks,

> 
> Signed-off-by: Naveen N. Rao 
> ---
>  tools/perf/util/probe-event.c | 12 +---
>  tools/perf/util/probe-file.c  |  7 +++
>  tools/perf/util/probe-file.h  |  1 +
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 35f5b7b7715c..faf5789902f5 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -757,7 +757,9 @@ post_process_kernel_probe_trace_events(struct 
> probe_trace_event *tevs,
>   }
>  
>   for (i = 0; i < ntevs; i++) {
> - if (!tevs[i].point.address || tevs[i].point.retprobe)
> + if (!tevs[i].point.address)
> + continue;
> + if (tevs[i].point.retprobe && !kretprobe_offset_is_supported())
>   continue;
>   /* If we found a wrong one, mark it by NULL symbol */
>   if (kprobe_warn_out_range(tevs[i].point.symbol,
> @@ -1528,11 +1530,6 @@ static int parse_perf_probe_point(char *arg, struct 
> perf_probe_event *pev)
>   return -EINVAL;
>   }
>  
> - if (pp->retprobe && !pp->function) {
> - semantic_error("Return probe requires an entry function.\n");
> - return -EINVAL;
> - }
> -
>   if ((pp->offset || pp->line || pp->lazy_line) && pp->retprobe) {
>   semantic_error("Offset/Line/Lazy pattern can't be used with "
>  "return probe.\n");
> @@ -2841,7 +2838,8 @@ static int find_probe_trace_events_from_map(struct 
> perf_probe_event *pev,
>   }
>  
>   /* Note that the symbols in the kmodule are not relocated */
> - if (!pev->uprobes && !pp->retprobe && !pev->target) {
> + if (!pev->uprobes && !pev->target &&
> + (!pp->retprobe || kretprobe_offset_is_supported())) {
>   reloc_sym = kernel_get_ref_reloc_sym();
>   if (!reloc_sym) {
>   pr_warning("Relocated base symbol is not found!\n");
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 8a219cd831b7..1542cd0d6799 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -879,6 +879,7 @@ int probe_cache__show_all_caches(struct strfilter *filter)
>  
>  enum ftrace_readme {
>   FTRACE_README_PROBE_TYPE_X = 0,
> + FTRACE_README_KRETPROBE_OFFSET,
>   FTRACE_README_END,
>  };
>  
> @@ -889,6 +890,7 @@ static struct {
>  #define DEFINE_TYPE(idx, pat)\
>   [idx] = {.pattern = pat, .avail = false}
>   DEFINE_TYPE(FTRACE_README_PROBE_TYPE_X, "*type: * x8/16/32/64,*"),
> + DEFINE_TYPE(FTRACE_README_KRETPROBE_OFFSET, "*place (kretprobe): *"),
>  };
>  
>  static bool scan_ftrace_readme(enum ftrace_readme type)
> @@ -939,3 +941,8 @@ bool probe_type_is_available(enum probe_type type)
>  
>   return true;
>  }
> +
> +bool kretprobe_offset_is_supported(void)
> +{
> + return scan_ftrace_readme(FTRACE_README_KRETPROBE_OFFSET);
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82eff8a0..dbf95a00864a 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -65,6 +65,7 @@ struct probe_cache_entry *probe_cache__find_by_name(struct 
> probe_cache *pcache,
>   const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> +bool kretprobe_offset_is_supported(void);
>  #else/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt 
> __maybe_unused)
>  {
> -- 
> 2.11.1
> 


-- 
Masami Hiramatsu 

Re: [RFC PATCH v1 29/30] fs: track whether the i_version has been queried with an i_state flag

[Jeff Layton]

On Sat, 2017-03-04 at 11:03 +1100, NeilBrown wrote:
> On Wed, Dec 21 2016, Jeff Layton wrote:
> 
> > @@ -2072,7 +2093,12 @@ inode_cmp_iversion(const struct inode *inode, const 
> > u64 old)
> >  static inline bool
> >  inode_iversion_need_inc(struct inode *inode)
> >  {
> > -   return true;
> > +   bool ret;
> > +
> > +   spin_lock(>i_lock);
> > +   ret = inode->i_state & I_VERS_BUMP;
> > +   spin_unlock(>i_lock);
> > +   return ret;
> >  }
> >  
> 
> I know this code gets removed, so this isn't really important.
> By why do you take the spinlock here?  What are you racing again?
> 
> Thanks,
> NeilBrown

I think I was worried about I_VERS_BUMP being set or cleared during an
increment or query. It is quite possible that that spinlock is not
necessary.
-- 
Jeff Layton 

Re: [RFC PATCH v1 29/30] fs: track whether the i_version has been queried with an i_state flag

[Jeff Layton]

On Sat, 2017-03-04 at 11:03 +1100, NeilBrown wrote:
> On Wed, Dec 21 2016, Jeff Layton wrote:
> 
> > @@ -2072,7 +2093,12 @@ inode_cmp_iversion(const struct inode *inode, const 
> > u64 old)
> >  static inline bool
> >  inode_iversion_need_inc(struct inode *inode)
> >  {
> > -   return true;
> > +   bool ret;
> > +
> > +   spin_lock(>i_lock);
> > +   ret = inode->i_state & I_VERS_BUMP;
> > +   spin_unlock(>i_lock);
> > +   return ret;
> >  }
> >  
> 
> I know this code gets removed, so this isn't really important.
> By why do you take the spinlock here?  What are you racing again?
> 
> Thanks,
> NeilBrown

I think I was worried about I_VERS_BUMP being set or cleared during an
increment or query. It is quite possible that that spinlock is not
necessary.
-- 
Jeff Layton