[PATCH v4 0/6] reorganize and add FADump sysfs files

[PATCH v4 0/6] reorganize and add FADump sysfs files

[Sourabh Jain]

Currently, FADump sysfs files are present inside /sys/kernel directory.
But as the number of FADump sysfs file increases it is not a good idea to
push all of them in /sys/kernel directory. It is better to have separate
directory to keep all the FADump sysfs files.

Patch series reorganizes the FADump sysfs files and avail all the existing
FADump sysfs files present inside /sys/kernel into a new directory
/sys/kernel/fadump. The backward compatibility is maintained by adding a
symlink for every sysfs file that has moved to new location. Also a new
FADump sys interface is added to get the amount of memory reserved by FADump
for saving the crash dump.

Changelog:
v1 -> v2:
 - Move fadump_release_opalcore sysfs to FADump Kobject instead of
   replicating.
 - Changed the patch order 1,2,3,4 -> 2,1,3,4 (First add the ABI doc for
   exisiting sysfs file then replicate them under FADump kobject).

v2 -> v3:
 - Remove the fadump_ prefix from replicated FADump sysfs file names.

 v3 -> v4:
 - New patch that adds a wrapper function to create symlink with
   custom symlink file name.
 - Add symlink instead of replicating the FADump sysfs files.
 - Move the OPAL core rel

Sourabh Jain (6):
  Documentation/ABI: add ABI documentation for /sys/kernel/fadump_*
  sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change
    the symlink name
  powerpc/fadump: reorganize /sys/kernel/fadump_* sysfs files
  powerpc/powernv: move core and fadump_release_opalcore under new
    kobject
  Documentation/ABI: mark /sys/kernel/fadump_* sysfs files deprecated
  powerpc/fadump: sysfs for fadump memory reservation

 .../ABI/obsolete/sysfs-kernel-fadump_enabled  |  9 +++
 .../obsolete/sysfs-kernel-fadump_registered   | 10 +++
 .../obsolete/sysfs-kernel-fadump_release_mem  | 10 +++
 .../sysfs-kernel-fadump_release_opalcore      |  9 +++
 Documentation/ABI/testing/sysfs-kernel-fadump | 40 +++++++++
 .../powerpc/firmware-assisted-dump.rst        | 28 +++++--
 arch/powerpc/kernel/fadump.c                  | 81 +++++++++++++++----
 arch/powerpc/platforms/powernv/opal-core.c    | 31 +++++--
 fs/sysfs/group.c                              | 28 ++++++-
 include/linux/sysfs.h                         | 12 +++
 10 files changed, 225 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-kernel-fadump_enabled
 create mode 100644 Documentation/ABI/obsolete/sysfs-kernel-fadump_registered
 create mode 100644 Documentation/ABI/obsolete/sysfs-kernel-fadump_release_mem
 create mode 100644 Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump

-- 
2.17.2

[PATCH v4 1/6] Documentation/ABI: add ABI documentation for /sys/kernel/fadump_*

[Sourabh Jain]

Add missing ABI documentation for existing FADump sysfs files.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 Documentation/ABI/testing/sysfs-kernel-fadump_enabled     | 7 +++++++
 Documentation/ABI/testing/sysfs-kernel-fadump_registered  | 8 ++++++++
 Documentation/ABI/testing/sysfs-kernel-fadump_release_mem | 8 ++++++++
 .../ABI/testing/sysfs-kernel-fadump_release_opalcore      | 7 +++++++
 4 files changed, 30 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_enabled
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_registered
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore

diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_enabled b/Documentation/ABI/testing/sysfs-kernel-fadump_enabled
new file mode 100644
index 000000000000..f73632b1c006
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump_enabled
@@ -0,0 +1,7 @@
+What:		/sys/kernel/fadump_enabled
+Date:		Feb 2012
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	read only
+		Primarily used to identify whether the FADump is enabled in
+		the kernel or not.
+User:		Kdump service
diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_registered b/Documentation/ABI/testing/sysfs-kernel-fadump_registered
new file mode 100644
index 000000000000..dcf925e53f0f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump_registered
@@ -0,0 +1,8 @@
+What:		/sys/kernel/fadump_registered
+Date:		Feb 2012
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	read/write
+		Helps to control the dump collect feature from userspace.
+		Setting 1 to this file enables the system to collect the
+		dump and 0 to disable it.
+User:		Kdump service
diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_mem b/Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
new file mode 100644
index 000000000000..9c20d64ab48d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
@@ -0,0 +1,8 @@
+What:		/sys/kernel/fadump_release_mem
+Date:		Feb 2012
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	write only
+		This is a special sysfs file and only available when
+		the system is booted to capture the vmcore using FADump.
+		It is used to release the memory reserved by FADump to
+		save the crash dump.
diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore b/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
new file mode 100644
index 000000000000..53313c1d4e7a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
@@ -0,0 +1,7 @@
+What:		/sys/kernel/fadump_release_opalcore
+Date:		Sep 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	write only
+		The sysfs file is available when the system is booted to
+		collect the dump on OPAL based machine. It used to release
+		the memory used to collect the opalcore.
-- 
2.17.2

[PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Sourabh Jain]

The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
kobject but doesn't provide an option to change the symlink file name.

This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
extends the __compat_only_sysfs_link_entry_to_kobj functionality which
allows function caller to customize the symlink name.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
 include/linux/sysfs.h | 12 ++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index d41c21fef138..5eb38145b957 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
 int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 				      struct kobject *target_kobj,
 				      const char *target_name)
+{
+	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
+						target_name, NULL);
+}
+EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
+
+/**
+ * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
+ * to a group or an attribute
+ * @kobj:		The kobject containing the group.
+ * @target_kobj:	The target kobject.
+ * @target_name:	The name of the target group or attribute.
+ * @symlink_name:	The name of the symlink file (target_name will be
+ *			considered if symlink_name is NULL).
+ */
+int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
+				       struct kobject *target_kobj,
+				       const char *target_name,
+				       const char *symlink_name)
 {
 	struct kernfs_node *target;
 	struct kernfs_node *entry;
@@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 		return -ENOENT;
 	}
 
-	link = kernfs_create_link(kobj->sd, target_name, entry);
+	if (!symlink_name)
+		symlink_name = target_name;
+
+	link = kernfs_create_link(kobj->sd, symlink_name, entry);
 	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
-		sysfs_warn_dup(kobj->sd, target_name);
+		sysfs_warn_dup(kobj->sd, symlink_name);
 
 	kernfs_put(entry);
 	kernfs_put(target);
 	return PTR_ERR_OR_ZERO(link);
 }
-EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
+EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 5420817ed317..123c6f10333a 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
 int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 				      struct kobject *target_kobj,
 				      const char *target_name);
+int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
+				       struct kobject *target_kobj,
+				       const char *target_name,
+				       const char *symlink_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 
@@ -508,6 +512,14 @@ static inline int __compat_only_sysfs_link_entry_to_kobj(
 	return 0;
 }
 
