Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230404155121.1824126-1-james.clark@arm.com> <20230404155121.1824126-10-james.clark@arm.com>
In-Reply-To: <20230404155121.1824126-10-james.clark@arm.com>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Wed, 12 Apr 2023 16:17:31 +0100
Message-ID: <CAJ9a7VggC3Xc8=1n=rohVc1Z30sCwWbeT0wzvW0qStZhM0bK=w@mail.gmail.com>
Subject: Re: [PATCH v5 09/13] coresight: Store in-connections as well as out-connections
To:     James Clark <james.clark@arm.com>
Cc:     coresight@lists.linaro.org, quic_jinlmao@quicinc.com,
        suzuki.poulose@arm.com,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 4 Apr 2023 at 16:52, James Clark <james.clark@arm.com> wrote:
>
> This will allow CATU to get its associated ETR in a generic way where
> currently the enable path has some hard coded searches which avoid
> the need to store input connections.
>
> This also means that the full search for connected devices on removal
> can be replaced with a loop through only the input and output devices.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-core.c  | 81 +++++++++----------
>  .../hwtracing/coresight/coresight-platform.c  | 31 ++++++-
>  drivers/hwtracing/coresight/coresight-sysfs.c |  7 --
>  include/linux/coresight.h                     | 26 ++++++
>  4 files changed, 95 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 8d377a59e0be..a0a0ea2c626b 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1349,6 +1349,17 @@ static int coresight_orphan_match(struct device *dev, void *data)
>                         ret = coresight_make_links(src_csdev, conn, dst_csdev);
>                         if (ret)
>                                 return ret;
> +
> +                       /*
> +                        * Install the device connection. This also indicates that
> +                        * the links are operational on both ends.
> +                        */
> +                       conn->dest_dev = dst_csdev;
> +                       conn->src_dev = src_csdev;
> +
> +                       ret = coresight_add_in_conn(conn);
> +                       if (ret)
> +                               return ret;
>                 } else {
>                         /* This component still has an orphan */
>                         still_orphan = true;
> @@ -1370,58 +1381,43 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)
>                          csdev, coresight_orphan_match);
>  }
>
> -static int coresight_remove_match(struct device *dev, void *data)
> +/* coresight_remove_conns - Remove other device's references to this device */
> +static void coresight_remove_conns(struct coresight_device *csdev)
>  {
> -       int i;
> -       struct coresight_device *csdev, *iterator;
> +       int i, j;
>         struct coresight_connection *conn;
>
> -       csdev = data;
> -       iterator = to_coresight_device(dev);
> -
> -       /* No need to check oneself */
> -       if (csdev == iterator)
> -               return 0;
> -
>         /*
> -        * Circle throuch all the connection of that component.  If we find
> -        * a connection whose name matches @csdev, remove it.
> +        * Remove the input connection references from the destination device
> +        * for each output connection.
>          */
> -       for (i = 0; i < iterator->pdata->nr_outconns; i++) {
> -               conn = iterator->pdata->out_conns[i];
> -
> -               /* Child_dev being set signifies that the links were made */
> -               if (csdev->dev.fwnode == conn->dest_fwnode && conn->dest_dev) {
> -                       iterator->orphan = true;
> -                       coresight_remove_links(iterator, conn);
> -                       conn->dest_dev = NULL;
> -                       /* No need to continue */
> -                       break;
> -               }
> +       for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> +               conn = csdev->pdata->out_conns[i];
> +               if (!conn->dest_dev)
> +                       continue;
> +
> +               for (j = 0; j < conn->dest_dev->pdata->nr_inconns; ++j)
> +                       if (conn->dest_dev->pdata->in_conns[j] == conn) {
> +                               conn->dest_dev->pdata->in_conns[j] = NULL;
> +                               break;
> +                       }
>         }
>
>         /*
> -        * Returning '0' ensures that all known component on the
> -        * bus will be checked.
> +        * For all input connections, remove references to this device.
> +        * Connection objects are shared so modifying this device's input
> +        * connections affects the other device's output connection.
>          */
> -       return 0;
> -}
> +       for (i = 0; i < csdev->pdata->nr_inconns; ++i) {
> +               conn = csdev->pdata->in_conns[i];
> +               /* Input conns array is sparse */
> +               if (!conn)
> +                       continue;
>
> -/*
> - * coresight_remove_conns - Remove references to this given devices
> - * from the connections of other devices.
> - */
> -static void coresight_remove_conns(struct coresight_device *csdev)
> -{
> -       /*
> -        * Another device will point to this device only if there is
> -        * an output port connected to this one. i.e, if the device
> -        * doesn't have at least one input port, there is no point
> -        * in searching all the devices.
> -        */
> -       if (csdev->pdata->high_inport)
> -               bus_for_each_dev(&coresight_bustype, NULL,
> -                                csdev, coresight_remove_match);
> +               conn->src_dev->orphan = true;
> +               coresight_remove_links(conn->src_dev, conn);
> +               conn->dest_dev = NULL;
> +       }
>  }
>
>  /**
> @@ -1532,6 +1528,7 @@ void coresight_release_platform_data(struct coresight_device *csdev,
>                 devm_kfree(dev, conns[i]);
>         }
>         devm_kfree(dev, pdata->out_conns);
> +       devm_kfree(dev, pdata->in_conns);
>         devm_kfree(dev, pdata);
>         if (csdev)
>                 coresight_remove_conns_sysfs_group(csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 9c05f787278b..257ad48925a1 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -70,6 +70,35 @@ coresight_add_out_conn(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(coresight_add_out_conn);
>
> +/*
> + * Add an input connection reference to @out_conn in the target's in_conns array
> + *
> + * @out_conn: Existing output connection to store as an input on the
> + *           connection's remote device.
> + */
> +int coresight_add_in_conn(struct coresight_connection *out_conn)
> +{
> +       int i;
> +       struct device *dev = out_conn->dest_dev->dev.parent;
> +       struct coresight_platform_data *pdata = out_conn->dest_dev->pdata;
> +
> +       for (i = 0; i < pdata->nr_inconns; ++i)
> +               if (!pdata->in_conns[i]) {
> +                       pdata->in_conns[i] = out_conn;
> +                       return 0;
> +               }
> +
> +       pdata->nr_inconns++;
> +       pdata->in_conns =
> +               devm_krealloc_array(dev, pdata->in_conns, pdata->nr_inconns,
> +                                   sizeof(*pdata->in_conns), GFP_KERNEL);
> +       if (!pdata->in_conns)
> +               return -ENOMEM;
> +       pdata->in_conns[pdata->nr_inconns - 1] = out_conn;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(coresight_add_in_conn);
> +
>  static struct device *
>  coresight_find_device_by_fwnode(struct fwnode_handle *fwnode)
>  {
> @@ -240,7 +269,7 @@ static int of_coresight_get_cpu(struct device *dev)
>
>  /*
>   * of_coresight_parse_endpoint : Parse the given output endpoint @ep
> - * and fill the connection information in @conn
> + * and fill the connection information in @pdata->out_conns
>   *
>   * Parses the local port, remote device name and the remote port.
>   *
> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> index a4a8e8e642e8..464ba5e1343b 100644
> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> @@ -173,12 +173,6 @@ int coresight_make_links(struct coresight_device *orig,
>                         break;
>
>                 conn->link = link;
> -
> -               /*
> -                * Install the device connection. This also indicates that
> -                * the links are operational on both ends.
> -                */
> -               conn->dest_dev = target;
>                 return 0;
>         } while (0);
>
> @@ -202,5 +196,4 @@ void coresight_remove_links(struct coresight_device *orig,
>         devm_kfree(&orig->dev, conn->link->orig_name);
>         devm_kfree(&orig->dev, conn->link);
>         conn->link = NULL;
> -       conn->dest_dev = NULL;
>  }
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index abf36a37fdb0..e9c52c5ca7f3 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -106,6 +106,9 @@ union coresight_dev_subtype {
>   * @nr_outconns: Number of elements for the output connections.
>   * @out_conns: Array of nr_outconns pointers to connections from this
>   *            component.
> + * @in_conns: Sparse array of pointers to input connections. Sparse
> + *            because the source device owns the connection so when it's
> + *            unloaded the connection leaves an empty slot.
>   */
>  struct coresight_platform_data {
>         int high_inport;
> @@ -113,6 +116,7 @@ struct coresight_platform_data {
>         int nr_inconns;
>         int nr_outconns;
>         struct coresight_connection **out_conns;
> +       struct coresight_connection **in_conns;
>  };
>
>  /**
> @@ -173,6 +177,26 @@ struct coresight_desc {
>   * @dest_dev:  a @coresight_device representation of the component
>                 connected to @src_port. NULL until the device is created
>   * @link: Representation of the connection as a sysfs link.
> + *
> + * The full connection structure looks like this, where in_conns store
> + * references to same connection as the source device's out_conns.
> + *
> + * +-----------------------------+   +-----------------------------+
> + * |coresight_device             |   |coresight_connection         |
> + * |-----------------------------|   |-----------------------------|
> + * |                             |   |                             |
> + * |                             |   |                    dest_dev*|<--
> + * |pdata->out_conns[nr_outconns]|<->|src_dev*                     |   |
> + * |                             |   |                             |   |
> + * +-----------------------------+   +-----------------------------+   |
> + *                                                                     |
> + *                                   +-----------------------------+   |
> + *                                   |coresight_device             |   |
> + *                                   |------------------------------   |
> + *                                   |                             |   |
> + *                                   |  pdata->in_conns[nr_inconns]|<--
> + *                                   |                             |
> + *                                   +-----------------------------+
>   */
>  struct coresight_connection {
>         int src_port;
> @@ -180,6 +204,7 @@ struct coresight_connection {
>         struct fwnode_handle *dest_fwnode;
>         struct coresight_device *dest_dev;
>         struct coresight_sysfs_link *link;
> +       struct coresight_device *src_dev;
>  };
>
>  /**
> @@ -616,5 +641,6 @@ struct coresight_connection *
>  coresight_add_out_conn(struct device *dev,
>                        struct coresight_platform_data *pdata,
>                        const struct coresight_connection *new_conn);
> +int coresight_add_in_conn(struct coresight_connection *conn);
>
>  #endif         /* _LINUX_COREISGHT_H */
> --
> 2.34.1
>

Reviewed-by: Mike Leach <mike.leach@linaro.org>

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK