Re: [PATCH v26 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

[Mathieu Poirier <mathieu.poirier@linaro.org>]

[...]

>     >     > +/*
>     >     > + * zynqmp_r5_remoteproc_probe
>     >     > + *
>     >     > + * @pdev: domain platform device for R5 cluster
>     >     > + *
>     >     > + * called when driver is probed, for each R5 core specified in DT,
>     >     > + * setup as needed to do remoteproc-related operations
>     >     > + *
>     >     > + * Return: 0 for success, negative value for failure.
>     >     > + */
>     >     > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
>     >     > +{
>     >     > +	int ret, core_count;
>     >     > +	struct device *dev = &pdev->dev;
>     >     > +	struct device_node *nc;
>     >     > +	enum rpu_oper_mode rpu_mode = PM_RPU_MODE_LOCKSTEP;
>     >     > +	struct list_head *cluster; /* list to track each core's rproc */
>     >     > +	struct zynqmp_r5_rproc *z_rproc;
>     >     > +	struct platform_device *child_pdev;
>     >     > +	struct list_head *pos;
>     >     > +
>     >     > +	ret = of_property_read_u32(dev->of_node, "xlnx,cluster-mode", &rpu_mode);
>     >     > +	if (ret < 0 || (rpu_mode != PM_RPU_MODE_LOCKSTEP &&
>     >     > +			rpu_mode != PM_RPU_MODE_SPLIT)) {
>     >     > +		dev_err(dev, "invalid cluster mode: ret %d mode %x\n",
>     >     > +			ret, rpu_mode);
>     >     > +		return ret;
>     >     > +	}
>     >     > +
>     >     > +	dev_dbg(dev, "RPU configuration: %s\n",
>     >     > +		rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" : "split");
>     >     > +
>     >     > +	/*
>     >     > +	 * if 2 RPUs provided but one is lockstep, then we have an
>     >     > +	 * invalid configuration.
>     >     > +	 */
>     >     > +
>     >     > +	core_count = of_get_available_child_count(dev->of_node);
>     >     > +	if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && core_count != 1) ||
>     >     > +	    core_count > MAX_RPROCS)
>     >     > +		return -EINVAL;
>     >     > +
>     >     > +	cluster = devm_kzalloc(dev, sizeof(*cluster), GFP_KERNEL);
>     >     > +	if (!cluster)
>     >     > +		return -ENOMEM;
>     >     > +	INIT_LIST_HEAD(cluster);
>     >     > +
>     >     > +	ret = devm_of_platform_populate(dev);
>     >     > +	if (ret) {
>     >     > +		dev_err(dev, "devm_of_platform_populate failed, ret = %d\n", ret);
>     >     > +		return ret;
>     >     > +	}
>     >     > +
>     >     > +	/* probe each individual r5 core's remoteproc-related info */
>     >     > +	for_each_available_child_of_node(dev->of_node, nc) {
>     >     > +		child_pdev = of_find_device_by_node(nc);
>     > 
>     >     The device reference needs to be dropped after use, as described in the function
>     >     documentation.
>     > 
>     >     I'm out of time - I will continue tomorrow.
>     > 
>     >     Mathieu
>     > 
>     > 
>     > [Ben] By this do you mean that for each platform_device should have a call like
>     > 	platform_set_drvdata(child_pdev, NULL); if it fails? or something else?
> 
>     Have another read at the documentation and look at how other people have used
>     it.  You may already be aware but Bootlin's kernel cross-reference tool is
>     really good for that.
> 
>     https://elixir.bootlin.com/linux/v5.12-rc3/source
> 
> If I understand what you are saying I will add calls for put_device(child_pdev) in error handling and at end of the loop.

That's one part of it.  But what will happen if there is no errors to deal with?
Where will the reference to child_pdev->dev be dropped?

> 
>