[PATCH 0/3] ttp_port: Clean up kernel-docs

[PATCH 0/3] ttp_port: Clean up kernel-docs

[Tobin C. Harding]

Hi,

This set is just putting a toe in the water to see if kernel-docs
cleanup is wanted in drivers/tty/

While chasing syzkaller bugs I opened up drivers/tty/tty_port.c and
noticed that a bunch of the kernel-docs function comments were
incorrectly styled.  This is the case for more files under drivers/tty/,
if fixing this is deemed worthwhile I can add it to my list of jobs to
do when my brain is too fried to do anything else.

Changes comments that look like this:

/**
 *	ptmx_open		-	open a unix 98 pty master
 *	@inode: inode of device file
 *	@filp: file pointer to tty
 *
 *	Allocate a unix98 pty master device from the ptmx driver.
 *
 *	Locking: tty_mutex protects the init_dev work. tty->count should
 *		protect the rest.
 *		allocated_ptys_lock handles the list of free pty numbers
 */

static int ptmx_open(struct inode *inode, struct file *filp)

To look like this:

/**
 * ptmx_open() - Open a unix 98 pty master.
 * @inode: inode of device file
 * @filp: file pointer to tty
 *
 * Allocate a unix98 pty master device from the ptmx driver.
 *
 * Locking: tty_mutex protects the init_dev work. tty->count should
 * 	protect the rest.
 * 	allocated_ptys_lock handles the list of free pty numbers
 */
static int ptmx_open(struct inode *inode, struct file *filp)

This is super trivial clean up - I will not be offended if this is
deemed code churn.

thanks,
Tobin.


Tobin C. Harding (3):
  tty_port: Remove incorrect whitespace after comments
  tty_port: Fix function name and brief desc
  tty_port: Fix kernel-docs warnings

 drivers/tty/tty_port.c | 133 ++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 68 deletions(-)

-- 
2.17.1

[PATCH 1/3] tty_port: Remove incorrect whitespace after comments

[Tobin C. Harding]

Currently there are a bunch of kernel-doc function comments that have a
line of whitespace after the comment and before the function they
comment - this is incorrect, there should be no whitespace here.

Remove incorrect whitespace between comment and associated function.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/tty/tty_port.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 25d736880013..cb6075096a5b 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -279,7 +279,6 @@ EXPORT_SYMBOL(tty_port_put);
  *	Return a refcount protected tty instance or NULL if the port is not
  *	associated with a tty (eg due to close or hangup)
  */
