[PATCH 0/5] coresight: Miscellaneous fixes

[PATCH 0/5] coresight: Miscellaneous fixes

[Suzuki K Poulose]

This is a collection fixes / cleanups for the coresight driver.

Patches 1,2 are proper fixes and would be good to have it fixed for 4.7.
Patches 3-5 are cleanups.

Suzuki K Poulose (5):
  coresight: Fix NULL pointer dereference in _coresight_build_path
  coresight: etmv4: Fix ETMv4x peripheral ID table
  coresight: Fix csdev connections initialisation
  coresight: Add better messages for coresight_timeout
  coresight: Cleanup TMC status check

 drivers/hwtracing/coresight/coresight-etb10.c   |  6 ++--
 drivers/hwtracing/coresight/coresight-etm4x.c   | 14 ++++----
 drivers/hwtracing/coresight/coresight-tmc-etr.c |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c     |  6 ++--
 drivers/hwtracing/coresight/coresight.c         | 48 ++++++++++++++-----------
 5 files changed, 38 insertions(+), 38 deletions(-)

-- 
1.9.1

[PATCH 0/5] coresight: Miscellaneous fixes

[Suzuki K Poulose]

This is a collection fixes / cleanups for the coresight driver.

Patches 1,2 are proper fixes and would be good to have it fixed for 4.7.
Patches 3-5 are cleanups.

Suzuki K Poulose (5):
  coresight: Fix NULL pointer dereference in _coresight_build_path
  coresight: etmv4: Fix ETMv4x peripheral ID table
  coresight: Fix csdev connections initialisation
  coresight: Add better messages for coresight_timeout
  coresight: Cleanup TMC status check

 drivers/hwtracing/coresight/coresight-etb10.c   |  6 ++--
 drivers/hwtracing/coresight/coresight-etm4x.c   | 14 ++++----
 drivers/hwtracing/coresight/coresight-tmc-etr.c |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c     |  6 ++--
 drivers/hwtracing/coresight/coresight.c         | 48 ++++++++++++++-----------
 5 files changed, 38 insertions(+), 38 deletions(-)

-- 
1.9.1

[PATCH 1/5] coresight: Fix NULL pointer dereference in _coresight_build_path

[Suzuki K Poulose]

_coresight_build_path assumes that all the connections of a csdev
has the child_dev initialised. This may not be true if the particular
component is not supported by the kernel config(e.g TPIU) but is
present in the DT. In which case, building a path can cause a crash like this :

  Unable to handle kernel NULL pointer dereference at virtual address 00000010
  pgd = ffffffc9750dd000
  [00000010] *pgd=00000009f5e90003, *pud=00000009f5e90003, *pmd=0000000000000000
  Internal error: Oops: 96000006 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 4 PID: 1348 Comm: bash Not tainted 4.6.0-next-20160517 #1646
  Hardware name: ARM Juno development board (r0) (DT)
  task: ffffffc97517a280 ti: ffffffc9762c4000 task.ti: ffffffc9762c4000
  PC is at _coresight_build_path+0x18/0xe4
  LR is at _coresight_build_path+0xc0/0xe4
  pc : [<ffffff80083d5130>] lr : [<ffffff80083d51d8>] pstate: 20000145
  sp : ffffffc9762c7ba0

  [<ffffff80083d5130>] _coresight_build_path+0x18/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d5cdc>] coresight_build_path+0x40/0x68
  [<ffffff80083d5e14>] coresight_enable+0x74/0x1bc
  [<ffffff80083d60a0>] enable_source_store+0x3c/0x6c
  [<ffffff800830b17c>] dev_attr_store+0x18/0x28
  [<ffffff80081ca9c4>] sysfs_kf_write+0x40/0x50
  [<ffffff80081c9e38>] kernfs_fop_write+0x140/0x1cc
  [<ffffff8008163ec8>] __vfs_write+0x28/0x110
  [<ffffff8008164bf0>] vfs_write+0xa0/0x174
  [<ffffff8008165d18>] SyS_write+0x44/0xa0
  [<ffffff8008084e70>] el0_svc_naked+0x24/0x28

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 5443d03..0fdaaf4 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -385,7 +385,6 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	int i;
 	bool found = false;
 	struct coresight_node *node;
-	struct coresight_connection *conn;
 
 	/* An activated sink has been found.  Enqueue the element */
 	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
@@ -394,8 +393,9 @@ static int _coresight_build_path(struct coresight_device *csdev,
 
 	/* Not a sink - recursively explore each port found on this element */
 	for (i = 0; i < csdev->nr_outport; i++) {
-		conn = &csdev->conns[i];
-		if (_coresight_build_path(conn->child_dev, path) == 0) {
+		struct coresight_device *child_dev = csdev->conns[i].child_dev;
+
+		if (child_dev && _coresight_build_path(child_dev, path) == 0) {
 			found = true;
 			break;
 		}
-- 
1.9.1

[PATCH 1/5] coresight: Fix NULL pointer dereference in _coresight_build_path

[Suzuki K Poulose]

_coresight_build_path assumes that all the connections of a csdev
has the child_dev initialised. This may not be true if the particular
component is not supported by the kernel config(e.g TPIU) but is
present in the DT. In which case, building a path can cause a crash like this :

  Unable to handle kernel NULL pointer dereference at virtual address 00000010
  pgd = ffffffc9750dd000
  [00000010] *pgd=00000009f5e90003, *pud=00000009f5e90003, *pmd=0000000000000000
  Internal error: Oops: 96000006 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 4 PID: 1348 Comm: bash Not tainted 4.6.0-next-20160517 #1646
  Hardware name: ARM Juno development board (r0) (DT)
  task: ffffffc97517a280 ti: ffffffc9762c4000 task.ti: ffffffc9762c4000
  PC is at _coresight_build_path+0x18/0xe4
  LR is at _coresight_build_path+0xc0/0xe4
  pc : [<ffffff80083d5130>] lr : [<ffffff80083d51d8>] pstate: 20000145
  sp : ffffffc9762c7ba0

  [<ffffff80083d5130>] _coresight_build_path+0x18/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
  [<ffffff80083d5cdc>] coresight_build_path+0x40/0x68
  [<ffffff80083d5e14>] coresight_enable+0x74/0x1bc
  [<ffffff80083d60a0>] enable_source_store+0x3c/0x6c
  [<ffffff800830b17c>] dev_attr_store+0x18/0x28
  [<ffffff80081ca9c4>] sysfs_kf_write+0x40/0x50
  [<ffffff80081c9e38>] kernfs_fop_write+0x140/0x1cc
  [<ffffff8008163ec8>] __vfs_write+0x28/0x110
  [<ffffff8008164bf0>] vfs_write+0xa0/0x174
  [<ffffff8008165d18>] SyS_write+0x44/0xa0
  [<ffffff8008084e70>] el0_svc_naked+0x24/0x28

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 5443d03..0fdaaf4 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -385,7 +385,6 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	int i;
 	bool found = false;
 	struct coresight_node *node;
-	struct coresight_connection *conn;
 
 	/* An activated sink has been found.  Enqueue the element */
 	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
@@ -394,8 +393,9 @@ static int _coresight_build_path(struct coresight_device *csdev,
 
 	/* Not a sink - recursively explore each port found on this element */
 	for (i = 0; i < csdev->nr_outport; i++) {
-		conn = &csdev->conns[i];
-		if (_coresight_build_path(conn->child_dev, path) == 0) {
+		struct coresight_device *child_dev = csdev->conns[i].child_dev;
+
+		if (child_dev && _coresight_build_path(child_dev, path) == 0) {
 			found = true;
 			break;
 		}
-- 
1.9.1

Re: [PATCH 1/5] coresight: Fix NULL pointer dereference in _coresight_build_path

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> _coresight_build_path assumes that all the connections of a csdev
> has the child_dev initialised. This may not be true if the particular
> component is not supported by the kernel config(e.g TPIU) but is
> present in the DT. In which case, building a path can cause a crash like this :
>
>   Unable to handle kernel NULL pointer dereference at virtual address 00000010
>   pgd = ffffffc9750dd000
>   [00000010] *pgd=00000009f5e90003, *pud=00000009f5e90003, *pmd=0000000000000000
>   Internal error: Oops: 96000006 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 4 PID: 1348 Comm: bash Not tainted 4.6.0-next-20160517 #1646
>   Hardware name: ARM Juno development board (r0) (DT)
>   task: ffffffc97517a280 ti: ffffffc9762c4000 task.ti: ffffffc9762c4000
>   PC is at _coresight_build_path+0x18/0xe4
>   LR is at _coresight_build_path+0xc0/0xe4
>   pc : [<ffffff80083d5130>] lr : [<ffffff80083d51d8>] pstate: 20000145
>   sp : ffffffc9762c7ba0
>
>   [<ffffff80083d5130>] _coresight_build_path+0x18/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d5cdc>] coresight_build_path+0x40/0x68
>   [<ffffff80083d5e14>] coresight_enable+0x74/0x1bc
>   [<ffffff80083d60a0>] enable_source_store+0x3c/0x6c
>   [<ffffff800830b17c>] dev_attr_store+0x18/0x28
>   [<ffffff80081ca9c4>] sysfs_kf_write+0x40/0x50
>   [<ffffff80081c9e38>] kernfs_fop_write+0x140/0x1cc
>   [<ffffff8008163ec8>] __vfs_write+0x28/0x110
>   [<ffffff8008164bf0>] vfs_write+0xa0/0x174
>   [<ffffff8008165d18>] SyS_write+0x44/0xa0
>   [<ffffff8008084e70>] el0_svc_naked+0x24/0x28
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 5443d03..0fdaaf4 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -385,7 +385,6 @@ static int _coresight_build_path(struct coresight_device *csdev,
>         int i;
>         bool found = false;
>         struct coresight_node *node;
> -       struct coresight_connection *conn;
>
>         /* An activated sink has been found.  Enqueue the element */
>         if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> @@ -394,8 +393,9 @@ static int _coresight_build_path(struct coresight_device *csdev,
>
>         /* Not a sink - recursively explore each port found on this element */
>         for (i = 0; i < csdev->nr_outport; i++) {
> -               conn = &csdev->conns[i];
> -               if (_coresight_build_path(conn->child_dev, path) == 0) {
> +               struct coresight_device *child_dev = csdev->conns[i].child_dev;
> +
> +               if (child_dev && _coresight_build_path(child_dev, path) == 0) {
>                         found = true;
>                         break;
>                 }
> --
> 1.9.1
>

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Greg, since this is a bug fix can we add it to 4.7-rc2?

Thanks,
Mathieu

[PATCH 1/5] coresight: Fix NULL pointer dereference in _coresight_build_path

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> _coresight_build_path assumes that all the connections of a csdev
> has the child_dev initialised. This may not be true if the particular
> component is not supported by the kernel config(e.g TPIU) but is
> present in the DT. In which case, building a path can cause a crash like this :
>
>   Unable to handle kernel NULL pointer dereference at virtual address 00000010
>   pgd = ffffffc9750dd000
>   [00000010] *pgd=00000009f5e90003, *pud=00000009f5e90003, *pmd=0000000000000000
>   Internal error: Oops: 96000006 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 4 PID: 1348 Comm: bash Not tainted 4.6.0-next-20160517 #1646
>   Hardware name: ARM Juno development board (r0) (DT)
>   task: ffffffc97517a280 ti: ffffffc9762c4000 task.ti: ffffffc9762c4000
>   PC is at _coresight_build_path+0x18/0xe4
>   LR is at _coresight_build_path+0xc0/0xe4
>   pc : [<ffffff80083d5130>] lr : [<ffffff80083d51d8>] pstate: 20000145
>   sp : ffffffc9762c7ba0
>
>   [<ffffff80083d5130>] _coresight_build_path+0x18/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d51d8>] _coresight_build_path+0xc0/0xe4
>   [<ffffff80083d5cdc>] coresight_build_path+0x40/0x68
>   [<ffffff80083d5e14>] coresight_enable+0x74/0x1bc
>   [<ffffff80083d60a0>] enable_source_store+0x3c/0x6c
>   [<ffffff800830b17c>] dev_attr_store+0x18/0x28
>   [<ffffff80081ca9c4>] sysfs_kf_write+0x40/0x50
>   [<ffffff80081c9e38>] kernfs_fop_write+0x140/0x1cc
>   [<ffffff8008163ec8>] __vfs_write+0x28/0x110
>   [<ffffff8008164bf0>] vfs_write+0xa0/0x174
>   [<ffffff8008165d18>] SyS_write+0x44/0xa0
>   [<ffffff8008084e70>] el0_svc_naked+0x24/0x28
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 5443d03..0fdaaf4 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -385,7 +385,6 @@ static int _coresight_build_path(struct coresight_device *csdev,
>         int i;
>         bool found = false;
>         struct coresight_node *node;
> -       struct coresight_connection *conn;
>
>         /* An activated sink has been found.  Enqueue the element */
>         if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> @@ -394,8 +393,9 @@ static int _coresight_build_path(struct coresight_device *csdev,
>
>         /* Not a sink - recursively explore each port found on this element */
>         for (i = 0; i < csdev->nr_outport; i++) {
> -               conn = &csdev->conns[i];
> -               if (_coresight_build_path(conn->child_dev, path) == 0) {
> +               struct coresight_device *child_dev = csdev->conns[i].child_dev;
> +
> +               if (child_dev && _coresight_build_path(child_dev, path) == 0) {
>                         found = true;
>                         break;
>                 }
> --
> 1.9.1
>

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Greg, since this is a bug fix can we add it to 4.7-rc2?

Thanks,
Mathieu

[PATCH 2/5] coresight: etmv4: Fix ETMv4x peripheral ID table

[Suzuki K Poulose]

This patch cleans up the peripheral id table for different ETMv4
implementations.

As per Cortex-A53 TRM, the ETM has following id values:

Peripheral ID0	0x5D	0xFE0
Peripheral ID1	0xB9	0xFE4
Peripheral ID2	0x4B	0xFE8
Peripheral ID3	0x00	0xFEC

where, PID2: has the following format:

[7:4]   Revision
[3]     JEDEC   0b1     res1. Indicates a JEP106 identity code is used
[2:0]   DES_1   0b011   ARM Limited. This is bits[6:4] of JEP106 ID code

The existing table entry checks only the bits [1:0], which is not
sufficient enough. Fix it to match bits [3:0], just like the other
entries do. While at it, correct the comment for A57 and the A53 entry.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 462f0dc..88947f3 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -815,12 +815,12 @@ err_arch_supported:
 }
 
 static struct amba_id etm4_ids[] = {
-	{       /* ETM 4.0 - Qualcomm */
-		.id	= 0x0003b95d,
-		.mask	= 0x0003ffff,
+	{       /* ETM 4.0 - Cortex-A53  */
+		.id	= 0x000bb95d,
+		.mask	= 0x000fffff,
 		.data	= "ETM 4.0",
 	},
-	{       /* ETM 4.0 - Juno board */
+	{       /* ETM 4.0 - Cortex-A57 */
 		.id	= 0x000bb95e,
 		.mask	= 0x000fffff,
 		.data	= "ETM 4.0",
-- 
1.9.1

[PATCH 2/5] coresight: etmv4: Fix ETMv4x peripheral ID table

[Suzuki K Poulose]

This patch cleans up the peripheral id table for different ETMv4
implementations.

As per Cortex-A53 TRM, the ETM has following id values:

Peripheral ID0	0x5D	0xFE0
Peripheral ID1	0xB9	0xFE4
Peripheral ID2	0x4B	0xFE8
Peripheral ID3	0x00	0xFEC

where, PID2: has the following format:

[7:4]   Revision
[3]     JEDEC   0b1     res1. Indicates a JEP106 identity code is used
[2:0]   DES_1   0b011   ARM Limited. This is bits[6:4] of JEP106 ID code

The existing table entry checks only the bits [1:0], which is not
sufficient enough. Fix it to match bits [3:0], just like the other
entries do. While at it, correct the comment for A57 and the A53 entry.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 462f0dc..88947f3 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -815,12 +815,12 @@ err_arch_supported:
 }
 
 static struct amba_id etm4_ids[] = {
-	{       /* ETM 4.0 - Qualcomm */
-		.id	= 0x0003b95d,
-		.mask	= 0x0003ffff,
+	{       /* ETM 4.0 - Cortex-A53  */
+		.id	= 0x000bb95d,
+		.mask	= 0x000fffff,
 		.data	= "ETM 4.0",
 	},
-	{       /* ETM 4.0 - Juno board */
+	{       /* ETM 4.0 - Cortex-A57 */
 		.id	= 0x000bb95e,
 		.mask	= 0x000fffff,
 		.data	= "ETM 4.0",
-- 
1.9.1

Re: [PATCH 2/5] coresight: etmv4: Fix ETMv4x peripheral ID table

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> This patch cleans up the peripheral id table for different ETMv4
> implementations.
>
> As per Cortex-A53 TRM, the ETM has following id values:
>
> Peripheral ID0  0x5D    0xFE0
> Peripheral ID1  0xB9    0xFE4
> Peripheral ID2  0x4B    0xFE8
> Peripheral ID3  0x00    0xFEC
>
> where, PID2: has the following format:
>
> [7:4]   Revision
> [3]     JEDEC   0b1     res1. Indicates a JEP106 identity code is used
> [2:0]   DES_1   0b011   ARM Limited. This is bits[6:4] of JEP106 ID code
>
> The existing table entry checks only the bits [1:0], which is not
> sufficient enough. Fix it to match bits [3:0], just like the other
> entries do. While at it, correct the comment for A57 and the A53 entry.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 462f0dc..88947f3 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -815,12 +815,12 @@ err_arch_supported:
>  }
>
>  static struct amba_id etm4_ids[] = {
> -       {       /* ETM 4.0 - Qualcomm */
> -               .id     = 0x0003b95d,
> -               .mask   = 0x0003ffff,
> +       {       /* ETM 4.0 - Cortex-A53  */
> +               .id     = 0x000bb95d,
> +               .mask   = 0x000fffff,
>                 .data   = "ETM 4.0",
>         },
> -       {       /* ETM 4.0 - Juno board */
> +       {       /* ETM 4.0 - Cortex-A57 */
>                 .id     = 0x000bb95e,
>                 .mask   = 0x000fffff,
>                 .data   = "ETM 4.0",
> --
> 1.9.1
>

I'll apply this to my next tree but since it doesn't cause a core dump
or prevent the kernel from booting, I don't think it deserves to be in
-rc2.

Thanks,
Mathieu

[PATCH 2/5] coresight: etmv4: Fix ETMv4x peripheral ID table

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> This patch cleans up the peripheral id table for different ETMv4
> implementations.
>
> As per Cortex-A53 TRM, the ETM has following id values:
>
> Peripheral ID0  0x5D    0xFE0
> Peripheral ID1  0xB9    0xFE4
> Peripheral ID2  0x4B    0xFE8
> Peripheral ID3  0x00    0xFEC
>
> where, PID2: has the following format:
>
> [7:4]   Revision
> [3]     JEDEC   0b1     res1. Indicates a JEP106 identity code is used
> [2:0]   DES_1   0b011   ARM Limited. This is bits[6:4] of JEP106 ID code
>
> The existing table entry checks only the bits [1:0], which is not
> sufficient enough. Fix it to match bits [3:0], just like the other
> entries do. While at it, correct the comment for A57 and the A53 entry.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 462f0dc..88947f3 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -815,12 +815,12 @@ err_arch_supported:
>  }
>
>  static struct amba_id etm4_ids[] = {
> -       {       /* ETM 4.0 - Qualcomm */
> -               .id     = 0x0003b95d,
> -               .mask   = 0x0003ffff,
> +       {       /* ETM 4.0 - Cortex-A53  */
> +               .id     = 0x000bb95d,
> +               .mask   = 0x000fffff,
>                 .data   = "ETM 4.0",
>         },
> -       {       /* ETM 4.0 - Juno board */
> +       {       /* ETM 4.0 - Cortex-A57 */
>                 .id     = 0x000bb95e,
>                 .mask   = 0x000fffff,
>                 .data   = "ETM 4.0",
> --
> 1.9.1
>

I'll apply this to my next tree but since it doesn't cause a core dump
or prevent the kernel from booting, I don't think it deserves to be in
-rc2.

Thanks,
Mathieu

[PATCH 3/5] coresight: Fix csdev connections initialisation

[Suzuki K Poulose]

This is a cleanup patch.

coresight_device->conns holds an array to point to the devices
connected to the OUT ports of a component. Sinks, e.g ETR, do not
have an OUT port (nr_outport = 0), as it streams the trace to
memory via AXI.

At coresight_register() we do :

	conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
	if (!conns) {
		ret = -ENOMEM;
		goto err_kzalloc_conns;
	}

For ETR, since the total size requested for kcalloc is zero, the return value is,
ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
verified later to contain a valid pointer. The code which accesses the csdev->conns
is bounded by the csdev->nr_outport check, hence we don't try to dereference the
ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
to make sure we initialise it properly(i.e, either NULL or valid conns array).

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 0fdaaf4..8410420 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	int nr_refcnts = 1;
 	atomic_t *refcnts = NULL;
 	struct coresight_device *csdev;
-	struct coresight_connection *conns;
+	struct coresight_connection *conns = NULL;
 
 	csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
 	if (!csdev) {
@@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 			nr_refcnts = desc->pdata->nr_outport;
 	}
 
-	refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
-	if (!refcnts) {
-		ret = -ENOMEM;
-		goto err_kzalloc_refcnts;
-	}
+	if (nr_refcnts) {
+		refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
+		if (!refcnts) {
+			ret = -ENOMEM;
+			goto err_kzalloc_refcnts;
+		}
 
-	csdev->refcnt = refcnts;
+		csdev->refcnt = refcnts;
+	}
 
 	csdev->nr_inport = desc->pdata->nr_inport;
 	csdev->nr_outport = desc->pdata->nr_outport;
-	conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
-	if (!conns) {
-		ret = -ENOMEM;
-		goto err_kzalloc_conns;
-	}
 
-	for (i = 0; i < csdev->nr_outport; i++) {
-		conns[i].outport = desc->pdata->outports[i];
-		conns[i].child_name = desc->pdata->child_names[i];
-		conns[i].child_port = desc->pdata->child_ports[i];
-	}
+	/* Initialise connections if there is at least one outport for this component */
+	if (csdev->nr_outport) {
+		conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
+		if (!conns) {
+			ret = -ENOMEM;
+			goto err_kzalloc_conns;
+		}
 
-	csdev->conns = conns;
+		for (i = 0; i < csdev->nr_outport; i++) {
+			conns[i].outport = desc->pdata->outports[i];
+			conns[i].child_name = desc->pdata->child_names[i];
+			conns[i].child_port = desc->pdata->child_ports[i];
+		}
+
+		csdev->conns = conns;
+	}
 
 	csdev->type = desc->type;
 	csdev->subtype = desc->subtype;
-- 
1.9.1

[PATCH 3/5] coresight: Fix csdev connections initialisation

[Suzuki K Poulose]

This is a cleanup patch.

coresight_device->conns holds an array to point to the devices
connected to the OUT ports of a component. Sinks, e.g ETR, do not
have an OUT port (nr_outport = 0), as it streams the trace to
memory via AXI.

At coresight_register() we do :

	conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
	if (!conns) {
		ret = -ENOMEM;
		goto err_kzalloc_conns;
	}

For ETR, since the total size requested for kcalloc is zero, the return value is,
ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
verified later to contain a valid pointer. The code which accesses the csdev->conns
is bounded by the csdev->nr_outport check, hence we don't try to dereference the
ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
to make sure we initialise it properly(i.e, either NULL or valid conns array).

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 0fdaaf4..8410420 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	int nr_refcnts = 1;
 	atomic_t *refcnts = NULL;
 	struct coresight_device *csdev;
-	struct coresight_connection *conns;
+	struct coresight_connection *conns = NULL;
 
 	csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
 	if (!csdev) {
@@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 			nr_refcnts = desc->pdata->nr_outport;
 	}
 
-	refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
-	if (!refcnts) {
-		ret = -ENOMEM;
-		goto err_kzalloc_refcnts;
-	}
+	if (nr_refcnts) {
+		refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
+		if (!refcnts) {
+			ret = -ENOMEM;
+			goto err_kzalloc_refcnts;
+		}
 
-	csdev->refcnt = refcnts;
+		csdev->refcnt = refcnts;
+	}
 
 	csdev->nr_inport = desc->pdata->nr_inport;
 	csdev->nr_outport = desc->pdata->nr_outport;
-	conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
-	if (!conns) {
-		ret = -ENOMEM;
-		goto err_kzalloc_conns;
-	}
 
-	for (i = 0; i < csdev->nr_outport; i++) {
-		conns[i].outport = desc->pdata->outports[i];
-		conns[i].child_name = desc->pdata->child_names[i];
-		conns[i].child_port = desc->pdata->child_ports[i];
-	}
+	/* Initialise connections if there is at least one outport for this component */
+	if (csdev->nr_outport) {
+		conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
+		if (!conns) {
+			ret = -ENOMEM;
+			goto err_kzalloc_conns;
+		}
 
-	csdev->conns = conns;
+		for (i = 0; i < csdev->nr_outport; i++) {
+			conns[i].outport = desc->pdata->outports[i];
+			conns[i].child_name = desc->pdata->child_names[i];
+			conns[i].child_port = desc->pdata->child_ports[i];
+		}
+
+		csdev->conns = conns;
+	}
 
 	csdev->type = desc->type;
 	csdev->subtype = desc->subtype;
-- 
1.9.1

Re: [PATCH 3/5] coresight: Fix csdev connections initialisation

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> This is a cleanup patch.
>
> coresight_device->conns holds an array to point to the devices
> connected to the OUT ports of a component. Sinks, e.g ETR, do not
> have an OUT port (nr_outport = 0), as it streams the trace to
> memory via AXI.
>
> At coresight_register() we do :
>
>         conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
>         if (!conns) {
>                 ret = -ENOMEM;
>                 goto err_kzalloc_conns;
>         }
>
> For ETR, since the total size requested for kcalloc is zero, the return value is,
> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
> verified later to contain a valid pointer. The code which accesses the csdev->conns
> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 0fdaaf4..8410420 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>         int nr_refcnts = 1;
>         atomic_t *refcnts = NULL;
>         struct coresight_device *csdev;
> -       struct coresight_connection *conns;
> +       struct coresight_connection *conns = NULL;
>
>         csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>         if (!csdev) {
> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>                         nr_refcnts = desc->pdata->nr_outport;
>         }
>
> -       refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> -       if (!refcnts) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc_refcnts;
> -       }
> +       if (nr_refcnts) {

Did you manage to find a usecase where "nr_refcnts == 0" ?  Since
components have at least one port this condition will always be true.

> +               refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> +               if (!refcnts) {
> +                       ret = -ENOMEM;
> +                       goto err_kzalloc_refcnts;
> +               }
>
> -       csdev->refcnt = refcnts;
> +               csdev->refcnt = refcnts;
> +       }
>
>         csdev->nr_inport = desc->pdata->nr_inport;
>         csdev->nr_outport = desc->pdata->nr_outport;
> -       conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> -       if (!conns) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc_conns;
> -       }
>
> -       for (i = 0; i < csdev->nr_outport; i++) {
> -               conns[i].outport = desc->pdata->outports[i];
> -               conns[i].child_name = desc->pdata->child_names[i];
> -               conns[i].child_port = desc->pdata->child_ports[i];
> -       }
> +       /* Initialise connections if there is at least one outport for this component */
> +       if (csdev->nr_outport) {
> +               conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> +               if (!conns) {
> +                       ret = -ENOMEM;
> +                       goto err_kzalloc_conns;
> +               }
>
> -       csdev->conns = conns;
> +               for (i = 0; i < csdev->nr_outport; i++) {
> +                       conns[i].outport = desc->pdata->outports[i];
> +                       conns[i].child_name = desc->pdata->child_names[i];
> +                       conns[i].child_port = desc->pdata->child_ports[i];
> +               }
> +
> +               csdev->conns = conns;

The purpose of your patch is to correctly initialise csdev->conns to
NULL if there is no output port to speak of.  As such shouldn't the
above statement be out of the if (csdev->nr_outport) {} ?.

I'm also getting a couple of checkpatch.pl warnings on this patch.

> +       }
>
>         csdev->type = desc->type;
>         csdev->subtype = desc->subtype;
> --
> 1.9.1
>

[PATCH 3/5] coresight: Fix csdev connections initialisation

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> This is a cleanup patch.
>
> coresight_device->conns holds an array to point to the devices
> connected to the OUT ports of a component. Sinks, e.g ETR, do not
> have an OUT port (nr_outport = 0), as it streams the trace to
> memory via AXI.
>
> At coresight_register() we do :
>
>         conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
>         if (!conns) {
>                 ret = -ENOMEM;
>                 goto err_kzalloc_conns;
>         }
>
> For ETR, since the total size requested for kcalloc is zero, the return value is,
> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
> verified later to contain a valid pointer. The code which accesses the csdev->conns
> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 0fdaaf4..8410420 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>         int nr_refcnts = 1;
>         atomic_t *refcnts = NULL;
>         struct coresight_device *csdev;
> -       struct coresight_connection *conns;
> +       struct coresight_connection *conns = NULL;
>
>         csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>         if (!csdev) {
> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>                         nr_refcnts = desc->pdata->nr_outport;
>         }
>
> -       refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> -       if (!refcnts) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc_refcnts;
> -       }
> +       if (nr_refcnts) {

Did you manage to find a usecase where "nr_refcnts == 0" ?  Since
components have at least one port this condition will always be true.

> +               refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> +               if (!refcnts) {
> +                       ret = -ENOMEM;
> +                       goto err_kzalloc_refcnts;
> +               }
>
> -       csdev->refcnt = refcnts;
> +               csdev->refcnt = refcnts;
> +       }
>
>         csdev->nr_inport = desc->pdata->nr_inport;
>         csdev->nr_outport = desc->pdata->nr_outport;
> -       conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> -       if (!conns) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc_conns;
> -       }
>
> -       for (i = 0; i < csdev->nr_outport; i++) {
> -               conns[i].outport = desc->pdata->outports[i];
> -               conns[i].child_name = desc->pdata->child_names[i];
> -               conns[i].child_port = desc->pdata->child_ports[i];
> -       }
> +       /* Initialise connections if there is at least one outport for this component */
> +       if (csdev->nr_outport) {
> +               conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> +               if (!conns) {
> +                       ret = -ENOMEM;
> +                       goto err_kzalloc_conns;
> +               }
>
> -       csdev->conns = conns;
> +               for (i = 0; i < csdev->nr_outport; i++) {
> +                       conns[i].outport = desc->pdata->outports[i];
> +                       conns[i].child_name = desc->pdata->child_names[i];
> +                       conns[i].child_port = desc->pdata->child_ports[i];
> +               }
> +
> +               csdev->conns = conns;

The purpose of your patch is to correctly initialise csdev->conns to
NULL if there is no output port to speak of.  As such shouldn't the
above statement be out of the if (csdev->nr_outport) {} ?.

I'm also getting a couple of checkpatch.pl warnings on this patch.

> +       }
>
>         csdev->type = desc->type;
>         csdev->subtype = desc->subtype;
> --
> 1.9.1
>

Re: [PATCH 3/5] coresight: Fix csdev connections initialisation

[Suzuki K Poulose]

On 31/05/16 18:55, Mathieu Poirier wrote:
> On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>> This is a cleanup patch.
>>
>> coresight_device->conns holds an array to point to the devices
>> connected to the OUT ports of a component. Sinks, e.g ETR, do not
>> have an OUT port (nr_outport = 0), as it streams the trace to
>> memory via AXI.
>>
>> At coresight_register() we do :
>>
>>          conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
>>          if (!conns) {
>>                  ret = -ENOMEM;
>>                  goto err_kzalloc_conns;
>>          }
>>
>> For ETR, since the total size requested for kcalloc is zero, the return value is,
>> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
>> verified later to contain a valid pointer. The code which accesses the csdev->conns
>> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
>> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
>> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>> index 0fdaaf4..8410420 100644
>> --- a/drivers/hwtracing/coresight/coresight.c
>> +++ b/drivers/hwtracing/coresight/coresight.c
>> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>>          int nr_refcnts = 1;
>>          atomic_t *refcnts = NULL;
>>          struct coresight_device *csdev;
>> -       struct coresight_connection *conns;
>> +       struct coresight_connection *conns = NULL;
>>
>>          csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>>          if (!csdev) {
>> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>>                          nr_refcnts = desc->pdata->nr_outport;
>>          }
>>
>> -       refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
>> -       if (!refcnts) {
>> -               ret = -ENOMEM;
>> -               goto err_kzalloc_refcnts;
>> -       }
>> +       if (nr_refcnts) {
>
> Did you manage to find a usecase where "nr_refcnts == 0" ?  Since
> components have at least one port this condition will always be true.

No, I didn't find one. While I was fixing the nr_outport, I thought it would
be good to check the refcnts as well.

>>
>> -       csdev->conns = conns;
>> +               for (i = 0; i < csdev->nr_outport; i++) {
>> +                       conns[i].outport = desc->pdata->outports[i];
>> +                       conns[i].child_name = desc->pdata->child_names[i];
>> +                       conns[i].child_port = desc->pdata->child_ports[i];
>> +               }
>> +
>> +               csdev->conns = conns;
>
> The purpose of your patch is to correctly initialise csdev->conns to
> NULL if there is no output port to speak of.  As such shouldn't the
> above statement be out of the if (csdev->nr_outport) {} ?.

Not necessarily. We do a kzalloc() for csdev, which implies csdev->conns
is already NULL. I can move the code to make that explicit assignment.

>
> I'm also getting a couple of checkpatch.pl warnings on this patch.

I can fix those warnings. I ignored them initially, as it was for the length of the comment.

Cheers
Suzuki

[PATCH 3/5] coresight: Fix csdev connections initialisation

[Suzuki K Poulose]

On 31/05/16 18:55, Mathieu Poirier wrote:
> On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>> This is a cleanup patch.
>>
>> coresight_device->conns holds an array to point to the devices
>> connected to the OUT ports of a component. Sinks, e.g ETR, do not
>> have an OUT port (nr_outport = 0), as it streams the trace to
>> memory via AXI.
>>
>> At coresight_register() we do :
>>
>>          conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
>>          if (!conns) {
>>                  ret = -ENOMEM;
>>                  goto err_kzalloc_conns;
>>          }
>>
>> For ETR, since the total size requested for kcalloc is zero, the return value is,
>> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
>> verified later to contain a valid pointer. The code which accesses the csdev->conns
>> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
>> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
>> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>> index 0fdaaf4..8410420 100644
>> --- a/drivers/hwtracing/coresight/coresight.c
>> +++ b/drivers/hwtracing/coresight/coresight.c
>> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>>          int nr_refcnts = 1;
>>          atomic_t *refcnts = NULL;
>>          struct coresight_device *csdev;
>> -       struct coresight_connection *conns;
>> +       struct coresight_connection *conns = NULL;
>>
>>          csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>>          if (!csdev) {
>> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>>                          nr_refcnts = desc->pdata->nr_outport;
>>          }
>>
>> -       refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
>> -       if (!refcnts) {
>> -               ret = -ENOMEM;
>> -               goto err_kzalloc_refcnts;
>> -       }
>> +       if (nr_refcnts) {
>
> Did you manage to find a usecase where "nr_refcnts == 0" ?  Since
> components have at least one port this condition will always be true.

No, I didn't find one. While I was fixing the nr_outport, I thought it would
be good to check the refcnts as well.

>>
>> -       csdev->conns = conns;
>> +               for (i = 0; i < csdev->nr_outport; i++) {
>> +                       conns[i].outport = desc->pdata->outports[i];
>> +                       conns[i].child_name = desc->pdata->child_names[i];
>> +                       conns[i].child_port = desc->pdata->child_ports[i];
>> +               }
>> +
>> +               csdev->conns = conns;
>
> The purpose of your patch is to correctly initialise csdev->conns to
> NULL if there is no output port to speak of.  As such shouldn't the
> above statement be out of the if (csdev->nr_outport) {} ?.

Not necessarily. We do a kzalloc() for csdev, which implies csdev->conns
is already NULL. I can move the code to make that explicit assignment.

>
> I'm also getting a couple of checkpatch.pl warnings on this patch.

I can fix those warnings. I ignored them initially, as it was for the length of the comment.

Cheers
Suzuki

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Suzuki K Poulose]

When we encounter a timeout waiting for a status change via
coresight_timeout, the caller always print the offset which
was tried. This is pretty much useless as it doesn't specify
the bit position we wait for. Also, one needs to lookup the
TRM to figure out, what was wrong. This patch changes all
such error messages to print something more meaningful.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 6 ++----
 drivers/hwtracing/coresight/coresight-etm4x.c | 6 ++----
 drivers/hwtracing/coresight/coresight-tmc.c   | 6 ++----
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 4d20b0b..6bd4b93 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
 
 	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			ETB_FFCR);
+			"timeout while waiting for completion of Manual Flush\n");
 	}
 
 	/* disable trace capture */
@@ -193,8 +192,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
 
 	if (coresight_timeout(drvdata->base, ETB_FFSR, ETB_FFSR_BIT, 1)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			ETB_FFCR);
+			"timeout while waiting for Formatter to Stop\n");
 	}
 
 	CS_LOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 88947f3..e494042 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -111,8 +111,7 @@ static void etm4_enable_hw(void *info)
 	/* wait for TRCSTATR.IDLE to go up */
 	if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TRCSTATR);
+			"timeout when waiting for Idle Trace Status\n");
 
 	writel_relaxed(config->pe_sel, drvdata->base + TRCPROCSELR);
 	writel_relaxed(config->cfg, drvdata->base + TRCCONFIGR);
@@ -184,8 +183,7 @@ static void etm4_enable_hw(void *info)
 	/* wait for TRCSTATR.IDLE to go back down to '0' */
 	if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TRCSTATR);
+			"timeout when waiting for Idle Trace Status\n");
 
 	CS_LOCK(drvdata->base);
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 9e02ac9..c7d8ba6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -38,8 +38,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 	if (coresight_timeout(drvdata->base,
 			      TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TMC_STS);
+			"timeout observed when waiting for TMC to be Ready\n");
 	}
 }
 
@@ -56,8 +55,7 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 	if (coresight_timeout(drvdata->base,
 			      TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TMC_FFCR);
+			"timeout observed while waiting for completion of Manual Flush\n");
 	}
 
 	tmc_wait_for_tmcready(drvdata);
-- 
1.9.1

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Suzuki K Poulose]

When we encounter a timeout waiting for a status change via
coresight_timeout, the caller always print the offset which
was tried. This is pretty much useless as it doesn't specify
the bit position we wait for. Also, one needs to lookup the
TRM to figure out, what was wrong. This patch changes all
such error messages to print something more meaningful.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 6 ++----
 drivers/hwtracing/coresight/coresight-etm4x.c | 6 ++----
 drivers/hwtracing/coresight/coresight-tmc.c   | 6 ++----
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 4d20b0b..6bd4b93 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
 
 	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			ETB_FFCR);
+			"timeout while waiting for completion of Manual Flush\n");
 	}
 
 	/* disable trace capture */
@@ -193,8 +192,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
 
 	if (coresight_timeout(drvdata->base, ETB_FFSR, ETB_FFSR_BIT, 1)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			ETB_FFCR);
+			"timeout while waiting for Formatter to Stop\n");
 	}
 
 	CS_LOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 88947f3..e494042 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -111,8 +111,7 @@ static void etm4_enable_hw(void *info)
 	/* wait for TRCSTATR.IDLE to go up */
 	if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TRCSTATR);
+			"timeout when waiting for Idle Trace Status\n");
 
 	writel_relaxed(config->pe_sel, drvdata->base + TRCPROCSELR);
 	writel_relaxed(config->cfg, drvdata->base + TRCCONFIGR);
@@ -184,8 +183,7 @@ static void etm4_enable_hw(void *info)
 	/* wait for TRCSTATR.IDLE to go back down to '0' */
 	if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TRCSTATR);
+			"timeout when waiting for Idle Trace Status\n");
 
 	CS_LOCK(drvdata->base);
 
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 9e02ac9..c7d8ba6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -38,8 +38,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 	if (coresight_timeout(drvdata->base,
 			      TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TMC_STS);
+			"timeout observed when waiting for TMC to be Ready\n");
 	}
 }
 
@@ -56,8 +55,7 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 	if (coresight_timeout(drvdata->base,
 			      TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
 		dev_err(drvdata->dev,
-			"timeout observed when probing at offset %#x\n",
-			TMC_FFCR);
+			"timeout observed while waiting for completion of Manual Flush\n");
 	}
 
 	tmc_wait_for_tmcready(drvdata);
-- 
1.9.1

Re: [PATCH 4/5] coresight: Add better messages for coresight_timeout

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> When we encounter a timeout waiting for a status change via
> coresight_timeout, the caller always print the offset which
> was tried. This is pretty much useless as it doesn't specify
> the bit position we wait for. Also, one needs to lookup the
> TRM to figure out, what was wrong. This patch changes all
> such error messages to print something more meaningful.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etb10.c | 6 ++----
>  drivers/hwtracing/coresight/coresight-etm4x.c | 6 ++----
>  drivers/hwtracing/coresight/coresight-tmc.c   | 6 ++----
>  3 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index 4d20b0b..6bd4b93 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>
>         if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       ETB_FFCR);
> +                       "timeout while waiting for completion of Manual Flush\n");
>         }
>
>         /* disable trace capture */
> @@ -193,8 +192,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>
>         if (coresight_timeout(drvdata->base, ETB_FFSR, ETB_FFSR_BIT, 1)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       ETB_FFCR);
> +                       "timeout while waiting for Formatter to Stop\n");
>         }
>
>         CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 88947f3..e494042 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -111,8 +111,7 @@ static void etm4_enable_hw(void *info)
>         /* wait for TRCSTATR.IDLE to go up */
>         if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TRCSTATR);
> +                       "timeout when waiting for Idle Trace Status\n");
>
>         writel_relaxed(config->pe_sel, drvdata->base + TRCPROCSELR);
>         writel_relaxed(config->cfg, drvdata->base + TRCCONFIGR);
> @@ -184,8 +183,7 @@ static void etm4_enable_hw(void *info)
>         /* wait for TRCSTATR.IDLE to go back down to '0' */
>         if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TRCSTATR);
> +                       "timeout when waiting for Idle Trace Status\n");
>
>         CS_LOCK(drvdata->base);
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 9e02ac9..c7d8ba6 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -38,8 +38,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>         if (coresight_timeout(drvdata->base,
>                               TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TMC_STS);
> +                       "timeout observed when waiting for TMC to be Ready\n");
>         }
>  }
>
> @@ -56,8 +55,7 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>         if (coresight_timeout(drvdata->base,
>                               TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TMC_FFCR);
> +                       "timeout observed while waiting for completion of Manual Flush\n");
>         }
>
>         tmc_wait_for_tmcready(drvdata);
> --
> 1.9.1
>

I'm fine with this change but in all 3 files the beginning of the new
error message is different.  Please select one and use it throughout.

Thanks,
Mathieu

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> When we encounter a timeout waiting for a status change via
> coresight_timeout, the caller always print the offset which
> was tried. This is pretty much useless as it doesn't specify
> the bit position we wait for. Also, one needs to lookup the
> TRM to figure out, what was wrong. This patch changes all
> such error messages to print something more meaningful.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-etb10.c | 6 ++----
>  drivers/hwtracing/coresight/coresight-etm4x.c | 6 ++----
>  drivers/hwtracing/coresight/coresight-tmc.c   | 6 ++----
>  3 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index 4d20b0b..6bd4b93 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>
>         if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       ETB_FFCR);
> +                       "timeout while waiting for completion of Manual Flush\n");
>         }
>
>         /* disable trace capture */
> @@ -193,8 +192,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>
>         if (coresight_timeout(drvdata->base, ETB_FFSR, ETB_FFSR_BIT, 1)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       ETB_FFCR);
> +                       "timeout while waiting for Formatter to Stop\n");
>         }
>
>         CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 88947f3..e494042 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -111,8 +111,7 @@ static void etm4_enable_hw(void *info)
>         /* wait for TRCSTATR.IDLE to go up */
>         if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 1))
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TRCSTATR);
> +                       "timeout when waiting for Idle Trace Status\n");
>
>         writel_relaxed(config->pe_sel, drvdata->base + TRCPROCSELR);
>         writel_relaxed(config->cfg, drvdata->base + TRCCONFIGR);
> @@ -184,8 +183,7 @@ static void etm4_enable_hw(void *info)
>         /* wait for TRCSTATR.IDLE to go back down to '0' */
>         if (coresight_timeout(drvdata->base, TRCSTATR, TRCSTATR_IDLE_BIT, 0))
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TRCSTATR);
> +                       "timeout when waiting for Idle Trace Status\n");
>
>         CS_LOCK(drvdata->base);
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 9e02ac9..c7d8ba6 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -38,8 +38,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>         if (coresight_timeout(drvdata->base,
>                               TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TMC_STS);
> +                       "timeout observed when waiting for TMC to be Ready\n");
>         }
>  }
>
> @@ -56,8 +55,7 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>         if (coresight_timeout(drvdata->base,
>                               TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
>                 dev_err(drvdata->dev,
> -                       "timeout observed when probing at offset %#x\n",
> -                       TMC_FFCR);
> +                       "timeout observed while waiting for completion of Manual Flush\n");
>         }
>
>         tmc_wait_for_tmcready(drvdata);
> --
> 1.9.1
>

I'm fine with this change but in all 3 files the beginning of the new
error message is different.  Please select one and use it throughout.

Thanks,
Mathieu

Re: [PATCH 4/5] coresight: Add better messages for coresight_timeout

[Joe Perches]

On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
> When we encounter a timeout waiting for a status change via
> coresight_timeout, the caller always print the offset which
> was tried. This is pretty much useless as it doesn't specify
> the bit position we wait for. Also, one needs to lookup the
> TRM to figure out, what was wrong. This patch changes all
> such error messages to print something more meaningful.

trivia:

Perhaps consistently using
	dev_err(dev, "timeout while waiting for %s\n", "<foo>");
could make the object code a bit smaller.

> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
[]
> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>  
>  	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
>  		dev_err(drvdata->dev,
> -			"timeout observed when probing at offset %#x\n",
> -			ETB_FFCR);
> +			"timeout while waiting for completion of Manual Flush\n");

ie:
		dev_err(drvdata->dev,
			"timeout while waiting for %s\n",
			"completion of Manual Flush");

but that depends on how many of these coresight
files are compiled and linked.

There is a while/when usage difference in some of
the output messages.

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Joe Perches]

On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
> When we encounter a timeout waiting for a status change via
> coresight_timeout, the caller always print the offset which
> was tried. This is pretty much useless as it doesn't specify
> the bit position we wait for. Also, one needs to lookup the
> TRM to figure out, what was wrong. This patch changes all
> such error messages to print something more meaningful.

trivia:

Perhaps consistently using
	dev_err(dev, "timeout while waiting for %s\n", "<foo>");
could make the object code a bit smaller.

> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
[]
> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
> ?
> ?	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
> ?		dev_err(drvdata->dev,
> -			"timeout observed when probing at offset %#x\n",
> -			ETB_FFCR);
> +			"timeout while waiting for completion of Manual Flush\n");

ie:
		dev_err(drvdata->dev,
			"timeout while waiting for %s\n",
			"completion of Manual Flush");

but that depends on how many of these coresight
files are compiled and linked.

There is a while/when usage difference in some of
the output messages.

Re: [PATCH 4/5] coresight: Add better messages for coresight_timeout

[Suzuki K Poulose]

On 31/05/16 18:58, Joe Perches wrote:
> On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
>> When we encounter a timeout waiting for a status change via
>> coresight_timeout, the caller always print the offset which
>> was tried. This is pretty much useless as it doesn't specify
>> the bit position we wait for. Also, one needs to lookup the
>> TRM to figure out, what was wrong. This patch changes all
>> such error messages to print something more meaningful.
>
> trivia:
>
> Perhaps consistently using
> 	dev_err(dev, "timeout while waiting for %s\n", "<foo>");
> could make the object code a bit smaller.
>
>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> []
>> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>>
>>   	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
>>   		dev_err(drvdata->dev,
>> -			"timeout observed when probing at offset %#x\n",
>> -			ETB_FFCR);
>> +			"timeout while waiting for completion of Manual Flush\n");
>
> ie:
> 		dev_err(drvdata->dev,
> 			"timeout while waiting for %s\n",
> 			"completion of Manual Flush");
>
> but that depends on how many of these coresight
> files are compiled and linked.

Or we could move the timeout message to coresight_timeout(). The only disadvantage is
if a caller is OK with silent timeouts. How about :

int coresight_timeout(void *base, u32 offset, u32 bit, u32 val, char *info)

where the message can be suppressed if info == NULL ?

Mathieu, your thoughts ?

>
> There is a while/when usage difference in some of
> the output messages.

Right, I will fix them. This was a merged version of individual patches, hence
the changes.

Cheers
Suzuki

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Suzuki K Poulose]

On 31/05/16 18:58, Joe Perches wrote:
> On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
>> When we encounter a timeout waiting for a status change via
>> coresight_timeout, the caller always print the offset which
>> was tried. This is pretty much useless as it doesn't specify
>> the bit position we wait for. Also, one needs to lookup the
>> TRM to figure out, what was wrong. This patch changes all
>> such error messages to print something more meaningful.
>
> trivia:
>
> Perhaps consistently using
> 	dev_err(dev, "timeout while waiting for %s\n", "<foo>");
> could make the object code a bit smaller.
>
>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> []
>> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata)
>>
>>   	if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0)) {
>>   		dev_err(drvdata->dev,
>> -			"timeout observed when probing at offset %#x\n",
>> -			ETB_FFCR);
>> +			"timeout while waiting for completion of Manual Flush\n");
>
> ie:
> 		dev_err(drvdata->dev,
> 			"timeout while waiting for %s\n",
> 			"completion of Manual Flush");
>
> but that depends on how many of these coresight
> files are compiled and linked.

Or we could move the timeout message to coresight_timeout(). The only disadvantage is
if a caller is OK with silent timeouts. How about :

int coresight_timeout(void *base, u32 offset, u32 bit, u32 val, char *info)

where the message can be suppressed if info == NULL ?

Mathieu, your thoughts ?

>
> There is a while/when usage difference in some of
> the output messages.

Right, I will fix them. This was a merged version of individual patches, hence
the changes.

Cheers
Suzuki

Re: [PATCH 4/5] coresight: Add better messages for coresight_timeout

[Mathieu Poirier]

On 1 June 2016 at 03:34, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 31/05/16 18:58, Joe Perches wrote:
>>
>> On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
>>>
>>> When we encounter a timeout waiting for a status change via
>>> coresight_timeout, the caller always print the offset which
>>> was tried. This is pretty much useless as it doesn't specify
>>> the bit position we wait for. Also, one needs to lookup the
>>> TRM to figure out, what was wrong. This patch changes all
>>> such error messages to print something more meaningful.
>>
>>
>> trivia:
>>
>> Perhaps consistently using
>>         dev_err(dev, "timeout while waiting for %s\n", "<foo>");
>> could make the object code a bit smaller.
>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
>>> b/drivers/hwtracing/coresight/coresight-etb10.c
>>
>> []
>>>
>>> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata
>>> *drvdata)
>>>
>>>         if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0))
>>> {
>>>                 dev_err(drvdata->dev,
>>> -                       "timeout observed when probing at offset %#x\n",
>>> -                       ETB_FFCR);
>>> +                       "timeout while waiting for completion of Manual
>>> Flush\n");
>>
>>
>> ie:
>>                 dev_err(drvdata->dev,
>>                         "timeout while waiting for %s\n",
>>                         "completion of Manual Flush");
>>
>> but that depends on how many of these coresight
>> files are compiled and linked.
>
>
> Or we could move the timeout message to coresight_timeout(). The only
> disadvantage is
> if a caller is OK with silent timeouts. How about :
>
> int coresight_timeout(void *base, u32 offset, u32 bit, u32 val, char *info)
>
> where the message can be suppressed if info == NULL ?
>
> Mathieu, your thoughts ?

I'd rather keep things separate.

Thanks,
Mathieu

>
>>
>> There is a while/when usage difference in some of
>> the output messages.
>
>
> Right, I will fix them. This was a merged version of individual patches,
> hence
> the changes.
>
> Cheers
> Suzuki

[PATCH 4/5] coresight: Add better messages for coresight_timeout

[Mathieu Poirier]

On 1 June 2016 at 03:34, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 31/05/16 18:58, Joe Perches wrote:
>>
>> On Tue, 2016-05-31 at 12:57 +0100, Suzuki K Poulose wrote:
>>>
>>> When we encounter a timeout waiting for a status change via
>>> coresight_timeout, the caller always print the offset which
>>> was tried. This is pretty much useless as it doesn't specify
>>> the bit position we wait for. Also, one needs to lookup the
>>> TRM to figure out, what was wrong. This patch changes all
>>> such error messages to print something more meaningful.
>>
>>
>> trivia:
>>
>> Perhaps consistently using
>>         dev_err(dev, "timeout while waiting for %s\n", "<foo>");
>> could make the object code a bit smaller.
>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c
>>> b/drivers/hwtracing/coresight/coresight-etb10.c
>>
>> []
>>>
>>> @@ -184,8 +184,7 @@ static void etb_disable_hw(struct etb_drvdata
>>> *drvdata)
>>>
>>>         if (coresight_timeout(drvdata->base, ETB_FFCR, ETB_FFCR_BIT, 0))
>>> {
>>>                 dev_err(drvdata->dev,
>>> -                       "timeout observed when probing at offset %#x\n",
>>> -                       ETB_FFCR);
>>> +                       "timeout while waiting for completion of Manual
>>> Flush\n");
>>
>>
>> ie:
>>                 dev_err(drvdata->dev,
>>                         "timeout while waiting for %s\n",
>>                         "completion of Manual Flush");
>>
>> but that depends on how many of these coresight
>> files are compiled and linked.
>
>
> Or we could move the timeout message to coresight_timeout(). The only
> disadvantage is
> if a caller is OK with silent timeouts. How about :
>
> int coresight_timeout(void *base, u32 offset, u32 bit, u32 val, char *info)
>
> where the message can be suppressed if info == NULL ?
>
> Mathieu, your thoughts ?

I'd rather keep things separate.

Thanks,
Mathieu

>
>>
>> There is a while/when usage difference in some of
>> the output messages.
>
>
> Right, I will fix them. This was a merged version of individual patches,
> hence
> the changes.
>
> Cheers
> Suzuki

[PATCH 5/5] coresight: Cleanup TMC status check

[Suzuki K Poulose]

Use the defined symbol rather than hardcoding the value to
check whether the TMC buffer is full.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 847d1b5..b0dce93 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -65,7 +65,7 @@ static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
 	val = readl_relaxed(drvdata->base + TMC_STS);
 
 	/* How much memory do we still have */
-	if (val & BIT(0))
+	if (val & TMC_STS_FULL)
 		drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
 	else
 		drvdata->buf = drvdata->vaddr;
-- 
1.9.1

[PATCH 5/5] coresight: Cleanup TMC status check

[Suzuki K Poulose]

Use the defined symbol rather than hardcoding the value to
check whether the TMC buffer is full.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 847d1b5..b0dce93 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -65,7 +65,7 @@ static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
 	val = readl_relaxed(drvdata->base + TMC_STS);
 
 	/* How much memory do we still have */
-	if (val & BIT(0))
+	if (val & TMC_STS_FULL)
 		drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
 	else
 		drvdata->buf = drvdata->vaddr;
-- 
1.9.1

Re: [PATCH 5/5] coresight: Cleanup TMC status check

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> Use the defined symbol rather than hardcoding the value to
> check whether the TMC buffer is full.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 847d1b5..b0dce93 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -65,7 +65,7 @@ static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
>         val = readl_relaxed(drvdata->base + TMC_STS);
>
>         /* How much memory do we still have */
> -       if (val & BIT(0))
> +       if (val & TMC_STS_FULL)
>                 drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
>         else
>                 drvdata->buf = drvdata->vaddr;
> --
> 1.9.1
>

Applied - thanks.
Mathieu

[PATCH 5/5] coresight: Cleanup TMC status check

[Mathieu Poirier]

On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> Use the defined symbol rather than hardcoding the value to
> check whether the TMC buffer is full.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 847d1b5..b0dce93 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -65,7 +65,7 @@ static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
>         val = readl_relaxed(drvdata->base + TMC_STS);
>
>         /* How much memory do we still have */
> -       if (val & BIT(0))
> +       if (val & TMC_STS_FULL)
>                 drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
>         else
>                 drvdata->buf = drvdata->vaddr;
> --
> 1.9.1
>

Applied - thanks.
Mathieu