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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250513103340.GB26114@e132581.arm.com>
Date: Tue, 13 May 2025 11:33:40 +0100
From: Leo Yan <leo.yan@....com>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: suzuki.poulose@....com, mike.leach@...aro.org, james.clark@...aro.org,
	alexander.shishkin@...ux.intel.com, coresight@...ts.linaro.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] coresight: prevent deactivate active config while
 enabling the config

Hi Levi,

On Fri, May 02, 2025 at 11:34:17AM +0100, Yeoreum Yun wrote:

[...]

> > > > > --- 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,
Leo