Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 2/7] usb: mtu3: remove repeated setting of gadget state
From: Chunfeng Yun @ 2018-05-10  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <6b82c11302a81368bd6936424d081ede3340b431.1525930818.git.chunfeng.yun@mediatek.com>

The usb_add_gadget_udc() will set the gadget state as
USB_STATE_NOTATTACHED, so we needn't set it again.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_gadget.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_gadget.c b/drivers/usb/mtu3/mtu3_gadget.c
index f05f10f..de0de01 100644
--- a/drivers/usb/mtu3/mtu3_gadget.c
+++ b/drivers/usb/mtu3/mtu3_gadget.c
@@ -660,14 +660,10 @@ int mtu3_gadget_setup(struct mtu3 *mtu)
 	mtu3_gadget_init_eps(mtu);
 
 	ret = usb_add_gadget_udc(mtu->dev, &mtu->g);
-	if (ret) {
+	if (ret)
 		dev_err(mtu->dev, "failed to register udc\n");
-		return ret;
-	}
 
-	usb_gadget_set_state(&mtu->g, USB_STATE_NOTATTACHED);
-
-	return 0;
+	return ret;
 }
 
 void mtu3_gadget_cleanup(struct mtu3 *mtu)
-- 
1.9.1

^ permalink raw reply related

* [PATCH v2 1/7] usb: mtu3: avoid TX data length truncated in SS/SSP mode
From: Chunfeng Yun @ 2018-05-10  5:47 UTC (permalink / raw)
  To: linux-arm-kernel

The variable of 'count' is declared as u8, this will cause an issue
due to value truncated when works in SS or SSP mode and data length
is greater than 255, so change it as u32.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_gadget_ep0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c b/drivers/usb/mtu3/mtu3_gadget_ep0.c
index ebdcf7a..d67b540 100644
--- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
+++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
@@ -546,7 +546,7 @@ static void ep0_tx_state(struct mtu3 *mtu)
 	struct usb_request *req;
 	u32 csr;
 	u8 *src;
-	u8 count;
+	u32 count;
 	u32 maxp;
 
 	dev_dbg(mtu->dev, "%s\n", __func__);
-- 
1.9.1

^ permalink raw reply related

* [GIT PULL] ARM: uniphier: fixes for v4.17
From: Olof Johansson @ 2018-05-10  4:23 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAK7LNARwR+9RhGTSJDxZY+NpLAzJNYgTV=v5GOR=nuT5SWqQFA@mail.gmail.com>

On Wed, May 9, 2018 at 9:18 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Arnd, Olof,
>
>
> 2018-04-29 0:47 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>> Hi Arnd, Olof,
>>
>> Please pull some fixes of ARM UniPhier DT.
>
>
> I am guessing why this has not been pulled yet.
> Right, I was late - you had already sent the fixes pull request to Linus.
>
> Do you have a chance to send another fixes PR for 4.17?

Hi,

Yes, I'm about to do a pass of merges and this will be part of that.
Arnd is on leave and I haven't spun up merging yet, I should get to it
tonight or tomorrow at the latest.


-Olof

^ permalink raw reply

* [GIT PULL] ARM: uniphier: fixes for v4.17
From: Masahiro Yamada @ 2018-05-10  4:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAK7LNASuFh4wcSJ7HTW-+ZQCjZWt0QtjrOQQogoqzbM2W9n+nA@mail.gmail.com>

Hi Arnd, Olof,


2018-04-29 0:47 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Arnd, Olof,
>
> Please pull some fixes of ARM UniPhier DT.


I am guessing why this has not been pulled yet.
Right, I was late - you had already sent the fixes pull request to Linus.

Do you have a chance to send another fixes PR for 4.17?






>
>
> The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
>
>   Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-uniphier.git
> tags/uniphier-fixes-v4.17
>
> for you to fetch changes up to 8b82b66e68b32dd8e22a04db6a0494bfdd65156f:
>
>   arm64: dts: uniphier: stabilize ethernet of LD20 reference board
> (2018-04-29 00:33:42 +0900)
>
> ----------------------------------------------------------------
> UniPhier ARM SoC fixes for v4.17
>
> - Fix input delay parameter of eMMC PHY
> - Weaken drive-strength of ethernet PHY pins of LD20 reference board
>
> ----------------------------------------------------------------
> Masahiro Yamada (2):
>       arm64: dts: uniphier: fix input delay value for legacy mode of eMMC
>       arm64: dts: uniphier: stabilize ethernet of LD20 reference board
>
>  arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi    | 2 +-
>  arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts | 8 ++++++++
>  arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi    | 2 +-
>  arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi    | 2 +-
>  4 files changed, 11 insertions(+), 3 deletions(-)
>
>
>
>
> --
> Best Regards
> Masahiro Yamada



-- 
Best Regards
Masahiro Yamada

^ permalink raw reply

* [PATCH RESEND v2 2/2] perf cs-etm: Remove redundant space
From: Leo Yan @ 2018-05-10  4:02 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1525924920-4381-1-git-send-email-leo.yan@linaro.org>

There have two spaces ahead function name cs_etm__set_pid_tid_cpu(), so
remove one space and correct indentation.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/util/cs-etm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 69f21de..822ba91 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -611,8 +611,8 @@ cs_etm__get_trace(struct cs_etm_buffer *buff, struct cs_etm_queue *etmq)
 	return buff->len;
 }
 
-static void  cs_etm__set_pid_tid_cpu(struct cs_etm_auxtrace *etm,
-				     struct auxtrace_queue *queue)
+static void cs_etm__set_pid_tid_cpu(struct cs_etm_auxtrace *etm,
+				    struct auxtrace_queue *queue)
 {
 	struct cs_etm_queue *etmq = queue->priv;
 
-- 
2.7.4

^ permalink raw reply related

* [PATCH RESEND v2 1/2] perf cs-etm: Support unknown_thread in cs_etm_auxtrace
From: Leo Yan @ 2018-05-10  4:01 UTC (permalink / raw)
  To: linux-arm-kernel

CoreSight doesn't allocate thread structure for unknown_thread in etm
auxtrace, so unknown_thread is NULL pointer.  If the perf data doesn't
contain valid tid and then cs_etm__mem_access() uses unknown_thread
instead as thread handler, this results in segmentation fault when
thread__find_addr_map() accesses thread handler.

This commit creates new thread data which is used by unknown_thread, so
CoreSight tracing can roll back to use unknown_thread if perf data
doesn't include valid thread info.  This commit also releases thread
data for initialization failure case and for normal auxtrace free flow.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/util/cs-etm.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 6533b1a..69f21de 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -239,6 +239,7 @@ static void cs_etm__free(struct perf_session *session)
 	for (i = 0; i < aux->num_cpu; i++)
 		zfree(&aux->metadata[i]);
 
+	thread__zput(aux->unknown_thread);
 	zfree(&aux->metadata);
 	zfree(&aux);
 }
@@ -1355,6 +1356,23 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
 	etm->auxtrace.free = cs_etm__free;
 	session->auxtrace = &etm->auxtrace;
 
+	etm->unknown_thread = thread__new(999999999, 999999999);
+	if (!etm->unknown_thread)
+		goto err_free_queues;
+
+	/*
+	 * Initialize list node so that at thread__zput() we can avoid
+	 * segmentation fault at list_del_init().
+	 */
+	INIT_LIST_HEAD(&etm->unknown_thread->node);
+
+	err = thread__set_comm(etm->unknown_thread, "unknown", 0);
+	if (err)
+		goto err_delete_thread;
+
+	if (thread__init_map_groups(etm->unknown_thread, etm->machine))
+		goto err_delete_thread;
+
 	if (dump_trace) {
 		cs_etm__print_auxtrace_info(auxtrace_info->priv, num_cpu);
 		return 0;
@@ -1369,16 +1387,18 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
 
 	err = cs_etm__synth_events(etm, session);
 	if (err)
-		goto err_free_queues;
+		goto err_delete_thread;
 
 	err = auxtrace_queues__process_index(&etm->queues, session);
 	if (err)
-		goto err_free_queues;
+		goto err_delete_thread;
 
 	etm->data_queued = etm->queues.populated;
 
 	return 0;
 
+err_delete_thread:
+	thread__zput(etm->unknown_thread);
 err_free_queues:
 	auxtrace_queues__free(&etm->queues);
 	session->auxtrace = NULL;
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 1/2] perf cs-etm: Support unknown_thread in cs_etm_auxtrace
From: Leo Yan @ 2018-05-10  3:56 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509155150.GC25559@xps15>

