linux-kernel - Re: [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function

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: <6352eb65.050a0220.3eb5e.14c7@mx.google.com>
Date:   Fri, 21 Oct 2022 20:56:30 +0200
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Amit Kucheria <amitk@...nel.org>,
        Thara Gopinath <thara.gopinath@...il.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>, linux-arm-msm@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Thara Gopinath <thara.gopinath@...aro.org>
Subject: Re: [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init
 function

On Fri, Oct 21, 2022 at 08:33:41PM +0200, Daniel Lezcano wrote:
> On 21/10/2022 20:19, Christian Marangi wrote:
> > Simplify debugfs init function.
> > - Add check for existing dev directory.
> > - Fix wrong version in dbg_version_show (with version 0.0.0, 0.1.0 was
> >    incorrectly reported)
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > Reviewed-by: Thara Gopinath <thara.gopinath@...aro.org>
> > ---
> >   drivers/thermal/qcom/tsens.c | 14 +++++---------
> >   1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index cc2965b8d409..ff82626cecef 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -692,7 +692,7 @@ static int dbg_version_show(struct seq_file *s, void *data)
> >   			return ret;
> >   		seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver);
> >   	} else {
> > -		seq_puts(s, "0.1.0\n");
> > +		seq_printf(s, "0.%d.0\n", priv->feat->ver_major);
> >   	}
> >   	return 0;
> > @@ -704,21 +704,17 @@ DEFINE_SHOW_ATTRIBUTE(dbg_sensors);
> >   static void tsens_debug_init(struct platform_device *pdev)
> >   {
> >   	struct tsens_priv *priv = platform_get_drvdata(pdev);
> > -	struct dentry *root, *file;
> > -	root = debugfs_lookup("tsens", NULL);
> > -	if (!root)
> > +	priv->debug_root = debugfs_lookup("tsens", NULL);
> > +	if (!priv->debug_root)
> >   		priv->debug_root = debugfs_create_dir("tsens", NULL);
> > -	else
> > -		priv->debug_root = root;
> > -	file = debugfs_lookup("version", priv->debug_root);
> > -	if (!file)
> > +	if (!debugfs_lookup("version", priv->debug_root))
> >   		debugfs_create_file("version", 0444, priv->debug_root,
> >   				    pdev, &dbg_version_fops);
> >   	/* A directory for each instance of the TSENS IP */
> > -	priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root);
> > +	priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root);
> 
> Why the directory creation is replaced by the lookup ?
>

Hi,
thanks for the review! I have to be honest and do not create some fake
excuse for this. This patch is a bit old and was pending from a long
time so out of despair i just tried to RESEND this hoping someone would
pick it up. And it seems it have worked... Sooo sorry for making you
asking this.

On rechecking the change here, the entire debug_init logic seems
wrong... I don't know if it's possible but what if in the system there
are multiple version of tsens istance? Looks wrong to overwrite the
version with the last one...

I think the original idea of this was to create a directory for each
istance and put in there version and sensors debugfs.

I will propose this in the next version if it's ok for you and you agree
with this logic. Also I think I will split this in 2 different patch one
for the version fixup and one for this new logic.

Waiting for your feedback and again sorry for the noise.

> >   	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
> >   }
> >   #else
> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

-- 
	Ansuel