[PATCH 0/2] mtd: ubi: Fix some initialisation failure cases

[PATCH 0/2] mtd: ubi: Fix some initialisation failure cases

[Ben Hutchings]

ubi currently fails to initialise if debugfs is enabled at build time
but disabled at boot time (CONFIG_DEBUG_FS is enabled and debugfs=off
is set on the kernel command line).  Errors from debugfs should always
be ignored but this wasn't done everywhere.

While fixing this I also noticed that a recent change to ubi removed
some cleanup from the ubi_init() failure path that I think is still
needed.

This series should fix both problems.

Ben.

Ben Hutchings (2):
  mtd: ubi: Restore missing cleanup on ubi_init() failure path
  mtd: ubi: Ignore all debugfs initialisation failures

 drivers/mtd/ubi/build.c | 15 ++++++---------
 drivers/mtd/ubi/debug.c | 33 +++++++--------------------------
 drivers/mtd/ubi/debug.h |  4 ++--
 3 files changed, 15 insertions(+), 37 deletions(-)

-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: ubi: Restore missing cleanup on ubi_init() failure path

[Ben Hutchings]

We need to clean-up debugfs and ubiblock if we fail after initialising
them.

Signed-off-by: Ben Hutchings <ben.hutchings@mind.be>
Fixes: 927c145208b0 ("mtd: ubi: attach from device tree")
---
 drivers/mtd/ubi/build.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index a7e3a6246c0e..50d78975b3ed 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1372,7 +1372,7 @@ static int __init ubi_init(void)
 
 		/* See comment above re-ubi_is_module(). */
 		if (ubi_is_module())
-			goto out_slab;
+			goto out_debugfs;
 	}
 
 	register_mtd_user(&ubi_mtd_notifier);
@@ -1387,6 +1387,9 @@ static int __init ubi_init(void)
 
 out_mtd_notifier:
 	unregister_mtd_user(&ubi_mtd_notifier);
+	ubiblock_exit();
+out_debugfs:
+	ubi_debugfs_exit();
 out_slab:
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_dev_unreg:
-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: ubi: Ignore all debugfs initialisation failures

[Ben Hutchings]

Drivers should never stop initialising or probing because of a failure
to create debugfs entries.  Such failures are harmless and could be
the intended result of setting the kernel parameter debugfs=off.

This was partially fixed in 2019, but some error checks were missed
and another one was been added since.

Signed-off-by: Ben Hutchings <ben.hutchings@mind.be>
Fixes: 2a734bb8d502 ("UBI: use debugfs for the extra checks knobs")
Fixes: c2d73ba892ea ("mtd: no need to check return value of ...")
Fixes: 6931fb44858c ("ubi: Use the fault injection framework to enhance ...")
---
 drivers/mtd/ubi/build.c | 10 ++--------
 drivers/mtd/ubi/debug.c | 33 +++++++--------------------------
 drivers/mtd/ubi/debug.h |  4 ++--
 3 files changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 50d78975b3ed..c07a4754b521 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1018,9 +1018,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	if (err)
 		goto out_detach;
 
-	err = ubi_debugfs_init_dev(ubi);
-	if (err)
-		goto out_uif;
+	ubi_debugfs_init_dev(ubi);
 
 	ubi->bgt_thread = kthread_create(ubi_thread, ubi, "%s", ubi->bgt_name);
 	if (IS_ERR(ubi->bgt_thread)) {
@@ -1064,7 +1062,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 
 out_debugfs:
 	ubi_debugfs_exit_dev(ubi);
-out_uif:
 	uif_close(ubi);
 out_detach:
 	ubi_wl_close(ubi);
@@ -1362,9 +1359,7 @@ static int __init ubi_init(void)
 		goto out_dev_unreg;
 	}
 
-	err = ubi_debugfs_init();
-	if (err)
-		goto out_slab;
+	ubi_debugfs_init();
 
 	err = ubiblock_init();
 	if (err) {
@@ -1390,7 +1385,6 @@ static int __init ubi_init(void)
 	ubiblock_exit();
 out_debugfs:
 	ubi_debugfs_exit();
-out_slab:
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_dev_unreg:
 	misc_deregister(&ubi_ctrl_cdev);
diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index d57f52bd2ff3..505bc07d7dbb 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -248,13 +248,6 @@ static void dfs_create_fault_entry(struct dentry *parent)
 	struct dentry *dir;
 
 	dir = debugfs_create_dir("fault_inject", parent);
-	if (IS_ERR_OR_NULL(dir)) {
-		int err = dir ? PTR_ERR(dir) : -ENODEV;
-
-		pr_warn("UBI error: cannot create \"fault_inject\" debugfs directory, error %d\n",
-			 err);
-		return;
-	}
 
 	fault_create_debugfs_attr("emulate_eccerr", dir,
 				  &fault_eccerr_attr);
@@ -291,28 +284,18 @@ static void dfs_create_fault_entry(struct dentry *parent)
 /**
  * ubi_debugfs_init - create UBI debugfs directory.
  *
- * Create UBI debugfs directory. Returns zero in case of success and a negative
- * error code in case of failure.
+ * Try to create UBI debugfs directory.
  */
-int ubi_debugfs_init(void)
+void ubi_debugfs_init(void)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
-		return 0;
+		return;
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
-	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
-
-		pr_err("UBI error: cannot create \"ubi\" debugfs directory, error %d\n",
-		       err);
-		return err;
-	}
 
 #ifdef CONFIG_MTD_UBI_FAULT_INJECTION
 	dfs_create_fault_entry(dfs_rootdir);
 #endif
-
-	return 0;
 }
 
 /**
@@ -585,10 +568,9 @@ static const struct file_operations eraseblk_count_fops = {
  * ubi_debugfs_init_dev - initialize debugfs for an UBI device.
  * @ubi: UBI device description object
  *
- * This function creates all debugfs files for UBI device @ubi. Returns zero in
- * case of success and a negative error code in case of failure.
+ * This function tries to create all debugfs files for UBI device @ubi.
  */
-int ubi_debugfs_init_dev(struct ubi_device *ubi)
+void ubi_debugfs_init_dev(struct ubi_device *ubi)
 {
 	unsigned long ubi_num = ubi->ubi_num;
 	struct ubi_debug_info *d = &ubi->dbg;
@@ -596,13 +578,13 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 	int n;
 
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
-		return 0;
+		return;
 
 	n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN + 1, UBI_DFS_DIR_NAME,
 		     ubi->ubi_num);
 	if (n > UBI_DFS_DIR_LEN) {
 		/* The array size is too small */
-		return -EINVAL;
+		return;
 	}
 
 	d->dfs_dir = debugfs_create_dir(d->dfs_dir_name, dfs_rootdir);
@@ -653,7 +635,6 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 						       (void *)ubi_num,
 						       &dfs_fops);
 #endif
-	return 0;
 }
 
 /**
diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
index b2fd97548808..1dc430d3f482 100644
--- a/drivers/mtd/ubi/debug.h
+++ b/drivers/mtd/ubi/debug.h
@@ -47,9 +47,9 @@ void ubi_dump_aeb(const struct ubi_ainf_peb *aeb, int type);
 void ubi_dump_mkvol_req(const struct ubi_mkvol_req *req);
 int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset,
 			  int len);
-int ubi_debugfs_init(void);
+void ubi_debugfs_init(void);
 void ubi_debugfs_exit(void);
-int ubi_debugfs_init_dev(struct ubi_device *ubi);
+void ubi_debugfs_init_dev(struct ubi_device *ubi);
 void ubi_debugfs_exit_dev(struct ubi_device *ubi);
 
 /**
-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: ubi: Ignore all debugfs initialisation failures

[Zhihao Cheng]

在 2024/4/11 6:42, Ben Hutchings 写道:
> Drivers should never stop initialising or probing because of a failure
> to create debugfs entries.  Such failures are harmless and could be
> the intended result of setting the kernel parameter debugfs=off.
> 
> This was partially fixed in 2019, but some error checks were missed
> and another one was been added since.
> 
> Signed-off-by: Ben Hutchings <ben.hutchings@mind.be>
> Fixes: 2a734bb8d502 ("UBI: use debugfs for the extra checks knobs")
> Fixes: c2d73ba892ea ("mtd: no need to check return value of ...")
> Fixes: 6931fb44858c ("ubi: Use the fault injection framework to enhance ...")
> ---
>   drivers/mtd/ubi/build.c | 10 ++--------
>   drivers/mtd/ubi/debug.c | 33 +++++++--------------------------
>   drivers/mtd/ubi/debug.h |  4 ++--
>   3 files changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 50d78975b3ed..c07a4754b521 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1018,9 +1018,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>   	if (err)
>   		goto out_detach;
>   
> -	err = ubi_debugfs_init_dev(ubi);
> -	if (err)
> -		goto out_uif;
> +	ubi_debugfs_init_dev(ubi);
>   
>   	ubi->bgt_thread = kthread_create(ubi_thread, ubi, "%s", ubi->bgt_name);
>   	if (IS_ERR(ubi->bgt_thread)) {
> @@ -1064,7 +1062,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>   
>   out_debugfs:
>   	ubi_debugfs_exit_dev(ubi);
> -out_uif:
>   	uif_close(ubi);
>   out_detach:
>   	ubi_wl_close(ubi);
> @@ -1362,9 +1359,7 @@ static int __init ubi_init(void)
>   		goto out_dev_unreg;
>   	}
>   
> -	err = ubi_debugfs_init();
> -	if (err)
> -		goto out_slab;
> +	ubi_debugfs_init();
>   
>   	err = ubiblock_init();
>   	if (err) {
> @@ -1390,7 +1385,6 @@ static int __init ubi_init(void)
>   	ubiblock_exit();
>   out_debugfs:
>   	ubi_debugfs_exit();
> -out_slab:
>   	kmem_cache_destroy(ubi_wl_entry_slab);
>   out_dev_unreg:
>   	misc_deregister(&ubi_ctrl_cdev);
> diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
> index d57f52bd2ff3..505bc07d7dbb 100644
> --- a/drivers/mtd/ubi/debug.c
> +++ b/drivers/mtd/ubi/debug.c
> @@ -248,13 +248,6 @@ static void dfs_create_fault_entry(struct dentry *parent)
>   	struct dentry *dir;
>   
>   	dir = debugfs_create_dir("fault_inject", parent);
> -	if (IS_ERR_OR_NULL(dir)) {
> -		int err = dir ? PTR_ERR(dir) : -ENODEV;
> -
> -		pr_warn("UBI error: cannot create \"fault_inject\" debugfs directory, error %d\n",
> -			 err);
> -		return;
> -	}

I think we can keep this error path, sub dirs won't be created by 
following functions if dir is assigned as error code. so ubi could 
return in advance and prints a warning message to notify user that fault 
injection debugfs initialization is failed.
>   
>   	fault_create_debugfs_attr("emulate_eccerr", dir,
>   				  &fault_eccerr_attr);
> @@ -291,28 +284,18 @@ static void dfs_create_fault_entry(struct dentry *parent)
>   /**
>    * ubi_debugfs_init - create UBI debugfs directory.
>    *
> - * Create UBI debugfs directory. Returns zero in case of success and a negative
> - * error code in case of failure.
> + * Try to create UBI debugfs directory.
>    */
> -int ubi_debugfs_init(void)
> +void ubi_debugfs_init(void)
>   {
>   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
> -		return 0;
> +		return;
>   
>   	dfs_rootdir = debugfs_create_dir("ubi", NULL);
> -	if (IS_ERR_OR_NULL(dfs_rootdir)) {
> -		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
> -
> -		pr_err("UBI error: cannot create \"ubi\" debugfs directory, error %d\n",
> -		       err);
> -		return err;
> -	}

Similar to previous, also replace 'pr_err' with 'pr_warn'.
>   
>   #ifdef CONFIG_MTD_UBI_FAULT_INJECTION
>   	dfs_create_fault_entry(dfs_rootdir);
>   #endif
> -
> -	return 0;
>   }

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/2] mtd: ubi: Fix some initialisation failure cases

