coresight: allow to build coresight as modules

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/hwtracing/coresight/coresight.c +23 -5

index b7151c5f81b1..17bc76ea86ae 100644
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * don't appear on the trace path, they should be handled along with the
  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
        int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+                       if (!try_module_get(child->dev.parent->driver->owner))
+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }
+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
        int i;
 
        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
                        pm_runtime_put(child->dev.parent);
+                       module_put(child->dev.parent->driver->owner);
+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
        if (!node)
                return -ENOMEM;
 
-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Hi Tingwei,

Review

Quoted 11 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along with

the

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
        int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c +2 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+                       if

(!try_module_get(child->dev.parent->driver->owner))

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 21 lines

drivers/hwtracing/coresight/coresight.c +14 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }
+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 17 lines

drivers/hwtracing/coresight/coresight.c +4 -1

        int i;
 
        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
                        pm_runtime_put(child->dev.parent);
+                       module_put(child->dev.parent->driver->owner);
+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 11 lines

drivers/hwtracing/coresight/coresight.c +2 -2

        if (!node)
                return -ENOMEM;
 
-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components using csdev->ect_dev; These are not handled in the main linear trace path as these have a star topology interlinking all components. However, when a component has an associated ect_dev then is enabled at the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent the CTI being removed while a trace session is active.
I suggest in coresight.c : coresight_control_assoc_ectdev()

Regards

Mike

Signature · 8 lines

Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Mike,

Quoted 100 lines · Mike Leach

Hi Tingwei,

Review

Quoted 11 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along with

the

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
        int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c +2 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+                       if

(!try_module_get(child->dev.parent->driver->owner))

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 21 lines

drivers/hwtracing/coresight/coresight.c +14 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }
+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 17 lines

drivers/hwtracing/coresight/coresight.c +4 -1

        int i;
 
        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
                        pm_runtime_put(child->dev.parent);
+                       module_put(child->dev.parent->driver->owner);
+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 11 lines

drivers/hwtracing/coresight/coresight.c +2 -2

        if (!node)
                return -ENOMEM;
 
-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components using csdev->ect_dev; These are not handled in the main linear trace path as these have a star topology interlinking all components. However, when a component has an associated ect_dev then is enabled at the same time as the other component is enabled.

Quoted 4 lines

So a module_get/put will be needed for this association to prevent the CTI being removed while a trace session is active.
I suggest in coresight.c : coresight_control_assoc_ectdev()

In module unload process, devices of that module will be removed. In this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Tingwei,

Quoted 16 lines · Tingwei Zhang

Hi Mike,

Quoted 12 lines · Mike Leach

Hi Tingwei,

Quoted 8 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 10 lines

Quoted 10 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along

with

Quoted 8 lines

Quoted 8 lines

the

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
        int i;
 

Quoted 6 lines

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 16 lines

Quoted 16 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +9 -0

+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct coresight_device *csdev)

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 32 lines

Quoted 27 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 11 lines

drivers/hwtracing/coresight/coresight.c +2 -2

        if (!node)
                return -ENOMEM;
 
-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components using csdev->ect_dev; These are not handled in the main linear trace path as these have a star topology interlinking all components. However, when a component has an associated ect_dev then is enabled at the same time as the other component is enabled.

Quoted 4 lines

So a module_get/put will be needed for this association to prevent the CTI being removed while a trace session is active.
I suggest in coresight.c : coresight_control_assoc_ectdev()

Quoted 9 lines

In module unload process, devices of that module will be removed. In this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Regards

Mike

Quoted 13 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 10 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Quoted 4 lines

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 8 lines

Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Mike,

Quoted 131 lines · Mike Leach

Hi Tingwei,

Quoted 16 lines · Tingwei Zhang

Hi Mike,

Quoted 12 lines · Mike Leach

Hi Tingwei,

Quoted 8 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 10 lines

Quoted 10 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along

with

Quoted 8 lines

Quoted 8 lines

the

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
        int i;
 

Quoted 6 lines

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 16 lines

Quoted 16 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +9 -0

+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct coresight_device *csdev)

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 32 lines

Quoted 27 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 11 lines

drivers/hwtracing/coresight/coresight.c +2 -2

        if (!node)
                return -ENOMEM;
 
-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components using csdev->ect_dev; These are not handled in the main linear trace path as these have a star topology interlinking all components. However, when a component has an associated ect_dev then is enabled at the same time as the other component is enabled.

Quoted 4 lines

So a module_get/put will be needed for this association to prevent the CTI being removed while a trace session is active.
I suggest in coresight.c : coresight_control_assoc_ectdev()

Quoted 9 lines

In module unload process, devices of that module will be removed. In this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Quoted 8 lines

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional.

