Re: [PATCH] chardev: call tty_init() in real chrdev_init()

[Yu Xu]

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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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