[PATCH 1/2] mfd: fsl-imx25: set irq handler and data in one go

[PATCH 1/2] mfd: fsl-imx25: set irq handler and data in one go

[Martin Kaiser]

Replace the two separate calls for setting the irq handler and data with
a single irq_set_chained_handler_and_data() call.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/mfd/fsl-imx25-tsadc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index b3767c3..14189ef 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -84,8 +84,7 @@ static int mx25_tsadc_setup_irq(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	irq_set_chained_handler(irq, mx25_tsadc_irq_handler);
-	irq_set_handler_data(irq, tsadc);
+	irq_set_chained_handler_and_data(irq, mx25_tsadc_irq_handler, tsadc);
 
 	return 0;
 }
-- 
2.1.4

[PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser]

When fsl-imx25-tsadc is compiled as a module, unloading and reloading
the module will lead to a crash.

Add a removal function which clears the irq handler and removes the irq
domain. With this cleanup in place, it's possible to unload and reload
the module.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index 14189ef..a6423bd 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
 	return devm_of_platform_populate(dev);
 }
 
+static int __exit mx25_tsadc_remove(struct platform_device *pdev)
+{
+	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, NULL, NULL);
+		irq_domain_remove(tsadc->domain);
+	}
+
+	return 0;
+}
+
 static const struct of_device_id mx25_tsadc_ids[] = {
 	{ .compatible = "fsl,imx25-tsadc" },
 	{ /* Sentinel */ }
@@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
 		.of_match_table = of_match_ptr(mx25_tsadc_ids),
 	},
 	.probe = mx25_tsadc_probe,
+	.remove = __exit_p(mx25_tsadc_remove),
 };
 module_platform_driver(mx25_tsadc_driver);
 
-- 
2.1.4

Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Lee Jones]

On Tue, 12 Sep 2017, Martin Kaiser wrote:

> When fsl-imx25-tsadc is compiled as a module, unloading and reloading
> the module will lead to a crash.
> 
> Add a removal function which clears the irq handler and removes the irq
> domain. With this cleanup in place, it's possible to unload and reload
> the module.

More information please.

What is it specifically that causes the crash?

Why is this code not required?  What's stopping this causing a leak?

> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> index 14189ef..a6423bd 100644
> --- a/drivers/mfd/fsl-imx25-tsadc.c
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
>  	return devm_of_platform_populate(dev);
>  }
>  
> +static int __exit mx25_tsadc_remove(struct platform_device *pdev)
> +{
> +	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, NULL, NULL);
> +		irq_domain_remove(tsadc->domain);
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id mx25_tsadc_ids[] = {
>  	{ .compatible = "fsl,imx25-tsadc" },
>  	{ /* Sentinel */ }
> @@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
>  		.of_match_table = of_match_ptr(mx25_tsadc_ids),
>  	},
>  	.probe = mx25_tsadc_probe,
> +	.remove = __exit_p(mx25_tsadc_remove),
>  };
>  module_platform_driver(mx25_tsadc_driver);
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] mfd: fsl-imx25: set irq handler and data in one go

[Lee Jones]

On Tue, 12 Sep 2017, Martin Kaiser wrote:

> Replace the two separate calls for setting the irq handler and data with
> a single irq_set_chained_handler_and_data() call.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/mfd/fsl-imx25-tsadc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser]

Thus wrote Lee Jones (lee.jones@linaro.org):

> On Tue, 12 Sep 2017, Martin Kaiser wrote:

> > When fsl-imx25-tsadc is compiled as a module, unloading and reloading
> > the module will lead to a crash.

> > Add a removal function which clears the irq handler and removes the irq
> > domain. With this cleanup in place, it's possible to unload and reload
> > the module.

> More information please.

> What is it specifically that causes the crash?

I tested using a touchscreen and compiled both
drivers/input/touchscreen/fsl-imx25-tcq.c and
drivers/mfd/fsl-imx25-tsadc.c
as modules.

I got the crash after running
insmod fsl-imx25-tcq.ko
insmod fsl-imx25-tsadc.ko
rmmod fsl-imx25-tsadc.ko
insmod fsl-imx25-tsadc.ko

