Hi Dan,
Thanks for looking at this. I agree with the patch, other than the one
change below.
I have compiled and run on my DB410 system, against 5.8-rc1.
On Fri, 12 Jun 2020 at 14:46, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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>
> ---
> Please note!!! I cannot compile this patch. Mike can you review it?
>
> 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] = drvdata;
This should remain as cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
here. We are reversing the assignment in cti_pm_setup().
> + 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
>
Reviewed-by Mike Leach <mike.leach@xxxxxxxxxx>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