[Zhihao Cheng]

在 2024/4/11 6:42, Ben Hutchings 写道:
> ubi currently fails to initialise if debugfs is enabled at build time
> but disabled at boot time (CONFIG_DEBUG_FS is enabled and debugfs=off
> is set on the kernel command line).  Errors from debugfs should always
> be ignored but this wasn't done everywhere.
> 
> While fixing this I also noticed that a recent change to ubi removed
> some cleanup from the ubi_init() failure path that I think is still
> needed.
> 
> This series should fix both problems.
> 
> Ben.

Hi, Ben. I have a same patch with your first one, I have merge our 
patchset into a new series [1]
[1] 
https://lore.kernel.org/linux-mtd/20240411031903.3050278-1-chengzhihao1@huawei.com/
> 
> Ben Hutchings (2):
>    mtd: ubi: Restore missing cleanup on ubi_init() failure path
>    mtd: ubi: Ignore all debugfs initialisation failures
> 
>   drivers/mtd/ubi/build.c | 15 ++++++---------
>   drivers/mtd/ubi/debug.c | 33 +++++++--------------------------
>   drivers/mtd/ubi/debug.h |  4 ++--
>   3 files changed, 15 insertions(+), 37 deletions(-)
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/2] mtd: ubi: Fix some initialisation failure cases

[Ben Hutchings]

On Thu, Apr 11, 2024 at 11:30:41AM +0800, Zhihao Cheng wrote:
> 在 2024/4/11 6:42, Ben Hutchings 写道:
> > ubi currently fails to initialise if debugfs is enabled at build time
> > but disabled at boot time (CONFIG_DEBUG_FS is enabled and debugfs=off
> > is set on the kernel command line).  Errors from debugfs should always
> > be ignored but this wasn't done everywhere.
> > 
> > While fixing this I also noticed that a recent change to ubi removed
> > some cleanup from the ubi_init() failure path that I think is still
> > needed.
> > 
> > This series should fix both problems.
> > 
> > Ben.
> 
> Hi, Ben. I have a same patch with your first one, I have merge our patchset
> into a new series [1]
> [1] https://lore.kernel.org/linux-mtd/20240411031903.3050278-1-chengzhihao1@huawei.com/
[...]

Thank you!

Ben.

-- 
Ben Hutchings · Senior Embedded Software Engineer, Essensium-Mind · mind.be

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/