Re: [PATCH v3 25/30] coresight: Use fwnode handle instead of device names

[Mathieu Poirier]

On Tue, May 07, 2019 at 11:52:52AM +0100, Suzuki K Poulose wrote:
> We rely on the device names to find a CoreSight device on the
> coresight bus. The device name however is obtained from the platform,
> which is bound to the real platform/amba device. As we are about
> to use different naming scheme for the coresight devices, we can't
> rely on the platform device name to find the corresponding
> coresight device. Instead we use the platform agnostic
> "fwnode handle" of the parent device to find the devices.
> We also reuse the same fwnode as the parent for the Coresight
> device we create.
> 
> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> ---
>  drivers/hwtracing/coresight/coresight-platform.c | 14 +++++---
>  drivers/hwtracing/coresight/coresight-priv.h     |  6 ++--
>  drivers/hwtracing/coresight/coresight.c          | 42 +++++++++++++++++++-----
>  include/linux/coresight.h                        |  4 +--
>  4 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 4394095..49112a5 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -36,7 +36,7 @@ static int coresight_alloc_conns(struct device *dev,
>  	return 0;
>  }
>  
> -static int coresight_device_fwnode_match(struct device *dev, void *fwnode)
> +int coresight_device_fwnode_match(struct device *dev, void *fwnode)
>  {
>  	return dev_fwnode(dev) == fwnode;
>  }
> @@ -219,9 +219,15 @@ static int of_coresight_parse_endpoint(struct device *dev,
>  		}
>  
>  		conn->outport = endpoint.port;
> -		conn->child_name = devm_kstrdup(dev,
> -						dev_name(rdev),
> -						GFP_KERNEL);
> +		/*
> +		 * Hold the refcount to the target device. This could be
> +		 * released via:
> +		 * 1) coresight_release_platform_data() if the probe fails or
> +		 *    this device is unregistered.
> +		 * 2) While removing the target device via
> +		 *    coresight_remove_match()
> +		 */
> +		conn->child_fwnode = fwnode_handle_get(rdev_fwnode);
>  		conn->child_port = rendpoint.port;
>  		/* Connection record updated */
>  		ret = 1;
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index c216421..8b07fe5 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -200,8 +200,8 @@ static inline void *coresight_get_uci_data(const struct amba_id *id)
>  	return 0;
>  }
>  
> -static inline void
> -coresight_release_platform_data(struct coresight_platform_data *pdata)
> -{}
> +void coresight_release_platform_data(struct coresight_platform_data *pdata);
> +
> +int coresight_device_fwnode_match(struct device *dev, void *fwnode);
>  
>  #endif
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 526141c..c9c8a8c 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -978,6 +978,7 @@ static void coresight_device_release(struct device *dev)
>  {
>  	struct coresight_device *csdev = to_coresight_device(dev);
>  
> +	fwnode_handle_put(csdev->dev.fwnode);
>  	kfree(csdev->refcnt);
>  	kfree(csdev);
>  }
> @@ -1009,13 +1010,11 @@ static int coresight_orphan_match(struct device *dev, void *data)
>  		/* We have found at least one orphan connection */
>  		if (conn->child_dev == NULL) {
>  			/* Does it match this newly added device? */
> -			if (conn->child_name &&
> -			    !strcmp(dev_name(&csdev->dev), conn->child_name)) {
> +			if (conn->child_fwnode ==  csdev->dev.fwnode)

There is one space too mnay after the '=='. 

>  				conn->child_dev = csdev;
> -			} else {
> +			else
>  				/* This component still has an orphan */
>  				still_orphan = true;
> -			}
>  		}
>  	}
>  
> @@ -1047,9 +1046,9 @@ static void coresight_fixup_device_conns(struct coresight_device *csdev)
>  		struct coresight_connection *conn = &csdev->pdata->conns[i];
>  		struct device *dev = NULL;
>  
> -		if (conn->child_name)
> -			dev = bus_find_device_by_name(&coresight_bustype, NULL,
> -						      conn->child_name);
> +		dev = bus_find_device(&coresight_bustype, NULL,
> +				      (void *)conn->child_fwnode,
> +				      coresight_device_fwnode_match);
>  		if (dev) {
>  			conn->child_dev = to_coresight_device(dev);
>  			/* and put reference from 'bus_find_device()' */
> @@ -1084,9 +1083,15 @@ static int coresight_remove_match(struct device *dev, void *data)
>  		if (conn->child_dev == NULL)
>  			continue;
>  
> -		if (!strcmp(dev_name(&csdev->dev), conn->child_name)) {
> +		if (csdev->dev.fwnode == conn->child_fwnode) {
>  			iterator->orphan = true;
>  			conn->child_dev = NULL;
> +			/*
> +			 * Drop the reference to the handle for the remote
> +			 * device acquired in parsing the connections from
> +			 * platform data.
> +			 */
> +			fwnode_handle_put(conn->child_fwnode);
>  			/* No need to continue */
>  			break;
>  		}
> @@ -1166,6 +1171,22 @@ static int __init coresight_init(void)
>  }
>  postcore_initcall(coresight_init);
>  
> +/*
> + * coresight_release_platform_data: Release references to the devices connected
> + * to the output port of this device.
> + */
> +void coresight_release_platform_data(struct coresight_platform_data *pdata)
> +{
> +	int i;
> +
> +	for (i = 0; i < pdata->nr_outport; i++) {
> +		if (pdata->conns[i].child_fwnode) {
> +			fwnode_handle_put(pdata->conns[i].child_fwnode);
> +			pdata->conns[i].child_fwnode = 0;

I know there isn't a set standard for using '0' or NULL but since the latter is
used throughout the file I'd like to avoid an hybrid scheme.

> +		}
> +	}
> +}
> +
>  struct coresight_device *coresight_register(struct coresight_desc *desc)
>  {
>  	int ret;
> @@ -1210,6 +1231,11 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>  	csdev->dev.parent = desc->dev;
>  	csdev->dev.release = coresight_device_release;
>  	csdev->dev.bus = &coresight_bustype;
> +	/*
> +	 * Hold the reference to our parent device. This will be
> +	 * dropped only in coresight_device_release().
> +	 */
> +	csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
>  	dev_set_name(&csdev->dev, "%s", desc->name);
>  
>  	ret = device_register(&csdev->dev);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index b67d507..b40544b 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -126,15 +126,15 @@ struct coresight_desc {
>  /**
>   * struct coresight_connection - representation of a single connection
>   * @outport:	a connection's output port number.
> - * @chid_name:	remote component's name.
>   * @child_port:	remote component's port number @output is connected to.
> + * @chid_fwnode: remote component's fwnode handle.
>   * @child_dev:	a @coresight_device representation of the component
>  		connected to @outport.
>   */
>  struct coresight_connection {
>  	int outport;
> -	const char *child_name;
>  	int child_port;
> +	struct fwnode_handle *child_fwnode;
>  	struct coresight_device *child_dev;
>  };
> 

With the above: 

Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
 
> -- 
> 2.7.4
>