x86: cleanup debugfs usage

PATCH In review Greg Kroah-Hartman 4 participants 14 messages Jan 22, 2019 – Jan 28, 2019

Subsystems (7)

x86 KVM KVM x86 x86 MM +3 more subsystems

lore

Share & tools

lore

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #0 lore raw

cover x86: cleanup debugfs usage

When calling debugfs code, there is no need to ever check the return value of the call, as no logic should ever change if a call works properly or not. Fix up a bunch of x86-specific code to not care about the results of debugfs.

Greg Kroah-Hartman (6):
x86: mce: no need to check return value of debugfs_create functions
x86: kdebugfs: no need to check return value of debugfs_create
functions
x86: kvm: no need to check return value of debugfs_create functions
x86: mm: no need to check return value of debugfs_create functions
x86: platform: no need to check return value of debugfs_create
functions
x86: xen: no need to check return value of debugfs_create functions

 arch/x86/kernel/cpu/mce/core.c            | 15 ++----
 arch/x86/kernel/cpu/mce/inject.c          | 34 ++-----------
 arch/x86/kernel/cpu/mce/severity.c        | 14 ++----
 arch/x86/kernel/kdebugfs.c                | 60 +++++------------------
 arch/x86/kvm/debugfs.c                    | 26 +++-------
 arch/x86/mm/debug_pagetables.c            | 35 +++----------
 arch/x86/platform/atom/punit_atom_debug.c | 22 ++-------
 arch/x86/platform/intel-quark/imr.c       | 14 ++----
 arch/x86/platform/intel/iosf_mbi.c        | 21 ++------
 arch/x86/platform/uv/tlb_uv.c             | 15 +-----
 arch/x86/xen/debugfs.c                    |  7 +--
 arch/x86/xen/p2m.c                        |  3 --
 12 files changed, 52 insertions(+), 214 deletions(-)

Signature · 1 line

2.20.1

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #1 lore raw

PATCH 1/6 x86: mce: no need to check return value of debugfs_create functions

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Cc Tony Luck <[email protected]>

Cc Vishal Verma <[email protected]>

Cc Pu Wen <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/kernel/cpu/mce/core.c     | 15 ++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 12 insertions(+), 51 deletions(-)

arch/x86/kernel/cpu/mce/core.c +4 ~~-11~~

index 672c7225cb1b..1f13f9032a8a 100644
@@ -2457,22 +2457,15 @@ static int fake_panic_set(void *data, u64 val)
 DEFINE_SIMPLE_ATTRIBUTE(fake_panic_fops, fake_panic_get,
 			fake_panic_set, "%llu\n");
 
-static int __init mcheck_debugfs_init(void)
+static void __init mcheck_debugfs_init(void)
 {
-	struct dentry *dmce, *ffake_panic;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		return -ENOMEM;
-	ffake_panic = debugfs_create_file("fake_panic", 0444, dmce, NULL,
-					  &fake_panic_fops);
-	if (!ffake_panic)
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_file("fake_panic", 0444, dmce, NULL, &fake_panic_fops);
 }
 #else
-static int __init mcheck_debugfs_init(void) { return -EINVAL; }
+static void __init mcheck_debugfs_init(void) { }
 #endif
 
 DEFINE_STATIC_KEY_FALSE(mcsafe_key);

arch/x86/kernel/cpu/mce/inject.c +5 ~~-29~~

