[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Haojian Zhuang]

If uart driver is probed defer, console_setup will be called later
after __init && __initdata sections destroyed. And amba_console isn't
defined in __init or __initdata section. So we needn't define
pl011_console_setup() && pl011_console_get_options() in __init section.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial at vger.kernel.org
---
 drivers/tty/serial/amba-pl011.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 3ea5408..d43d530 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1810,7 +1810,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 	clk_disable(uap->clk);
 }
 
-static void __init
+static void
 pl011_console_get_options(struct uart_amba_port *uap, int *baud,
 			     int *parity, int *bits)
 {
@@ -1845,7 +1845,7 @@ pl011_console_get_options(struct uart_amba_port *uap, int *baud,
 	}
 }
 
-static int __init pl011_console_setup(struct console *co, char *options)
+static int pl011_console_setup(struct console *co, char *options)
 {
 	struct uart_amba_port *uap;
 	int baud = 38400;
-- 
1.7.10.4

[PATCH 2/2] tty: serial: use module_init on pl011_init

[Haojian Zhuang]

If amba serial driver is probed defer, amba serial driver may be
probed after init process. So the error log shows in below.

[    0.389403] Warning: unable to open an initial console.
[    0.390107] Freeing init memory: 2328K

It results in serial console not enabled. So replace arch_initcall
by module_init on pl011_init(). The boot sequence is changed in below.

  pinctrl driver --> amba serial driver --> init process

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial at vger.kernel.org
---
 drivers/tty/serial/amba-pl011.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d43d530..8b7dacb 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2129,11 +2129,7 @@ static void __exit pl011_exit(void)
 	uart_unregister_driver(&amba_reg);
 }
 
-/*
- * While this can be a module, if builtin it's most likely the console
- * So let's leave module_exit but move module_init to an earlier place
- */
-arch_initcall(pl011_init);
+module_init(pl011_init);
 module_exit(pl011_exit);
 
 MODULE_AUTHOR("ARM Ltd/Deep Blue Solutions Ltd");
-- 
1.7.10.4

[PATCH 2/2] tty: serial: use module_init on pl011_init

[Linus Walleij]

Please include Russell on patches to this driver.

On Wed, Feb 6, 2013 at 12:47 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:
> If amba serial driver is probed defer, amba serial driver may be
> probed after init process. So the error log shows in below.
>
> [    0.389403] Warning: unable to open an initial console.
> [    0.390107] Freeing init memory: 2328K
>
> It results in serial console not enabled. So replace arch_initcall
> by module_init on pl011_init(). The boot sequence is changed in below.
>
>   pinctrl driver --> amba serial driver --> init process

ONLY on systems using pinctrl, nota bene.

> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-serial at vger.kernel.org
> ---
>  drivers/tty/serial/amba-pl011.c |    6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index d43d530..8b7dacb 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2129,11 +2129,7 @@ static void __exit pl011_exit(void)
>         uart_unregister_driver(&amba_reg);
>  }
>
> -/*
> - * While this can be a module, if builtin it's most likely the console
> - * So let's leave module_exit but move module_init to an earlier place
> - */
> -arch_initcall(pl011_init);
> +module_init(pl011_init);

Hm. PL011 is actually the only serial driver that gets called as
arch_initcall().

Alessandro Rubini changed this in 2009.

Alessandro, can you explain? Is the sole purpose to get the
console up early?

Yours,
Linus Walleij

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Russell King - ARM Linux]

On Wed, Feb 06, 2013 at 07:47:14PM +0800, Haojian Zhuang wrote:
> If uart driver is probed defer, console_setup will be called later
> after __init && __initdata sections destroyed. And amba_console isn't
> defined in __init or __initdata section. So we needn't define
> pl011_console_setup() && pl011_console_get_options() in __init section.

It sounds like there's a deeper problem here - if this driver being
deferred, why isn't it being retried after the pinctrl stuff gets
its driver registered?

We've had bugs in this deferred probing before, I wouldn't be surprised
if there's more... and we should not be "fixing" drivers because of bugs
elsewhere.

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Haojian Zhuang]

On 6 February 2013 19:52, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Feb 06, 2013 at 07:47:14PM +0800, Haojian Zhuang wrote:
>> If uart driver is probed defer, console_setup will be called later
>> after __init && __initdata sections destroyed. And amba_console isn't
>> defined in __init or __initdata section. So we needn't define
>> pl011_console_setup() && pl011_console_get_options() in __init section.
>
> It sounds like there's a deeper problem here - if this driver being
> deferred, why isn't it being retried after the pinctrl stuff gets
> its driver registered?
>
> We've had bugs in this deferred probing before, I wouldn't be surprised
> if there's more... and we should not be "fixing" drivers because of bugs
> elsewhere.

It retried if driver is being deferred. But those console setup
functions are released
since they're declared in __init section.

rest_init()
   --> creates kernel_init thread that could free __init section memory

deferred_probe_initicall() creates a workqueue that is used to retry
deferred probe
function.

I think that these two threads are competing.

Best Regards
Haojian

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Linus Walleij]

On Wed, Feb 6, 2013 at 1:31 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

CC Grant on this issue...

> On 6 February 2013 19:52, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Feb 06, 2013 at 07:47:14PM +0800, Haojian Zhuang wrote:
>>> If uart driver is probed defer, console_setup will be called later
>>> after __init && __initdata sections destroyed. And amba_console isn't
>>> defined in __init or __initdata section. So we needn't define
>>> pl011_console_setup() && pl011_console_get_options() in __init section.
>>
>> It sounds like there's a deeper problem here - if this driver being
>> deferred, why isn't it being retried after the pinctrl stuff gets
>> its driver registered?
>>
>> We've had bugs in this deferred probing before, I wouldn't be surprised
>> if there's more... and we should not be "fixing" drivers because of bugs
>> elsewhere.
>
> It retried if driver is being deferred. But those console setup
> functions are released
> since they're declared in __init section.
>
> rest_init()
>    --> creates kernel_init thread that could free __init section memory
>
> deferred_probe_initicall() creates a workqueue that is used to retry
> deferred probe
> function.
>
> I think that these two threads are competing.

Now that is the *real* problem. The __init sections surely should
not be discarded until *after* all deferred probes are complete
and the deferral workqueue terminates.

How about writing a patch to fix that instead? (If possible...)

Yours,
Linus Walleij

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Russell King - ARM Linux]

On Wed, Feb 06, 2013 at 05:31:24PM +0100, Linus Walleij wrote:
> On Wed, Feb 6, 2013 at 1:31 PM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
> 
> CC Grant on this issue...
> 
> > On 6 February 2013 19:52, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >> On Wed, Feb 06, 2013 at 07:47:14PM +0800, Haojian Zhuang wrote:
> >>> If uart driver is probed defer, console_setup will be called later
> >>> after __init && __initdata sections destroyed. And amba_console isn't
> >>> defined in __init or __initdata section. So we needn't define
> >>> pl011_console_setup() && pl011_console_get_options() in __init section.
> >>
> >> It sounds like there's a deeper problem here - if this driver being
> >> deferred, why isn't it being retried after the pinctrl stuff gets
> >> its driver registered?
> >>
> >> We've had bugs in this deferred probing before, I wouldn't be surprised
> >> if there's more... and we should not be "fixing" drivers because of bugs
> >> elsewhere.
> >
> > It retried if driver is being deferred. But those console setup
> > functions are released
> > since they're declared in __init section.
> >
> > rest_init()
> >    --> creates kernel_init thread that could free __init section memory
> >
> > deferred_probe_initicall() creates a workqueue that is used to retry
> > deferred probe
> > function.
> >
> > I think that these two threads are competing.
> 
> Now that is the *real* problem. The __init sections surely should
> not be discarded until *after* all deferred probes are complete
> and the deferral workqueue terminates.
> 
> How about writing a patch to fix that instead? (If possible...)

Well, we have the facilities to flush workqueues, so it should be possible.

However, I'm just wondering if this shows that we _do_ need to get rid
of a pile of __init marked functions as well as fixing this, thanks to
the deferred probing we now have (maybe the whole __init thing becomes
useless with deferred probing?)  There's nothing really to guarantee
that the pinctrl stuff will be available by the time the init sections
are discarded (it could be in a loadable module?) or indeed any other
resources that might be necessary.

I think in this regard, deferred probing has opened a similar can of
worms which hotplug devices created (which eventually ended up with
killing the __dev* marking).

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Linus Walleij]

On Wed, Feb 6, 2013 at 5:38 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> However, I'm just wondering if this shows that we _do_ need to get rid
> of a pile of __init marked functions as well as fixing this, thanks to
> the deferred probing we now have (maybe the whole __init thing becomes
> useless with deferred probing?)

And all __initdata and __initconst as well.

> There's nothing really to guarantee
> that the pinctrl stuff will be available by the time the init sections
> are discarded (it could be in a loadable module?) or indeed any other
> resources that might be necessary.

I don't dare to guess how deferred probing is supposed to work
with loadable modules. When I start to think of it my mind
boggles. But I think maybe all drivers that support module loading
are tagging things wrong. Shouldn't they all be using
__init_or_module, __initdata_or_module, __initconst_or_module
so they only get discarded unless compiled as modules?

As far as I can tell from <linux/init.h> that is the intention.

Talk about can of worms :-(

> I think in this regard, deferred probing has opened a similar can of
> worms which hotplug devices created (which eventually ended up with
> killing the __dev* marking).

I'm afraid you're right.

Pinctrl core grabbing of pins seems to trigger the problem more
often on platforms that are using it.

Yours,
Linus Walleij

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Haojian Zhuang]

On 7 February 2013 00:38, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Feb 06, 2013 at 05:31:24PM +0100, Linus Walleij wrote:
>> On Wed, Feb 6, 2013 at 1:31 PM, Haojian Zhuang
>> <haojian.zhuang@linaro.org> wrote:
>>
>> CC Grant on this issue...
>>
>> > On 6 February 2013 19:52, Russell King - ARM Linux
>> > <linux@arm.linux.org.uk> wrote:
>> >> On Wed, Feb 06, 2013 at 07:47:14PM +0800, Haojian Zhuang wrote:
>> >>> If uart driver is probed defer, console_setup will be called later
>> >>> after __init && __initdata sections destroyed. And amba_console isn't
>> >>> defined in __init or __initdata section. So we needn't define
>> >>> pl011_console_setup() && pl011_console_get_options() in __init section.
>> >>
>> >> It sounds like there's a deeper problem here - if this driver being
>> >> deferred, why isn't it being retried after the pinctrl stuff gets
>> >> its driver registered?
>> >>
>> >> We've had bugs in this deferred probing before, I wouldn't be surprised
>> >> if there's more... and we should not be "fixing" drivers because of bugs
>> >> elsewhere.
>> >
>> > It retried if driver is being deferred. But those console setup
>> > functions are released
>> > since they're declared in __init section.
>> >
>> > rest_init()
>> >    --> creates kernel_init thread that could free __init section memory
>> >
>> > deferred_probe_initicall() creates a workqueue that is used to retry
>> > deferred probe
>> > function.
>> >
>> > I think that these two threads are competing.
>>
>> Now that is the *real* problem. The __init sections surely should
>> not be discarded until *after* all deferred probes are complete
>> and the deferral workqueue terminates.
>>
>> How about writing a patch to fix that instead? (If possible...)
>
> Well, we have the facilities to flush workqueues, so it should be possible.
>
> However, I'm just wondering if this shows that we _do_ need to get rid
> of a pile of __init marked functions as well as fixing this, thanks to
> the deferred probing we now have (maybe the whole __init thing becomes
> useless with deferred probing?)  There's nothing really to guarantee
> that the pinctrl stuff will be available by the time the init sections
> are discarded (it could be in a loadable module?) or indeed any other
> resources that might be necessary.
>
> I think in this regard, deferred probing has opened a similar can of
> worms which hotplug devices created (which eventually ended up with
> killing the __dev* marking).

At first, I considered to remove those __init functions. But it may seem
unreasonable.

Now I'm trying to reuse wait_for_device_probe() after do_initcalls(). Please
help to review.

Thanks
Haojian

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Grant Likely]

