Re: [PATCH v7 06/25] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang <tingweiz@codeaurora.org>]

On Wed, Aug 05, 2020 at 06:55:40PM +0800, Suzuki K Poulose wrote:
> On 08/05/2020 03:54 AM, 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.
> >Use get_device()/put_device() to protect device data
> >in the middle of active session.
> >
> >Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> >Tested-by: Mike Leach <mike.leach@linaro.org>
> >---
> >  drivers/hwtracing/coresight/coresight.c | 39 +++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/hwtracing/coresight/coresight.c
> b/drivers/hwtracing/coresight/coresight.c
> >index b7151c5f81b1..1626bc885dfe 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;
> 
> Please could you add a pair of helper functions to hold/drop reference
> to a device/driver ? i.e,
> 
> static inline bool coresight_get_ref(struct coresight_device *csdev)
> {
> 	struct device *dev = csdev->dev.parent;
> 
> 	/* Make sure the driver cant be removed */
> 	if (!try_module_get(dev->driver->owner))
> 		return false;
> 	/* Make sure the device can't go away */
> 	get_device(dev);
> 	pm_runtime_get_sync(dev);
> 	return true;
> }
>
Sure. I'll add it to next revision.

Thanks,
Tingwei 
> static inline void coresight_put_ref(struct coresight_device *csdev)
> {
> 	struct device *dev = csdev->dev.parent;
> 
> 	pm_runtime_put(dev);
> 	put_device(dev);
> 	module_put(dev->driver->owner);
> }
> 
> 
> >@@ -648,10 +648,30 @@ 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;
> >+			get_device(child->dev.parent);
> >  			pm_runtime_get_sync(child->dev.parent);
> >+		}
> >  	}
> 
> This could then be :
> 		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> 			if (!coresight_get_ref(child))
> 				goto err;
> 
> >+	if (!try_module_get(csdev->dev.parent->driver->owner))
> >+		goto err;
> >+	get_device(csdev->dev.parent);
> >  	pm_runtime_get_sync(csdev->dev.parent);
> 
> 	if (coresight_get_ref(csdev))
> 		return 0;
> 
> >+	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);
> >+			put_device(child->dev.parent);
> >+			module_put(child->dev.parent->driver->owner);
> >+		}
> 
> And this could be :
> 
> 		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> 			coresight_put_ref(child);
> 
> >+	}
> >+	return -ENODEV;
> >  }
> >  /*
> >@@ -663,12 +683,17 @@ static void coresight_drop_device(struct
> coresight_device *csdev)
> >  	int i;
> >  	pm_runtime_put(csdev->dev.parent);
> >+	put_device(csdev->dev.parent);
> >+	module_put(csdev->dev.parent->driver->owner);
> 
> 	coresight_put_ref(csdev);
> 
> >  	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);
> >+			put_device(child->dev.parent);
> >+			module_put(child->dev.parent->driver->owner);
> >+		}
> 
> 	coresight_put_ref(child);
> 
> >  	}
> >  }
> >@@ -687,7 +712,7 @@ static int _coresight_build_path(struct
> coresight_device *csdev,
> >  				 struct coresight_device *sink,
> >  				 struct list_head *path)
> >  {
> >-	int i;
> >+	int i, ret;
> >  	bool found = false;
> >  	struct coresight_node *node;
> >@@ -721,7 +746,11 @@ static int _coresight_build_path(struct
> coresight_device *csdev,
> >  	if (!node)
> >  		return -ENOMEM;
> >-	coresight_grab_device(csdev);
> >+	ret = coresight_grab_device(csdev);
> >+	if (ret) {
> >+		kfree(node);
> >+		return ret;
> >+	}
> >  	node->csdev = csdev;
> >  	list_add(&node->link, path);
> >
> 
> 
> Cheers
> Suzuki
> 
> _______________________________________________
> 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