index 8492ef7d9015..0f5b6c57ffde 100644
@@ -648,7 +648,6 @@ static const struct file_operations readme_fops = {
 
 static struct dfs_node {
 	char *name;
-	struct dentry *d;
 	const struct file_operations *fops;
 	umode_t perm;
 } dfs_fls[] = {

@@ -662,7 +661,7 @@ static struct dfs_node {
 	{ .name = "README",	.fops = &readme_fops, .perm = S_IRUSR | S_IRGRP | S_IROTH },
 };
 
-static int __init debugfs_init(void)
+static void __init debugfs_init(void)
 {
 	unsigned int i;
 	u64 cap;

@@ -671,30 +670,11 @@ static int __init debugfs_init(void)
 	n_banks = cap & MCG_BANKCNT_MASK;
 
 	dfs_inj = debugfs_create_dir("mce-inject", NULL);
-	if (!dfs_inj)
-		return -EINVAL;
-
-	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++) {
-		dfs_fls[i].d = debugfs_create_file(dfs_fls[i].name,
-						    dfs_fls[i].perm,
-						    dfs_inj,
-						    &i_mce,
-						    dfs_fls[i].fops);
-
-		if (!dfs_fls[i].d)
-			goto err_dfs_add;
-	}
-
-	return 0;
 
-err_dfs_add:
-	while (i-- > 0)
-		debugfs_remove(dfs_fls[i].d);
+	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
+		debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
+				    &i_mce, dfs_fls[i].fops);
 
-	debugfs_remove(dfs_inj);
-	dfs_inj = NULL;
-
-	return -ENODEV;
 }
 
 static int __init inject_init(void)

@@ -704,11 +684,7 @@ static int __init inject_init(void)
 	if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL))
 		return -ENOMEM;
 
-	err = debugfs_init();
-	if (err) {
-		free_cpumask_var(mce_inject_cpumask);
-		return err;
-	}
+	debugfs_init();
 
 	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
 	mce_register_injector_chain(&inject_nb);

arch/x86/kernel/cpu/mce/severity.c +3 ~~-11~~

index dc3e26e905a3..a2d297981b8a 100644
@@ -399,21 +399,13 @@ static const struct file_operations severities_coverage_fops = {
 
 static int __init severities_debugfs_init(void)
 {
-	struct dentry *dmce, *fsev;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		goto err_out;
-
-	fsev = debugfs_create_file("severities-coverage", 0444, dmce, NULL,
-				   &severities_coverage_fops);
-	if (!fsev)
-		goto err_out;
 
+	debugfs_create_file("severities-coverage", 0444, dmce, NULL,
+			    &severities_coverage_fops);
 	return 0;
-
-err_out:
-	return -ENOMEM;
 }
 late_initcall(severities_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */

Signature · 1 line

2.20.1

Borislav Petkov

maintainer

2019-01-22 21:53 UTC #2 lore raw

Re: [PATCH 1/6] x86: mce: no need to check return value of debugfs_create functions

Quoted 33 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 arch/x86/kernel/cpu/mce/core.c     | 15 ++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 12 insertions(+), 51 deletions(-)
arch/x86/kernel/cpu/mce/core.c +2 -2
index 672c7225cb1b..1f13f9032a8a 100644
@@ -2457,22 +2457,15 @@ static int fake_panic_set(void *data, u64 val)
 DEFINE_SIMPLE_ATTRIBUTE(fake_panic_fops, fake_panic_get,
 			fake_panic_set, "%llu\n");
 
-static int __init mcheck_debugfs_init(void)
+static void __init mcheck_debugfs_init(void)
 {
-	struct dentry *dmce, *ffake_panic;
+	struct dentry *dmce;
 

arch/x86/kernel/cpu/mce/core.c static int fake_panic_set(void *data, u64 val) · line 2465

Quoted 10 lines

arch/x86/kernel/cpu/mce/core.c +1 -8

 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		return -ENOMEM;
-	ffake_panic = debugfs_create_file("fake_panic", 0444, dmce, NULL,
-					  &fake_panic_fops);
-	if (!ffake_panic)
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_file("fake_panic", 0444, dmce, NULL, &fake_panic_fops);

Right, for that I don't care if it fails or not...

Quoted 43 lines

arch/x86/kernel/cpu/mce/core.c +1 -1

 #else
-static int __init mcheck_debugfs_init(void) { return -EINVAL; }
+static void __init mcheck_debugfs_init(void) { }
 #endif
 
 DEFINE_STATIC_KEY_FALSE(mcsafe_key);

arch/x86/kernel/cpu/mce/inject.c +1 ~~-14~~

index 8492ef7d9015..0f5b6c57ffde 100644
@@ -648,7 +648,6 @@ static const struct file_operations readme_fops = {
 
 static struct dfs_node {
 	char *name;
-	struct dentry *d;
 	const struct file_operations *fops;
 	umode_t perm;
 } dfs_fls[] = {

@@ -662,7 +661,7 @@ static struct dfs_node {
 	{ .name = "README",	.fops = &readme_fops, .perm = S_IRUSR | S_IRGRP | S_IROTH },
 };
 
-static int __init debugfs_init(void)
+static void __init debugfs_init(void)
 {
 	unsigned int i;
 	u64 cap;

@@ -671,30 +670,11 @@ static int __init debugfs_init(void)
 	n_banks = cap & MCG_BANKCNT_MASK;
 
 	dfs_inj = debugfs_create_dir("mce-inject", NULL);
-	if (!dfs_inj)
-		return -EINVAL;
-
-	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++) {
-		dfs_fls[i].d = debugfs_create_file(dfs_fls[i].name,
-						    dfs_fls[i].perm,
-						    dfs_inj,
-						    &i_mce,
-						    dfs_fls[i].fops);
-
-		if (!dfs_fls[i].d)
-			goto err_dfs_add;

arch/x86/kernel/cpu/mce/inject.c static int __init debugfs_init(void) · line 676

Quoted 10 lines

arch/x86/kernel/cpu/mce/inject.c +3 -6

-	}
-
-	return 0;
 
-err_dfs_add:
-	while (i-- > 0)
-		debugfs_remove(dfs_fls[i].d);
+	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
+		debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
+				    &i_mce, dfs_fls[i].fops);

... but that matters: if any of those debugfs files cannot be created for whatever reason, that injector module is useless because I can't inject MCEs properly.

I mean, I like the simplification but ...

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #3 lore raw

PATCH 2/6 x86: kdebugfs: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/kernel/kdebugfs.c | 60 +++++++-------------------------------
 1 file changed, 11 insertions(+), 49 deletions(-)

arch/x86/kernel/kdebugfs.c +11 ~~-49~~

index fd6f8fbbe6f2..0758e7ca8977 100644
@@ -68,33 +68,18 @@ static const struct file_operations fops_setup_data = {
 	.llseek		= default_llseek,
 };
 
-static int __init
+static void __init
 create_setup_data_node(struct dentry *parent, int no,
 		       struct setup_data_node *node)
 {
-	struct dentry *d, *type, *data;
+	struct dentry *d;
 	char buf[16];
 
 	sprintf(buf, "%d", no);
 	d = debugfs_create_dir(buf, parent);
-	if (!d)
-		return -ENOMEM;
-
-	type = debugfs_create_x32("type", S_IRUGO, d, &node->type);
-	if (!type)
-		goto err_dir;
-
-	data = debugfs_create_file("data", S_IRUGO, d, node, &fops_setup_data);
-	if (!data)
-		goto err_type;
 
-	return 0;
-
-err_type:
-	debugfs_remove(type);
-err_dir:
-	debugfs_remove(d);
-	return -ENOMEM;
+	debugfs_create_x32("type", S_IRUGO, d, &node->type);
+	debugfs_create_file("data", S_IRUGO, d, node, &fops_setup_data);
 }
 
 static int __init create_setup_data_nodes(struct dentry *parent)

@@ -107,8 +92,6 @@ static int __init create_setup_data_nodes(struct dentry *parent)
 	int no = 0;
 
 	d = debugfs_create_dir("setup_data", parent);
-	if (!d)
-		return -ENOMEM;
 
 	pa_data = boot_params.hdr.setup_data;

@@ -129,19 +112,17 @@ static int __init create_setup_data_nodes(struct dentry *parent)
 		node->paddr = pa_data;
 		node->type = data->type;
 		node->len = data->len;
-		error = create_setup_data_node(d, no, node);
+		create_setup_data_node(d, no, node);
 		pa_data = data->next;
 
 		memunmap(data);
-		if (error)
-			goto err_dir;
 		no++;
 	}
 
 	return 0;
 
 err_dir:
-	debugfs_remove(d);
+	debugfs_remove_recursive(d);
 	return error;
 }

@@ -152,35 +133,18 @@ static struct debugfs_blob_wrapper boot_params_blob = {
 
 static int __init boot_params_kdebugfs_init(void)
 {
-	struct dentry *dbp, *version, *data;
-	int error = -ENOMEM;
+	struct dentry *dbp;
+	int error;
 
 	dbp = debugfs_create_dir("boot_params", arch_debugfs_dir);
-	if (!dbp)
-		return -ENOMEM;
-
-	version = debugfs_create_x16("version", S_IRUGO, dbp,
-				     &boot_params.hdr.version);
-	if (!version)
-		goto err_dir;
 
-	data = debugfs_create_blob("data", S_IRUGO, dbp,
-				   &boot_params_blob);
-	if (!data)
-		goto err_version;
+	debugfs_create_x16("version", S_IRUGO, dbp, &boot_params.hdr.version);
+	debugfs_create_blob("data", S_IRUGO, dbp, &boot_params_blob);
 
 	error = create_setup_data_nodes(dbp);
 	if (error)
-		goto err_data;
+		debugfs_remove_recursive(dbp);
 
-	return 0;
-
-err_data:
-	debugfs_remove(data);
-err_version:
-	debugfs_remove(version);
-err_dir:
-	debugfs_remove(dbp);
 	return error;
 }
 #endif /* CONFIG_DEBUG_BOOT_PARAMS */

@@ -190,8 +154,6 @@ static int __init arch_kdebugfs_init(void)
 	int error = 0;
 
 	arch_debugfs_dir = debugfs_create_dir("x86", NULL);
-	if (!arch_debugfs_dir)
-		return -ENOMEM;
 
 #ifdef CONFIG_DEBUG_BOOT_PARAMS
 	error = boot_params_kdebugfs_init();

Signature · 1 line

2.20.1

Greg Kroah-Hartman

author

2019-01-22 14:35 UTC #4 lore raw

PATCH 3/6 x86: kvm: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/kvm/debugfs.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

arch/x86/kvm/debugfs.c +8 ~~-18~~

index c19c7ede9bd6..827cd58400d2 100644
@@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
 
 int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-	struct dentry *ret;
-
-	ret = debugfs_create_file("tsc-offset", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_offset_fops);
-	if (!ret)
-		return -ENOMEM;
+	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
+			    &vcpu_tsc_offset_fops);
 
 	if (kvm_has_tsc_control) {
-		ret = debugfs_create_file("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,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_scaling_frac_fops);
-		if (!ret)
-			return -ENOMEM;
-
+		debugfs_create_file("tsc-scaling-ratio", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_fops);
+		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_frac_fops);
 	}
 
 	return 0;

Signature · 1 line

2.20.1

Paolo Bonzini

maintainer

2019-01-25 17:49 UTC #5 lore raw

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

Quoted 47 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.
Cc Paolo Bonzini <[email protected]>
Cc "Radim Krčmář" <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 arch/x86/kvm/debugfs.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)
arch/x86/kvm/debugfs.c +2 ~~-17~~
index c19c7ede9bd6..827cd58400d2 100644
@@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
 
 int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-	struct dentry *ret;
-
-	ret = debugfs_create_file("tsc-offset", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_offset_fops);
-	if (!ret)
-		return -ENOMEM;
+	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
+			    &vcpu_tsc_offset_fops);
 
 	if (kvm_has_tsc_control) {
-		ret = debugfs_create_file("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,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_scaling_frac_fops);
-		if (!ret)
-			return -ENOMEM;

arch/x86/kvm/debugfs.c DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi · line 58

Quoted 10 lines

arch/x86/kvm/debugfs.c +6 -1

-
+		debugfs_create_file("tsc-scaling-ratio", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_fops);
+		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_frac_fops);
 	}
 
 	return 0;

