Re: [PATCHv2] coresight: etm4x: Fix etm4_count race by moving cpuhp callbacks to init

[Sai Prakash Ranjan]

On 2020-07-28 13:59, Stephen Boyd wrote:
> Quoting Sai Prakash Ranjan (2020-07-28 00:51:02)
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c 
> > b/drivers/hwtracing/coresight/coresight-etm4x.c
> > index 6d7d2169bfb2..adb71987a1e3 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> > @@ -48,8 +48,6 @@ module_param(pm_save_enable, int, 0444);
> >  MODULE_PARM_DESC(pm_save_enable,
> >         "Save/restore state on power down: 1 = never, 2 = 
> > self-hosted");
> >
> > -/* The number of ETMv4 currently registered */
> > -static int etm4_count;
> >  static struct etmv4_drvdata *etmdrvdata[NR_CPUS];
> >  static void etm4_set_default_config(struct etmv4_config *config);
> >  static int etm4_set_event_filters(struct etmv4_drvdata *drvdata,
> > @@ -1403,12 +1401,9 @@ static int etm4_pm_setup_cpuslocked(void)
>
> Is this only called from __init now? If so please mark it as __init
> then.

Yes, will change it.

> >  {
> >         int ret;
> >
> > -       if (etm4_count++)
> > -               return 0;
> > -
> >         ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
> >         if (ret)
> > -               goto reduce_count;
> > +               return ret;
> >
> >         ret = 
> > cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
> >                                                    
> > "arm/coresight4:starting",
> > @@ -1432,17 +1427,11 @@ static int etm4_pm_setup_cpuslocked(void)
> >
> >  unregister_notifier:
> >         cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
> > -
> > -reduce_count:
> > -       --etm4_count;
> >         return ret;
> >  }
> >
> >  static void etm4_pm_clear(void)
>
> This is __init too?

Will change.

> >  {
> > -       if (--etm4_count != 0)
> > -               return;
> > -
> >         cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
> >         cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
> >         if (hp_online) {
> > @@ -1598,4 +1576,29 @@ static struct amba_driver etm4x_driver = {
> >         .probe          = etm4_probe,
> >         .id_table       = etm4_ids,
> >  };
> > -builtin_amba_driver(etm4x_driver);
> > +
> > +static int __init etm4x_init(void)
> > +{
> > +       int ret;
> > +
> > +       cpus_read_lock();
> > +       ret = etm4_pm_setup_cpuslocked();
> > +       cpus_read_unlock();
> > +
> > +       /* etm4_pm_setup_cpuslocked() does its own cleanup - exit on 
> > error */
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = amba_driver_register(&etm4x_driver);
> > +       if (ret) {
> > +               pr_info("Error registering etm4x driver\n");
>
> Use pr_err() please.

Yes indeed, will change.

> > +               goto err_init;
> > +       }
> > +
> > +       return ret;
> > +
> > +err_init:
>
> Why is this a goto?
>
> > +       etm4_pm_clear();
> > +       return ret;
>
> Instead of just putting this in the if (ret) arm?

Will change.

> > +}
> > +module_init(etm4x_init);
>
> It was device_initcall before with builtin_amba_driver(), best to not
> change that.

Sure.

I will wait to see if there are any more comments on this patch and then 
post a v3.
Thanks for the review Stephen.

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation