kvm: x86: Use DEFINE_DEBUGFS_ATTRIBUTE for debugfs files
diff mbox series

Message ID 1563780839-14739-1-git-send-email-wang.yi59@zte.com.cn
State New
Headers show
Series
  • kvm: x86: Use DEFINE_DEBUGFS_ATTRIBUTE for debugfs files
Related show

Commit Message

Yi Wang July 22, 2019, 7:33 a.m. UTC
We got these coccinelle warning:
./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
should be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
should be defined with DEFINE_DEBUGFS_ATTRIBUTE

Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
to fix this.

Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
 arch/x86/kvm/debugfs.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini Sept. 17, 2019, 5:18 p.m. UTC | #1
On 22/07/19 09:33, Yi Wang wrote:
> We got these coccinelle warning:
> ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> 
> Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> to fix this.
> 
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>

It sucks though that you have to use a function with "unsafe" in the name.

Greg, is the patch doing the right thing?

Paolo

> ---
>  arch/x86/kvm/debugfs.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
> index 329361b..24016fb 100644
> --- a/arch/x86/kvm/debugfs.c
> +++ b/arch/x86/kvm/debugfs.c
> @@ -20,7 +20,7 @@ static int vcpu_get_timer_advance_ns(void *data, u64 *val)
>  	return 0;
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
>  
>  static int vcpu_get_tsc_offset(void *data, u64 *val)
>  {
> @@ -29,7 +29,7 @@ static int vcpu_get_tsc_offset(void *data, u64 *val)
>  	return 0;
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
>  
>  static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
>  {
> @@ -38,7 +38,7 @@ static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
>  	return 0;
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
>  
>  static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
>  {
> @@ -46,20 +46,20 @@ static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
>  	return 0;
>  }
>  
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
>  
>  int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
>  {
>  	struct dentry *ret;
>  
> -	ret = debugfs_create_file("tsc-offset", 0444,
> +	ret = debugfs_create_file_unsafe("tsc-offset", 0444,
>  							vcpu->debugfs_dentry,
>  							vcpu, &vcpu_tsc_offset_fops);
>  	if (!ret)
>  		return -ENOMEM;
>  
>  	if (lapic_in_kernel(vcpu)) {
> -		ret = debugfs_create_file("lapic_timer_advance_ns", 0444,
> +		ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444,
>  								vcpu->debugfs_dentry,
>  								vcpu, &vcpu_timer_advance_ns_fops);
>  		if (!ret)
> @@ -67,12 +67,12 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
>  	}
>  
>  	if (kvm_has_tsc_control) {
> -		ret = debugfs_create_file("tsc-scaling-ratio", 0444,
> +		ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444,
>  							vcpu->debugfs_dentry,
>  							vcpu, &vcpu_tsc_scaling_fops);
>  		if (!ret)
>  			return -ENOMEM;
> -		ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> +		ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444,
>  							vcpu->debugfs_dentry,
>  							vcpu, &vcpu_tsc_scaling_frac_fops);
>  		if (!ret)
>
Greg KH Sept. 17, 2019, 6:12 p.m. UTC | #2
On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote:
> On 22/07/19 09:33, Yi Wang wrote:
> > We got these coccinelle warning:
> > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > 
> > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> > to fix this.
> > 
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> 
> It sucks though that you have to use a function with "unsafe" in the name.

I agree, why make this change?

> Greg, is the patch doing the right thing?

I can't tell.  What coccinelle script generated this patch?

thanks,

greg k-h
Paolo Bonzini Sept. 17, 2019, 7:34 p.m. UTC | #3
On 17/09/19 20:12, Greg Kroah-Hartman wrote:
> On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote:
>> On 22/07/19 09:33, Yi Wang wrote:
>>> We got these coccinelle warning:
>>> ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
>>> be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
>>> be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>
>>> Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
>>> to fix this.
>>>
>>> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
>>
>> It sucks though that you have to use a function with "unsafe" in the name.
> 
> I agree, why make this change?
> 
>> Greg, is the patch doing the right thing?
> 
> I can't tell.  What coccinelle script generated this patch?

Seems to be scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci.

//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
//# imposes some significant overhead as compared to
//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().

Paolo

> thanks,
> 
> greg k-h
>
Greg KH Sept. 18, 2019, 12:03 p.m. UTC | #4
On Wed, Sep 18, 2019 at 08:19:44AM +0800, wang.yi59@zte.com.cn wrote:
> Hi Paolo,
> 
> > On 22/07/19 09:33, Yi Wang wrote:
> > > We got these coccinelle warning:
> > > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> > > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> > > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > >
> > > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> > > to fix this.
> > >
> > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> >
> > It sucks though that you have to use a function with "unsafe" in the name.
> 
> Yes, it does, but I found some patches in the git log:
> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/log/?qt=grep&q=DEFINE_DEBUGFS_ATTRIBUTE+
> 
> And, do you think the function name "debugfs_create_file_unsafe" is not proper?

Only if you _KNOW_ you are creating/removing these files in a way that
is safe is it ok to use these calls.  Hint, what is your locking
strategy for when these files are removed?

Is that the case here?  If not, please stick with what is there today,
as we know it works, and it is "safe" to do so.

thanks,

greg k-h

Patch
diff mbox series

diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
index 329361b..24016fb 100644
--- a/arch/x86/kvm/debugfs.c
+++ b/arch/x86/kvm/debugfs.c
@@ -20,7 +20,7 @@  static int vcpu_get_timer_advance_ns(void *data, u64 *val)
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
 
 static int vcpu_get_tsc_offset(void *data, u64 *val)
 {
@@ -29,7 +29,7 @@  static int vcpu_get_tsc_offset(void *data, u64 *val)
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
 
 static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
 {
@@ -38,7 +38,7 @@  static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
 
 static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
 {
@@ -46,20 +46,20 @@  static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
 	return 0;
 }
 
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
 
 int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
 	struct dentry *ret;
 
-	ret = debugfs_create_file("tsc-offset", 0444,
+	ret = debugfs_create_file_unsafe("tsc-offset", 0444,
 							vcpu->debugfs_dentry,
 							vcpu, &vcpu_tsc_offset_fops);
 	if (!ret)
 		return -ENOMEM;
 
 	if (lapic_in_kernel(vcpu)) {
-		ret = debugfs_create_file("lapic_timer_advance_ns", 0444,
+		ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444,
 								vcpu->debugfs_dentry,
 								vcpu, &vcpu_timer_advance_ns_fops);
 		if (!ret)
@@ -67,12 +67,12 @@  int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 	}
 
 	if (kvm_has_tsc_control) {
-		ret = debugfs_create_file("tsc-scaling-ratio", 0444,
+		ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444,
 							vcpu->debugfs_dentry,
 							vcpu, &vcpu_tsc_scaling_fops);
 		if (!ret)
 			return -ENOMEM;
-		ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+		ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444,
 							vcpu->debugfs_dentry,
 							vcpu, &vcpu_tsc_scaling_frac_fops);
 		if (!ret)