[PATCH 1/5] irqchip/gic-pm: add driver remove support

[PATCH 1/5] irqchip/gic-pm: add driver remove support

[Sameer Pujar]

This is a preparatory patch for using irq-gic-pm driver as module and thus
implement remove() call for the driver. Details of remove() are as below,

 * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
   following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
 * Function gic_teardown() is exported from gic driver and called in remove
   to perform io unmap.
 * pm_clk_destroy() to free clock resources
 * irq is unmapped and freed with irq_dispose_mapping()

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 drivers/irqchip/irq-gic-pm.c    | 37 ++++++++++++++++++++++++++++++++-----
 drivers/irqchip/irq-gic.c       |  3 ++-
 include/linux/irqchip/arm-gic.h |  2 ++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index ecafd29..56a785f 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -28,9 +28,15 @@ struct gic_clk_data {
 	const char *const *clocks;
 };
 
+struct gic_chip_pm {
+	struct gic_chip_data *chip_data;
+	int irq;
+};
+
 static int gic_runtime_resume(struct device *dev)
 {
-	struct gic_chip_data *gic = dev_get_drvdata(dev);
+	struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
+	struct gic_chip_data *gic = chip_pm->chip_data;
 	int ret;
 
 	ret = pm_clk_resume(dev);
@@ -54,7 +60,8 @@ static int gic_runtime_resume(struct device *dev)
 
 static int gic_runtime_suspend(struct device *dev)
 {
-	struct gic_chip_data *gic = dev_get_drvdata(dev);
+	struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
+	struct gic_chip_data *gic = chip_pm->chip_data;
 
 	gic_dist_save(gic);
 	gic_cpu_save(gic);
@@ -91,9 +98,14 @@ static int gic_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct gic_clk_data *data;
-	struct gic_chip_data *gic;
+	struct gic_chip_pm *chip_pm;
 	int ret, irq;
 
+	chip_pm = devm_kzalloc(dev, sizeof(struct gic_chip_pm),
+			       GFP_KERNEL);
+	if (!chip_pm)
+		return -ENOMEM;
+
 	data = of_device_get_match_data(&pdev->dev);
 	if (!data) {
 		dev_err(&pdev->dev, "no device match found\n");
@@ -116,14 +128,15 @@ static int gic_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto rpm_disable;
 
-	ret = gic_of_init_child(dev, &gic, irq);
+	ret = gic_of_init_child(dev, &chip_pm->chip_data, irq);
 	if (ret)
 		goto rpm_put;
 
-	platform_set_drvdata(pdev, gic);
+	platform_set_drvdata(pdev, chip_pm);
 
 	pm_runtime_put(dev);
 
+	chip_pm->irq = irq;
 	dev_info(dev, "GIC IRQ controller registered\n");
 
 	return 0;
@@ -139,6 +152,19 @@ static int gic_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int gic_remove(struct platform_device *pdev)
+{
+	struct gic_chip_pm *chip_pm = dev_get_drvdata(&pdev->dev);
+	struct gic_chip_data *gic = chip_pm->chip_data;
+
+	pm_runtime_force_suspend(&pdev->dev);
+	gic_teardown(gic);
+	pm_clk_destroy(&pdev->dev);
+	irq_dispose_mapping(chip_pm->irq);
+
+	return 0;
+}
+
 static const struct dev_pm_ops gic_pm_ops = {
 	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
 			   gic_runtime_resume, NULL)
@@ -161,6 +187,7 @@ MODULE_DEVICE_TABLE(of, gic_match);
 
 static struct platform_driver gic_driver = {
 	.probe		= gic_probe,
+	.remove		= gic_remove,
 	.driver		= {
 		.name	= "gic",
 		.of_match_table	= gic_match,
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ba2a37a..f88018b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1259,7 +1259,7 @@ void __init gic_init(unsigned int gic_nr, int irq_start,
 	__gic_init_bases(gic, irq_start, NULL);
 }
 
-static void gic_teardown(struct gic_chip_data *gic)
+void gic_teardown(struct gic_chip_data *gic)
 {
 	if (WARN_ON(!gic))
 		return;
@@ -1269,6 +1269,7 @@ static void gic_teardown(struct gic_chip_data *gic)
 	if (gic->raw_cpu_base)
 		iounmap(gic->raw_cpu_base);
 }
+EXPORT_SYMBOL_GPL(gic_teardown);
 
 #ifdef CONFIG_OF
 static int gic_cnt __initdata;
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 6261790..f790232 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -161,6 +161,8 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq);
 void gic_init(unsigned int nr, int start,
 	      void __iomem *dist , void __iomem *cpu);
 
+void gic_teardown(struct gic_chip_data *gic);
+
 int gicv2m_init(struct fwnode_handle *parent_handle,
 		struct irq_domain *parent);
 
-- 
2.7.4

[PATCH 2/5] irqchip/gic: allow gic-pm driver to be used as module

[Sameer Pujar]

In order to use irq-gic-pm driver as module following changes are made.

1. config ARM_GIC_PM is changed to tristate to allow it to be built as
   module.

2. following functions are exported from irq-gic driver.
   * gic_of_init_child()
   * gic_cpu_save()
   * gic_dist_save()
   * gic_cpu_restore()
   * gic_dist_restore()

3. MODULE_LICENSE is added to make sure driver load is successful, else
   it fails to resolve required symbols. The 'builtin_platform_driver' is
   replaced with 'module_platform_driver' to allow driver removal, else it
   complains that device or resource is busy.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 drivers/irqchip/Kconfig      | 2 +-
 drivers/irqchip/irq-gic-pm.c | 3 ++-
 drivers/irqchip/irq-gic.c    | 6 ++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5438abb..8b5ce12 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -12,7 +12,7 @@ config ARM_GIC
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
 
 config ARM_GIC_PM
-	bool
+	tristate "ARM GIC with PM support"
 	depends on PM
 	select ARM_GIC
 	select PM_CLK
diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index 56a785f..cde9714 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -195,4 +195,5 @@ static struct platform_driver gic_driver = {
 	}
 };
 
-builtin_platform_driver(gic_driver);
+module_platform_driver(gic_driver);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index f88018b..b3ad7ed 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -600,6 +600,7 @@ void gic_dist_save(struct gic_chip_data *gic)
 		gic->saved_spi_active[i] =
 			readl_relaxed(dist_base + GIC_DIST_ACTIVE_SET + i * 4);
 }
+EXPORT_SYMBOL_GPL(gic_dist_save);
 
 /*
  * Restores the GIC distributor registers during resume or when coming out of
@@ -653,6 +654,7 @@ void gic_dist_restore(struct gic_chip_data *gic)
 
 	writel_relaxed(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
 }
+EXPORT_SYMBOL_GPL(gic_dist_restore);
 
 void gic_cpu_save(struct gic_chip_data *gic)
 {
@@ -683,6 +685,7 @@ void gic_cpu_save(struct gic_chip_data *gic)
 		ptr[i] = readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
 
 }
+EXPORT_SYMBOL_GPL(gic_cpu_save);
 
 void gic_cpu_restore(struct gic_chip_data *gic)
 {
@@ -725,6 +728,7 @@ void gic_cpu_restore(struct gic_chip_data *gic)
 	writel_relaxed(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
 	gic_cpu_if_up(gic);
 }
+EXPORT_SYMBOL_GPL(gic_cpu_restore);
 
 static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
 {
@@ -1410,6 +1414,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gic_of_init_child);
 
 static void __init gic_of_setup_kvm_info(struct device_node *node)
 {
@@ -1496,6 +1501,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
 {
 	return -ENOTSUPP;
 }
+EXPORT_SYMBOL_GPL(gic_of_init_child);
 #endif
 
 #ifdef CONFIG_ACPI
-- 
2.7.4

[PATCH 3/5] arm64: defconfig: build gic-pm driver as module

[Sameer Pujar]

Following config/driver is enabled as module,
 - ARM_GIC_PM (drivers/irqchip/irq-gic-pm.c)

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index e9d514c..e919804 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -715,6 +715,7 @@ CONFIG_PWM_ROCKCHIP=y
 CONFIG_PWM_SAMSUNG=y
 CONFIG_PWM_SUN4I=m
 CONFIG_PWM_TEGRA=m
+CONFIG_ARM_GIC_PM=m
 CONFIG_RESET_TI_SCI=y
 CONFIG_PHY_XGENE=y
 CONFIG_PHY_SUN4I_USB=y
-- 
2.7.4

[PATCH 4/5] irqchip/gic-pm: use devm_clk_*() helpers

[Sameer Pujar]

irq-gic-pm driver is using pm_clk_*() interface to manage clock resources.
With this, clocks always remain ON. This happens on Tegra devices which use
BPMP co-processor to manage clocks, where clocks are enabled during prepare
phase. This is necessary because calls to BPMP are always blocking. When
pm_clk_*() interface is used on such devices, clock prepare count is not
balanced till driver remove() gets executed and hence clocks are seen ON
always. This patch replaces pm_clk_*() with devm_clk_*() framework.

Suggested-by: Mohan Kumar D <mkumard@nvidia.com>
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic-pm.c | 52 +++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index cde9714..b5405df 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -19,7 +19,6 @@
 #include <linux/of_irq.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/platform_device.h>
-#include <linux/pm_clock.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
@@ -30,6 +29,8 @@ struct gic_clk_data {
 
 struct gic_chip_pm {
 	struct gic_chip_data *chip_data;
+	const struct gic_clk_data *clk_data;
+	struct clk **clk_handle;
 	int irq;
 };
 
@@ -37,11 +38,19 @@ static int gic_runtime_resume(struct device *dev)
 {
 	struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
 	struct gic_chip_data *gic = chip_pm->chip_data;
-	int ret;
+	const struct gic_clk_data *data = chip_pm->clk_data;
+	int ret, i;
 
-	ret = pm_clk_resume(dev);
-	if (ret)
-		return ret;
+	for (i = 0; i < data->num_clocks; i++) {
+		ret = clk_prepare_enable(chip_pm->clk_handle[i]);
+		if (ret) {
+			while (--i >= 0)
+				clk_disable_unprepare(chip_pm->clk_handle[i]);
+
+			dev_err(dev, " clk_enable failed: %d\n", ret);
+			return ret;
+		}
+	}
 
 	/*
 	 * On the very first resume, the pointer to the driver data
@@ -62,32 +71,37 @@ static int gic_runtime_suspend(struct device *dev)
 {
 	struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
 	struct gic_chip_data *gic = chip_pm->chip_data;
+	const struct gic_clk_data *data = chip_pm->clk_data;
+	int i;
 
 	gic_dist_save(gic);
 	gic_cpu_save(gic);
 
-	return pm_clk_suspend(dev);
+	for (i = 0; i < data->num_clocks; i++)
+		clk_disable_unprepare(chip_pm->clk_handle[i]);
+
+	return 0;
 }
 
-static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
+static int gic_get_clocks(struct device *dev, struct gic_chip_pm *chip_pm)
 {
 	unsigned int i;
-	int ret;
+	const struct gic_clk_data *data = chip_pm->clk_data;
 
 	if (!dev || !data)
 		return -EINVAL;
 
-	ret = pm_clk_create(dev);
-	if (ret)
-		return ret;
+	chip_pm->clk_handle = devm_kzalloc(dev, data->num_clocks *
+					   sizeof(struct clk *), GFP_KERNEL);
+	if (!chip_pm->clk_handle)
+		return -ENOMEM;
 
 	for (i = 0; i < data->num_clocks; i++) {
-		ret = of_pm_clk_add_clk(dev, data->clocks[i]);
-		if (ret) {
+		chip_pm->clk_handle[i] = devm_clk_get(dev, data->clocks[i]);
+		if (IS_ERR(chip_pm->clk_handle[i])) {
 			dev_err(dev, "failed to add clock %s\n",
 				data->clocks[i]);
-			pm_clk_destroy(dev);
-			return ret;
+			return PTR_ERR(chip_pm->clk_handle[i]);
 		}
 	}
 
@@ -111,6 +125,8 @@ static int gic_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "no device match found\n");
 		return -ENODEV;
 	}
+	chip_pm->clk_data = data;
+	platform_set_drvdata(pdev, chip_pm);
 
 	irq = irq_of_parse_and_map(dev->of_node, 0);
 	if (!irq) {
@@ -118,7 +134,7 @@ static int gic_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = gic_get_clocks(dev, data);
+	ret = gic_get_clocks(dev, chip_pm);
 	if (ret)
 		goto irq_dispose;
 
@@ -132,8 +148,6 @@ static int gic_probe(struct platform_device *pdev)
 	if (ret)
 		goto rpm_put;
 
-	platform_set_drvdata(pdev, chip_pm);
-
 	pm_runtime_put(dev);
 
 	chip_pm->irq = irq;
@@ -145,7 +159,6 @@ static int gic_probe(struct platform_device *pdev)
 	pm_runtime_put_sync(dev);
 rpm_disable:
 	pm_runtime_disable(dev);
-	pm_clk_destroy(dev);
 irq_dispose:
 	irq_dispose_mapping(irq);
 
@@ -159,7 +172,6 @@ static int gic_remove(struct platform_device *pdev)
 
 	pm_runtime_force_suspend(&pdev->dev);
 	gic_teardown(gic);
-	pm_clk_destroy(&pdev->dev);
 	irq_dispose_mapping(chip_pm->irq);
 
 	return 0;
-- 
2.7.4

[PATCH 5/5] irqchip/gic-pm: fix suspend handling

[Sameer Pujar]

If interrupts are enabled for a non-root GIC device that uses the
gic-pm driver, when system suspend occurs, the current interrupt
state is not saved and restored correctly and so interrupts do not
work again on resuming the system. Add a late suspend handler to
save and restore the state for these devices.

Suggested-by: Jonathan Hunter <jonathanh@nvidia.com>
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 drivers/irqchip/irq-gic-pm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index b5405df..583858d 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -180,6 +180,8 @@ static int gic_remove(struct platform_device *pdev)
 static const struct dev_pm_ops gic_pm_ops = {
 	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
 			   gic_runtime_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
 };
 
 static const char * const gic400_clocks[] = {
-- 
2.7.4

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

First things first:

- Where is the cover letter?
- This series should be flagged as v2, as it not the same as the one you
sent last week.

On 13/03/2019 11:02, Sameer Pujar wrote:
> This is a preparatory patch for using irq-gic-pm driver as module and thus
> implement remove() call for the driver. Details of remove() are as below,
> 
>  * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
>    following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
>  * Function gic_teardown() is exported from gic driver and called in remove
>    to perform io unmap.
>  * pm_clk_destroy() to free clock resources
>  * irq is unmapped and freed with irq_dispose_mapping()
> 

Let's be clear, I have no desire to export any GIC symbol at all. Why
should we do this? This "driver" is the tiniest thing, and making it
modular doesn't get us anything.

So what's the rational for doing so?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Sameer Pujar]

On 3/13/2019 4:52 PM, Marc Zyngier wrote:
> First things first:
>
> - Where is the cover letter?
> - This series should be flagged as v2, as it not the same as the one you
> sent last week.
I had the dilemma whether to name this series as v2 or not, thought the 
commits
in the series are different and v2 may not be necessary.
Also felt commit messages are descriptive enough and all belong to 
irq-gic-pm,
hence did not send cover letter.
If you suggest so, I will send a cover letter next patch version(v2)
>
> On 13/03/2019 11:02, Sameer Pujar wrote:
>> This is a preparatory patch for using irq-gic-pm driver as module and thus
>> implement remove() call for the driver. Details of remove() are as below,
>>
>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
>>   * Function gic_teardown() is exported from gic driver and called in remove
>>     to perform io unmap.
>>   * pm_clk_destroy() to free clock resources
>>   * irq is unmapped and freed with irq_dispose_mapping()
>>
> Let's be clear, I have no desire to export any GIC symbol at all. Why
> should we do this? This "driver" is the tiniest thing, and making it
> modular doesn't get us anything.
>
> So what's the rational for doing so?
Reason for this was, the driver gets used for AGIC block and audio is not
boot critical and hence module option was preferred.

Thanks,
Sameer.

>
> Thanks,
>
> 	M.

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

On 13/03/2019 13:50, Sameer Pujar wrote:
> 
> On 3/13/2019 4:52 PM, Marc Zyngier wrote:
>> First things first:
>>
>> - Where is the cover letter?
>> - This series should be flagged as v2, as it not the same as the one you
>> sent last week.
> I had the dilemma whether to name this series as v2 or not, thought the 
> commits
> in the series are different and v2 may not be necessary.

This is an iteration on the same theme. Please always bump up the
counter. Better do it more often than not.

> Also felt commit messages are descriptive enough and all belong to 
> irq-gic-pm,
> hence did not send cover letter.
> If you suggest so, I will send a cover letter next patch version(v2)

You should always send a cover letter if you have more than a single patch.

>>
>> On 13/03/2019 11:02, Sameer Pujar wrote:
>>> This is a preparatory patch for using irq-gic-pm driver as module and thus
>>> implement remove() call for the driver. Details of remove() are as below,
>>>
>>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
>>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
>>>   * Function gic_teardown() is exported from gic driver and called in remove
>>>     to perform io unmap.
>>>   * pm_clk_destroy() to free clock resources
>>>   * irq is unmapped and freed with irq_dispose_mapping()
>>>
>> Let's be clear, I have no desire to export any GIC symbol at all. Why
>> should we do this? This "driver" is the tiniest thing, and making it
>> modular doesn't get us anything.
>>
>> So what's the rational for doing so?
> Reason for this was, the driver gets used for AGIC block and audio is not
> boot critical and hence module option was preferred.

Sure, but look at the result:

- you remove your gic-pm module
- the MMIO mapping disappears
- the GIC data structures *are still live*
- a driver does a disable_irq() on an interrupt routed to this block
(because nothing has taken the interrupts away, as far as the kernel is
concerned)
- ...
- profit! (or kernel panic, your choice)

Even better if something else in the system has mapped anything that
ends up in the same vmalloc range. Congratulations, you have now
corrupted unsuspecting memory. This reminds me of the e1000 corruption
bug. Great stuff.

So for the whole thing, NAK. You don't pull an irqchip from under the
kernel's feet.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:
> On 13/03/2019 13:50, Sameer Pujar wrote:
> > 
> > On 3/13/2019 4:52 PM, Marc Zyngier wrote:
> >> First things first:
> >>
> >> - Where is the cover letter?
> >> - This series should be flagged as v2, as it not the same as the one you
> >> sent last week.
> > I had the dilemma whether to name this series as v2 or not, thought the 
> > commits
> > in the series are different and v2 may not be necessary.
> 
> This is an iteration on the same theme. Please always bump up the
> counter. Better do it more often than not.
> 
> > Also felt commit messages are descriptive enough and all belong to 
> > irq-gic-pm,
> > hence did not send cover letter.
> > If you suggest so, I will send a cover letter next patch version(v2)
> 
> You should always send a cover letter if you have more than a single patch.
> 
> >>
> >> On 13/03/2019 11:02, Sameer Pujar wrote:
> >>> This is a preparatory patch for using irq-gic-pm driver as module and thus
> >>> implement remove() call for the driver. Details of remove() are as below,
> >>>
> >>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
> >>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
> >>>   * Function gic_teardown() is exported from gic driver and called in remove
> >>>     to perform io unmap.
> >>>   * pm_clk_destroy() to free clock resources
> >>>   * irq is unmapped and freed with irq_dispose_mapping()
> >>>
> >> Let's be clear, I have no desire to export any GIC symbol at all. Why
> >> should we do this? This "driver" is the tiniest thing, and making it
> >> modular doesn't get us anything.
> >>
> >> So what's the rational for doing so?
> > Reason for this was, the driver gets used for AGIC block and audio is not
> > boot critical and hence module option was preferred.
> 
> Sure, but look at the result:
> 
> - you remove your gic-pm module
> - the MMIO mapping disappears
> - the GIC data structures *are still live*
> - a driver does a disable_irq() on an interrupt routed to this block
> (because nothing has taken the interrupts away, as far as the kernel is
> concerned)
> - ...
> - profit! (or kernel panic, your choice)

I suppose we could use device links to model the dependency, but perhaps
it's not worth it for something like the GIC. The AGIC is somewhat of an
outlier because it serves a fairly encapsulated part of the system, so
it would be manageable to make it unloadable.

> Even better if something else in the system has mapped anything that
> ends up in the same vmalloc range. Congratulations, you have now
> corrupted unsuspecting memory. This reminds me of the e1000 corruption
> bug. Great stuff.

Can you elaborate on this point? How would unloading a driver cause
memory corruption in another driver's mapped memory? I've never heard of
this before, so I want to make sure I understand what to look out for in
the future.

> So for the whole thing, NAK. You don't pull an irqchip from under the
> kernel's feet.

Maybe we should also set the driver's .suppress_bind_attrs = true while
at it, to prevent anyone from trying to force unbind the driver via
sysfs?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

Hi Thierry,

On 13/03/2019 16:34, Thierry Reding wrote:
> On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:

[...]

>> Sure, but look at the result:
>>
>> - you remove your gic-pm module
>> - the MMIO mapping disappears
>> - the GIC data structures *are still live*
>> - a driver does a disable_irq() on an interrupt routed to this block
>> (because nothing has taken the interrupts away, as far as the kernel is
>> concerned)
>> - ...
>> - profit! (or kernel panic, your choice)
> 
> I suppose we could use device links to model the dependency, but perhaps
> it's not worth it for something like the GIC. The AGIC is somewhat of an
> outlier because it serves a fairly encapsulated part of the system, so
> it would be manageable to make it unloadable.

The problem is that the GIC is required so early that it cannot be a
real device (devices require sysfs, which requires the VFS, which has
indirect dependencies in the scheduler, which needs the timer, which
needs interrupts -- I've been pondering turning that upside down for
years, but life is too short).

> 
>> Even better if something else in the system has mapped anything that
>> ends up in the same vmalloc range. Congratulations, you have now
>> corrupted unsuspecting memory. This reminds me of the e1000 corruption
>> bug. Great stuff.
> 
> Can you elaborate on this point? How would unloading a driver cause
> memory corruption in another driver's mapped memory? I've never heard of
> this before, so I want to make sure I understand what to look out for in
> the future.

The MMIO ranges get their VA as part of the vmalloc space. If you free
that space (iounmap), you allow it to be reused by another mapping if
the hole you've created fits the new ioremap. Now the GIC driver (which,
remember, still has references to that VA space all over the place)
unexpectedly pokes into another driver's MMIO space.

But it can be worse. You could have loaded a module instead, which also
ends up in the vmalloc region. And now the GIC driver is writing to the
module code or data. Or anything that gets allocated using vmalloc.

> 
>> So for the whole thing, NAK. You don't pull an irqchip from under the
>> kernel's feet.
> 
> Maybe we should also set the driver's .suppress_bind_attrs = true while
> at it, to prevent anyone from trying to force unbind the driver via
> sysfs?

Would that prevent a 'modprobe -r'? With the current approach, anything
that allows the driver to be removed is potentially fatal.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...