On Sat, Feb 9, 2013 at 5:22 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:
> On 7 February 2013 00:38, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Feb 06, 2013 at 05:31:24PM +0100, Linus Walleij wrote:
>>> On Wed, Feb 6, 2013 at 1:31 PM, Haojian Zhuang
>>> <haojian.zhuang@linaro.org> wrote:
>>>
>>> CC Grant on this issue...
>>>
>>> > On 6 February 2013 19:52, Russell King - ARM Linux
>>> > rest_init()
>>> >    --> creates kernel_init thread that could free __init section memory
>>> >
>>> > deferred_probe_initicall() creates a workqueue that is used to retry
>>> > deferred probe
>>> > function.
>>> >
>>> > I think that these two threads are competing.
>>>
>>> Now that is the *real* problem. The __init sections surely should
>>> not be discarded until *after* all deferred probes are complete
>>> and the deferral workqueue terminates.
>>>
>>> How about writing a patch to fix that instead? (If possible...)

That just masks the issue. See below.

>>
>> Well, we have the facilities to flush workqueues, so it should be possible.
>>
>> However, I'm just wondering if this shows that we _do_ need to get rid
>> of a pile of __init marked functions as well as fixing this, thanks to
>> the deferred probing we now have (maybe the whole __init thing becomes
>> useless with deferred probing?)  There's nothing really to guarantee
>> that the pinctrl stuff will be available by the time the init sections
>> are discarded (it could be in a loadable module?) or indeed any other
>> resources that might be necessary.

If it is a probe function, then yes the __init annotations need to be
removed or the driver needs to use platform_driver_probe(). This has
been the model as long as I can remember. As long as a driver has a
.probe hook, the driver core can call it at any time. Deferred probe
has only exposed the issue.

If you want to discard the .probe() hook, then the probe() pointer
needs to be cleared from the driver structure before discarding the
sections. Otherwise the driver core can still call it which is what
happens in deferred probe and can happen if a device gets unbound from
a driver and then re-attached.

>> I think in this regard, deferred probing has opened a similar can of
>> worms which hotplug devices created (which eventually ended up with
>> killing the __dev* marking).
>
> At first, I considered to remove those __init functions. But it may seem
> unreasonable.
>
> Now I'm trying to reuse wait_for_device_probe() after do_initcalls(). Please
> help to review.

That patch doesn't actually fix the problem, it just masks it.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Russell King - ARM Linux]

On Sat, Feb 09, 2013 at 10:31:05PM +0000, Grant Likely wrote:
> If it is a probe function, then yes the __init annotations need to be
> removed or the driver needs to use platform_driver_probe(). This has
> been the model as long as I can remember. As long as a driver has a
> .probe hook, the driver core can call it at any time. Deferred probe
> has only exposed the issue.
> 
> If you want to discard the .probe() hook, then the probe() pointer
> needs to be cleared from the driver structure before discarding the
> sections. Otherwise the driver core can still call it which is what
> happens in deferred probe and can happen if a device gets unbound from
> a driver and then re-attached.

You're talking about something completely different on the assumption
that what is being talked about is the probe hook.  It isn't.  It's
a path for the console initialization which has _always_ been __init
marked since the dawn of time, because modules are not supposed to
be calling that path.

What has been exposed is that console drivers which have resources
which are not immediately available no longer have the same guarantees
that they used to have (that is, they will be called before the __init
section is given away.)

Remember: console drivers used not to support removal of themselves.
Many still do not support that - in fact, these do not on the basis
that they call register_console() but not unregister_console():

drivers/net/ethernet/sgi/ioc3-eth.c
drivers/s390/char/con3215.c
drivers/s390/char/con3270.c
drivers/s390/char/sclp_con.c
drivers/s390/char/sclp_vt220.c
drivers/tty/amiserial.c
drivers/tty/bfin_jtag_comm.c
drivers/tty/ehv_bytechan.c
drivers/tty/hvc/hvsi.c
drivers/tty/serial/21285.c
drivers/tty/serial/68328serial.c
drivers/tty/serial/8250/8250.c
drivers/tty/serial/8250/8250_early.c
drivers/tty/serial/altera_jtaguart.c
drivers/tty/serial/altera_uart.c
drivers/tty/serial/apbuart.c
drivers/tty/serial/arc_uart.c
drivers/tty/serial/atmel_serial.c
drivers/tty/serial/bcm63xx_uart.c
drivers/tty/serial/bfin_sport_uart.c
drivers/tty/serial/bfin_uart.c
drivers/tty/serial/cpm_uart/cpm_uart_core.c
drivers/tty/serial/dz.c
drivers/tty/serial/lantiq.c
drivers/tty/serial/lpc32xx_hs.c
drivers/tty/serial/m32r_sio.c
drivers/tty/serial/mcf.c
drivers/tty/serial/mpc52xx_uart.c
drivers/tty/serial/mpsc.c
drivers/tty/serial/netx-serial.c
drivers/tty/serial/nwpserial.c
drivers/tty/serial/pmac_zilog.c
drivers/tty/serial/pnx8xxx_uart.c
drivers/tty/serial/sa1100.c
drivers/tty/serial/samsung.c
drivers/tty/serial/sb1250-duart.c
drivers/tty/serial/serial_core.c
drivers/tty/serial/serial_ks8695.c
drivers/tty/serial/serial_txx9.c
drivers/tty/serial/sh-sci.c
drivers/tty/serial/sirfsoc_uart.c
drivers/tty/serial/uartlite.c
drivers/tty/serial/vr41xx_siu.c
drivers/tty/serial/xilinx_uartps.c
drivers/tty/serial/zs.c
drivers/tty/vt/vt.c

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Haojian Zhuang]

On 10 February 2013 06:41, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Feb 09, 2013 at 10:31:05PM +0000, Grant Likely wrote:
>> If it is a probe function, then yes the __init annotations need to be
>> removed or the driver needs to use platform_driver_probe(). This has
>> been the model as long as I can remember. As long as a driver has a
>> .probe hook, the driver core can call it at any time. Deferred probe
>> has only exposed the issue.
>>
>> If you want to discard the .probe() hook, then the probe() pointer
>> needs to be cleared from the driver structure before discarding the
>> sections. Otherwise the driver core can still call it which is what
>> happens in deferred probe and can happen if a device gets unbound from
>> a driver and then re-attached.
>
> You're talking about something completely different on the assumption
> that what is being talked about is the probe hook.  It isn't.  It's
> a path for the console initialization which has _always_ been __init
> marked since the dawn of time, because modules are not supposed to
> be calling that path.
>
> What has been exposed is that console drivers which have resources
> which are not immediately available no longer have the same guarantees
> that they used to have (that is, they will be called before the __init
> section is given away.)
>
> Remember: console drivers used not to support removal of themselves.
> Many still do not support that - in fact, these do not on the basis
> that they call register_console() but not unregister_console():
>
> drivers/net/ethernet/sgi/ioc3-eth.c
> drivers/s390/char/con3215.c
> drivers/s390/char/con3270.c
> drivers/s390/char/sclp_con.c
> drivers/s390/char/sclp_vt220.c
> drivers/tty/amiserial.c
> drivers/tty/bfin_jtag_comm.c
> drivers/tty/ehv_bytechan.c
> drivers/tty/hvc/hvsi.c
> drivers/tty/serial/21285.c
> drivers/tty/serial/68328serial.c
> drivers/tty/serial/8250/8250.c
> drivers/tty/serial/8250/8250_early.c
> drivers/tty/serial/altera_jtaguart.c
> drivers/tty/serial/altera_uart.c
> drivers/tty/serial/apbuart.c
> drivers/tty/serial/arc_uart.c
> drivers/tty/serial/atmel_serial.c
> drivers/tty/serial/bcm63xx_uart.c
> drivers/tty/serial/bfin_sport_uart.c
> drivers/tty/serial/bfin_uart.c
> drivers/tty/serial/cpm_uart/cpm_uart_core.c
> drivers/tty/serial/dz.c
> drivers/tty/serial/lantiq.c
> drivers/tty/serial/lpc32xx_hs.c
> drivers/tty/serial/m32r_sio.c
> drivers/tty/serial/mcf.c
> drivers/tty/serial/mpc52xx_uart.c
> drivers/tty/serial/mpsc.c
> drivers/tty/serial/netx-serial.c
> drivers/tty/serial/nwpserial.c
> drivers/tty/serial/pmac_zilog.c
> drivers/tty/serial/pnx8xxx_uart.c
> drivers/tty/serial/sa1100.c
> drivers/tty/serial/samsung.c
> drivers/tty/serial/sb1250-duart.c
> drivers/tty/serial/serial_core.c
> drivers/tty/serial/serial_ks8695.c
> drivers/tty/serial/serial_txx9.c
> drivers/tty/serial/sh-sci.c
> drivers/tty/serial/sirfsoc_uart.c
> drivers/tty/serial/uartlite.c
> drivers/tty/serial/vr41xx_siu.c
> drivers/tty/serial/xilinx_uartps.c
> drivers/tty/serial/zs.c
> drivers/tty/vt/vt.c

I agree on Russell. And console isn't the only one device.
In kernel_init_freeable(), we can find that we have an
assumption that all initial calls are already finished, and
we need to discard them & start the user-mode stuff.
But at that time, deferred probe may still not run since
they're in another kernel thread. It's very dangerous that
user mode stuff start running but device driver isn't ready
yet.

Then let's move to my error case. Serial driver fails to
bind pinctrl, and it's moved into deferred probe list. And
kernel_init_freeable() in kernel_init thread keeps working
on CPU, the deferred probe doesn't have chances to be
scheduled yet. So two issues happen.
1. /dev/console is accessed before serial driver is ready.
Since it fails, user can't input anything on console any
more.
2. __init memory section is released before serial driver
is ready. It results in system hang while deferred probe
runs.

Regards
Haojian

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Grant Likely]

On Sun, 10 Feb 2013 11:31:53 +0800, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> I agree on Russell. And console isn't the only one device.
> In kernel_init_freeable(), we can find that we have an
> assumption that all initial calls are already finished, and
> we need to discard them & start the user-mode stuff.
> But at that time, deferred probe may still not run since
> they're in another kernel thread. It's very dangerous that
> user mode stuff start running but device driver isn't ready
> yet.

Linux has been moving in that direction for quite a while now. Userspace
can start before a lot of devices are ready. Mass storage for instance.
An initramfs can start userspace, but if the real rootfs is on rotating
storage, then it will have to wait for it to become ready before
mounting the real root. Distributions ran into this problem quite a
while ago.

That said, console is pretty important, and it isn't good that the
fallout of deferred probe is that console gets initialized even later.

Linus, we should get some engineers around a whiteboard at connect and
map out the initialization order issues. I've been pushing for moving as
much as possible to device_initcall() time, which in general I think is
the right thing to do, but this is the downside.

> Then let's move to my error case. Serial driver fails to
> bind pinctrl, and it's moved into deferred probe list. And
> kernel_init_freeable() in kernel_init thread keeps working
> on CPU, the deferred probe doesn't have chances to be
> scheduled yet. So two issues happen.
> 1. /dev/console is accessed before serial driver is ready.
> Since it fails, user can't input anything on console any
> more.

This is a fair point and is a better argument for me to having a pass
over the deferred drivers before exiting the device initcalls.

> 2. __init memory section is released before serial driver
> is ready. It results in system hang while deferred probe
> runs.

The __init section in a .probe() path is a problem in and of itself. I
do think that the __init annotations either need to be removed or the
hooks into them need to be cleared when the init sections are
discarded.

g.

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Grant Likely]

On Sat, 9 Feb 2013 22:41:38 +0000, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Sat, Feb 09, 2013 at 10:31:05PM +0000, Grant Likely wrote:
> > If it is a probe function, then yes the __init annotations need to be
> > removed or the driver needs to use platform_driver_probe(). This has
> > been the model as long as I can remember. As long as a driver has a
> > .probe hook, the driver core can call it at any time. Deferred probe
> > has only exposed the issue.
> > 
> > If you want to discard the .probe() hook, then the probe() pointer
> > needs to be cleared from the driver structure before discarding the
> > sections. Otherwise the driver core can still call it which is what
> > happens in deferred probe and can happen if a device gets unbound from
> > a driver and then re-attached.
> 
> You're talking about something completely different on the assumption
> that what is being talked about is the probe hook.  It isn't.  It's
> a path for the console initialization which has _always_ been __init
> marked since the dawn of time, because modules are not supposed to
> be calling that path.

Sorry, you're right. Hitting reply too quickly I guess.

However, the point still stands that anything that can be called from
a .probe() path cannot be in an __init section with the current driver
model. A driver can even be unbound from a device and reattached at
runtime. That would also cause problems.

