Re: [PATCH v2] coresight: cti: Fix error handling in probe

[Mathieu Poirier]

On Wed, Jun 17, 2020 at 08:15:50PM +0300, Dan Carpenter wrote:
> There were a couple problems with error handling in the probe function:
> 1)  If the "drvdata" allocation failed then it lead to a NULL
>     dereference.
> 2)  On several error paths we decremented "nr_cti_cpu" before it was
>     incremented which lead to a reference counting bug.
> 
> There were also some parts of the error handling which were not bugs but
> were messy.  The error handling was confusing to read.  It printed some
> unnecessary error messages.
> 
> The simplest way to fix these problems was to create a cti_pm_setup()
> function that did all the power management setup in one go.  That way
> when we call cti_pm_release() we don't have to deal with the
> complications of a partially configured power management config.
> 
> I reversed the "if (drvdata->ctidev.cpu >= 0)" condition in cti_pm_release()
> so that it mirros the new cti_pm_setup() function.
> 
> Fixes: 6a0953ce7de9 ("coresight: cti: Add CPU idle pm notifer to CTI devices")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> v2: I accidentally introduced a bug in cti_pm_release() in v1.

Thanks for the cleanup.  I'll send this to Greg for a 5.8 fixup.

Regards,
Mathieu

> 
>  drivers/hwtracing/coresight/coresight-cti.c | 96 ++++++++++++---------
>  1 file changed, 54 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> index 40387d58c8e7..d2da5bf9f552 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.c
> +++ b/drivers/hwtracing/coresight/coresight-cti.c
> @@ -747,17 +747,50 @@ static int cti_dying_cpu(unsigned int cpu)
>  	return 0;
>  }
>  
> +static int cti_pm_setup(struct cti_drvdata *drvdata)
> +{
> +	int ret;
> +
> +	if (drvdata->ctidev.cpu == -1)
> +		return 0;
> +
> +	if (nr_cti_cpu)
> +		goto done;
> +
> +	cpus_read_lock();
> +	ret = cpuhp_setup_state_nocalls_cpuslocked(
> +			CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> +			"arm/coresight_cti:starting",
> +			cti_starting_cpu, cti_dying_cpu);
> +	if (ret) {
> +		cpus_read_unlock();
> +		return ret;
> +	}
> +
> +	ret = cpu_pm_register_notifier(&cti_cpu_pm_nb);
> +	cpus_read_unlock();
> +	if (ret) {
> +		cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
> +		return ret;
> +	}
> +
> +done:
> +	nr_cti_cpu++;
> +	cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
> +
> +	return 0;
> +}
> +
>  /* release PM registrations */
>  static void cti_pm_release(struct cti_drvdata *drvdata)
>  {
> -	if (drvdata->ctidev.cpu >= 0) {
> -		if (--nr_cti_cpu == 0) {
> -			cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
> +	if (drvdata->ctidev.cpu == -1)
> +		return;
>  
> -			cpuhp_remove_state_nocalls(
> -				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
> -		}
> -		cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> +	cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> +	if (--nr_cti_cpu == 0) {
> +		cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
> +		cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
>  	}
>  }
>  
> @@ -823,19 +856,14 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	/* driver data*/
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> -	if (!drvdata) {
> -		ret = -ENOMEM;
> -		dev_info(dev, "%s, mem err\n", __func__);
> -		goto err_out;
> -	}
> +	if (!drvdata)
> +		return -ENOMEM;
>  
>  	/* Validity for the resource is already checked by the AMBA core */
>  	base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(base)) {
> -		ret = PTR_ERR(base);
> -		dev_err(dev, "%s, remap err\n", __func__);
> -		goto err_out;
> -	}
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
>  	drvdata->base = base;
>  
>  	dev_set_drvdata(dev, drvdata);
> @@ -854,8 +882,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  	pdata = coresight_cti_get_platform_data(dev);
>  	if (IS_ERR(pdata)) {
>  		dev_err(dev, "coresight_cti_get_platform_data err\n");
> -		ret =  PTR_ERR(pdata);
> -		goto err_out;
> +		return  PTR_ERR(pdata);
>  	}
>  
>  	/* default to powered - could change on PM notifications */
> @@ -867,35 +894,20 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  					       drvdata->ctidev.cpu);
>  	else
>  		cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
> -	if (!cti_desc.name) {
> -		ret = -ENOMEM;
> -		goto err_out;
> -	}
> +	if (!cti_desc.name)
> +		return -ENOMEM;
>  
>  	/* setup CPU power management handling for CPU bound CTI devices. */
> -	if (drvdata->ctidev.cpu >= 0) {
> -		cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
> -		if (!nr_cti_cpu++) {
> -			cpus_read_lock();
> -			ret = cpuhp_setup_state_nocalls_cpuslocked(
> -				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> -				"arm/coresight_cti:starting",
> -				cti_starting_cpu, cti_dying_cpu);
> -
> -			if (!ret)
> -				ret = cpu_pm_register_notifier(&cti_cpu_pm_nb);
> -			cpus_read_unlock();
> -			if (ret)
> -				goto err_out;
> -		}
> -	}
> +	ret = cti_pm_setup(drvdata);
> +	if (ret)
> +		return ret;
>  
>  	/* create dynamic attributes for connections */
>  	ret = cti_create_cons_sysfs(dev, drvdata);
>  	if (ret) {
>  		dev_err(dev, "%s: create dynamic sysfs entries failed\n",
>  			cti_desc.name);
> -		goto err_out;
> +		goto pm_release;
>  	}
>  
>  	/* set up coresight component description */
> @@ -908,7 +920,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  	drvdata->csdev = coresight_register(&cti_desc);
>  	if (IS_ERR(drvdata->csdev)) {
>  		ret = PTR_ERR(drvdata->csdev);
> -		goto err_out;
> +		goto pm_release;
>  	}
>  
>  	/* add to list of CTI devices */
> @@ -927,7 +939,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev_info(&drvdata->csdev->dev, "CTI initialized\n");
>  	return 0;
>  
> -err_out:
> +pm_release:
>  	cti_pm_release(drvdata);
>  	return ret;
>  }
> -- 
> 2.27.0