Hi Mike,
On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:

Hi Tingwei,

On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang [off-list ref]
wrote:

quoted

Hi Mike,

 Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:

quoted

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang [off-list ref]
wrote:

quoted

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 <redacted>
---
 drivers/hwtracing/coresight/coresight.c | 27

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

quoted

quoted

quoted

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

diff --git a/drivers/hwtracing/coresight/coresight.c

b/drivers/hwtracing/coresight/coresight.c

quoted

index b7151c5f81b1..17bc76ea86ae 100644

--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c

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

*coresight_get_sink_by_id(u32 id)

quoted

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

with

quoted

quoted

the

quoted

  * 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

                struct coresight_device *child;

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

CORESIGHT_DEV_TYPE_HELPER)

quoted

quoted

quoted

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

CORESIGHT_DEV_TYPE_HELPER) {

quoted

quoted

quoted

+                       if

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

quoted

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

quoted

quoted

quoted

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

 /*

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

coresight_device *csdev)

quoted

        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)

quoted

quoted

quoted

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

CORESIGHT_DEV_TYPE_HELPER) {

quoted

quoted

quoted

                        pm_runtime_put(child->dev.parent);
+

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

quoted

quoted

quoted

+               }
        }
 }

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

coresight_device *csdev,

quoted

        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

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()

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.

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
 

Regards

Mike

quoted

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

Thanks, Tingwei

quoted

Regards

Mike


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

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

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

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

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