| Message ID | 1376987548-12366-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: > Instead of use platform_driver_probe, use module_platform_driver > To support deferred probing > Also subsys_initcall may too early to auto set pinctl > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Acked-by: Baruch Siach <baruch@tkos.co.il> This patch is tougher than it looks. You need it, because subsys_initcall may be too early for pinctrl. Other people might be depending on subsys_initcall to get I2C active before they want to activate, say, PMICs. So, I fear regressions, since deferred probing might not be available in the needed places to avoid these regressions. I am all ears for a nice transition away from subsys_initcall, anyone? > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 4c5fada..36ceebc 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -236,6 +236,7 @@ static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume); > MODULE_ALIAS("platform:i2c_designware"); > > static struct platform_driver dw_i2c_driver = { > + .probe = dw_i2c_probe, > .remove = dw_i2c_remove, > .driver = { > .name = "i2c_designware", > @@ -245,18 +246,7 @@ static struct platform_driver dw_i2c_driver = { > .pm = &dw_i2c_dev_pm_ops, > }, > }; > - > -static int __init dw_i2c_init_driver(void) > -{ > - return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe); > -} > -subsys_initcall(dw_i2c_init_driver); > - > -static void __exit dw_i2c_exit_driver(void) > -{ > - platform_driver_unregister(&dw_i2c_driver); > -} > -module_exit(dw_i2c_exit_driver); > +module_platform_driver(dw_i2c_driver); > > MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>"); > MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter"); > -- > 1.7.9.5 >
On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: >> Instead of use platform_driver_probe, use module_platform_driver >> To support deferred probing >> Also subsys_initcall may too early to auto set pinctl >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Acked-by: Baruch Siach <baruch@tkos.co.il> > > This patch is tougher than it looks. You need it, because > subsys_initcall may be too early for pinctrl. pinctrl is initialized very early, core_initcall(). This is more a question of individual pin control drivers and when they probe, and dependencies trying to take a pinctrl handle before the pin controller is available will be deferred. Even by those grabbed in the core by drivers/base/pinctrl.c. Yours, Linus Walleij
On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: > >>> Instead of use platform_driver_probe, use module_platform_driver >>> To support deferred probing >>> Also subsys_initcall may too early to auto set pinctl >>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>> Acked-by: Baruch Siach <baruch@tkos.co.il> >> >> This patch is tougher than it looks. You need it, because >> subsys_initcall may be too early for pinctrl. > > pinctrl is initialized very early, core_initcall(). > > This is more a question of individual pin control drivers > and when they probe, and dependencies trying to take > a pinctrl handle before the pin controller is available > will be deferred. Even by those grabbed in the core > by drivers/base/pinctrl.c. Thanks Linus. Your explanation is really make sense. We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for pinctrl-single, no issue at all. Checked in 3.11-rc4, there is really deferring probe happen. i2c_designware fcb08000.i2c: could not find pctldev for node /amba/pinmux@fc803000/i2c0 _pmx_func, deferring probe However, bus_probe_device failed, since the drv name list does not have i2c_designware. deferred_probe_work_func -> bus_probe_device -> device_attach -> bus_for_each_drv -> __device_attach It can be solved change return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe); to return platform_driver_register(&dw_i2c_driver); static struct platform_driver dw_i2c_driver = { .probe = dw_i2c_probe, ~ Dear Wolfram Thanks for telling me the dependency about subsys_initcall. Should I resubmit one patch using platform_driver_register while keeping subsys_initcall? Besides, also find platform_driver_probe is used in drivers/i2c/busses/i2c-imx.c and drivers/i2c/busses/i2c-stu300.c. Thanks
On Thu, Aug 29, 2013 at 12:55 PM, zhangfei gao <zhangfei.gao@gmail.com> wrote: > Besides, also find platform_driver_probe is used in > drivers/i2c/busses/i2c-imx.c and drivers/i2c/busses/i2c-stu300.c. The platform_driver_probe() is basically a footprint optimization (more code can be discarded after boot) and I'm happy to patch it if it disturbs anything, it is *really* not important for this driver. Do you guys need a low footprint? Else there is no use to have platform_driver_probe() in there. Yours, Linus Walleij
On 13-08-28 05:57 PM, Wolfram Sang wrote: > On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: >> Instead of use platform_driver_probe, use module_platform_driver >> To support deferred probing >> Also subsys_initcall may too early to auto set pinctl >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Acked-by: Baruch Siach <baruch@tkos.co.il> > > This patch is tougher than it looks. You need it, because > subsys_initcall may be too early for pinctrl. Other people might be > depending on subsys_initcall to get I2C active before they want to > activate, say, PMICs. So, I fear regressions, since deferred probing > might not be available in the needed places to avoid these regressions. > I am all ears for a nice transition away from subsys_initcall, anyone? Dear Wolfram, The deferred probe will be successful if change platform_driver_probe to platform_driver_register, to register itself to drv name list. However, the effect is subsys_initcall is postponed, until pin control driver is probed successfully. If the pin is specific and does not need configure, there is no issue. There may still have issue if the mulit-function pin need be configured via pinctrl, except put pin control driver earlier too. Thanks
* zhangfei gao <zhangfei.gao@gmail.com> [130829 04:03]: > On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: > > > >>> Instead of use platform_driver_probe, use module_platform_driver > >>> To support deferred probing > >>> Also subsys_initcall may too early to auto set pinctl > >>> > >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > >>> Acked-by: Baruch Siach <baruch@tkos.co.il> > >> > >> This patch is tougher than it looks. You need it, because > >> subsys_initcall may be too early for pinctrl. > > > > pinctrl is initialized very early, core_initcall(). > > > > This is more a question of individual pin control drivers > > and when they probe, and dependencies trying to take > > a pinctrl handle before the pin controller is available > > will be deferred. Even by those grabbed in the core > > by drivers/base/pinctrl.c. > > Thanks Linus. > Your explanation is really make sense. > > We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for > pinctrl-single, no issue at all. So far we've seen that if you have issues with this, the real problem is that some other driver is trying to initialize way too early probably because of legacy reasons that no longer apply. FYI, it's best to have all the drivers initialize with just module_init and make them work as loadable modules because of the following reasons: 1. You will get real console error messages when something goes wrong with no need for debug_ll and earlyprintk 2. By creating loadable driver modules you're already getting some protection from spaghetti code as the interfaces are defined 3. It will be easier for distros to support various ARM SoCs with loadable driver modules Regards, Tony
On Fri, Aug 30, 2013 at 1:48 PM, Tony Lindgren <tony@atomide.com> wrote: > * zhangfei gao <zhangfei.gao@gmail.com> [130829 04:03]: >> On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> > On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote: >> > >> >>> Instead of use platform_driver_probe, use module_platform_driver >> >>> To support deferred probing >> >>> Also subsys_initcall may too early to auto set pinctl >> >>> >> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> >>> Acked-by: Baruch Siach <baruch@tkos.co.il> >> >> >> >> This patch is tougher than it looks. You need it, because >> >> subsys_initcall may be too early for pinctrl. >> > >> > pinctrl is initialized very early, core_initcall(). >> > >> > This is more a question of individual pin control drivers >> > and when they probe, and dependencies trying to take >> > a pinctrl handle before the pin controller is available >> > will be deferred. Even by those grabbed in the core >> > by drivers/base/pinctrl.c. >> >> Thanks Linus. >> Your explanation is really make sense. >> >> We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for >> pinctrl-single, no issue at all. > > So far we've seen that if you have issues with this, the real > problem is that some other driver is trying to initialize way > too early probably because of legacy reasons that no longer > apply. > > FYI, it's best to have all the drivers initialize with just > module_init and make them work as loadable modules because of > the following reasons: > > 1. You will get real console error messages when something > goes wrong with no need for debug_ll and earlyprintk > > 2. By creating loadable driver modules you're already getting > some protection from spaghetti code as the interfaces > are defined > > 3. It will be easier for distros to support various ARM SoCs > with loadable driver modules > > Regards, > > Tony Thanks Tony, What about concerns from Wolfram: " Other people might be depending on subsys_initcall to get I2C active before they want to activate, say, PMICs. So, I fear regressions, since deferred probing might not be available in the needed places to avoid these regressions." Is it too late using module_init for PMIC? Besides, the deferred probing still there if depend on late registered pin control driver. Thanks
* zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: > What about concerns from Wolfram: > " Other people might be > depending on subsys_initcall to get I2C active before they want to > activate, say, PMICs. So, I fear regressions, since deferred probing > might not be available in the needed places to avoid these regressions." There should not be any reason to get a PMIC activated early on. The system should be booting already at that point, and the PMIC related init can be done later on. > Is it too late using module_init for PMIC? You can probably do it as a fix early on during the -rc cycle too. Of course it needs to be verified to work first :) > Besides, the deferred probing still there if depend on late registered > pin control driver. At least for omaps we have things working just fine with pinctrl-single and a PMIC on I2C controller. Regards, Tony
On Fri, Aug 30, 2013 at 4:27 PM, Tony Lindgren <tony@atomide.com> wrote: > * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: >> What about concerns from Wolfram: >> " Other people might be >> depending on subsys_initcall to get I2C active before they want to >> activate, say, PMICs. So, I fear regressions, since deferred probing >> might not be available in the needed places to avoid these regressions." > > There should not be any reason to get a PMIC activated > early on. The system should be booting already at that point, > and the PMIC related init can be done later on. > >> Is it too late using module_init for PMIC? > > You can probably do it as a fix early on during the -rc > cycle too. Of course it needs to be verified to work first :) > Dear Wolfram What's your suggestion about this issue. Use subsys_initcall, deferred probing still exist if base on pin control driver. Thanks
On Thu, Sep 12, 2013 at 9:12 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote: > On Fri, Aug 30, 2013 at 4:27 PM, Tony Lindgren <tony@atomide.com> wrote: >> * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: >>> What about concerns from Wolfram: >>> " Other people might be >>> depending on subsys_initcall to get I2C active before they want to >>> activate, say, PMICs. So, I fear regressions, since deferred probing >>> might not be available in the needed places to avoid these regressions." >> >> There should not be any reason to get a PMIC activated >> early on. The system should be booting already at that point, >> and the PMIC related init can be done later on. >> >>> Is it too late using module_init for PMIC? >> >> You can probably do it as a fix early on during the -rc >> cycle too. Of course it needs to be verified to work first :) >> > > Dear Wolfram > > What's your suggestion about this issue. > Use subsys_initcall, deferred probing still exist if base on pin control driver. > > Thanks Dear Wolfram Any plan about the patch? On one hand, module_X_driver is trend to replace subsys_initcall Refer from Mark "We're trying to move away from needing to do this and to using deferred probing to resolve init ordering issues. Should we not be able to convert the drivers to module_X_driver()?" On the other hand, subsys_initcall still been defered if pin controller driver been relied on. Thanks
> Any plan about the patch?
I am right now working on a patch series dealing with this.
On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote: > * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: > > What about concerns from Wolfram: > > " Other people might be > > depending on subsys_initcall to get I2C active before they want to > > activate, say, PMICs. So, I fear regressions, since deferred probing > > might not be available in the needed places to avoid these regressions." > > There should not be any reason to get a PMIC activated > early on. The system should be booting already at that point, > and the PMIC related init can be done later on. Okay, here is a more concrete example: Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware of deferred probing, I wrote the following code to get the GPIOs (which are optional): err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute"); max9768->mute_gpio = err ?: pdata->mute_gpio; And later in the process: if (gpio_is_valid(max9768->mute_gpio)) { ret = snd_soc_add_codec_controls(codec, max9768_mute, ARRAY_SIZE(max9768_mute)); if (ret) return ret; } So, the mute control will only be added if the gpio_request succeeded. On that particular board, the mute GPIO was wired to an I2C GPIO controller. If I now change the I2C (or GPIO) driver from subsys_initcall to module_init, then the gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute control will then disappear. Yes, the driver can be fixed easily, yet I fear a number of regressions like this. Instead of people digging into why things disappear after a kernel update, I wonder if there is a way to guide users if this happens. I didn't have time for that, though, sadly. Still, it makes me wonder how easily we could shift from subsys_initcall to module_init, although I'd really love to get away from subsys_initcall in device drivers. Regards, Wolfram
* Wolfram Sang <wsa@the-dreams.de> [131008 14:01]: > On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote: > > * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: > > > What about concerns from Wolfram: > > > " Other people might be > > > depending on subsys_initcall to get I2C active before they want to > > > activate, say, PMICs. So, I fear regressions, since deferred probing > > > might not be available in the needed places to avoid these regressions." > > > > There should not be any reason to get a PMIC activated > > early on. The system should be booting already at that point, > > and the PMIC related init can be done later on. > > Okay, here is a more concrete example: > > Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware > of deferred probing, I wrote the following code to get the GPIOs (which are > optional): > > err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute"); > max9768->mute_gpio = err ?: pdata->mute_gpio; > > And later in the process: > > if (gpio_is_valid(max9768->mute_gpio)) { > ret = snd_soc_add_codec_controls(codec, max9768_mute, > ARRAY_SIZE(max9768_mute)); > if (ret) > return ret; > } > > So, the mute control will only be added if the gpio_request succeeded. On that > particular board, the mute GPIO was wired to an I2C GPIO controller. If I now > change the I2C (or GPIO) driver from subsys_initcall to module_init, then the > gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute > control will then disappear. Yes, the driver can be fixed easily, yet I fear a > number of regressions like this. Instead of people digging into why things > disappear after a kernel update, I wonder if there is a way to guide users if > this happens. I didn't have time for that, though, sadly. Still, it makes me > wonder how easily we could shift from subsys_initcall to module_init, although > I'd really love to get away from subsys_initcall in device drivers. Well it should be pretty trivial to update drivers to use deferred probing. Maybe some spatch to check for that in driver probes would help getting an idea how many might be affected? Anyways, it should be fixed as otherwise we'll just dig ourselves deeper into the mess of things not working as loadable modules. Regards, Tony
On 10/09/2013 04:53 AM, Wolfram Sang wrote: > On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote: >> * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: >>> What about concerns from Wolfram: >>> " Other people might be >>> depending on subsys_initcall to get I2C active before they want to >>> activate, say, PMICs. So, I fear regressions, since deferred probing >>> might not be available in the needed places to avoid these regressions." >> >> There should not be any reason to get a PMIC activated >> early on. The system should be booting already at that point, >> and the PMIC related init can be done later on. > > Okay, here is a more concrete example: > > Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware > of deferred probing, I wrote the following code to get the GPIOs (which are > optional): > > err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute"); > max9768->mute_gpio = err ?: pdata->mute_gpio; > > And later in the process: > > if (gpio_is_valid(max9768->mute_gpio)) { > ret = snd_soc_add_codec_controls(codec, max9768_mute, > ARRAY_SIZE(max9768_mute)); > if (ret) > return ret; > } > > So, the mute control will only be added if the gpio_request succeeded. On that > particular board, the mute GPIO was wired to an I2C GPIO controller. If I now > change the I2C (or GPIO) driver from subsys_initcall to module_init, then the > gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute > control will then disappear. However, this may requires I2C, GPIO, pinctrl all use subsys_initcall. -EPROBE_DEFER still be returned, if pinctrl happen to use module_platform_driver, which is most case in drivers/pinctrl/. Yes, the driver can be fixed easily, yet I fear a > number of regressions like this. Instead of people digging into why things > disappear after a kernel update, I wonder if there is a way to guide users if > this happens. I didn't have time for that, though, sadly. Still, it makes me > wonder how easily we could shift from subsys_initcall to module_init, although > I'd really love to get away from subsys_initcall in device drivers. > Thanks for clarify the concern.
* Tony Lindgren <tony@atomide.com> [131008 15:19]: > * Wolfram Sang <wsa@the-dreams.de> [131008 14:01]: > > On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote: > > > * zhangfei gao <zhangfei.gao@gmail.com> [130829 23:36]: > > > > What about concerns from Wolfram: > > > > " Other people might be > > > > depending on subsys_initcall to get I2C active before they want to > > > > activate, say, PMICs. So, I fear regressions, since deferred probing > > > > might not be available in the needed places to avoid these regressions." > > > > > > There should not be any reason to get a PMIC activated > > > early on. The system should be booting already at that point, > > > and the PMIC related init can be done later on. > > > > Okay, here is a more concrete example: > > > > Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware > > of deferred probing, I wrote the following code to get the GPIOs (which are > > optional): > > > > err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute"); > > max9768->mute_gpio = err ?: pdata->mute_gpio; > > > > And later in the process: > > > > if (gpio_is_valid(max9768->mute_gpio)) { > > ret = snd_soc_add_codec_controls(codec, max9768_mute, > > ARRAY_SIZE(max9768_mute)); > > if (ret) > > return ret; > > } > > > > So, the mute control will only be added if the gpio_request succeeded. On that > > particular board, the mute GPIO was wired to an I2C GPIO controller. If I now > > change the I2C (or GPIO) driver from subsys_initcall to module_init, then the > > gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute > > control will then disappear. Yes, the driver can be fixed easily, yet I fear a > > number of regressions like this. Instead of people digging into why things > > disappear after a kernel update, I wonder if there is a way to guide users if > > this happens. I didn't have time for that, though, sadly. Still, it makes me > > wonder how easily we could shift from subsys_initcall to module_init, although > > I'd really love to get away from subsys_initcall in device drivers. > > Well it should be pretty trivial to update drivers to use deferred > probing. Maybe some spatch to check for that in driver probes would > help getting an idea how many might be affected? > > Anyways, it should be fixed as otherwise we'll just dig ourselves > deeper into the mess of things not working as loadable modules. BTW, another place where things can go wrong is if there's an irqchip driver that is being set up at module_init time. If an interrupt client driver does irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0), the resources may not have been initialize for the DT case as those are populated triggered by of_platform_populate(). The fix there is to use irq = irq_of_parse_and_map(pdev->dev.of_node, 0) instead. Or somehow make of_platform_populate() support -EPROBE_DEFER. Just FYI, Tony Tony
> Well it should be pretty trivial to update drivers to use deferred > probing. Maybe some spatch to check for that in driver probes would > help getting an idea how many might be affected? That's what I am trying to say. It surely is easy to fix the drivers, once we know there is something in need of fixing. My question was if there is common sense to simply risk breaking things and fix them later (then I'd apply patches switching from subsys_initcall to module_init right away), or if we can gather ideas how to minimize the impact of regressions (before applying such patches).
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 4c5fada..36ceebc 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -236,6 +236,7 @@ static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume); MODULE_ALIAS("platform:i2c_designware"); static struct platform_driver dw_i2c_driver = { + .probe = dw_i2c_probe, .remove = dw_i2c_remove, .driver = { .name = "i2c_designware", @@ -245,18 +246,7 @@ static struct platform_driver dw_i2c_driver = { .pm = &dw_i2c_dev_pm_ops, }, }; - -static int __init dw_i2c_init_driver(void) -{ - return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe); -} -subsys_initcall(dw_i2c_init_driver); - -static void __exit dw_i2c_exit_driver(void) -{ - platform_driver_unregister(&dw_i2c_driver); -} -module_exit(dw_i2c_exit_driver); +module_platform_driver(dw_i2c_driver); MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>"); MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");