On Wed, May 09, 2018 at 09:51:50AM -0600, Mathieu Poirier wrote:
> On Wed, May 09, 2018 at 12:15:11PM +0800, Leo Yan wrote:
> > CoreSight doesn't allocate thread structure for unknown_thread in etm
> > auxtrace, so unknown_thread is NULL pointer.  If the perf data doesn't
> > contain valid tid and then cs_etm__mem_access() uses unknown_thread
> > instead as thread handler, this results in segmentation fault when
> > thread__find_addr_map() accesses thread handler.
> > 
> > This commit creates new thread data which is used by unknown_thread, so
> > CoreSight tracing can roll back to use unknown_thread if perf data
> > doesn't include valid thread info.  This commit also releases thread
> > data for initialization failure case and for normal auxtrace free flow.
> 
> Hi Leo,
> 
> Arnaldo maintains the perf tools and he receives dozens of patches every day.
> If his name is not the first one to appear on the "To:" list, it is unlikely
> that his email filters will catch your patches.

Thanks for reminding, Mathieu.  Will resend patches for this.

Thanks,
Leo Yan

^ permalink raw reply

* [PATCH] bpf, arm32: Correct check_imm24
From: Wang YanQing @ 2018-05-10  3:20 UTC (permalink / raw)
  To: linux-arm-kernel

imm24 is signed, so the right range is:
[-(2<<(24 - 1)), (2<<(24 - 1)) - 1]

Note:this patch also fix a typo.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 arch/arm/net/bpf_jit_32.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index c0b4124..19c6d77 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -84,7 +84,7 @@
  *
  * 1. First argument is passed using the arm 32bit registers and rest of the
  * arguments are passed on stack scratch space.
- * 2. First callee-saved arugument is mapped to arm 32 bit registers and rest
+ * 2. First callee-saved argument is mapped to arm 32 bit registers and rest
  * arguments are mapped to scratch space on stack.
  * 3. We need two 64 bit temp registers to do complex operations on eBPF
  * registers.
@@ -1198,15 +1198,14 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 	u8 rd, rt, rm, rn;
 	s32 jmp_offset;
 
-#define check_imm(bits, imm) do {				\
-	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
-	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
-		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
-			i, imm, imm);				\
+#define check_imm_range(min, max, imm) do {			\
+	if (imm < min || imm > max) {				\
+		pr_info("[%2d] imm=%d is out of range\n",	\
+			i, imm);				\
 		return -EINVAL;					\
 	}							\
 } while (0)