+static int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
+					      struct kobject *target_kobj,
+					      const char *target_name,
+					      const char *symlink_name)
+{
+	return 0;
+}
+
 static inline void sysfs_notify(struct kobject *kobj, const char *dir,
 				const char *attr)
 {
-- 
2.17.2

[PATCH v4 3/6] powerpc/fadump: reorganize /sys/kernel/fadump_* sysfs files

[Sourabh Jain]

As the number of FADump sysfs files increases it is hard to manage all of
them inside /sys/kernel directory. It's better to have all the FADump
related sysfs files in a dedicated directory /sys/kernel/fadump. But in
order to maintain backward compatibility a symlink has been added for every
sysfs that has moved to new location.

As the FADump sysfs files are now part of a dedicated directory there is no
need to prefix their name with fadump_, hence sysfs file names are also
updated. For example fadump_enabled sysfs file is now referred as enabled.

Also consolidate ABI documentation for all the FADump sysfs files in a
single file Documentation/ABI/testing/sysfs-kernel-fadump.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 Documentation/ABI/testing/sysfs-kernel-fadump | 33 ++++++++++
 arch/powerpc/kernel/fadump.c                  | 66 ++++++++++++++-----
 2 files changed, 83 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump

diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump b/Documentation/ABI/testing/sysfs-kernel-fadump
new file mode 100644
index 000000000000..5d988b919e81
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump
@@ -0,0 +1,33 @@
+What:		/sys/kernel/fadump/*
+Date:		Dec 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:
+		The /sys/kernel/fadump/* is a collection of FADump sysfs
+		file provide information about the configuration status
+		of Firmware Assisted Dump (FADump).
+
+What:		/sys/kernel/fadump/enabled
+Date:		Dec 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	read only
+		Primarily used to identify whether the FADump is enabled in
+		the kernel or not.
+User:		Kdump service
+
+What:		/sys/kernel/fadump/registered
+Date:		Dec 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	read/write
+		Helps to control the dump collect feature from userspace.
+		Setting 1 to this file enables the system to collect the
+		dump and 0 to disable it.
+User:		Kdump service
+
+What:		/sys/kernel/fadump/release_mem
+Date:		Dec 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	write only
+		This is a special sysfs file and only available when
+		the system is booted to capture the vmcore using FADump.
+		It is used to release the memory reserved by FADump to
+		save the crash dump.
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index ed59855430b9..41a3cda81791 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -1418,13 +1418,16 @@ static int fadump_region_show(struct seq_file *m, void *private)
 	return 0;
 }
 
-static struct kobj_attribute fadump_release_attr = __ATTR(fadump_release_mem,
+struct kobject *fadump_kobj;
+EXPORT_SYMBOL_GPL(fadump_kobj);
+
+static struct kobj_attribute release_attr = __ATTR(release_mem,
 						0200, NULL,
 						fadump_release_memory_store);
-static struct kobj_attribute fadump_attr = __ATTR(fadump_enabled,
+static struct kobj_attribute enable_attr = __ATTR(enabled,
 						0444, fadump_enabled_show,
 						NULL);
-static struct kobj_attribute fadump_register_attr = __ATTR(fadump_registered,
+static struct kobj_attribute register_attr = __ATTR(registered,
 						0644, fadump_register_show,
 						fadump_register_store);
 
@@ -1435,16 +1438,11 @@ static void fadump_init_files(void)
 	struct dentry *debugfs_file;
 	int rc = 0;
 
-	rc = sysfs_create_file(kernel_kobj, &fadump_attr.attr);
-	if (rc)
-		printk(KERN_ERR "fadump: unable to create sysfs file"
-			" fadump_enabled (%d)\n", rc);
-
-	rc = sysfs_create_file(kernel_kobj, &fadump_register_attr.attr);
-	if (rc)
-		printk(KERN_ERR "fadump: unable to create sysfs file"
-			" fadump_registered (%d)\n", rc);
-
+	fadump_kobj = kobject_create_and_add("fadump", kernel_kobj);
+	if (!fadump_kobj) {
+		pr_err("failed to create fadump kobject\n");
+		return;
+	}
 	debugfs_file = debugfs_create_file("fadump_region", 0444,
 					powerpc_debugfs_root, NULL,
 					&fadump_region_fops);
@@ -1452,11 +1450,47 @@ static void fadump_init_files(void)
 		printk(KERN_ERR "fadump: unable to create debugfs file"
 				" fadump_region\n");
 
+	rc = sysfs_create_file(fadump_kobj, &enable_attr.attr);
+	if (rc)
+		pr_err("unable to create enabled sysfs file (%d)\n",
+		       rc);
+	rc = sysfs_create_file(fadump_kobj, &register_attr.attr);
+	if (rc)
+		pr_err("unable to create registered sysfs file (%d)\n",
+		       rc);
+	if (fw_dump.dump_active) {
+		rc = sysfs_create_file(fadump_kobj, &release_attr.attr);
+		if (rc)
+			pr_err("unable to create release_mem sysfs file (%d)\n",
+			       rc);
+	}
+
+	/* The FADump sysfs are moved from kernel_kobj to fadump_kobj need to
+	 * create symlink at old location to maintain backward compatibility.
+	 *
+	 *      - fadump_enabled -> fadump/enabled
+	 *      - fadump_registered -> fadump/registered
+	 *      - fadump_release_mem -> fadump/release_mem
+	 */
+	rc = create_sysfs_symlink_entry_to_kobj(kernel_kobj, fadump_kobj,
+						"enabled", "fadump_enabled");
+	if (rc)
+		pr_err("unable to create fadump_enabled symlink (%d)\n", rc);
+
+	rc = create_sysfs_symlink_entry_to_kobj(kernel_kobj, fadump_kobj,
+						"registered",
+						"fadump_registered");
+	if (rc)
+		pr_err("unable to create fadump_registered symlink (%d)\n", rc);
+
 	if (fw_dump.dump_active) {
-		rc = sysfs_create_file(kernel_kobj, &fadump_release_attr.attr);
+		rc = create_sysfs_symlink_entry_to_kobj(kernel_kobj,
+							fadump_kobj,
+							"release_mem",
+							"fadump_release_mem");
 		if (rc)
-			printk(KERN_ERR "fadump: unable to create sysfs file"
-				" fadump_release_mem (%d)\n", rc);
+			pr_err("unable to create fadump_release_mem symlink (%d)\n",
+			       rc);
 	}
 	return;
 }
-- 
2.17.2

[PATCH v4 4/6] powerpc/powernv: move core and fadump_release_opalcore under new kobject

[Sourabh Jain]

The /sys/firmware/opal/core and /sys/kernel/fadump_release_opalcore sysfs
files are used to export and release the OPAL memory on PowerNV platform.
let's organize them into a new kobject under /sys/firmware/opal/mpipl/
directory.

A symlink is added to maintain the backward compatibility for
/sys/firmware/opal/core sysfs file.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 .../sysfs-kernel-fadump_release_opalcore      |  2 ++
 .../powerpc/firmware-assisted-dump.rst        | 15 +++++----
 arch/powerpc/platforms/powernv/opal-core.c    | 31 ++++++++++++++-----
 3 files changed, 34 insertions(+), 14 deletions(-)
 rename Documentation/ABI/{testing => removed}/sysfs-kernel-fadump_release_opalcore (82%)

diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
similarity index 82%
rename from Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
rename to Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
index 53313c1d4e7a..a8d46cd0f4e6 100644
--- a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
+++ b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
@@ -1,3 +1,5 @@
+This ABI is moved to /sys/firmware/opal/mpipl/release_core.
+
 What:		/sys/kernel/fadump_release_opalcore
 Date:		Sep 2019
 Contact:	linuxppc-dev@lists.ozlabs.org
diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
index 0455a78486d5..345a3405206e 100644
--- a/Documentation/powerpc/firmware-assisted-dump.rst
+++ b/Documentation/powerpc/firmware-assisted-dump.rst
@@ -112,13 +112,13 @@ to ensure that crash data is preserved to process later.
 
 -- On OPAL based machines (PowerNV), if the kernel is build with
    CONFIG_OPAL_CORE=y, OPAL memory at the time of crash is also
-   exported as /sys/firmware/opal/core file. This procfs file is
+   exported as /sys/firmware/opal/mpipl/core file. This procfs file is
    helpful in debugging OPAL crashes with GDB. The kernel memory
    used for exporting this procfs file can be released by echo'ing
-   '1' to /sys/kernel/fadump_release_opalcore node.
+   '1' to /sys/firmware/opal/mpipl/release_core node.
 
    e.g.
-     # echo 1 > /sys/kernel/fadump_release_opalcore
+     # echo 1 > /sys/firmware/opal/mpipl/release_core
 
 Implementation details:
 -----------------------
@@ -283,14 +283,17 @@ Here is the list of files under kernel sysfs:
     enhanced to use this interface to release the memory reserved for
     dump and continue without 2nd reboot.
 
- /sys/kernel/fadump_release_opalcore
+Note: /sys/kernel/fadump_release_opalcore sysfs has moved to
+      /sys/firmware/opal/mpipl/release_core
+
+ /sys/firmware/opal/mpipl/release_core
 
     This file is available only on OPAL based machines when FADump is
     active during capture kernel. This is used to release the memory
-    used by the kernel to export /sys/firmware/opal/core file. To
+    used by the kernel to export /sys/firmware/opal/mpipl/core file. To
     release this memory, echo '1' to it:
 
-    echo 1  > /sys/kernel/fadump_release_opalcore
+    echo 1  > /sys/firmware/opal/mpipl/release_core
 
 Here is the list of files under powerpc debugfs:
 (Assuming debugfs is mounted on /sys/kernel/debug directory.)
diff --git a/arch/powerpc/platforms/powernv/opal-core.c b/arch/powerpc/platforms/powernv/opal-core.c
index ed895d82c048..7fcc092d065e 100644
--- a/arch/powerpc/platforms/powernv/opal-core.c
+++ b/arch/powerpc/platforms/powernv/opal-core.c
@@ -589,7 +589,8 @@ static ssize_t fadump_release_opalcore_store(struct kobject *kobj,
 	return count;
 }
 
-static struct kobj_attribute opalcore_rel_attr = __ATTR(fadump_release_opalcore,
+struct kobject *mpipl_kobj;
+static struct kobj_attribute opalcore_rel_attr = __ATTR(release_core,
 						0200, NULL,
 						fadump_release_opalcore_store);
 
@@ -609,7 +610,7 @@ static int __init opalcore_init(void)
 	 * then capture the dump.
 	 */
 	if (!(is_opalcore_usable())) {
-		pr_err("Failed to export /sys/firmware/opal/core\n");
+		pr_err("Failed to export /sys/firmware/opal/mpipl/core\n");
 		opalcore_cleanup();
 		return rc;
 	}
@@ -617,18 +618,32 @@ static int __init opalcore_init(void)
 	/* Set OPAL core file size */
 	opal_core_attr.size = oc_conf->opalcore_size;
 
+	mpipl_kobj = kobject_create_and_add("mpipl", opal_kobj);
+	if (!mpipl_kobj) {
+		pr_err("unable to create mpipl kobject\n");
+		return -ENOMEM;
+	}
+
 	/* Export OPAL core sysfs file */
-	rc = sysfs_create_bin_file(opal_kobj, &opal_core_attr);
+	rc = sysfs_create_bin_file(mpipl_kobj, &opal_core_attr);
 	if (rc != 0) {
-		pr_err("Failed to export /sys/firmware/opal/core\n");
+		pr_err("Failed to export /sys/firmware/opal/mpipl/core\n");
 		opalcore_cleanup();
 		return rc;
 	}
-
-	rc = sysfs_create_file(kernel_kobj, &opalcore_rel_attr.attr);
+	/* The /sys/firmware/opal/core is moved to /sys/firmware/opal/mpipl/
+	 * directory, need to create symlink at old location to maintain
+	 * backward compatibility.
+	 */
+	rc = create_sysfs_symlink_entry_to_kobj(opal_kobj, mpipl_kobj,
+						"core", NULL);
+	if (rc) {
+		pr_err("unable to create core symlink (%d)\n", rc);
+		return rc;
+	}
+	rc = sysfs_create_file(mpipl_kobj, &opalcore_rel_attr.attr);
 	if (rc) {
-		pr_warn("unable to create sysfs file fadump_release_opalcore (%d)\n",
-			rc);
+		pr_warn("unable to create sysfs file release_core (%d)\n", rc);
 	}
 
 	return 0;
-- 
2.17.2

[PATCH v4 5/6] Documentation/ABI: mark /sys/kernel/fadump_* sysfs files deprecated

[Sourabh Jain]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3604 bytes --]

Add a deprecation note in FADump sysfs ABI documentation files and move
them from ABI/testing to ABI/obsolete directory.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 .../ABI/{testing => obsolete}/sysfs-kernel-fadump_enabled | 2 ++
 .../{testing => obsolete}/sysfs-kernel-fadump_registered  | 2 ++
 .../{testing => obsolete}/sysfs-kernel-fadump_release_mem | 2 ++
 Documentation/powerpc/firmware-assisted-dump.rst          | 8 ++++++++
 4 files changed, 14 insertions(+)
 rename Documentation/ABI/{testing => obsolete}/sysfs-kernel-fadump_enabled (73%)
 rename Documentation/ABI/{testing => obsolete}/sysfs-kernel-fadump_registered (77%)
 rename Documentation/ABI/{testing => obsolete}/sysfs-kernel-fadump_release_mem (78%)

diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_enabled b/Documentation/ABI/obsolete/sysfs-kernel-fadump_enabled
similarity index 73%
rename from Documentation/ABI/testing/sysfs-kernel-fadump_enabled
rename to Documentation/ABI/obsolete/sysfs-kernel-fadump_enabled
index f73632b1c006..e9c2de8b3688 100644
--- a/Documentation/ABI/testing/sysfs-kernel-fadump_enabled
+++ b/Documentation/ABI/obsolete/sysfs-kernel-fadump_enabled
@@ -1,3 +1,5 @@
+This ABI is renamed and moved to a new location /sys/kernel/fadump/enabled.
+
 What:		/sys/kernel/fadump_enabled
 Date:		Feb 2012
 Contact:	linuxppc-dev@lists.ozlabs.org
diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_registered b/Documentation/ABI/obsolete/sysfs-kernel-fadump_registered
similarity index 77%
rename from Documentation/ABI/testing/sysfs-kernel-fadump_registered
rename to Documentation/ABI/obsolete/sysfs-kernel-fadump_registered
index dcf925e53f0f..0360be39c98e 100644
--- a/Documentation/ABI/testing/sysfs-kernel-fadump_registered
+++ b/Documentation/ABI/obsolete/sysfs-kernel-fadump_registered
@@ -1,3 +1,5 @@
+This ABI is renamed and moved to a new location /sys/kernel/fadump/registered.¬
+
 What:		/sys/kernel/fadump_registered
 Date:		Feb 2012
 Contact:	linuxppc-dev@lists.ozlabs.org
diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_mem b/Documentation/ABI/obsolete/sysfs-kernel-fadump_release_mem
similarity index 78%
rename from Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
rename to Documentation/ABI/obsolete/sysfs-kernel-fadump_release_mem
index 9c20d64ab48d..6ce0b129ab12 100644
--- a/Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
+++ b/Documentation/ABI/obsolete/sysfs-kernel-fadump_release_mem
@@ -1,3 +1,5 @@
+This ABI is renamed and moved to a new location /sys/kernel/fadump/release_mem.¬
+
 What:		/sys/kernel/fadump_release_mem
 Date:		Feb 2012
 Contact:	linuxppc-dev@lists.ozlabs.org
diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
index 345a3405206e..365c10209ef3 100644
--- a/Documentation/powerpc/firmware-assisted-dump.rst
+++ b/Documentation/powerpc/firmware-assisted-dump.rst
@@ -295,6 +295,14 @@ Note: /sys/kernel/fadump_release_opalcore sysfs has moved to
 
     echo 1  > /sys/firmware/opal/mpipl/release_core
 
+Note: The following FADump sysfs files are deprecated.
+
+    Deprecated                           Alternative
+    --------------------------------------------------------------------
+    /sys/kernel/fadump_enabled           /sys/kernel/fadump/enabled
+    /sys/kernel/fadump_registered        /sys/kernel/fadump/registered
+    /sys/kernel/fadump_release_mem       /sys/kernel/fadump/release_mem
+
 Here is the list of files under powerpc debugfs:
 (Assuming debugfs is mounted on /sys/kernel/debug directory.)
 
-- 
2.17.2

[PATCH v4 6/6] powerpc/fadump: sysfs for fadump memory reservation

[Sourabh Jain]

Add a sys interface to allow querying the memory reserved by FADump for
saving the crash dump.

Also added Documentation/ABI for the new sysfs file.

Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
---
 Documentation/ABI/testing/sysfs-kernel-fadump    |  7 +++++++
 Documentation/powerpc/firmware-assisted-dump.rst |  5 +++++
 arch/powerpc/kernel/fadump.c                     | 15 +++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump b/Documentation/ABI/testing/sysfs-kernel-fadump
index 5d988b919e81..8f7a64a81783 100644
--- a/Documentation/ABI/testing/sysfs-kernel-fadump
+++ b/Documentation/ABI/testing/sysfs-kernel-fadump
@@ -31,3 +31,10 @@ Description:	write only
 		the system is booted to capture the vmcore using FADump.
 		It is used to release the memory reserved by FADump to
 		save the crash dump.
+
+What:		/sys/kernel/fadump/mem_reserved
+Date:		Dec 2019
+Contact:	linuxppc-dev@lists.ozlabs.org
+Description:	read only
+		Provide information about the amount of memory reserved by
+		FADump to save the crash dump in bytes.
diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
index 365c10209ef3..04993eaf3113 100644
--- a/Documentation/powerpc/firmware-assisted-dump.rst
+++ b/Documentation/powerpc/firmware-assisted-dump.rst
@@ -268,6 +268,11 @@ Here is the list of files under kernel sysfs:
     be handled and vmcore will not be captured. This interface can be
     easily integrated with kdump service start/stop.
 
+ /sys/kernel/fadump/mem_reserved
+
+   This is used to display the memory reserved by FADump for saving the
+   crash dump.
+
  /sys/kernel/fadump_release_mem
     This file is available only when FADump is active during
     second kernel. This is used to release the reserved memory
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 41a3cda81791..b2af51b7c750 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -1357,6 +1357,13 @@ static ssize_t fadump_enabled_show(struct kobject *kobj,
 	return sprintf(buf, "%d\n", fw_dump.fadump_enabled);
 }
 
+static ssize_t fadump_mem_reserved_show(struct kobject *kobj,
+					struct kobj_attribute *attr,
+					char *buf)
+{
+	return sprintf(buf, "%ld\n", fw_dump.reserve_dump_area_size);
+}
+
 static ssize_t fadump_register_show(struct kobject *kobj,
 					struct kobj_attribute *attr,
 					char *buf)
@@ -1430,6 +1437,10 @@ static struct kobj_attribute enable_attr = __ATTR(enabled,
 static struct kobj_attribute register_attr = __ATTR(registered,
 						0644, fadump_register_show,
 						fadump_register_store);
+static struct kobj_attribute mem_reserved_attr = __ATTR(mem_reserved,
+						0444, fadump_mem_reserved_show,
+						NULL);
+
 
 DEFINE_SHOW_ATTRIBUTE(fadump_region);
 
@@ -1464,6 +1475,10 @@ static void fadump_init_files(void)
 			pr_err("unable to create release_mem sysfs file (%d)\n",
 			       rc);
 	}
+	rc = sysfs_create_file(fadump_kobj, &mem_reserved_attr.attr);
+	if (rc)
+		pr_err("unable to create mem_reserved sysfs file (%d)\n",
+		       rc);
 
 	/* The FADump sysfs are moved from kernel_kobj to fadump_kobj need to
 	 * create symlink at old location to maintain backward compatibility.
-- 
2.17.2

Re: [PATCH v4 3/6] powerpc/fadump: reorganize /sys/kernel/fadump_* sysfs files

[Greg KH]

On Fri, Dec 06, 2019 at 05:54:31PM +0530, Sourabh Jain wrote:
> +static struct kobj_attribute release_attr = __ATTR(release_mem,
>  						0200, NULL,
>  						fadump_release_memory_store);
> -static struct kobj_attribute fadump_attr = __ATTR(fadump_enabled,
> +static struct kobj_attribute enable_attr = __ATTR(enabled,
>  						0444, fadump_enabled_show,
>  						NULL);

__ATTR_RO()?

> -static struct kobj_attribute fadump_register_attr = __ATTR(fadump_registered,
> +static struct kobj_attribute register_attr = __ATTR(registered,
>  						0644, fadump_register_show,
>  						fadump_register_store);

__ATTR_RW()?

And then use an ATTRIBUTE_GROUP() macro to create a group so that you
then can do:

> @@ -1452,11 +1450,47 @@ static void fadump_init_files(void)
>  		printk(KERN_ERR "fadump: unable to create debugfs file"
>  				" fadump_region\n");
>  
> +	rc = sysfs_create_file(fadump_kobj, &enable_attr.attr);
> +	if (rc)
> +		pr_err("unable to create enabled sysfs file (%d)\n",
> +		       rc);
> +	rc = sysfs_create_file(fadump_kobj, &register_attr.attr);
> +	if (rc)
> +		pr_err("unable to create registered sysfs file (%d)\n",
> +		       rc);
> +	if (fw_dump.dump_active) {
> +		rc = sysfs_create_file(fadump_kobj, &release_attr.attr);
> +		if (rc)
> +			pr_err("unable to create release_mem sysfs file (%d)\n",
> +			       rc);
> +	}

a single call to sysfs_create_groups() here instead of trying to unwind
the mess if something went wrong.

thanks,

greg k-h

Re: [PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Greg KH]

On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote:
> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
> kobject but doesn't provide an option to change the symlink file name.
> 
> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
> extends the __compat_only_sysfs_link_entry_to_kobj functionality which
> allows function caller to customize the symlink name.
> 
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
>  include/linux/sysfs.h | 12 ++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index d41c21fef138..5eb38145b957 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>  				      struct kobject *target_kobj,
>  				      const char *target_name)
> +{
> +	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
> +						target_name, NULL);
> +}
> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> +
> +/**
> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
> + * to a group or an attribute
> + * @kobj:		The kobject containing the group.
> + * @target_kobj:	The target kobject.
> + * @target_name:	The name of the target group or attribute.
> + * @symlink_name:	The name of the symlink file (target_name will be
> + *			considered if symlink_name is NULL).
> + */
> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
> +				       struct kobject *target_kobj,
> +				       const char *target_name,
> +				       const char *symlink_name)
>  {
>  	struct kernfs_node *target;
>  	struct kernfs_node *entry;
> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>  		return -ENOENT;
>  	}
>  
> -	link = kernfs_create_link(kobj->sd, target_name, entry);
> +	if (!symlink_name)
> +		symlink_name = target_name;
> +
> +	link = kernfs_create_link(kobj->sd, symlink_name, entry);
>  	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
> -		sysfs_warn_dup(kobj->sd, target_name);
> +		sysfs_warn_dup(kobj->sd, symlink_name);
>  
>  	kernfs_put(entry);
>  	kernfs_put(target);
>  	return PTR_ERR_OR_ZERO(link);
>  }
> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 5420817ed317..123c6f10333a 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>  				      struct kobject *target_kobj,
>  				      const char *target_name);
> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
> +				       struct kobject *target_kobj,
> +				       const char *target_name,
> +				       const char *symlink_name);

sysfs_create_symlink_entry_to_kobj()?

I can't remember why we put __compat_only there, perhaps because we do
not want people to really use this unless you really really have to?

So then keep compat_only here as well?

What breaks if you remove those undocumented sysfs files?  What
userspace tool do you have that will even notice?

thanks,

greg k-h

Re: [PATCH v4 1/6] Documentation/ABI: add ABI documentation for /sys/kernel/fadump_*

[Greg KH]

On Fri, Dec 06, 2019 at 05:54:29PM +0530, Sourabh Jain wrote:
> Add missing ABI documentation for existing FADump sysfs files.
> 
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-fadump_enabled     | 7 +++++++
>  Documentation/ABI/testing/sysfs-kernel-fadump_registered  | 8 ++++++++
>  Documentation/ABI/testing/sysfs-kernel-fadump_release_mem | 8 ++++++++
>  .../ABI/testing/sysfs-kernel-fadump_release_opalcore      | 7 +++++++
>  4 files changed, 30 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_enabled
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_registered
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_release_mem
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_enabled b/Documentation/ABI/testing/sysfs-kernel-fadump_enabled
> new file mode 100644
> index 000000000000..f73632b1c006
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-fadump_enabled
> @@ -0,0 +1,7 @@
> +What:		/sys/kernel/fadump_enabled

