| 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: <20180503170040.GA11425@xps15>
Date: Thu, 3 May 2018 11:00:40 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
mike.leach@...aro.org, robert.walker@....com, mark.rutland@....com,
will.deacon@....com, robin.murphy@....com, sudeep.holla@....com,
frowand.list@...il.com, robh@...nel.org, john.horley@....com
Subject: Re: [PATCH v2 03/27] coresight: Add helper device type
On Tue, May 01, 2018 at 10:10:33AM +0100, Suzuki K Poulose wrote:
> Add a new coresight device type, which do not belong to any
> of the existing types, i.e, source, sink, link etc. A helper
> device could be connected to a coresight device, which could
> augment the functionality of the coresight device.
>
> This is intended to cover Coresight Address Translation Unit (CATU)
> devices, which provide improved Scatter Gather mechanism for TMC
> ETR. The idea is that the helper device could be controlled by
> the driver of the device it is attached to (in this case ETR),
> transparent to the generic coresight driver (and paths).
>
> The operations include enable(), disable(), both of which could
> accept a device specific "data" which the driving device and
> the helper device could share. Since they don't appear in the
> coresight "path" tracked by software, we have to ensure that
> they are powered up/down whenever the master device is turned
> on.
>
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> ---
> drivers/hwtracing/coresight/coresight.c | 46 ++++++++++++++++++++++++++++++---
> include/linux/coresight.h | 24 +++++++++++++++++
> 2 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 389c4ba..fd0251e 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -430,6 +430,43 @@ struct coresight_device *coresight_get_enabled_sink(bool deactivate)
> return dev ? to_coresight_device(dev) : NULL;
> }
>
> +/*
> + * coresight_prepare_device - Prepare this device and any of the helper
> + * devices connected to it for trace operation. Since the helper devices
> + * don't appear on the trace path, they should be handled along with the
> + * the master device.
> + */
> +static void coresight_prepare_device(struct coresight_device *csdev)
> +{
> + int i;
> +
> + for (i = 0; i < csdev->nr_outport; i++) {
> + struct coresight_device *child = csdev->conns[i].child_dev;
> +
> + if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> + pm_runtime_get_sync(child->dev.parent);
> + }
> +
> + pm_runtime_get_sync(csdev->dev.parent);
> +}
> +
> +/*
> + * coresight_release_device - Release this device and any of the helper
> + * devices connected to it for trace operation.
> + */
> +static void coresight_release_device(struct coresight_device *csdev)
> +{
> + int i;
> +
> + for (i = 0; i < csdev->nr_outport; i++) {
> + struct coresight_device *child = csdev->conns[i].child_dev;
> +
> + if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> + pm_runtime_put(child->dev.parent);
> + }
There is a newline here in coresight_prepare_device(). Either add one (or not)
in both function but please be consistent.
> + pm_runtime_put(csdev->dev.parent);
> +}
> +
> /**
> * _coresight_build_path - recursively build a path from a @csdev to a sink.
> * @csdev: The device to start from.
> @@ -480,8 +517,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
>
> node->csdev = csdev;
> list_add(&node->link, path);
> - pm_runtime_get_sync(csdev->dev.parent);
> -
> + coresight_prepare_device(csdev);
There was a newline between pm_runtime_get_sync() and the return statement in
the original code.
> return 0;
> }
>
> @@ -524,7 +560,7 @@ void coresight_release_path(struct list_head *path)
> list_for_each_entry_safe(nd, next, path, link) {
> csdev = nd->csdev;
>
> - pm_runtime_put_sync(csdev->dev.parent);
> + coresight_release_device(csdev);
> list_del(&nd->link);
> kfree(nd);
> }
> @@ -775,6 +811,10 @@ static struct device_type coresight_dev_type[] = {
> .name = "source",
> .groups = coresight_source_groups,
> },
> + {
> + .name = "helper",
> + },
> +
Extra newline.
> };
>
> static void coresight_device_release(struct device *dev)
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 556fe59..5e926f7 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -47,6 +47,7 @@ enum coresight_dev_type {
> CORESIGHT_DEV_TYPE_LINK,
> CORESIGHT_DEV_TYPE_LINKSINK,
> CORESIGHT_DEV_TYPE_SOURCE,
> + CORESIGHT_DEV_TYPE_HELPER,
> };
>
> enum coresight_dev_subtype_sink {
> @@ -69,6 +70,10 @@ enum coresight_dev_subtype_source {
> CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
> };
>
> +enum coresight_dev_subtype_helper {
> + CORESIGHT_DEV_SUBTYPE_HELPER_NONE,
> +};
> +
> /**
> * union coresight_dev_subtype - further characterisation of a type
> * @sink_subtype: type of sink this component is, as defined
> @@ -77,6 +82,8 @@ enum coresight_dev_subtype_source {
> * by @coresight_dev_subtype_link.
> * @source_subtype: type of source this component is, as defined
> * by @coresight_dev_subtype_source.
> + * @helper_subtype: type of helper this component is, as defined
> + * by @coresight_dev_subtype_helper.
> */
> union coresight_dev_subtype {
> /* We have some devices which acts as LINK and SINK */
> @@ -85,6 +92,7 @@ union coresight_dev_subtype {
> enum coresight_dev_subtype_link link_subtype;
> };
> enum coresight_dev_subtype_source source_subtype;
> + enum coresight_dev_subtype_helper helper_subtype;
> };
>
> /**
> @@ -181,6 +189,7 @@ struct coresight_device {
> #define source_ops(csdev) csdev->ops->source_ops
> #define sink_ops(csdev) csdev->ops->sink_ops
> #define link_ops(csdev) csdev->ops->link_ops
> +#define helper_ops(csdev) csdev->ops->helper_ops
>
> /**
> * struct coresight_ops_sink - basic operations for a sink
> @@ -240,10 +249,25 @@ struct coresight_ops_source {
> struct perf_event *event);
> };
>
> +/**
> + * struct coresight_ops_helper - Operations for a helper device.
> + *
> + * All operations could pass in a device specific data, which could
> + * help the helper device to determine what to do.
> + *
> + * @enable : Turn the device ON.
> + * @disable : Turn the device OFF.
There is a discrepancy between the comment and the operations, i.e enabling a
device is not synonymous of turning it on. Looking at patch 04/27 the ops is
called in tmc_etr_enable/disable_catu() so the comment propably needs to be
changed.
> + */
> +struct coresight_ops_helper {
> + int (*enable)(struct coresight_device *csdev, void *data);
> + int (*disable)(struct coresight_device *csdev, void *data);
> +};
> +
> struct coresight_ops {
> const struct coresight_ops_sink *sink_ops;
> const struct coresight_ops_link *link_ops;
> const struct coresight_ops_source *source_ops;
> + const struct coresight_ops_helper *helper_ops;
> };
>
> #ifdef CONFIG_CORESIGHT
> --
> 2.7.4
>
Powered by blists - more mailing lists