-#define check_imm24(imm) check_imm(24, imm)
+#define check_imm24(imm) check_imm_range(-16777216, 16777215, imm)
 
 	switch (code) {
 	/* ALU operations */
-- 
1.8.5.6.2.g3d8a54e.dirty

^ permalink raw reply related

* [PATCH] bpf, arm32: Fix inconsistent naming about emit_a32_lsr_r64|emit_a32_lsr_i64
From: Wang YanQing @ 2018-05-10  3:14 UTC (permalink / raw)
  To: linux-arm-kernel

The reasonable names for emit_a32_lsr_r64|emit_a32_lsr_i64 are
emit_a32_rsh_r64|emit_a32_rsh_i64.

This patch also correct a wrong comment.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 arch/arm/net/bpf_jit_32.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 82689b9..c0b4124 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -708,7 +708,7 @@ static inline void emit_a32_arsh_r64(const u8 dst[], const u8 src[], bool dstk,
 }
 
 /* dst = dst >> src */
-static inline void emit_a32_lsr_r64(const u8 dst[], const u8 src[], bool dstk,
+static inline void emit_a32_rsh_r64(const u8 dst[], const u8 src[], bool dstk,
 				     bool sstk, struct jit_ctx *ctx) {
 	const u8 *tmp = bpf2a32[TMP_REG_1];
 	const u8 *tmp2 = bpf2a32[TMP_REG_2];
@@ -724,7 +724,7 @@ static inline void emit_a32_lsr_r64(const u8 dst[], const u8 src[], bool dstk,
 		emit(ARM_LDR_I(rm, ARM_SP, STACK_VAR(dst_hi)), ctx);
 	}
 
-	/* Do LSH operation */
+	/* Do RSH operation */
 	emit(ARM_RSB_I(ARM_IP, rt, 32), ctx);
 	emit(ARM_SUBS_I(tmp2[0], rt, 32), ctx);
 	emit(ARM_MOV_SR(ARM_LR, rd, SRTYPE_LSR, rt), ctx);
@@ -774,7 +774,7 @@ static inline void emit_a32_lsh_i64(const u8 dst[], bool dstk,
 }
 
 /* dst = dst >> val */
-static inline void emit_a32_lsr_i64(const u8 dst[], bool dstk,
+static inline void emit_a32_rsh_i64(const u8 dst[], bool dstk,
 				    const u32 val, struct jit_ctx *ctx) {
 	const u8 *tmp = bpf2a32[TMP_REG_1];
 	const u8 *tmp2 = bpf2a32[TMP_REG_2];
@@ -1330,7 +1330,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 	case BPF_ALU64 | BPF_RSH | BPF_K:
 		if (unlikely(imm > 63))
 			return -EINVAL;
-		emit_a32_lsr_i64(dst, dstk, imm, ctx);
+		emit_a32_rsh_i64(dst, dstk, imm, ctx);
 		break;
 	/* dst = dst << src */
 	case BPF_ALU64 | BPF_LSH | BPF_X:
@@ -1338,7 +1338,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 		break;
 	/* dst = dst >> src */
 	case BPF_ALU64 | BPF_RSH | BPF_X:
-		emit_a32_lsr_r64(dst, src, dstk, sstk, ctx);
+		emit_a32_rsh_r64(dst, src, dstk, sstk, ctx);
 		break;
 	/* dst = dst >> src (signed) */
 	case BPF_ALU64 | BPF_ARSH | BPF_X:
-- 
1.8.5.6.2.g3d8a54e.dirty

^ permalink raw reply related

* [RESEND PATCH 1/5] usb: mtu3: avoid TX data length truncated in SS/SSP mode
From: Chunfeng Yun @ 2018-05-10  3:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1525918606.32173.9.camel@mhfsdcap03>

Hi, Greg
On Thu, 2018-05-10 at 10:16 +0800, Chunfeng Yun wrote:
> Hi, Greg
> 
>    Could you please pick up the series of patches, thanks a lot
Please ignore it, I find a problem in [RESEND PATCH 4/5], and need send
a new version.

Very sorry

>  
> On Sat, 2018-05-05 at 10:21 +0800, Chunfeng Yun wrote:
> > The variable of 'count' is declared as u8, this will cause an issue
> > due to value truncated when works in SS or SSP mode and data length
> > is greater than 255, so change it as u32.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/usb/mtu3/mtu3_gadget_ep0.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > index ebdcf7a..d67b540 100644
> > --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > @@ -546,7 +546,7 @@ static void ep0_tx_state(struct mtu3 *mtu)
> >  	struct usb_request *req;
> >  	u32 csr;
> >  	u8 *src;
> > -	u8 count;
> > +	u32 count;
> >  	u32 maxp;
> >  
> >  	dev_dbg(mtu->dev, "%s\n", __func__);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [xlnx:xlnx_rebase_v4.14 74/917] drivers//fpga/zynqmp-fpga.c:79:2: error: implicit declaration of function '__flush_cache_user_range'; did you mean 'flush_cache_user_range'?
From: kbuild test robot @ 2018-05-10  3:07 UTC (permalink / raw)
  To: linux-arm-kernel

tree:   https://github.com/Xilinx/linux-xlnx xlnx_rebase_v4.14
head:   a24ae3acb778c948234fa8f227bacd818034055d
commit: d91b9e49c2c2663091e2d71a136ad2db919a91b3 [74/917] fpga manager: Adding FPGA Manager support for Xilinx zynqmp
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout d91b9e49c2c2663091e2d71a136ad2db919a91b3
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers//fpga/zynqmp-fpga.c: In function 'zynqmp_fpga_ops_write':
>> drivers//fpga/zynqmp-fpga.c:79:2: error: implicit declaration of function '__flush_cache_user_range'; did you mean 'flush_cache_user_range'? [-Werror=implicit-function-declaration]
     __flush_cache_user_range((unsigned long)kbuf,
     ^~~~~~~~~~~~~~~~~~~~~~~~
     flush_cache_user_range
   cc1: some warnings being treated as errors

vim +79 drivers//fpga/zynqmp-fpga.c

    45	
    46	static int zynqmp_fpga_ops_write(struct fpga_manager *mgr,
    47						const char *buf, size_t size)
    48	{
    49		struct zynqmp_fpga_priv *priv;
    50		char *kbuf;
    51		size_t dma_size;
    52		dma_addr_t dma_addr;
    53		u32 transfer_length;
    54		int ret;
    55		const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
    56	
    57		if (!eemi_ops || !eemi_ops->fpga_load)
    58			return -ENXIO;
    59	
    60		priv = mgr->priv;
    61	
    62		if (mgr->flags & IXR_FPGA_ENCRYPTION_EN)
    63			dma_size = size + ENCRYPTED_KEY_LEN + ENCRYPTED_IV_LEN;
    64		else
    65			dma_size = size;
    66	
    67		kbuf = dma_alloc_coherent(priv->dev, dma_size, &dma_addr, GFP_KERNEL);
    68		if (!kbuf)
    69			return -ENOMEM;
    70	
    71		memcpy(kbuf, buf, size);
    72	
    73		if (mgr->flags & IXR_FPGA_ENCRYPTION_EN) {
    74			memcpy(kbuf + size, mgr->key, ENCRYPTED_KEY_LEN);
    75			memcpy(kbuf + size + ENCRYPTED_KEY_LEN, mgr->iv,
    76							ENCRYPTED_IV_LEN);
    77		}
    78	
  > 79		__flush_cache_user_range((unsigned long)kbuf,
    80					 (unsigned long)kbuf + dma_size);
    81	
    82		/**
    83		 * Translate size from bytes to number of 32bit words that
    84		 * the DMA should write to the PCAP interface
    85		 */
    86		if (size & 3)
    87			transfer_length = (size >> 2) + 1;
    88		else
    89			transfer_length = size >> 2;
    90	
    91		ret = eemi_ops->fpga_load(dma_addr, transfer_length, mgr->flags);
    92	
    93		dma_free_coherent(priv->dev, dma_size, kbuf, dma_addr);
    94	
    95		return ret;
    96	}
    97	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 64038 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180510/f2a9f042/attachment-0001.gz>

^ permalink raw reply

* [PATCH] soc: mediatek: remove unneeded semicolon
From: sean.wang at mediatek.com @ 2018-05-10  2:57 UTC (permalink / raw)
  To: linux-arm-kernel

From: Sean Wang <sean.wang@mediatek.com>

Fix up drivers/soc/mediatek/mtk-scpsys.c:255:2-3: Unneeded semicolon
accidently being added in commit f9e2f65dd561 ("soc: mediatek: add a
fixed wait for SRAM stable").

Fixes: f9e2f65dd561 ("soc: mediatek: add a fixed wait for SRAM stable")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/soc/mediatek/mtk-scpsys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index 2080c8f..128e3dd 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -252,7 +252,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 					 MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
 		if (ret < 0)
 			goto err_pwr_ack;
-	};
+	}
 
 	if (scpd->data->bus_prot_mask) {
 		ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
-- 
2.7.4

^ permalink raw reply related

* [PATCH] PCI: Check whether bridges allow access to extended config space
From: Frederick Lawler @ 2018-05-10  2:44 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <5AF2E91E.3030804@kontron.com>

Hi,

Gilles Buloz wrote:
> Here are full dmesg + lspci -vv + ... for the following cases :
> - 4.17-rc1_probe_failed_without_pcieaspmoff.txt : I get a broken PCI config at most boots, without pcie_aspm=off
> - 4.17-rc1_probe_ok_without_pcieaspmoff.txt : I get a correct PCI config at some boots, even without pcie_aspm=off
> - 4.17-rc1_probe_always_ok_with_pcieaspmoff.txt : I get a correct PCI config at all boots when using pcie_aspm=off
> - 4.1.35_probe_always_ok_without_pcieaspmoff.txt : I get a correct PCI config at all boots when using kernel 4.1.35-rt41 (from NXP
> Yocto BSP), even without pcie_aspm=off
> 
> I noticed these strange things :
> - with 4.17-rc1, without pcie_aspm=off, the kernel is hanging during ~1 second :
>     - before line "ASPM: Could not configure common clock" when PCI config is OK
>     - or before line "bridge configuration invalid ([bus ff-ff]), reconfiguring" when PCI config is NOT OK
>       (the FFs in "[bus ff-ff]" seems side effects of read returning ~0 because device is off)
> - no such hang when pcie_aspm=off is used
> - applying reverse patch for "PCI/ASPM: Don't warn if already in common clock mode"
>     (commit 04875177dbe034055f23960981ecf6fb8ea1d638) does not give more message

I submitted this as a bug here:
https://bugzilla.kernel.org/show_bug.cgi?id=199671

Thanks,
Fred

^ permalink raw reply

* [PATCH 2/4] pid: Export find_task_by_vpid for use in external modules
From: Eric W. Biederman @ 2018-05-10  2:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509152505.GA25559@xps15>

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On Tue, May 08, 2018 at 11:59:38PM -0500, Eric W. Biederman wrote:
>> Kim Phillips <kim.phillips@arm.com> writes:
>> 
>> > This patch is in the context of allowing the Coresight h/w
>> > trace driver suite to be loaded as modules.  Coresight uses
>> > find_task_by_vpid when running in direct capture mode (via sysfs)
>> > when getting/setting the context ID comparator to trigger on
>> > (/sys/bus/coresight/devices/<x>.etm/ctxid_pid).
>> 
>> Aside from my objection about how bad an interface a pid in sysfs is.
>> The implementation of coresight_vpid_to_pid is horrible.
>> 
>> The code should be just:
>> 
>> static inline pid_t coresight_vpid_to_pid(pid_t vpid)
>> {
>> 	rcu_read_lock();
>>         pid = pid_nr(find_vpid(vpid));
>> 	rcu_read_unlock();
>> 
>> 	return pid;
>> }
>> Which takes find_task_by_vpid out of the picture.
>
> Many thanks for pointing out the right way to do this.  When Chunyan added
> this feature she broadly published her work and find_task_by_vpid() is the
> function she was asked to used.

Clearly no one was thinking through the implications of a sysfs file
which does not have pid namespace support on namespacing.  I am quite
upset at this mess of an API.  It is not a maintainable way to do things.

>> But reading further I am seeing code writing a pid to hardware.  That is
>> broken.  That is a layering violation of the first order.  Giving
>> implementation details like that to hardware.
>
> This is how the feature works - as Robin pointed out tracers are designed to
> match pid values with the CPU's contextID register.  The input value has no
> other effect than triggering trace collection, which has absolutely no baring on
> the CPU.

So please tell me how we make the tracer pid namespace aware.  Or is it
guaranteed that only the global root user will use this functionality?

As you are taking a vpid it looks like users with lesser privileges are
able to request this.   From the other reply it appears this is the
value the tracer returns to put in logs.  Perhaps I missed it but I
didn't see anything that translated from the global pid to something
else.  Which would make using this feature in a pid namespace confusing
and a problematic information leak if I have understood what has been
said so far.

Eric

^ permalink raw reply

* [RESEND PATCH 1/5] usb: mtu3: avoid TX data length truncated in SS/SSP mode
From: Chunfeng Yun @ 2018-05-10  2:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3b0c7ca576fee8220d63c9914483b70b0fbc8783.1525486663.git.chunfeng.yun@mediatek.com>

Hi, Greg

   Could you please pick up the series of patches, thanks a lot
 
On Sat, 2018-05-05 at 10:21 +0800, Chunfeng Yun wrote:
> The variable of 'count' is declared as u8, this will cause an issue
> due to value truncated when works in SS or SSP mode and data length
> is greater than 255, so change it as u32.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/usb/mtu3/mtu3_gadget_ep0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> index ebdcf7a..d67b540 100644
> --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> @@ -546,7 +546,7 @@ static void ep0_tx_state(struct mtu3 *mtu)
>  	struct usb_request *req;
>  	u32 csr;
>  	u8 *src;
> -	u8 count;
> +	u32 count;
>  	u32 maxp;
>  
>  	dev_dbg(mtu->dev, "%s\n", __func__);

^ permalink raw reply

* [PATCH 3/3] ARM: dts: imx6q-gk802: Do not hardcode input codes
From: Fabio Estevam @ 2018-05-10  2:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1525918089-26242-1-git-send-email-festevam@gmail.com>

From: Fabio Estevam <fabio.estevam@nxp.com>

Instead of hardcoding the input codes we can use the symbol name for
better readability.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/boot/dts/imx6q-gk802.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6q-gk802.dts b/arch/arm/boot/dts/imx6q-gk802.dts
index 0be3756..84d3540 100644
--- a/arch/arm/boot/dts/imx6q-gk802.dts
+++ b/arch/arm/boot/dts/imx6q-gk802.dts
@@ -8,6 +8,7 @@
 
 /dts-v1/;
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 #include "imx6q.dtsi"
 
 / {
@@ -43,7 +44,7 @@
 		recovery-button {
 			label = "recovery";
 			gpios = <&gpio3 16 1>;
-			linux,code = <0x198>; /* KEY_RESTART */
+			linux,code = <KEY_RESTART>;
 			wakeup-source;
 		};
 	};
-- 
2.7.4

^ permalink raw reply related

* [PATCH 2/3] ARM: dts: imx53-smd: Do not hardcode input codes
From: Fabio Estevam @ 2018-05-10  2:08 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1525918089-26242-1-git-send-email-festevam@gmail.com>

From: Fabio Estevam <fabio.estevam@nxp.com>

Instead of hardcoding the input codes we can use the symbol name for
better readability.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/boot/dts/imx53-smd.dts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-smd.dts b/arch/arm/boot/dts/imx53-smd.dts
index fd03012..705af5e 100644
--- a/arch/arm/boot/dts/imx53-smd.dts
+++ b/arch/arm/boot/dts/imx53-smd.dts
@@ -11,6 +11,7 @@
  */
 
 /dts-v1/;
+#include <dt-bindings/input/input.h>
 #include "imx53.dtsi"
 
 / {
@@ -27,13 +28,13 @@
 		volume-up {
 			label = "Volume Up";
 			gpios = <&gpio2 14 0>;
-			linux,code = <115>; /* KEY_VOLUMEUP */
+			linux,code = <KEY_VOLUMEUP>;
 		};
 
 		volume-down {
 			label = "Volume Down";
 			gpios = <&gpio2 15 0>;
-			linux,code = <114>; /* KEY_VOLUMEDOWN */
+			linux,code = <KEY_VOLUMEDOWN>;
 		};
 	};
 };
-- 
2.7.4

^ permalink raw reply related

* [PATCH 1/3] ARM: dts: imx53-ard: Do not hardcode input codes
From: Fabio Estevam @ 2018-05-10  2:08 UTC (permalink / raw)
  To: linux-arm-kernel

From: Fabio Estevam <fabio.estevam@nxp.com>

Instead of hardcoding the input codes we can use the symbol name for
better readability.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/boot/dts/imx53-ard.dts | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-ard.dts b/arch/arm/boot/dts/imx53-ard.dts
index 80fc007..117bd00 100644
--- a/arch/arm/boot/dts/imx53-ard.dts
+++ b/arch/arm/boot/dts/imx53-ard.dts
@@ -11,6 +11,7 @@
  */
 
 /dts-v1/;
+#include <dt-bindings/input/input.h>
 #include "imx53.dtsi"
 
 / {
@@ -68,34 +69,34 @@
 		home {
 			label = "Home";
 			gpios = <&gpio5 10 0>;
-			linux,code = <102>; /* KEY_HOME */
+			linux,code = <KEY_HOME>;
 			wakeup-source;
 		};
 
 		back {
 			label = "Back";
 			gpios = <&gpio5 11 0>;
-			linux,code = <158>; /* KEY_BACK */
+			linux,code = <KEY_BACK>;
 			wakeup-source;
 		};
 
 		program {
 			label = "Program";
 			gpios = <&gpio5 12 0>;
-			linux,code = <362>; /* KEY_PROGRAM */
+			linux,code = <KEY_PROGRAM >;
 			wakeup-source;
 		};
 
 		volume-up {
 			label = "Volume Up";
 			gpios = <&gpio5 13 0>;
-			linux,code = <115>; /* KEY_VOLUMEUP */
+			linux,code = <KEY_VOLUMEUP>;
 		};
 
 		volume-down {
 			label = "Volume Down";
 			gpios = <&gpio4 0 0>;
-			linux,code = <114>; /* KEY_VOLUMEDOWN */
+			linux,code = <KEY_VOLUMEDOWN>;
 		};
 	};
 };
-- 
2.7.4

^ permalink raw reply related

* [PATCH] clk: imx6ull: use OSC clock during AXI rate change
From: Jacky Bai @ 2018-05-10  1:53 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <ae5b239a57bd7e3f3e62cc23a1c3788e@agner.ch>

> Subject: Re: [PATCH] clk: imx6ull: use OSC clock during AXI rate change
> 
> On 09.05.2018 03:26, Jacky Bai wrote:
> >> Subject: Re: [PATCH] clk: imx6ull: use OSC clock during AXI rate
> >> change
> >>
> >> Quoting Stefan Agner (2018-05-08 06:20:03)
> >> > On 08.05.2018 09:32, Jacky Bai wrote:
> >> > >
> >> > > I have tried two 6ULL board, I don't meet such issue. System can
> >> > > boot up successfully with commit 6f9575e55632 included.
> >> > > Anyway, the change in this patch is ok for me. it is no harm to
> >> > > the BUS clock change flow.
> >> >
> >> > Hm, that is interesting, maybe it is because we use a different SKU?
> >> > Or maybe bootloader?
> >> >
> >> > I tested here with a 800 MHz clocked variant on a Colibri iMX6ULL
> >> > using U-Boot 2016.11.
> >> >
> >>
> >> I'll throw this into fixes today because it fixes something to be
> >> useful. It's still interesting to see what's different between the two setups
> though.
> >
> > Thanks.
> >
> > I will find some 800MHz parts and have a try with different bootloader later.
> > Will let you know, when I have some results.
> >
> 
> FWIW, I tried two SKUs here:
> MCIMX6Z2DVM05AA (528Mhz)
> MCIMX6Z2CVM08AA (800MHz)
> 

 MCIMX6Z or  MCIMX6Y ? I think i.MX6ULL should be 6Y.

Jacky

> Both run at 396MHz in U-Boot, and both freeze and the same location.
> 
> --
> Stefan

^ permalink raw reply

* [PATCH] ARM: DTS: imx53: Add support for imx53 HSC/DDC boards from K+P
From: Fabio Estevam @ 2018-05-10  1:45 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509153428.1440-1-lukma@denx.de>

On Wed, May 9, 2018 at 12:34 PM, Lukasz Majewski <lukma@denx.de> wrote:

> +               button at 1 {
> +                       label = "Kaltstart";
> +                       linux,code = <64>; /* KEY_F6 */

If you include <dt-bindings/input/input.h> then you can simply do:

 linux,code = <KEY_F6 >;

> +                       gpios = <&gpio2 26 GPIO_ACTIVE_HIGH>;
> +               };
> +               button at 2 {
> +                       label = "PowerFailInterrupt";
> +                       linux,code = <65>; /* KEY_F7 */

Same here.

^ permalink raw reply

* [PATCH] ARM: DTS: imx53: Add support for imx53 HSC/DDC boards from K+P
From: Fabio Estevam @ 2018-05-10  1:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509153428.1440-1-lukma@denx.de>

Hi Lukasz,

On Wed, May 9, 2018 at 12:34 PM, Lukasz Majewski <lukma@denx.de> wrote:

> +&iomuxc {
> +       imx53-kp-ddc {

No need for keeping this imx53-kp-ddc.

> diff --git a/arch/arm/boot/dts/imx53-kp-hsc.dts b/arch/arm/boot/dts/imx53-kp-hsc.dts
> new file mode 100644
> index 000000000000..fff358395c9d
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx53-kp-hsc.dts
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2018
> + * Lukasz Majewski, DENX Software Engineering, lukma at denx.de
> + */
> +
> +/dts-v1/;
> +#include "imx53-kp.dtsi"
> +
> +/ {
> +       model = "K+P imx53 HSC";
> +       compatible = "kiebackpeter,imx53-hsc", "fsl,imx53";
> +

No need for this blank line.

> +};
> +
> +&fec {
> +       status = "okay";

We usually put the status in the last line.

> +       gpio_buttons {
> +               compatible = "gpio-keys";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pinctrl_gpiobuttons>;
> +
> +               button at 1 {

You pass @1 without a reg property. This triggers a warning when
building with W=1.

You could remove the @1.

Please make sure this patch does not introduce any W=1 dtc warning.

> +&iomuxc {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_kp_common>;
> +
> +       imx53-kp-common {

No need for this  imx53-kp-common

> +&uart4 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_uart4>;
> +

No need for this blank line.

^ permalink raw reply

* [PATCH v4 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
From: Kim Phillips @ 2018-05-10  1:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <bb3e3c6f-f4df-a182-c947-10c77c998cd1@arm.com>

On Fri, 4 May 2018 18:10:44 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> Hi Kim,

Hi Robin,

> On 04/05/18 01:30, Kim Phillips wrote:
> > On Tue, 1 May 2018 12:54:05 +0100
> > Will Deacon <will.deacon@arm.com> wrote:
> >> On Fri, Apr 27, 2018 at 11:56:25AM -0500, Kim Phillips wrote:
> >>> On Fri, 27 Apr 2018 17:09:14 +0100
> >>> Will Deacon <will.deacon@arm.com> wrote:
> >>>> On Fri, Apr 27, 2018 at 10:46:29AM -0500, Kim Phillips wrote:
> >>>>> On Fri, 27 Apr 2018 15:37:20 +0100
> >>>>> Will Deacon <will.deacon@arm.com> wrote:
> >>>>>> For anything under drivers/perf/, I'd prefer not to have these prints
> >>>>>> and instead see efforts to improve error reporting via the perf system
> >>>>>> call interface.
> >>>>>
> >>>>> We'd all prefer that, and for all PMU drivers, why should ones under
> >>>>> drivers/perf be treated differently?
> >>>>
> >>>> Because they're the ones I maintain...
> >>>
> >>> You represent a minority on your opinion on this matter though.
> >>
> >> I'm not sure that's true
> > 
> > I haven't seen another arch/PMU driver maintainer actively oppose it
> > other than you (and Mark).
> > 
> >> -- you tend to be the only one raising this issue;
> > 
> > If you're basing that on list activity, I wouldn't, since most Arm perf
> > users don't subscribe to LKML.  I'm trying to represent current and
> > future Arm perf users that shouldn't be expected to hang out here on
> > LKML and review PMU drivers.  Couldn't tell you why PMU driver authors
> > themselves don't chime in, but will note that some drivers in
> > drivers/perf do print things from their event_init functions.
> 
> As a PMU driver author who *has* already invested not-inconsiderable 
> time and effort in trying to explain to you the technical issues from a 
> non-maintainer perspective (and is about to do so again), I can't help 
> but feel rather slighted by that comment.

Very sorry!  You're quite right.  I believe I had considered you as a
kernel developer first, and was trying to keep LKML activity affined to
LKML, vs. other activity on other mailing lists.

> > Also, you have been cc'd on off-list email threads where SPE users had
> > to debug the SPE driver's event_init() manually.  There are other SPE
> > users I know that have had to go through the same painstaking process of
> > debugging the SPE driver.  Last but not least, there's me, and I'd sure
> > appreciate more verbose PMU drivers, based on my real-life performance
> > monitoring experience(s).
> > 
> >> I think most people don't actually care one way or the other. If you know of
> > 
> > Like I said, I think you're limiting your audience to LKML subscribers,
> > who are more amenable to being convinced they need to debug their
> > kernel.
> > 
> >> others who care about it too then I suggest you work together as a group to
> >> solve the problem in a way which is amenable to the upstream maintainers.
> > 
> > I know a few off-list people that would like a more user-friendly Arm
> > perf experience, but we're not qualified to solve the larger problem
> > that perf has had since its inception, and I think it's a little unfair
> > for you to suggest that, esp. given you're one of the maintainers, and
> > there's Linus on top of you.
> > 
> >> Then you won't have to worry about fixing it personally. You could even
> >> propose a topic for plumbers if there is enough interest in a general
> >> framework for propagating error messages to userspace.
> > 
> > I'm not worried about fixing it personally or not.  I'm worried about
> > our perf users' experience given your objection to using the vehicle
> > already in use on other architectures and PMU drivers.
> > 
> > If you - or anyone else for that matter - know of a way that'll get us
> > past this, by all means, say something.
> > 
> > If it helps, I recently asked acme if we could borrow bits from struct
> > perf_event and got no response.
> > 
> >>>>> As you are already aware, I've personally tried to fix this problem -
> >>>>> that has existed since before the introduction of the perf tool (I
> >>>>> consider it a syscall-independent enhanced error interface), multiple
> >>>>> times, and failed.
> >>>>
> >>>> Why is that my problem? Try harder?
> >>>
> >>> It's your problem because we're here reviewing a patch that happens to
> >>> fall under your maintainership.  I'll be the first person to tell you
> >>> I'm obviously incompetent and haven't been able to come up with a
> >>> solution that is acceptable for everyone up to and including Linus
> >>> Torvalds.  I'm just noticing a chronic usability problem that can be
> >>> easily alleviated in the context of this patch review.
> >>
> >> I don't see how incompetence has anything to do with it and, like most
> > 
> > Well you told me to try harder, and I'm trying to let you know that I
> > can try harder - ad infinitum - and still not get it, so telling me to
> > try harder isn't going to necessarily resolve the issue.
> > 
> >> people (lucky gits...), I doubt Torvalds cares too deeply about PMU driver
> >> internals. No arguments on the usability problem, but this ain't the fix
> > 
> > Like acme, I doubt Linus runs perf on real Arm h/w, so yes, I don't
> > think they care that much, but if they did try to do it one day, I'd
> > like to that they'd be able to have dmesg contain that extra error
> > information for them.
> > 
> > As you know, David Howells wrote a supplemental error syscall facility
> > [1], that Linus nacked (off list no less).  Does Linus care about what
> > David Howells was developing it for?  I don't know, but he does have
> > enough experience to know whether a certain approach to a problem is
> > sustainable in his kernel or not.
> > 
> >> you're looking for: it's a bodge. We've been over this many times before.
> > 
> > I think it's OK - necessary even - until this error infrastructure
> > problem perf has is fixed, and I think you're being unrealistic if you
> > think it will be fixed anytime soon.
> > 
> >>>>> So until someone comes up with a solution that works for everyone
> >>>>> up to and including Linus Torvalds (who hasn't put up a problem
> >>>>> pulling PMU drivers emitting things to dmesg so far, by the way), this
> >>>>> keep PMU drivers' errors silent preference of yours is unnecessarily
> >>>>> impeding people trying to measure system performance on Arm based
> >>>>> machines - all other archs' maintainers are fine with PMU drivers using
> >>>>> dmesg.
> >>>>
> >>>> Good for them, although I'm pretty sure that at least the x86 folks are
> >>>> against this crap too.
> >>>
> >>> Unfortunately, it doesn't affect them nearly as much as it does our
> >>> more diverse platforms, which is why I don't think they care to do
> >>> much about it.
> >>
> >> x86 has its fair share of PMUs too, so I don't think we're special in this
> >> regard. The main difference seems to be that they have better support in
> >> the perf tool for diagnosing problems.
> > 
> > Indeed, their software is as vertically integrated as their hardware,
> > and, well, they have more people with more experience working and caring
> > about perf running and being more usable.
> 
> I note that uncore_pmu_event_init() and snb_uncore_imc_event_init(), to 
> pick a couple of Intel examples, contain a lot of the same logic to the 
> ARM system PMU drivers you repeatedly request should be more verbose. 
> Now, surely either perf tool already has some magic which somehow makes 
> those routines "user-friendly", in which case there seems no reason that 
> at least any generic event attribute stuff couldn't be applied to ARM 
> PMUs as well; or if not then you could easily send a patch adding 
> "helpful" printk()s to those drivers too and actively solicit the 
> maintainer feedback you lament not having.

Those Intel drivers?  IIRC, those drivers have been developed
historically more in sync with the tool.  I believe early drivers
started out with four simple error cases- each with its own -Evalue -
and as the infrastructure grew, they still enjoyed more-or-less a
one-to-one correspondence between the -Evalue they returned and what
the tool did in response. Periodically the fall-back (retry) code and
message texts get amended as drivers get added.

In the Arm infrastructure, things are generally more 'disabling' to
the user - they'd rather error out instead of be more accommodating
(bit.LITTLE is a good example of this), so we have people copying
others' very strict event_inits that return the most generic -EINVAL
for most things, without trying to see what can be done to (a) either
continue, going "we knew they meant this," or (b) looking at returning
a different -Evalue, and/or eventually printing something to dmesg (in
the case it's pretty clear that the user will have no idea what
esoteric thing happened).

So whilst what you're saying may be true, I care about Arm more, as I
don't see that big an error messaging problem on Intel right now.

> >>>>>> Anyway, I think this driver has bigger problems that need addressing.
> >>>>>
> >>>>> To me it represents yet another PMU driver submission - as the years go
> >>>>> by - that is lacking in the user messaging area.  Which reminds me, can
> >>>>> you take another look at applying this?:
> >>>>
> >>>> As I said before, I'm not going to take anything that logs above pr_debug
> >>>> for things that are directly triggerable from userspace. Spin a version
> >>>
> >>> Why?  There are plenty of things that emit stuff into dmesg that are
> >>> directly triggerable from userspace.  Is it because it upsets fuzzing
> >>> tests?  How about those be run with a patched kernel that somehow
> >>> mitigates the printing?
> >>
> >> [we've been over this before]
> >> There are two camps that might find this information useful:
> >>
> >>    1. People writing userspace support for a new PMU using the
> >>       perf_event_open syscall
> >>
> >>    2. People trying to use a tool which abstracts the PMU details to some
> >>       degree (e.g. perf tool)
> >>
> >> Those in (1) can get by with pr_debug. Sure, they have to recompile, but
> >> it's not like there are many people in this camp and they'll probably be
> >> working with the PMU driver writer to some degree anyway and taking new
> >> kernel drops.
> > 
> > No, please, that's a worse user experience than necessary.
> > 
> > FWIW, I see this type of person as e.g., a JIT developer - a
> > compiler/toolchain person - who has decided to use h/w monitoring to
> > help the JIT engine find hotspots.  The first place they should start
> > is to start using the perf tool itself, experimenting with events and
> > PMUs of interest.  When satisfied, they should clone the parameters
> > the tool makes to the perf_event_open syscall in their JIT engine.  We
> > should not be wasting their time forcing them to hack around in the
> > kernel trying to debug perf driver semantics.
> > 
> >> Those in (2) may not have access to dmesg, absolutely should not be able
> > 
> > I don't think you're being realistic here: we're talking about people
> > trying to improve performance: They would likely have kptr_restrict ==
> > 0 (unrestricted), so why would they have dmesg_restrict set?  Btw, the
> > rationale for dmesg_restrict was kernel pointer visibility [2], but now
> > %Kp hashes addresses before printing them, so it's even less likely to
> > be used.  Curious, where have you seen dmesg_restrict being set?
> > 
> >> to spam it (since this could hide other important messages), will probably
> > 
> > How will it hide other important messages?  Run a logging daemon?
> > We're not talking about a lot of messages here: one line per perf
> > invocation: see e.g. runs in [2].
> 
> Assuming you meant [3], that's a pretty trivial case which doesn't 
> really illustrate the whole scope of the problem.

You're right, they're trivial cases that have trivial messages that
enable the trivial user.  It's not the whole scope of the problem, but
it's a very good start.

> Just for you, I added prints in 4 places where event_init validation 
> returns -EINVAL in my not-yet-upstream SMMUv2 PMU driver. Let's capture 
> a few events on my Juno board to get some arbitrary cross-sections of 
> system-wide I/O activity:
> 
> [root at space-channel-5 ~]# perf stat -e arm_smmu/cycles/ -e 
> arm_smmu/access,tcefcfg=1/ -e arm_smmu/access,tcefcfg=1,smr_id=1/ -e 
> arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ -e 
> arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ find /usr > /dev/null
> 
>   Performance counter stats for 'system wide':
> 
>           729128494      arm_smmu/cycles/ 
>                (12.08%)
>           841301489      arm_smmu/cycles/ 
>                (28.10%)
>           834458758      arm_smmu/cycles/ 
>                (23.40%)
>           774373529      arm_smmu/cycles/ 
>                (15.69%)
>          1028501665      arm_smmu/cycles/ 
>                (13.84%)
>                5401      arm_smmu/access,tcefcfg=1/ 
>                 (27.11%)
>                   0      arm_smmu/access,tcefcfg=1/ 
>                 (14.73%)
>                   0      arm_smmu/access,tcefcfg=1/ 
>                 (23.27%)
>                   0      arm_smmu/access,tcefcfg=1/ 
>                 (20.54%)
>              220788      arm_smmu/access,tcefcfg=1/ 
>                 (17.37%)
>                   0      arm_smmu/access,tcefcfg=1,smr_id=1/ 
>                          (28.18%)
>                   0      arm_smmu/access,tcefcfg=1,smr_id=1/ 
>                          (24.10%)
>                   0      arm_smmu/access,tcefcfg=1,smr_id=1/ 
>                          (12.52%)
>                   0      arm_smmu/access,tcefcfg=1,smr_id=1/ 
>                          (16.17%)
>                   0      arm_smmu/access,tcefcfg=1,smr_id=1/ 
>                          (19.71%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
>                          (19.68%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
>                          (23.69%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
>                          (29.15%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
>                          (29.19%)
>               12092      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
>                          (27.70%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
>                          (16.97%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
>                          (13.36%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
>                          (16.01%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
>                          (24.70%)
>                   0      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
>                          (28.39%)
> 
>         2.315150449 seconds time elapsed
> 
> 
> Wonderful, it worked OK. And yet in that brief time it spammed 
> *sixty-one* lines to the kernel log:
> 
> [ 2096.220934] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.226893] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.232852] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.238798] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.244744] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.250690] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.256641] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.262594] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.268546] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.274492] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.280437] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.286381] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.292326] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.298271] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.304215] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.310164] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.316109] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.322053] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.327997] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.333941] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.339887] event_source arm_smmu_3: Incompatible SMR value
> [ 2096.345408] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.351361] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.357313] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.363263] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.369214] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.375171] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.381124] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.387078] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.393029] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.398984] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.404936] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.410890] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.416840] event_source arm_smmu_4: Incompatible context bank index
> [ 2096.423130] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.429075] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.435019] event_source arm_smmu_1: Incompatible context bank index
> [ 2096.441308] event_source arm_smmu_3: Incompatible context bank index
> [ 2096.447611] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.453557] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.459501] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.465445] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.471390] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.477342] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.483286] event_source arm_smmu_2: Incompatible SMR value
> [ 2096.488800] event_source arm_smmu_0: Incompatible SMR value
> [ 2096.494316] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.500319] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.571799] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.577749] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.583705] event_source arm_smmu_4: Incompatible SMR value
> [ 2096.589221] event_source arm_smmu_2: Incompatible SMR value
> [ 2096.594736] event_source arm_smmu_0: Incompatible SMR value
> [ 2096.600256] event_source arm_smmu_1: Incompatible SMR value
> [ 2096.605770] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.611718] event_source arm_smmu_4: Incompatible filtering mode
> [ 2096.617663] event_source arm_smmu_2: Incompatible filtering mode
> [ 2096.623607] event_source arm_smmu_0: Incompatible filtering mode
> [ 2096.629550] event_source arm_smmu_1: Incompatible filtering mode
> [ 2096.635494] event_source arm_smmu_3: Incompatible filtering mode
> [ 2096.641443] event_source arm_smmu_4: Incompatible context bank index
> 
> (and in case it's not clear, that output is *continual* during the perf 
> run; empirically, the runtime only needs to reach about 6 seconds or so 
> for the dmesg ringbuffer to wrap so I have to fall back to journalctl to 
> see what I missed)
> 
> Now how about we try counting those exact same events a slightly 
> different way:
> 
> [root at space-channel-5 ~]# perf stat -e 
> \{arm_smmu/cycles/,arm_smmu/access,tcefcfg=1/,arm_smmu/access,tcefcfg=1,smr_id=1/,arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/,arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/\} 
> find /usr > /dev/null
> 
>   Performance counter stats for 'system wide':
> 
>       <not counted>      arm_smmu/cycles/ 
> 
>     <not supported>      arm_smmu/cycles/ 
> 
>     <not supported>      arm_smmu/cycles/ 
> 
>     <not supported>      arm_smmu/cycles/ 
> 
>     <not supported>      arm_smmu/cycles/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1,smr_id=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1,smr_id=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1,smr_id=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1,smr_id=1/ 
> 
>     <not supported>      arm_smmu/access,tcefcfg=1,smr_id=1/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=0/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
> 
>     <not supported>      arm_smmu/tlb_alloc,tcefcfg=2,ndx=1/ 
> 
> 
>         0.721310193 seconds time elapsed
> 
> 
> Oh, that didn't work. Let's check dmesg to see why:
> 
> [ 2416.494951] event_source arm_smmu_3: Incompatible filtering mode
> [ 2416.500933] event_source arm_smmu_3: Incompatible filtering mode
> [ 2416.506958] event_source arm_smmu_3: Incompatible filtering mode
> [ 2416.513276] event_source arm_smmu_3: Incompatible filtering mode
> 
> 
> Well, it said the same thing continually when it worked correctly, so I 
> guess it must have failed for some other reason. As the user I'm now not 
> only confused but unknowingly heading towards the *wrong* conclusion 
> thanks to those "helpful" messages. And that's not even contemplating 
> the "what if multiple programs did this at the same time" case.

