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

[Suzuki K Poulose]

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 at codeaurora.org>
> Tested-by: Mike Leach <mike.leach at 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;
}

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