I'm still not sure about this. I think it's better if debugfs files are created "all or none", i.e. something like

virt/kvm/kvm_main.c +1 -3

index 5ecea812cb6a..ce70c30b2861 100644
@@ -2573,9 +2573,7 @@ static int kvm_vm_ioctl_create_vcpu(
 	if (r)
 		goto vcpu_destroy;
 
-	r = kvm_create_vcpu_debugfs(vcpu);
-	if (r)
-		goto vcpu_destroy;
+	kvm_create_vcpu_debugfs(vcpu);
 
 	mutex_lock(&kvm->lock);
 	if (kvm_get_vcpu_by_id(kvm, id)) {

Paolo

Greg Kroah-Hartman

author

2019-01-26 13:53 UTC #6 lore raw

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

Quoted 67 lines · Paolo Bonzini

Quoted 47 lines · Greg Kroah-Hartman
When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.
Cc Paolo Bonzini <[email protected]>
Cc "Radim Krčmář" <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 arch/x86/kvm/debugfs.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)
arch/x86/kvm/debugfs.c +2 ~~-17~~
index c19c7ede9bd6..827cd58400d2 100644
@@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
 
 int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-	struct dentry *ret;
-
-	ret = debugfs_create_file("tsc-offset", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_offset_fops);
-	if (!ret)
-		return -ENOMEM;
+	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
+			    &vcpu_tsc_offset_fops);
 
 	if (kvm_has_tsc_control) {
-		ret = debugfs_create_file("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,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_scaling_frac_fops);
-		if (!ret)
-			return -ENOMEM;
arch/x86/kvm/debugfs.c DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi · line 58
Quoted 10 lines
arch/x86/kvm/debugfs.c +6 -1
-
+		debugfs_create_file("tsc-scaling-ratio", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_fops);
+		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_frac_fops);
 	}
 
 	return 0;
I'm still not sure about this. I think it's better if debugfs files are created "all or none", i.e. something like
virt/kvm/kvm_main.c +0 -0
index 5ecea812cb6a..ce70c30b2861 100644

virt/kvm/kvm_main.c static int kvm_vm_ioctl_create_vcpu( · line 2576

Quoted 7 lines

virt/kvm/kvm_main.c +1 -3

@@ -2573,9 +2573,7 @@ static int kvm_vm_ioctl_create_vcpu(
 	if (r)
 		goto vcpu_destroy;
 
-	r = kvm_create_vcpu_debugfs(vcpu);
-	if (r)
-		goto vcpu_destroy;
+	kvm_create_vcpu_debugfs(vcpu);

Ah, yes, sorry, I missed that. I'll respin the patch with this change in it in a few days.

thanks for the review.

greg k-h

Paolo Bonzini

maintainer

2019-01-28 09:19 UTC #7 lore raw

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

Quoted 2 lines · Greg Kroah-Hartman

Ah, yes, sorry, I missed that. I'll respin the patch with this change in it in a few days.

Just drop this patch. I'll post the kvm_vm_ioctl_create_vcpu patch as a replacement for this one and your other debugfs_create patch for virt/kvm.

Thanks,

Paolo

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #8 lore raw

PATCH 4/6 x86: mm: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Dave Hansen <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/mm/debug_pagetables.c | 35 +++++++---------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

arch/x86/mm/debug_pagetables.c +7 ~~-28~~

index cd84f067e41d..1f23bea5da87 100644
@@ -25,8 +25,6 @@ static int ptdump_curknl_show(struct seq_file *m, void *v)
 DEFINE_SHOW_ATTRIBUTE(ptdump_curknl);
 
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
-static struct dentry *pe_curusr;
-
 static int ptdump_curusr_show(struct seq_file *m, void *v)
 {
 	if (current->mm->pgd) {

@@ -41,8 +39,6 @@ DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
 #endif
 
 #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
-static struct dentry *pe_efi;
-
 static int ptdump_efi_show(struct seq_file *m, void *v)
 {
 	if (efi_mm.pgd)

@@ -53,41 +49,24 @@ static int ptdump_efi_show(struct seq_file *m, void *v)
 DEFINE_SHOW_ATTRIBUTE(ptdump_efi);
 #endif
 
-static struct dentry *dir, *pe_knl, *pe_curknl;
+static struct dentry *dir;
 
 static int __init pt_dump_debug_init(void)
 {
 	dir = debugfs_create_dir("page_tables", NULL);
-	if (!dir)
-		return -ENOMEM;
-
-	pe_knl = debugfs_create_file("kernel", 0400, dir, NULL,
-				     &ptdump_fops);
-	if (!pe_knl)
-		goto err;
 
-	pe_curknl = debugfs_create_file("current_kernel", 0400,
-					dir, NULL, &ptdump_curknl_fops);
-	if (!pe_curknl)
-		goto err;
+	debugfs_create_file("kernel", 0400, dir, NULL, &ptdump_fops);
+	debugfs_create_file("current_kernel", 0400, dir, NULL,
+			    &ptdump_curknl_fops);
 
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
-	pe_curusr = debugfs_create_file("current_user", 0400,
-					dir, NULL, &ptdump_curusr_fops);
-	if (!pe_curusr)
-		goto err;
+	debugfs_create_file("current_user", 0400, dir, NULL,
+			    &ptdump_curusr_fops);
 #endif
-
 #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
-	pe_efi = debugfs_create_file("efi", 0400, dir, NULL, &ptdump_efi_fops);
-	if (!pe_efi)
-		goto err;
+	debugfs_create_file("efi", 0400, dir, NULL, &ptdump_efi_fops);
 #endif
-
 	return 0;
-err:
-	debugfs_remove_recursive(dir);
-	return -ENOMEM;
 }
 
 static void __exit pt_dump_debug_exit(void)

Signature · 1 line

2.20.1

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #9 lore raw

PATCH 5/6 x86: platform: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/platform/atom/punit_atom_debug.c | 22 ++++------------------
 arch/x86/platform/intel-quark/imr.c       | 14 ++++----------
 arch/x86/platform/intel/iosf_mbi.c        | 21 +++------------------
 arch/x86/platform/uv/tlb_uv.c             | 15 ++-------------
 4 files changed, 13 insertions(+), 59 deletions(-)

arch/x86/platform/atom/punit_atom_debug.c +4 ~~-18~~

index 6cb6076223ba..4ac47e9f2089 100644
@@ -113,24 +113,12 @@ DEFINE_SHOW_ATTRIBUTE(punit_dev_state);
 
 static struct dentry *punit_dbg_file;
 
-static int punit_dbgfs_register(struct punit_device *punit_device)
+static void punit_dbgfs_register(struct punit_device *punit_device)
 {
-	struct dentry *dev_state;
-
 	punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
-	if (!punit_dbg_file)
-		return -ENXIO;
-
-	dev_state = debugfs_create_file("dev_power_state", 0444,
-					punit_dbg_file, punit_device,
-					&punit_dev_state_fops);
-	if (!dev_state) {
-		pr_err("punit_dev_state register failed\n");
-		debugfs_remove(punit_dbg_file);
-		return -ENXIO;
-	}
 
-	return 0;
+	debugfs_create_file("dev_power_state", 0444, punit_dbg_file,
+			    punit_device, &punit_dev_state_fops);
 }
 
 static void punit_dbgfs_unregister(void)

@@ -160,9 +148,7 @@ static int __init punit_atom_debug_init(void)
 	if (!id)
 		return -ENODEV;
 
-	ret = punit_dbgfs_register((struct punit_device *)id->driver_data);
-	if (ret < 0)
-		return ret;
+	punit_dbgfs_register((struct punit_device *)id->driver_data);
 
 	return 0;
 }

arch/x86/platform/intel-quark/imr.c +4 ~~-10~~

index 49828c2707ac..98d7982e2594 100644
@@ -34,7 +34,6 @@
 #include <linux/types.h>
 
 struct imr_device {
-	struct dentry	*file;
 	bool		init;
 	struct mutex	lock;
 	int		max_imr;

@@ -230,13 +229,11 @@ DEFINE_SHOW_ATTRIBUTE(imr_dbgfs_state);
  * imr_debugfs_register - register debugfs hooks.
  *
  * @idev:	pointer to imr_device structure.
- * @return:	0 on success - errno on failure.
  */
-static int imr_debugfs_register(struct imr_device *idev)
+static void imr_debugfs_register(struct imr_device *idev)
 {
-	idev->file = debugfs_create_file("imr_state", 0444, NULL, idev,
-					 &imr_dbgfs_state_fops);
-	return PTR_ERR_OR_ZERO(idev->file);
+	debugfs_create_file("imr_state", 0444, NULL, idev,
+			    &imr_dbgfs_state_fops);
 }
 
 /**

@@ -581,7 +578,6 @@ static const struct x86_cpu_id imr_ids[] __initconst = {
 static int __init imr_init(void)
 {
 	struct imr_device *idev = &imr_dev;
-	int ret;
 
 	if (!x86_match_cpu(imr_ids) || !iosf_mbi_available())
 		return -ENODEV;

@@ -591,9 +587,7 @@ static int __init imr_init(void)
 	idev->init = true;
 
 	mutex_init(&idev->lock);
-	ret = imr_debugfs_register(idev);
-	if (ret != 0)
-		pr_warn("debugfs register failed!\n");
+	imr_debugfs_register(idev);
 	imr_fixup_memmap(idev);
 	return 0;
 }

arch/x86/platform/intel/iosf_mbi.c +3 ~~-18~~

index a9f2e888e135..b18d4dec4658 100644
@@ -470,31 +470,16 @@ static struct dentry *iosf_dbg;
 
 static void iosf_sideband_debug_init(void)
 {
-	struct dentry *d;
-
 	iosf_dbg = debugfs_create_dir("iosf_sb", NULL);
-	if (IS_ERR_OR_NULL(iosf_dbg))
-		return;
 
 	/* mdr */
-	d = debugfs_create_x32("mdr", 0660, iosf_dbg, &dbg_mdr);
-	if (!d)
-		goto cleanup;
+	debugfs_create_x32("mdr", 0660, iosf_dbg, &dbg_mdr);
 
 	/* mcrx */
-	d = debugfs_create_x32("mcrx", 0660, iosf_dbg, &dbg_mcrx);
-	if (!d)
-		goto cleanup;
+	debugfs_create_x32("mcrx", 0660, iosf_dbg, &dbg_mcrx);
 
 	/* mcr - initiates mailbox tranaction */
-	d = debugfs_create_file("mcr", 0660, iosf_dbg, &dbg_mcr, &iosf_mcr_fops);
-	if (!d)
-		goto cleanup;
-
-	return;
-
-cleanup:
-	debugfs_remove_recursive(d);
+	debugfs_create_file("mcr", 0660, iosf_dbg, &dbg_mcr, &iosf_mcr_fops);
 }
 
 static void iosf_debugfs_init(void)

arch/x86/platform/uv/tlb_uv.c +2 ~~-13~~

index a4130b84d1ff..33f79198d4c4 100644
@@ -68,7 +68,6 @@ static struct tunables tunables[] = {
 };
 
 static struct dentry *tunables_dir;
-static struct dentry *tunables_file;
 
 /* these correspond to the statistics printed by ptc_seq_show() */
 static char *stat_description[] = {

@@ -1702,18 +1701,8 @@ static int __init uv_ptc_init(void)
 	}
 
 	tunables_dir = debugfs_create_dir(UV_BAU_TUNABLES_DIR, NULL);
-	if (!tunables_dir) {
-		pr_err("unable to create debugfs directory %s\n",
-		       UV_BAU_TUNABLES_DIR);
-		return -EINVAL;
-	}
-	tunables_file = debugfs_create_file(UV_BAU_TUNABLES_FILE, 0600,
-					tunables_dir, NULL, &tunables_fops);
-	if (!tunables_file) {
-		pr_err("unable to create debugfs file %s\n",
-		       UV_BAU_TUNABLES_FILE);
-		return -EINVAL;
-	}
+	debugfs_create_file(UV_BAU_TUNABLES_FILE, 0600, tunables_dir, NULL,
+			    &tunables_fops);
 	return 0;
 }

Signature · 1 line

2.20.1

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #10 lore raw

PATCH 6/6 x86: xen: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/xen/debugfs.c | 7 +------
 arch/x86/xen/p2m.c     | 3 ---
 2 files changed, 1 insertion(+), 9 deletions(-)

arch/x86/xen/debugfs.c +1 -6

index 13da87918b4f..532410998684 100644
@@ -9,13 +9,8 @@ static struct dentry *d_xen_debug;
 
 struct dentry * __init xen_init_debugfs(void)
 {
-	if (!d_xen_debug) {
+	if (!d_xen_debug)
 		d_xen_debug = debugfs_create_dir("xen", NULL);
-
-		if (!d_xen_debug)
-			pr_warning("Could not create 'xen' debugfs directory\n");
-	}
-
 	return d_xen_debug;
 }

arch/x86/xen/p2m.c +0 -3

index 055e37e43541..9d3e29805b73 100644
@@ -810,9 +810,6 @@ static int __init xen_p2m_debugfs(void)
 {
 	struct dentry *d_xen = xen_init_debugfs();
 
-	if (d_xen == NULL)
-		return -ENOMEM;
-
 	d_mmu_debug = debugfs_create_dir("mmu", d_xen);
 
 	debugfs_create_file("p2m", 0600, d_mmu_debug, NULL, &p2m_dump_fops);

Signature · 7 lines

2.20.1


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Greg Kroah-Hartman

author

2019-01-22 14:36 UTC #11 lore raw

PATCH 6/6 x86: xen: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/xen/debugfs.c | 7 +------
 arch/x86/xen/p2m.c     | 3 ---
 2 files changed, 1 insertion(+), 9 deletions(-)

arch/x86/xen/debugfs.c +1 -6

index 13da87918b4f..532410998684 100644
@@ -9,13 +9,8 @@ static struct dentry *d_xen_debug;
 
 struct dentry * __init xen_init_debugfs(void)
 {
-	if (!d_xen_debug) {
+	if (!d_xen_debug)
 		d_xen_debug = debugfs_create_dir("xen", NULL);
-
-		if (!d_xen_debug)
-			pr_warning("Could not create 'xen' debugfs directory\n");
-	}
-
 	return d_xen_debug;
 }

arch/x86/xen/p2m.c +0 -3

index 055e37e43541..9d3e29805b73 100644
@@ -810,9 +810,6 @@ static int __init xen_p2m_debugfs(void)
 {
 	struct dentry *d_xen = xen_init_debugfs();
 
-	if (d_xen == NULL)
-		return -ENOMEM;
-
 	d_mmu_debug = debugfs_create_dir("mmu", d_xen);
 
 	debugfs_create_file("p2m", 0600, d_mmu_debug, NULL, &p2m_dump_fops);

Signature · 1 line

2.20.1

Juergen Gross

maintainer Reviewed-by on v1 6/6

2019-01-23 08:53 UTC #12 lore raw

Re: [PATCH 6/6] x86: xen: no need to check return value of debugfs_create functions

Quoted 4 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Quoted 10 lines

Cc Boris Ostrovsky <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

Reviewed-by Juergen Gross <[email protected]>

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Juergen Gross

maintainer Reviewed-by on v1 6/6

2019-01-23 08:53 UTC #13 lore raw

Re: [PATCH 6/6] x86: xen: no need to check return value of debugfs_create functions

In reply to Greg Kroah-Hartman

Quoted 4 lines · Greg Kroah-Hartman

When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this.

Quoted 10 lines

Cc Boris Ostrovsky <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

Reviewed-by Juergen Gross <[email protected]>

Juergen