For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup.

In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful.

For example,
One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all.

My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function.

Thanks, Tingwei

Quoted 33 lines

Regards

Mike

Quoted 17 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Tingwei,

Quoted 14 lines · Tingwei Zhang

Hi Mike,

Quoted 11 lines · Mike Leach

Hi Tingwei,

Quoted 7 lines · Tingwei Zhang

Hi Mike,

Quoted 3 lines · Mike Leach

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang

<[email protected]>

Quoted 19 lines

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines · wrote:

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 9 lines

Quoted 9 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along

with

Quoted 4 lines

Quoted 4 lines

the

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device

*csdev)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 643

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+static int coresight_grab_device(struct coresight_device

*csdev)

Quoted 61 lines

Quoted 61 lines

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -0

 {
        int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 16 lines

Quoted 16 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +9 -0

+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct coresight_device *csdev)

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        if (!node)
                return -ENOMEM;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +2 -2

-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code

Aurora

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components

using

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

csdev->ect_dev; These are not handled in the main linear trace

path as

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

these have a star topology interlinking all components. However, when a component has an associated ect_dev then is

enabled at

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent

the

Quoted 23 lines

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

CTI being removed while a trace session is active. I suggest in coresight.c : coresight_control_assoc_ectdev()

Quoted 9 lines

In module unload process, devices of that module will be removed. In this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Quoted 8 lines

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Quoted 9 lines

Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional.

For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup.

The module get/put do more than ensure that the system can be enabled. They ensure that no components in the coresight path can be unloaded until all paths in use are released. This ensures that clients do not get the trace system pulled out from underneath - corrupting trace data, and possibly affecting the client program.

Quoted 2 lines

In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful.

And there is no guarantee that it is not programmed to perform a key task in controlling the generation of trace.

The entire programmed coresight system must be protected when in use. That includes CTI devices. Otherwise the integrity of the captured trace could be compromised, and indeed there may be no trace captured if this is relying on CTI triggers.

Quoted 6 lines

For example,
One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all.

Quoted 6 lines

My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function.

Precisely - and this is what should never be allowed - why can the CTI be used to break the trace system but not other components?

Moving forwards we will see users who simply use pre-programmed settings from client programs such as perf. It may be that some users do not have the credentials to add and remove modules - this could be an admin function where larger systems with multiple sessions are in use.

Regards

Mike

Quoted 32 lines

Thanks, Tingwei

Quoted 29 lines

Regards

Mike

Quoted 17 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Quoted 4 lines

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 8 lines

Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Mike,

Quoted 165 lines · Mike Leach

Hi Tingwei,

Quoted 14 lines · Tingwei Zhang

Hi Mike,

Quoted 11 lines · Mike Leach

Hi Tingwei,

Quoted 7 lines · Tingwei Zhang

Hi Mike,

Quoted 3 lines · Mike Leach

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang

<[email protected]>

Quoted 19 lines

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines · wrote:

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 9 lines

Quoted 9 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along

with

Quoted 4 lines

Quoted 4 lines

the

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device

*csdev)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 643

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+static int coresight_grab_device(struct coresight_device

*csdev)

Quoted 61 lines

Quoted 61 lines

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -0

 {
        int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 16 lines

Quoted 16 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -0

+                               goto err;
                        pm_runtime_get_sync(child->dev.parent);
+               }
        }

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 666

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +9 -0

+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;
        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 670

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        if (!node)
                return -ENOMEM;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +2 -2

-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code

Aurora

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components

using

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

csdev->ect_dev; These are not handled in the main linear trace

path as

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

these have a star topology interlinking all components. However, when a component has an associated ect_dev then is

enabled at

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent

the

Quoted 23 lines

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

CTI being removed while a trace session is active. I suggest in coresight.c : coresight_control_assoc_ectdev()

Quoted 9 lines

In module unload process, devices of that module will be removed. In this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Quoted 8 lines

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Quoted 9 lines

Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional.

For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup.

Quoted 6 lines

The module get/put do more than ensure that the system can be enabled. They ensure that no components in the coresight path can be unloaded until all paths in use are released. This ensures that clients do not get the trace system pulled out from underneath - corrupting trace data, and possibly affecting the client program.

I agree.

Quoted 6 lines

Quoted 2 lines

In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful.

And there is no guarantee that it is not programmed to perform a key task in controlling the generation of trace.

Quoted 5 lines

The entire programmed coresight system must be protected when in use. That includes CTI devices. Otherwise the integrity of the captured trace could be compromised, and indeed there may be no trace captured if this is relying on CTI triggers.

That's correct. If CTI is not there, the trace may be wrong or no trace as all.