> What has been exposed is that console drivers which have resources
> which are not immediately available no longer have the same guarantees
> that they used to have (that is, they will be called before the __init
> section is given away.)

Okay, I'll reply to Haojian's proposed solution patch on this point.

g.

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Russell King - ARM Linux]

On Wed, Feb 13, 2013 at 09:04:59PM +0000, Grant Likely wrote:
> On Sat, 9 Feb 2013 22:41:38 +0000, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Sat, Feb 09, 2013 at 10:31:05PM +0000, Grant Likely wrote:
> > > If it is a probe function, then yes the __init annotations need to be
> > > removed or the driver needs to use platform_driver_probe(). This has
> > > been the model as long as I can remember. As long as a driver has a
> > > .probe hook, the driver core can call it at any time. Deferred probe
> > > has only exposed the issue.
> > > 
> > > If you want to discard the .probe() hook, then the probe() pointer
> > > needs to be cleared from the driver structure before discarding the
> > > sections. Otherwise the driver core can still call it which is what
> > > happens in deferred probe and can happen if a device gets unbound from
> > > a driver and then re-attached.
> > 
> > You're talking about something completely different on the assumption
> > that what is being talked about is the probe hook.  It isn't.  It's
> > a path for the console initialization which has _always_ been __init
> > marked since the dawn of time, because modules are not supposed to
> > be calling that path.
> 
> Sorry, you're right. Hitting reply too quickly I guess.
> 
> However, the point still stands that anything that can be called from
> a .probe() path cannot be in an __init section with the current driver
> model. A driver can even be unbound from a device and reattached at
> runtime. That would also cause problems.

However, it reveals much bigger problems - that is, if you defer the
probe of the console, what happens when we need to open the console...

Although you can say that, there's _much_ bigger problems here if you
delay console initialization - you'll effectively end up calling init
with no system console in place, which means you'll see no output from
init until the deferred probe sorts itself out.

Yes, it's an unintended side effect of deferred probing, but nevertheless
one which needs to be resolved such that the console _is_ initialized
before it's required, which happens _before_ the init section is
discarded.

[PATCH 1/2] tty: serial: remove __init on pl011 console ops

[Grant Likely]

On Wed, Feb 13, 2013 at 10:52 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Feb 13, 2013 at 09:04:59PM +0000, Grant Likely wrote:
>> On Sat, 9 Feb 2013 22:41:38 +0000, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>> > On Sat, Feb 09, 2013 at 10:31:05PM +0000, Grant Likely wrote:
>> > > If it is a probe function, then yes the __init annotations need to be
>> > > removed or the driver needs to use platform_driver_probe(). This has
>> > > been the model as long as I can remember. As long as a driver has a
>> > > .probe hook, the driver core can call it at any time. Deferred probe
>> > > has only exposed the issue.
>> > >
>> > > If you want to discard the .probe() hook, then the probe() pointer
>> > > needs to be cleared from the driver structure before discarding the
>> > > sections. Otherwise the driver core can still call it which is what
>> > > happens in deferred probe and can happen if a device gets unbound from
>> > > a driver and then re-attached.
>> >
>> > You're talking about something completely different on the assumption
>> > that what is being talked about is the probe hook.  It isn't.  It's
>> > a path for the console initialization which has _always_ been __init
>> > marked since the dawn of time, because modules are not supposed to
>> > be calling that path.
>>
>> Sorry, you're right. Hitting reply too quickly I guess.
>>
>> However, the point still stands that anything that can be called from
>> a .probe() path cannot be in an __init section with the current driver
>> model. A driver can even be unbound from a device and reattached at
>> runtime. That would also cause problems.
>
> However, it reveals much bigger problems - that is, if you defer the
> probe of the console, what happens when we need to open the console...
>
> Although you can say that, there's _much_ bigger problems here if you
> delay console initialization - you'll effectively end up calling init
> with no system console in place, which means you'll see no output from
> init until the deferred probe sorts itself out.
>
> Yes, it's an unintended side effect of deferred probing, but nevertheless
> one which needs to be resolved such that the console _is_ initialized
> before it's required, which happens _before_ the init section is
> discarded.

I've replied with a suggested solution to Haojian's patch to driver
core. If he can test it and post a proper patch, then I'll ack it.

g.



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.