-
 struct tty_struct *tty_port_tty_get(struct tty_port *port)
 {
 	unsigned long flags;
@@ -300,7 +299,6 @@ EXPORT_SYMBOL(tty_port_tty_get);
  *	Associate the port and tty pair. Manages any internal refcounts.
  *	Pass NULL to deassociate a port
  */
-
 void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty)
 {
 	unsigned long flags;
@@ -343,7 +341,6 @@ static void tty_port_shutdown(struct tty_port *port, struct tty_struct *tty)
  *
  *	Caller holds tty lock.
  */
-
 void tty_port_hangup(struct tty_port *port)
 {
 	struct tty_struct *tty;
@@ -399,7 +396,6 @@ EXPORT_SYMBOL_GPL(tty_port_tty_wakeup);
  *	to hide some internal details. This will eventually become entirely
  *	internal to the tty port.
  */
-
 int tty_port_carrier_raised(struct tty_port *port)
 {
 	if (port->ops->carrier_raised == NULL)
@@ -416,7 +412,6 @@ EXPORT_SYMBOL(tty_port_carrier_raised);
  *	to hide some internal details. This will eventually become entirely
  *	internal to the tty port.
  */
-
 void tty_port_raise_dtr_rts(struct tty_port *port)
 {
 	if (port->ops->dtr_rts)
@@ -432,7 +427,6 @@ EXPORT_SYMBOL(tty_port_raise_dtr_rts);
  *	to hide some internal details. This will eventually become entirely
  *	internal to the tty port.
  */
-
 void tty_port_lower_dtr_rts(struct tty_port *port)
 {
 	if (port->ops->dtr_rts)
@@ -464,7 +458,6 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
  *      NB: May drop and reacquire tty lock when blocking, so tty and tty_port
  *      may have changed state (eg., may have been hung up).
  */
-
 int tty_port_block_til_ready(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
-- 
2.17.1

[PATCH 2/3] tty_port: Fix function name and brief desc

[Tobin C. Harding]

File contains kernel-doc function comments that are not in the correct
style.

 - Add '()' to function names
 - Capitalise brief description and append a full stop.
 - Use correct spacing for function name / brief description as per the
   kernel docs (Documentation/doc-guide/kernel-doc.rst)

	/**
	 * function_name() - Brief description of function.
	 * @arg1: Describe the first argument.

 - Fix spacing for kernel-doc function comments (no tab space after '*').

These changes all touch the same lines so do them in a single patch.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/tty/tty_port.c | 120 ++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 61 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index cb6075096a5b..382282013c9d 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -74,7 +74,7 @@ void tty_port_init(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_init);
 
 /**
- * tty_port_link_device - link tty and tty_port
+ * tty_port_link_device() - Link tty and tty_port.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -94,7 +94,7 @@ void tty_port_link_device(struct tty_port *port,
 EXPORT_SYMBOL_GPL(tty_port_link_device);
 
 /**
- * tty_port_register_device - register tty device
+ * tty_port_register_device() - Register tty device.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -113,7 +113,7 @@ struct device *tty_port_register_device(struct tty_port *port,
 EXPORT_SYMBOL_GPL(tty_port_register_device);
 
 /**
- * tty_port_register_device_attr - register tty device
+ * tty_port_register_device_attr() - Register tty device.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -137,7 +137,7 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
 
 /**
- * tty_port_register_device_attr_serdev - register tty or serdev device
+ * tty_port_register_device_attr_serdev() - Register tty or serdev device.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -169,7 +169,7 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
 
 /**
- * tty_port_register_device_serdev - register tty or serdev device
+ * tty_port_register_device_serdev() - Register tty or serdev device.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -188,7 +188,7 @@ struct device *tty_port_register_device_serdev(struct tty_port *port,
 EXPORT_SYMBOL_GPL(tty_port_register_device_serdev);
 
 /**
- * tty_port_unregister_device - deregister a tty or serdev device
+ * tty_port_unregister_device() - Deregister a tty or serdev device.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @index: index of the tty
@@ -235,7 +235,7 @@ void tty_port_free_xmit_buf(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_free_xmit_buf);
 
 /**
- * tty_port_destroy -- destroy inited port
+ * tty_port_destroy() - Destroy inited port.
  * @port: tty port to be destroyed
  *
  * When a port was initialized using tty_port_init, one has to destroy the
@@ -273,11 +273,11 @@ void tty_port_put(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_put);
 
 /**
- *	tty_port_tty_get	-	get a tty reference
- *	@port: tty port
+ * tty_port_tty_get() - Get a tty reference.
+ * @port: tty port
  *
- *	Return a refcount protected tty instance or NULL if the port is not
- *	associated with a tty (eg due to close or hangup)
+ * Return a refcount protected tty instance or NULL if the port is not
+ * associated with a tty (eg due to close or hangup)
  */
 struct tty_struct *tty_port_tty_get(struct tty_port *port)
 {
@@ -292,12 +292,12 @@ struct tty_struct *tty_port_tty_get(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_tty_get);
 
 /**
- *	tty_port_tty_set	-	set the tty of a port
- *	@port: tty port
- *	@tty: the tty
+ * tty_port_tty_set - Set the tty of a port.
+ * @port: tty port
+ * @tty: the tty
  *
- *	Associate the port and tty pair. Manages any internal refcounts.
- *	Pass NULL to deassociate a port
+ * Associate the port and tty pair. Manages any internal refcounts.
+ * Pass NULL to deassociate a port
  */
 void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty)
 {
@@ -333,13 +333,13 @@ static void tty_port_shutdown(struct tty_port *port, struct tty_struct *tty)
 }
 
 /**
- *	tty_port_hangup		-	hangup helper
- *	@port: tty port
+ * tty_port_hangup() - Hangup helper.
+ * @port: tty port
  *
- *	Perform port level tty hangup flag and count changes. Drop the tty
- *	reference.
+ * Perform port level tty hangup flag and count changes. Drop the tty
+ * reference.
  *
- *	Caller holds tty lock.
+ * Caller holds tty lock.
  */
 void tty_port_hangup(struct tty_port *port)
 {
@@ -362,8 +362,7 @@ void tty_port_hangup(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_hangup);
 
 /**
- * tty_port_tty_hangup - helper to hang up a tty
- *
+ * tty_port_tty_hangup() - Helper to hang up a tty.
  * @port: tty port
  * @check_clocal: hang only ttys with CLOCAL unset?
  */
@@ -378,8 +377,7 @@ void tty_port_tty_hangup(struct tty_port *port, bool check_clocal)
 EXPORT_SYMBOL_GPL(tty_port_tty_hangup);
 
 /**
- * tty_port_tty_wakeup - helper to wake up a tty
- *
+ * tty_port_tty_wakeup() - Helper to wake up a tty.
  * @port: tty port
  */
 void tty_port_tty_wakeup(struct tty_port *port)
@@ -389,12 +387,12 @@ void tty_port_tty_wakeup(struct tty_port *port)
 EXPORT_SYMBOL_GPL(tty_port_tty_wakeup);
 
 /**
- *	tty_port_carrier_raised	-	carrier raised check
- *	@port: tty port
+ * tty_port_carrier_raised() - Carrier raised check.
+ * @port: tty port
  *
- *	Wrapper for the carrier detect logic. For the moment this is used
- *	to hide some internal details. This will eventually become entirely
- *	internal to the tty port.
+ * Wrapper for the carrier detect logic. For the moment this is used
+ * to hide some internal details. This will eventually become entirely
+ * internal to the tty port.
  */
 int tty_port_carrier_raised(struct tty_port *port)
 {
@@ -405,12 +403,12 @@ int tty_port_carrier_raised(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_carrier_raised);
 
 /**
- *	tty_port_raise_dtr_rts	-	Raise DTR/RTS
- *	@port: tty port
+ * tty_port_raise_dtr_rts() - Raise DTR/RTS.
+ * @port: tty port
  *
- *	Wrapper for the DTR/RTS raise logic. For the moment this is used
- *	to hide some internal details. This will eventually become entirely
- *	internal to the tty port.
+ * Wrapper for the DTR/RTS raise logic. For the moment this is used
+ * to hide some internal details. This will eventually become entirely
+ * internal to the tty port.
  */
 void tty_port_raise_dtr_rts(struct tty_port *port)
 {
@@ -420,12 +418,12 @@ void tty_port_raise_dtr_rts(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_raise_dtr_rts);
 
 /**
- *	tty_port_lower_dtr_rts	-	Lower DTR/RTS
- *	@port: tty port
+ * tty_port_lower_dtr_rts() - Lower DTR/RTS.
+ * @port: tty port
  *
- *	Wrapper for the DTR/RTS raise logic. For the moment this is used
- *	to hide some internal details. This will eventually become entirely
- *	internal to the tty port.
+ * Wrapper for the DTR/RTS raise logic. For the moment this is used
+ * to hide some internal details. This will eventually become entirely
+ * internal to the tty port.
  */
 void tty_port_lower_dtr_rts(struct tty_port *port)
 {
@@ -435,28 +433,28 @@ void tty_port_lower_dtr_rts(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_lower_dtr_rts);
 
 /**
- *	tty_port_block_til_ready	-	Waiting logic for tty open
- *	@port: the tty port being opened
- *	@tty: the tty device being bound
- *	@filp: the file pointer of the opener or NULL
+ * tty_port_block_til_ready() -	Waiting logic for tty open.
+ * @port: the tty port being opened
+ * @tty: the tty device being bound
+ * @filp: the file pointer of the opener or NULL
  *
- *	Implement the core POSIX/SuS tty behaviour when opening a tty device.
- *	Handles:
- *		- hangup (both before and during)
- *		- non blocking open
- *		- rts/dtr/dcd
- *		- signals
- *		- port flags and counts
+ * Implement the core POSIX/SuS tty behaviour when opening a tty device.
+ * Handles:
+ *  - hangup (both before and during)
+ *  - non blocking open
+ *  - rts/dtr/dcd
+ *  - signals
+ *  - port flags and counts
  *
- *	The passed tty_port must implement the carrier_raised method if it can
- *	do carrier detect and the dtr_rts method if it supports software
- *	management of these lines. Note that the dtr/rts raise is done each
- *	iteration as a hangup may have previously dropped them while we wait.
+ * The passed tty_port must implement the carrier_raised method if it can
+ * do carrier detect and the dtr_rts method if it supports software
+ * management of these lines. Note that the dtr/rts raise is done each
+ * iteration as a hangup may have previously dropped them while we wait.
  *
- *	Caller holds tty lock.
+ * Caller holds tty lock.
  *
- *      NB: May drop and reacquire tty lock when blocking, so tty and tty_port
- *      may have changed state (eg., may have been hung up).
+ * NB: May drop and reacquire tty lock when blocking, so tty and tty_port
+ * may have changed state (eg., may have been hung up).
  */
 int tty_port_block_til_ready(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
@@ -623,7 +621,7 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
 EXPORT_SYMBOL(tty_port_close_end);
 
 /**
- * tty_port_close
+ * tty_port_close()
  *
  * Caller holds tty lock
  */
@@ -640,7 +638,7 @@ void tty_port_close(struct tty_port *port, struct tty_struct *tty,
 EXPORT_SYMBOL(tty_port_close);
 
 /**
- * tty_port_install - generic tty->ops->install handler
+ * tty_port_install() - Generic tty->ops->install handler.
  * @port: tty_port of the device
  * @driver: tty_driver for this device
  * @tty: tty to be installed
@@ -658,7 +656,7 @@ int tty_port_install(struct tty_port *port, struct tty_driver *driver,
 EXPORT_SYMBOL_GPL(tty_port_install);
 
 /**
- * tty_port_open
+ * tty_port_open()
  *
  * Caller holds tty lock.
  *
-- 
2.17.1

Re: [PATCH 2/3] tty_port: Fix function name and brief desc

[Johan Hovold]

On Wed, Sep 12, 2018 at 05:50:43PM +1000, Tobin C. Harding wrote:
> File contains kernel-doc function comments that are not in the correct
> style.
> 
>  - Add '()' to function names
>  - Capitalise brief description and append a full stop.

Cleaning up these comments may not be a bad idea, but I have my doubts
about this particular item. Many descriptions aren't full sentences and
still won't be even if you capitalise and slap a full stop there, for
example:

> - *	tty_port_hangup		-	hangup helper
> + * tty_port_hangup() - Hangup helper.
	
There's no requirement to use full sentences, and the old kernel-doc
nano doc uses incomplete ones, as do most (?) kernel-doc comments
throughout the kernel.

I'd just drop this part of the patch. 

>  - Use correct spacing for function name / brief description as per the
>    kernel docs (Documentation/doc-guide/kernel-doc.rst)
> 
> 	/**
> 	 * function_name() - Brief description of function.
> 	 * @arg1: Describe the first argument.
> 
>  - Fix spacing for kernel-doc function comments (no tab space after '*').
> 
> These changes all touch the same lines so do them in a single patch.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Thanks,
Johan

Re: [PATCH 2/3] tty_port: Fix function name and brief desc

[Tobin C. Harding]

On Wed, Sep 12, 2018 at 11:31:16AM +0200, Johan Hovold wrote:
> On Wed, Sep 12, 2018 at 05:50:43PM +1000, Tobin C. Harding wrote:
> > File contains kernel-doc function comments that are not in the correct
> > style.
> > 
> >  - Add '()' to function names
> >  - Capitalise brief description and append a full stop.
> 
> Cleaning up these comments may not be a bad idea, but I have my doubts
> about this particular item. Many descriptions aren't full sentences and
> still won't be even if you capitalise and slap a full stop there, for
> example:
> 
> > - *	tty_port_hangup		-	hangup helper
> > + * tty_port_hangup() - Hangup helper.
> 	
> There's no requirement to use full sentences, and the old kernel-doc
> nano doc uses incomplete ones, as do most (?) kernel-doc comments
> throughout the kernel.
> 
> I'd just drop this part of the patch. 

Ok, sounds good to me.  Thanks for the review.


	Tobin

Re: [PATCH 2/3] tty_port: Fix function name and brief desc

[Greg Kroah-Hartman]

On Wed, Sep 12, 2018 at 05:50:43PM +1000, Tobin C. Harding wrote:
> @@ -235,7 +235,7 @@ void tty_port_free_xmit_buf(struct tty_port *port)
>  EXPORT_SYMBOL(tty_port_free_xmit_buf);
>  
>  /**
> - * tty_port_destroy -- destroy inited port
> + * tty_port_destroy() - Destroy inited port.

Ok, using () for a function name, that's fine, but then you do:

>  /**
> - *	tty_port_tty_set	-	set the tty of a port
> - *	@port: tty port
> - *	@tty: the tty
> + * tty_port_tty_set - Set the tty of a port.

Oops, not here.  Consistency is the key.

thanks,

greg k-h

[PATCH 3/3] tty_port: Fix kernel-docs warnings

[Tobin C. Harding]

Currently extracting the kernel-docs causes a six warnings of type

  warning: Function parameter or member 'foo' not described in 'bar'

Add function parameter descriptions to the function kernel-docs.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/tty/tty_port.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 382282013c9d..2444ed90b93d 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -622,6 +622,9 @@ EXPORT_SYMBOL(tty_port_close_end);
 
 /**
  * tty_port_close()
+ * @port: tty port
+ * @tty: the tty
+ * @filp: the file pointer
  *
  * Caller holds tty lock
  */
@@ -657,6 +660,9 @@ EXPORT_SYMBOL_GPL(tty_port_install);
 
 /**
  * tty_port_open()
+ * @port: tty port
+ * @tty: the tty
+ * @filp: the file pointer
  *
  * Caller holds tty lock.
  *
-- 
2.17.1

Re: [PATCH 3/3] tty_port: Fix kernel-docs warnings

[Greg Kroah-Hartman]

On Wed, Sep 12, 2018 at 05:50:44PM +1000, Tobin C. Harding wrote:
> Currently extracting the kernel-docs causes a six warnings of type
> 
>   warning: Function parameter or member 'foo' not described in 'bar'
> 
> Add function parameter descriptions to the function kernel-docs.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  drivers/tty/tty_port.c | 6 ++++++
>  1 file changed, 6 insertions(+)

As I didn't take patch 2/3, this one doesn't apply :(

thanks,

greg k-h

Re: [PATCH 3/3] tty_port: Fix kernel-docs warnings

[Tobin C. Harding]

On Tue, Sep 18, 2018 at 03:38:49PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 12, 2018 at 05:50:44PM +1000, Tobin C. Harding wrote:
> > Currently extracting the kernel-docs causes a six warnings of type
> > 
> >   warning: Function parameter or member 'foo' not described in 'bar'
> > 
> > Add function parameter descriptions to the function kernel-docs.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >  drivers/tty/tty_port.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> As I didn't take patch 2/3, this one doesn't apply :(

Oh I thought this whole set would have been ignored because of comments
on it.  I've been working on doing these changes for all of drivers/tty/

Thanks for reviewing and taking the first patch.  I'll be sure to
rebase.

thanks,
Tobin.

Re: [PATCH 3/3] tty_port: Fix kernel-docs warnings

[Greg Kroah-Hartman]

On Wed, Sep 19, 2018 at 08:02:46AM +1000, Tobin C. Harding wrote:
> On Tue, Sep 18, 2018 at 03:38:49PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 12, 2018 at 05:50:44PM +1000, Tobin C. Harding wrote:
> > > Currently extracting the kernel-docs causes a six warnings of type
> > > 
> > >   warning: Function parameter or member 'foo' not described in 'bar'
> > > 
> > > Add function parameter descriptions to the function kernel-docs.
> > > 
> > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > ---
> > >  drivers/tty/tty_port.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > 
> > As I didn't take patch 2/3, this one doesn't apply :(
> 
> Oh I thought this whole set would have been ignored because of comments
> on it.

Not at all, kerneldoc cleanups are great to have!

> I've been working on doing these changes for all of drivers/tty/

That's good, it needs it, thanks.

greg k-h