Ugh, no wonder no one wanted to document these, that would have been
noticed right away :(

greg k-h

Re: [PATCH v4 4/6] powerpc/powernv: move core and fadump_release_opalcore under new kobject

[Greg KH]

On Fri, Dec 06, 2019 at 05:54:32PM +0530, Sourabh Jain wrote:
> The /sys/firmware/opal/core and /sys/kernel/fadump_release_opalcore sysfs
> files are used to export and release the OPAL memory on PowerNV platform.
> let's organize them into a new kobject under /sys/firmware/opal/mpipl/
> directory.
> 
> A symlink is added to maintain the backward compatibility for
> /sys/firmware/opal/core sysfs file.
> 
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  .../sysfs-kernel-fadump_release_opalcore      |  2 ++
>  .../powerpc/firmware-assisted-dump.rst        | 15 +++++----
>  arch/powerpc/platforms/powernv/opal-core.c    | 31 ++++++++++++++-----
>  3 files changed, 34 insertions(+), 14 deletions(-)
>  rename Documentation/ABI/{testing => removed}/sysfs-kernel-fadump_release_opalcore (82%)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
> similarity index 82%
> rename from Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
> rename to Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
> index 53313c1d4e7a..a8d46cd0f4e6 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
> +++ b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
> @@ -1,3 +1,5 @@
> +This ABI is moved to /sys/firmware/opal/mpipl/release_core.
> +
>  What:		/sys/kernel/fadump_release_opalcore
>  Date:		Sep 2019
>  Contact:	linuxppc-dev@lists.ozlabs.org
> diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
> index 0455a78486d5..345a3405206e 100644
> --- a/Documentation/powerpc/firmware-assisted-dump.rst
> +++ b/Documentation/powerpc/firmware-assisted-dump.rst
> @@ -112,13 +112,13 @@ to ensure that crash data is preserved to process later.
>  
>  -- On OPAL based machines (PowerNV), if the kernel is build with
>     CONFIG_OPAL_CORE=y, OPAL memory at the time of crash is also
> -   exported as /sys/firmware/opal/core file. This procfs file is
> +   exported as /sys/firmware/opal/mpipl/core file. This procfs file is
>     helpful in debugging OPAL crashes with GDB. The kernel memory
>     used for exporting this procfs file can be released by echo'ing
> -   '1' to /sys/kernel/fadump_release_opalcore node.
> +   '1' to /sys/firmware/opal/mpipl/release_core node.
>  
>     e.g.
> -     # echo 1 > /sys/kernel/fadump_release_opalcore
> +     # echo 1 > /sys/firmware/opal/mpipl/release_core
>  
>  Implementation details:
>  -----------------------
> @@ -283,14 +283,17 @@ Here is the list of files under kernel sysfs:
>      enhanced to use this interface to release the memory reserved for
>      dump and continue without 2nd reboot.
>  
> - /sys/kernel/fadump_release_opalcore
> +Note: /sys/kernel/fadump_release_opalcore sysfs has moved to
> +      /sys/firmware/opal/mpipl/release_core
> +
> + /sys/firmware/opal/mpipl/release_core
>  
>      This file is available only on OPAL based machines when FADump is
>      active during capture kernel. This is used to release the memory
> -    used by the kernel to export /sys/firmware/opal/core file. To
> +    used by the kernel to export /sys/firmware/opal/mpipl/core file. To
>      release this memory, echo '1' to it:
>  
> -    echo 1  > /sys/kernel/fadump_release_opalcore
> +    echo 1  > /sys/firmware/opal/mpipl/release_core
>  
>  Here is the list of files under powerpc debugfs:
>  (Assuming debugfs is mounted on /sys/kernel/debug directory.)
> diff --git a/arch/powerpc/platforms/powernv/opal-core.c b/arch/powerpc/platforms/powernv/opal-core.c
> index ed895d82c048..7fcc092d065e 100644
> --- a/arch/powerpc/platforms/powernv/opal-core.c
> +++ b/arch/powerpc/platforms/powernv/opal-core.c
> @@ -589,7 +589,8 @@ static ssize_t fadump_release_opalcore_store(struct kobject *kobj,
>  	return count;
>  }
>  
> -static struct kobj_attribute opalcore_rel_attr = __ATTR(fadump_release_opalcore,
> +struct kobject *mpipl_kobj;
> +static struct kobj_attribute opalcore_rel_attr = __ATTR(release_core,
>  						0200, NULL,
>  						fadump_release_opalcore_store);

__ATTR_WO()?

>  
> @@ -609,7 +610,7 @@ static int __init opalcore_init(void)
>  	 * then capture the dump.
>  	 */
>  	if (!(is_opalcore_usable())) {
> -		pr_err("Failed to export /sys/firmware/opal/core\n");
> +		pr_err("Failed to export /sys/firmware/opal/mpipl/core\n");
>  		opalcore_cleanup();
>  		return rc;
>  	}
> @@ -617,18 +618,32 @@ static int __init opalcore_init(void)
>  	/* Set OPAL core file size */
>  	opal_core_attr.size = oc_conf->opalcore_size;
>  
> +	mpipl_kobj = kobject_create_and_add("mpipl", opal_kobj);
> +	if (!mpipl_kobj) {
> +		pr_err("unable to create mpipl kobject\n");
> +		return -ENOMEM;
> +	}
> +
>  	/* Export OPAL core sysfs file */
> -	rc = sysfs_create_bin_file(opal_kobj, &opal_core_attr);
> +	rc = sysfs_create_bin_file(mpipl_kobj, &opal_core_attr);

Again, create an attribute group and add everything all at once, makes
it much simpler and your error cleanup logic will actually work :)

thanks,

greg k-h

Re: [PATCH v4 6/6] powerpc/fadump: sysfs for fadump memory reservation

[Greg KH]

On Fri, Dec 06, 2019 at 05:54:34PM +0530, Sourabh Jain wrote:
> Add a sys interface to allow querying the memory reserved by FADump for
> saving the crash dump.
> 
> Also added Documentation/ABI for the new sysfs file.
> 
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-fadump    |  7 +++++++
>  Documentation/powerpc/firmware-assisted-dump.rst |  5 +++++
>  arch/powerpc/kernel/fadump.c                     | 15 +++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump b/Documentation/ABI/testing/sysfs-kernel-fadump
> index 5d988b919e81..8f7a64a81783 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-fadump
> +++ b/Documentation/ABI/testing/sysfs-kernel-fadump
> @@ -31,3 +31,10 @@ Description:	write only
>  		the system is booted to capture the vmcore using FADump.
>  		It is used to release the memory reserved by FADump to
>  		save the crash dump.
> +
> +What:		/sys/kernel/fadump/mem_reserved
> +Date:		Dec 2019
> +Contact:	linuxppc-dev@lists.ozlabs.org
> +Description:	read only
> +		Provide information about the amount of memory reserved by
> +		FADump to save the crash dump in bytes.
> diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
> index 365c10209ef3..04993eaf3113 100644
> --- a/Documentation/powerpc/firmware-assisted-dump.rst
> +++ b/Documentation/powerpc/firmware-assisted-dump.rst
> @@ -268,6 +268,11 @@ Here is the list of files under kernel sysfs:
>      be handled and vmcore will not be captured. This interface can be
>      easily integrated with kdump service start/stop.
>  
> + /sys/kernel/fadump/mem_reserved
> +
> +   This is used to display the memory reserved by FADump for saving the
> +   crash dump.
> +
>   /sys/kernel/fadump_release_mem
>      This file is available only when FADump is active during
>      second kernel. This is used to release the reserved memory
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 41a3cda81791..b2af51b7c750 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1357,6 +1357,13 @@ static ssize_t fadump_enabled_show(struct kobject *kobj,
>  	return sprintf(buf, "%d\n", fw_dump.fadump_enabled);
>  }
>  
> +static ssize_t fadump_mem_reserved_show(struct kobject *kobj,
> +					struct kobj_attribute *attr,
> +					char *buf)
> +{
> +	return sprintf(buf, "%ld\n", fw_dump.reserve_dump_area_size);
> +}
> +
>  static ssize_t fadump_register_show(struct kobject *kobj,
>  					struct kobj_attribute *attr,
>  					char *buf)
> @@ -1430,6 +1437,10 @@ static struct kobj_attribute enable_attr = __ATTR(enabled,
>  static struct kobj_attribute register_attr = __ATTR(registered,
>  						0644, fadump_register_show,
>  						fadump_register_store);
> +static struct kobj_attribute mem_reserved_attr = __ATTR(mem_reserved,
> +						0444, fadump_mem_reserved_show,
> +						NULL);

__ATTRI_RO()?

> +
>  
>  DEFINE_SHOW_ATTRIBUTE(fadump_region);
>  
> @@ -1464,6 +1475,10 @@ static void fadump_init_files(void)
>  			pr_err("unable to create release_mem sysfs file (%d)\n",
>  			       rc);
>  	}
> +	rc = sysfs_create_file(fadump_kobj, &mem_reserved_attr.attr);
> +	if (rc)
> +		pr_err("unable to create mem_reserved sysfs file (%d)\n",
> +		       rc);

Again, put it in an attribute group, that would have only required one
line, and not this mess of not cleaning up if something went wrong.

thanks,

greg k-h

Re: [PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Sourabh Jain]

On 12/6/19 6:16 PM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote:
>> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
>> kobject but doesn't provide an option to change the symlink file name.
>>
>> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
>> extends the __compat_only_sysfs_link_entry_to_kobj functionality which
>> allows function caller to customize the symlink name.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>  fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
>>  include/linux/sysfs.h | 12 ++++++++++++
>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>> index d41c21fef138..5eb38145b957 100644
>> --- a/fs/sysfs/group.c
>> +++ b/fs/sysfs/group.c
>> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  				      struct kobject *target_kobj,
>>  				      const char *target_name)
>> +{
>> +	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
>> +						target_name, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>> +
>> +/**
>> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
>> + * to a group or an attribute
>> + * @kobj:		The kobject containing the group.
>> + * @target_kobj:	The target kobject.
>> + * @target_name:	The name of the target group or attribute.
>> + * @symlink_name:	The name of the symlink file (target_name will be
>> + *			considered if symlink_name is NULL).
>> + */
>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>> +				       struct kobject *target_kobj,
>> +				       const char *target_name,
>> +				       const char *symlink_name)
>>  {
>>  	struct kernfs_node *target;
>>  	struct kernfs_node *entry;
>> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  		return -ENOENT;
>>  	}
>>  
>> -	link = kernfs_create_link(kobj->sd, target_name, entry);
>> +	if (!symlink_name)
>> +		symlink_name = target_name;
>> +
>> +	link = kernfs_create_link(kobj->sd, symlink_name, entry);
>>  	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
>> -		sysfs_warn_dup(kobj->sd, target_name);
>> +		sysfs_warn_dup(kobj->sd, symlink_name);
>>  
>>  	kernfs_put(entry);
>>  	kernfs_put(target);
>>  	return PTR_ERR_OR_ZERO(link);
>>  }
>> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>> index 5420817ed317..123c6f10333a 100644
>> --- a/include/linux/sysfs.h
>> +++ b/include/linux/sysfs.h
>> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  				      struct kobject *target_kobj,
>>  				      const char *target_name);
>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>> +				       struct kobject *target_kobj,
>> +				       const char *target_name,
>> +				       const char *symlink_name);
> 
> sysfs_create_symlink_entry_to_kobj()?
> 
> I can't remember why we put __compat_only there, perhaps because we do
> not want people to really use this unless you really really have to?