[  133.594246] Unable to handle kernel paging request at virtual address bf005430
[  133.601685] pgd = d3b90000
[  133.604435] [bf005430] *pgd=93b61811, *pte=00000000, *ppte=00000000
[  133.610902] Internal error: Oops: 7 [#1] ARM
[  133.615208] Modules linked in: fsl_imx25_tsadc(+) fsl_imx25_tcq [last unloaded: fsl_imx25_tsadc]
[  133.624078] CPU: 0 PID: 173 Comm: insmod Not tainted 4.13.0-next-20170911+ #132
[  133.631413] Hardware name: Freescale i.MX25 (Device Tree Support)
[  133.637537] task: d3b64000 task.stack: d3a64000
[  133.642131] PC is at irq_find_matching_fwspec+0x40/0x108
[  133.647484] LR is at irq_find_matching_fwspec+0x30/0x108
[  133.652826] pc : [<c004dfac>]    lr : [<c004df9c>]    psr: 20000013
[  133.659116] sp : d3a65bb0  ip : d3a65bb0  fp : d3a65bd4
[  133.664362] r10: 00000000  r9 : c03cac4c  r8 : d3a65c20
[  133.669612] r7 : 00000000  r6 : c0ab5a58  r5 : d3d700dc  r4 : d3b80000
[  133.676167] r3 : bf00542c  r2 : 40000013  r1 : d3b64000  r0 : c0ab5a4c
[  133.682721] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  133.689883] Control: 0005317f  Table: 93b90000  DAC: 00000051
[  133.695655] Process insmod (pid: 173, stack limit = 0xd3a64190)
...
[  133.993772] Backtrace:
[  133.996307] [<c004df6c>] (irq_find_matching_fwspec) from [<c028d5ec>] (of_irq_get+0x58/0x74)
[  134.004798]  r9:d3d737ec r8:d3948000 r7:d3948010 r6:d3b6cb10 r5:00000000 r4:d3d700dc
[  134.012607] [<c028d594>] (of_irq_get) from [<c01ff970>] (platform_get_irq+0x48/0xc8)
[  134.020375]  r4:d3948000
[  134.022990] [<c01ff928>] (platform_get_irq) from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
[  134.033019]  r5:00000012 r4:00000000
[  134.036663] [<bf00e11c>] (mx25_tsadc_probe [fsl_imx25_tsadc]) from [<c01ff658>] (platform_drv_probe+0x60/0xb0)
[  134.046711]  r9:00000008 r8:00000000 r7:c0b15680 r6:bf00e7b0 r5:d3948010 r4:bf00e11c
[  134.054504] [<c01ff5f8>] (platform_drv_probe) from [<c01fd5fc>] (driver_probe_device+0x204/0x470)
[  134.063414]  r6:00000000 r5:bf00e7b0 r4:d3948010 r3:c01ff5f8
[  134.069119] [<c01fd3f8>] (driver_probe_device) from [<c01fd940>] (__driver_attach+0xd8/0x118)
[  134.077691]  r9:bf00e84c r8:c0af6078 r7:c0ad8ee0 r6:bf00e7b0 r5:d3948044 r4:d3948010
[  134.085483] [<c01fd868>] (__driver_attach) from [<c01fb734>] (bus_for_each_dev+0x7c/0xa0)


irq_find_matching_fwspec() loops over all registered irq domains, I guess the
crash happens inside this loop. The irq domain is still registered from last
time the module was loaded but the pointer to its operations is invalid after
the module was unloaded.

My hope was that removing the irq domain along with module removal would fix
the crash in a proper way.

> Why is this code not required?  What's stopping this causing a leak?

Not sure I got your point here.

My assumption is that (at least for platform drivers), the removal function is
called only when the previous probe was successful. platform_get_irq() should
then get us the same irq we got in the probe function and we can remove the
handler and the irq domain.

Needless to say, I'm happy to update and re-test the patch if required.

Best regards,

   Martin

Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Lee Jones]

On Thu, 14 Sep 2017, Martin Kaiser wrote:

