[RFC PATCH 0/5] kobject: Add and use init predicate

[RFC PATCH 0/5] kobject: Add and use init predicate

[Tobin C. Harding]

Hi,

This set patches kobject to add a predicate function for determining the
initialization state of a kobject.  Stripped down, the predicate is:

	bool kobject_is_initialized(struct kobject *kobj)
	{
		return kobj->state_initialized
	}

This is RFC because there are merge conflicts with Greg's driver-core
tree.  I'm guessing this is caused by the cleanup patches (#2 and #3).
If the set is deemed likeable then I can re-work the set targeting
whomever's tree this would go in through.

Applies on top of:

	mainline tag: v5.1-rc6
	livepatching branch: for-next

Series Description
------------------
	
Patch #1 is a memleak patch, previously posted and not overly
interesting.  Comment by Greg on the thread on that patch was the
incentive for this series.

Patch #2 and #3 are kobject kernel-doc comment clean ups.  Can be
dropped if not liked.

Patch #4 adds the predicate function to the kobject API.

Patch #5 uses the new predicate to remove the custom logic from livepatch
for tracking kobject initialization state.

Testing
-------

Kernel build configuration

	$ egrep LIVEPATCH .config
	CONFIG_HAVE_LIVEPATCH=y
	CONFIG_LIVEPATCH=y
	CONFIG_TEST_LIVEPATCH=m

	$ egrep FTRACE .config
	CONFIG_KPROBES_ON_FTRACE=y
	CONFIG_HAVE_KPROBES_ON_FTRACE=y
	CONFIG_HAVE_DYNAMIC_FTRACE=y
	CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS=y
	CONFIG_HAVE_FTRACE_MCOUNT_RECORD=y
	CONFIG_FTRACE=y
	CONFIG_FTRACE_SYSCALLS=y
	CONFIG_DYNAMIC_FTRACE=y
	CONFIG_DYNAMIC_FTRACE_WITH_REGS=y
	CONFIG_FTRACE_MCOUNT_RECORD=y
	# CONFIG_FTRACE_STARTUP_TEST is not set

Builds fine but doesn't boot in Qemu.  I've never run dynamic Ftrace, it
appears to crash during this.  Was hoping to run the livepatch tests but
not sure how to at this moment.  Is dynamic Ftrace and livepatch testing
something that can even be done in a VM or do I need to do this or
baremetal?

Thanks for taking the time to look at this.

	Tobin


Tobin C. Harding (5):
  livepatch: Fix kobject memleak
  kobject: Remove docstring reference to kset
  kobject: Fix kernel-doc comment first line
  kobject: Add kobject initialized predicate
  livepatch: Do not manually track kobject initialization

 include/linux/kobject.h   |  2 ++
 include/linux/livepatch.h |  6 ----
 kernel/livepatch/core.c   | 28 +++++++++---------
 lib/kobject.c             | 60 +++++++++++++++++++++++----------------
 4 files changed, 51 insertions(+), 45 deletions(-)

-- 
2.21.0

[RFC PATCH 1/5] livepatch: Fix kobject memleak

[Tobin C. Harding]

Currently error return from kobject_init_and_add() is not followed by a
call to kobject_put().  This means there is a memory leak.

Add call to kobject_put() in error path of kobject_init_and_add().

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 kernel/livepatch/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..98295de2172b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
 				   &obj->kobj, "%s,%lu", func->old_name,
 				   func->old_sympos ? func->old_sympos : 1);
-	if (!ret)
+	if (ret)
+		kobject_put(&func->kobj);
+	else
 		func->kobj_added = true;
 
 	return ret;
@@ -803,8 +805,10 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	name = klp_is_module(obj) ? obj->name : "vmlinux";
 	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
 				   &patch->kobj, "%s", name);
-	if (ret)
+	if (ret) {
+		kobject_put(&obj->kobj);
 		return ret;
+	}
 	obj->kobj_added = true;
 
 	klp_for_each_func(obj, func) {
@@ -862,8 +866,10 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
 				   klp_root_kobj, "%s", patch->mod->name);