Like I said in other threads, multiple invocations aren't a concern vs.
experimenting with multiple atomic invocations to get the pattern of
events and their parameters down to a valid, successfully executing
(hopefully without dmesg output?) state.

WRT the rest of what you say above, I'd have to check the individual
error condition details, but yes, you're right to think the user's
confused, but - and forgive the pun here - armed with the information
coming from the driver, they are able to adapt and learn to know what's
possible and what's not when combining different event groups.

The volume of messaging doesn't concern me much either:

- it doesn't change with dev_dbg vs. dev_warn
- using a logger to scroll far back isn't generally a problem
- users tend to focus on one event at a time when experimenting before
composing and invoking the final event list.

> >> struggle to match an internal message from the PMU driver to their
> >> invocation of the high-level tool and may well be in a multi-user
> >> environment so will struggle to identify the messages that they are
> >> responsible for.
> > 
> > Again, I think you're being unrealistic.  My experience - in
> > multiple performance analysis roles - was that I was always the only
> > person using the machine at the time, and can easily correlate perf
> > invocations with dmesg output, particularly if I run 'dmesg -w &' prior
> > to invoking perf.  In one of those roles, having to mess with the
> > kernel was almost inconceivable - it literally 'just worked' for
> > everything else.  I was lucky that I had prior kernel experience in
> > order to debug the driver and be able find out how to invoke perf.
> > 
> >> What they actually want is for the perf tool to give
> >> them more information, and dmesg is not the right channel for that, for
> >> similar reasons.
> > 
> > We'd all like the perf tool to do things better, and dmesg is all we
> > have for now, and now I'm sounding like I did 1 1/2 years ago.
> > 
> >>>> using pr_debug and I'll queue it.
> >>>
> >>> How about using a ratelimited dev_err variant?
> >>
> >> Nah, I don't want dmesg being parsed by users of perf tool. pr_debug should
> >> be sufficient for perf tool developers working with a new PMU type.
> > 
> > OK the question was to the ratelimited part, but I think that even
> > might be the default these days [4].
> > 
> > For reasons (different perceptions?) already mentioned above, I also
> > don't agree that this has to do with just perf tool developers, and
> > only when working with a new PMU type.  My main concern is new/future
> > users to perf on Arm, working with any PMU type - new or old.  If there
> > is a perf tool-side component to this driver, I don't see it.  Having
> > said that, I think the owners of this and other PMU drivers should have
> > a say in what type of experience they want for their users in this
> > regard...is that fair?
> 
> As I've said before, a significant portion of what you want to improve 
> involves generic perf_event properties which would already be much 
> better validated in the core than by every driver individually, so the 
> first straightforward step would be to improve the interface between 
> drivers and perf core, such that the "hard" problem reduces to 
> communicating from a single part of perf core to userspace. As it is, 
> PMU drivers alone simply don't have enough context to know when an 
> event_init "failure" is actually significant to the user or not, thus if 
> they display user-visible messages directly then half the time the user 
> will be thoroughly misled unless they are intimately familiar with how 
> perf core grouping and event rotation work (see above), yet required 
> knowledge of perf internals is exactly what you're arguing against!

You're right, perf is inherently confusing to use, but more information
is better than none, esp. for the trivial cases.

Thanks,

Kim

^ permalink raw reply

* [PATCH] drivers/perf: arm-ccn: stop spamming dmesg in event_init
From: Florian Fainelli @ 2018-05-10  0:22 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509184154.9994df2874e7a08025b56dea@arm.com>

On 05/09/2018 04:41 PM, Kim Phillips wrote:
> On Wed, 9 May 2018 17:06:43 +0100
> Pawel Moll <pawel.moll@arm.com> wrote:
> 
>> On Wed, 2018-05-09 at 00:40 +0100, Kim Phillips wrote:
>>> On Fri,  4 May 2018 11:41:17 +0100 Mark Rutland <mark.rutland@arm.com> wrote:
>>>
>>> [adding Pawel, arm-ccn driver author]
>>
>> We had this discussion way too many times for my liking. As I said
>> before - *I* will be fine with the debug messages in the CCN driver.
>> Now, if there ever turns out to be other user of this thing and gets
> 
> Sure, this isn't just about Pawel using the driver he wrote - we know
> you know how to use it because you wrote it.  No, it's about all the
> other potential users out there, esp. first time users, as I once was.
> 
>> into problems with event configuration, I'd hope that he/she can count
>> on support from the knowledgable people here... (just checked and both
> 
> I abhor having to suggest our users email this list in order to find out
> how to use the PMU drivers.  First time users are going to tend to
> steer completely away if they don't have the patience to debug a silent
> PMU, rather than email this mailing list - sorry, but that's just
> adding a huge usage barrier - for what - fuzzer runner's convenience?
> 
>> RHEL 7.5 and Ubuntu 16.04.3 kernels on AArch64 come with
>> CONFIG_DYNAMIC_DEBUG=y, so it's a matter of explaining what has to be
>> enabled where; sadly this information did not find its way into the
>> commit description)
> 
> I can't think of a more difficult to find, more far-away, alien
> place for end users to find help with perf, sorry.
> 
>>>> The ARM CCN PMU driver uses dev_warn() to complain about parameters
>>> in
>>>> the user-provided perf_event_attr. This means that under normal
>>>> operation (e.g. a single invocation of the perf tool), dmesg may be
>>>> spammed with multiple messages.
>>
>> Surely Mark, in his role as maintainer of drivers/perf/ (and a few
>> other locations), meant to use much more technical and emotion-free
>> subject, along the lines of "reduce a number of dmesg warnings at event
>> init".
> 
> 'reduce a number' is the wrong word:  warnings are completely
> eliminated. Debug-level messages occur at exactly the same
> frequency/amount.
> 
> But I still object to the rationale overall - it seems this is about
> running fuzzers?  I even offered an alternative for fuzzer runners: is
> modifying the loglevel prior to fuzzing somehow unacceptable?

