[PATCH 0/2] tty: hvc: Convert to platform remove callback returning void

[PATCH 0/2] tty: hvc: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

The hvc_opal driver had an error path in its remove function resulting
in returning a non-zero value. This is a bad thing because the core
doesn't do error handling and effectively ignores the return value.

In this case it's not as bad as it sounds though, as this error path is
never taken.

After the first patch changes hvc_remove() to return void the remove
function obviously always returns zero. Then it can be trivially
converted to .remove_new(). See commit 5c5a7680e67b ("platform: Provide
a remove callback that returns no value") for an extended explanation
and the eventual goal of this conversion.

Uwe Kleine-König (2):
  tty: hvc: Make hvc_remove() return no value
  tty: hvc: hvc_opal: Convert to platform remove callback returning void

 drivers/tty/hvc/hvc_console.c |  3 +--
 drivers/tty/hvc/hvc_console.h |  2 +-
 drivers/tty/hvc/hvc_opal.c    | 17 +++++++----------
 3 files changed, 9 insertions(+), 13 deletions(-)

base-commit: e27090b1413ff236ca1aec26d6b022149115de2c
-- 
2.42.0

[PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Uwe Kleine-König]

The function hvc_remove() returns zero unconditionally. Make it return
void instead to make it obvious that the caller doesn't need to do any
error handling. Accordingly drop the error handling from
hvc_opal_remove().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/hvc/hvc_console.c |  3 +--
 drivers/tty/hvc/hvc_console.h |  2 +-
 drivers/tty/hvc/hvc_opal.c    | 15 +++++++--------
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 959fae54ca39..57f5c37125e6 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -976,7 +976,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 }
 EXPORT_SYMBOL_GPL(hvc_alloc);
 
-int hvc_remove(struct hvc_struct *hp)
+void hvc_remove(struct hvc_struct *hp)
 {
 	unsigned long flags;
 	struct tty_struct *tty;
@@ -1010,7 +1010,6 @@ int hvc_remove(struct hvc_struct *hp)
 		tty_vhangup(tty);
 		tty_kref_put(tty);
 	}
-	return 0;
 }
 EXPORT_SYMBOL_GPL(hvc_remove);
 
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 9668f821db01..78f7543511f1 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -77,7 +77,7 @@ extern int hvc_instantiate(uint32_t vtermno, int index,
 extern struct hvc_struct * hvc_alloc(uint32_t vtermno, int data,
 				     const struct hv_ops *ops, int outbuf_size);
 /* remove a vterm from hvc tty operation (module_exit or hotplug remove) */
-extern int hvc_remove(struct hvc_struct *hp);
+extern void hvc_remove(struct hvc_struct *hp);
 
 /* data available */
 int hvc_poll(struct hvc_struct *hp);
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 992e199e0ea8..8995b253cf90 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -235,16 +235,15 @@ static int hvc_opal_probe(struct platform_device *dev)
 static int hvc_opal_remove(struct platform_device *dev)
 {
 	struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
-	int rc, termno;
+	int termno;
 
 	termno = hp->vtermno;
-	rc = hvc_remove(hp);
-	if (rc == 0) {
-		if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
-			kfree(hvc_opal_privs[termno]);
-		hvc_opal_privs[termno] = NULL;
-	}
-	return rc;
+	hvc_remove(hp);
+	if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
+		kfree(hvc_opal_privs[termno]);
+	hvc_opal_privs[termno] = NULL;
+
+	return 0;
 }
 
 static struct platform_driver hvc_opal_driver = {
-- 
2.42.0

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Nicholas Piggin]

On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> The function hvc_remove() returns zero unconditionally. Make it return
> void instead to make it obvious that the caller doesn't need to do any
> error handling. Accordingly drop the error handling from
> hvc_opal_remove().
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

IIUC these are functionally no change, just tidying and removing
dead code? Unless I'm mistaken, then

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  drivers/tty/hvc/hvc_console.c |  3 +--
>  drivers/tty/hvc/hvc_console.h |  2 +-
>  drivers/tty/hvc/hvc_opal.c    | 15 +++++++--------
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 959fae54ca39..57f5c37125e6 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -976,7 +976,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  }
>  EXPORT_SYMBOL_GPL(hvc_alloc);
>  
> -int hvc_remove(struct hvc_struct *hp)
> +void hvc_remove(struct hvc_struct *hp)
>  {
>  	unsigned long flags;
>  	struct tty_struct *tty;
> @@ -1010,7 +1010,6 @@ int hvc_remove(struct hvc_struct *hp)
>  		tty_vhangup(tty);
>  		tty_kref_put(tty);
>  	}
> -	return 0;
>  }
>  EXPORT_SYMBOL_GPL(hvc_remove);
>  
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index 9668f821db01..78f7543511f1 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -77,7 +77,7 @@ extern int hvc_instantiate(uint32_t vtermno, int index,
>  extern struct hvc_struct * hvc_alloc(uint32_t vtermno, int data,
>  				     const struct hv_ops *ops, int outbuf_size);
>  /* remove a vterm from hvc tty operation (module_exit or hotplug remove) */
> -extern int hvc_remove(struct hvc_struct *hp);
> +extern void hvc_remove(struct hvc_struct *hp);
>  
>  /* data available */
>  int hvc_poll(struct hvc_struct *hp);
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 992e199e0ea8..8995b253cf90 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -235,16 +235,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>  static int hvc_opal_remove(struct platform_device *dev)
>  {
>  	struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
> -	int rc, termno;
> +	int termno;
>  
>  	termno = hp->vtermno;
> -	rc = hvc_remove(hp);
> -	if (rc == 0) {
> -		if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
> -			kfree(hvc_opal_privs[termno]);
> -		hvc_opal_privs[termno] = NULL;
> -	}
> -	return rc;
> +	hvc_remove(hp);
> +	if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
> +		kfree(hvc_opal_privs[termno]);
> +	hvc_opal_privs[termno] = NULL;
> +
> +	return 0;
>  }
>  
>  static struct platform_driver hvc_opal_driver = {

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On Mon, Nov 13, 2023 at 07:45:27PM +1000, Nicholas Piggin wrote:
> On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> > The function hvc_remove() returns zero unconditionally. Make it return
> > void instead to make it obvious that the caller doesn't need to do any
> > error handling. Accordingly drop the error handling from
> > hvc_opal_remove().
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> IIUC these are functionally no change, just tidying and removing
> dead code?

In case this isn't only a rethorical question: There is indeed no
change in behaviour. hvc_remove() returned always zero, so

	rc = hvc_remove(hp);
	if (rc == 0) {
		... some code not changing rc ...
	}
	... some more code not changing rc ...
	return rc

can be simplified to

	hvc_remove(hp);
	... some code not changing rc ...
	... some more code not changing rc ...
	return 0;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] tty: hvc: Make hvc_remove() return no value

[Nicholas Piggin]

On Mon Nov 13, 2023 at 7:57 PM AEST, Uwe Kleine-König wrote:
> On Mon, Nov 13, 2023 at 07:45:27PM +1000, Nicholas Piggin wrote:
> > On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> > > The function hvc_remove() returns zero unconditionally. Make it return
> > > void instead to make it obvious that the caller doesn't need to do any
> > > error handling. Accordingly drop the error handling from
> > > hvc_opal_remove().
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > IIUC these are functionally no change, just tidying and removing
> > dead code?
>
> In case this isn't only a rethorical question: There is indeed no
> change in behaviour.

Thanks, it wasn't. Your changelog and code seemed to be quite clear,
I just wanted to confirm I didn't misread or misunderstand it.

Thanks,
Nick

> hvc_remove() returned always zero, so
>
> 	rc = hvc_remove(hp);
> 	if (rc == 0) {
> 		... some code not changing rc ...
> 	}
> 	... some more code not changing rc ...
> 	return rc
>
> can be simplified to
>
> 	hvc_remove(hp);
> 	... some code not changing rc ...
> 	... some more code not changing rc ...
> 	return 0;
>
> Best regards
> Uwe

[PATCH 2/2] tty: hvc: hvc_opal: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/hvc/hvc_opal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 8995b253cf90..2cdf66e395cc 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -232,7 +232,7 @@ static int hvc_opal_probe(struct platform_device *dev)
 	return 0;
 }
 
-static int hvc_opal_remove(struct platform_device *dev)
+static void hvc_opal_remove(struct platform_device *dev)
 {
 	struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
 	int termno;
@@ -242,13 +242,11 @@ static int hvc_opal_remove(struct platform_device *dev)
 	if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
 		kfree(hvc_opal_privs[termno]);
 	hvc_opal_privs[termno] = NULL;
-
-	return 0;
 }
 
 static struct platform_driver hvc_opal_driver = {
 	.probe		= hvc_opal_probe,
-	.remove		= hvc_opal_remove,
+	.remove_new	= hvc_opal_remove,
 	.driver		= {
 		.name	= hvc_opal_name,
 		.of_match_table	= hvc_opal_match,
-- 
2.42.0

Re: [PATCH 2/2] tty: hvc: hvc_opal: Convert to platform remove callback returning void

[Nicholas Piggin]

On Mon Nov 6, 2023 at 7:44 AM AEST, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
>
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/hvc/hvc_opal.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 8995b253cf90..2cdf66e395cc 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -232,7 +232,7 @@ static int hvc_opal_probe(struct platform_device *dev)
>  	return 0;
>  }
>  
> -static int hvc_opal_remove(struct platform_device *dev)
> +static void hvc_opal_remove(struct platform_device *dev)
>  {
>  	struct hvc_struct *hp = dev_get_drvdata(&dev->dev);
>  	int termno;
> @@ -242,13 +242,11 @@ static int hvc_opal_remove(struct platform_device *dev)
>  	if (hvc_opal_privs[termno] != &hvc_opal_boot_priv)
>  		kfree(hvc_opal_privs[termno]);
>  	hvc_opal_privs[termno] = NULL;
> -
> -	return 0;
>  }
>  
>  static struct platform_driver hvc_opal_driver = {
>  	.probe		= hvc_opal_probe,
> -	.remove		= hvc_opal_remove,
> +	.remove_new	= hvc_opal_remove,
>  	.driver		= {
>  		.name	= hvc_opal_name,
>  		.of_match_table	= hvc_opal_match,