Quoted 16 lines

Quoted 6 lines

For example,
One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all.

Quoted 6 lines

My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function.

Precisely - and this is what should never be allowed - why can the CTI be used to break the trace system but not other components?

We can't prevent user to unload CTI module when there's no active trace session.

If we don't allow CTI to be not available, we need to check whether CTI is there in build_path routine or coresight enable routine. We need return failure if CTI is not there when user enables the trace session. Currently, coresight framework check associate CTI device, but if CTI deivce is not probed due to probe failure or module unload, coresight framework assume there's no assocaite CTI device and just continue the enablement. It won't return failure.

If we have plan to change CTI check to be mandatory, make the coresight enablement fail if CTI is not there like other trace components. I'm perfect fine to add module_get/module_put for CTI.

Quoted 56 lines

Moving forwards we will see users who simply use pre-programmed settings from client programs such as perf. It may be that some users do not have the credentials to add and remove modules - this could be an admin function where larger systems with multiple sessions are in use.

Regards

Mike

Quoted 36 lines

Thanks, Tingwei

Quoted 33 lines

Regards

Mike

Quoted 17 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Tingwei,

Quoted 191 lines · Tingwei Zhang

Hi Mike,

Quoted 172 lines · Mike Leach

Hi Tingwei,

Quoted 15 lines · Tingwei Zhang

Hi Mike,

Quoted 12 lines · Mike Leach

Hi Tingwei,

Quoted 7 lines · Tingwei Zhang

Hi Mike,

Quoted 3 lines · Mike Leach

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang

<[email protected]>

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines · wrote:

When coresight device is in an active session, driver module of
that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 9 lines

Quoted 9 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled 

along

with

Quoted 4 lines

Quoted 4 lines

the

Quoted 3 lines

* the master device.
*/
-static void coresight_grab_device(struct coresight_device

*csdev)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+static int coresight_grab_device(struct coresight_device

*csdev)

Quoted 62 lines

Quoted 62 lines

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 4 lines

 {
        int i;

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 4 lines

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+                               goto err;
 

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 pm_runtime_get_sync(child->dev.parent);
+               }
        }
+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 667

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +7 -0

        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 

/*

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 670

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        if (!node)
                return -ENOMEM;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +2 -2

-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code

Aurora

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components

using

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

csdev->ect_dev; These are not handled in the main linear trace

path as

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

these have a star topology interlinking all components. However, when a component has an associated ect_dev then is

enabled at

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent

the

Quoted 25 lines

Quoted 15 lines

Quoted 11 lines

Quoted 3 lines

CTI being removed while a trace session is active. I suggest in coresight.c : coresight_control_assoc_ectdev()

In module unload process, devices of that module will be removed. In
this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

Quoted 3 lines

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Quoted 9 lines

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight
trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc. If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Quoted 9 lines

Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional.

For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup.

Quoted 6 lines

The module get/put do more than ensure that the system can be enabled. They ensure that no components in the coresight path can be unloaded until all paths in use are released. This ensures that clients do not get the trace system pulled out from underneath - corrupting trace data, and possibly affecting the client program.

I agree.

Quoted 6 lines

Quoted 2 lines

In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful.

And there is no guarantee that it is not programmed to perform a key task in controlling the generation of trace.

Quoted 26 lines

Quoted 5 lines

The entire programmed coresight system must be protected when in use. That includes CTI devices. Otherwise the integrity of the captured trace could be compromised, and indeed there may be no trace captured if this is relying on CTI triggers.

That's correct. If CTI is not there, the trace may be wrong or no trace as all.

Quoted 16 lines

Quoted 6 lines

For example,
One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all.

Quoted 6 lines

My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function.

Precisely - and this is what should never be allowed - why can the CTI be used to break the trace system but not other components?

Sorry - I may not have been clear here - I do not want to make having a CTI mandatory for enabling the trace session - this is an optional component.

Quoted 2 lines

We can't prevent user to unload CTI module when there's no active trace session.

Agreed - the same applies to all the other components.

Quoted 8 lines

If we don't allow CTI to be not available, we need to check whether CTI is there in build_path routine or coresight enable routine. We need return failure if CTI is not there when user enables the trace session. Currently, coresight framework check associate CTI device, but if CTI deivce is not probed due to probe failure or module unload, coresight framework assume there's no assocaite CTI device and just continue the enablement. It won't return failure.

Quoted 4 lines

If we have plan to change CTI check to be mandatory, make the coresight enablement fail if CTI is not there like other trace components. I'm perfect fine to add module_get/module_put for CTI.

If the CTI is present at the time the trace path is created, then it must be enabled, and a failure to enable the CTI fails the entire path enable - this is the operation that is implemented by coresight_control_assoc_ectdev() at this time.
If a CTI is not present at the time the trace path is created, then as you have said previously, the ect_dev pointer will be NULL and the check and enable of CTI will be skipped by coresight_control_assoc_ectdev() and have no effect on the trace path.

For the module version, if CTI is present and enabled then the module must be locked so it cannot be removed while the trace is active - this is the only change that is required. So adding get / put module in coresight_control_assoc_ectdev() will lock the module in for the duration that trace is active.

I've tested the code below to ensure that the cti module is correctly locked while trace is active, but can be removed as required when trace is inactive.

Regards

Mike

---------------------------------------------------------------------------------
static int
coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)

{
    int ect_ret = 0;
    struct coresight_device *ect_csdev = csdev->ect_dev;
    struct module *mod;

    if (!ect_csdev)
        return 0;

    if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
        return 0;

    mod = ect_csdev->dev.parent->driver->owner;

    if (enable) {
        if (try_module_get(mod)) {
            ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);

            if (ect_ret)
                module_put(mod);
        } else
            ect_ret = -ENODEV;
    } else {
        module_put(mod);
        ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
    }

    /* output warning if ECT enable is preventing trace operation */
    if (ect_ret)
        dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
             dev_name(&ect_csdev->dev),
             enable ? "enable" : "disable");
    return ect_ret;
}
-----------------------------------------------------------------------------------------
Tests:-
-----------------------
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is in use
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000834116 s, 9.8 MB/s
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-cti.ko
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# insmod 
../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod 
../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is not currently loaded
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000819117 s, 10.0 MB/s

}

