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

[tingwei@codeaurora.org]

Thanks for reviewing this, Mathieu.

On 2020-07-14 06:11, Mathieu Poirier wrote:
> 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

I agree with you, Mathieu. Using try_module_get() and module_put() is a
simpler, cleaner and safer way. I'll make the change, verify them and
send a new patch set for review.

Tingwei

> 
>> +	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

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