From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:44:46
Coresight uses DT graph bindings to describe the connections of the
components. However we have some undocumented usage of the bindings
to describe some of the properties of the connections.
The coresight driver needs to know the hardware ports invovled
in the connection and the direction of data flow to effectively
manage the trace sessions. So far we have relied on the "port"
address (as described by the generic graph bindings) to represent
the hardware port of the component for a connection.
The hardware uses separate numbering scheme for input and output
ports, which implies, we could have two different (input and output)
ports with the same port number. This could create problems in the
graph bindings where the label of the port wouldn't match the address.
e.g, with the existing bindings we get :
port@0{ // Output port 0
reg = <0>;
...
};
port@1{
reg = <0>; // Input port 0
endpoint {
slave-mode;
...
};
};
With the new enforcement in the DT rules, mismatches in label and address
are not allowed (as see in the case for port@1). So, we need a new mechanism
to describe the hardware port number reliably.
Also, we relied on an undocumented "slave-mode" property (see the above
example) to indicate if the port is an input port. Let us formalise and
switch to a new property to describe the direction of data flow.
There were three options considered for the hardware port number scheme:
1) Use natural ordering in the DT to infer the hardware port number.
i.e, Mandate that the all ports are listed in the DT and in the ascending
order for each class (input and output respectively).
Pros :
- We don't need new properties and if the existing DTS list them in
order (which most of them do), they work out of the box.
Cons :
- We must list all the ports even if the system cannot/shouldn't use
it.
- It is prone to human errors (if the order is not kept).
2) Use an explicit property to list both the direction and the hw port
number and direction. Define "coresight,hwid" as 2 member array of u32,
where the members are port number and the direction respectively.
e.g
port@0{
reg = <0>;
endpoint {
coresight,hwid = <0 1>; // Port # 0, Output
}
};
port@1{
reg = <1>;
endpoint {
coresight,hwid = <0 0>; // Port # 0, Input
};
};
Pros:
- The bindings are formal but not so reader friendly and could potentially
lead to human errors.
Cons:
- Backward compatiblity is lost.
3) Use explicit properties (implemented in the series) for the hardware
port id and direction. We define a new property "coresight,hwid" for
each endpoint in coresight devices to specify the hardware port number
explicitly. Also use a separate property "direction" to specify the
direction of the data flow.
e.g,
port@0{
reg = <0>;
endpoint {
direction = <1>; // Output
coresight,hwid = <0>; // Port # 0
}
};
port@1{
reg = <1>;
endpoint {
direction = <0>; // Input
coresight,hwid = <0>; // Port # 0
};
};
Pros:
- The bindings are formal and reader friendly, and less prone to errors.
Cons:
- Backward compatibility is lost.
This series implements Option (3) listed above and falls back to the old
bindings if the new bindings are not available. This allows the systems
with old bindings work with the new driver. The driver now issues a warning
(once) when it encounters the old bindings.
It also cleans up the platform parsing code to reduce the memory usage by
reusing the platform description.
I am not sure what is the best route to push these DTS changes. Thoughts ?
Changes since RFC [0] :
- Fixed style issues
- Fix an existing memory leak coresight_register (Found in code update)
- Fix missing of_node_put() in the existing driver (Reported-by Mathieu)
- Update the existing dts in kernel tree.
- Rename new helper functions for consistency
Suzuki K Poulose (20):
coresight: Fix memory leak in coresight_register
coresight: of: Fix refcounting for graph nodes
coresight: Fix remote endpoint parsing
coresight: Cleanup platform description data
coresight: platform: Cleanup coresight connection handling
coresight: Handle errors in finding input/output ports
coresight: dts: Document usage of graph bindings
coresight: dts: Cleanup device tree graph bindings
coresight: dts: Define new bindings for direction of data flow
dts: juno: Update coresight bindings for hw port
dts: hisilicon: Update coresight bindings for hw ports
dts: spreadtrum: Update coresight bindings for hw ports
dts: qcom: Update coresight bindings for hw ports
dts: arm: hisilicon: Update coresight bindings for hardware port
dts: arm: imx7{d,s}: Update coresight binding for hardware ports
dts: arm: omap: Update coresight bindings for hardware ports
dts: arm: qcom: Update coresight bindings for hardware ports
dts: sama5d2: Update coresight bindings for hardware ports
dts: ste-dbx5x0: Update coresight bindings for hardware port
dts: tc2: Update coresight bindings for hardware ports
.../devicetree/bindings/arm/coresight.txt | 76 +++++--
arch/arm/boot/dts/hip04.dtsi | 195 +++++++++++++-----
arch/arm/boot/dts/imx7d.dtsi | 5 +-
arch/arm/boot/dts/imx7s.dtsi | 41 ++--
arch/arm/boot/dts/omap3-beagle-xm.dts | 5 +-
arch/arm/boot/dts/omap3-beagle.dts | 5 +-
arch/arm/boot/dts/qcom-apq8064.dtsi | 37 +++-
arch/arm/boot/dts/qcom-msm8974.dtsi | 60 ++++--
arch/arm/boot/dts/sama5d2.dtsi | 5 +-
arch/arm/boot/dts/ste-dbx5x0.dtsi | 31 ++-
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts | 48 +++--
arch/arm64/boot/dts/arm/juno-base.dtsi | 82 +++++---
arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi | 26 ++-
arch/arm64/boot/dts/arm/juno.dts | 5 +-
.../arm64/boot/dts/hisilicon/hi6220-coresight.dtsi | 89 ++++++---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 55 ++++--
arch/arm64/boot/dts/sprd/sc9836.dtsi | 40 ++--
arch/arm64/boot/dts/sprd/sc9860.dtsi | 101 +++++++---
drivers/hwtracing/coresight/coresight.c | 30 +--
drivers/hwtracing/coresight/of_coresight.c | 219 +++++++++++++--------
include/linux/coresight.h | 11 +-
21 files changed, 818 insertions(+), 348 deletions(-)
--
2.7.4
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:44:51
commit 6403587a930c ("coresight: use put_device() instead of kfree()")
introduced a memory leak where, if we fail to register the device
for coresight_device, we don't free the "coresight_device" object,
which was allocated via kzalloc(). Fix this by jumping to the
appropriate error path.
Fixes: commit 6403587a930c ("coresight: use put_device() instead of kfree()")
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Arvind Yadav <redacted>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:44:55
When parsing the remote endpoint of an output port, we do :
rport = of_graph_get_remote_port(ep);
rparent = of_graph_get_remote_port_parent(ep);
and then parse the "remote_port" as if it was the remote endpoint,
which is wrong. The code worked fine because we used endpoint number
as the port number. Let us fix it and optimise a bit as:
remote_ep = of_graph_get_remote_endpoint(ep);
if (remote_ep)
remote_parent = of_graph_get_port_parent(remote_ep);
and then, parse the remote_ep for the port/endpoint details.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/of_coresight.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
@@ -129,7 +129,7 @@ static int of_coresight_parse_endpoint(struct device_node *ep,intret=0;structof_endpointendpoint,rendpoint;structdevice_node*rparent=NULL;-structdevice_node*rport=NULL;+structdevice_node*rep=NULL;structdevice*rdev=NULL;do{
@@ -144,16 +144,16 @@ static int of_coresight_parse_endpoint(struct device_node *ep,if(of_graph_parse_endpoint(ep,&endpoint))break;/*-*Getahandleontheremoteportandparent-*attachedtoit.+*Getahandleontheremoteendpointandthedeviceitis+*attachedto.*/-rparent=of_graph_get_remote_port_parent(ep);+rep=of_graph_get_remote_endpoint(ep);+if(!rep)+break;+rparent=of_graph_get_port_parent(rep);if(!rparent)break;-rport=of_graph_get_remote_port(ep);-if(!rport)-break;-if(of_graph_parse_endpoint(rport,&rendpoint))+if(of_graph_parse_endpoint(rep,&rendpoint))break;/* If the remote device is not available, defer probing */
@@ -165,15 +165,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep,pdata->outports[*i]=endpoint.port;pdata->child_names[*i]=dev_name(rdev);-pdata->child_ports[*i]=rendpoint.id;+pdata->child_ports[*i]=rendpoint.port;/* Move the index */(*i)++;}while(0);if(rparent)of_node_put(rparent);-if(rport)-of_node_put(rport);+if(rep)+of_node_put(rep);returnret;}
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:45:02
The platform code parses the component connections and populates
a platform-description of the output connections in arrays of fields
(which is never freed). This is later copied in the coresight_register
to a newly allocated area, represented by coresight_connection(s).
This patch cleans up the code dealing with connections by making
use of the "coresight_connection" structure right at the platform
code and lets the generic driver simply re-use information provided
by the platform.
Thus making it reader friendly as well as avoiding the wastage of
unused memory.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 21 +-----------
drivers/hwtracing/coresight/of_coresight.c | 51 ++++++++++++------------------
include/linux/coresight.h | 9 ++----
3 files changed, 23 insertions(+), 58 deletions(-)
@@ -988,22 +986,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)csdev->nr_inport=desc->pdata->nr_inport;csdev->nr_outport=desc->pdata->nr_outport;-/* Initialise connections if there is at least one outport */-if(csdev->nr_outport){-conns=kcalloc(csdev->nr_outport,sizeof(*conns),GFP_KERNEL);-if(!conns){-ret=-ENOMEM;-gotoerr_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];-}-}--csdev->conns=conns;+csdev->conns=desc->pdata->conns;csdev->type=desc->type;csdev->subtype=desc->subtype;
@@ -70,26 +70,13 @@ static void of_coresight_get_ports(const struct device_node *node,staticintof_coresight_alloc_memory(structdevice*dev,structcoresight_platform_data*pdata){-/* List of output port on this component */-pdata->outports=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->outports),-GFP_KERNEL);-if(!pdata->outports)-return-ENOMEM;--/* Children connected to this component via @outports */-pdata->child_names=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->child_names),-GFP_KERNEL);-if(!pdata->child_names)-return-ENOMEM;--/* Port number on the child this component is connected to */-pdata->child_ports=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->child_ports),-GFP_KERNEL);-if(!pdata->child_ports)-return-ENOMEM;+if(pdata->nr_outport){+pdata->conns=devm_kzalloc(dev,pdata->nr_outport*+sizeof(*pdata->conns),+GFP_KERNEL);+if(!pdata->conns)+return-ENOMEM;+}return0;}
@@ -163,11 +150,11 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;}-pdata->outports[*i]=endpoint.port;-pdata->child_names[*i]=dev_name(rdev);-pdata->child_ports[*i]=rendpoint.port;-/* Move the index */-(*i)++;+conn->outport=endpoint.port;+conn->child_name=dev_name(rdev);+conn->child_port=rendpoint.port;+/* Move the connection record */+(*pconn)++;}while(0);if(rparent)
@@ -205,13 +193,14 @@ of_get_coresight_platform_data(struct device *dev,if(ret)returnERR_PTR(ret);+conn=pdata->conns;/* Iterate through each port to discover topology */do{/* Get a handle on a port */ep=of_graph_get_next_endpoint(node,ep);if(!ep)break;-ret=of_coresight_parse_endpoint(ep,pdata,&i);+ret=of_coresight_parse_endpoint(ep,&conn);if(ret)returnERR_PTR(ret);}while(ep);
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:45:06
Before we update the bindings, document the current graph bindings
and usage of additional properties.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 31 +++++++++++++++++++---
1 file changed, 27 insertions(+), 4 deletions(-)
@@ -52,9 +52,7 @@ its hardware characteristcs. clocks the core of that coresight component. The latter clock is optional.- * port or ports: The representation of the component's port- layout using the generic DT graph presentation found in- "bindings/graph.txt".+ * port or ports: see "Graph bindings for Coresight" below. * Additional required properties for System Trace Macrocells (STM): * reg: along with the physical base address and length of the register
@@ -71,7 +69,7 @@ its hardware characteristcs. AMBA markee): - "arm,coresight-replicator"- * port or ports: same as above.+ * port or ports: see "Graph bindings for Coresight" below. * Optional properties for ETM/PTMs:
@@ -90,6 +88,31 @@ its hardware characteristcs. * arm,scatter-gather: boolean. Indicates that the TMC-ETR can safely use the SG mode on this system.+Graph bindings for Coresight+-------------------------------++Coresight components are interconnected to create a data path for the flow of+trace data generated from the "sources" to their collection points "sink".+Each coresight component must describe the "input" and "output" connections.+The connections must be described via generic DT graph bindings as described+by the "bindings/graph.txt", where each "port" along with an "endpoint"+component represents a hardware port and the connection.++Since it is possible to have multiple connections for any coresight component+with a specific direction of data flow, each connection must define the+following properties to uniquely identify the connection details.++ * Direction of the data flow w.r.t the component :+ Each input port must have the following property defined at the "endpoint"+ for the port.+ "slave-mode"++ * Hardware Port number at the component:+ - The hardware port number is assumed to be the address of the "port"+ component.+++ Example: 1. Sinks
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:45:10
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29 ++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49 +++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
@@ -108,8 +108,13 @@ following properties to uniquely identify the connection details. "slave-mode" * Hardware Port number at the component:- - The hardware port number is assumed to be the address of the "port"- component.+ - Each "endpoint" must define the hardware port of the local end of the+ connection using the following property :++ "coresight,hwid" - 32bit integer, local hardware port.++ - [ ** Obsolete ** ] The hardware port number is assumed to be the address+ of the "port" component.
@@ -128,7 +153,8 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;/* Parse the local port details */-if(of_graph_parse_endpoint(ep,&endpoint))+local_port=of_coresight_endpoint_get_port_id(dev,ep);+if(local_port<0)break;/**Getahandleontheremoteendpointandthedeviceitis
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct device_node *ep,rparent=of_graph_get_port_parent(rep);if(!rparent)break;-if(of_graph_parse_endpoint(rep,&rendpoint))-break;-/* If the remote device is not available, defer probing */rdev=of_coresight_get_endpoint_device(rparent);if(!rdev){
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;}-conn->outport=endpoint.port;+child_port=of_coresight_endpoint_get_port_id(rdev,rep);+if(child_port<0){+ret=0;+break;+}++conn->outport=local_port;conn->child_name=dev_name(rdev);-conn->child_port=rendpoint.port;+conn->child_port=child_port;/* Move the connection record */(*pconn)++;}while(0);
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:45:36
Switch to the new hardware port bindings for coresight
Cc: Andy Gross <redacted>
Cc: David Brown <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm/boot/dts/qcom-apq8064.dtsi | 37 +++++++++++++++++------
arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++----------
2 files changed, 73 insertions(+), 24 deletions(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:47:45
So far we have relied on an undocumented property "slave-mode",
to indicate if the given port is input or not. Since we are
redefining the coresight bindings, define new property for the
"direction" of data flow for a given connection endpoint in the
device.
Each endpoint must define the following property.
- "direction" : 0 => Port is input
1 => Port is output
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 24 ++++++++++++++--------
drivers/hwtracing/coresight/of_coresight.c | 22 ++++++++++++++++----
2 files changed, 34 insertions(+), 12 deletions(-)
@@ -103,9 +103,11 @@ with a specific direction of data flow, each connection must define the following properties to uniquely identify the connection details. * Direction of the data flow w.r.t the component :- Each input port must have the following property defined at the "endpoint"+ Each hardware port must have the following property defined at the "endpoint" for the port.- "slave-mode"+ "direction" - 32bit integer, whose values are defined as follows :+ 0 => the endpoint is an Input port+ 1 => the endpoint is an Output port. * Hardware Port number at the component: - Each "endpoint" must define the hardware port of the local end of the
@@ -149,7 +162,7 @@ static int of_coresight_parse_endpoint(struct device *dev,*Noneedtodealwithinputports,processingforas*processingforoutputportswilldealwiththem.*/-if(of_find_property(ep,"slave-mode",NULL))+if(of_coresight_endpoint_is_input(dev,ep))break;/* Parse the local port details */
@@ -212,7 +225,8 @@ of_get_coresight_platform_data(struct device *dev,pdata->cpu=of_coresight_get_cpu(node);/* Get the number of input and output port for this component */-of_coresight_get_ports(node,&pdata->nr_inport,&pdata->nr_outport);+of_coresight_get_ports(dev,node,+&pdata->nr_inport,&pdata->nr_outport);/* If there are not output connections, we are done */if(!pdata->nr_outport)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:48:37
If we fail to find the input / output port for a LINK component
while enabling a path, we should fail gracefully rather than
assuming port "0".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-05 21:49:25
The coresight driver doesn't drop the references on the
remote endpoint/port nodes. Add the missing of_node_put()
calls. To make it easier to handle different corner cases
cleanly, move the parsing of an endpoint to separate
function.
Reported-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/of_coresight.c | 139 +++++++++++++++++------------
1 file changed, 84 insertions(+), 55 deletions(-)
@@ -111,17 +111,80 @@ int of_coresight_get_cpu(const struct device_node *node)}EXPORT_SYMBOL_GPL(of_coresight_get_cpu);+/*+*of_coresight_parse_endpoint:Parsethegivenoutputendpoint@ep+*andfilltheconnectioninformationin@pdata[*@i].+*+*Parsesthelocalport,remotedevicenameandtheremoteport.Also+*updates*@itopointtothenextindex,whenanentryisadded.+*+*Returns:+*0-Iftheparsingcompletedwithoutanyfatalerrors.+*-Errno-Fatalerror,abortthescanning.+*/+staticintof_coresight_parse_endpoint(structdevice_node*ep,+structcoresight_platform_data*pdata,+int*i)+{+intret=0;+structof_endpointendpoint,rendpoint;+structdevice_node*rparent=NULL;+structdevice_node*rport=NULL;+structdevice*rdev=NULL;++do{+/*+*Noneedtodealwithinputports,processingforas+*processingforoutputportswilldealwiththem.+*/+if(of_find_property(ep,"slave-mode",NULL))+break;++/* Parse the local port details */+if(of_graph_parse_endpoint(ep,&endpoint))+break;+/*+*Getahandleontheremoteportandparent+*attachedtoit.+*/+rparent=of_graph_get_remote_port_parent(ep);+if(!rparent)+break;+rport=of_graph_get_remote_port(ep);+if(!rport)+break;+if(of_graph_parse_endpoint(rport,&rendpoint))+break;++/* If the remote device is not available, defer probing */+rdev=of_coresight_get_endpoint_device(rparent);+if(!rdev){+ret=-EPROBE_DEFER;+break;+}++pdata->outports[*i]=endpoint.port;+pdata->child_names[*i]=dev_name(rdev);+pdata->child_ports[*i]=rendpoint.id;+/* Move the index */+(*i)++;+}while(0);++if(rparent)+of_node_put(rparent);+if(rport)+of_node_put(rport);++returnret;+}+structcoresight_platform_data*of_get_coresight_platform_data(structdevice*dev,conststructdevice_node*node){inti=0,ret=0;structcoresight_platform_data*pdata;-structof_endpointendpoint,rendpoint;-structdevice*rdev;structdevice_node*ep=NULL;-structdevice_node*rparent=NULL;-structdevice_node*rport=NULL;pdata=devm_kzalloc(dev,sizeof(*pdata),GFP_KERNEL);if(!pdata)
@@ -129,63 +192,29 @@ of_get_coresight_platform_data(struct device *dev,/* Use device name as sysfs handle */pdata->name=dev_name(dev);+pdata->cpu=of_coresight_get_cpu(node);/* Get the number of input and output port for this component */of_coresight_get_ports(node,&pdata->nr_inport,&pdata->nr_outport);-if(pdata->nr_outport){-ret=of_coresight_alloc_memory(dev,pdata);+/* If there are not output connections, we are done */+if(!pdata->nr_outport)+returnpdata;++ret=of_coresight_alloc_memory(dev,pdata);+if(ret)+returnERR_PTR(ret);++/* Iterate through each port to discover topology */+do{+/* Get a handle on a port */+ep=of_graph_get_next_endpoint(node,ep);+if(!ep)+break;+ret=of_coresight_parse_endpoint(ep,pdata,&i);if(ret)returnERR_PTR(ret);--/* Iterate through each port to discover topology */-do{-/* Get a handle on a port */-ep=of_graph_get_next_endpoint(node,ep);-if(!ep)-break;--/*-*Noneedtodealwithinputports,processingforas-*processingforoutputportswilldealwiththem.-*/-if(of_find_property(ep,"slave-mode",NULL))-continue;--/* Get a handle on the local endpoint */-ret=of_graph_parse_endpoint(ep,&endpoint);--if(ret)-continue;--/* The local out port number */-pdata->outports[i]=endpoint.port;--/*-*Getahandleontheremoteportandparent-*attachedtoit.-*/-rparent=of_graph_get_remote_port_parent(ep);-rport=of_graph_get_remote_port(ep);--if(!rparent||!rport)-continue;--if(of_graph_parse_endpoint(rport,&rendpoint))-continue;--rdev=of_coresight_get_endpoint_device(rparent);-if(!rdev)-returnERR_PTR(-EPROBE_DEFER);--pdata->child_names[i]=dev_name(rdev);-pdata->child_ports[i]=rendpoint.id;--i++;-}while(ep);-}--pdata->cpu=of_coresight_get_cpu(node);+}while(ep);returnpdata;}
Hi Suzuki,
On Wednesday 06 June 2018 03:13 AM, Suzuki K Poulose wrote:
commit 6403587a930c ("coresight: use put_device() instead of kfree()")
introduced a memory leak where, if we fail to register the device
for coresight_device, we don't free the "coresight_device" object,
which was allocated via kzalloc(). Fix this by jumping to the
appropriate error path.
put_device() will decrement the last reference and then
free the memory by calling dev->release. Internally
put_device() -> kobject_put() -> kobject_cleanup() which is
responsible to call 'dev -> release' and also free other kobject
resources. If you will see the coresight_device_release. There
we are releasing all allocated memory. Still if you call kfree() again
then it'll be redundancy.
~arvind
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2018-06-06 10:16:23
On 06/06/2018 07:44 AM, Arvind Yadav wrote:
Hi Suzuki,
On Wednesday 06 June 2018 03:13 AM, Suzuki K Poulose wrote:
quoted
commit 6403587a930c ("coresight: use put_device() instead of kfree()")
introduced a memory leak where, if we fail to register the device
for coresight_device, we don't free the "coresight_device" object,
which was allocated via kzalloc(). Fix this by jumping to the
appropriate error path.
put_device() will decrement the last reference and then
free the memory by calling dev->release. Internally
put_device() -> kobject_put() -> kobject_cleanup() which is
responsible to call 'dev -> release' and also free other kobject
resources. If you will see the coresight_device_release. There
we are releasing all allocated memory. Still if you call kfree() again
then it'll be redundancy.
You're right. I think it would be good to have a comment explaining this
to prevent this fix popping up in the future :-). I will add it
Thanks
Suzuki
On Tue, Jun 05, 2018 at 10:43:13PM +0100, Suzuki K Poulose wrote:
The coresight driver doesn't drop the references on the
remote endpoint/port nodes. Add the missing of_node_put()
calls. To make it easier to handle different corner cases
cleanly, move the parsing of an endpoint to separate
function.
Please split this as those are two different things.
@@ -111,17 +111,80 @@ int of_coresight_get_cpu(const struct device_node *node)}EXPORT_SYMBOL_GPL(of_coresight_get_cpu);+/*+*of_coresight_parse_endpoint:Parsethegivenoutputendpoint@ep+*andfilltheconnectioninformationin@pdata[*@i].+*+*Parsesthelocalport,remotedevicenameandtheremoteport.Also+*updates*@itopointtothenextindex,whenanentryisadded.+*+*Returns:+*0-Iftheparsingcompletedwithoutanyfatalerrors.+*-Errno-Fatalerror,abortthescanning.+*/+staticintof_coresight_parse_endpoint(structdevice_node*ep,+structcoresight_platform_data*pdata,+int*i)+{+intret=0;+structof_endpointendpoint,rendpoint;+structdevice_node*rparent=NULL;+structdevice_node*rport=NULL;+structdevice*rdev=NULL;++do{+/*+*Noneedtodealwithinputports,processingforas+*processingforoutputportswilldealwiththem.+*/+if(of_find_property(ep,"slave-mode",NULL))+break;++/* Parse the local port details */+if(of_graph_parse_endpoint(ep,&endpoint))+break;+/*+*Getahandleontheremoteportandparent+*attachedtoit.+*/+rparent=of_graph_get_remote_port_parent(ep);+if(!rparent)+break;+rport=of_graph_get_remote_port(ep);+if(!rport)+break;+if(of_graph_parse_endpoint(rport,&rendpoint))+break;++/* If the remote device is not available, defer probing */+rdev=of_coresight_get_endpoint_device(rparent);+if(!rdev){+ret=-EPROBE_DEFER;+break;+}++pdata->outports[*i]=endpoint.port;+pdata->child_names[*i]=dev_name(rdev);+pdata->child_ports[*i]=rendpoint.id;+/* Move the index */+(*i)++;+}while(0);
That's a clever way of coding a classic 'goto' block.
quoted hunk
++ if (rparent)+ of_node_put(rparent);+ if (rport)+ of_node_put(rport);
@@ -129,63 +192,29 @@ of_get_coresight_platform_data(struct device *dev, /* Use device name as sysfs handle */ pdata->name = dev_name(dev);+ pdata->cpu = of_coresight_get_cpu(node); /* Get the number of input and output port for this component */ of_coresight_get_ports(node, &pdata->nr_inport, &pdata->nr_outport);- if (pdata->nr_outport) {- ret = of_coresight_alloc_memory(dev, pdata);+ /* If there are not output connections, we are done */
/not/no
+ if (!pdata->nr_outport)
+ return pdata;
+
+ ret = of_coresight_alloc_memory(dev, pdata);
+ if (ret)
+ return ERR_PTR(ret);
+
+ /* Iterate through each port to discover topology */
+ do {
+ /* Get a handle on a port */
+ ep = of_graph_get_next_endpoint(node, ep);
+ if (!ep)
+ break;
+ ret = of_coresight_parse_endpoint(ep, pdata, &i);
if (ret)
return ERR_PTR(ret);
-
- /* Iterate through each port to discover topology */
- do {
- /* Get a handle on a port */
- ep = of_graph_get_next_endpoint(node, ep);
- if (!ep)
- break;
-
- /*
- * No need to deal with input ports, processing for as
- * processing for output ports will deal with them.
- */
- if (of_find_property(ep, "slave-mode", NULL))
- continue;
-
- /* Get a handle on the local endpoint */
- ret = of_graph_parse_endpoint(ep, &endpoint);
-
- if (ret)
- continue;
-
- /* The local out port number */
- pdata->outports[i] = endpoint.port;
-
- /*
- * Get a handle on the remote port and parent
- * attached to it.
- */
- rparent = of_graph_get_remote_port_parent(ep);
- rport = of_graph_get_remote_port(ep);
-
- if (!rparent || !rport)
- continue;
-
- if (of_graph_parse_endpoint(rport, &rendpoint))
- continue;
-
- rdev = of_coresight_get_endpoint_device(rparent);
- if (!rdev)
- return ERR_PTR(-EPROBE_DEFER);
-
- pdata->child_names[i] = dev_name(rdev);
- pdata->child_ports[i] = rendpoint.id;
-
- i++;
- } while (ep);
- }
-
- pdata->cpu = of_coresight_get_cpu(node);
+ } while (ep);
return pdata;
}
--
2.7.4
On Tue, Jun 05, 2018 at 10:43:14PM +0100, Suzuki K Poulose wrote:
quoted hunk
When parsing the remote endpoint of an output port, we do :
rport = of_graph_get_remote_port(ep);
rparent = of_graph_get_remote_port_parent(ep);
and then parse the "remote_port" as if it was the remote endpoint,
which is wrong. The code worked fine because we used endpoint number
as the port number. Let us fix it and optimise a bit as:
remote_ep = of_graph_get_remote_endpoint(ep);
if (remote_ep)
remote_parent = of_graph_get_port_parent(remote_ep);
and then, parse the remote_ep for the port/endpoint details.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/of_coresight.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
@@ -129,7 +129,7 @@ static int of_coresight_parse_endpoint(struct device_node *ep,intret=0;structof_endpointendpoint,rendpoint;structdevice_node*rparent=NULL;-structdevice_node*rport=NULL;+structdevice_node*rep=NULL;structdevice*rdev=NULL;do{
@@ -144,16 +144,16 @@ static int of_coresight_parse_endpoint(struct device_node *ep,if(of_graph_parse_endpoint(ep,&endpoint))break;/*-*Getahandleontheremoteportandparent-*attachedtoit.+*Getahandleontheremoteendpointandthedeviceitis+*attachedto.*/-rparent=of_graph_get_remote_port_parent(ep);+rep=of_graph_get_remote_endpoint(ep);+if(!rep)+break;+rparent=of_graph_get_port_parent(rep);if(!rparent)break;-rport=of_graph_get_remote_port(ep);-if(!rport)-break;-if(of_graph_parse_endpoint(rport,&rendpoint))+if(of_graph_parse_endpoint(rep,&rendpoint))break;/* If the remote device is not available, defer probing */
@@ -165,15 +165,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep,pdata->outports[*i]=endpoint.port;pdata->child_names[*i]=dev_name(rdev);-pdata->child_ports[*i]=rendpoint.id;+pdata->child_ports[*i]=rendpoint.port;/* Move the index */(*i)++;}while(0);if(rparent)of_node_put(rparent);-if(rport)-of_node_put(rport);+if(rep)+of_node_put(rep);returnret;}
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
(Please add to the next iteration so that I don't have to review again)
On Tue, Jun 05, 2018 at 10:43:16PM +0100, Suzuki K Poulose wrote:
quoted hunk
The platform code parses the component connections and populates
a platform-description of the output connections in arrays of fields
(which is never freed). This is later copied in the coresight_register
to a newly allocated area, represented by coresight_connection(s).
This patch cleans up the code dealing with connections by making
use of the "coresight_connection" structure right at the platform
code and lets the generic driver simply re-use information provided
by the platform.
Thus making it reader friendly as well as avoiding the wastage of
unused memory.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 21 +-----------
drivers/hwtracing/coresight/of_coresight.c | 51 ++++++++++++------------------
include/linux/coresight.h | 9 ++----
3 files changed, 23 insertions(+), 58 deletions(-)
@@ -988,22 +986,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)csdev->nr_inport=desc->pdata->nr_inport;csdev->nr_outport=desc->pdata->nr_outport;-/* Initialise connections if there is at least one outport */-if(csdev->nr_outport){-conns=kcalloc(csdev->nr_outport,sizeof(*conns),GFP_KERNEL);-if(!conns){-ret=-ENOMEM;-gotoerr_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];-}-}--csdev->conns=conns;+csdev->conns=desc->pdata->conns;csdev->type=desc->type;csdev->subtype=desc->subtype;
@@ -70,26 +70,13 @@ static void of_coresight_get_ports(const struct device_node *node,staticintof_coresight_alloc_memory(structdevice*dev,structcoresight_platform_data*pdata){-/* List of output port on this component */-pdata->outports=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->outports),-GFP_KERNEL);-if(!pdata->outports)-return-ENOMEM;--/* Children connected to this component via @outports */-pdata->child_names=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->child_names),-GFP_KERNEL);-if(!pdata->child_names)-return-ENOMEM;--/* Port number on the child this component is connected to */-pdata->child_ports=devm_kzalloc(dev,pdata->nr_outport*-sizeof(*pdata->child_ports),-GFP_KERNEL);-if(!pdata->child_ports)-return-ENOMEM;+if(pdata->nr_outport){+pdata->conns=devm_kzalloc(dev,pdata->nr_outport*+sizeof(*pdata->conns),+GFP_KERNEL);+if(!pdata->conns)+return-ENOMEM;+}return0;}
@@ -163,11 +150,11 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;}-pdata->outports[*i]=endpoint.port;-pdata->child_names[*i]=dev_name(rdev);-pdata->child_ports[*i]=rendpoint.port;-/* Move the index */-(*i)++;+conn->outport=endpoint.port;+conn->child_name=dev_name(rdev);+conn->child_port=rendpoint.port;+/* Move the connection record */+(*pconn)++;}while(0);if(rparent)
@@ -205,13 +193,14 @@ of_get_coresight_platform_data(struct device *dev,if(ret)returnERR_PTR(ret);+conn=pdata->conns;/* Iterate through each port to discover topology */do{/* Get a handle on a port */ep=of_graph_get_next_endpoint(node,ep);if(!ep)break;-ret=of_coresight_parse_endpoint(ep,pdata,&i);+ret=of_coresight_parse_endpoint(ep,&conn);if(ret)returnERR_PTR(ret);}while(ep);
On Tue, Jun 05, 2018 at 10:43:17PM +0100, Suzuki K Poulose wrote:
quoted hunk
If we fail to find the input / output port for a LINK component
while enabling a path, we should fail gracefully rather than
assuming port "0".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
On Tue, Jun 05, 2018 at 10:43:18PM +0100, Suzuki K Poulose wrote:
Before we update the bindings, document the current graph bindings
and usage of additional properties.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
@@ -52,9 +52,7 @@ its hardware characteristcs. clocks the core of that coresight component. The latter clock is optional.- * port or ports: The representation of the component's port- layout using the generic DT graph presentation found in- "bindings/graph.txt".+ * port or ports: see "Graph bindings for Coresight" below. * Additional required properties for System Trace Macrocells (STM): * reg: along with the physical base address and length of the register
@@ -71,7 +69,7 @@ its hardware characteristcs. AMBA markee): - "arm,coresight-replicator"- * port or ports: same as above.+ * port or ports: see "Graph bindings for Coresight" below. * Optional properties for ETM/PTMs:
@@ -90,6 +88,31 @@ its hardware characteristcs. * arm,scatter-gather: boolean. Indicates that the TMC-ETR can safely use the SG mode on this system.+Graph bindings for Coresight+-------------------------------++Coresight components are interconnected to create a data path for the flow of+trace data generated from the "sources" to their collection points "sink".+Each coresight component must describe the "input" and "output" connections.+The connections must be described via generic DT graph bindings as described+by the "bindings/graph.txt", where each "port" along with an "endpoint"+component represents a hardware port and the connection.++Since it is possible to have multiple connections for any coresight component+with a specific direction of data flow, each connection must define the+following properties to uniquely identify the connection details.++ * Direction of the data flow w.r.t the component :+ Each input port must have the following property defined at the "endpoint"+ for the port.+ "slave-mode"++ * Hardware Port number at the component:+ - The hardware port number is assumed to be the address of the "port"+ component.+++ Example: 1. Sinks
On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
quoted hunk
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29 ++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49 +++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
@@ -108,8 +108,13 @@ following properties to uniquely identify the connection details. "slave-mode" * Hardware Port number at the component:- - The hardware port number is assumed to be the address of the "port"- component.+ - Each "endpoint" must define the hardware port of the local end of the+ connection using the following property :++ "coresight,hwid" - 32bit integer, local hardware port.++ - [ ** Obsolete ** ] The hardware port number is assumed to be the address+ of the "port" component.
@@ -128,7 +153,8 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;/* Parse the local port details */-if(of_graph_parse_endpoint(ep,&endpoint))+local_port=of_coresight_endpoint_get_port_id(dev,ep);+if(local_port<0)break;/**Getahandleontheremoteendpointandthedeviceitis
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct device_node *ep,rparent=of_graph_get_port_parent(rep);if(!rparent)break;-if(of_graph_parse_endpoint(rep,&rendpoint))-break;-/* If the remote device is not available, defer probing */rdev=of_coresight_get_endpoint_device(rparent);if(!rdev){
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep,break;}-conn->outport=endpoint.port;+child_port=of_coresight_endpoint_get_port_id(rdev,rep);+if(child_port<0){+ret=0;
Why returning '0' on an error condition? Same for 'local_port' above.
quoted hunk
+ break;+ }++ conn->outport = local_port; conn->child_name = dev_name(rdev);- conn->child_port = rendpoint.port;+ conn->child_port = child_port; /* Move the connection record */ (*pconn)++; } while (0);
@@ -200,7 +229,7 @@ of_get_coresight_platform_data(struct device *dev, ep = of_graph_get_next_endpoint(node, ep); if (!ep) break;- ret = of_coresight_parse_endpoint(ep, &conn);+ ret = of_coresight_parse_endpoint(dev, ep, &conn); if (ret) return ERR_PTR(ret); } while (ep);
On Tue, Jun 05, 2018 at 10:43:20PM +0100, Suzuki K Poulose wrote:
quoted hunk
So far we have relied on an undocumented property "slave-mode",
to indicate if the given port is input or not. Since we are
redefining the coresight bindings, define new property for the
"direction" of data flow for a given connection endpoint in the
device.
Each endpoint must define the following property.
- "direction" : 0 => Port is input
1 => Port is output
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 24 ++++++++++++++--------
drivers/hwtracing/coresight/of_coresight.c | 22 ++++++++++++++++----
2 files changed, 34 insertions(+), 12 deletions(-)
@@ -103,9 +103,11 @@ with a specific direction of data flow, each connection must define the following properties to uniquely identify the connection details. * Direction of the data flow w.r.t the component :- Each input port must have the following property defined at the "endpoint"+ Each hardware port must have the following property defined at the "endpoint" for the port.- "slave-mode"+ "direction" - 32bit integer, whose values are defined as follows :+ 0 => the endpoint is an Input port+ 1 => the endpoint is an Output port. * Hardware Port number at the component: - Each "endpoint" must define the hardware port of the local end of the
@@ -149,7 +162,7 @@ static int of_coresight_parse_endpoint(struct device *dev,*Noneedtodealwithinputports,processingforas*processingforoutputportswilldealwiththem.*/-if(of_find_property(ep,"slave-mode",NULL))+if(of_coresight_endpoint_is_input(dev,ep))break;/* Parse the local port details */
@@ -212,7 +225,8 @@ of_get_coresight_platform_data(struct device *dev,pdata->cpu=of_coresight_get_cpu(node);/* Get the number of input and output port for this component */-of_coresight_get_ports(node,&pdata->nr_inport,&pdata->nr_outport);+of_coresight_get_ports(dev,node,+&pdata->nr_inport,&pdata->nr_outport);/* If there are not output connections, we are done */if(!pdata->nr_outport)
For both the binding and the code:
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2018-06-11 09:18:31
On 08/06/18 20:55, Mathieu Poirier wrote:
On Tue, Jun 05, 2018 at 10:43:13PM +0100, Suzuki K Poulose wrote:
quoted
The coresight driver doesn't drop the references on the
remote endpoint/port nodes. Add the missing of_node_put()
calls. To make it easier to handle different corner cases
cleanly, move the parsing of an endpoint to separate
function.
Please split this as those are two different things.
Mathieu,
I can do that, its only that if someone were to backport
the fix for an older kernel, they would need to pick up these
two patches, which is still fine.
+static int of_coresight_parse_endpoint(struct device_node *ep,+ struct coresight_platform_data *pdata,+ int *i)+{+ int ret = 0;+ struct of_endpoint endpoint, rendpoint;+ struct device_node *rparent = NULL;+ struct device_node *rport = NULL;+ struct device *rdev = NULL;++ do {
...
quoted
+ } while (0);
That's a clever way of coding a classic 'goto' block.
quoted
++ if (rparent)+ of_node_put(rparent);+ if (rport)+ of_node_put(rport);
Perfect - thank you for that.
quoted
pdata->name = dev_name(dev);+ pdata->cpu = of_coresight_get_cpu(node); /* Get the number of input and output port for this component */ of_coresight_get_ports(node, &pdata->nr_inport, &pdata->nr_outport);- if (pdata->nr_outport) {- ret = of_coresight_alloc_memory(dev, pdata);+ /* If there are not output connections, we are done */
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2018-06-11 09:24:39
On 08/06/18 22:22, Mathieu Poirier wrote:
On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
quoted
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29 ++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49 +++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
+/* * of_coresight_parse_endpoint : Parse the given output endpoint @ep * and fill the connection information in *@pconn. *
@@ -109,11 +134,11 @@ EXPORT_SYMBOL_GPL(of_coresight_get_cpu); * 0 - If the parsing completed without any fatal errors.
Please note the return value description here. Further comments below.
quoted
* -Errno - Fatal error, abort the scanning. */-static int of_coresight_parse_endpoint(struct device_node *ep,+static int of_coresight_parse_endpoint(struct device *dev,+ struct device_node *ep, struct coresight_connection **pconn) {- int ret = 0;- struct of_endpoint endpoint, rendpoint;+ int ret = 0, local_port, child_port; struct device_node *rparent = NULL; struct device_node *rep = NULL; struct device *rdev = NULL;
@@ -128,7 +153,8 @@ static int of_coresight_parse_endpoint(struct device_node *ep, break; /* Parse the local port details */- if (of_graph_parse_endpoint(ep, &endpoint))+ local_port = of_coresight_endpoint_get_port_id(dev, ep);+ if (local_port < 0) break; /* * Get a handle on the remote endpoint and the device it is
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct device_node *ep, rparent = of_graph_get_port_parent(rep); if (!rparent) break;- if (of_graph_parse_endpoint(rep, &rendpoint))- break;- /* If the remote device is not available, defer probing */ rdev = of_coresight_get_endpoint_device(rparent); if (!rdev) {
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct device_node *ep, break; }- conn->outport = endpoint.port;+ child_port = of_coresight_endpoint_get_port_id(rdev, rep);+ if (child_port < 0) {+ ret = 0;
Why returning '0' on an error condition? Same for 'local_port' above.
If we are unable to parse a port, we can simply ignore the port and continue, which
is what we have today with the existing code. I didn't change it and still think
it is the best effort thing. We could spit a warning for such cases, if really needed.
Also, the parsing code almost never fails at the moment. If it fails to find "reg" field,
it is assumed to be '0'. Either way ignoring it seems harmless. That said I am open
to suggestions.
Cheers
Suzuki
On 11 June 2018 at 03:22, Suzuki K Poulose [off-list ref] wrote:
On 08/06/18 22:22, Mathieu Poirier wrote:
quoted
On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
quoted
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29 ++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49
+++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
+/* * of_coresight_parse_endpoint : Parse the given output endpoint @ep * and fill the connection information in *@pconn. *
@@ -109,11 +134,11 @@ EXPORT_SYMBOL_GPL(of_coresight_get_cpu); * 0 - If the parsing completed without any fatal errors.
Please note the return value description here. Further comments below.
quoted
quoted
* -Errno - Fatal error, abort the scanning.
*/
-static int of_coresight_parse_endpoint(struct device_node *ep,
+static int of_coresight_parse_endpoint(struct device *dev,
+ struct device_node *ep,
struct coresight_connection
**pconn)
{
- int ret = 0;
- struct of_endpoint endpoint, rendpoint;
+ int ret = 0, local_port, child_port;
struct device_node *rparent = NULL;
struct device_node *rep = NULL;
struct device *rdev = NULL;
@@ -128,7 +153,8 @@ static int of_coresight_parse_endpoint(struct
device_node *ep,
break;
/* Parse the local port details */
- if (of_graph_parse_endpoint(ep, &endpoint))
+ local_port = of_coresight_endpoint_get_port_id(dev, ep);
+ if (local_port < 0)
break;
/*
* Get a handle on the remote endpoint and the device it
is
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct
device_node *ep,
rparent = of_graph_get_port_parent(rep);
if (!rparent)
break;
- if (of_graph_parse_endpoint(rep, &rendpoint))
- break;
-
/* If the remote device is not available, defer probing
*/
rdev = of_coresight_get_endpoint_device(rparent);
if (!rdev) {
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct
Why returning '0' on an error condition? Same for 'local_port' above.
If we are unable to parse a port, we can simply ignore the port and
continue, which
is what we have today with the existing code. I didn't change it and still
think
it is the best effort thing. We could spit a warning for such cases, if
really needed.
Also, the parsing code almost never fails at the moment. If it fails to find
"reg" field,
it is assumed to be '0'. Either way ignoring it seems harmless. That said I
am open
to suggestions.
Looking at the original code I remember not mandating enpoints to be
valid for debugging purposes. That certainly helps when building up a
device tree file but also has the side effect of silently overlooking
specification problems. Fortunately the revamping you did on that
part of the code makes it very easy to change that, something I think
we should take advantage of (it can only lead to positive scenarios
where defective specifications get pointed out).
That being said and because the original behaviour is just as
permissive, you can leave as is.
Thanks,
Mathieu
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2018-06-11 16:55:36
On 11/06/18 17:52, Mathieu Poirier wrote:
On 11 June 2018 at 03:22, Suzuki K Poulose [off-list ref] wrote:
quoted
On 08/06/18 22:22, Mathieu Poirier wrote:
quoted
On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
quoted
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29 ++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49
+++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
@@ -108,8 +108,13 @@ following properties to uniquely identify the
connection details.
"slave-mode"
quoted
quoted
};
For the binding part:
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
...
quoted
quoted
quoted
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct
device_node *ep,
rparent = of_graph_get_port_parent(rep);
if (!rparent)
break;
- if (of_graph_parse_endpoint(rep, &rendpoint))
- break;
-
/* If the remote device is not available, defer probing
*/
rdev = of_coresight_get_endpoint_device(rparent);
if (!rdev) {
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct
Why returning '0' on an error condition? Same for 'local_port' above.
If we are unable to parse a port, we can simply ignore the port and
continue, which
is what we have today with the existing code. I didn't change it and still
think
it is the best effort thing. We could spit a warning for such cases, if
really needed.
Also, the parsing code almost never fails at the moment. If it fails to find
"reg" field,
it is assumed to be '0'. Either way ignoring it seems harmless. That said I
am open
to suggestions.
Looking at the original code I remember not mandating enpoints to be
valid for debugging purposes. That certainly helps when building up a
device tree file but also has the side effect of silently overlooking
specification problems. Fortunately the revamping you did on that
part of the code makes it very easy to change that, something I think
we should take advantage of (it can only lead to positive scenarios
where defective specifications get pointed out).
That being said and because the original behaviour is just as
permissive, you can leave as is.
Thanks. So can I assume the Reviewed-by applies for the code now ?
Suzuki
On 11 June 2018 at 10:55, Suzuki K Poulose [off-list ref] wrote:
On 11/06/18 17:52, Mathieu Poirier wrote:
quoted
On 11 June 2018 at 03:22, Suzuki K Poulose [off-list ref] wrote:
quoted
On 08/06/18 22:22, Mathieu Poirier wrote:
quoted
On Tue, Jun 05, 2018 at 10:43:19PM +0100, Suzuki K Poulose wrote:
quoted
The coresight drivers relied on default bindings for graph
in DT, while reusing the "reg" field of the "ports" to indicate
the actual hardware port number for the connections. However,
with the rules getting stricter w.r.t to the address mismatch
with the label, it is no longer possible to use the port address
field for the hardware port number. Hence, we add an explicit
property to denote the hardware port number, "coresight,hwid"
which must be specified for each "endpoint".
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Sudeep Holla <redacted>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
.../devicetree/bindings/arm/coresight.txt | 29
++++++++++---
drivers/hwtracing/coresight/of_coresight.c | 49
+++++++++++++++++-----
2 files changed, 62 insertions(+), 16 deletions(-)
@@ -108,8 +108,13 @@ following properties to uniquely identify the
connection details.
"slave-mode"
quoted
quoted
};
For the binding part:
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
...
quoted
quoted
quoted
quoted
@@ -140,9 +166,6 @@ static int of_coresight_parse_endpoint(struct
device_node *ep,
rparent = of_graph_get_port_parent(rep);
if (!rparent)
break;
- if (of_graph_parse_endpoint(rep, &rendpoint))
- break;
-
/* If the remote device is not available, defer
probing
*/
rdev = of_coresight_get_endpoint_device(rparent);
if (!rdev) {
@@ -150,9 +173,15 @@ static int of_coresight_parse_endpoint(struct
Why returning '0' on an error condition? Same for 'local_port' above.
If we are unable to parse a port, we can simply ignore the port and
continue, which
is what we have today with the existing code. I didn't change it and
still
think
it is the best effort thing. We could spit a warning for such cases, if
really needed.
Also, the parsing code almost never fails at the moment. If it fails to
find
"reg" field,
it is assumed to be '0'. Either way ignoring it seems harmless. That said
I
am open
to suggestions.
Looking at the original code I remember not mandating enpoints to be
valid for debugging purposes. That certainly helps when building up a
device tree file but also has the side effect of silently overlooking
specification problems. Fortunately the revamping you did on that
part of the code makes it very easy to change that, something I think
we should take advantage of (it can only lead to positive scenarios
where defective specifications get pointed out).
That being said and because the original behaviour is just as
permissive, you can leave as is.
Thanks. So can I assume the Reviewed-by applies for the code now ?
Sudeep please hold on before applying this as there is more work to be
done on this set.
Sure, I plan to apply the DT changes only after the driver changes are
queued by you or whichever tree it's channelled through. I was already
told the same to Suzuki.
--
Regards,
Sudeep
On Mon, 18 Jun 2018 at 20:13, Shawn Guo [off-list ref] wrote:
Hi Stefan,
Can you take a look at the patch? Thanks.
These bindings are still being discussed and patches related to them
shouldn't be merged. The next iteration of this patchset will not
included individual modifications to device tree files, that will be
left for a later time when bindings have been agreed upon.
Thanks,
Mathieu
Shawn
On Tue, Jun 05, 2018 at 10:43:26PM +0100, Suzuki K Poulose wrote:
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2018-06-20 09:53:36
On 05/06/18 22:43, Suzuki K Poulose wrote:
Coresight uses DT graph bindings to describe the connections of the
components. However we have some undocumented usage of the bindings
to describe some of the properties of the connections.
The coresight driver needs to know the hardware ports invovled
in the connection and the direction of data flow to effectively
manage the trace sessions. So far we have relied on the "port"
address (as described by the generic graph bindings) to represent
the hardware port of the component for a connection.
...
There were three options considered for the hardware port number scheme:
...
3) Use explicit properties (implemented in the series) for the hardware
port id and direction. We define a new property "coresight,hwid" for
each endpoint in coresight devices to specify the hardware port number
explicitly. Also use a separate property "direction" to specify the
direction of the data flow.
e.g,
port@0{
reg = <0>;
endpoint {
direction = <1>; // Output
coresight,hwid = <0>; // Port # 0
}
};
port@1{
reg = <1>;
endpoint {
direction = <0>; // Input
coresight,hwid = <0>; // Port # 0
};
};
Pros:
- The bindings are formal and reader friendly, and less prone to errors.
Cons:
- Backward compatibility is lost.
This series implements Option (3) listed above and falls back to the old
bindings if the new bindings are not available. This allows the systems
with old bindings work with the new driver. The driver now issues a warning
(once) when it encounters the old bindings.
....
dts: juno: Update coresight bindings for hw port
dts: hisilicon: Update coresight bindings for hw ports
dts: spreadtrum: Update coresight bindings for hw ports
dts: qcom: Update coresight bindings for hw ports
dts: arm: hisilicon: Update coresight bindings for hardware port
dts: arm: imx7{d,s}: Update coresight binding for hardware ports
dts: arm: omap: Update coresight bindings for hardware ports
dts: arm: qcom: Update coresight bindings for hardware ports
dts: sama5d2: Update coresight bindings for hardware ports
dts: ste-dbx5x0: Update coresight bindings for hardware port
dts: tc2: Update coresight bindings for hardware ports
All,
Pleas hold on with applying the DTS changes listed above. There are
still some on going discussions on the bindings and we are yet to
come to a conclusion [0]. And there are high chances that these might
change. Sorry for the inconvenience.
[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-June/582269.html
Kind regards
Suzuki
On Tue, Jun 5, 2018 at 11:45 PM Suzuki K Poulose [off-list ref] wrote:
Switch to the new coresight bindings
Cc: Linus Walleij <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
I think I was requested in another mail to hold back applying this
patch until it's been discussed.
I will apply once Mathieu (etc) ACKs the series.
Yours,
Linus Walleij
From: Suzuki K Poulose <Suzuki.Poulose@arm.com> Date: 2018-06-26 09:32:32
On 26/06/18 10:30, Linus Walleij wrote:
On Tue, Jun 5, 2018 at 11:45 PM Suzuki K Poulose [off-list ref] wrote:
quoted
Switch to the new coresight bindings
Cc: Linus Walleij <redacted>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
I think I was requested in another mail to hold back applying this
patch until it's been discussed.
I will apply once Mathieu (etc) ACKs the series.
Linus,
Yes, thats right. Please ignore this for now.
Suzuki