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;
Message-ID: <e4eb9000-e246-c01b-abde-de1535ff0374@linux.alibaba.com>
Date:   Mon, 7 Feb 2022 14:37:46 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.5.1
Subject: Re: [PATCH] chardev: call tty_init() in real chrdev_init()
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        arnd@arndb.de, viro@zeniv.linux.org.uk, dhowells@redhat.com
References: <4e753e51d0516413fbf557cf861d654ca73486cc.1644164597.git.xuyu@linux.alibaba.com>
 <Yf//U1s3DbTuSqo2@kroah.com>
From:   Yu Xu <xuyu@linux.alibaba.com>
In-Reply-To: <Yf//U1s3DbTuSqo2@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/7/22 1:03 AM, Greg KH wrote:
> 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?

To be honest, not 100% sure.

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

I tried to reproduce the issue described in the original commit, and
failed. The issue does not appear, or my reproduction is wrong.
   1. revert 31d1d48e199e manually;
   2. request_module("xxx") anywhere before do_initcalls(), since
      tty_init() now is initialized by module_init();
   3. no warning on request_module is shown.

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

No real problem actually. As described in the log, it is confusing that
tty_init() in called in the initialization of memdev. They don't have
strong dependencies. I found the issue when I read through codes of
drivers/char/mem.c.

> 
> 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.

Got it. But I will first try to figure out whether this patch is a real
issue.

> 
> thanks,
> 
> greg k-h

-- 
Thanks,
Yu