[PATCH 1/2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUGFS_FS

[PATCH 1/2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUGFS_FS

[Arnd Bergmann]

The slp_s0_residency_usec debugfs file currently uses
DEFINE_DEBUGFS_ATTRIBUTE(), but that macro cannot really be used to
define files outside of the debugfs code, as it has no reference to
the get/set functions if CONFIG_DEBUGFS_FS is not defined:

drivers/platform/x86/intel_pmc_core.c:80:12: error: ‘pmc_core_dev_state_get’ defined but not used [-Werror=unused-function]

This fixes the macro to always contain the reference, and instead rely
on the stubbed-out debugfs_create_file to not actually refer to
its arguments so the compiler can still drop the reference.
This works because the attribute definition is always 'static',
and the dead-code removal silently drops all static symbols
that are not used.

Fixes: c64688081490 ("debugfs: add support for self-protecting attribute file fops")
Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/debugfs.h | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4d3f0d1aec73..e94f5f8dced3 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -62,6 +62,26 @@ static inline const struct file_operations *debugfs_real_fops(struct file *filp)
 	return filp->f_path.dentry->d_fsdata;
 }
 
+ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+			size_t len, loff_t *ppos);
+ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
+			size_t len, loff_t *ppos);
+
+#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
+static int __fops ## _open(struct inode *inode, struct file *file)	\
+{									\
+	__simple_attr_check_format(__fmt, 0ull);			\
+	return simple_attr_open(inode, file, __get, __set, __fmt);	\
+}									\
+static const struct file_operations __fops = {				\
+	.owner	 = THIS_MODULE,						\
+	.open	 = __fops ## _open,					\
+	.release = simple_attr_release,					\
+	.read	 = debugfs_attr_read,					\
+	.write	 = debugfs_attr_write,					\
+	.llseek  = generic_file_llseek,					\
+}
+
 #if defined(CONFIG_DEBUG_FS)
 
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
@@ -94,26 +114,6 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 
 void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
 
-ssize_t debugfs_attr_read(struct file *file, char __user *buf,
-			size_t len, loff_t *ppos);
-ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
-			size_t len, loff_t *ppos);
-
-#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
-static int __fops ## _open(struct inode *inode, struct file *file)	\
-{									\
-	__simple_attr_check_format(__fmt, 0ull);			\
-	return simple_attr_open(inode, file, __get, __set, __fmt);	\
-}									\
-static const struct file_operations __fops = {				\
-	.owner	 = THIS_MODULE,					\
-	.open	 = __fops ## _open,					\
-	.release = simple_attr_release,				\
-	.read	 = debugfs_attr_read,					\
-	.write	 = debugfs_attr_write,					\
-	.llseek  = generic_file_llseek,				\
-}
-
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
 
@@ -233,9 +233,6 @@ static inline void debugfs_use_file_finish(int srcu_idx)
 	__releases(&debugfs_srcu)
 { }
 
-#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	\
-	static const struct file_operations __fops = { 0 }
-
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
 {
-- 
2.9.0

[PATCH 2/2] intel_pmc_core: avoid boot time warning for !CONFIG_DEBUGFS_FS

[Arnd Bergmann]

While looking at a patch that introduced a compile-time warning
"‘pmc_core_dev_state_get’ defined but not used" (I sent a patch
for debugfs to fix it), I noticed that the same patch caused
it in intel_pmc_core also introduced a bogus run-time warning:
"PMC Core: debugfs register failed".

The problem is the IS_ERR_OR_NULL() check that as usual gets
things wrong: when CONFIG_DEBUGFS_FS is disabled,
debugfs_create_dir() fails with an error code, and we don't
need to warn about it, unlike the case in which it returns
NULL.

This reverts the driver to the previous state of not warning
about CONFIG_DEBUGFS_FS being disabled. I chose not to
restore the driver to making a runtime error in debugfs
fatal in pmc_core_probe().

Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/platform/x86/intel_pmc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 520b58a04daa..e8b1b836ca2d 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -100,7 +100,7 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	struct dentry *dir, *file;
 
 	dir = debugfs_create_dir("pmc_core", NULL);
-	if (IS_ERR_OR_NULL(dir))
+	if (!dir)
 		return -ENOMEM;
 
 	pmcdev->dbgfs_dir = dir;
-- 
2.9.0

Re: [PATCH 2/2] intel_pmc_core: avoid boot time warning for !CONFIG_DEBUGFS_FS

[Greg Kroah-Hartman]

On Mon, Oct 10, 2016 at 01:12:58PM +0200, Arnd Bergmann wrote:
> While looking at a patch that introduced a compile-time warning
> "‘pmc_core_dev_state_get’ defined but not used" (I sent a patch
> for debugfs to fix it), I noticed that the same patch caused
> it in intel_pmc_core also introduced a bogus run-time warning:
> "PMC Core: debugfs register failed".
> 
> The problem is the IS_ERR_OR_NULL() check that as usual gets
> things wrong: when CONFIG_DEBUGFS_FS is disabled,
> debugfs_create_dir() fails with an error code, and we don't
> need to warn about it, unlike the case in which it returns
> NULL.
> 
> This reverts the driver to the previous state of not warning
> about CONFIG_DEBUGFS_FS being disabled. I chose not to
> restore the driver to making a runtime error in debugfs
> fatal in pmc_core_probe().
> 
> Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 520b58a04daa..e8b1b836ca2d 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -100,7 +100,7 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  	struct dentry *dir, *file;
>  
>  	dir = debugfs_create_dir("pmc_core", NULL);
> -	if (IS_ERR_OR_NULL(dir))
> +	if (!dir)
>  		return -ENOMEM;

Hah, no, you shouldn't ever care about any return value being "wrong"
from debugfs, the code should just continue on as normal.

And yes, you are correct, the IS_ERR_OR_NULL() is totally wrong.

thanks,

greg k-h

Re: [PATCH 2/2] intel_pmc_core: avoid boot time warning for !CONFIG_DEBUGFS_FS

[Darren Hart]

On Mon, Oct 10, 2016 at 02:29:17PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 10, 2016 at 01:12:58PM +0200, Arnd Bergmann wrote:
> > While looking at a patch that introduced a compile-time warning
> > "‘pmc_core_dev_state_get’ defined but not used" (I sent a patch
> > for debugfs to fix it), I noticed that the same patch caused
> > it in intel_pmc_core also introduced a bogus run-time warning:
> > "PMC Core: debugfs register failed".
> > 
> > The problem is the IS_ERR_OR_NULL() check that as usual gets
> > things wrong: when CONFIG_DEBUGFS_FS is disabled,
> > debugfs_create_dir() fails with an error code, and we don't
> > need to warn about it, unlike the case in which it returns
> > NULL.
> > 
> > This reverts the driver to the previous state of not warning
> > about CONFIG_DEBUGFS_FS being disabled. I chose not to
> > restore the driver to making a runtime error in debugfs
> > fatal in pmc_core_probe().
> > 
> > Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/platform/x86/intel_pmc_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > index 520b58a04daa..e8b1b836ca2d 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -100,7 +100,7 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> >  	struct dentry *dir, *file;
> >  
> >  	dir = debugfs_create_dir("pmc_core", NULL);
> > -	if (IS_ERR_OR_NULL(dir))
> > +	if (!dir)
> >  		return -ENOMEM;
> 
> Hah, no, you shouldn't ever care about any return value being "wrong"
> from debugfs, the code should just continue on as normal.
> 
> And yes, you are correct, the IS_ERR_OR_NULL() is totally wrong.
> 
> thanks,
> 
> greg k-h
> 

Thanks Arnd and Greg, appreciate the catch and the fix.

Applied.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUGFS_FS

[Nicolai Stange]

Hi Arnd,

thanks for this (and sorry for the late reply)!

Arnd Bergmann <arnd@arndb.de> writes:

> The slp_s0_residency_usec debugfs file currently uses
> DEFINE_DEBUGFS_ATTRIBUTE(), but that macro cannot really be used to
> define files outside of the debugfs code, as it has no reference to
> the get/set functions if CONFIG_DEBUGFS_FS is not defined:
>
> drivers/platform/x86/intel_pmc_core.c:80:12: error: ‘pmc_core_dev_state_get’ defined but not used [-Werror=unused-function]
>
> This fixes the macro to always contain the reference, and instead rely
> on the stubbed-out debugfs_create_file to not actually refer to
> its arguments so the compiler can still drop the reference.
> This works because the attribute definition is always 'static',
> and the dead-code removal silently drops all static symbols
> that are not used.
>
> Fixes: c64688081490 ("debugfs: add support for self-protecting attribute file fops")
> Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/linux/debugfs.h | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 4d3f0d1aec73..e94f5f8dced3 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -62,6 +62,26 @@ static inline const struct file_operations *debugfs_real_fops(struct file *filp)
>  	return filp->f_path.dentry->d_fsdata;
>  }
>  
> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> +			size_t len, loff_t *ppos);
> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
> +			size_t len, loff_t *ppos);
> +
> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
> +static int __fops ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +	__simple_attr_check_format(__fmt, 0ull);			\
> +	return simple_attr_open(inode, file, __get, __set, __fmt);	\
> +}									\
> +static const struct file_operations __fops = {				\
> +	.owner	 = THIS_MODULE,						\
> +	.open	 = __fops ## _open,					\
> +	.release = simple_attr_release,					\
> +	.read	 = debugfs_attr_read,					\
> +	.write	 = debugfs_attr_write,					\

This depends on GCC dead code elimination to always work for this
situation, otherwise we'd get undefined references to
debugfs_attr_read/write(), right?

In order to avoid having to test your patch against all those older
versions of GCC, can we have a safety net here and define some dummy
debugfs_attr_read/write() for the !CONFIG_DEBUGFS case?

If nothing else, it would IMHO make the !CONFIG_DEBUGFS case more
understandable because one had not to figure out that this actually
relies on dead code elimination to work.

Thanks,

Nicolai

Re: [PATCH 1/2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUGFS_FS

[Arnd Bergmann]

On Thursday, October 13, 2016 11:59:54 AM CEST Nicolai Stange wrote:
> >  
> > +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> > +                     size_t len, loff_t *ppos);
> > +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
> > +                     size_t len, loff_t *ppos);
> > +
> > +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)                \
> > +static int __fops ## _open(struct inode *inode, struct file *file)   \
> > +{                                                                    \
> > +     __simple_attr_check_format(__fmt, 0ull);                        \
> > +     return simple_attr_open(inode, file, __get, __set, __fmt);      \
> > +}                                                                    \
> > +static const struct file_operations __fops = {                               \
> > +     .owner   = THIS_MODULE,                                         \
> > +     .open    = __fops ## _open,                                     \
> > +     .release = simple_attr_release,                                 \
> > +     .read    = debugfs_attr_read,                                   \
> > +     .write   = debugfs_attr_write,                                  \
> 
> This depends on GCC dead code elimination to always work for this
> situation, otherwise we'd get undefined references to
> debugfs_attr_read/write(), right?

Correct.

> In order to avoid having to test your patch against all those older
> versions of GCC, can we have a safety net here and define some dummy
> debugfs_attr_read/write() for the !CONFIG_DEBUGFS case?

The question of dead-code elimination in older gcc versions comes up
occasionally, and I think all versions that are able to build the
kernel these days get this right all the time, otherwise any code
using IS_ENABLED() helpers to control the calling of external interfaces
would be broken.

We could probably use that macro here if you think that's better
and do:

static const struct file_operations __fops = {
    .owner   = THIS_MODULE,
    .open    = IS_ENABLED(CONFIG_DEBUGFS_FS) ? __fops ## _open : NULL,                                     
    ...

> If nothing else, it would IMHO make the !CONFIG_DEBUGFS case more
> understandable because one had not to figure out that this actually
> relies on dead code elimination to work.

Sure, that's fine. Can you do the new version of that patch with
the change then?

	Arnd

Re: [PATCH 1/2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUGFS_FS

[Nicolai Stange]

Arnd Bergmann <arnd@arndb.de> writes:

> On Thursday, October 13, 2016 11:59:54 AM CEST Nicolai Stange wrote:
>> >  
>> > +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
>> > +                     size_t len, loff_t *ppos);
>> > +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
>> > +                     size_t len, loff_t *ppos);
>> > +
>> > +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)                \
>> > +static int __fops ## _open(struct inode *inode, struct file *file)   \
>> > +{                                                                    \
>> > +     __simple_attr_check_format(__fmt, 0ull);                        \
>> > +     return simple_attr_open(inode, file, __get, __set, __fmt);      \
>> > +}                                                                    \
>> > +static const struct file_operations __fops = {                               \
>> > +     .owner   = THIS_MODULE,                                         \
>> > +     .open    = __fops ## _open,                                     \
>> > +     .release = simple_attr_release,                                 \
>> > +     .read    = debugfs_attr_read,                                   \
>> > +     .write   = debugfs_attr_write,                                  \
>> 
>> This depends on GCC dead code elimination to always work for this
>> situation, otherwise we'd get undefined references to
>> debugfs_attr_read/write(), right?
>
> Correct.
>
>> In order to avoid having to test your patch against all those older
>> versions of GCC, can we have a safety net here and define some dummy
>> debugfs_attr_read/write() for the !CONFIG_DEBUGFS case?
>
> The question of dead-code elimination in older gcc versions comes up
> occasionally, and I think all versions that are able to build the
> kernel these days get this right all the time, otherwise any code
> using IS_ENABLED() helpers to control the calling of external interfaces
> would be broken.
>
> We could probably use that macro here if you think that's better
> and do:
>
> static const struct file_operations __fops = {
>     .owner   = THIS_MODULE,
>     .open    = IS_ENABLED(CONFIG_DEBUGFS_FS) ? __fops ## _open : NULL,                                     
>     ...
>
>> If nothing else, it would IMHO make the !CONFIG_DEBUGFS case more
>> understandable because one had not to figure out that this actually
>> relies on dead code elimination to work.
>
> Sure, that's fine. Can you do the new version of that patch with
> the change then?

I'd be happy to (won't be able to do this before tomorrow though).

Thanks,

Nicolai

[PATCH v2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUG_FS

[Nicolai Stange]

From: Arnd Bergmann <arnd@arndb.de>

The slp_s0_residency_usec debugfs file currently uses
DEFINE_DEBUGFS_ATTRIBUTE(), but that macro cannot really be used to
define files outside of the debugfs code, as it has no reference to
the get/set functions if CONFIG_DEBUG_FS is not defined:

drivers/platform/x86/intel_pmc_core.c:80:12: error: ‘pmc_core_dev_state_get’ defined but not used [-Werror=unused-function]

This fixes the macro to always contain the reference, and instead rely
on the stubbed-out debugfs_create_file to not actually refer to
its arguments so the compiler can still drop the reference.
This works because the attribute definition is always 'static',
and the dead-code removal silently drops all static symbols
that are not used.

Fixes: c64688081490 ("debugfs: add support for self-protecting attribute file fops")
Fixes: df2294fb6428 ("intel_pmc_core: Convert to DEFINE_DEBUGFS_ATTRIBUTE")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[nicstange@gmail.com: Add dummy implementations of debugfs_attr_read() and
  debugfs_attr_write() in order to protect against possibly broken dead
  code elimination and to improve readability.
  Correct CONFIG_DEBUGFS_FS -> CONFIG_DEBUG_FS typo in changelog.]
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
Compile-tested on top of next-20161020 with gcc-4.9.0 and gcc-6.2.1,
CONFIG_INTEL_PMC_CORE=y and both, CONFIG_DEBUG_FS=n and CONFIG_DEBUG_FS=y.

I verified that there isn't anything in the intel_pmc_core.o that shouldn'tbe there with CONFIG_DEBUG_FS=n.

Changes to v1:
 - Add dummy implementations of debugfs_attr_read()/debugfs_attr_write()
   for the CONFIG_DEBUG_FS=n case.
 - Fix a typo in the changelog.


 include/linux/debugfs.h | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4d3f0d1..1b413a9 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -62,6 +62,21 @@ static inline const struct file_operations *debugfs_real_fops(struct file *filp)
 	return filp->f_path.dentry->d_fsdata;
 }
 
+#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
+static int __fops ## _open(struct inode *inode, struct file *file)	\
+{									\
+	__simple_attr_check_format(__fmt, 0ull);			\
+	return simple_attr_open(inode, file, __get, __set, __fmt);	\
+}									\
+static const struct file_operations __fops = {				\
+	.owner	 = THIS_MODULE,						\
+	.open	 = __fops ## _open,					\
+	.release = simple_attr_release,					\
+	.read	 = debugfs_attr_read,					\
+	.write	 = debugfs_attr_write,					\
+	.llseek  = generic_file_llseek,					\
+}
+
 #if defined(CONFIG_DEBUG_FS)
 
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
@@ -99,21 +114,6 @@ ssize_t debugfs_attr_read(struct file *file, char __user *buf,
 ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
 			size_t len, loff_t *ppos);
 
-#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
-static int __fops ## _open(struct inode *inode, struct file *file)	\
-{									\
-	__simple_attr_check_format(__fmt, 0ull);			\
-	return simple_attr_open(inode, file, __get, __set, __fmt);	\
-}									\
-static const struct file_operations __fops = {				\
-	.owner	 = THIS_MODULE,					\
-	.open	 = __fops ## _open,					\
-	.release = simple_attr_release,				\
-	.read	 = debugfs_attr_read,					\
-	.write	 = debugfs_attr_write,					\
-	.llseek  = generic_file_llseek,				\
-}
-
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
 
@@ -233,8 +233,18 @@ static inline void debugfs_use_file_finish(int srcu_idx)
 	__releases(&debugfs_srcu)
 { }
 
-#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	\
-	static const struct file_operations __fops = { 0 }
+static inline ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+					size_t len, loff_t *ppos)
+{
+	return -ENODEV;
+}
+
+static inline ssize_t debugfs_attr_write(struct file *file,
+					const char __user *buf,
+					size_t len, loff_t *ppos)
+{
+	return -ENODEV;
+}
 
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
-- 
2.10.1

