[PATCH v1 19/21] coresight: disable trace path with device being removed
tingwei at codeaurora.org
tingwei at codeaurora.org
Mon Jul 13 21:10:20 EDT 2020
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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list