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

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Mike Leach <mike.leach@linaro.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>
Cc: linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Coresight ML <coresight@lists.linaro.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Linu Cherian <lcherian@marvell.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V3 02/14] coresight: Do not scan for graph if none is present
Date: Tue, 2 Feb 2021 14:36:05 +0000	[thread overview]
Message-ID: <81648285-a321-173f-a0da-6ba0bc077abf@arm.com> (raw)
In-Reply-To: <CAJ9a7VjB8-h_0DBuMHvv4A+mHpz1GUczbGB6Tx1cBocRghm7mQ@mail.gmail.com>

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