linux-kernel - Re: [PATCH] soc: qcom: Introduce debugfs interface to smem

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: <20201124172655.GO95182@builder.lan>
Date:   Tue, 24 Nov 2020 11:26:55 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Andy Gross <agross@...nel.org>, elder@...aro.org,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH] soc: qcom: Introduce debugfs interface to smem

On Tue 24 Nov 11:04 CST 2020, Vinod Koul wrote:

> On 24-11-20, 10:39, Bjorn Andersson wrote:
> > On Tue 24 Nov 09:34 CST 2020, Vinod Koul wrote:
> > 
> > > On 22-11-20, 23:21, Bjorn Andersson wrote:
> > > > Every now and then it's convenient to be able to inspect the content of
> > > > SMEM items. Rather than carrying some hack locally let's upstream a
> > > > driver that when inserted exposes a debugfs interface for dumping
> > > > available items.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > > > ---
> > > >  drivers/soc/qcom/Kconfig        |   7 +++
> > > >  drivers/soc/qcom/Makefile       |   1 +
> > > >  drivers/soc/qcom/smem_debugfs.c | 102 ++++++++++++++++++++++++++++++++
> > > >  3 files changed, 110 insertions(+)
> > > >  create mode 100644 drivers/soc/qcom/smem_debugfs.c
> > > > 
> > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > > > index 3dc3e3d61ea3..7e1dd6b3f33a 100644
> > > > --- a/drivers/soc/qcom/Kconfig
> > > > +++ b/drivers/soc/qcom/Kconfig
> > > > @@ -128,6 +128,13 @@ config QCOM_SMEM
> > > >  	  The driver provides an interface to items in a heap shared among all
> > > >  	  processors in a Qualcomm platform.
> > > >  
> > > > +config QCOM_SMEM_DEBUGFS
> > > > +	tristate "Qualcomm Shared Memory Manager (SMEM) DebugFS interface"
> > > > +	depends on QCOM_SMEM
> > > > +	depends on DEBUG_FS
> > > > +	help
> > > > +	  Provides a debugfs interface for inspecting SMEM.
> > > 
> > > Do we need additional debugfs entry, maybe better to depend on DEBUG_FS
> > > being enabled and this file part of QCOM_SMEM?
> > > 
> > 
> > We don't need this in any form of production system, so rather than
> > tainting qcom_smem.c I put it in a separate driver that isn't even
> > automatically loaded.
> 
> Debugfs in production :D
> 
> I would leave it to you to decide.. lazy me needs to select another
> option!
> 

We can still leave it =m in defconfig, lazy you would have to type
"modprobe qcom_smem_debugfs" when you want this thing.

In all other cases I will waste a little bit of disk, but none of your
ram or cpu cycles.

Regards,
Bjorn

> > > > +static int smem_debugfs_rescan(struct seq_file *seq, void *p)
> > > > +{
> > > > +	struct dentry *root = seq->private;
> > > > +	unsigned long item;
> > > > +	unsigned long host;
> > > > +	unsigned long data;
> > > > +	char name[10];
> > > > +	char *ptr;
> > > > +
> > > > +	for (host = 0; host < 10; host++) {
> > > > +		for (item = 0; item < 512; item++) {
> > > > +			ptr = qcom_smem_get(host, item, NULL);
> > > > +			if (IS_ERR(ptr))
> > > > +				continue;
> > > > +
> > > > +			sprintf(name, "%ld-%ld", host, item);
> > > > +
> > > > +			data = host << 16 | item;
> > > > +			debugfs_create_file(name, 0400, root,
> > > > +					    (void *)data, &smem_debugfs_item_ops);
> > > 
> > > So IIUC user invokes scan file which creates additional files, right?
> > > Additional invoke will do that as well..?
> > > 
> > 
> > Yes, so if you run it a second time debugfs_create_file() will fail for
> > any items that was present during the last invocation.
> > 
> > I did consider adding some logic to keep track of what items we have
> > already registered, but it is just debugging code and given that after a
> > few second of operations the set of items has stabilized you typically
> > don't run this repeatedly.
> > 
> > So I don't think it's worth the memory occupied by an idr or 5000+ bits
> > in a map.
> 
> Okay sounds good to me
> 
> -- 
> ~Vinod