-	if (ret)
+	if (ret) {
+		kobject_put(&patch->kobj);
 		return ret;
+	}
 	patch->kobj_added = true;
 
 	if (patch->replace) {
-- 
2.21.0

[RFC PATCH 2/5] kobject: Remove docstring reference to kset

[Tobin C. Harding]

Currently the docstring for kobject_get_path() mentions 'kset'.  The
kset is not used in the function callchain starting from this function.

Remove docstring reference to kset from the function kobject_get_path().

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 lib/kobject.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index aa89edcd2b63..3eacd5b4643f 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -153,12 +153,11 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
 }
 
 /**
- * kobject_get_path - generate and return the path associated with a given kobj and kset pair.
- *
+ * kobject_get_path() - Allocate memory and fill in the path for @kobj.
  * @kobj:	kobject in question, with which to build the path
  * @gfp_mask:	the allocation type used to allocate the path
  *
- * The result must be freed by the caller with kfree().
+ * Return: The newly allocated memory, caller must free with kfree().
  */
 char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask)
 {
-- 
2.21.0

[RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Tobin C. Harding]

kernel-doc comments have a prescribed format.  This includes parenthesis
on the function name.  To be _particularly_ correct we should also
capitalise the brief description and terminate it with a period.

In preparation for adding/updating kernel-doc function comments clean up
the ones currently present.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 lib/kobject.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 3eacd5b4643f..0181f102cd1c 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -18,7 +18,7 @@
 #include <linux/random.h>
 
 /**
- * kobject_namespace - return @kobj's namespace tag
+ * kobject_namespace() - Return @kobj's namespace tag.
  * @kobj: kobject in question
  *
  * Returns namespace tag of @kobj if its parent has namespace ops enabled
@@ -36,7 +36,7 @@ const void *kobject_namespace(struct kobject *kobj)
 }
 
 /**
- * kobject_get_ownership - get sysfs ownership data for @kobj
+ * kobject_get_ownership() - Get sysfs ownership data for @kobj.
  * @kobj: kobject in question
  * @uid: kernel user ID for sysfs objects
  * @gid: kernel group ID for sysfs objects
@@ -264,7 +264,7 @@ static int kobject_add_internal(struct kobject *kobj)
 }
 
 /**
- * kobject_set_name_vargs - Set the name of an kobject
+ * kobject_set_name_vargs() - Set the name of a kobject.
  * @kobj: struct kobject to set the name of
  * @fmt: format string used to build the name
  * @vargs: vargs to format the string.
@@ -304,7 +304,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 }
 
 /**
- * kobject_set_name - Set the name of a kobject
+ * kobject_set_name() - Set the name of a kobject.
  * @kobj: struct kobject to set the name of
  * @fmt: format string used to build the name
  *
@@ -326,7 +326,7 @@ int kobject_set_name(struct kobject *kobj, const char *fmt, ...)
 EXPORT_SYMBOL(kobject_set_name);
 
 /**
- * kobject_init - initialize a kobject structure
+ * kobject_init() - Initialize a kobject structure.
  * @kobj: pointer to the kobject to initialize
  * @ktype: pointer to the ktype for this kobject.
  *
@@ -382,7 +382,7 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
 }
 
 /**
- * kobject_add - the main kobject add function
+ * kobject_add() - The main kobject add function.
  * @kobj: the kobject to add
  * @parent: pointer to the parent of the kobject.
  * @fmt: format to name the kobject with.
@@ -430,7 +430,8 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
 EXPORT_SYMBOL(kobject_add);
 
 /**
- * kobject_init_and_add - initialize a kobject structure and add it to the kobject hierarchy
+ * kobject_init_and_add() - Initialize a kobject structure and add it to
+ *                          the kobject hierarchy.
  * @kobj: pointer to the kobject to initialize
  * @ktype: pointer to the ktype for this kobject.
  * @parent: pointer to the parent of this kobject.
@@ -457,7 +458,7 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
 EXPORT_SYMBOL_GPL(kobject_init_and_add);
 
 /**
- * kobject_rename - change the name of an object
+ * kobject_rename() - Change the name of an object.
  * @kobj: object in question.
  * @new_name: object's new name
  *
@@ -524,7 +525,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 EXPORT_SYMBOL_GPL(kobject_rename);
 
 /**
- * kobject_move - move object to another parent
+ * kobject_move() - Move object to another parent.
  * @kobj: object in question.
  * @new_parent: object's new parent (can be NULL)
  */
@@ -577,7 +578,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
 EXPORT_SYMBOL_GPL(kobject_move);
 
 /**
- * kobject_del - unlink kobject from hierarchy.
+ * kobject_del() - Unlink kobject from hierarchy.
  * @kobj: object.
  */
 void kobject_del(struct kobject *kobj)
@@ -599,7 +600,7 @@ void kobject_del(struct kobject *kobj)
 EXPORT_SYMBOL(kobject_del);
 
 /**
- * kobject_get - increment refcount for object.
+ * kobject_get() - Increment refcount for object.
  * @kobj: object.
  */
 struct kobject *kobject_get(struct kobject *kobj)
@@ -692,7 +693,7 @@ static void kobject_release(struct kref *kref)
 }
 
 /**
- * kobject_put - decrement refcount for object.
+ * kobject_put() - Decrement refcount for object.
  * @kobj: object.
  *
  * Decrement the refcount, and if 0, call kobject_cleanup().
@@ -721,7 +722,7 @@ static struct kobj_type dynamic_kobj_ktype = {
 };
 
 /**
- * kobject_create - create a struct kobject dynamically
+ * kobject_create() - Create a struct kobject dynamically.
  *
  * This function creates a kobject structure dynamically and sets it up
  * to be a "dynamic" kobject with a default release function set up.
@@ -744,8 +745,8 @@ struct kobject *kobject_create(void)
 }
 
 /**
- * kobject_create_and_add - create a struct kobject dynamically and register it with sysfs
- *
+ * kobject_create_and_add() - Create a struct kobject dynamically and
+ *                            register it with sysfs.
  * @name: the name for the kobject
  * @parent: the parent kobject of this kobject, if any.
  *
@@ -776,7 +777,7 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
 EXPORT_SYMBOL_GPL(kobject_create_and_add);
 
 /**
- * kset_init - initialize a kset for use
+ * kset_init() - Initialize a kset for use.
  * @k: kset
  */
 void kset_init(struct kset *k)
@@ -818,7 +819,7 @@ const struct sysfs_ops kobj_sysfs_ops = {
 EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
 
 /**
- * kset_register - initialize and add a kset.
+ * kset_register() - Initialize and add a kset.
  * @k: kset.
  */
 int kset_register(struct kset *k)
@@ -838,7 +839,7 @@ int kset_register(struct kset *k)
 EXPORT_SYMBOL(kset_register);
 
 /**
- * kset_unregister - remove a kset.
+ * kset_unregister() - Remove a kset.
  * @k: kset.
  */
 void kset_unregister(struct kset *k)
@@ -851,7 +852,7 @@ void kset_unregister(struct kset *k)
 EXPORT_SYMBOL(kset_unregister);
 
 /**
- * kset_find_obj - search for object in kset.
+ * kset_find_obj() - Search for object in kset.
  * @kset: kset we're looking in.
  * @name: object's name.
  *
@@ -899,7 +900,7 @@ static struct kobj_type kset_ktype = {
 };
 
 /**
- * kset_create - create a struct kset dynamically
+ * kset_create() - Create a struct kset dynamically.
  *
  * @name: the name for the kset
  * @uevent_ops: a struct kset_uevent_ops for the kset
@@ -943,7 +944,7 @@ static struct kset *kset_create(const char *name,
 }
 
 /**
- * kset_create_and_add - create a struct kset dynamically and add it to sysfs
+ * kset_create_and_add() - Create a struct kset dynamically and add it to sysfs.
  *
  * @name: the name for the kset
  * @uevent_ops: a struct kset_uevent_ops for the kset
-- 
2.21.0

[RFC PATCH 4/5] kobject: Add kobject initialized predicate

[Tobin C. Harding]

A call to kobject_init() is required to be paired with a call to
kobject_put() in order to correctly free up the kobject.  During cleanup
functions it would be useful to know if a kobject was initialized in
order to correctly pair the call to kobject_put().  For example this is
necessary if we attempt to initialize multiple objects on a list and one
fails - in order to correctly do cleanup we need to know which objects
have been initialized.

Add a predicate kobject_is_initialized() to the kobject API.  This
function maintains the kobject layer of abstraction; simply returns
kobj->state_initialized.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 include/linux/kobject.h |  2 ++
 lib/kobject.c           | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 1ab0d624fb36..65a317b65d9c 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -100,6 +100,8 @@ int kobject_init_and_add(struct kobject *kobj,
 			 struct kobj_type *ktype, struct kobject *parent,
 			 const char *fmt, ...);
 
+extern bool kobject_is_initialized(struct kobject *kobj);
+
 extern void kobject_del(struct kobject *kobj);
 
 extern struct kobject * __must_check kobject_create(void);
diff --git a/lib/kobject.c b/lib/kobject.c
index 0181f102cd1c..ecddf417f452 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -366,6 +366,18 @@ void kobject_init(struct kobject *kobj, struct kobj_type *ktype)
 }
 EXPORT_SYMBOL(kobject_init);
 
+/**
+ * kobject_is_initialized() - Kobject initialized predicate.
+ * @kobj: The kobject to query
+ *
+ * Return: True if @kobj has been initialized.
+ */
+bool kobject_is_initialized(struct kobject *kobj)
+{
+	return kobj->state_initialized;
+}
+EXPORT_SYMBOL(kobject_is_initialized);
+
 static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
 					   struct kobject *parent,
 					   const char *fmt, va_list vargs)