We don't have much option here. I tried replicating the sysfs files
in older patch series but creating symlink at old location is much
better approach.

The __compat_only_sysfs_link_entry_to_kobj function is pretty generic,
unable to understand the reason behind restricting its usage.

> 
> So then keep compat_only here as well?

Sure, I will rename the wrapper function.

But how about changing the function signature instead of creating
a wrapper function?

Considering the fact that there are only two places this function
has called.

> 
> What breaks if you remove those undocumented sysfs files?  What
> userspace tool do you have that will even notice?

The scripts used in kdump service need those sysfs files to control
the dump collection. So we can't just move the sysfs files to the
new location.

Thanks,
Sourabh Jain

Re: [PATCH v4 3/6] powerpc/fadump: reorganize /sys/kernel/fadump_* sysfs files

[Sourabh Jain]

On 12/6/19 6:15 PM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 05:54:31PM +0530, Sourabh Jain wrote:
>> +static struct kobj_attribute release_attr = __ATTR(release_mem,
>>  						0200, NULL,
>>  						fadump_release_memory_store);
>> -static struct kobj_attribute fadump_attr = __ATTR(fadump_enabled,
>> +static struct kobj_attribute enable_attr = __ATTR(enabled,
>>  						0444, fadump_enabled_show,
>>  						NULL);
> 
> __ATTR_RO()?
> 
>> -static struct kobj_attribute fadump_register_attr = __ATTR(fadump_registered,
>> +static struct kobj_attribute register_attr = __ATTR(registered,
>>  						0644, fadump_register_show,
>>  						fadump_register_store);
> 
> __ATTR_RW()?

Thanks I was not aware of these macros.

> 
> And then use an ATTRIBUTE_GROUP() macro to create a group so that you
> then can do:
> 
>> @@ -1452,11 +1450,47 @@ static void fadump_init_files(void)
>>  		printk(KERN_ERR "fadump: unable to create debugfs file"
>>  				" fadump_region\n");
>>  
>> +	rc = sysfs_create_file(fadump_kobj, &enable_attr.attr);
>> +	if (rc)
>> +		pr_err("unable to create enabled sysfs file (%d)\n",
>> +		       rc);
>> +	rc = sysfs_create_file(fadump_kobj, &register_attr.attr);
>> +	if (rc)
>> +		pr_err("unable to create registered sysfs file (%d)\n",
>> +		       rc);
>> +	if (fw_dump.dump_active) {
>> +		rc = sysfs_create_file(fadump_kobj, &release_attr.attr);
>> +		if (rc)
>> +			pr_err("unable to create release_mem sysfs file (%d)\n",
>> +			       rc);
>> +	}
> 
> a single call to sysfs_create_groups() here instead of trying to unwind
> the mess if something went wrong.
> 

Sure, I will replace the individual calls with a single group call.

Thanks,
Sourabh Jain

Re: [PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Greg KH]

On Fri, Dec 06, 2019 at 11:57:53PM +0530, Sourabh Jain wrote:
> 
> 
> On 12/6/19 6:16 PM, Greg KH wrote:
> > On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote:
> >> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
> >> kobject but doesn't provide an option to change the symlink file name.
> >>
> >> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
> >> extends the __compat_only_sysfs_link_entry_to_kobj functionality which
> >> allows function caller to customize the symlink name.
> >>
> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> >> ---
> >>  fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
> >>  include/linux/sysfs.h | 12 ++++++++++++
> >>  2 files changed, 37 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> >> index d41c21fef138..5eb38145b957 100644
> >> --- a/fs/sysfs/group.c
> >> +++ b/fs/sysfs/group.c
> >> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
> >>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
> >>  				      struct kobject *target_kobj,
> >>  				      const char *target_name)
> >> +{
> >> +	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
> >> +						target_name, NULL);
> >> +}
> >> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> >> +
> >> +/**
> >> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
> >> + * to a group or an attribute
> >> + * @kobj:		The kobject containing the group.
> >> + * @target_kobj:	The target kobject.
> >> + * @target_name:	The name of the target group or attribute.
> >> + * @symlink_name:	The name of the symlink file (target_name will be
> >> + *			considered if symlink_name is NULL).
> >> + */
> >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
> >> +				       struct kobject *target_kobj,
> >> +				       const char *target_name,
> >> +				       const char *symlink_name)
> >>  {
> >>  	struct kernfs_node *target;
> >>  	struct kernfs_node *entry;
> >> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
> >>  		return -ENOENT;
> >>  	}
> >>  
> >> -	link = kernfs_create_link(kobj->sd, target_name, entry);
> >> +	if (!symlink_name)
> >> +		symlink_name = target_name;
> >> +
> >> +	link = kernfs_create_link(kobj->sd, symlink_name, entry);
> >>  	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
> >> -		sysfs_warn_dup(kobj->sd, target_name);
> >> +		sysfs_warn_dup(kobj->sd, symlink_name);
> >>  
> >>  	kernfs_put(entry);
> >>  	kernfs_put(target);
> >>  	return PTR_ERR_OR_ZERO(link);
> >>  }
> >> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> >> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
> >> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> >> index 5420817ed317..123c6f10333a 100644
> >> --- a/include/linux/sysfs.h
> >> +++ b/include/linux/sysfs.h
> >> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
> >>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
> >>  				      struct kobject *target_kobj,
> >>  				      const char *target_name);
> >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
> >> +				       struct kobject *target_kobj,
> >> +				       const char *target_name,
> >> +				       const char *symlink_name);
> > 
> > sysfs_create_symlink_entry_to_kobj()?
> > 
> > I can't remember why we put __compat_only there, perhaps because we do
> > not want people to really use this unless you really really have to?
> 
> We don't have much option here. I tried replicating the sysfs files
> in older patch series but creating symlink at old location is much
> better approach.
> 
> The __compat_only_sysfs_link_entry_to_kobj function is pretty generic,
> unable to understand the reason behind restricting its usage.
> 
> > 
> > So then keep compat_only here as well?
> 
> Sure, I will rename the wrapper function.
> 
> But how about changing the function signature instead of creating
> a wrapper function?
> 
> Considering the fact that there are only two places this function
> has called.
> 
> > 
> > What breaks if you remove those undocumented sysfs files?  What
> > userspace tool do you have that will even notice?
> 
> The scripts used in kdump service need those sysfs files to control
> the dump collection. So we can't just move the sysfs files to the
> new location.