Quoted 56 lines

Quoted 56 lines

Moving forwards we will see users who simply use pre-programmed settings from client programs such as perf. It may be that some users do not have the credentials to add and remove modules - this could be an admin function where larger systems with multiple sessions are in use.

Regards

Mike

Quoted 36 lines

Thanks, Tingwei

Quoted 33 lines

Regards

Mike

Quoted 17 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Quoted 4 lines

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Hi Mike,

Quoted 258 lines · Mike Leach

Hi Tingwei,

Quoted 191 lines · Tingwei Zhang

Hi Mike,

Quoted 172 lines · Mike Leach

Hi Tingwei,

Quoted 15 lines · Tingwei Zhang

Hi Mike,

Quoted 12 lines · Mike Leach

Hi Tingwei,

Quoted 7 lines · Tingwei Zhang

Hi Mike,

Quoted 3 lines · Mike Leach

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang

<[email protected]>

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines · wrote:

When coresight device is in an active session, driver module of
that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 9 lines

Quoted 9 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled

along

with

Quoted 4 lines

Quoted 4 lines

the

Quoted 3 lines

* the master device.
*/
-static void coresight_grab_device(struct coresight_device

*csdev)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+static int coresight_grab_device(struct coresight_device

*csdev)

Quoted 62 lines

Quoted 62 lines

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 4 lines

 {
        int i;

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 4 lines

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+                       if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+                               goto err;
 

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 pm_runtime_get_sync(child->dev.parent);
+               }
        }
+       if (!try_module_get(csdev->dev.parent->driver->owner))
+               goto err;

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 667

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +7 -0

        pm_runtime_get_sync(csdev->dev.parent);
+       return 0;
+err:
+       for (i--; i >= 0; i--) {
+               struct coresight_device *child;
+
+               child  = csdev->pdata->conns[i].child_dev;
+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 10 lines

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +3 -0

+                       pm_runtime_put(child->dev.parent);
+       }
+       return -ENODEV;
 }
 

/*

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        int i;
 

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 670

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +1 -1

        pm_runtime_put(csdev->dev.parent);
+       module_put(csdev->dev.parent->driver->owner);
        for (i = 0; i < csdev->pdata->nr_outport; i++) {
                struct coresight_device *child;
 
                child  = csdev->pdata->conns[i].child_dev;
-               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 686

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+               if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 2 lines

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct · line 688

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -0

                        pm_runtime_put(child->dev.parent);
+

module_put(child->dev.parent->driver->owner);

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+               }
        }
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -0

        if (!node)
                return -ENOMEM;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +2 -2

-       coresight_grab_device(csdev);
+       if (coresight_grab_device(csdev))
+               return -ENODEV;
        node->csdev = csdev;
        list_add(&node->link, path);
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code

Aurora

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

The CTI devices are associated with other coresight components

using

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

csdev->ect_dev; These are not handled in the main linear trace

path as

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

these have a star topology interlinking all components. However, when a component has an associated ect_dev then is

enabled at

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent

the

Quoted 25 lines

Quoted 15 lines

Quoted 11 lines

Quoted 3 lines

CTI being removed while a trace session is active. I suggest in coresight.c : coresight_control_assoc_ectdev()

In module unload process, devices of that module will be removed. In
this case, cti_remove() is called on all cti device when unloading cti module. All cti connections is cleaned at that time. csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev() will return since since ect_csdev is NULL.

Quoted 3 lines

I think we are safe without holding module reference since cti driver has done a pretty good job on clean up.

Quoted 9 lines

The issue here is not about clean up. We need to keep all the programmed coresight modules loaded and operational while a coresight
trace session is in progress. The CTI can be used to control the generation of trace, trigger trace related events etc. If the module is removed while a session is in progress then this programming is lost and the trace data collected may not be what is expected.

Quoted 9 lines

Got your point now. In my opinion, CTI is kinda different to other coresight components since it's optional.

For other components, they have to been there when path is built or the enablement fails. Use can either get a successfully return which means it's good and all devices/modules are there until path is released, or a failed return which means trace session can't be setup.

Quoted 6 lines

The module get/put do more than ensure that the system can be enabled. They ensure that no components in the coresight path can be unloaded until all paths in use are released. This ensures that clients do not get the trace system pulled out from underneath - corrupting trace data, and possibly affecting the client program.

I agree.

Quoted 6 lines

Quoted 2 lines

In CTI case, there's no garrantee that CTI related to the trace session is there even the enablement is successful.

And there is no guarantee that it is not programmed to perform a key task in controlling the generation of trace.

Quoted 26 lines

Quoted 5 lines

The entire programmed coresight system must be protected when in use. That includes CTI devices. Otherwise the integrity of the captured trace could be compromised, and indeed there may be no trace captured if this is relying on CTI triggers.

That's correct. If CTI is not there, the trace may be wrong or no trace as all.

Quoted 16 lines

Quoted 6 lines

For example,
One cti is required for ETM tracing session. If cti module is unloaded before enable that trace session. The enablement will still be successfully done since there's no ect_dev connected at all.

Quoted 6 lines

My point is holding cti module reference in enable path won't fix all the problem. Altenatively, user should be aware that unload cti module leads to all cti functioniol failure. With current design, the behavior is consistant on cti. User can unload cti module whenever he wants but it will break the function.

Precisely - and this is what should never be allowed - why can the CTI be used to break the trace system but not other components?

Sorry - I may not have been clear here - I do not want to make having a CTI mandatory for enabling the trace session - this is an optional component.

Quoted 2 lines

We can't prevent user to unload CTI module when there's no active trace session.

Agreed - the same applies to all the other components.

Quoted 8 lines

If we don't allow CTI to be not available, we need to check whether CTI is there in build_path routine or coresight enable routine. We need return failure if CTI is not there when user enables the trace session. Currently, coresight framework check associate CTI device, but if CTI deivce is not probed due to probe failure or module unload, coresight framework assume there's no assocaite CTI device and just continue the enablement. It won't return failure.

Quoted 4 lines

If we have plan to change CTI check to be mandatory, make the coresight enablement fail if CTI is not there like other trace components. I'm perfect fine to add module_get/module_put for CTI.

If the CTI is present at the time the trace path is created, then it must be enabled, and a failure to enable the CTI fails the entire path enable - this is the operation that is implemented by coresight_control_assoc_ectdev() at this time.
If a CTI is not present at the time the trace path is created, then as you have said previously, the ect_dev pointer will be NULL and the check and enable of CTI will be skipped by coresight_control_assoc_ectdev() and have no effect on the trace path.

For the module version, if CTI is present and enabled then the module must be locked so it cannot be removed while the trace is active - this is the only change that is required. So adding get / put module in coresight_control_assoc_ectdev() will lock the module in for the duration that trace is active.

Quoted 4 lines

I've tested the code below to ensure that the cti module is correctly locked while trace is active, but can be removed as required when trace is inactive.

Thanks a lot for coming up with the patch and testing it, Mike.

Quoted 23 lines

Regards

Mike

---------------------------------------------------------------------------------
static int
coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)

{
    int ect_ret = 0;
    struct coresight_device *ect_csdev = csdev->ect_dev;
    struct module *mod;

    if (!ect_csdev)
        return 0;

    if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
        return 0;

    mod = ect_csdev->dev.parent->driver->owner;

    if (enable) {
        if (try_module_get(mod)) {
            ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);

Quoted 8 lines

            if (ect_ret)
                module_put(mod);
        } else
            ect_ret = -ENODEV;
    } else {
        module_put(mod);
        ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
    }

If CTI module is removed before enablement of ETM and installed after ETM enablement and before ETM disablement. We have a unbalanced module reference number here. I made one test to prove this with above code.

We may need move module_put() after disable(). Return proper error from disable() to indicate CTI was not enabled before so we can skip module_put().

Tests:
db845c:/sys/bus/coresight/devices # echo 1 > tmc_etr0/enable_sink db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source db845c:/sys/bus/coresight/devices # rmmod coresight_cti
rmmod: failed to unload coresight_cti: Try again 1|db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source db845c:/sys/bus/coresight/devices # rmmod coresight_cti db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source db845c:/sys/bus/coresight/devices # insmod /data/modules/coresight-cti.ko db845c:/sys/bus/coresight/devices # echo 1 > etm2/enable_source db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source db845c:/sys/bus/coresight/devices # rmmod coresight_cti db845c:/sys/bus/coresight/devices #

Above module removal should fail since etm2 is still active now.

Quoted 135 lines

    /* output warning if ECT enable is preventing trace operation */
    if (ect_ret)
        dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
             dev_name(&ect_csdev->dev),
             enable ? "enable" : "disable");
    return ect_ret;
}
-----------------------------------------------------------------------------------------
Tests:-
-----------------------
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is in use
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000834116 s, 9.8 MB/s
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-cti.ko
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# insmod
../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod
../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is not currently loaded
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000819117 s, 10.0 MB/s

}

Quoted 60 lines

Quoted 60 lines

Moving forwards we will see users who simply use pre-programmed settings from client programs such as perf. It may be that some users do not have the credentials to add and remove modules - this could be an admin function where larger systems with multiple sessions are in use.

Regards

Mike

Quoted 36 lines

Thanks, Tingwei

Quoted 33 lines

Regards

Mike

Quoted 17 lines

Let me know if you still think we need hold the module reference.

Thanks, Tingwei

Quoted 14 lines

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 4 lines

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/hwtracing/coresight/coresight.c +1 -1

index b7151c5f81b1..17bc76ea86ae 100644
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * don't appear on the trace path, they should be handled along with the
  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

drivers/hwtracing/coresight/coresight.c +14 -1

 	pm_runtime_get_sync(csdev->dev.parent);
+	return 0;
+err:
+	for (i--; i >= 0; i--) {
+		struct coresight_device *child;
+
+		child  = csdev->pdata->conns[i].child_dev;
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+			pm_runtime_put(child->dev.parent);
+	}
+	return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
 	int i;
 
 	pm_runtime_put(csdev->dev.parent);
+	module_put(csdev->dev.parent->driver->owner);
 	for (i = 0; i < csdev->pdata->nr_outport; i++) {
 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
 			pm_runtime_put(child->dev.parent);
+			module_put(child->dev.parent->driver->owner);
+		}
 	}
 }
 

Quoted 3 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Are you sure this works? Why is it needed at all? Why not just tear down the children properly when a module is removed so that you don't need this at all?

Quoted 23 lines

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/hwtracing/coresight/coresight.c +1 -1

index b7151c5f81b1..17bc76ea86ae 100644
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * don't appear on the trace path, they should be handled along with the
  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
 		struct coresight_device *child;
 

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 652

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -1

 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+			if (!try_module_get(child->dev.parent->driver->owner))

Why the child's parent? Why not the child itself?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 653

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+				goto err;

What about the error given to you here? Why throw that away?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 658

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;

You don't reduce the child's parent's driver owner module reference here?

Ugh, that's a horid sentence :(

Quoted 32 lines

drivers/hwtracing/coresight/coresight.c +14 -1

 	pm_runtime_get_sync(csdev->dev.parent);
+	return 0;
+err:
+	for (i--; i >= 0; i--) {
+		struct coresight_device *child;
+
+		child  = csdev->pdata->conns[i].child_dev;
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+			pm_runtime_put(child->dev.parent);
+	}
+	return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
 	int i;
 
 	pm_runtime_put(csdev->dev.parent);
+	module_put(csdev->dev.parent->driver->owner);
 	for (i = 0; i < csdev->pdata->nr_outport; i++) {
 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
 			pm_runtime_put(child->dev.parent);
+			module_put(child->dev.parent->driver->owner);
+		}
 	}
 }
 

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct coresight_device *csdev, · line 743

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c +2 -1

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	if (!node)
 		return -ENOMEM;
 
-	coresight_grab_device(csdev);
+	if (coresight_grab_device(csdev))
+		return -ENODEV;

Why not return the error given to you?

thanks,

greg k-h

Hi Greg,

Quoted 8 lines · Greg Kroah-Hartman

Quoted 3 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Are you sure this works? Why is it needed at all? Why not just tear down the children properly when a module is removed so that you don't need this at all?

Using the terms parent and child is somewhat ambiguous... This is not a parent-child relationship but simply an association between devices, something like port 1 on device "parent" is connected to port 2 on device "child". The parent-child nomenclature was chosen to reflect that a device appears before another in a coresight path. Otherwise there is no other relation between devices, hence the choice of using try_get_module()/put_module() to prevent drivers from being taken away. I'd be happy to proceed differently but haven't found better options.

Going back to parent/child, we could have chosen left/right, up/down or A/B, all of which are just as confusion.

Quoted 21 lines

Quoted 21 lines

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/hwtracing/coresight/coresight.c +1 -1

index b7151c5f81b1..17bc76ea86ae 100644
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * don't appear on the trace path, they should be handled along with the
  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
 		struct coresight_device *child;
 

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 652

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -1

 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+			if (!try_module_get(child->dev.parent->driver->owner))

Why the child's parent? Why not the child itself?

The device structure of each coresight_device is not associated with a driver. It is there to take advantages of device goodies such as dev.type, dev.group, dev.release and dev.bus. Coresight IP blocks are discovered on the AMBA bus and as such amba_device::dev::driver holds the driver itself. In coresight_register() the association coresigth::dev::parent = amba_device::dev is made.

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 653

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+				goto err;

What about the error given to you here? Why throw that away?

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 658

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;

You don't reduce the child's parent's driver owner module reference here?

Here @parent is referencing the current device. Now that helper devices connected to any of its outgoing ports have been enabled (and a reference count to the helper device driver incremented), a reference count to the current device driver can also be incremented.

Quoted 21 lines · Mathieu Poirier

Hi Greg,

Quoted 8 lines · Greg Kroah-Hartman

Quoted 3 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Are you sure this works? Why is it needed at all? Why not just tear down the children properly when a module is removed so that you don't need this at all?

Using the terms parent and child is somewhat ambiguous... This is not a parent-child relationship but simply an association between devices, something like port 1 on device "parent" is connected to port 2 on device "child". The parent-child nomenclature was chosen to reflect that a device appears before another in a coresight path. Otherwise there is no other relation between devices, hence the choice of using try_get_module()/put_module() to prevent drivers from being taken away. I'd be happy to proceed differently but haven't found better options.

Quoted 2 lines

Going back to parent/child, we could have chosen left/right, up/down or A/B, all of which are just as confusion.

Ok, thanks.

But this causes confusion for everyone as seen below:

Quoted 49 lines

Quoted 21 lines

Quoted 21 lines

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/hwtracing/coresight/coresight.c +1 -1

index b7151c5f81b1..17bc76ea86ae 100644
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * don't appear on the trace path, they should be handled along with the
  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
 		struct coresight_device *child;
 

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 652

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +2 -1

 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+			if (!try_module_get(child->dev.parent->driver->owner))

Why the child's parent? Why not the child itself?

The device structure of each coresight_device is not associated with a driver. It is there to take advantages of device goodies such as dev.type, dev.group, dev.release and dev.bus. Coresight IP blocks are discovered on the AMBA bus and as such amba_device::dev::driver holds the driver itself. In coresight_register() the association coresigth::dev::parent = amba_device::dev is made.

Quoted 14 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 653

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+				goto err;

What about the error given to you here? Why throw that away?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 658

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;

You don't reduce the child's parent's driver owner module reference here?

Quoted 4 lines

Here @parent is referencing the current device. Now that helper devices connected to any of its outgoing ports have been enabled (and a reference count to the helper device driver incremented), a reference count to the current device driver can also be incremented.

I mean the fact that your error handling does not seem to roll back the module reference count you got up above in the other loop.

Or if it does, it's really really not obvious, and should at the very least, be commented as to how it's all cleaning up properly.

thanks,

greg k-h

Quoted 10 lines · Greg Kroah-Hartman

Quoted 9 lines · Mathieu Poirier

Hi Greg,

Quoted 6 lines · Greg Kroah-Hartman

Quoted 3 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Are you sure this works? Why is it needed at all? Why not just

tear

Quoted 1 line

Quoted 1 line

Quoted 1 line

down the children properly when a module is removed so that you

don't

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

need this at all?

Using the terms parent and child is somewhat ambiguous... This is not

a

Quoted 1 line

Quoted 1 line

parent-child relationship but simply an association between devices,

something

Quoted 1 line

Quoted 1 line

like port 1 on device "parent" is connected to port 2 on device

"child". The

Quoted 1 line

Quoted 1 line

parent-child nomenclature was chosen to reflect that a device appears

before

Quoted 1 line

Quoted 1 line

another in a coresight path. Otherwise there is no other relation

between

Quoted 1 line

Quoted 1 line

devices, hence the choice of using try_get_module()/put_module() to

prevent

Quoted 1 line

Quoted 1 line

drivers from being taken away. I'd be happy to proceed differently

but haven't

Quoted 3 lines

Quoted 3 lines

found better options.

Going back to parent/child, we could have chosen left/right, up/down

or A/B, all

Quoted 9 lines

Quoted 1 line

of which are just as confusion.

Ok, thanks.

But this causes confusion for everyone as seen below:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27

+++++++++++++++++++++----

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along

with the

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +0 -1

 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER)

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 651

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+		if (child && child->type ==

CORESIGHT_DEV_TYPE_HELPER) {

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 652

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+			if

(!try_module_get(child->dev.parent->driver->owner))

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Why the child's parent? Why not the child itself?

The device structure of each coresight_device is not associated with a

driver.

Quoted 1 line

Quoted 1 line

It is there to take advantages of device goodies such as dev.type,

dev.group,

Quoted 1 line

Quoted 1 line

dev.release and dev.bus. Coresight IP blocks are discovered on the

AMBA bus and as

Quoted 1 line

Quoted 1 line

such amba_device::dev::driver holds the driver itself. In

coresight_register()

Quoted 17 lines

Quoted 17 lines

the association coresigth::dev::parent = amba_device::dev is made.

Quoted 14 lines

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 653

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +1 -0

+				goto err;

What about the error given to you here? Why throw that away?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 658

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;

You don't reduce the child's parent's driver owner module reference here?

Quoted 1 line

Quoted 1 line

Here @parent is referencing the current device. Now that helper

devices

Quoted 1 line

Quoted 1 line

connected to any of its outgoing ports have been enabled (and a

reference count

Quoted 1 line

Quoted 1 line

to the helper device driver incremented), a reference count to the

current device

Quoted 2 lines

Quoted 1 line

driver can also be incremented.

Quoted 2 lines

I mean the fact that your error handling does not seem to roll back the module reference count you got up above in the other loop.

Ah! You were talking about the error condition, while I thought you were referring to the normal execution path. I am in agreement with all your comments on error
handling.

Quoted 4 lines

Or if it does, it's really really not obvious, and should at the very least, be commented as to how it's all cleaning up properly.

Quoted 3 lines

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 3 lines · Tingwei Zhang

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded.

Are you sure this works? Why is it needed at all? Why not just tear down the children properly when a module is removed so that you don't need this at all?

Review

Quoted 7 lines

Signed-off-by Tingwei Zhang <[email protected]>

---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..17bc76ea86ae 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -640,7 +640,7 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 640

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

  * don't appear on the trace path, they should be handled along with

the

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * the master device.
  */
-static void coresight_grab_device(struct coresight_device *csdev)
+static int coresight_grab_device(struct coresight_device *csdev)
 {
 	int i;
 

@@ -648,10 +648,25 @@ static void coresight_grab_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev)

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c +2 -1

 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+			if

(!try_module_get(child->dev.parent->driver->owner))

Why the child's parent? Why not the child itself?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct · line 653

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +1 -0

+				goto err;

What about the error given to you here? Why throw that away?

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct coresight_device *csdev) · line 658

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +3 -0

 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;

You don't reduce the child's parent's driver owner module reference here?

Ugh, that's a horid sentence :(

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 15 lines

drivers/hwtracing/coresight/coresight.c +10 -0

 	pm_runtime_get_sync(csdev->dev.parent);
+	return 0;
+err:
+	for (i--; i >= 0; i--) {
+		struct coresight_device *child;
+
+		child  = csdev->pdata->conns[i].child_dev;
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+			pm_runtime_put(child->dev.parent);
+	}
+	return -ENODEV;
 }
 
 /*

@@ -663,12 +678,15 @@ static void coresight_drop_device(struct

coresight_device *csdev)

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 17 lines

drivers/hwtracing/coresight/coresight.c +4 -1

 	int i;
 
 	pm_runtime_put(csdev->dev.parent);
+	module_put(csdev->dev.parent->driver->owner);
 	for (i = 0; i < csdev->pdata->nr_outport; i++) {
 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
 			pm_runtime_put(child->dev.parent);
+			module_put(child->dev.parent->driver->owner);
+		}
 	}
 }
 

@@ -721,7 +739,8 @@ static int _coresight_build_path(struct

coresight_device *csdev,

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct coresight_device *csdev, · line 743

Quoted 6 lines

drivers/hwtracing/coresight/coresight.c +2 -1

 	if (!node)
 		return -ENOMEM;
 
-	coresight_grab_device(csdev);
+	if (coresight_grab_device(csdev))
+		return -ENODEV;

Why not return the error given to you?

thanks,

greg k-h