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