I don't have any dog in this, but maybe if providing information to the
users is so essential to having a pleasant user experience, then
rethinking the whole way these messages are funneled is necessary
because the kernel log + dmesg is by no means appropriate. Take a look
at what the networking maintainers recently did with netlink extended
ack. You used to just get an "unsupported" error, and now you know
exactly what is wrong when extack is available. It seems to me like
something like this is what you might want here since you want to have
perf be as user friendly as possible.

My 2 cents.
-- 
Florian

^ permalink raw reply

* [PATCH] usb: gadget: composite: fill bcdUSB as 0x0320 for SuperSpeed or higher speeds
From: Chunfeng Yun @ 2018-05-10  0:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <8736z1kqrq.fsf@linux.intel.com>

Hi,

On Wed, 2018-05-09 at 14:33 +0300, Felipe Balbi wrote:
> Hi,
> 
> Chunfeng Yun <chunfeng.yun@mediatek.com> writes:
> > The USB3CV version 2.1.80 (March 26, 2018) requires all devices
> > ( gen1, gen2, single lane, dual lane) to return the value of 0x0320
> > in the bcdUSB field
> 
> this sounds really odd. What happens when I get a USB 3.1 compliant
> device off-the-shelf and run it through USB3CV? will it fail now?
It will fail, the last version require it 0x0310
> 
> Care to share a screenshot or the raw html of the test result?
A screenshot is attached
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dev_desc_test_fail.png
Type: image/png
Size: 16866 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180510/64cc8879/attachment-0001.png>

