All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>,
	Grant Likely <grant.likely@secretlab.ca>,
	jslaby@suse.cz, anton.vorontsov@linaro.org,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alan Cox <alan@linux.intel.com>,
	linux-serial@vger.kernel.org, Patch Tracking <patches@linaro.org>
Subject: Re: [PATCH 1/2] tty: serial: remove __init on pl011 console ops
Date: Wed, 6 Feb 2013 16:38:11 +0000	[thread overview]
Message-ID: <20130206163811.GI17833@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <CACRpkdZX-YF+jzg453Vmfxo_2LLuASvZyQqLommaM4Xd3LpwfQ@mail.gmail.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] tty: serial: remove __init on pl011 console ops
Date: Wed, 6 Feb 2013 16:38:11 +0000	[thread overview]
Message-ID: <20130206163811.GI17833@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <CACRpkdZX-YF+jzg453Vmfxo_2LLuASvZyQqLommaM4Xd3LpwfQ@mail.gmail.com>

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

  reply	other threads:[~2013-02-06 16:39 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-06 11:47 [PATCH 1/2] tty: serial: remove __init on pl011 console ops Haojian Zhuang
2013-02-06 11:47 ` Haojian Zhuang
2013-02-06 11:47 ` [PATCH 2/2] tty: serial: use module_init on pl011_init Haojian Zhuang
2013-02-06 11:47   ` Haojian Zhuang
2013-02-06 17:17   ` Linus Walleij
2013-02-06 17:17     ` Linus Walleij
2013-02-06 11:52 ` [PATCH 1/2] tty: serial: remove __init on pl011 console ops Russell King - ARM Linux
2013-02-06 11:52   ` Russell King - ARM Linux
2013-02-06 12:31   ` Haojian Zhuang
2013-02-06 12:31     ` Haojian Zhuang
2013-02-06 16:31     ` Linus Walleij
2013-02-06 16:31       ` Linus Walleij
2013-02-06 16:38       ` Russell King - ARM Linux [this message]
2013-02-06 16:38         ` Russell King - ARM Linux
2013-02-06 16:46         ` Linus Walleij
2013-02-06 16:46           ` Linus Walleij
2013-02-09 17:22         ` Haojian Zhuang
2013-02-09 17:22           ` Haojian Zhuang
2013-02-09 22:31           ` Grant Likely
2013-02-09 22:31             ` Grant Likely
2013-02-09 22:41             ` Russell King - ARM Linux
2013-02-09 22:41               ` Russell King - ARM Linux
2013-02-10  3:31               ` Haojian Zhuang
2013-02-10  3:31                 ` Haojian Zhuang
2013-02-13 21:21                 ` Grant Likely
2013-02-13 21:21                   ` Grant Likely
2013-02-13 21:04               ` Grant Likely
2013-02-13 21:04                 ` Grant Likely
2013-02-13 22:52                 ` Russell King - ARM Linux
2013-02-13 22:52                   ` Russell King - ARM Linux
2013-02-13 23:04                   ` Grant Likely
2013-02-13 23:04                     ` Grant Likely

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130206163811.GI17833@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=alan@linux.intel.com \
    --cc=anton.vorontsov@linaro.org \
    --cc=grant.likely@secretlab.ca \
    --cc=gregkh@linuxfoundation.org \
    --cc=haojian.zhuang@linaro.org \
    --cc=jslaby@suse.cz \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=patches@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.