Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 6 Feb 2022 18:03:15 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Xu Yu <xuyu@linux.alibaba.com>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        arnd@arndb.de, viro@zeniv.linux.org.uk, dhowells@redhat.com
Subject: Re: [PATCH] chardev: call tty_init() in real chrdev_init()
Message-ID: <Yf//U1s3DbTuSqo2@kroah.com>
References: <4e753e51d0516413fbf557cf861d654ca73486cc.1644164597.git.xuyu@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4e753e51d0516413fbf557cf861d654ca73486cc.1644164597.git.xuyu@linux.alibaba.com>
Precedence: bulk

On Mon, Feb 07, 2022 at 12:27:31AM +0800, Xu Yu wrote:
> It is confusing that tty_init() in called in the initialization of
> memdev, i.e., static chr_dev_init().
> 
> Through blame, it is introduced by commit 31d1d48e199e ("Fix init
> ordering of /dev/console vs callers of modprobe"), which fixes the
> initialization order of /dev/console driver. However, there seems
> to be a typo in the patch, i.e., chrdev_init, instead of chr_dev_init.
> 
> This fixes the typo, IIUC.
> 
> Note that the return value of tty_init() is always 0, and thus no error
> handling is provided in chrdev_init().
> 
> Fixes: 31d1d48e199e ("Fix init ordering of /dev/console vs callers of modprobe")
> Signed-off-by: Xu Yu <xuyu@linux.alibaba.com>
> ---
>  drivers/char/mem.c | 2 +-
>  fs/char_dev.c      | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index cc296f0823bd..8c90881f8115 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -775,7 +775,7 @@ static int __init chr_dev_init(void)
>  			      NULL, devlist[minor].name);
>  	}
>  
> -	return tty_init();
> +	return 0;
>  }
>  
>  fs_initcall(chr_dev_init);
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index ba0ded7842a7..fc042a0a098f 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -667,6 +667,7 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
>  void __init chrdev_init(void)
>  {
>  	cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
> +	tty_init();
>  }
>  

You just changed the ordering sequence here, are you SURE this is
correct?

How was this tested?  Did you verify that the problem that the original
commit here was fixing is now not happening again?

And what real problem is this solving?  How did you hit the issue that
this solves?

And finally, yes, it is not good to throw away the return value of
tty_init().  If it really can not return anything but 0, then let us
make it a void function first.

thanks,

greg k-h