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

[James Clark]

On 24/12/2024 10:13 am, Yeoreum Yun wrote:
> 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.

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.

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.