Re: [PATCH v1 19/21] coresight: disable trace path with device being removed

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Good day Tingwei,

On Wed, Jul 01, 2020 at 03:14:25PM +0800, Tingwei Zhang wrote:
> Coresight trace path holds the reference to devices on the path.
> Disable trace path before really remove coresight device.
> 
> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight.c | 137 ++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index cf7079f4b99c..0870316ec4c4 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -490,6 +490,138 @@ void coresight_disable_path(struct list_head *path)
>  }
>  EXPORT_SYMBOL_GPL(coresight_disable_path);
>  
> +/*
> + *  coresight_dev_on_path - Check if device is on trace path.
> + *
> + *  @path - The trace path to check with.
> + *  @csdev - The coresight device to check
> + *
> + *  Returns true if the device is on trace path.
> + */
> +bool coresight_dev_on_path(struct list_head *path,
> +			struct coresight_device *csdev)
> +{
> +	struct coresight_node *nd;
> +
> +	list_for_each_entry(nd, path, link) {
> +		if (csdev == nd->csdev)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + *  coresight_disable_path_with - Disable trace path
> + *  if trace path has concerned device on it.
> + *
> + *  @path - The trace path to check.
> + *  @csdev - Concerned coresight device
> + *
> + *  Returns true if the device is on trace path.
> + */
> +static bool coresight_disable_path_with(struct list_head *path,
> +				struct coresight_device *csdev)
> +{
> +	struct coresight_node *src_nd;
> +	struct coresight_device *srcdev;
> +	bool ret;
> +
> +	ret = coresight_dev_on_path(path, csdev);
> +	if (ret) {
> +		src_nd = list_first_entry(path, struct coresight_node, link);
> +		srcdev = src_nd->csdev;
> +		if (source_ops(srcdev)->disable)
> +			source_ops(srcdev)->disable(srcdev, NULL);
> +		srcdev->enable = false;
> +		coresight_disable_path(path);
> +		coresight_release_path(path);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + *  __coresight_disable_with - Check cpu trace path and
> + *  stm trace path. Disable any path with conerned device.
> + *
> + *  @csdev - Concerned coresight device
> + *
> + */
> +static void __coresight_disable_with(struct coresight_device *csdev)
> +{
> +	int cpu;
> +	struct list_head *path = NULL;
> +
> +	for_each_possible_cpu(cpu) {
> +		path = per_cpu(tracer_path, cpu);
> +		if (path) {
> +			if (coresight_disable_path_with(path, csdev))
> +				per_cpu(tracer_path, cpu) = NULL;
> +		}
> +	}
> +	if (stm_path) {
> +		path = stm_path;
> +		if (coresight_disable_path_with(path, csdev))
> +			stm_path = NULL;
> +	}
> +}
> +
> +static int coresight_disable_match(struct device *dev, void *data)
> +{
> +	int i;
> +	struct coresight_device *csdev, *iterator;
> +	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, disable path with it.
> +	 */
> +	for (i = 0; i < iterator->pdata->nr_outport; i++) {
> +		conn = &iterator->pdata->conns[i];
> +
> +		if (conn->child_dev == NULL)
> +			continue;
> +
> +		if (csdev->dev.fwnode == conn->child_fwnode) {
> +			__coresight_disable_with(iterator);
> +
> +			/* No need to continue */
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * Returning '0' ensures that all known components on the
> +	 * bus will be checked.
> +	 */
> +	return 0;
> +}
> +
> +/**
> + * coresight_disable_with - disable any path with @csdev.
> + * @csdev:	The device to check.
> + *
> + * Search for all the active paths. If @csdev is part of that path,
> + * disable the whole path. Device with CORESIGHT_DEV_TYPE_HELPER type
> + * is not part of path. Search for its parent device and disable any
> + * path with that device.
> + */
> +void coresight_disable_with(struct coresight_device *csdev)
> +{
> +	if (csdev->type != CORESIGHT_DEV_TYPE_HELPER)
> +		return __coresight_disable_with(csdev);

The main problem is indeed to take care of ongoing sessions which, other than the
module mechanic, is the bulk of the code added in this set.

I've been thinking about this problem while reviewing an unrelated patchset and
the solution I see is to let the driver core work for us.  Since there is no
implicit relation between modules, all we need is to introduce one using
try_module_get() and module_put().  The module for each csdev can be accessed
via csdev->dev->parent->driver->module.

From looking at the code if we call try_module_get() in coresight_grab_device()
and module_put() in coresight_drop_device(), the driver core won't let users
remove modules for as long as a session is using the driver.  That way all you
need is the loop below and that is quite simple.

Let me know what you think,
Mathieu

> +	if (csdev->pdata->nr_inport)
> +		bus_for_each_dev(&coresight_bustype, NULL,
> +				 csdev, coresight_disable_match);
> +}
> +
>  int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data)
>  {
>  
> @@ -1243,6 +1375,7 @@ void coresight_release_platform_data(struct coresight_platform_data *pdata)
>  
>  	for (i = 0; i < pdata->nr_outport; i++) {
>  		if (pdata->conns[i].child_fwnode) {
> +			pdata->conns[i].child_dev = NULL;
>  			fwnode_handle_put(pdata->conns[i].child_fwnode);
>  			pdata->conns[i].child_fwnode = NULL;
>  		}
> @@ -1349,9 +1482,13 @@ EXPORT_SYMBOL_GPL(coresight_register);
>  void coresight_unregister(struct coresight_device *csdev)
>  {
>  	etm_perf_del_symlink_sink(csdev);
> +
> +	mutex_lock(&coresight_mutex);
> +	coresight_disable_with(csdev);
>  	/* Remove references of that device in the topology */
>  	coresight_remove_conns(csdev);
>  	coresight_release_platform_data(csdev->pdata);
> +	mutex_unlock(&coresight_mutex);
>  	device_unregister(&csdev->dev);
>  }
>  EXPORT_SYMBOL_GPL(coresight_unregister);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel