| Message ID | 1551975829-16350-1-git-send-email-fabien.dessenne@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip: stm32: add a second level init to request hwspinlock | expand |
On 07/03/2019 16:23, Fabien Dessenne wrote: > Requesting hwspinlock, at the first time it is used, is not correct: > indeed, at that moment we are under raw_spin_lock_irqsave() context and > hwspin_lock_request_specific() may sleep ("BUG: sleeping function called > from invalid context"). > Requesting hwspinlock during the init (stm32*_exti_of_init()) is also > not possible (the hwspinlock framework is not ready at that stage of the > kernel init). > As a consequence, add a second level init (probed with arch_initcall) > where we can safely request hwspinlock. No, this is fairly broken. You're playing with stuff you're not supposed to (OF_POPULATE? really?), and adding initcalls is completely unreliable (things depend on the link order and will randomly break). If you need dependencies, implement them correctly. Turn this driver into a real device driver (in the platform device sense), and return PROBE_DEFER when you can't find your dependency. Thanks, M.
Hi Marc, Thank you for your feedback. Let me try to explain this patch, and the reason of its unusual implementation choices. Regarding the driver init mode: As an important requirement, I want to keep this irq driver declared with IRQCHIP_DECLARE(), so it is initialized early from start_kernel()/init_IRQ(). Most of the other irq drivers are implemented this way and I imagine that this ensures the availability of the irq drivers, before the other platform drivers get probed. Regarding the second init: With the usage of the hwspinlock framework (used to protect against coprocessor concurrent access to registers) we have a problem as the hwspinlock driver is not available when the irq driver is being initialized. In order to solve this, I added a second initialization where we get a reference to hwspinlock. You pointed that we are not supposed to use of_node_clear_flag (which allows to get a second init call) : I spent some time to find any information about it, but could not find any reason to not use it. Please, let me know if I missed something here. Regarding the inits sequence and dependencies: - The second init is guaranteed to be called after the first one, since start_kernel()->init_IRQ() is called before platform drivers init. - During the second init, the dependency with the hwspinlock driver is implemented correctly : it makes use of defered probe when needed. I understand that this patch is 'surprising' but I hope that my explanations justify its implementation. Waiting for your feedback BR Fabien On 07/03/2019 5:44 PM, Marc Zyngier wrote: > On 07/03/2019 16:23, Fabien Dessenne wrote: >> Requesting hwspinlock, at the first time it is used, is not correct: >> indeed, at that moment we are under raw_spin_lock_irqsave() context and >> hwspin_lock_request_specific() may sleep ("BUG: sleeping function called >> from invalid context"). >> Requesting hwspinlock during the init (stm32*_exti_of_init()) is also >> not possible (the hwspinlock framework is not ready at that stage of the >> kernel init). >> As a consequence, add a second level init (probed with arch_initcall) >> where we can safely request hwspinlock. > No, this is fairly broken. You're playing with stuff you're not supposed > to (OF_POPULATE? really?), and adding initcalls is completely unreliable > (things depend on the link order and will randomly break). > > If you need dependencies, implement them correctly. Turn this driver > into a real device driver (in the platform device sense), and return > PROBE_DEFER when you can't find your dependency. > > Thanks, > > M.
On Fri, 08 Mar 2019 14:03:55 +0000, Fabien DESSENNE <fabien.dessenne@st.com> wrote: Fabien, > > Hi Marc, > > Thank you for your feedback. Let me try to explain this patch, and the > reason of its unusual implementation choices. > > > Regarding the driver init mode: > As an important requirement, I want to keep this irq driver declared > with IRQCHIP_DECLARE(), so it is initialized early from > start_kernel()/init_IRQ(). > Most of the other irq drivers are implemented this way and I imagine > that this ensures the availability of the irq drivers, before the other > platform drivers get probed. Let me get this straight: - Either you don't have dependencies on anything, and you need to enable your irqchip early -> you use IRQCHIP_DECLARE. - Or you have dependencies on other subsystems -> You *do not* use IRQCHIP_DECLARE, and use the expected dependency system (deferred probing) There is no intermediate state. The other irqchip controllers that use IRQCHIP_DECLARE *do NOT* have dependencies on external subsystems. > Regarding the second init: > With the usage of the hwspinlock framework (used to protect against > coprocessor concurrent access to registers) we have a problem as the > hwspinlock driver is not available when the irq driver is being initialized. > In order to solve this, I added a second initialization where we get a > reference to hwspinlock. > You pointed that we are not supposed to use of_node_clear_flag (which > allows to get a second init call) : > I spent some time to find any information about it, but could not find > any reason to not use it. > Please, let me know if I missed something here. Yes, you missed the fact that each time someone tries to add some driver probing via an initcall, we push back. This is an internal kernel mechanism that is not to be used by random, non architectural drivers such as this interrupt controller. Furthermore, you're playing with stuff that is outside of the exported API of the DT framework. Clearing node flags is not something I really want to see, as you're messing with a state machine that isn't under your control. > Regarding the inits sequence and dependencies: > - The second init is guaranteed to be called after the first one, since > start_kernel()->init_IRQ() is called before platform drivers init. There is no such requirements that holds for secondary interrupt controllers. > - During the second init, the dependency with the hwspinlock driver is > implemented correctly : it makes use of defered probe when needed. Then do the right thing all the way: move your favourite toy irqchip to being a proper device driver, use the right abstraction, and stop piling ugly hacks on top of each other. Other irqchip drivers do that just fine (all GPIO irqchips, for example), and most drivers are already able to defer their probe routine. > I understand that this patch is 'surprising' but I hope that my > explanations justify its implementation. Surprising is not the word I'd have used. The explanations do not justify anything, as all you're saying is "it is the way it is because it is the way it is". So please fix your irqchip driver properly, in a way that is maintainable in the long term, using the abstractions that are available. If such abstractions are not good enough, please explain what you need and we'll work something out. Thanks, M.
Hi Marc, Thanks for your detailed answer. I will study how to move this driver to a device one. BR Fabien On 08/03/2019 4:30 PM, Marc Zyngier wrote: > On Fri, 08 Mar 2019 14:03:55 +0000, > Fabien DESSENNE <fabien.dessenne@st.com> wrote: > > Fabien, > >> Hi Marc, >> >> Thank you for your feedback. Let me try to explain this patch, and the >> reason of its unusual implementation choices. >> >> >> Regarding the driver init mode: >> As an important requirement, I want to keep this irq driver declared >> with IRQCHIP_DECLARE(), so it is initialized early from >> start_kernel()/init_IRQ(). >> Most of the other irq drivers are implemented this way and I imagine >> that this ensures the availability of the irq drivers, before the other >> platform drivers get probed. > Let me get this straight: > > - Either you don't have dependencies on anything, and you need to > enable your irqchip early -> you use IRQCHIP_DECLARE. > > - Or you have dependencies on other subsystems -> You *do not* use > IRQCHIP_DECLARE, and use the expected dependency system (deferred > probing) > > There is no intermediate state. The other irqchip controllers that use > IRQCHIP_DECLARE *do NOT* have dependencies on external subsystems. > >> Regarding the second init: >> With the usage of the hwspinlock framework (used to protect against >> coprocessor concurrent access to registers) we have a problem as the >> hwspinlock driver is not available when the irq driver is being initialized. >> In order to solve this, I added a second initialization where we get a >> reference to hwspinlock. >> You pointed that we are not supposed to use of_node_clear_flag (which >> allows to get a second init call) : >> I spent some time to find any information about it, but could not find >> any reason to not use it. >> Please, let me know if I missed something here. > Yes, you missed the fact that each time someone tries to add some > driver probing via an initcall, we push back. This is an internal > kernel mechanism that is not to be used by random, non architectural > drivers such as this interrupt controller. > > Furthermore, you're playing with stuff that is outside of the exported > API of the DT framework. Clearing node flags is not something I really > want to see, as you're messing with a state machine that isn't under > your control. > >> Regarding the inits sequence and dependencies: >> - The second init is guaranteed to be called after the first one, since >> start_kernel()->init_IRQ() is called before platform drivers init. > There is no such requirements that holds for secondary interrupt > controllers. > >> - During the second init, the dependency with the hwspinlock driver is >> implemented correctly : it makes use of defered probe when needed. > Then do the right thing all the way: move your favourite toy irqchip > to being a proper device driver, use the right abstraction, and stop > piling ugly hacks on top of each other. Other irqchip drivers do that > just fine (all GPIO irqchips, for example), and most drivers are > already able to defer their probe routine. > >> I understand that this patch is 'surprising' but I hope that my >> explanations justify its implementation. > Surprising is not the word I'd have used. The explanations do not > justify anything, as all you're saying is "it is the way it is because > it is the way it is". So please fix your irqchip driver properly, in a > way that is maintainable in the long term, using the abstractions that > are available. If such abstractions are not good enough, please > explain what you need and we'll work something out. > > Thanks, > > M. >
diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c index 6edfd4b..2a5cc00 100644 --- a/drivers/irqchip/irq-stm32-exti.c +++ b/drivers/irqchip/irq-stm32-exti.c @@ -14,8 +14,10 @@ #include <linux/irqchip.h> #include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> +#include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/of_platform.h> #include <linux/syscore_ops.h> #include <dt-bindings/interrupt-controller/arm-gic.h> @@ -37,12 +39,6 @@ struct stm32_exti_bank { #define UNDEF_REG ~0 -enum stm32_exti_hwspinlock { - HWSPINLOCK_UNKNOWN, - HWSPINLOCK_NONE, - HWSPINLOCK_READY, -}; - struct stm32_desc_irq { u32 exti; u32 irq_parent; @@ -69,8 +65,6 @@ struct stm32_exti_host_data { void __iomem *base; struct stm32_exti_chip_data *chips_data; const struct stm32_exti_drv_data *drv_data; - struct device_node *node; - enum stm32_exti_hwspinlock hwlock_state; struct hwspinlock *hwlock; }; @@ -285,49 +279,27 @@ static int stm32_exti_set_type(struct irq_data *d, static int stm32_exti_hwspin_lock(struct stm32_exti_chip_data *chip_data) { - struct stm32_exti_host_data *host_data = chip_data->host_data; - struct hwspinlock *hwlock; - int id, ret = 0, timeout = 0; - - /* first time, check for hwspinlock availability */ - if (unlikely(host_data->hwlock_state == HWSPINLOCK_UNKNOWN)) { - id = of_hwspin_lock_get_id(host_data->node, 0); - if (id >= 0) { - hwlock = hwspin_lock_request_specific(id); - if (hwlock) { - /* found valid hwspinlock */ - host_data->hwlock_state = HWSPINLOCK_READY; - host_data->hwlock = hwlock; - pr_debug("%s hwspinlock = %d\n", __func__, id); - } else { - host_data->hwlock_state = HWSPINLOCK_NONE; - } - } else if (id != -EPROBE_DEFER) { - host_data->hwlock_state = HWSPINLOCK_NONE; - } else { - /* hwspinlock driver shall be ready at that stage */ - ret = -EPROBE_DEFER; - } - } + int ret, timeout = 0; - if (likely(host_data->hwlock_state == HWSPINLOCK_READY)) { - /* - * Use the x_raw API since we are under spin_lock protection. - * Do not use the x_timeout API because we are under irq_disable - * mode (see __setup_irq()) - */ - do { - ret = hwspin_trylock_raw(host_data->hwlock); - if (!ret) - return 0; - - udelay(HWSPNLCK_RETRY_DELAY); - timeout += HWSPNLCK_RETRY_DELAY; - } while (timeout < HWSPNLCK_TIMEOUT); - - if (ret == -EBUSY) - ret = -ETIMEDOUT; - } + if (!chip_data->host_data->hwlock) + return 0; + + /* + * Use the x_raw API since we are under spin_lock protection. + * Do not use the x_timeout API because we are under irq_disable + * mode (see __setup_irq()) + */ + do { + ret = hwspin_trylock_raw(chip_data->host_data->hwlock); + if (!ret) + return 0; + + udelay(HWSPNLCK_RETRY_DELAY); + timeout += HWSPNLCK_RETRY_DELAY; + } while (timeout < HWSPNLCK_TIMEOUT); + + if (ret == -EBUSY) + ret = -ETIMEDOUT; if (ret) pr_err("%s can't get hwspinlock (%d)\n", __func__, ret); @@ -337,7 +309,7 @@ static int stm32_exti_hwspin_lock(struct stm32_exti_chip_data *chip_data) static void stm32_exti_hwspin_unlock(struct stm32_exti_chip_data *chip_data) { - if (likely(chip_data->host_data->hwlock_state == HWSPINLOCK_READY)) + if (chip_data->host_data->hwlock) hwspin_unlock_raw(chip_data->host_data->hwlock); } @@ -683,8 +655,6 @@ stm32_exti_host_data *stm32_exti_host_init(const struct stm32_exti_drv_data *dd, return NULL; host_data->drv_data = dd; - host_data->node = node; - host_data->hwlock_state = HWSPINLOCK_UNKNOWN; host_data->chips_data = kcalloc(dd->bank_nr, sizeof(struct stm32_exti_chip_data), GFP_KERNEL); @@ -711,7 +681,8 @@ stm32_exti_host_data *stm32_exti_host_init(const struct stm32_exti_drv_data *dd, static struct stm32_exti_chip_data *stm32_exti_chip_init(struct stm32_exti_host_data *h_data, - u32 bank_idx) + u32 bank_idx, + struct device_node *node) { const struct stm32_exti_bank *stm32_bank; struct stm32_exti_chip_data *chip_data; @@ -741,7 +712,7 @@ stm32_exti_chip_data *stm32_exti_chip_init(struct stm32_exti_host_data *h_data, if (stm32_bank->fpr_ofst != UNDEF_REG) writel_relaxed(~0UL, base + stm32_bank->fpr_ofst); - pr_info("%pOF: bank%d\n", h_data->node, bank_idx); + pr_info("%pOF: bank%d\n", node, bank_idx); return chip_data; } @@ -781,7 +752,7 @@ static int __init stm32_exti_init(const struct stm32_exti_drv_data *drv_data, struct stm32_exti_chip_data *chip_data; stm32_bank = drv_data->exti_banks[i]; - chip_data = stm32_exti_chip_init(host_data, i); + chip_data = stm32_exti_chip_init(host_data, i, node); gc = irq_get_domain_generic_chip(domain, i * IRQS_PER_BANK); @@ -844,7 +815,7 @@ __init stm32_exti_hierarchy_init(const struct stm32_exti_drv_data *drv_data, return -ENOMEM; for (i = 0; i < drv_data->bank_nr; i++) - stm32_exti_chip_init(host_data, i); + stm32_exti_chip_init(host_data, i, node); domain = irq_domain_add_hierarchy(parent_domain, 0, drv_data->bank_nr * IRQS_PER_BANK, @@ -868,6 +839,71 @@ __init stm32_exti_hierarchy_init(const struct stm32_exti_drv_data *drv_data, return ret; } +/* Note : stm32_exti_probe() is called after stm32*_exti_of_init() */ +static int stm32_exti_probe(struct platform_device *pdev) +{ + int id, ret = 0; + + id = of_hwspin_lock_get_id(pdev->dev.of_node, 0); + + if (id == -EPROBE_DEFER) + /* hwspinlock framework not ready */ + return -EPROBE_DEFER; + + if (id == -ENOENT) + /* no hwspinlock defined (not an error, it is optional) */ + return 0; + + if (id >= 0) { + stm32_host_data->hwlock = hwspin_lock_request_specific(id); + if (!stm32_host_data->hwlock) { + dev_err(&pdev->dev, "Failed to request hwspinlock\n"); + ret = -EINVAL; + } + } else { + dev_err(&pdev->dev, "Failed to get hwspinlock\n"); + ret = id; + } + + return ret; +} + +static int stm32_exti_remove(struct platform_device *pdev) +{ + if (stm32_host_data->hwlock) + return hwspin_lock_free(stm32_host_data->hwlock); + + return 0; +} + +static const struct of_device_id stm32_exti_ids[] = { + { .compatible = "st,stm32mp1-exti", }, + {}, +}; +MODULE_DEVICE_TABLE(of, stm32_exti_ids); + +static struct platform_driver stm32_exti_driver = { + .probe = stm32_exti_probe, + .remove = stm32_exti_remove, + .driver = { + .name = "stm32_exti", + .of_match_table = stm32_exti_ids, + }, +}; + +static int __init stm32_exti_arch_init(void) +{ + return platform_driver_register(&stm32_exti_driver); +} + +static void __exit stm32_exti_arch_exit(void) +{ + return platform_driver_unregister(&stm32_exti_driver); +} + +arch_initcall(stm32_exti_arch_init); +module_exit(stm32_exti_arch_exit); + static int __init stm32f4_exti_of_init(struct device_node *np, struct device_node *parent) { @@ -887,7 +923,15 @@ IRQCHIP_DECLARE(stm32h7_exti, "st,stm32h7-exti", stm32h7_exti_of_init); static int __init stm32mp1_exti_of_init(struct device_node *np, struct device_node *parent) { - return stm32_exti_hierarchy_init(&stm32mp1_drv_data, np, parent); + int ret; + + ret = stm32_exti_hierarchy_init(&stm32mp1_drv_data, np, parent); + + /* Clear the OF_POPULATED flag so that stm32_exti_probe can be called */ + if (!ret) + of_node_clear_flag(np, OF_POPULATED); + + return ret; } IRQCHIP_DECLARE(stm32mp1_exti, "st,stm32mp1-exti", stm32mp1_exti_of_init);
Requesting hwspinlock, at the first time it is used, is not correct: indeed, at that moment we are under raw_spin_lock_irqsave() context and hwspin_lock_request_specific() may sleep ("BUG: sleeping function called from invalid context"). Requesting hwspinlock during the init (stm32*_exti_of_init()) is also not possible (the hwspinlock framework is not ready at that stage of the kernel init). As a consequence, add a second level init (probed with arch_initcall) where we can safely request hwspinlock. Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com> --- drivers/irqchip/irq-stm32-exti.c | 160 +++++++++++++++++++++++++-------------- 1 file changed, 102 insertions(+), 58 deletions(-)