[PATCH 20/29] tty: srmcons: fix retval from srmcons_init()

Jiri Slaby (SUSE) posted 29 patches 10 months ago
There is a newer version of this series
[PATCH 20/29] tty: srmcons: fix retval from srmcons_init()
Posted by Jiri Slaby (SUSE) 10 months ago
The value returned from srmcons_init() was -ENODEV for over 2 decades.
But it does not matter, given device_initcall() ignores retvals.

But to be honest, return 0 in case the tty driver was registered
properly.

To do that, the condition is inverted and a short path taken in case of
error.

err_free_drv is introduced as it will be used from more places later.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 62 ++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index 3e61073f4b30..b9cd364e814e 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -196,40 +196,44 @@ static const struct tty_operations srmcons_ops = {
 static int __init
 srmcons_init(void)
 {
+	struct tty_driver *driver;
+	int err;
+
 	timer_setup(&srmcons_singleton.timer, srmcons_receive_chars, 0);
-	if (srm_is_registered_console) {
-		struct tty_driver *driver;
-		int err;
-
-		driver = tty_alloc_driver(MAX_SRM_CONSOLE_DEVICES, 0);
-		if (IS_ERR(driver))
-			return PTR_ERR(driver);
-
-		tty_port_init(&srmcons_singleton.port);
-
-		driver->driver_name = "srm";
-		driver->name = "srm";
-		driver->major = 0; 	/* dynamic */
-		driver->minor_start = 0;
-		driver->type = TTY_DRIVER_TYPE_SYSTEM;
-		driver->subtype = SYSTEM_TYPE_SYSCONS;
-		driver->init_termios = tty_std_termios;
-		tty_set_operations(driver, &srmcons_ops);
-		tty_port_link_device(&srmcons_singleton.port, driver, 0);
-		err = tty_register_driver(driver);
-		if (err) {
-			tty_driver_kref_put(driver);
-			tty_port_destroy(&srmcons_singleton.port);
-			return err;
-		}
-		srmcons_driver = driver;
-	}
 
-	return -ENODEV;
+	if (!srm_is_registered_console)
+		return -ENODEV;
+
+	driver = tty_alloc_driver(MAX_SRM_CONSOLE_DEVICES, 0);
+	if (IS_ERR(driver))
+		return PTR_ERR(driver);
+
+	tty_port_init(&srmcons_singleton.port);
+
+	driver->driver_name = "srm";
+	driver->name = "srm";
+	driver->major = 0;	/* dynamic */
+	driver->minor_start = 0;
+	driver->type = TTY_DRIVER_TYPE_SYSTEM;
+	driver->subtype = SYSTEM_TYPE_SYSCONS;
+	driver->init_termios = tty_std_termios;
+	tty_set_operations(driver, &srmcons_ops);
+	tty_port_link_device(&srmcons_singleton.port, driver, 0);
+	err = tty_register_driver(driver);
+	if (err)
+		goto err_free_drv;
+
+	srmcons_driver = driver;
+
+	return 0;
+err_free_drv:
+	tty_driver_kref_put(driver);
+	tty_port_destroy(&srmcons_singleton.port);
+
+	return err;
 }
 device_initcall(srmcons_init);
 
-
 /*
  * The console driver
  */
-- 
2.48.1
Re: [PATCH 20/29] tty: srmcons: fix retval from srmcons_init()
Posted by Magnus Lindholm 10 months ago
I've applied and verified this patch on an Alphaserver ES40 with
serial console.

Regarding the future use of label err_free_drv, is the intention to
use it to break out early if tty_alloc_driver() fails?


Tested-by: Magnus Lindholm <linmag7@gmail.com>




On Thu, Feb 20, 2025 at 12:22 PM Jiri Slaby (SUSE) <jirislaby@kernel.org> wrote:
>
> The value returned from srmcons_init() was -ENODEV for over 2 decades.
> But it does not matter, given device_initcall() ignores retvals.
>
> But to be honest, return 0 in case the tty driver was registered
> properly.
>
> To do that, the condition is inverted and a short path taken in case of
> error.
>
> err_free_drv is introduced as it will be used from more places later.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>  arch/alpha/kernel/srmcons.c | 62 ++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index 3e61073f4b30..b9cd364e814e 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -196,40 +196,44 @@ static const struct tty_operations srmcons_ops = {
>  static int __init
>  srmcons_init(void)
>  {
> +       struct tty_driver *driver;
> +       int err;
> +
>         timer_setup(&srmcons_singleton.timer, srmcons_receive_chars, 0);
> -       if (srm_is_registered_console) {
> -               struct tty_driver *driver;
> -               int err;
> -
> -               driver = tty_alloc_driver(MAX_SRM_CONSOLE_DEVICES, 0);
> -               if (IS_ERR(driver))
> -                       return PTR_ERR(driver);
> -
> -               tty_port_init(&srmcons_singleton.port);
> -
> -               driver->driver_name = "srm";
> -               driver->name = "srm";
> -               driver->major = 0;      /* dynamic */
> -               driver->minor_start = 0;
> -               driver->type = TTY_DRIVER_TYPE_SYSTEM;
> -               driver->subtype = SYSTEM_TYPE_SYSCONS;
> -               driver->init_termios = tty_std_termios;
> -               tty_set_operations(driver, &srmcons_ops);
> -               tty_port_link_device(&srmcons_singleton.port, driver, 0);
> -               err = tty_register_driver(driver);
> -               if (err) {
> -                       tty_driver_kref_put(driver);
> -                       tty_port_destroy(&srmcons_singleton.port);
> -                       return err;
> -               }
> -               srmcons_driver = driver;
> -       }
>
> -       return -ENODEV;
> +       if (!srm_is_registered_console)
> +               return -ENODEV;
> +
> +       driver = tty_alloc_driver(MAX_SRM_CONSOLE_DEVICES, 0);
> +       if (IS_ERR(driver))
> +               return PTR_ERR(driver);
> +
> +       tty_port_init(&srmcons_singleton.port);
> +
> +       driver->driver_name = "srm";
> +       driver->name = "srm";
> +       driver->major = 0;      /* dynamic */
> +       driver->minor_start = 0;
> +       driver->type = TTY_DRIVER_TYPE_SYSTEM;
> +       driver->subtype = SYSTEM_TYPE_SYSCONS;
> +       driver->init_termios = tty_std_termios;
> +       tty_set_operations(driver, &srmcons_ops);
> +       tty_port_link_device(&srmcons_singleton.port, driver, 0);
> +       err = tty_register_driver(driver);
> +       if (err)
> +               goto err_free_drv;
> +
> +       srmcons_driver = driver;
> +
> +       return 0;
> +err_free_drv:
> +       tty_driver_kref_put(driver);
> +       tty_port_destroy(&srmcons_singleton.port);
> +
> +       return err;
>  }
>  device_initcall(srmcons_init);
>
> -
>  /*
>   * The console driver
>   */
> --
> 2.48.1
>
>
Re: [PATCH 20/29] tty: srmcons: fix retval from srmcons_init()
Posted by Jiri Slaby 10 months ago
On 20. 02. 25, 22:48, Magnus Lindholm wrote:
> I've applied and verified this patch on an Alphaserver ES40 with
> serial console.
> 
> Regarding the future use of label err_free_drv, is the intention to
> use it to break out early if tty_alloc_driver() fails?

...

>> +       if (!srm_is_registered_console)
>> +               return -ENODEV;
>> +
>> +       driver = tty_alloc_driver(MAX_SRM_CONSOLE_DEVICES, 0);
>> +       if (IS_ERR(driver))
>> +               return PTR_ERR(driver);
>> +
>> +       tty_port_init(&srmcons_singleton.port);
>> +
>> +       driver->driver_name = "srm";
>> +       driver->name = "srm";
>> +       driver->major = 0;      /* dynamic */
>> +       driver->minor_start = 0;
>> +       driver->type = TTY_DRIVER_TYPE_SYSTEM;
>> +       driver->subtype = SYSTEM_TYPE_SYSCONS;
>> +       driver->init_termios = tty_std_termios;
>> +       tty_set_operations(driver, &srmcons_ops);
>> +       tty_port_link_device(&srmcons_singleton.port, driver, 0);

I plan on removing tty_port_link_device() as it was a temporary aid. 
Yay, for 13 years! (commit 2cb4ca0208722).

>> +       err = tty_register_driver(driver);
>> +       if (err)
>> +               goto err_free_drv;

Instead, the idea is to properly tty_port_register_device() after 
tty_register_driver() instead. And that can fail. So is fail path reuses 
err_free_drv (and adds tty_unregister_driver() on top).

>> +
>> +       srmcons_driver = driver;
>> +
>> +       return 0;
>> +err_free_drv:
>> +       tty_driver_kref_put(driver);
>> +       tty_port_destroy(&srmcons_singleton.port);
>> +
>> +       return err;

thanks,
-- 
js
suse labs