> Thus wrote Lee Jones (lee.jones@linaro.org):
> 
> > On Tue, 12 Sep 2017, Martin Kaiser wrote:
> 
> > > When fsl-imx25-tsadc is compiled as a module, unloading and reloading
> > > the module will lead to a crash.
> 
> > > Add a removal function which clears the irq handler and removes the irq
> > > domain. With this cleanup in place, it's possible to unload and reload
> > > the module.
> 
> > More information please.
> 
> > What is it specifically that causes the crash?
> 
> I tested using a touchscreen and compiled both
> drivers/input/touchscreen/fsl-imx25-tcq.c and
> drivers/mfd/fsl-imx25-tsadc.c
> as modules.
> 
> I got the crash after running
> insmod fsl-imx25-tcq.ko
> insmod fsl-imx25-tsadc.ko
> rmmod fsl-imx25-tsadc.ko
> insmod fsl-imx25-tsadc.ko
> 
> [  133.594246] Unable to handle kernel paging request at virtual address bf005430
> [  133.601685] pgd = d3b90000
> [  133.604435] [bf005430] *pgd=93b61811, *pte=00000000, *ppte=00000000
> [  133.610902] Internal error: Oops: 7 [#1] ARM
> [  133.615208] Modules linked in: fsl_imx25_tsadc(+) fsl_imx25_tcq [last unloaded: fsl_imx25_tsadc]
> [  133.624078] CPU: 0 PID: 173 Comm: insmod Not tainted 4.13.0-next-20170911+ #132
> [  133.631413] Hardware name: Freescale i.MX25 (Device Tree Support)
> [  133.637537] task: d3b64000 task.stack: d3a64000
> [  133.642131] PC is at irq_find_matching_fwspec+0x40/0x108
> [  133.647484] LR is at irq_find_matching_fwspec+0x30/0x108
> [  133.652826] pc : [<c004dfac>]    lr : [<c004df9c>]    psr: 20000013
> [  133.659116] sp : d3a65bb0  ip : d3a65bb0  fp : d3a65bd4
> [  133.664362] r10: 00000000  r9 : c03cac4c  r8 : d3a65c20
> [  133.669612] r7 : 00000000  r6 : c0ab5a58  r5 : d3d700dc  r4 : d3b80000
> [  133.676167] r3 : bf00542c  r2 : 40000013  r1 : d3b64000  r0 : c0ab5a4c
> [  133.682721] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [  133.689883] Control: 0005317f  Table: 93b90000  DAC: 00000051
> [  133.695655] Process insmod (pid: 173, stack limit = 0xd3a64190)
> ...
> [  133.993772] Backtrace:
> [  133.996307] [<c004df6c>] (irq_find_matching_fwspec) from [<c028d5ec>] (of_irq_get+0x58/0x74)
> [  134.004798]  r9:d3d737ec r8:d3948000 r7:d3948010 r6:d3b6cb10 r5:00000000 r4:d3d700dc
> [  134.012607] [<c028d594>] (of_irq_get) from [<c01ff970>] (platform_get_irq+0x48/0xc8)
> [  134.020375]  r4:d3948000
> [  134.022990] [<c01ff928>] (platform_get_irq) from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
> [  134.033019]  r5:00000012 r4:00000000
> [  134.036663] [<bf00e11c>] (mx25_tsadc_probe [fsl_imx25_tsadc]) from [<c01ff658>] (platform_drv_probe+0x60/0xb0)
> [  134.046711]  r9:00000008 r8:00000000 r7:c0b15680 r6:bf00e7b0 r5:d3948010 r4:bf00e11c
> [  134.054504] [<c01ff5f8>] (platform_drv_probe) from [<c01fd5fc>] (driver_probe_device+0x204/0x470)
> [  134.063414]  r6:00000000 r5:bf00e7b0 r4:d3948010 r3:c01ff5f8
> [  134.069119] [<c01fd3f8>] (driver_probe_device) from [<c01fd940>] (__driver_attach+0xd8/0x118)
> [  134.077691]  r9:bf00e84c r8:c0af6078 r7:c0ad8ee0 r6:bf00e7b0 r5:d3948044 r4:d3948010
> [  134.085483] [<c01fd868>] (__driver_attach) from [<c01fb734>] (bus_for_each_dev+0x7c/0xa0)
> 
> 
> irq_find_matching_fwspec() loops over all registered irq domains, I guess the
> crash happens inside this loop. The irq domain is still registered from last
> time the module was loaded but the pointer to its operations is invalid after
> the module was unloaded.

Great.  Please condense this information as much as possible and place
it in the commit log.

> My hope was that removing the irq domain along with module removal would fix
> the crash in a proper way.
> 
> > Why is this code not required?  What's stopping this causing a leak?
> 
> Not sure I got your point here.

Never mind.  This was due to an incorrect assumption I made.

You can ignore this.

> My assumption is that (at least for platform drivers), the removal function is
> called only when the previous probe was successful. platform_get_irq() should
> then get us the same irq we got in the probe function and we can remove the
> handler and the irq domain.
> 
> Needless to say, I'm happy to update and re-test the patch if required.
> 
> Best regards,
> 
>    Martin

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 1/2] mfd: fsl-imx25: set irq handler and data in one go

