Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config

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

Hi James,

> > Hi James.
> > > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > index a70c1454b410..dfa7dcbaf25d 100644
> > > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> > > >    			cscfg_mgr->sysfs_active_config = cfg_hash;
> > > >    	} else {
> > > >    		/* disable if matching current value */
> > > > -		if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> > > > +		if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> > > > +		    !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> > > >    			_cscfg_deactivate_config(cfg_hash);
> > >
> > > So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
> > > be a per-config refcount?
> > >
> > > Say you have two active configs, sys_enable_cnt is now 2, how do you disable
> > > one without it always skipping here when the other config is enabled?
> >
> > Sorry to miss this one!.
> > Because when one configuration is enabled,
> > cscfg_mgr->sysfs_active_config becomes !NULL, so it wouldn't happen
> > there is no two activate configurations. so sys_enable_cnt wouldn't be
> > 2.
> >
> >
> >
>
> Maybe "sys_enabled" is a better name then. Count implies that it can be more
> than one. And the doc could be updated to say it's only ever 0 or 1.

I think I'm not fully explained. sys_enable_cnt could be more than 2
if it runs with perf and sysfs both.
Because, perf uses cscfg_active_config() to activate configuration.
but, "sysfs" only can activate 1 configuration because it enables with
cscfg_config_sysfs_activate(). so the sys_enable_cnt could be more than
However, sys_enable_cnt can be increased only 1 by sysfs interface.

Maybe It could be hinder to disable by enabled activated by the sysfs.
However, That's wouldn't be critical.

>
> But what about my other point about enabled always being a subset of active?
> Can we not change "sys_active_cnt" to a more generic "refcount", then both
> activation and enabling steps increment that same refcount, because they are
> both technically users of the config. Then you can solve the problem without
> adding another separate counter. I think that's potentially easier to
> understand.

Actually, I think merging this two count (or with module ref too),
seems increasing commplexity right now.
To make clear It would be good However to fix bug for above case,
I think it's enough to add sys_enable_cnt and I think it doesn't loss
its name meaning and it seems more to be backportable.

> Although the easiest is just locking every function with the mutex (or a
> spinlock if it also needs to be used from Perf). Obviously all these atomics
> are harder to get right than that, and this isn't performance sensitive in
> any way.

Agree, That's why there's one option to merge cscfg_mutex with
coresight_mutex. But I think this is too much to fix this problem.

Thanks.