^ permalink raw reply

* [PATCH] drivers/perf: arm-ccn: stop spamming dmesg in event_init
From: Kim Phillips @ 2018-05-10  0:02 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180509161120.ilnukvismwfb452g@lakrids.cambridge.arm.com>

On Wed, 9 May 2018 17:12:42 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, May 09, 2018 at 05:06:43PM +0100, Pawel Moll wrote:
> > On Wed, 2018-05-09 at 00:40 +0100, Kim Phillips wrote:
> > > On Fri,  4 May 2018 11:41:17 +0100 Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > [adding Pawel, arm-ccn driver author]
> > 
> > We had this discussion way too many times for my liking. As I said
> > before - *I* will be fine with the debug messages in the CCN driver.
> 
> > > > The ARM CCN PMU driver uses dev_warn() to complain about parameters
> > > in
> > > > the user-provided perf_event_attr. This means that under normal
> > > > operation (e.g. a single invocation of the perf tool), dmesg may be
> > > > spammed with multiple messages.
> > 
> > Surely Mark, in his role as maintainer of drivers/perf/ (and a few
> > other locations), meant to use much more technical and emotion-free
> > subject, along the lines of "reduce a number of dmesg warnings at event
> > init".
> 
> True. I'll reword the above to:
> 
>   The ARM CCN PMU driver uses dev_warn() to complain about parameters in
>   the user-provided perf_event_attr. This means that under normal
>   operation (e.g. a single invocation of the perf tool), a number of
>   messages warnings may be logged to dmesg.