If you can not change them, then just document them and live with it.
Why do this extra work to create a symlink for something you will never
use?

greg k-h

Re: [PATCH v4 4/6] powerpc/powernv: move core and fadump_release_opalcore under new kobject

[Sourabh Jain]

On 12/6/19 6:18 PM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 05:54:32PM +0530, Sourabh Jain wrote:
>> The /sys/firmware/opal/core and /sys/kernel/fadump_release_opalcore sysfs
>> files are used to export and release the OPAL memory on PowerNV platform.
>> let's organize them into a new kobject under /sys/firmware/opal/mpipl/
>> directory.
>>
>> A symlink is added to maintain the backward compatibility for
>> /sys/firmware/opal/core sysfs file.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>  .../sysfs-kernel-fadump_release_opalcore      |  2 ++
>>  .../powerpc/firmware-assisted-dump.rst        | 15 +++++----
>>  arch/powerpc/platforms/powernv/opal-core.c    | 31 ++++++++++++++-----
>>  3 files changed, 34 insertions(+), 14 deletions(-)
>>  rename Documentation/ABI/{testing => removed}/sysfs-kernel-fadump_release_opalcore (82%)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
>> similarity index 82%
>> rename from Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
>> rename to Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
>> index 53313c1d4e7a..a8d46cd0f4e6 100644
>> --- a/Documentation/ABI/testing/sysfs-kernel-fadump_release_opalcore
>> +++ b/Documentation/ABI/removed/sysfs-kernel-fadump_release_opalcore
>> @@ -1,3 +1,5 @@
>> +This ABI is moved to /sys/firmware/opal/mpipl/release_core.
>> +
>>  What:		/sys/kernel/fadump_release_opalcore
>>  Date:		Sep 2019
>>  Contact:	linuxppc-dev@lists.ozlabs.org
>> diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
>> index 0455a78486d5..345a3405206e 100644
>> --- a/Documentation/powerpc/firmware-assisted-dump.rst
>> +++ b/Documentation/powerpc/firmware-assisted-dump.rst
>> @@ -112,13 +112,13 @@ to ensure that crash data is preserved to process later.
>>  
>>  -- On OPAL based machines (PowerNV), if the kernel is build with
>>     CONFIG_OPAL_CORE=y, OPAL memory at the time of crash is also
>> -   exported as /sys/firmware/opal/core file. This procfs file is
>> +   exported as /sys/firmware/opal/mpipl/core file. This procfs file is
>>     helpful in debugging OPAL crashes with GDB. The kernel memory
>>     used for exporting this procfs file can be released by echo'ing
>> -   '1' to /sys/kernel/fadump_release_opalcore node.
>> +   '1' to /sys/firmware/opal/mpipl/release_core node.
>>  
>>     e.g.
>> -     # echo 1 > /sys/kernel/fadump_release_opalcore
>> +     # echo 1 > /sys/firmware/opal/mpipl/release_core
>>  
>>  Implementation details:
>>  -----------------------
>> @@ -283,14 +283,17 @@ Here is the list of files under kernel sysfs:
>>      enhanced to use this interface to release the memory reserved for
>>      dump and continue without 2nd reboot.
>>  
>> - /sys/kernel/fadump_release_opalcore
>> +Note: /sys/kernel/fadump_release_opalcore sysfs has moved to
>> +      /sys/firmware/opal/mpipl/release_core
>> +
>> + /sys/firmware/opal/mpipl/release_core
>>  
>>      This file is available only on OPAL based machines when FADump is
>>      active during capture kernel. This is used to release the memory
>> -    used by the kernel to export /sys/firmware/opal/core file. To
>> +    used by the kernel to export /sys/firmware/opal/mpipl/core file. To
>>      release this memory, echo '1' to it:
>>  
>> -    echo 1  > /sys/kernel/fadump_release_opalcore
>> +    echo 1  > /sys/firmware/opal/mpipl/release_core
>>  
>>  Here is the list of files under powerpc debugfs:
>>  (Assuming debugfs is mounted on /sys/kernel/debug directory.)
>> diff --git a/arch/powerpc/platforms/powernv/opal-core.c b/arch/powerpc/platforms/powernv/opal-core.c
>> index ed895d82c048..7fcc092d065e 100644
>> --- a/arch/powerpc/platforms/powernv/opal-core.c
>> +++ b/arch/powerpc/platforms/powernv/opal-core.c
>> @@ -589,7 +589,8 @@ static ssize_t fadump_release_opalcore_store(struct kobject *kobj,
>>  	return count;
>>  }
>>  
>> -static struct kobj_attribute opalcore_rel_attr = __ATTR(fadump_release_opalcore,
>> +struct kobject *mpipl_kobj;
>> +static struct kobj_attribute opalcore_rel_attr = __ATTR(release_core,
>>  						0200, NULL,
>>  						fadump_release_opalcore_store);
> 
> __ATTR_WO()?

Thanks :)
> 
>>  
>> @@ -609,7 +610,7 @@ static int __init opalcore_init(void)
>>  	 * then capture the dump.
>>  	 */
>>  	if (!(is_opalcore_usable())) {
>> -		pr_err("Failed to export /sys/firmware/opal/core\n");
>> +		pr_err("Failed to export /sys/firmware/opal/mpipl/core\n");
>>  		opalcore_cleanup();
>>  		return rc;
>>  	}
>> @@ -617,18 +618,32 @@ static int __init opalcore_init(void)
>>  	/* Set OPAL core file size */
>>  	opal_core_attr.size = oc_conf->opalcore_size;
>>  
>> +	mpipl_kobj = kobject_create_and_add("mpipl", opal_kobj);
>> +	if (!mpipl_kobj) {
>> +		pr_err("unable to create mpipl kobject\n");
>> +		return -ENOMEM;
>> +	}
>> +
>>  	/* Export OPAL core sysfs file */
>> -	rc = sysfs_create_bin_file(opal_kobj, &opal_core_attr);
>> +	rc = sysfs_create_bin_file(mpipl_kobj, &opal_core_attr);
> 
> Again, create an attribute group and add everything all at once, makes
> it much simpler and your error cleanup logic will actually work :)

