| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6af8c6b8-92b4-93af-409e-43c4b604f338@arm.com>
Date: Mon, 3 Apr 2023 18:54:38 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: James Clark <james.clark@....com>, coresight@...ts.linaro.org,
quic_jinlmao@...cinc.com, mike.leach@...aro.org
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>,
Leo Yan <leo.yan@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v3 12/13] coresight: Enable and disable helper devices
adjacent to the path
On 29/03/2023 12:53, James Clark wrote:
> Currently CATU is the only helper device, and its enable and disable
> calls are hard coded. To allow more helper devices to be added in a
> generic way, remove these hard coded calls and just enable and disable
> all helper devices.
>
> This has to apply to helpers adjacent to the path, because they will
> never be in the path. CATU was already discovered in this way, so
> there is no change there.
>
> One change that is needed is for CATU to call back into ETR to allocate
> the buffer. Because the enable call was previously hard coded, it was
> done at a point where the buffer was already allocated, but this is no
> longer the case.
>
> Signed-off-by: James Clark <james.clark@....com>
Looks good to me. Some minor nits below.
> ---
> drivers/hwtracing/coresight/coresight-catu.c | 21 ++-
> drivers/hwtracing/coresight/coresight-core.c | 147 +++++++++++++++++-
> .../hwtracing/coresight/coresight-etm-perf.c | 4 +-
> drivers/hwtracing/coresight/coresight-priv.h | 3 +
> .../hwtracing/coresight/coresight-tmc-etr.c | 43 +----
> include/linux/coresight.h | 11 +-
> 6 files changed, 177 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index bc90a03f478f..3949ded0d4fa 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -395,13 +395,18 @@ static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)
> return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);
> }
>
> -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> +static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> + void *data) > {
> int rc;
> u32 control, mode;
> - struct etr_buf *etr_buf = data;
> + struct etr_buf *etr_buf = NULL;
> struct device *dev = &drvdata->csdev->dev;
> struct coresight_device *csdev = drvdata->csdev;
> + struct coresight_device *etrdev;
> + union coresight_dev_subtype etr_subtype = {
> + .sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM
> + };
>
> if (catu_wait_for_ready(drvdata))
> dev_warn(dev, "Timeout while waiting for READY\n");
> @@ -416,6 +421,13 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> if (rc)
> return rc;
>
> + etrdev = coresight_find_input_type(
> + csdev->pdata, CORESIGHT_DEV_TYPE_SINK, etr_subtype);
> + if (etrdev) {
> + etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
> + if (IS_ERR(etr_buf))
> + return PTR_ERR(etr_buf);
> + }
> control |= BIT(CATU_CONTROL_ENABLE);
>
> if (etr_buf && etr_buf->mode == ETR_MODE_CATU) {
> @@ -441,13 +453,14 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> return 0;
> }
>
> -static int catu_enable(struct coresight_device *csdev, void *data)
> +static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> + void *data)
> {
> int rc;
> struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>
> CS_UNLOCK(catu_drvdata->base);
> - rc = catu_enable_hw(catu_drvdata, data);
> + rc = catu_enable_hw(catu_drvdata, mode, data);
> CS_LOCK(catu_drvdata->base);
> return rc;
> }
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index baa23aa53971..65f5bd8516d8 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -421,8 +421,8 @@ static void coresight_disable_link(struct coresight_device *csdev,
> csdev->enable = false;
> }
>
> -static int coresight_enable_source(struct coresight_device *csdev,
> - enum cs_mode mode)
> +int coresight_enable_source(struct coresight_device *csdev, void *data,
> + enum cs_mode mode)
minor nit: The order of these parameters are inconsistent.
i.e., would prefer:
enable_source(csdev, mode, data);
and I very well understand that it is coming from the existing code
(i.e., source_ops->enable). But, at least we could keep the order
for the wrapper, inline with other wrappers.
> {
> int ret;
>
> @@ -431,7 +431,7 @@ static int coresight_enable_source(struct coresight_device *csdev,
> ret = coresight_control_assoc_ectdev(csdev, true);
> if (ret)
> return ret;
> - ret = source_ops(csdev)->enable(csdev, NULL, mode);
> + ret = source_ops(csdev)->enable(csdev, data, mode);
> if (ret) {
> coresight_control_assoc_ectdev(csdev, false);
> return ret;
> @@ -444,6 +444,47 @@ static int coresight_enable_source(struct coresight_device *csdev,
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(coresight_enable_source);
> +
> +static int coresight_enable_helper(struct coresight_device *csdev,
> + enum cs_mode mode, void *data)
> +{
> + int ret;
> +
> + if (!helper_ops(csdev)->enable)
> + return 0;
> + ret = helper_ops(csdev)->enable(csdev, mode, data);
> + if (ret)
> + return ret;
> +
> + csdev->enable = true;
> + return 0;
> +}
> +
> +static void coresight_disable_helper(struct coresight_device *csdev)
> +{
> + int ret;
> +
> + if (!helper_ops(csdev)->disable)
> + return;
> +
> + ret = helper_ops(csdev)->disable(csdev, NULL);
> + if (ret)
> + return;
> + csdev->enable = false;
> +}
> +
> +static void coresight_disable_helpers(struct coresight_device *csdev)
> +{
> + int i;
> + struct coresight_device *helper;
> +
> + for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> + helper = csdev->pdata->out_conns[i]->dest_dev;
> + if (helper && helper->type == CORESIGHT_DEV_TYPE_HELPER)
minor nit: Do we need a wrapper ? is_coresight_helper(helper).
We need it in enable path too.
> + coresight_disable_helper(helper);
> + }
> +}
>
> /**
> * coresight_disable_source - Drop the reference count by 1 and disable
> @@ -453,16 +494,18 @@ static int coresight_enable_source(struct coresight_device *csdev,
> *
> * Returns true if the device has been disabled.
> */
> -static bool coresight_disable_source(struct coresight_device *csdev)
> +bool coresight_disable_source(struct coresight_device *csdev, void *data)
> {
> if (atomic_dec_return(&csdev->refcnt) == 0) {
> if (source_ops(csdev)->disable)
> - source_ops(csdev)->disable(csdev, NULL);
> + source_ops(csdev)->disable(csdev, data);
> coresight_control_assoc_ectdev(csdev, false);
> + coresight_disable_helpers(csdev);
> csdev->enable = false;
> }
> return !csdev->enable;
> }
> +EXPORT_SYMBOL_GPL(coresight_disable_source);
>
> /*
> * coresight_disable_path_from : Disable components in the given path beyond
> @@ -513,6 +556,9 @@ static void coresight_disable_path_from(struct list_head *path,
> default:
> break;
> }
> +
> + /* Disable all helpers adjacent along the path last */
> + coresight_disable_helpers(csdev);
> }
> }
>
> @@ -522,9 +568,28 @@ void coresight_disable_path(struct list_head *path)
> }
> EXPORT_SYMBOL_GPL(coresight_disable_path);
>
> -int coresight_enable_path(struct list_head *path, enum cs_mode mode, void *sink_data)
> +static int coresight_enable_helpers(struct coresight_device *csdev,
> + enum cs_mode mode, void *data)
> {
> + int i, ret = 0;
> + struct coresight_device *helper;
> +
> + for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> + helper = csdev->pdata->out_conns[i]->dest_dev;
> + if (!helper || helper->type != CORESIGHT_DEV_TYPE_HELPER)
> + continue;
> +
> + ret = coresight_enable_helper(helper, mode, data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
>
> +int coresight_enable_path(struct list_head *path, enum cs_mode mode,
> + void *sink_data)
> +{
> int ret = 0;
> u32 type;
> struct coresight_node *nd;
> @@ -534,6 +599,10 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode, void *sink_
> csdev = nd->csdev;
> type = csdev->type;
>
> + /* Enable all helpers adjacent to the path first */
> + ret = coresight_enable_helpers(csdev, mode, sink_data);
> + if (ret)
> + goto err;
> /*
> * ETF devices are tricky... They can be a link or a sink,
> * depending on how they are configured. If an ETF has been
> @@ -1120,7 +1189,7 @@ int coresight_enable(struct coresight_device *csdev)
> if (ret)
> goto err_path;
>
> - ret = coresight_enable_source(csdev, CS_MODE_SYSFS);
> + ret = coresight_enable_source(csdev, NULL, CS_MODE_SYSFS);
> if (ret)
> goto err_source;
>
> @@ -1177,7 +1246,7 @@ void coresight_disable(struct coresight_device *csdev)
> if (ret)
> goto out;
>
> - if (!csdev->enable || !coresight_disable_source(csdev))
> + if (!csdev->enable || !coresight_disable_source(csdev, NULL))
> goto out;
>
> switch (csdev->subtype.source_subtype) {
> @@ -1653,6 +1722,68 @@ static inline int coresight_search_device_idx(struct coresight_dev_list *dict,
> return -ENOENT;
> }
>
> +static bool coresight_compare_type(enum coresight_dev_type type_a,
> + union coresight_dev_subtype subtype_a,
> + enum coresight_dev_type type_b,
> + union coresight_dev_subtype subtype_b)
> +{
> + if (type_a != type_b)
> + return false;
> +
> + switch (type_a) {
> + case CORESIGHT_DEV_TYPE_SINK:
> + return subtype_a.sink_subtype == subtype_b.sink_subtype;
> + case CORESIGHT_DEV_TYPE_LINK:
> + return subtype_a.link_subtype == subtype_b.link_subtype;
> + case CORESIGHT_DEV_TYPE_LINKSINK:
> + return subtype_a.link_subtype == subtype_b.link_subtype &&
> + subtype_a.sink_subtype == subtype_b.sink_subtype;
> + case CORESIGHT_DEV_TYPE_SOURCE:
> + return subtype_a.source_subtype == subtype_b.source_subtype;
> + case CORESIGHT_DEV_TYPE_HELPER:
> + return subtype_a.helper_subtype == subtype_b.helper_subtype;
> + default:
> + return false;
> + }
> +}
minor nit: new line
> +struct coresight_device *
> +coresight_find_input_type(struct coresight_platform_data *pdata,
> + enum coresight_dev_type type,
> + union coresight_dev_subtype subtype)
Suzuki
Powered by blists - more mailing lists