Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Leo,

> [...]
>
> > > > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> > > > > >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> > > > > >
> > > > > >  	raw_spin_unlock(&drvdata->spinlock);
> > > > > > +
> > > > > > +	cscfg_csdev_disable_active_config(csdev);
> > > > > > +
> > > > >
> > > > > In general, we need to split changes into several patches if each
> > > > > addresses a different issue.  From my understanding, the change above is
> > > > > to fix missing to disable config when disable Sysfs mode.
> > > > >
> > > > > If so, could we use a seperate patch for this change?
> > > > >
> > > >
> > > > It's not a differnt issue. Without this line, the active count wouldn't
> > > > decrese and it raise another issue -- unloadable moudle for active_cnt :(
> > > > So I think it should be included in this patch.
> > >
> > > I read the code again and concluded the change above is not related to
> > > locking and would be a separate issue: when we close a Sysfs session,
> > > we need to disable a config on a CoreSight device.
> > > Could you clarify what is meaning "unloadable moudle for active_cnt"?
> > > I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading,
> > > but have no clue why "active_cnt" impacts module unloading.
> >
> > Yes. but it also related "by this patch".
> > When the load config and "activate" configuration via sysfs,
> > not only the cscfg_mgr->sys_active_cnt is increase but also
> > "individual cscfg->active_cnt".
> > This patch extends the meaning of "cscfg->active_cnt" includes
> > "enable of configuraiton". so that cscfg_config_desc_put() prevent
> > decrease "module reference" while holding individual active_cnt.
> > That's why without this change, the "module reference" couldn't be
> > decreased. The problem before this change is deactivation doesn't
> > chekc cscfg->active_cnt but put module reference whenever it calls.
>
> Thanks for explanation and it makes sense to me.
>
> As we discussed, given this patch is relative big, let us divide into
> three small patches for easier review.
>
> - The first patch is to address that the sysfs interface misses to
>   call cscfg_csdev_disable_active_config() for disabling config.
>
> - The second patch fixes the locking issue for "config_csdev_list".
>   As the "config_csdev_list" is protected by cscfg_csdev_lock, the
>   cscfg_remove_owned_csdev_configs() function should use lock for
>   exclusive operations.
>
> - The third patch is to fix reference counter of a config module.
>   The patch adds cscfg_config_desc_get() and cscfg_config_desc_put()
>   in the config enabling / disabling flow for acquiring module
>   reference counter.
>

Thanks for your review.
I'll separate the patchset according to your suggestion.

--
Sincerely,
Yeoreum Yun