Hi Mathieu,

On Thu, 3 Oct 2019 at 23:02, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Thu, 3 Oct 2019 at 15:59, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

On Thu, Oct 03, 2019 at 02:07:51PM -0600, Mathieu Poirier wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

Coresight device connections are a bit complicated and is not exposed currently to the user. One has to look at the platform descriptions (DT bindings or ACPI bindings) to make an understanding. Given the new naming scheme, it will be helpful to have this information to choose the appropriate devices for tracing. This patch exposes the device connections via links in the sysfs directories.

e.g, for a connection devA[OutputPort_X] -> devB[InputPort_Y] is represented as two symlinks:

/sys/bus/coresight/.../devA/out:X -> /sys/bus/coresight/.../devB /sys/bus/coresight/.../devB/in:Y -> /sys/bus/coresight/.../devA

Provides default connections group if requested for any CoreSight device using the coresight_connections structure.

If a connections group is present on a device then links are collated there.

Co-developed-by: Mike Leach mike.leach@linaro.org Signed-off-by: Mike Leach mike.leach@linaro.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com

---

drivers/hwtracing/coresight/coresight-priv.h | 7 + drivers/hwtracing/coresight/coresight-sysfs.c | 150 ++++++++++++++++++ drivers/hwtracing/coresight/coresight.c | 42 +++-- include/linux/coresight.h | 7 + 4 files changed, 195 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index fa4b02ed3900..120928db426a 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -155,6 +155,13 @@ struct list_head *coresight_build_path(struct coresight_device *csdev, void coresight_release_path(struct list_head *path); int coresight_add_sysfs_link(struct coresight_sysfs_link *info); void coresight_remove_sysfs_link(struct coresight_sysfs_link *info); +int coresight_make_links(struct coresight_device *orig,

•                  struct coresight_connection *conn,
  

•                  struct coresight_device *target);
  

+void coresight_remove_links(struct coresight_device *orig,

•                     struct coresight_connection *conn);
  

+int coresight_create_conns_sysfs_group(struct coresight_device *csdev); +void coresight_remove_conns_sysfs_group(struct coresight_device *csdev);

#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X extern int etm_readl_cp14(u32 off, unsigned int *val); diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c index 058a092ba75f..f2ece44e2158 100644 --- a/drivers/hwtracing/coresight/coresight-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-sysfs.c @@ -80,3 +80,153 @@ void coresight_remove_sysfs_link(struct coresight_sysfs_link *info) info->target_name, info->target->sysfs_link_grp); }

+/*

• • coresight_make_links: Make a link for a connection from a @orig
• • device to @target, represented by @conn.
• • e.g, for devOrig[output_X] -> devTarget[input_Y] is represented
• • as two symbolic links :
• • /sys/.../devOrig/out:X -> /sys/.../devTarget/
• • /sys/.../devTarget/in:Y -> /sys/.../devOrig/
• • The link names are allocated for a device where it appears. i.e, the
• • "out" link on the master and "in" link on the slave device.
• • The link info is stored in the connection record for avoiding
• • the reconstruction of names for removal.
• */

+int coresight_make_links(struct coresight_device *orig,

•                  struct coresight_connection *conn,
  

•                  struct coresight_device *target)
  

+{

• int ret = -ENOMEM;
  

• char *outs = NULL, *ins = NULL;
  

• struct coresight_sysfs_link *link = NULL;
  

• do {
  

•         outs = devm_kasprintf(&orig->dev, GFP_KERNEL,
  

•                               "out:%d", conn->outport);
  

•         if (!outs)
  

•                 break;
  

•         ins = devm_kasprintf(&target->dev, GFP_KERNEL,
  

•                              "in:%d", conn->child_port);
  

•         if (!ins)
  

•                 break;
  

•         link = devm_kzalloc(&orig->dev,
  

•                             sizeof(struct coresight_sysfs_link),
  

•                             GFP_KERNEL);
  

•         if (!link)
  

•                 break;
  

•         link->orig = orig;
  

•         link->target = target;
  

•         link->orig_name = outs;
  

•         link->target_name = ins;
  

•         ret = coresight_add_sysfs_link(link);
  

•         if (ret)
  

•                 break;
  

•         conn->link = link;
  

•         /*
  

•          * Install the device connection. This also indicates that
  

•          * the links are operational on both ends.
  

•          */
  

•         conn->child_dev = target;
  

•         return 0;
  

• } while (0);
  

• if (outs)
  

•         devm_kfree(&orig->dev, outs);
  

• if (ins)
  

•         devm_kfree(&target->dev, ins);
  

• if (link)
  

•         devm_kfree(&orig->dev, link);
  

We should have to worry about freeing things if making a link fails. Device probing should fair and the memory released automatically since devm_kasprintf() was used.

Let me try thing sentence again:

We should not have to worry about freeing things if making a link fails. Device proving should fail and...

Knew what you meant - fixed in new version

...

...

• return ret;
  

+}

+/*

• • coresight_remove_links: Remove the sysfs links for a given connection @conn,
• • from @orig device to @target device. See coresight_make_links() for more
• • details.
• */

+void coresight_remove_links(struct coresight_device *orig,

•                     struct coresight_connection *conn)
  

+{

• if (!orig || !conn->link)
  

•         return;
  

• coresight_remove_sysfs_link(conn->link);
  

• devm_kfree(&conn->child_dev->dev, conn->link->target_name);
  

• devm_kfree(&orig->dev, conn->link->orig_name);
  

• devm_kfree(&orig->dev, conn->link);
  

• conn->link = NULL;
  

• conn->child_dev = NULL;
  

+}

+static ssize_t nr_outports_show(struct device *dev,

•                         struct device_attribute *attr,
  

•                         char *buf)
  

+{

• struct coresight_device *csdev = to_coresight_device(dev);
  

• return scnprintf(buf, PAGE_SIZE, "%d\n", csdev->pdata->nr_outport);
  

+} +static DEVICE_ATTR_RO(nr_outports);

+static ssize_t nr_inports_show(struct device *dev,

•                        struct device_attribute *attr,
  

•                        char *buf)
  

+{

• struct coresight_device *csdev = to_coresight_device(dev);
  

• return scnprintf(buf, PAGE_SIZE, "%d\n", csdev->pdata->nr_inport);
  

+} +static DEVICE_ATTR_RO(nr_inports);

+static struct attribute *coresight_conns_attrs[] = {

• &dev_attr_nr_outports.attr,
  

• &dev_attr_nr_inports.attr,
  

• NULL,
  

+};

+static struct attribute_group coresight_conns_group = {

• .attrs = coresight_conns_attrs,
  

• .name = "connections",
  

+};

Since those are present for all CS device, I would put this in the coresight_dev_type[] for all the CS devices, except etc. By proceeding this way we don't need coresight_create_conns_sysfs_group() and coresight_remove_conns_sysfs_group().

With the re-structuring following moving the CTI support into this set, then the need for this feature has changed. That said - the new version calls add_group during register_coresight_device() which keeps all the new connections code nicely encapsulated and avoids changing all the specific coresight device code. I like the fact that the common code is in a common place - better for maintenence in my view. I am of course open to suggestions based on v2 of the set.

...

...

+/*

• • Create connections group for CoreSight devices on the trace path
• • which use inports and outports to represent connections.
• • This group will then be used to collate the sysfs links between
• • devices.
• */

+int coresight_create_conns_sysfs_group(struct coresight_device *csdev) +{

• int ret = 0;
  

• if (!csdev)
  

•         return -EINVAL;
  

• ret = sysfs_create_group(&csdev->dev.kobj, &coresight_conns_group);
  

• if (ret)
  

•         return ret;
  

• csdev->sysfs_link_grp = coresight_conns_group.name;
  

• csdev->default_conns_grp = true;
  

• return ret;
  

+}

+void coresight_remove_conns_sysfs_group(struct coresight_device *csdev) +{

• if (!csdev)
  

•         return;
  

• if (csdev->sysfs_link_grp && csdev->default_conns_grp) {
  

•         sysfs_remove_group(&csdev->dev.kobj, &coresight_conns_group);
  

•         csdev->sysfs_link_grp = NULL;
  

• }
  

+} diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 1e75cc7203b5..e6e1850a6e96 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -1034,7 +1034,7 @@ static void coresight_device_release(struct device *dev)

static int coresight_orphan_match(struct device *dev, void *data) {

• int i;
  

• int i, ret = 0;
  bool still_orphan = false;
  struct coresight_device *csdev, *i_csdev;
  struct coresight_connection *conn;
  

@@ -1059,19 +1059,23 @@ static int coresight_orphan_match(struct device *dev, void *data) /* We have found at least one orphan connection */ if (conn->child_dev == NULL) { /* Does it match this newly added device? */

•                 if (conn->child_fwnode == csdev->dev.fwnode)
  

•                         conn->child_dev = csdev;
  

•                 else
  

•                 if (conn->child_fwnode == csdev->dev.fwnode) {
  

•                         ret = coresight_make_links(i_csdev,
  

•                                                    conn, csdev);
  

•                         if (ret)
  

•                                 return ret;
  

•                 } else {
                          /* This component still has an orphan */
                          still_orphan = true;
  

•                 }
          }
  }
  
  i_csdev->orphan = still_orphan;
  
  /*
  

•  * Returning '0' ensures that all known component on the
  

•  * bus will be checked.
  

•  * Returning '0' in case we didn't encounter any error,
  

•  * ensures that all known component on the bus will be checked.
   */
  return 0;
  

} @@ -1085,7 +1089,7 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)

static int coresight_fixup_device_conns(struct coresight_device *csdev) {

• int i;
  

• int i, ret = 0;
  
  for (i = 0; i < csdev->pdata->nr_outport; i++) {
          struct coresight_connection *conn = &csdev->pdata->conns[i];
  

@@ -1093,9 +1097,12 @@ static int coresight_fixup_device_conns(struct coresight_device *csdev)

          dev = bus_find_device_by_fwnode(&coresight_bustype, conn->child_fwnode);
          if (dev) {

•                 conn->child_dev = to_coresight_device(dev);
  

•                 ret = coresight_make_links(csdev, conn,
  

•                                            to_coresight_device(dev));
                  /* and put reference from 'bus_find_device()' */
                  put_device(dev);
  

•                 if (ret)
  

•                         break;
          } else {
                  csdev->orphan = true;
                  conn->child_dev = NULL;
  

@@ -1130,7 +1137,7 @@ static int coresight_remove_match(struct device *dev, void *data)

          if (csdev->dev.fwnode == conn->child_fwnode) {
                  iterator->orphan = true;

•                 conn->child_dev = NULL;
  

•                 coresight_remove_links(iterator, conn);
                  /*
                   * Drop the reference to the handle for the remote
                   * device acquired in parsing the connections from
  

@@ -1224,13 +1231,23 @@ void coresight_release_platform_data(struct coresight_device *csdev, struct coresight_platform_data *pdata) { int i;

• struct coresight_connection *conns = pdata->conns;
  
  for (i = 0; i < pdata->nr_outport; i++) {
  

•         if (pdata->conns[i].child_fwnode) {
  

•                 fwnode_handle_put(pdata->conns[i].child_fwnode);
  

•         /* If we have made the links, remove them now */
  

•         if (csdev && conns[i].child_dev)
  

•                 coresight_remove_links(csdev, &conns[i]);
  

•         /*
  

•          * Drop the refcount and clear the handle as this device
  

•          * is going away
  

•          */
  

•         if (conns[i].child_fwnode) {
  

•                 fwnode_handle_put(conns[i].child_fwnode);
                  pdata->conns[i].child_fwnode = NULL;
          }
  }
  

• if (csdev)
  

•         coresight_remove_conns_sysfs_group(csdev);
  

}

struct coresight_device *coresight_register(struct coresight_desc *desc) @@ -1312,6 +1329,9 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)

  mutex_lock(&coresight_mutex);

• if (desc->add_conns_group)
  

•         ret = coresight_create_conns_sysfs_group(csdev);
  

• ret = coresight_fixup_device_conns(csdev);
  if (!ret)
          ret = coresight_fixup_orphan_conns(csdev);
  

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index cd0d92611be6..95634d4ce7ff 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -122,6 +122,8 @@ struct coresight_platform_data {

• @groups: operations specific to this component. These will end up

•       in the component's sysfs sub-directory.
  

• @name: name for the coresight device, also shown under sysfs.

• • @add_conns_group: Dynamically add default connections group to the supplied

• •        groups.
    

  */

struct coresight_desc { enum coresight_dev_type type; @@ -131,6 +133,7 @@ struct coresight_desc { struct device *dev; const struct attribute_group **groups; const char *name;

• bool add_conns_group;
  

I don't think we need this since displaying connection information should be mandatory.

Indeed - the new version drops this and enforces a connections dir at coresight_register.

...

...

};

/** @@ -140,12 +143,14 @@ struct coresight_desc {

• @chid_fwnode: remote component's fwnode handle.
• @child_dev: a @coresight_device representation of the component connected to @outport.

• • @link: Representation of the connection as a sysfs link.
  */

struct coresight_connection { int outport; int child_port; struct fwnode_handle *child_fwnode; struct coresight_device *child_dev;

• struct coresight_sysfs_link *link;
  

};

/** @@ -182,6 +187,7 @@ struct coresight_sysfs_link {

• @sysfs_link_grp: Optional name for a group created to add sysfs links into

•       on this device. Group must have been created on device
  

•       registration.
  

• • @default_conns_grp: Added a default connections group.
  */

struct coresight_device { struct coresight_platform_data *pdata; @@ -198,6 +204,7 @@ struct coresight_device { /* cross trigger handling */ struct coresight_device *ect_dev; const char *sysfs_link_grp;

• bool default_conns_grp;
  

Same here.

This actually stays so the error release path in the next version does not remove a group that was never created.

However - best judged in the context of the reworked code.

...

...

};

/*

2.17.1

Thanks for looking at this. The reworked version is somewhat different in the logic that creates the group - as it is on registration of all devices - rather than have separate paths for CTI - due largely to the changes to CTI and moving the CTI specifics into this set. I think the result is far more consistent and understandable - but I'll let you be the judge of that once the next version comes out. I had hoped this might be today, but a late revision meant I was unable to test the latest builds - so as I am off for a few days it will be next week. Not an Issue I think as this is dependent on the CTI set and unlikely to make the 5.4 release anyway.

Regards

Mike