Re: [PATCH v2] debugfs: improve DEFINE_DEBUGFS_ATTRIBUTE for !CONFIG_DEBUG_FS

[Andy Shevchenko]

On Thu, 2016-10-20 at 22:07 +0200, Nicolai Stange wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The slp_s0_residency_usec debugfs file currently uses
> DEFINE_DEBUGFS_ATTRIBUTE(), but that macro cannot really be used to
> define files outside of the debugfs code, as it has no reference to
> the get/set functions if CONFIG_DEBUG_FS is not defined:
> 
> drivers/platform/x86/intel_pmc_core.c:80:12: error:
> ‘pmc_core_dev_state_get’ defined but not used [-Werror=unused-
> function]
> 
> This fixes the macro to always contain the reference, and instead rely
> on the stubbed-out debugfs_create_file to not actually refer to
> its arguments so the compiler can still drop the reference.
> This works because the attribute definition is always 'static',
> and the dead-code removal silently drops all static symbols
> that are not used.

Thanks for the fix! Looks good to me.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Fixes: c64688081490 ("debugfs: add support for self-protecting
> attribute file fops")
> Fixes: df2294fb6428 ("intel_pmc_core: Convert to
> DEFINE_DEBUGFS_ATTRIBUTE")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [nicstange@gmail.com: Add dummy implementations of debugfs_attr_read()
> and
>   debugfs_attr_write() in order to protect against possibly broken
> dead
>   code elimination and to improve readability.
>   Correct CONFIG_DEBUGFS_FS -> CONFIG_DEBUG_FS typo in changelog.]
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> Compile-tested on top of next-20161020 with gcc-4.9.0 and gcc-6.2.1,
> CONFIG_INTEL_PMC_CORE=y and both, CONFIG_DEBUG_FS=n and
> CONFIG_DEBUG_FS=y.
> 
> I verified that there isn't anything in the intel_pmc_core.o that
> shouldn'tbe there with CONFIG_DEBUG_FS=n.
> 
> Changes to v1:
>  - Add dummy implementations of
> debugfs_attr_read()/debugfs_attr_write()
>    for the CONFIG_DEBUG_FS=n case.
>  - Fix a typo in the changelog.
> 
> 
>  include/linux/debugfs.h | 44 +++++++++++++++++++++++++++-------------
> ----
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 4d3f0d1..1b413a9 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -62,6 +62,21 @@ static inline const struct file_operations
> *debugfs_real_fops(struct file *filp)
>  	return filp->f_path.dentry->d_fsdata;
>  }
>  
> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		
> \
> +static int __fops ## _open(struct inode *inode, struct file *file)	
> \
> +{									
> \
> +	__simple_attr_check_format(__fmt, 0ull);			
> \
> +	return simple_attr_open(inode, file, __get, __set, __fmt);	
> \
> +}									
> \
> +static const struct file_operations __fops = {			
> 	\
> +	.owner	 = THIS_MODULE,					
> 	\
> +	.open	 = __fops ## _open,				
> 	\
> +	.release = simple_attr_release,				
> 	\
> +	.read	 = debugfs_attr_read,				
> 	\
> +	.write	 = debugfs_attr_write,				
> 	\
> +	.llseek  = generic_file_llseek,				
> 	\
> +}
> +
>  #if defined(CONFIG_DEBUG_FS)
>  
>  struct dentry *debugfs_create_file(const char *name, umode_t mode,
> @@ -99,21 +114,6 @@ ssize_t debugfs_attr_read(struct file *file, char
> __user *buf,
>  ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
>  			size_t len, loff_t *ppos);
>  
> -#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		
> \
> -static int __fops ## _open(struct inode *inode, struct file *file)	
> \
> -{									
> \
> -	__simple_attr_check_format(__fmt, 0ull);			
> \
> -	return simple_attr_open(inode, file, __get, __set, __fmt);	
> \
> -}									
> \
> -static const struct file_operations __fops = {			
> 	\
> -	.owner	 = THIS_MODULE,					
> \
> -	.open	 = __fops ## _open,				
> 	\
> -	.release = simple_attr_release,				
> \
> -	.read	 = debugfs_attr_read,				
> 	\
> -	.write	 = debugfs_attr_write,				
> 	\
> -	.llseek  = generic_file_llseek,				
> \
> -}
> -
>  struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry
> *old_dentry,
>                  struct dentry *new_dir, const char *new_name);
>  
> @@ -233,8 +233,18 @@ static inline void debugfs_use_file_finish(int
> srcu_idx)
>  	__releases(&debugfs_srcu)
>  { }
>  
> -#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	
> \
> -	static const struct file_operations __fops = { 0 }
> +static inline ssize_t debugfs_attr_read(struct file *file, char
> __user *buf,
> +					size_t len, loff_t *ppos)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline ssize_t debugfs_attr_write(struct file *file,
> +					const char __user *buf,
> +					size_t len, loff_t *ppos)
> +{
> +	return -ENODEV;
> +}
>  
>  static inline struct dentry *debugfs_rename(struct dentry *old_dir,
> struct dentry *old_dentry,
>                  struct dentry *new_dir, char *new_name)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy