Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules

drivers/irqchip/qcom-pdc.c +16 -0

index 6ae9e1f0819d..3fee8b655da1 100644
@@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
        return ret;
 }
 
+#ifdef MODULE
+static int qcom_pdc_probe(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct device_node *parent = of_irq_find_parent(np);
+
+       return qcom_pdc_init(np, parent);
+}
+
+static const struct of_device_id qcom_pdc_match_table[] = {
+       { .compatible = "qcom,pdc" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pdc_match_table);
+
+static struct platform_driver qcom_pdc_driver = {

drivers/irqchip/qcom-pdc.c +4 -0

+#endif
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Power Domain Controller");
+MODULE_LICENSE("GPL v2");

Quoting John Stultz (2020-06-24 17:10:37)

Quoted 29 lines

drivers/irqchip/qcom-pdc.c +21 -0

index 6ae9e1f0819d..3fee8b655da1 100644
@@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
        return ret;
 }
 
+#ifdef MODULE
+static int qcom_pdc_probe(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct device_node *parent = of_irq_find_parent(np);
+
+       return qcom_pdc_init(np, parent);
+}
+
+static const struct of_device_id qcom_pdc_match_table[] = {
+       { .compatible = "qcom,pdc" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pdc_match_table);
+
+static struct platform_driver qcom_pdc_driver = {
+       .probe = qcom_pdc_probe,
+       .driver = {
+               .name = "qcom-pdc",
+               .of_match_table = qcom_pdc_match_table,
+               .suppress_bind_attrs = true,

drivers/irqchip/qcom-pdc.c static int qcom_pdc_init(struct device_node *node, struct device_node *parent) · line 460

Quoted 5 lines

drivers/irqchip/qcom-pdc.c +4 -0

+       },
+};
+module_platform_driver(qcom_pdc_driver);
+#else
 IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init);

Is there any reason to use IRQCHIP_DECLARE if this can work as a platform device driver?

Quoted 31 lines · Stephen Boyd

Quoting John Stultz (2020-06-24 17:10:37)

Quoted 29 lines

drivers/irqchip/qcom-pdc.c +21 -0

index 6ae9e1f0819d..3fee8b655da1 100644
@@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
        return ret;
 }
 
+#ifdef MODULE
+static int qcom_pdc_probe(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct device_node *parent = of_irq_find_parent(np);
+
+       return qcom_pdc_init(np, parent);
+}
+
+static const struct of_device_id qcom_pdc_match_table[] = {
+       { .compatible = "qcom,pdc" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pdc_match_table);
+
+static struct platform_driver qcom_pdc_driver = {
+       .probe = qcom_pdc_probe,
+       .driver = {
+               .name = "qcom-pdc",
+               .of_match_table = qcom_pdc_match_table,
+               .suppress_bind_attrs = true,

Quoted 9 lines

drivers/irqchip/qcom-pdc.c static int qcom_pdc_init(struct device_node *node, struct device_node *parent) · line 460

Quoted 5 lines

drivers/irqchip/qcom-pdc.c +4 -0

+       },
+};
+module_platform_driver(qcom_pdc_driver);
+#else
 IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init);

Is there any reason to use IRQCHIP_DECLARE if this can work as a platform device driver?

Hey! Thanks so much for the review!

Mostly it was done this way to minimize the change in the non-module case. But if you'd rather avoid the #ifdefery I'll respin it without.

Quoted 10 lines · John Stultz

Quoted 5 lines · Stephen Boyd

Is there any reason to use IRQCHIP_DECLARE if this can work as a platform device driver?

Hey! Thanks so much for the review!

Quoted 2 lines

Mostly it was done this way to minimize the change in the non-module case. But if you'd rather avoid the #ifdefery I'll respin it without.

That would certainly be my own preference. In general, IRQCHIP_DECLARE and platform drivers should be mutually exclusive in the same driver:
if you can delay the probing and have it as a proper platform device, then this should be the one true way.

Quoting Marc Zyngier (2020-06-27 02:37:47)

Quoted 12 lines

Quoted 10 lines · John Stultz

Quoted 5 lines · Stephen Boyd

Is there any reason to use IRQCHIP_DECLARE if this can work as a platform device driver?

Hey! Thanks so much for the review!

Quoted 8 lines

Quoted 2 lines

Mostly it was done this way to minimize the change in the non-module case. But if you'd rather avoid the #ifdefery I'll respin it without.

That would certainly be my own preference. In general, IRQCHIP_DECLARE and platform drivers should be mutually exclusive in the same driver:
if you can delay the probing and have it as a proper platform device, then this should be the one true way.

Does it work? I haven't looked in detail but I worry that the child irqdomain (i.e. pinctrl-msm) would need to delay probing until this parent irqdomain is registered. Or has the hierarchical irqdomain code been updated to handle the parent child relationship and wait for things to probe or be loaded?

Quoted 6 lines

Does it work? I haven't looked in detail but I worry that the child irqdomain (i.e. pinctrl-msm) would need to delay probing until this parent irqdomain is registered. Or has the hierarchical irqdomain code been updated to handle the parent child relationship and wait for things to probe or be loaded?

So I can't say I know the underlying hardware particularly well, but I've been using this successfully on the Dragonboard 845c with both static builds as well as module enabled builds.
And the same patch has been in the android-mainline and android-5.4 kernels for a while without objections from QCOM.

As to the probe ordering question, Saravana can maybe speak in more detail if it's involved in this case but the fw_devlink code has addressed many of these sorts of ordering issues.
However, I'm not sure if I'm lucking into the right probe order, as we have been able to boot android-mainline w/ both fw_devlink=on and fw_devlink=off (though in the =off case, we need deferred_probe_timeout=30 to give us a bit more time for modules to load after init starts).

Quoting John Stultz (2020-07-10 15:44:18)

Quoted 14 lines · Stephen Boyd

Quoted 6 lines

Does it work? I haven't looked in detail but I worry that the child irqdomain (i.e. pinctrl-msm) would need to delay probing until this parent irqdomain is registered. Or has the hierarchical irqdomain code been updated to handle the parent child relationship and wait for things to probe or be loaded?

So I can't say I know the underlying hardware particularly well, but I've been using this successfully on the Dragonboard 845c with both static builds as well as module enabled builds.
And the same patch has been in the android-mainline and android-5.4 kernels for a while without objections from QCOM.

Quoted 9 lines

As to the probe ordering question, Saravana can maybe speak in more detail if it's involved in this case but the fw_devlink code has addressed many of these sorts of ordering issues.
However, I'm not sure if I'm lucking into the right probe order, as we have been able to boot android-mainline w/ both fw_devlink=on and fw_devlink=off (though in the =off case, we need deferred_probe_timeout=30 to give us a bit more time for modules to load after init starts).

Ok I looked at the code (sorry for not checking earlier) and I see this in msm_gpio_init()

        np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
        if (np) {
                chip->irq.parent_domain = irq_find_matching_host(np,
                                                 DOMAIN_BUS_WAKEUP);
                of_node_put(np);
                if (!chip->irq.parent_domain)
                        return -EPROBE_DEFER;

so it looks like we'll probe defer the pinctrl driver until the pdc module loads. Meaning it should work to have pinctrl builtin and pdc as a module.

Allow the qcom_scm driver to be loadable as a permenent module.

Cc Andy Gross <[email protected]>

Cc Bjorn Andersson <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Maulik Shah <[email protected]>

Cc Lina Iyer <[email protected]>

Cc Saravana Kannan <[email protected]>

Cc Todd Kjos <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by John Stultz <[email protected]>

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Quoted 56 lines · John Stultz

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

Quoted 7 lines

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Yes, it looks ugly, but the part I really have issues with is that building QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC with the qcom implementation. I don't see why we need to enforce things here beyond making sure that all selectable permutations _build_ and fail gracefully at runtime on the qcom SoC if SCM isn't available.

Quoted 65 lines · Will Deacon

Quoted 56 lines · John Stultz

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

Quoted 7 lines

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Quoted 5 lines

Yes, it looks ugly, but the part I really have issues with is that building QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC with the qcom implementation. I don't see why we need to enforce things here beyond making sure that all selectable permutations _build_ and fail gracefully at runtime on the qcom SoC if SCM isn't available.

Ok. Thanks, that context/rationale makes sense to me now! I'll dig in and see if I can figure out the runtime get_symbol handling you suggested for the scm callout.

Quoted 56 lines · John Stultz

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

Quoted 7 lines

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Yes, it looks ugly, but the part I really have issues with is that building QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC with the qcom implementation. I don't see why we need to enforce things here beyond making sure that all selectable permutations _build_ and fail gracefully at runtime on the qcom SoC if SCM isn't available.

Quoted 65 lines · Will Deacon

Quoted 56 lines · John Stultz

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
      tristate "ARM Ltd. System MMU (SMMU) Support"
      depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
      select IOMMU_API
      select IOMMU_IO_PGTABLE_LPAE
      select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

        config ARM_SMMU_QCOM_IMPL
        default y
        #if QCOM_SCM=m this can't be =y
        depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

Quoted 7 lines

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Quoted 5 lines

Yes, it looks ugly, but the part I really have issues with is that building QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC with the qcom implementation. I don't see why we need to enforce things here beyond making sure that all selectable permutations _build_ and fail gracefully at runtime on the qcom SoC if SCM isn't available.

Hey Will,
Sorry to dredge up this old thread. I've been off busy with other
things and didn't get around to trying to rework this until now.

Unfortunately I'm still having some trouble coming up with a better solution. Initially I figured I'd rework the qcom_scm driver to, so that we have the various qcom_scm_* as inline functions, which call out to function pointers that the qcom_scm driver would register when the module loaded (Oof, and unfortunately there are a *ton* qcom_scm_* functions so its a bunch of churn).

The trouble I realized with that approach is that if the ARM_SMMU code is built in, then it may try to use the qcom_scm code before the module loads and sets those function pointers. So while it would build ok, the issue would be when the arm_smmu_device_reset() is done by done on arm_smmu_device_probe(), it wouldn't actually call the right code. There isn't a really good way to deal with the module loading at some random time after arm_smmu_device_probe() completes.

This is the benefit of the module symbol dependency tracking: If the arm_smmu.ko calls symbols in qcom_scm.ko then qcom_scm.ko has to load first.
But if arm_smmu is built in, I haven't found a clear mechanism to force qcom_scm to load before we probe, if it's configured as a module.

I also looked into the idea of reworking the arm-smmu-impl code to be modular instead, and while it does provide a similar method of using function pointers to minimize the amount of symbols the arm-smmu code needs to know about, the initialization call path is arm_smmu_device_probe -> arm_smmu_impl_init -> qcom_smmu_impl_init. So it doesn't really allow for dynamic registration of implementation modules at runtime.

So I'm sort of stewing on maybe trying to rework the directionality, so the arm-smmu-qcom.o code probes and calls arm_smmu_impl_init and that is what initializes the arm_smmu_device_probe logic?

Alternatively, I'm considering trying to switch the module dependency annotation so that the CONFIG_QCOM_SCM modularity depends on ARM_SMMU being a module. But that is sort of putting the restriction on the callee instead of the caller (sort of flipping the meaning of the depends), which feels prone to later trouble (and with multiple users of CONFIG_QCOM_SCM needing similar treatment, it would make it difficult to discover the right combination of configs needed to allow it to be a module).

Anyway, I wanted to reach out to see if you had any further ideas here. Sorry for letting such a large time gap pass!

Quoted 119 lines · John Stultz

Quoted 65 lines · Will Deacon

Quoted 56 lines · John Stultz

Quoted 42 lines · Will Deacon

Quoted 26 lines · John Stultz

Quoted 15 lines · Will Deacon

drivers/iommu/Kconfig config SPAPR_TCE_IOMMU · line 387

Quoted 12 lines · John Stultz

drivers/iommu/Kconfig +1 -0

index b510f67dfa49..714893535dd2 100644
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
  config ARM_SMMU
       tristate "ARM Ltd. System MMU (SMMU) Support"
       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
       select IOMMU_API
       select IOMMU_IO_PGTABLE_LPAE
       select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously isn't a good sign), but it's a fairly common way to ensure drivers don't get built in if they optionally depend on another driver that can be built as a module.
See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

Quoted 5 lines

I'm open to using a different method, and in a different thread you suggested using something like symbol_get(). I need to look into it more, but that approach looks even more messy and prone to runtime failures. Blocking the unwanted case at build time seems a bit cleaner to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this, as that driver _really_ doesn't care about SoC details like this. In other words, add a new entry along the lines of:

         config ARM_SMMU_QCOM_IMPL
         default y
         #if QCOM_SCM=m this can't be =y
         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

Quoted 5 lines

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init() which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o So if qcom_scm.o is part of a module, the calling code in arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU needs to be a module.

Quoted 7 lines

I know you said the arm-smmu driver doesn't care about SoC details, but the trouble is that currently the arm-smmu driver does directly call the qcom-scm code. So it is a real dependency. However, if QCOM_SCM is not configured, it calls stubs and that's ok. In that way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense. It looks terrible because we're used to boolean logic, but it's ternary.

Quoted 5 lines

Yes, it looks ugly, but the part I really have issues with is that building QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC with the qcom implementation. I don't see why we need to enforce things here beyond making sure that all selectable permutations _build_ and fail gracefully at runtime on the qcom SoC if SCM isn't available.

Hey Will,
Sorry to dredge up this old thread. I've been off busy with other
things and didn't get around to trying to rework this until now.

Unfortunately I'm still having some trouble coming up with a better solution. Initially I figured I'd rework the qcom_scm driver to, so that we have the various qcom_scm_* as inline functions, which call out to function pointers that the qcom_scm driver would register when the module loaded (Oof, and unfortunately there are a *ton* qcom_scm_* functions so its a bunch of churn).

The trouble I realized with that approach is that if the ARM_SMMU code is built in, then it may try to use the qcom_scm code before the module loads and sets those function pointers. So while it would build ok, the issue would be when the arm_smmu_device_reset() is done by done on arm_smmu_device_probe(), it wouldn't actually call the right code. There isn't a really good way to deal with the module loading at some random time after arm_smmu_device_probe() completes.

This is the benefit of the module symbol dependency tracking: If the arm_smmu.ko calls symbols in qcom_scm.ko then qcom_scm.ko has to load first.
But if arm_smmu is built in, I haven't found a clear mechanism to force qcom_scm to load before we probe, if it's configured as a module.

I also looked into the idea of reworking the arm-smmu-impl code to be modular instead, and while it does provide a similar method of using function pointers to minimize the amount of symbols the arm-smmu code needs to know about, the initialization call path is arm_smmu_device_probe -> arm_smmu_impl_init -> qcom_smmu_impl_init. So it doesn't really allow for dynamic registration of implementation modules at runtime.

So I'm sort of stewing on maybe trying to rework the directionality, so the arm-smmu-qcom.o code probes and calls arm_smmu_impl_init and that is what initializes the arm_smmu_device_probe logic?

Alternatively, I'm considering trying to switch the module dependency annotation so that the CONFIG_QCOM_SCM modularity depends on ARM_SMMU being a module. But that is sort of putting the restriction on the callee instead of the caller (sort of flipping the meaning of the depends), which feels prone to later trouble (and with multiple users of CONFIG_QCOM_SCM needing similar treatment, it would make it difficult to discover the right combination of configs needed to allow it to be a module).

Quoted 2 lines

Anyway, I wanted to reach out to see if you had any further ideas here. Sorry for letting such a large time gap pass!

Well we can always go with your original hack, if it helps?

https://lore.kernel.org/linux-iommu/20200714075603.GE4277@willie-the-truck/

Hmm, perhaps I'm missing something here, but even if the config options *do* line up, what prevents arm-smmu probing before qcom-scm and dereferencing NULL in qcom_scm_qsmmu500_wait_safe_toggle() before __scm is initialised?

Quoted 4 lines · Robin Murphy

Hmm, perhaps I'm missing something here, but even if the config options *do* line up, what prevents arm-smmu probing before qcom-scm and dereferencing NULL in qcom_scm_qsmmu500_wait_safe_toggle() before __scm is initialised?

Oh man, this spun me on a "wait, but how does it all work!" trip. :)

So in the non-module case, the qcom_scm driver is a subsys_initcall and the arm-smmu is a module_platform_driver, so the ordering works out.

In the module case, the arm-smmu code isn't loaded until the qcom_scm driver finishes probing due to the symbol dependency handling.

To double check this, I added a big msleep at the top of the qcom_scm_probe to try to open the race window you described, but the arm_smmu_device_probe() doesn't run until after qcom_scm_probe completes.

So at least as a built in / built in, or a module/module case its ok. And in the case where arm-smmu is a module and qcom_scm is built in that's ok too.

Its just the case my patch is trying to prevent is where arm-smmu is built in, but qcom_scm is a module that it can't work (due to build errors in missing symbols, or if we tried to use function pointers to plug in the qcom_scm - the lack of initialization ordering).

Hopefully that addresses your concern? Let me know if I'm still missing something.

Quoted 6 lines · John Stultz

Quoted 4 lines · Robin Murphy

Hmm, perhaps I'm missing something here, but even if the config options *do* line up, what prevents arm-smmu probing before qcom-scm and dereferencing NULL in qcom_scm_qsmmu500_wait_safe_toggle() before __scm is initialised?

Quoted 8 lines

Oh man, this spun me on a "wait, but how does it all work!" trip. :)

So in the non-module case, the qcom_scm driver is a subsys_initcall and the arm-smmu is a module_platform_driver, so the ordering works out.

