On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote:

Hi Greg,

On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:

quoted

On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang 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.

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. 

Ok, thanks.

But this causes confusion for everyone as seen below:

quoted

quoted

Signed-off-by: Tingwei Zhang <redacted>
---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
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)
  * 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))

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

quoted

+				goto err;

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

quoted

 			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. 

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

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