-- 
2.21.0

[RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Tobin C. Harding]

Currently we use custom logic to track kobject initialization.  Recently
a predicate function was added to the kobject API so we now no longer
need to do this.

Use kobject API to check for initialized state of kobjects instead of
using custom logic to track state.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 include/linux/livepatch.h |  6 ------
 kernel/livepatch/core.c   | 18 +++++-------------
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..955d46f37b72 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -47,7 +47,6 @@
  * @stack_node:	list node for klp_ops func_stack list
  * @old_size:	size of the old function
  * @new_size:	size of the new function
- * @kobj_added: @kobj has been added and needs freeing
  * @nop:        temporary patch to use the original code again; dyn. allocated
  * @patched:	the func has been added to the klp_ops list
  * @transition:	the func is currently being applied or reverted
@@ -86,7 +85,6 @@ struct klp_func {
 	struct list_head node;
 	struct list_head stack_node;
 	unsigned long old_size, new_size;
-	bool kobj_added;
 	bool nop;
 	bool patched;
 	bool transition;
@@ -126,7 +124,6 @@ struct klp_callbacks {
  * @node:	list node for klp_patch obj_list
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
- * @kobj_added: @kobj has been added and needs freeing
  * @dynamic:    temporary object for nop functions; dynamically allocated
  * @patched:	the object's funcs have been added to the klp_ops list
  */
@@ -141,7 +138,6 @@ struct klp_object {
 	struct list_head func_list;
 	struct list_head node;
 	struct module *mod;
-	bool kobj_added;
 	bool dynamic;
 	bool patched;
 };
@@ -154,7 +150,6 @@ struct klp_object {
  * @list:	list node for global list of actively used patches
  * @kobj:	kobject for sysfs resources
  * @obj_list:	dynamic list of the object entries
- * @kobj_added: @kobj has been added and needs freeing
  * @enabled:	the patch is enabled (but operation may be incomplete)
  * @forced:	was involved in a forced transition
  * @free_work:	patch cleanup from workqueue-context
@@ -170,7 +165,6 @@ struct klp_patch {
 	struct list_head list;
 	struct kobject kobj;
 	struct list_head obj_list;
-	bool kobj_added;
 	bool enabled;
 	bool forced;
 	struct work_struct free_work;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 98295de2172b..0b94aa5b38c9 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -590,7 +590,7 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
 		list_del(&func->node);
 
 		/* Might be called from klp_init_patch() error path. */
-		if (func->kobj_added) {
+		if (kobject_is_initialized(&func->kobj)) {
 			kobject_put(&func->kobj);
 		} else if (func->nop) {
 			klp_free_func_nop(func);
@@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
 		list_del(&obj->node);
 
 		/* Might be called from klp_init_patch() error path. */
-		if (obj->kobj_added) {
+		if (kobject_is_initialized(&obj->kobj)) {
 			kobject_put(&obj->kobj);
 		} else if (obj->dynamic) {
 			klp_free_object_dynamic(obj);
@@ -675,7 +675,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
 	 * this is called when the patch gets disabled and it
 	 * cannot get enabled again.
 	 */
-	if (patch->kobj_added) {
+	if (kobject_is_initialized(&patch->kobj)) {
 		kobject_put(&patch->kobj);
 		wait_for_completion(&patch->finish);
 	}
@@ -729,8 +729,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 				   func->old_sympos ? func->old_sympos : 1);
 	if (ret)
 		kobject_put(&func->kobj);
-	else
-		func->kobj_added = true;
 
 	return ret;
 }
@@ -809,7 +807,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		kobject_put(&obj->kobj);
 		return ret;
 	}
-	obj->kobj_added = true;
 
 	klp_for_each_func(obj, func) {
 		ret = klp_init_func(obj, func);
@@ -833,7 +830,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 
 	INIT_LIST_HEAD(&patch->list);
 	INIT_LIST_HEAD(&patch->obj_list);
-	patch->kobj_added = false;
 	patch->enabled = false;
 	patch->forced = false;
 	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
@@ -844,13 +840,10 @@ static int klp_init_patch_early(struct klp_patch *patch)
 			return -EINVAL;
 
 		INIT_LIST_HEAD(&obj->func_list);
-		obj->kobj_added = false;
 		list_add_tail(&obj->node, &patch->obj_list);
 
-		klp_for_each_func_static(obj, func) {
-			func->kobj_added = false;
+		klp_for_each_func_static(obj, func)
 			list_add_tail(&func->node, &obj->func_list);
-		}
 	}
 
 	if (!try_module_get(patch->mod))
@@ -870,7 +863,6 @@ static int klp_init_patch(struct klp_patch *patch)
 		kobject_put(&patch->kobj);
 		return ret;
 	}
-	patch->kobj_added = true;
 
 	if (patch->replace) {
 		ret = klp_add_nops(patch);
@@ -932,7 +924,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
-	if (!patch->kobj_added)
+	if (kobject_is_initialized(&patch->kobj))
 		return -EINVAL;
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
-- 
2.21.0

Re: [RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> Currently we use custom logic to track kobject initialization.  Recently
> a predicate function was added to the kobject API so we now no longer
> need to do this.
> 
> Use kobject API to check for initialized state of kobjects instead of
> using custom logic to track state.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  include/linux/livepatch.h |  6 ------
>  kernel/livepatch/core.c   | 18 +++++-------------
>  2 files changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..955d46f37b72 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -47,7 +47,6 @@
>   * @stack_node:	list node for klp_ops func_stack list
>   * @old_size:	size of the old function
>   * @new_size:	size of the new function
> - * @kobj_added: @kobj has been added and needs freeing
>   * @nop:        temporary patch to use the original code again; dyn. allocated
>   * @patched:	the func has been added to the klp_ops list
>   * @transition:	the func is currently being applied or reverted
> @@ -86,7 +85,6 @@ struct klp_func {
>  	struct list_head node;
>  	struct list_head stack_node;
>  	unsigned long old_size, new_size;
> -	bool kobj_added;
>  	bool nop;
>  	bool patched;
>  	bool transition;
> @@ -126,7 +124,6 @@ struct klp_callbacks {
>   * @node:	list node for klp_patch obj_list
>   * @mod:	kernel module associated with the patched object
>   *		(NULL for vmlinux)
> - * @kobj_added: @kobj has been added and needs freeing
>   * @dynamic:    temporary object for nop functions; dynamically allocated
>   * @patched:	the object's funcs have been added to the klp_ops list
>   */
> @@ -141,7 +138,6 @@ struct klp_object {
>  	struct list_head func_list;
>  	struct list_head node;
>  	struct module *mod;
> -	bool kobj_added;
>  	bool dynamic;
>  	bool patched;
>  };
> @@ -154,7 +150,6 @@ struct klp_object {
>   * @list:	list node for global list of actively used patches
>   * @kobj:	kobject for sysfs resources
>   * @obj_list:	dynamic list of the object entries
> - * @kobj_added: @kobj has been added and needs freeing
>   * @enabled:	the patch is enabled (but operation may be incomplete)
>   * @forced:	was involved in a forced transition
>   * @free_work:	patch cleanup from workqueue-context
> @@ -170,7 +165,6 @@ struct klp_patch {
>  	struct list_head list;
>  	struct kobject kobj;
>  	struct list_head obj_list;
> -	bool kobj_added;
>  	bool enabled;
>  	bool forced;
>  	struct work_struct free_work;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 98295de2172b..0b94aa5b38c9 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -590,7 +590,7 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
>  		list_del(&func->node);
>  
>  		/* Might be called from klp_init_patch() error path. */
> -		if (func->kobj_added) {
> +		if (kobject_is_initialized(&func->kobj)) {
>  			kobject_put(&func->kobj);
>  		} else if (func->nop) {
>  			klp_free_func_nop(func);

This feels really odd to me, why do we need to keep track of this type
of thing?

How can the kobject _not_ be initialized?  If it's just because we don't
want to write two separate error cleanup paths, that's not ok.

> @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
>  		list_del(&obj->node);
>  
>  		/* Might be called from klp_init_patch() error path. */
> -		if (obj->kobj_added) {
> +		if (kobject_is_initialized(&obj->kobj)) {
>  			kobject_put(&obj->kobj);
>  		} else if (obj->dynamic) {
>  			klp_free_object_dynamic(obj);

Same here, let's not be lazy.

The code should "know" if the kobject has been initialized or not
because it is the entity that asked for it to be initialized.  Don't add
extra logic to the kobject core (like the patch before this did) just
because this one subsystem wanted to only write 1 "cleanup" function.

thanks,

greg k-h

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> kernel-doc comments have a prescribed format.  This includes parenthesis
> on the function name.  To be _particularly_ correct we should also
> capitalise the brief description and terminate it with a period.

Ah, I didn't know that, sorry about that, my fault.  I'll take patch 2
and 3 now in my tree, thanks!

greg k-h

Re: [RFC PATCH 2/5] kobject: Remove docstring reference to kset

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 12:31:39PM +1000, Tobin C. Harding wrote:
> Currently the docstring for kobject_get_path() mentions 'kset'.  The
> kset is not used in the function callchain starting from this function.
> 
> Remove docstring reference to kset from the function kobject_get_path().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  lib/kobject.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index aa89edcd2b63..3eacd5b4643f 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -153,12 +153,11 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
>  }
>  
>  /**
> - * kobject_get_path - generate and return the path associated with a given kobj and kset pair.
> - *
> + * kobject_get_path() - Allocate memory and fill in the path for @kobj.

Wow, that's an old change that caused this to be not true anymore, nice catch.

greg k-h

Re: [RFC PATCH 1/5] livepatch: Fix kobject memleak

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 12:31:38PM +1000, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Petr Mladek]

On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > Currently we use custom logic to track kobject initialization.  Recently
> > a predicate function was added to the kobject API so we now no longer
> > need to do this.
> > 
> > Use kobject API to check for initialized state of kobjects instead of
> > using custom logic to track state.
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  include/linux/livepatch.h |  6 ------
> >  kernel/livepatch/core.c   | 18 +++++-------------
> >  2 files changed, 5 insertions(+), 19 deletions(-)
> > 
> > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> >  		list_del(&obj->node);
> >  
> >  		/* Might be called from klp_init_patch() error path. */
> > -		if (obj->kobj_added) {
> > +		if (kobject_is_initialized(&obj->kobj)) {
> >  			kobject_put(&obj->kobj);
> >  		} else if (obj->dynamic) {
> >  			klp_free_object_dynamic(obj);
> 
> Same here, let's not be lazy.
> 
> The code should "know" if the kobject has been initialized or not
> because it is the entity that asked for it to be initialized.  Don't add
> extra logic to the kobject core (like the patch before this did) just
> because this one subsystem wanted to only write 1 "cleanup" function.

We use kobject for a mix of statically and dynamically defined
structures[*]. And we misunderstood the behavior of kobject_init().

Anyway, the right solution is to call kobject_init()
already in klp_init_patch_early() for the statically
defined structures and in klp_alloc*() for the dynamically
allocated ones. Then we could simply call kobject_put()
every time.

Tobin, this goes deeper into the livepatching code that
you probably expected. Do you want to do the above
suggested change or should I prepare the patch?

Anyway, thanks for working on this.


[*] Yes, we know that kobject was not designed for
    static structures. We even tried to use them but
    there was a lot of extra code with not a big benefit.

Best Regards,
Petr

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Johan Hovold]

On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> kernel-doc comments have a prescribed format.  This includes parenthesis
> on the function name.  To be _particularly_ correct we should also
> capitalise the brief description and terminate it with a period.

Why do think capitalisation and full stop is required for the function
description?

Sure, the example in the current doc happen to use that, but I'm not
sure that's intended as a prescription.

The old kernel-doc nano-HOWTO specifically did not use this:

	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Johan

Re: [RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > Currently we use custom logic to track kobject initialization.  Recently
> > > a predicate function was added to the kobject API so we now no longer
> > > need to do this.
> > > 
> > > Use kobject API to check for initialized state of kobjects instead of
> > > using custom logic to track state.
> > > 
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > ---
> > >  include/linux/livepatch.h |  6 ------
> > >  kernel/livepatch/core.c   | 18 +++++-------------
> > >  2 files changed, 5 insertions(+), 19 deletions(-)
> > > 
> > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > >  		list_del(&obj->node);
> > >  
> > >  		/* Might be called from klp_init_patch() error path. */
> > > -		if (obj->kobj_added) {
> > > +		if (kobject_is_initialized(&obj->kobj)) {
> > >  			kobject_put(&obj->kobj);
> > >  		} else if (obj->dynamic) {
> > >  			klp_free_object_dynamic(obj);
> > 
> > Same here, let's not be lazy.
> > 
> > The code should "know" if the kobject has been initialized or not
> > because it is the entity that asked for it to be initialized.  Don't add
> > extra logic to the kobject core (like the patch before this did) just
> > because this one subsystem wanted to only write 1 "cleanup" function.
> 
> We use kobject for a mix of statically and dynamically defined
> structures[*]. And we misunderstood the behavior of kobject_init().

Eeek, no, a kobject should never be used for a static structure, that's
just wrong.

Well, almost wrong, ignore how the driver core itself does this in
places :)

thanks,

greg k-h

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Tobin C. Harding]

Adding Jon to CC

On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
> On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> > kernel-doc comments have a prescribed format.  This includes parenthesis
> > on the function name.  To be _particularly_ correct we should also
> > capitalise the brief description and terminate it with a period.
> 
> Why do think capitalisation and full stop is required for the function
> description?
> 
> Sure, the example in the current doc happen to use that, but I'm not
> sure that's intended as a prescription.
> 
> The old kernel-doc nano-HOWTO specifically did not use this:
> 
> 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> 

Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst

	Function documentation
	----------------------

	The general format of a function and function-like macro kernel-doc comment is::

	  /**
	   * function_name() - Brief description of function.
	   * @arg1: Describe the first argument.
	   * @arg2: Describe the second argument.
	   *        One can provide multiple line descriptions
	   *        for arguments.

I figured that was the canonical way to do kernel-doc function
comments.  I have however refrained from capitalising and adding the
period to argument strings to reduce code churn.  I figured if I'm
touching the line to add parenthesis then I might as well make it
perfect (if such a thing exists).

thanks,
Tobin.

Re: [RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Tobin C. Harding]

On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > Currently we use custom logic to track kobject initialization.  Recently
> > > a predicate function was added to the kobject API so we now no longer
> > > need to do this.
> > > 
> > > Use kobject API to check for initialized state of kobjects instead of
> > > using custom logic to track state.
> > > 
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > ---
> > >  include/linux/livepatch.h |  6 ------
> > >  kernel/livepatch/core.c   | 18 +++++-------------
> > >  2 files changed, 5 insertions(+), 19 deletions(-)
> > > 
> > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > >  		list_del(&obj->node);
> > >  
> > >  		/* Might be called from klp_init_patch() error path. */
> > > -		if (obj->kobj_added) {
> > > +		if (kobject_is_initialized(&obj->kobj)) {
> > >  			kobject_put(&obj->kobj);
> > >  		} else if (obj->dynamic) {
> > >  			klp_free_object_dynamic(obj);
> > 
> > Same here, let's not be lazy.
> > 
> > The code should "know" if the kobject has been initialized or not
> > because it is the entity that asked for it to be initialized.  Don't add
> > extra logic to the kobject core (like the patch before this did) just
> > because this one subsystem wanted to only write 1 "cleanup" function.
> 
> We use kobject for a mix of statically and dynamically defined
> structures[*]. And we misunderstood the behavior of kobject_init().
> 
> Anyway, the right solution is to call kobject_init()
> already in klp_init_patch_early() for the statically
> defined structures and in klp_alloc*() for the dynamically
> allocated ones. Then we could simply call kobject_put()
> every time.
> 
> Tobin, this goes deeper into the livepatching code that
> you probably expected. Do you want to do the above
> suggested change or should I prepare the patch?

I'd love for you to handle this one Petr, I'd say its a net gain
time wise that way since if I do it you'll have to review it too
carefully anyways.

So that will mean patch #1 and #5 of this series are dropped and handed
off to you (thanks).  Patch #2 and #3 Greg said he will take.  Patch #4
is not needed.  That's a win in my books :)

Thanks,
Tobin.

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Johan Hovold]

On Thu, May 02, 2019 at 06:25:39PM +1000, Tobin C. Harding wrote: > Adding Jon to CC
> 
> On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
> > On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> > > kernel-doc comments have a prescribed format.  This includes parenthesis
> > > on the function name.  To be _particularly_ correct we should also
> > > capitalise the brief description and terminate it with a period.
> > 
> > Why do think capitalisation and full stop is required for the function
> > description?
> > 
> > Sure, the example in the current doc happen to use that, but I'm not
> > sure that's intended as a prescription.
> > 
> > The old kernel-doc nano-HOWTO specifically did not use this:
> > 
> > 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > 
> 
> Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst
> 
> 	Function documentation
> 	----------------------
> 
> 	The general format of a function and function-like macro kernel-doc comment is::
> 
> 	  /**
> 	   * function_name() - Brief description of function.
> 	   * @arg1: Describe the first argument.
> 	   * @arg2: Describe the second argument.
> 	   *        One can provide multiple line descriptions
> 	   *        for arguments.
> 
> I figured that was the canonical way to do kernel-doc function
> comments.  I have however refrained from capitalising and adding the
> period to argument strings to reduce code churn.  I figured if I'm
> touching the line to add parenthesis then I might as well make it
> perfect (if such a thing exists).

I think you may have read too much into that example. Many of the
current function and parameter descriptions aren't even full sentences,
so sentence case and full stop doesn't really make any sense.

Looks like we discussed this last fall as well:

	https://lkml.kernel.org/r/20180912093116.GC1089@localhost

Johan

Re: [RFC PATCH 1/5] livepatch: Fix kobject memleak

[Petr Mladek]

On Thu 2019-05-02 12:31:38, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.

Strictly speaking there is no real memory leak in this case because
the structures are either static and or freed later via
klp_free*() functions.

That said, we could do the kobject manipulation a more clear way
as discussed in the 5th patch.

Anyway, thanks for cleaning this.

Best Regards,
Petr

Re: [RFC PATCH 5/5] livepatch: Do not manually track kobject initialization

[Petr Mladek]

On Thu 2019-05-02 18:31:27, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 09:30:44AM +0200, Petr Mladek wrote:
> > On Thu 2019-05-02 09:12:32, Greg Kroah-Hartman wrote:
> > > On Thu, May 02, 2019 at 12:31:42PM +1000, Tobin C. Harding wrote:
> > > > Currently we use custom logic to track kobject initialization.  Recently
> > > > a predicate function was added to the kobject API so we now no longer
> > > > need to do this.
> > > > 
> > > > Use kobject API to check for initialized state of kobjects instead of
> > > > using custom logic to track state.
> > > > 
> > > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > > ---
> > > >  include/linux/livepatch.h |  6 ------
> > > >  kernel/livepatch/core.c   | 18 +++++-------------
> > > >  2 files changed, 5 insertions(+), 19 deletions(-)
> > > > 
> > > > @@ -626,7 +626,7 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> > > >  		list_del(&obj->node);
> > > >  
> > > >  		/* Might be called from klp_init_patch() error path. */
> > > > -		if (obj->kobj_added) {
> > > > +		if (kobject_is_initialized(&obj->kobj)) {
> > > >  			kobject_put(&obj->kobj);
> > > >  		} else if (obj->dynamic) {
> > > >  			klp_free_object_dynamic(obj);
> > > 
> > > Same here, let's not be lazy.
> > > 
> > > The code should "know" if the kobject has been initialized or not
> > > because it is the entity that asked for it to be initialized.  Don't add
> > > extra logic to the kobject core (like the patch before this did) just
> > > because this one subsystem wanted to only write 1 "cleanup" function.
> > 
> > We use kobject for a mix of statically and dynamically defined
> > structures[*]. And we misunderstood the behavior of kobject_init().
> > 
> > Anyway, the right solution is to call kobject_init()
> > already in klp_init_patch_early() for the statically
> > defined structures and in klp_alloc*() for the dynamically
> > allocated ones. Then we could simply call kobject_put()
> > every time.
> > 
> > Tobin, this goes deeper into the livepatching code that
> > you probably expected. Do you want to do the above
> > suggested change or should I prepare the patch?
> 
> I'd love for you to handle this one Petr, I'd say its a net gain
> time wise that way since if I do it you'll have to review it too
> carefully anyways.
> 
> So that will mean patch #1 and #5 of this series are dropped and handed
> off to you (thanks).  Patch #2 and #3 Greg said he will take.  Patch #4
> is not needed.  That's a win in my books :)

Sound like a great plan. I am going to work on the patch for
the livepatching code.

Anyway, thanks a lot for your patches. It is a big relief to realize
that we could remove some hacks and do it clearly, modulo the static
structures ;-)

Best Regards,
Petr

Re: [RFC PATCH 0/5] kobject: Add and use init predicate

[Miroslav Benes]

> Testing
> -------
> 
> Kernel build configuration
> 
> 	$ egrep LIVEPATCH .config
> 	CONFIG_HAVE_LIVEPATCH=y
> 	CONFIG_LIVEPATCH=y
> 	CONFIG_TEST_LIVEPATCH=m
> 
> 	$ egrep FTRACE .config
> 	CONFIG_KPROBES_ON_FTRACE=y
> 	CONFIG_HAVE_KPROBES_ON_FTRACE=y
> 	CONFIG_HAVE_DYNAMIC_FTRACE=y
> 	CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS=y
> 	CONFIG_HAVE_FTRACE_MCOUNT_RECORD=y
> 	CONFIG_FTRACE=y
> 	CONFIG_FTRACE_SYSCALLS=y
> 	CONFIG_DYNAMIC_FTRACE=y
> 	CONFIG_DYNAMIC_FTRACE_WITH_REGS=y
> 	CONFIG_FTRACE_MCOUNT_RECORD=y
> 	# CONFIG_FTRACE_STARTUP_TEST is not set
> 
> Builds fine but doesn't boot in Qemu.  I've never run dynamic Ftrace, it
> appears to crash during this.  Was hoping to run the livepatch tests but
> not sure how to at this moment.  Is dynamic Ftrace and livepatch testing
> something that can even be done in a VM or do I need to do this or
> baremetal?

It definitely should work in VM/qemu. We use it like that all the time.

Miroslav

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Tobin C. Harding]

On Thu, May 02, 2019 at 10:39:22AM +0200, Johan Hovold wrote:
> On Thu, May 02, 2019 at 06:25:39PM +1000, Tobin C. Harding wrote: > Adding Jon to CC
> > 
> > On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
> > > On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> > > > kernel-doc comments have a prescribed format.  This includes parenthesis
> > > > on the function name.  To be _particularly_ correct we should also
> > > > capitalise the brief description and terminate it with a period.
> > > 
> > > Why do think capitalisation and full stop is required for the function
> > > description?
> > > 
> > > Sure, the example in the current doc happen to use that, but I'm not
> > > sure that's intended as a prescription.
> > > 
> > > The old kernel-doc nano-HOWTO specifically did not use this:
> > > 
> > > 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > 
> > 
> > Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst
> > 
> > 	Function documentation
> > 	----------------------
> > 
> > 	The general format of a function and function-like macro kernel-doc comment is::
> > 
> > 	  /**
> > 	   * function_name() - Brief description of function.
> > 	   * @arg1: Describe the first argument.
> > 	   * @arg2: Describe the second argument.
> > 	   *        One can provide multiple line descriptions
> > 	   *        for arguments.
> > 
> > I figured that was the canonical way to do kernel-doc function
> > comments.  I have however refrained from capitalising and adding the
> > period to argument strings to reduce code churn.  I figured if I'm
> > touching the line to add parenthesis then I might as well make it
> > perfect (if such a thing exists).
>
> I think you may have read too much into that example. Many of the
> current function and parameter descriptions aren't even full sentences,
> so sentence case and full stop doesn't really make any sense.
>
> Looks like we discussed this last fall as well:

Ha, this was funny.  By 'we' at first I thought you meant 'we the kernel
community' but you actually meant we as in 'me and you'.  Clearly you
failed to convince me last time :)

> 	https://lkml.kernel.org/r/20180912093116.GC1089@localhost

I am totally aware this is close to code churn and any discussion is
bikeshedding ... for me just because loads of places don't do this it
still looks nicer to my eyes

/**
* sfn() - Super awesome function.

than

/**
*/ sfn() - super awesome function

I most likely will keep doing these changes if I am touching the
kernel-doc comments for other reasons and then drop the changes if the
subsystem maintainer thinks its code churn.

I defiantly won't do theses changes in GNSS, GREYBUS, or USB SERIAL.

Oh, and I'm totally going to CC you know every time I flick one of these
patches, prepare to get spammed :)

Cheers,
Tobin.

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Randy Dunlap]

On 5/2/19 6:40 PM, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 10:39:22AM +0200, Johan Hovold wrote:
>> On Thu, May 02, 2019 at 06:25:39PM +1000, Tobin C. Harding wrote: > Adding Jon to CC
>>>
>>> On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
>>>> On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
>>>>> kernel-doc comments have a prescribed format.  This includes parenthesis
>>>>> on the function name.  To be _particularly_ correct we should also
>>>>> capitalise the brief description and terminate it with a period.
>>>>
>>>> Why do think capitalisation and full stop is required for the function
>>>> description?
>>>>
>>>> Sure, the example in the current doc happen to use that, but I'm not
>>>> sure that's intended as a prescription.
>>>>
>>>> The old kernel-doc nano-HOWTO specifically did not use this:
>>>>
>>>> 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>>>>
>>>
>>> Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst
>>>
>>> 	Function documentation
>>> 	----------------------
>>>
>>> 	The general format of a function and function-like macro kernel-doc comment is::
>>>
>>> 	  /**
>>> 	   * function_name() - Brief description of function.
>>> 	   * @arg1: Describe the first argument.
>>> 	   * @arg2: Describe the second argument.
>>> 	   *        One can provide multiple line descriptions
>>> 	   *        for arguments.
>>>
>>> I figured that was the canonical way to do kernel-doc function
>>> comments.  I have however refrained from capitalising and adding the
>>> period to argument strings to reduce code churn.  I figured if I'm
>>> touching the line to add parenthesis then I might as well make it
>>> perfect (if such a thing exists).
>>
>> I think you may have read too much into that example. Many of the
>> current function and parameter descriptions aren't even full sentences,
>> so sentence case and full stop doesn't really make any sense.
>>
>> Looks like we discussed this last fall as well:
> 
> Ha, this was funny.  By 'we' at first I thought you meant 'we the kernel
> community' but you actually meant we as in 'me and you'.  Clearly you
> failed to convince me last time :)
> 
>> 	https://lkml.kernel.org/r/20180912093116.GC1089@localhost
> 
> I am totally aware this is close to code churn and any discussion is
> bikeshedding ... for me just because loads of places don't do this it
> still looks nicer to my eyes
> 
> /**
> * sfn() - Super awesome function.
> 
> than
> 
> /**
> */ sfn() - super awesome function
> 
> I most likely will keep doing these changes if I am touching the
> kernel-doc comments for other reasons and then drop the changes if the
> subsystem maintainer thinks its code churn.
> 
> I defiantly won't do theses changes in GNSS, GREYBUS, or USB SERIAL.
> 
> Oh, and I'm totally going to CC you know every time I flick one of these
> patches, prepare to get spammed :)

I have seen this discussion before also.  And sometimes it is not even
a discussion -- it's more of an edict.  To which I object/disagree.
The current (or past) comment style is perfectly fine IMO.
No caps needed.  No ending '.' needed.



-- 
~Randy

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Johan Hovold]

On Fri, May 03, 2019 at 11:40:15AM +1000, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 10:39:22AM +0200, Johan Hovold wrote:
> > On Thu, May 02, 2019 at 06:25:39PM +1000, Tobin C. Harding wrote: > Adding Jon to CC
> > > 
> > > On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
> > > > On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> > > > > kernel-doc comments have a prescribed format.  This includes parenthesis
> > > > > on the function name.  To be _particularly_ correct we should also
> > > > > capitalise the brief description and terminate it with a period.
> > > > 
> > > > Why do think capitalisation and full stop is required for the function
> > > > description?
> > > > 
> > > > Sure, the example in the current doc happen to use that, but I'm not
> > > > sure that's intended as a prescription.
> > > > 
> > > > The old kernel-doc nano-HOWTO specifically did not use this:
> > > > 
> > > > 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > > 
> > > 
> > > Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst
> > > 
> > > 	Function documentation
> > > 	----------------------
> > > 
> > > 	The general format of a function and function-like macro kernel-doc comment is::
> > > 
> > > 	  /**
> > > 	   * function_name() - Brief description of function.
> > > 	   * @arg1: Describe the first argument.
> > > 	   * @arg2: Describe the second argument.
> > > 	   *        One can provide multiple line descriptions
> > > 	   *        for arguments.
> > > 
> > > I figured that was the canonical way to do kernel-doc function
> > > comments.  I have however refrained from capitalising and adding the
> > > period to argument strings to reduce code churn.  I figured if I'm
> > > touching the line to add parenthesis then I might as well make it
> > > perfect (if such a thing exists).
> >
> > I think you may have read too much into that example. Many of the
> > current function and parameter descriptions aren't even full sentences,
> > so sentence case and full stop doesn't really make any sense.
> >
> > Looks like we discussed this last fall as well:
> 
> Ha, this was funny.  By 'we' at first I thought you meant 'we the kernel
> community' but you actually meant we as in 'me and you'.  Clearly you
> failed to convince me last time :)
> 
> > 	https://lkml.kernel.org/r/20180912093116.GC1089@localhost
> 
> I am totally aware this is close to code churn and any discussion is
> bikeshedding ... for me just because loads of places don't do this it
> still looks nicer to my eyes
> 
> /**
> * sfn() - Super awesome function.
> 
> than
> 
> /**
> */ sfn() - super awesome function
> 
> I most likely will keep doing these changes if I am touching the
> kernel-doc comments for other reasons and then drop the changes if the
> subsystem maintainer thinks its code churn.
> 
> I defiantly won't do theses changes in GNSS, GREYBUS, or USB SERIAL.

This isn't about any particular subsystem, but more the tendency of
people to make up random rules and try to to force it on others. It's
churn, and also makes things like code forensics and backports harder
for no good reason.

Both capitalisation styles are about as common for the function
description judging from a quick grep, but only 10% or so use a full
stop ('.'). And forcing the use of sentence case and full stop for
things like

	/**
	 * maar_init() - Initialise MAARs.

or

	* @instr: Operational instruction.

would be not just ugly, but wrong (as these are not independent
clauses).

Johan

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Petr Mladek]

On Fri 2019-05-03 09:56:07, Johan Hovold wrote:
> On Fri, May 03, 2019 at 11:40:15AM +1000, Tobin C. Harding wrote:
> > On Thu, May 02, 2019 at 10:39:22AM +0200, Johan Hovold wrote:
> > I am totally aware this is close to code churn and any discussion is
> > bikeshedding ... for me just because loads of places don't do this it
> > still looks nicer to my eyes
> > 
> > /**
> > * sfn() - Super awesome function.
> > 
> > than
> > 
> > /**
> > */ sfn() - super awesome function
> > 
> > I most likely will keep doing these changes if I am touching the
> > kernel-doc comments for other reasons and then drop the changes if the
> > subsystem maintainer thinks its code churn.
> > 
> > I defiantly won't do theses changes in GNSS, GREYBUS, or USB SERIAL.
> 
> This isn't about any particular subsystem, but more the tendency of
> people to make up random rules and try to to force it on others. It's
> churn, and also makes things like code forensics and backports harder
> for no good reason.

+1

But I could understand that it is hard to keep it as is when it bothers
ones eyes. I tend to change these things as well and have to activelly
stop myself again and again ;-)

Best Regards,
Petr

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Jonathan Corbet]

On Thu, 2 May 2019 18:46:16 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> I have seen this discussion before also.  And sometimes it is not even
> a discussion -- it's more of an edict.  To which I object/disagree.
> The current (or past) comment style is perfectly fine IMO.
> No caps needed.  No ending '.' needed.

For however much this matters...I really don't see that there needs to be
a rule one way or the other on this; the documentation serves its purpose
either way.  I guess I see it like "British or American spelling";
there's nothing to drive a conversion in either direction.

jon

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Tobin C. Harding]

On Fri, May 03, 2019 at 09:56:07AM +0200, Johan Hovold wrote:
> On Fri, May 03, 2019 at 11:40:15AM +1000, Tobin C. Harding wrote:
> > On Thu, May 02, 2019 at 10:39:22AM +0200, Johan Hovold wrote:
> > > On Thu, May 02, 2019 at 06:25:39PM +1000, Tobin C. Harding wrote: > Adding Jon to CC
> > > > 
> > > > On Thu, May 02, 2019 at 09:38:23AM +0200, Johan Hovold wrote:
> > > > > On Thu, May 02, 2019 at 12:31:40PM +1000, Tobin C. Harding wrote:
> > > > > > kernel-doc comments have a prescribed format.  This includes parenthesis
> > > > > > on the function name.  To be _particularly_ correct we should also
> > > > > > capitalise the brief description and terminate it with a period.
> > > > > 
> > > > > Why do think capitalisation and full stop is required for the function
> > > > > description?
> > > > > 
> > > > > Sure, the example in the current doc happen to use that, but I'm not
> > > > > sure that's intended as a prescription.
> > > > > 
> > > > > The old kernel-doc nano-HOWTO specifically did not use this:
> > > > > 
> > > > > 	https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> > > > > 
> > > > 
> > > > Oh?  I was basing this on Documentation/doc-guide/kernel-doc.rst
> > > > 
> > > > 	Function documentation
> > > > 	----------------------
> > > > 
> > > > 	The general format of a function and function-like macro kernel-doc comment is::
> > > > 
> > > > 	  /**
> > > > 	   * function_name() - Brief description of function.
> > > > 	   * @arg1: Describe the first argument.
> > > > 	   * @arg2: Describe the second argument.
> > > > 	   *        One can provide multiple line descriptions
> > > > 	   *        for arguments.
> > > > 
> > > > I figured that was the canonical way to do kernel-doc function
> > > > comments.  I have however refrained from capitalising and adding the
> > > > period to argument strings to reduce code churn.  I figured if I'm
> > > > touching the line to add parenthesis then I might as well make it
> > > > perfect (if such a thing exists).
> > >
> > > I think you may have read too much into that example. Many of the
> > > current function and parameter descriptions aren't even full sentences,
> > > so sentence case and full stop doesn't really make any sense.
> > >
> > > Looks like we discussed this last fall as well:
> > 
> > Ha, this was funny.  By 'we' at first I thought you meant 'we the kernel
> > community' but you actually meant we as in 'me and you'.  Clearly you
> > failed to convince me last time :)
> > 
> > > 	https://lkml.kernel.org/r/20180912093116.GC1089@localhost
> > 
> > I am totally aware this is close to code churn and any discussion is
> > bikeshedding ... for me just because loads of places don't do this it
> > still looks nicer to my eyes
> > 
> > /**
> > * sfn() - Super awesome function.
> > 
> > than
> > 
> > /**
> > */ sfn() - super awesome function
> > 
> > I most likely will keep doing these changes if I am touching the
> > kernel-doc comments for other reasons and then drop the changes if the
> > subsystem maintainer thinks its code churn.
> > 
> > I defiantly won't do theses changes in GNSS, GREYBUS, or USB SERIAL.
> 
> This isn't about any particular subsystem, but more the tendency of
> people to make up random rules and try to to force it on others. It's
> churn, and also makes things like code forensics and backports harder
> for no good reason.

Points noted.

> Both capitalisation styles are about as common for the function
> description judging from a quick grep, but only 10% or so use a full
> stop ('.'). And forcing the use of sentence case and full stop for
> things like
> 
> 	/**
> 	 * maar_init() - Initialise MAARs.
> 
> or
> 
> 	* @instr: Operational instruction.
> 
> would be not just ugly, but wrong (as these are not independent
> clauses).

You are correct here.

Thanks for taking the time to flesh out your argument Johan, I am now in
agreement with you :)

Cheers,
Tobin.

Re: [RFC PATCH 3/5] kobject: Fix kernel-doc comment first line

[Johan Hovold]

On Tue, May 07, 2019 at 09:00:35AM +1000, Tobin C. Harding wrote:
> On Fri, May 03, 2019 at 09:56:07AM +0200, Johan Hovold wrote:

> > This isn't about any particular subsystem, but more the tendency of
> > people to make up random rules and try to to force it on others. It's
> > churn, and also makes things like code forensics and backports harder
> > for no good reason.
> 
> Points noted.
> 
> > Both capitalisation styles are about as common for the function
> > description judging from a quick grep, but only 10% or so use a full
> > stop ('.'). And forcing the use of sentence case and full stop for
> > things like
> > 
> > 	/**
> > 	 * maar_init() - Initialise MAARs.
> > 
> > or
> > 
> > 	* @instr: Operational instruction.
> > 
> > would be not just ugly, but wrong (as these are not independent
> > clauses).
> 
> You are correct here.

Actually, I may have been wrong about the first example (imperative),
but the second still stands.

> Thanks for taking the time to flesh out your argument Johan, I am now in
> agreement with you :)

Good to hear! :)

Johan