In the module case, the arm-smmu code isn't loaded until the qcom_scm driver finishes probing due to the symbol dependency handling.

My point is that module load != driver probe. AFAICS you could disable drivers_autoprobe, load both modules, bind the SMMU to its driver first, and boom!

Quoted 4 lines

To double check this, I added a big msleep at the top of the qcom_scm_probe to try to open the race window you described, but the arm_smmu_device_probe() doesn't run until after qcom_scm_probe completes.

I don't think asynchronous probing is enabled by default, so indeed I would expect that to still happen to work ;)

Quoted 3 lines

So at least as a built in / built in, or a module/module case its ok. And in the case where arm-smmu is a module and qcom_scm is built in that's ok too.

In the built-in case, I'm sure it happens to work out similarly because the order of nodes in the DTB tends to be the order in which devices are autoprobed. Again, async probe might be enough to break things; manually binding drivers definitely should; moving the firmware node to the end of the DTB probably would as well.

Quoted 5 lines

Its just the case my patch is trying to prevent is where arm-smmu is built in, but qcom_scm is a module that it can't work (due to build errors in missing symbols, or if we tried to use function pointers to plug in the qcom_scm - the lack of initialization ordering).

Quoted 2 lines

Hopefully that addresses your concern? Let me know if I'm still missing something.

What I was dancing around is that the SCM API (and/or its users) appears to need a general way to tell whether SCM is ready or not, because the initialisation ordering problem is there anyway. Neither Kconfig nor the module loader can solve that.

Quoted 9 lines · John Stultz

Alternatively, I'm considering trying to switch the module dependency annotation so that the CONFIG_QCOM_SCM modularity depends on ARM_SMMU being a module. But that is sort of putting the restriction on the callee instead of the caller (sort of flipping the meaning of the depends), which feels prone to later trouble (and with multiple users of CONFIG_QCOM_SCM needing similar treatment, it would make it difficult to discover the right combination of configs needed to allow it to be a module).

Quoted 2 lines

Anyway, I wanted to reach out to see if you had any further ideas here. Sorry for letting such a large time gap pass!

Well we can always go with your original hack, if it helps?

https://lore.kernel.org/linux-iommu/20200714075603.GE4277@willie-the-truck/