| Message ID | 20200912125148.1271481-1-maz@kernel.org |
|---|---|
| Headers | show |
| Series | irqchip: Hybrid probing | expand |
On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > IRQCHIP_DECLARE() is backed by macros that perform some elementary > type checking on the probe function. Unfortunately, IRQCHIP_MATCH() > doesn't, risking difficult debugging sessions... > > Rewrite IRQCHIP_MATCH() in terms of _OF_DECLARE_ELMT() restore > the missing type safety. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > include/linux/irqchip.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h > index 67351aac65ef..f8f25e9f8200 100644 > --- a/include/linux/irqchip.h > +++ b/include/linux/irqchip.h > @@ -33,7 +33,7 @@ extern int platform_irqchip_probe(struct platform_device *pdev); > #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \ > static const struct of_device_id drv_name##_irqchip_match_table[] = { > > -#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn }, > +#define IRQCHIP_MATCH(compat, fn) _OF_DECLARE_ELMT(compat, fn, of_init_fn_2) > > #define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \ > {}, \ > -- > 2.28.0 >
On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > Although we are trying to move to a world where a large number > of irqchip drivers can safely be built as platform drivers > the reality is that most endpoint drivers are not ready for that, > and will fail to probe as they expect their interrupt controller > to be up and running. > > A halfway house solution is to let the driver indicate that if > it is built-in (i.e. not a module), then it must use the earily > probe mechanism, IRQCHIP_DECLARE() style. Otherwise, it is a > normal module implemenenting a platform driver, and we can > fallback to the existing code. > > Hopefully we'll one day be able to drop this code altogether, > but that's not for tomorrow. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > include/linux/irqchip.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h > index f8f25e9f8200..31fc9d00101f 100644 > --- a/include/linux/irqchip.h > +++ b/include/linux/irqchip.h > @@ -50,6 +50,18 @@ static struct platform_driver drv_name##_driver = { \ > }; \ > builtin_platform_driver(drv_name##_driver) > > +#ifdef MODULE > +#define IRQCHIP_HYBRID_DRIVER_BEGIN(drv) \ > + IRQCHIP_PLATFORM_DRIVER_BEGIN(drv) > +#define IRQCHIP_HYBRID_DRIVER_END(drv) \ > + IRQCHIP_PLATFORM_DRIVER_END(drv) > +#else > +#define IRQCHIP_HYBRID_DRIVER_BEGIN(drv) \ > + _OF_DECLARE_ARRAY_START(irqchip, drv) > +#define IRQCHIP_HYBRID_DRIVER_END(drv) \ > + _OF_DECLARE_ARRAY_END; > +#endif > + > /* > * This macro must be used by the different irqchip drivers to declare > * the association between their version and their initialization function. > -- > 2.28.0 >
On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > Switch the driver to a "hybrid probe" model which preserves the > built-in behaviour while allowing the driver to be optionnally > built as a module for development purposes. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/irqchip/irq-mtk-cirq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > index 69ba8ce3c178..43e880b63ed2 100644 > --- a/drivers/irqchip/irq-mtk-cirq.c > +++ b/drivers/irqchip/irq-mtk-cirq.c > @@ -295,4 +295,6 @@ static int __init mtk_cirq_of_init(struct device_node *node, > return ret; > } > > -IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init); > +IRQCHIP_HYBRID_DRIVER_BEGIN(mtk_cirq) > +IRQCHIP_MATCH("mediatek,mtk-cirq", mtk_cirq_of_init) > +IRQCHIP_HYBRID_DRIVER_END(mtk_cirq) > -- > 2.28.0 >
On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > Switch the driver to a "hybrid probe" model which preserves the > built-in behaviour while allowing the driver to be optionnally > built as a module for development purposes. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/irqchip/irq-mtk-sysirq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c > index 6ff98b87e5c0..ee45d8f71ec3 100644 > --- a/drivers/irqchip/irq-mtk-sysirq.c > +++ b/drivers/irqchip/irq-mtk-sysirq.c > @@ -231,4 +231,6 @@ static int __init mtk_sysirq_of_init(struct device_node *node, > kfree(chip_data); > return ret; > } > -IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init); > +IRQCHIP_HYBRID_DRIVER_BEGIN(mtk_sysirq) > +IRQCHIP_MATCH("mediatek,mt6577-sysirq", mtk_sysirq_of_init) > +IRQCHIP_HYBRID_DRIVER_END(mtk_sysirq) > -- > 2.28.0 >
On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > Switch the driver to a "hybrid probe" model which preserves the > built-in behaviour while allowing the driver to be optionnally > built as a module for development purposes. > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > drivers/irqchip/qcom-pdc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index 6ae9e1f0819d..8543fa23da10 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -430,4 +430,6 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > return ret; > } > > -IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init); > +IRQCHIP_HYBRID_DRIVER_BEGIN(qcom_pdc) > +IRQCHIP_MATCH("qcom,pdc", qcom_pdc_init) > +IRQCHIP_HYBRID_DRIVER_END(qcom_pdc) > -- > 2.28.0 >
On Sat, Sep 12, 2020 at 5:52 AM Marc Zyngier <maz@kernel.org> wrote: > > A recent attempt at converting a couple of interrupt controllers from > early probing to standard platform drivers have badly failed, as it > became evident that although an interrupt controller can easily probe > late, device drivers for the endpoints connected to it are rarely > equipped to deal with probe deferral. Changes were swiftly reverted. > > However, there is some value in *optionally* enabling this, if only > for development purposes, as there is otherwise a "chicken and egg" > problem, and a few people (cc'd) are working on a potential solution. > > This short series enables the infrastructure for modular building > whilst retaining the usual early probing for monolithic build, and > introduces it to the three drivers that were previously made to probe > as platform drivers. > > As I don't have any of the HW, this series is fully untested and I'd > welcome some feedback on it. I've tested this on db845c along with a small follow-on patch I'll send to you which sets the qcom-pdc as a tristate in the Kconfig, both as a module and as a built-in. Tested-by: John Stultz <john.stultz@linaro.org> thanks -john
On Sat, Sep 12, 2020 at 01:51:42PM +0100, Marc Zyngier wrote: > A recent attempt at converting a couple of interrupt controllers from > early probing to standard platform drivers have badly failed, as it > became evident that although an interrupt controller can easily probe > late, device drivers for the endpoints connected to it are rarely > equipped to deal with probe deferral. Changes were swiftly reverted. > > However, there is some value in *optionally* enabling this, if only > for development purposes, as there is otherwise a "chicken and egg" > problem, and a few people (cc'd) are working on a potential solution. > > This short series enables the infrastructure for modular building > whilst retaining the usual early probing for monolithic build, and > introduces it to the three drivers that were previously made to probe > as platform drivers. I hardly expected more OF_DECLARE macros when I opened this up. Given desires to get rid of them, I don't think adding to it is the way forward. That wrapping a platform driver around OF_DECLARE looks pretty horrible IMO. I browsed some of the discussion around this. It didn't seem like it's a large number of drivers that have to be fixed to defer probe correctly. Am I missing something? I'd rather keep the pressure on getting fw_devlink on by default. Rob
Missatge de Bjorn Andersson <bjorn.andersson@linaro.org> del dia dg., 13 de set. 2020 a les 1:25: > > On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > > > Switch the driver to a "hybrid probe" model which preserves the > > built-in behaviour while allowing the driver to be optionnally > > built as a module for development purposes. > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> I've tested this on mt8173 and mt8183, and this time, the patches didn't break booting on these platforms. For MediaTek, right now, only makes sense the driver to be built-in as other drivers that use it are not ready for deferring their probe. So, Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Thanks Enric > > --- > > drivers/irqchip/irq-mtk-sysirq.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c > > index 6ff98b87e5c0..ee45d8f71ec3 100644 > > --- a/drivers/irqchip/irq-mtk-sysirq.c > > +++ b/drivers/irqchip/irq-mtk-sysirq.c > > @@ -231,4 +231,6 @@ static int __init mtk_sysirq_of_init(struct device_node *node, > > kfree(chip_data); > > return ret; > > } > > -IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init); > > +IRQCHIP_HYBRID_DRIVER_BEGIN(mtk_sysirq) > > +IRQCHIP_MATCH("mediatek,mt6577-sysirq", mtk_sysirq_of_init) > > +IRQCHIP_HYBRID_DRIVER_END(mtk_sysirq) > > -- > > 2.28.0 > >
Missatge de Bjorn Andersson <bjorn.andersson@linaro.org> del dia dg., 13 de set. 2020 a les 1:26: > > On Sat 12 Sep 07:51 CDT 2020, Marc Zyngier wrote: > > > Switch the driver to a "hybrid probe" model which preserves the > > built-in behaviour while allowing the driver to be optionnally > > built as a module for development purposes. > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> I've tested this on mt8173 and mt8183, and this time, the patches didn't break booting on these platforms. For MediaTek, right now, only makes sense the driver to be built-in as other drivers that use it are not ready for deferring their probe. So, Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Thanks Enric > > --- > > drivers/irqchip/irq-mtk-cirq.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > > index 69ba8ce3c178..43e880b63ed2 100644 > > --- a/drivers/irqchip/irq-mtk-cirq.c > > +++ b/drivers/irqchip/irq-mtk-cirq.c > > @@ -295,4 +295,6 @@ static int __init mtk_cirq_of_init(struct device_node *node, > > return ret; > > } > > > > -IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init); > > +IRQCHIP_HYBRID_DRIVER_BEGIN(mtk_cirq) > > +IRQCHIP_MATCH("mediatek,mtk-cirq", mtk_cirq_of_init) > > +IRQCHIP_HYBRID_DRIVER_END(mtk_cirq) > > -- > > 2.28.0 > >
On 2020-09-15 22:13, Rob Herring wrote: > On Sat, Sep 12, 2020 at 01:51:42PM +0100, Marc Zyngier wrote: >> A recent attempt at converting a couple of interrupt controllers from >> early probing to standard platform drivers have badly failed, as it >> became evident that although an interrupt controller can easily probe >> late, device drivers for the endpoints connected to it are rarely >> equipped to deal with probe deferral. Changes were swiftly reverted. >> >> However, there is some value in *optionally* enabling this, if only >> for development purposes, as there is otherwise a "chicken and egg" >> problem, and a few people (cc'd) are working on a potential solution. >> >> This short series enables the infrastructure for modular building >> whilst retaining the usual early probing for monolithic build, and >> introduces it to the three drivers that were previously made to probe >> as platform drivers. > > I hardly expected more OF_DECLARE macros when I opened this up. Given > desires to get rid of them, I don't think adding to it is the way > forward. That wrapping a platform driver around OF_DECLARE looks pretty > horrible IMO. Nobody said it was cute. It's a band aid that allows us to move from the status-quo that exists today. How would you propose we allow people to go and start "fixing" drivers if you don't give them the opportunity to even start trying? > I browsed some of the discussion around this. It didn't seem like it's > a large number of drivers that have to be fixed to defer probe > correctly. Am I missing something? Well, that was enough drivers for the two platforms that had it enabled to break horribly, without a way to go back to a working state. Do you find that acceptable? I don't. > I'd rather keep the pressure on getting fw_devlink on by default. So far, fw_devlink breaks everything under the sun, even without modular irqchips. Most of my systems fail to boot if I enable it. So yes, it really needs some work. And this series allows this work to happen. M.
On Wed, Sep 16, 2020 at 2:51 AM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-09-15 22:13, Rob Herring wrote: > > On Sat, Sep 12, 2020 at 01:51:42PM +0100, Marc Zyngier wrote: > >> A recent attempt at converting a couple of interrupt controllers from > >> early probing to standard platform drivers have badly failed, as it > >> became evident that although an interrupt controller can easily probe > >> late, device drivers for the endpoints connected to it are rarely > >> equipped to deal with probe deferral. Changes were swiftly reverted. > >> > >> However, there is some value in *optionally* enabling this, if only > >> for development purposes, as there is otherwise a "chicken and egg" > >> problem, and a few people (cc'd) are working on a potential solution. > >> > >> This short series enables the infrastructure for modular building > >> whilst retaining the usual early probing for monolithic build, and > >> introduces it to the three drivers that were previously made to probe > >> as platform drivers. > > > > I hardly expected more OF_DECLARE macros when I opened this up. Given > > desires to get rid of them, I don't think adding to it is the way > > forward. That wrapping a platform driver around OF_DECLARE looks pretty > > horrible IMO. > > Nobody said it was cute. It's a band aid that allows us to move from the > status-quo that exists today. How would you propose we allow people to > go and start "fixing" drivers if you don't give them the opportunity > to even start trying? Apply the reverted patches and start fixing the drivers. > > I browsed some of the discussion around this. It didn't seem like it's > > a large number of drivers that have to be fixed to defer probe > > correctly. Am I missing something? > > Well, that was enough drivers for the two platforms that had it enabled > to break horribly, without a way to go back to a working state. Do you > find that acceptable? I don't. I understand reverting for v5.9, that was the right choice. But Mediatek had 3 drivers broken. Is there more to it than getting EPROBE_DEFER handled correctly in those drivers? > > I'd rather keep the pressure on getting fw_devlink on by default. > > So far, fw_devlink breaks everything under the sun, even without modular > irqchips. Most of my systems fail to boot if I enable it. So yes, it > really needs some work. And this series allows this work to happen. I think we can do something more simple here. We just need to instantiate the irqchip devices earlier to get them to probe first and not cause deferrals. That's just a matter of calling of_platform_device_create() in an earlier initcall (<= arch_initcall_sync ('=' because of link order)). Then once dependent drivers are all fixed, all that has to be done is rip out that initcall and the default of_platform_populate call will create the device instead. Rob
A recent attempt at converting a couple of interrupt controllers from early probing to standard platform drivers have badly failed, as it became evident that although an interrupt controller can easily probe late, device drivers for the endpoints connected to it are rarely equipped to deal with probe deferral. Changes were swiftly reverted. However, there is some value in *optionally* enabling this, if only for development purposes, as there is otherwise a "chicken and egg" problem, and a few people (cc'd) are working on a potential solution. This short series enables the infrastructure for modular building whilst retaining the usual early probing for monolithic build, and introduces it to the three drivers that were previously made to probe as platform drivers. As I don't have any of the HW, this series is fully untested and I'd welcome some feedback on it. Thanks, M. Marc Zyngier (6): of: Add basic infrastructure to create early probe arrays irqchip: Make IRQCHIP_MATCH() type safe irqchip: Introduce IRQCHIP_HYBRID_DRIVER_{BEGIN,END} macros irqchip/mtk-cirq: Allow modular build irqchip/mtk-sysirq: Allow modular build irqchip/qcom-pdc: Allow modular build drivers/irqchip/irq-mtk-cirq.c | 4 +++- drivers/irqchip/irq-mtk-sysirq.c | 4 +++- drivers/irqchip/qcom-pdc.c | 4 +++- include/linux/irqchip.h | 14 +++++++++++++- include/linux/of.h | 15 +++++++++++++++ 5 files changed, 37 insertions(+), 4 deletions(-)