So from:

"...dmesg may be spammed with multiple messages."

to:

"...a number of messages warnings may be logged to dmesg."

Not sure what's *that* different between the two, but is the assumption
that "normal operation" - i.e., a single *valid* perf invocation -
still emits messages?  In which case, could the misunderstanding here
possibly be the following?:

Assuming I know what node is valid, I can still get messages:

$ dmesg -w &
$ sudo ./perf stat -e ccn/hnf_cache_miss,node=8/ find /usr > /dev/null
[626610.173759] arm-ccn e8000000.ccn: Can't exclude execution levels!
[626610.173790] arm-ccn e8000000.ccn: Can't exclude execution levels!

 Performance counter stats for 'system wide':

         1,096,602      ccn/hnf_cache_miss,node=8/                                   

       3.708314082 seconds time elapsed

But the driver is still technically correct in that it WARNs (not
invisibly DEBUGs) it's users about the EL problem.  In order to not
exclude the default execution levels, we postpend 'hku' to the event,
so now the EL messages are gone:

$ sudo ./perf stat -e ccn/hnf_cache_miss,node=8/hku find /usr > /dev/null

 Performance counter stats for 'system wide':

         1,086,521      ccn/hnf_cache_miss,node=8/hku                                   

       3.699726648 seconds time elapsed

What's important to keep the user warned about with arm-ccn is these
types of messages:

$ sudo ./perf stat -e ccn/hnf_cache_miss,node=1/hku find /usr > /dev/null
[626675.574828] arm-ccn e8000000.ccn: Invalid type 0x4 for node 1!
[626675.574841] arm-ccn e8000000.ccn: Invalid type 0x4 for node 1!

 Performance counter stats for 'system wide':

   <not supported>      ccn/hnf_cache_miss,node=1/                                   

       3.686169559 seconds time elapsed

See?  Now the user is pointed directly at the problem.

I've even enhanced the tool to help identify to the user the event with
an unsupported sampling error (commit 114bc191c370 "perf evsel: Say
which PMU Hardware event doesn't support sampling/overflow-interrupts").

BTW, the dmesg output above was only two lines per effective message:
That should be totally acceptable - it's only one line more than one
line per message.  Are you seeing much more than that?  If so, can we
see an example of an invocation that's completely unacceptable,
dmesg-volume-wise?

Thanks,

Kim

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox