diff mbox series

[V3,02/14] coresight: Do not scan for graph if none is present

Message ID 1611737738-1493-3-git-send-email-anshuman.khandual@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: coresight: Enable ETE and TRBE | expand

Commit Message

Anshuman Khandual Jan. 27, 2021, 8:55 a.m. UTC 
From: Suzuki K Poulose <suzuki.poulose@arm.com>

If a graph node is not found for a given node, of_get_next_endpoint()
will emit the following error message :

 OF: graph: no port node found in /<node_name>

If the given component doesn't have any explicit connections (e.g,
ETE) we could simply ignore the graph parsing.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Mathieu Poirier Feb. 1, 2021, 11:44 p.m. UTC | #1 
On Wed, Jan 27, 2021 at 02:25:26PM +0530, Anshuman Khandual wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> If a graph node is not found for a given node, of_get_next_endpoint()
> will emit the following error message :
> 
>  OF: graph: no port node found in /<node_name>
> 
> If the given component doesn't have any explicit connections (e.g,
> ETE) we could simply ignore the graph parsing.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 3629b78..c594f45 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -90,6 +90,12 @@ static void of_coresight_get_ports_legacy(const struct device_node *node,
>  	struct of_endpoint endpoint;
>  	int in = 0, out = 0;
>  
> +	/*
> +	 * Avoid warnings in of_graph_get_next_endpoint()
> +	 * if the device doesn't have any graph connections
> +	 */
> +	if (!of_graph_is_present(node))
> +		return;

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

>  	do {
>  		ep = of_graph_get_next_endpoint(node, ep);
>  		if (!ep)
> -- 
> 2.7.4
>
Mike Leach Feb. 2, 2021, 11:10 a.m. UTC | #2 
Hi Ansuman,

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> If a graph node is not found for a given node, of_get_next_endpoint()
> will emit the following error message :
>
>  OF: graph: no port node found in /<node_name>
>
> If the given component doesn't have any explicit connections (e.g,
> ETE) we could simply ignore the graph parsing.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 3629b78..c594f45 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -90,6 +90,12 @@ static void of_coresight_get_ports_legacy(const struct device_node *node,
>         struct of_endpoint endpoint;
>         int in = 0, out = 0;
>
> +       /*
> +        * Avoid warnings in of_graph_get_next_endpoint()
> +        * if the device doesn't have any graph connections
> +        */
> +       if (!of_graph_is_present(node))
> +         return;

The problem here is that you are masking genuine errors.
The solution is to either call this only if the device type is one
that ports are not required - i.e. ETE, or upgrade the .dts bindings
for the rest of the ETM devices to yaml so that the ports requirement
is checked and validated there.

Regards

Mike

>         do {
>                 ep = of_graph_get_next_endpoint(node, ep);
>                 if (!ep)
> --
> 2.7.4
>
Suzuki K Poulose Feb. 2, 2021, 2:36 p.m. UTC | #3 
Hi Mike

On 2/2/21 11:10 AM, Mike Leach wrote:
> Hi Ansuman,
> 
> On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> If a graph node is not found for a given node, of_get_next_endpoint()
>> will emit the following error message :
>>
>>   OF: graph: no port node found in /<node_name>
>>
>> If the given component doesn't have any explicit connections (e.g,
>> ETE) we could simply ignore the graph parsing.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 3629b78..c594f45 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -90,6 +90,12 @@ static void of_coresight_get_ports_legacy(const struct device_node *node,
>>          struct of_endpoint endpoint;
>>          int in = 0, out = 0;
>>
>> +       /*
>> +        * Avoid warnings in of_graph_get_next_endpoint()
>> +        * if the device doesn't have any graph connections
>> +        */
>> +       if (!of_graph_is_present(node))
>> +         return;
> 
> The problem here is that you are masking genuine errors.

If the graph is not described for a component, where it is
mandatory, it won't be usable by the driver and as such using
the devices will fail.

e.g, if an ETM misses the bindings, tracing will fail. (in either
mode).

> The solution is to either call this only if the device type is one
> that ports are not required - i.e. ETE, or upgrade the .dts bindings

The proposed change is too invasive and is not worth the benefit
that it brings.

The side effect of this patch is, if someone makes a mistake in the
bindings they don't see the "warning" in the dmesg. But will definitely
hit the issue when trying to use the system.

i.e, Functionally there is no change.

On the other hand issuing a warning message for ETE is confusing for
a well behaved user.

> for the rest of the ETM devices to yaml so that the ports requirement
> is checked and validated there.

This is a step that we must take, but in a separate series. And I
don't think this will solve handling non-compliant DTs *immediately*,
as there could be :
  a) DTS that are not upstream (Quite common for CoreSight)
  b) People are getting used to the schema and running schema checks.

So, personally I vote for :

1) Merge this patch in as is
2) Convert the bindings to Yaml in a separate series.

Suzuki
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 3629b78..c594f45 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -90,6 +90,12 @@  static void of_coresight_get_ports_legacy(const struct device_node *node,
 	struct of_endpoint endpoint;
 	int in = 0, out = 0;
 
+	/*
+	 * Avoid warnings in of_graph_get_next_endpoint()
+	 * if the device doesn't have any graph connections
+	 */
+	if (!of_graph_is_present(node))
+		return;
 	do {
 		ep = of_graph_get_next_endpoint(node, ep);
 		if (!ep)