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:
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:
On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang 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.
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:
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
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.
goto err;What about the error given to you here? Why throw that away?
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.
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.
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