Agree.

Thanks,
Sourabh Jain

Re: [PATCH v4 6/6] powerpc/fadump: sysfs for fadump memory reservation

[Sourabh Jain]

On 12/6/19 6:18 PM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 05:54:34PM +0530, Sourabh Jain wrote:
>> Add a sys interface to allow querying the memory reserved by FADump for
>> saving the crash dump.
>>
>> Also added Documentation/ABI for the new sysfs file.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>  Documentation/ABI/testing/sysfs-kernel-fadump    |  7 +++++++
>>  Documentation/powerpc/firmware-assisted-dump.rst |  5 +++++
>>  arch/powerpc/kernel/fadump.c                     | 15 +++++++++++++++
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-fadump b/Documentation/ABI/testing/sysfs-kernel-fadump
>> index 5d988b919e81..8f7a64a81783 100644
>> --- a/Documentation/ABI/testing/sysfs-kernel-fadump
>> +++ b/Documentation/ABI/testing/sysfs-kernel-fadump
>> @@ -31,3 +31,10 @@ Description:	write only
>>  		the system is booted to capture the vmcore using FADump.
>>  		It is used to release the memory reserved by FADump to
>>  		save the crash dump.
>> +
>> +What:		/sys/kernel/fadump/mem_reserved
>> +Date:		Dec 2019
>> +Contact:	linuxppc-dev@lists.ozlabs.org
>> +Description:	read only
>> +		Provide information about the amount of memory reserved by
>> +		FADump to save the crash dump in bytes.
>> diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
>> index 365c10209ef3..04993eaf3113 100644
>> --- a/Documentation/powerpc/firmware-assisted-dump.rst
>> +++ b/Documentation/powerpc/firmware-assisted-dump.rst
>> @@ -268,6 +268,11 @@ Here is the list of files under kernel sysfs:
>>      be handled and vmcore will not be captured. This interface can be
>>      easily integrated with kdump service start/stop.
>>  
>> + /sys/kernel/fadump/mem_reserved
>> +
>> +   This is used to display the memory reserved by FADump for saving the
>> +   crash dump.
>> +
>>   /sys/kernel/fadump_release_mem
>>      This file is available only when FADump is active during
>>      second kernel. This is used to release the reserved memory
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index 41a3cda81791..b2af51b7c750 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -1357,6 +1357,13 @@ static ssize_t fadump_enabled_show(struct kobject *kobj,
>>  	return sprintf(buf, "%d\n", fw_dump.fadump_enabled);
>>  }
>>  
>> +static ssize_t fadump_mem_reserved_show(struct kobject *kobj,
>> +					struct kobj_attribute *attr,
>> +					char *buf)
>> +{
>> +	return sprintf(buf, "%ld\n", fw_dump.reserve_dump_area_size);
>> +}
>> +
>>  static ssize_t fadump_register_show(struct kobject *kobj,
>>  					struct kobj_attribute *attr,
>>  					char *buf)
>> @@ -1430,6 +1437,10 @@ static struct kobj_attribute enable_attr = __ATTR(enabled,
>>  static struct kobj_attribute register_attr = __ATTR(registered,
>>  						0644, fadump_register_show,
>>  						fadump_register_store);
>> +static struct kobj_attribute mem_reserved_attr = __ATTR(mem_reserved,
>> +						0444, fadump_mem_reserved_show,
>> +						NULL);
> 
> __ATTRI_RO()?
> 
>> +
>>  
>>  DEFINE_SHOW_ATTRIBUTE(fadump_region);
>>  
>> @@ -1464,6 +1475,10 @@ static void fadump_init_files(void)
>>  			pr_err("unable to create release_mem sysfs file (%d)\n",
>>  			       rc);
>>  	}
>> +	rc = sysfs_create_file(fadump_kobj, &mem_reserved_attr.attr);
>> +	if (rc)
>> +		pr_err("unable to create mem_reserved sysfs file (%d)\n",
>> +		       rc);
> 
> Again, put it in an attribute group, that would have only required one
> line, and not this mess of not cleaning up if something went wrong.


Will make the changes accordingly.


Thanks,
Sourabh Jain

Re: [PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Sourabh Jain]

On 12/7/19 12:44 AM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 11:57:53PM +0530, Sourabh Jain wrote:
>>
>>
>> On 12/6/19 6:16 PM, Greg KH wrote:
>>> On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote:
>>>> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
>>>> kobject but doesn't provide an option to change the symlink file name.
>>>>
>>>> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
>>>> extends the __compat_only_sysfs_link_entry_to_kobj functionality which
>>>> allows function caller to customize the symlink name.
>>>>
>>>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>>> ---
>>>>  fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
>>>>  include/linux/sysfs.h | 12 ++++++++++++
>>>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>>>> index d41c21fef138..5eb38145b957 100644
>>>> --- a/fs/sysfs/group.c
>>>> +++ b/fs/sysfs/group.c
>>>> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
>>>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>>>  				      struct kobject *target_kobj,
>>>>  				      const char *target_name)
>>>> +{
>>>> +	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
>>>> +						target_name, NULL);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>>>> +
>>>> +/**
>>>> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
>>>> + * to a group or an attribute
>>>> + * @kobj:		The kobject containing the group.
>>>> + * @target_kobj:	The target kobject.
>>>> + * @target_name:	The name of the target group or attribute.
>>>> + * @symlink_name:	The name of the symlink file (target_name will be
>>>> + *			considered if symlink_name is NULL).
>>>> + */
>>>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>>>> +				       struct kobject *target_kobj,
>>>> +				       const char *target_name,
>>>> +				       const char *symlink_name)
>>>>  {
>>>>  	struct kernfs_node *target;
>>>>  	struct kernfs_node *entry;
>>>> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>>>  		return -ENOENT;
>>>>  	}
>>>>  
>>>> -	link = kernfs_create_link(kobj->sd, target_name, entry);
>>>> +	if (!symlink_name)
>>>> +		symlink_name = target_name;
>>>> +
>>>> +	link = kernfs_create_link(kobj->sd, symlink_name, entry);
>>>>  	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
>>>> -		sysfs_warn_dup(kobj->sd, target_name);
>>>> +		sysfs_warn_dup(kobj->sd, symlink_name);
>>>>  
>>>>  	kernfs_put(entry);
>>>>  	kernfs_put(target);
>>>>  	return PTR_ERR_OR_ZERO(link);
>>>>  }
>>>> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>>>> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>> index 5420817ed317..123c6f10333a 100644
>>>> --- a/include/linux/sysfs.h
>>>> +++ b/include/linux/sysfs.h
>>>> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>>>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>>>  				      struct kobject *target_kobj,
>>>>  				      const char *target_name);
>>>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>>>> +				       struct kobject *target_kobj,
>>>> +				       const char *target_name,
>>>> +				       const char *symlink_name);
>>>
>>> sysfs_create_symlink_entry_to_kobj()?
>>>
>>> I can't remember why we put __compat_only there, perhaps because we do
>>> not want people to really use this unless you really really have to?
>>
>> We don't have much option here. I tried replicating the sysfs files
>> in older patch series but creating symlink at old location is much
>> better approach.
>>
>> The __compat_only_sysfs_link_entry_to_kobj function is pretty generic,
>> unable to understand the reason behind restricting its usage.
>>
>>>
>>> So then keep compat_only here as well?
>>
>> Sure, I will rename the wrapper function.
>>
>> But how about changing the function signature instead of creating
>> a wrapper function?
>>
>> Considering the fact that there are only two places this function
>> has called.
>>
>>>
>>> What breaks if you remove those undocumented sysfs files?  What
>>> userspace tool do you have that will even notice?
>>
>> The scripts used in kdump service need those sysfs files to control
>> the dump collection. So we can't just move the sysfs files to the
>> new location.
> 
> If you can not change them, then just document them and live with it.
> Why do this extra work to create a symlink for something you will never
> use?

Eventually the scripts will change but I think it is better to have some
overlap time to avoid breaking those scripts.


Thanks,
Sourabh Jain