[Martin Kaiser]

Replace the two separate calls for setting the irq handler and data with
a single irq_set_chained_handler_and_data() call.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
changes in v2: none

 drivers/mfd/fsl-imx25-tsadc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index b3767c3..14189ef 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -84,8 +84,7 @@ static int mx25_tsadc_setup_irq(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	irq_set_chained_handler(irq, mx25_tsadc_irq_handler);
-	irq_set_handler_data(irq, tsadc);
+	irq_set_chained_handler_and_data(irq, mx25_tsadc_irq_handler, tsadc);
 
 	return 0;
 }
-- 
2.1.4

[PATCH v2 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser]

When fsl-imx25-tsadc is compiled as a module, loading, unloading and
reloading the module will lead to a crash.

Unable to handle kernel paging request at virtual address bf005430
[<c004df6c>] (irq_find_matching_fwspec)
   from [<c028d5ec>] (of_irq_get+0x58/0x74)
[<c028d594>] (of_irq_get)
   from [<c01ff970>] (platform_get_irq+0x48/0xc8)
[<c01ff928>] (platform_get_irq)
   from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])

irq_find_matching_fwspec() loops over all registered irq domains. The
irq domain is still registered from last time the module was loaded but
the pointer to its operations is invalid after the module was unloaded.

Add a removal function which clears the irq handler and removes the irq
domain. With this cleanup in place, it's possible to unload and reload
the module.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
changes in v2:
 add more details about the crash to the commit message

 drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index 14189ef..a6423bd 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
 	return devm_of_platform_populate(dev);
 }
 
+static int __exit mx25_tsadc_remove(struct platform_device *pdev)
+{
+	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, NULL, NULL);
+		irq_domain_remove(tsadc->domain);
+	}
+
+	return 0;
+}
+
 static const struct of_device_id mx25_tsadc_ids[] = {
 	{ .compatible = "fsl,imx25-tsadc" },
 	{ /* Sentinel */ }
@@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
 		.of_match_table = of_match_ptr(mx25_tsadc_ids),
 	},
 	.probe = mx25_tsadc_probe,
+	.remove = __exit_p(mx25_tsadc_remove),
 };
 module_platform_driver(mx25_tsadc_driver);
 
-- 
2.1.4

Re: [PATCH v2 1/2] mfd: fsl-imx25: set irq handler and data in one go

[Lucas Stach]

Am Dienstag, den 19.09.2017, 08:23 +0200 schrieb Martin Kaiser:
> Replace the two separate calls for setting the irq handler and data with
> a single irq_set_chained_handler_and_data() call.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> changes in v2: none
> 
>  drivers/mfd/fsl-imx25-tsadc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> index b3767c3..14189ef 100644
> --- a/drivers/mfd/fsl-imx25-tsadc.c
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -84,8 +84,7 @@ static int mx25_tsadc_setup_irq(struct platform_device *pdev,
>  		return -ENOMEM;
>  	}
>  
> -	irq_set_chained_handler(irq, mx25_tsadc_irq_handler);
> -	irq_set_handler_data(irq, tsadc);
> +	irq_set_chained_handler_and_data(irq, mx25_tsadc_irq_handler, tsadc);
>  
>  	return 0;
>  }

Re: [PATCH v2 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Lucas Stach]

Am Dienstag, den 19.09.2017, 08:23 +0200 schrieb Martin Kaiser:
> When fsl-imx25-tsadc is compiled as a module, loading, unloading and
> reloading the module will lead to a crash.
> 
> Unable to handle kernel paging request at virtual address bf005430
> [<c004df6c>] (irq_find_matching_fwspec)
>    from [<c028d5ec>] (of_irq_get+0x58/0x74)
> [<c028d594>] (of_irq_get)
>    from [<c01ff970>] (platform_get_irq+0x48/0xc8)
> [<c01ff928>] (platform_get_irq)
>    from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
> 
> irq_find_matching_fwspec() loops over all registered irq domains. The
> irq domain is still registered from last time the module was loaded but
> the pointer to its operations is invalid after the module was unloaded.
> 
> Add a removal function which clears the irq handler and removes the irq
> domain. With this cleanup in place, it's possible to unload and reload
> the module.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> changes in v2:
>  add more details about the crash to the commit message
> 
>  drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> index 14189ef..a6423bd 100644
> --- a/drivers/mfd/fsl-imx25-tsadc.c
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
>  	return devm_of_platform_populate(dev);
>  }
>  
> +static int __exit mx25_tsadc_remove(struct platform_device *pdev)

I don't think the __exit annotation makes sense. You can also unbind the
device by using sysfs, in which case you want to do the same IRQ
cleanup.

> +{
> +	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, NULL, NULL);
> +		irq_domain_remove(tsadc->domain);
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id mx25_tsadc_ids[] = {
>  	{ .compatible = "fsl,imx25-tsadc" },
>  	{ /* Sentinel */ }
> @@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
>  		.of_match_table = of_match_ptr(mx25_tsadc_ids),
>  	},
>  	.probe = mx25_tsadc_probe,
> +	.remove = __exit_p(mx25_tsadc_remove),
>  };
>  module_platform_driver(mx25_tsadc_driver);
>  

[PATCH v3 1/2] mfd: fsl-imx25: set irq handler and data in one go

[Martin Kaiser]

Replace the two separate calls for setting the irq handler and data with
a single irq_set_chained_handler_and_data() call.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
changes in v3: none
changes in v2: none

 drivers/mfd/fsl-imx25-tsadc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index b3767c3..14189ef 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -84,8 +84,7 @@ static int mx25_tsadc_setup_irq(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	irq_set_chained_handler(irq, mx25_tsadc_irq_handler);
-	irq_set_handler_data(irq, tsadc);
+	irq_set_chained_handler_and_data(irq, mx25_tsadc_irq_handler, tsadc);
 
 	return 0;
 }
-- 
2.1.4

[PATCH v3 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser]

When fsl-imx25-tsadc is compiled as a module, loading, unloading and
reloading the module will lead to a crash.

Unable to handle kernel paging request at virtual address bf005430
[<c004df6c>] (irq_find_matching_fwspec)
   from [<c028d5ec>] (of_irq_get+0x58/0x74)
[<c028d594>] (of_irq_get)
   from [<c01ff970>] (platform_get_irq+0x48/0xc8)
[<c01ff928>] (platform_get_irq)
   from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])

irq_find_matching_fwspec() loops over all registered irq domains. The
irq domain is still registered from last time the module was loaded but
the pointer to its operations is invalid after the module was unloaded.

Add a removal function which clears the irq handler and removes the irq
domain. With this cleanup in place, it's possible to unload and reload
the module.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
changes in v3:
 delete__exit qualifier from the remove routine

changes in v2:
 add more details about the crash to the commit message

 drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index 14189ef..dbb85ca 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
 	return devm_of_platform_populate(dev);
 }
 
+static int mx25_tsadc_remove(struct platform_device *pdev)
+{
+	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, NULL, NULL);
+		irq_domain_remove(tsadc->domain);
+	}
+
+	return 0;
+}
+
 static const struct of_device_id mx25_tsadc_ids[] = {
 	{ .compatible = "fsl,imx25-tsadc" },
 	{ /* Sentinel */ }
@@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
 		.of_match_table = of_match_ptr(mx25_tsadc_ids),
 	},
 	.probe = mx25_tsadc_probe,
+	.remove = mx25_tsadc_remove,
 };
 module_platform_driver(mx25_tsadc_driver);
 
-- 
2.1.4

Re: [PATCH v3 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Lucas Stach]

Am Dienstag, den 19.09.2017, 14:38 +0200 schrieb Martin Kaiser:
> When fsl-imx25-tsadc is compiled as a module, loading, unloading and
> reloading the module will lead to a crash.
> 
> Unable to handle kernel paging request at virtual address bf005430
> [<c004df6c>] (irq_find_matching_fwspec)
>    from [<c028d5ec>] (of_irq_get+0x58/0x74)
> [<c028d594>] (of_irq_get)
>    from [<c01ff970>] (platform_get_irq+0x48/0xc8)
> [<c01ff928>] (platform_get_irq)
>    from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
> 
> irq_find_matching_fwspec() loops over all registered irq domains. The
> irq domain is still registered from last time the module was loaded but
> the pointer to its operations is invalid after the module was unloaded.
> 
> Add a removal function which clears the irq handler and removes the irq
> domain. With this cleanup in place, it's possible to unload and reload
> the module.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> changes in v3:
>  delete__exit qualifier from the remove routine
> 
> changes in v2:
>  add more details about the crash to the commit message
> 
>  drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> index 14189ef..dbb85ca 100644
> --- a/drivers/mfd/fsl-imx25-tsadc.c
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
>  	return devm_of_platform_populate(dev);
>  }
>  
> +static int mx25_tsadc_remove(struct platform_device *pdev)
> +{
> +	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, NULL, NULL);
> +		irq_domain_remove(tsadc->domain);
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id mx25_tsadc_ids[] = {
>  	{ .compatible = "fsl,imx25-tsadc" },
>  	{ /* Sentinel */ }
> @@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
>  		.of_match_table = of_match_ptr(mx25_tsadc_ids),
>  	},
>  	.probe = mx25_tsadc_probe,
> +	.remove = mx25_tsadc_remove,
>  };
>  module_platform_driver(mx25_tsadc_driver);
>  

[PATCH RESEND v3 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser]

When fsl-imx25-tsadc is compiled as a module, loading, unloading and
reloading the module will lead to a crash.

Unable to handle kernel paging request at virtual address bf005430
[<c004df6c>] (irq_find_matching_fwspec)
   from [<c028d5ec>] (of_irq_get+0x58/0x74)
[<c028d594>] (of_irq_get)
   from [<c01ff970>] (platform_get_irq+0x48/0xc8)
[<c01ff928>] (platform_get_irq)
   from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])

irq_find_matching_fwspec() loops over all registered irq domains. The
irq domain is still registered from last time the module was loaded but
the pointer to its operations is invalid after the module was unloaded.

Add a removal function which clears the irq handler and removes the irq
domain. With this cleanup in place, it's possible to unload and reload
the module.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
Hello Lee,

thanks for merging the first patch of this series. Any chance you can
have a look at this one as well? Please let me know if you want me to
make any changes.

Best regards,
Martin 

changes in v3:
 delete__exit qualifier from the remove routine

changes in v2:
 add more details about the crash to the commit message

 drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
index 14189ef..dbb85ca 100644
--- a/drivers/mfd/fsl-imx25-tsadc.c
+++ b/drivers/mfd/fsl-imx25-tsadc.c
@@ -179,6 +179,19 @@ static int mx25_tsadc_probe(struct platform_device *pdev)
 	return devm_of_platform_populate(dev);
 }
 
+static int mx25_tsadc_remove(struct platform_device *pdev)
+{
+	struct mx25_tsadc *tsadc = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, NULL, NULL);
+		irq_domain_remove(tsadc->domain);
+	}
+
+	return 0;
+}
+
 static const struct of_device_id mx25_tsadc_ids[] = {
 	{ .compatible = "fsl,imx25-tsadc" },
 	{ /* Sentinel */ }
@@ -191,6 +204,7 @@ static struct platform_driver mx25_tsadc_driver = {
 		.of_match_table = of_match_ptr(mx25_tsadc_ids),
 	},
 	.probe = mx25_tsadc_probe,
+	.remove = mx25_tsadc_remove,
 };
 module_platform_driver(mx25_tsadc_driver);
 
-- 
2.1.4

Re: [PATCH RESEND v3 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Lee Jones]

On Tue, 17 Oct 2017, Martin Kaiser wrote:

> When fsl-imx25-tsadc is compiled as a module, loading, unloading and
> reloading the module will lead to a crash.
> 
> Unable to handle kernel paging request at virtual address bf005430
> [<c004df6c>] (irq_find_matching_fwspec)
>    from [<c028d5ec>] (of_irq_get+0x58/0x74)
> [<c028d594>] (of_irq_get)
>    from [<c01ff970>] (platform_get_irq+0x48/0xc8)
> [<c01ff928>] (platform_get_irq)
>    from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
> 
> irq_find_matching_fwspec() loops over all registered irq domains. The
> irq domain is still registered from last time the module was loaded but
> the pointer to its operations is invalid after the module was unloaded.
> 
> Add a removal function which clears the irq handler and removes the irq
> domain. With this cleanup in place, it's possible to unload and reload
> the module.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> Hello Lee,
> 
> thanks for merging the first patch of this series. Any chance you can
> have a look at this one as well? Please let me know if you want me to
> make any changes.
> 
> Best regards,
> Martin 
> 
> changes in v3:
>  delete__exit qualifier from the remove routine
> 
> changes in v2:
>  add more details about the crash to the commit message
> 
>  drivers/mfd/fsl-imx25-tsadc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Applied, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog