Fri, Jul 24, 2020 at 03:15:40AM +0800, Mathieu Poirier wrote:

On Thu, Jul 23, 2020 at 08:18:37PM +0200, Greg Kroah-Hartman wrote:

quoted

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

quoted

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

quoted

quoted

quoted

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

don't

quoted

quoted

quoted

need this at all?

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

a

quoted

quoted

parent-child relationship but simply an association between devices,

something

quoted

quoted

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

"child".  The

quoted

quoted

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

before

quoted

quoted

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

between

quoted

quoted

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

prevent

quoted

quoted

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

but haven't

quoted

quoted

found better options.

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

or A/B, all

quoted

quoted

of which are just as confusion. 

Ok, thanks.

But this causes confusion for everyone as seen below:

quoted

quoted

quoted

Signed-off-by: Tingwei Zhang <redacted>
---
 drivers/hwtracing/coresight/coresight.c | 27

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

quoted

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

quoted

quoted

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

quoted

quoted

quoted

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

with the

quoted

quoted

quoted

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

quoted

quoted

quoted

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

CORESIGHT_DEV_TYPE_HELPER)

quoted

quoted

quoted

quoted

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

CORESIGHT_DEV_TYPE_HELPER) {

quoted

quoted

quoted

quoted

+			if

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

quoted

quoted

quoted

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

The device structure of each coresight_device is not associated with a

driver.

quoted

quoted

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

dev.group,

quoted

quoted

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

AMBA bus and as

quoted

quoted

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

coresight_register()

quoted

quoted

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

quoted

quoted

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

reference count 

quoted

quoted

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

current device

quoted

quoted

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.

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. 

Thanks to point this error out, Greg and Mathieu. I'll check and fix
them in next revision.
 

quoted

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

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