[PATCH v1 1/4] spi/xilinx: Remove CONFIG_OF from the driver

[PATCH v1 1/4] spi/xilinx: Remove CONFIG_OF from the driver

[Michal Simek]

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

dev.of_node is in struct device all the time.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 35503b4..f3de5e5 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -375,7 +375,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		bits_per_word = pdata->bits_per_word;
 	}

-#ifdef CONFIG_OF
 	if (pdev->dev.of_node) {
 		const __be32 *prop;
 		int len;
@@ -386,7 +385,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		if (prop && len >= sizeof(*prop))
 			num_cs = __be32_to_cpup(prop);
 	}
-#endif

 	if (!num_cs) {
 		dev_err(&pdev->dev,
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v1 2/4] spi/xilinx: Clean ioremap calling

[Michal Simek]

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

devm_ioremap_resource() automatically checks that
struct resource is initialized.
Also group platform_get_resource() and devm_ioremap_resource()
together.
And remove mem resource from struct xilinx_spi.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index f3de5e5..c99c37b 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -80,7 +80,6 @@ struct xilinx_spi {
 	/* bitbang has to be first */
 	struct spi_bitbang bitbang;
 	struct completion done;
-	struct resource mem; /* phys mem */
 	void __iomem	*regs;	/* virt. address of the control registers */

 	u32		irq;
@@ -363,7 +362,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 {
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
-	struct resource *r;
+	struct resource *res;
 	int ret, irq, num_cs = 0, bits_per_word = 8;
 	struct spi_master *master;
 	u32 tmp;
@@ -392,10 +391,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}

-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r)
-		return -ENODEV;
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return -ENXIO;
@@ -415,7 +410,8 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	xspi->bitbang.master->setup = xilinx_spi_setup;
 	init_completion(&xspi->done);

-	xspi->regs = devm_ioremap_resource(&pdev->dev, r);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	xspi->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(xspi->regs)) {
 		ret = PTR_ERR(xspi->regs);
 		goto put_master;
@@ -425,7 +421,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	master->num_chipselect = num_cs;
 	master->dev.of_node = pdev->dev.of_node;

-	xspi->mem = *r;
 	xspi->irq = irq;

 	/*
@@ -477,7 +472,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	}

 	dev_info(&pdev->dev, "at 0x%08llX mapped to 0x%p, irq=%d\n",
-		(unsigned long long)r->start, xspi->regs, xspi->irq);
+		(unsigned long long)res->start, xspi->regs, xspi->irq);

 	if (pdata) {
 		for (i = 0; i < pdata->num_devices; i++)
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v1 2/4] spi/xilinx: Clean ioremap calling

[Mark Brown]

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

On Mon, Jul 08, 2013 at 03:29:15PM +0200, Michal Simek wrote:
> devm_ioremap_resource() automatically checks that
> struct resource is initialized.
> Also group platform_get_resource() and devm_ioremap_resource()
> together.
> And remove mem resource from struct xilinx_spi.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Michal Simek]

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

Use devm_request_irq() for irq allocation which
simplify driver code.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index c99c37b..a6bb5b0 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -363,7 +363,7 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	struct xilinx_spi *xspi;
 	struct xspi_platform_data *pdata;
 	struct resource *res;
-	int ret, irq, num_cs = 0, bits_per_word = 8;
+	int ret, num_cs = 0, bits_per_word = 8;
 	struct spi_master *master;
 	u32 tmp;
 	u8 i;
@@ -391,10 +391,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}

-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return -ENXIO;
-
 	master = spi_alloc_master(&pdev->dev, sizeof(struct xilinx_spi));
 	if (!master)
 		return -ENODEV;
@@ -421,8 +417,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	master->num_chipselect = num_cs;
 	master->dev.of_node = pdev->dev.of_node;

-	xspi->irq = irq;
-
 	/*
 	 * Detect endianess on the IP via loop bit in CR. Detection
 	 * must be done before reset is sent because incorrect reset
@@ -456,19 +450,25 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 		goto put_master;
 	}

-
 	/* SPI controller initializations */
 	xspi_init_hw(xspi);

+	xspi->irq = platform_get_irq(pdev, 0);
+	if (xspi->irq < 0) {
+		ret = xspi->irq;
+		goto put_master;
+	}
+
 	/* Register for SPI Interrupt */
-	ret = request_irq(xspi->irq, xilinx_spi_irq, 0, XILINX_SPI_NAME, xspi);
+	ret = devm_request_irq(&pdev->dev, xspi->irq, xilinx_spi_irq, 0,
+			       dev_name(&pdev->dev), xspi);
 	if (ret)
 		goto put_master;

 	ret = spi_bitbang_start(&xspi->bitbang);
 	if (ret) {
 		dev_err(&pdev->dev, "spi_bitbang_start FAILED\n");
-		goto free_irq;
+		goto put_master;
 	}

 	dev_info(&pdev->dev, "at 0x%08llX mapped to 0x%p, irq=%d\n",
@@ -482,8 +482,6 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, master);
 	return 0;

-free_irq:
-	free_irq(xspi->irq, xspi);
 put_master:
 	spi_master_put(master);

@@ -495,7 +493,6 @@ static int xilinx_spi_remove(struct platform_device *pdev)
 	struct xilinx_spi *xspi = platform_get_drvdata(pdev);

 	spi_bitbang_stop(&xspi->bitbang);
-	free_irq(xspi->irq, xspi);

 	spi_master_put(xspi->bitbang.master);

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Mark Brown]

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

On Mon, Jul 08, 2013 at 03:29:16PM +0200, Michal Simek wrote:
> Use devm_request_irq() for irq allocation which
> simplify driver code.

> @@ -495,7 +493,6 @@ static int xilinx_spi_remove(struct platform_device *pdev)
>  	struct xilinx_spi *xspi = platform_get_drvdata(pdev);
> 
>  	spi_bitbang_stop(&xspi->bitbang);
> -	free_irq(xspi->irq, xspi);
> 
>  	spi_master_put(xspi->bitbang.master);

Is it definitely safe to leave the IRQ hanging around after the master
has been freed - there's no possibility of a late error interrupt or
something?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Michal Simek]

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

On 07/08/2013 04:49 PM, Mark Brown wrote:
> On Mon, Jul 08, 2013 at 03:29:16PM +0200, Michal Simek wrote:
>> Use devm_request_irq() for irq allocation which
>> simplify driver code.
> 
>> @@ -495,7 +493,6 @@ static int xilinx_spi_remove(struct platform_device *pdev)
>>  	struct xilinx_spi *xspi = platform_get_drvdata(pdev);
>>
>>  	spi_bitbang_stop(&xspi->bitbang);
>> -	free_irq(xspi->irq, xspi);
>>
>>  	spi_master_put(xspi->bitbang.master);
> 
> Is it definitely safe to leave the IRQ hanging around after the master
> has been freed - there's no possibility of a late error interrupt or
> something?

I think it is more generic question if this race condition is fine
for all drivers which are using devres groups.

I have just looked at it and devres_release_all() is called where
driver is unload and irq are disabled there.

It means that all handlers should be unregistered and if IRQ happen
after it then it should be handled by Linux kernel if driver
doesn't disable it.

btw: What's the proper way for spi driver unregistration?

spi_unregistered_master() (which also free private structure)
and
spi_master_put()?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Mark Brown]

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

On Mon, Jul 08, 2013 at 05:48:14PM +0200, Michal Simek wrote:
> On 07/08/2013 04:49 PM, Mark Brown wrote:

> > Is it definitely safe to leave the IRQ hanging around after the master
> > has been freed - there's no possibility of a late error interrupt or
> > something?

> I think it is more generic question if this race condition is fine
> for all drivers which are using devres groups.

Well, it's mainly an issue for IRQs - the other resources don't initiate
events by themselves which is what causes the issue.  It just needs a
bit of extra care so I wanted to check that this has been thought of.

> I have just looked at it and devres_release_all() is called where
> driver is unload and irq are disabled there.

The problem is the gap between the resources used to handle the IRQ
being freed and the IRQ itself being freed - if the hardware can be
guaranteed to be idle then that's fine but we need to be sure that is
OK.  Otherwise the interrupt handler may get run and be looking at a
resource which was freed which would be unfortunate.

> btw: What's the proper way for spi driver unregistration?

> spi_unregistered_master() (which also free private structure)
> and
> spi_master_put()?

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Michal Simek]

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

On 07/08/2013 06:26 PM, Mark Brown wrote:
> On Mon, Jul 08, 2013 at 05:48:14PM +0200, Michal Simek wrote:
>> On 07/08/2013 04:49 PM, Mark Brown wrote:
> 
>>> Is it definitely safe to leave the IRQ hanging around after the master
>>> has been freed - there's no possibility of a late error interrupt or
>>> something?
> 
>> I think it is more generic question if this race condition is fine
>> for all drivers which are using devres groups.
> 
> Well, it's mainly an issue for IRQs - the other resources don't initiate
> events by themselves which is what causes the issue.  It just needs a
> bit of extra care so I wanted to check that this has been thought of.

That's good and thanks for that.

>> I have just looked at it and devres_release_all() is called where
>> driver is unload and irq are disabled there.
> 
> The problem is the gap between the resources used to handle the IRQ
> being freed and the IRQ itself being freed - if the hardware can be
> guaranteed to be idle then that's fine but we need to be sure that is
> OK.  Otherwise the interrupt handler may get run and be looking at a
> resource which was freed which would be unfortunate.

I am not sure if I understand you correctly

Here is the current flow which I see.
1. driver_remove() is called
2. spi_bitbang_stop() with spi_unregistered_master()
3. free_irq()
4. spi_master_put()
5. spi_master_release()
6. devres_release()

My patch is changing this flow where irq are freed in point 5.
1. driver_remove() is called
2. spi_bitbang_stop() with spi_unregistered_master()
3. spi_master_put()
4. spi_master_release()
5. devres_release() with irq

If interrupt happends between 4 and 5 than it can be the problem.
Do I understand you correctly?

If this is problematic case we can disable local and global interrupts
and add it between 2 and 3 or 3-4.
	/* Disable all the interrupts just in case */
	xspi->write_fn(0, regs_base + XIPIF_V123B_IIER_OFFSET);
	/* Disable the global IPIF interrupt */
	xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET);

What do you think about this solution?

I have also tried to run one thing with and without this patch
and the results are below.

When I add this irq disable function between 1 and 2 then
module removing stucks.

Thanks,
Michal


~ # modprobe spi-xilinx
xilinx_spi 75600000.spi: master is unqueued, this is deprecated
m25p80 spi0.0: found s25fl064k, expected m25p80
m25p80 spi0.0: s25fl064k (8192 Kbytes)
6 ofpart partitions found on MTD device spi0.0
Creating 6 MTD partitions on "spi0.0":
0x000000000000-0x000000200000 : "fpga"
0x000000200000-0x000000240000 : "boot"
0x000000240000-0x000000260000 : "bootenv"
0x000000260000-0x000000280000 : "config"
0x000000280000-0x000000e80000 : "image"
mtd: partition "image" extends beyond the end of device "spi0.0" -- size truncated to 0x580000
0x000000e80000-0x000000800000 : "spare"
mtd: partition "spare" is out of reach -- disabled
xilinx_spi 75600000.spi: at 0x75600000 mapped to 0xf0120000, irq=3
~ #
~ # dd if=/dev/zero of=/dev/mtd1 &
~ # sleep 1 && rmmod spi-xilinx
Removing MTD device #1 (boot) with use count 1
------------[ cut here ]------------
WARNING: at kernel/workqueue.c:1307 __queue_work+0xe4/0x258()
Modules linked in: spi_xilinx(-) [last unloaded: spi_xilinx]
CPU: 0 PID: 138 Comm: sh Not tainted 3.10.0-rc4+ #34
Kernel Stack:
c6f1fb10: c0009e98 ffffffff 00000000 00292d28 c6f5c200 00000000 00000009 c0009ee4
c6f1fb30: c02880b8 c028d8a8 0000051b c0022bb4 00000400 00005d14 000065a0 c6ebaca0
c6f1fb50: c6ed2740 00000001 00000000 c0022bb4 c0035658 c0037028 c6f3cd60 c6f3caec
c6f1fb70: c6f3cd8c c6f3caec c0022d88 c0314dd4 c6f3cd60 c0314dd4 c6f3cd8c c0314e14
c6f1fb90: 00000001 000065a0 000065a0 c6e9b6c0 c6e9b6c0 c6f1fc48 c01b10c4 00000200
c6f1fbb0: c6f1fbc4 c0314dd4 7fffffff c0036694 c6f3ceb8 c6f1fca4 c01aef1c 00000000
c6f1fbd0: c6f1fbd8 c6f3cac0 c0314dd4 c6f3cac0 c0314e14 000065a2 c6ed2600 00000000
c6f1fbf0: c01aef94 c6f1fc00 c6f3cac0 c7829810 c6f1fc10 c6f1fc10 c6f3cd60 c01af26c
c6f1fc10: c01af260 c01aef1c c6ed2600 c782fb00 c6ed2740 c0021f2c c6f1fca4 c01af2e8
c6f1fc30: c6f1fd40 7fffffff 00000002 00000000 00000000 00000100 00000000 c6f1fc4c
c6f1fc50: c6f1fc4c c0022bf0 c7844ca0 c7844ca1 c6f1fca4 00000001 c6f1fd50 c01af434
c6f1fc70: c0022d88 c6f1fcf0 c01af2e8 c0044088 00000000 c002d8e0 c01ad7c4 000065a0
c6f1fc90: 000065a0 c6e9b6c0 c6e9b6c0 c6f1fd40 c01b10c4 c6f1fcec c6f1fd10 c6e9b6c0
c6f1fcb0: 00000000 c01af4c0 c6f1fc48 00000000 ffffff8d c6ed2750 c6ed2750 00000000
c6f1fcd0: c7844ca0 00000000 00000001 00000000 00000000 00000800 00bebc20 c6f1fd10
c6f1fcf0: c6f1fca4 00000000 c7844ca1 00000001 00000000 00000000 00000800 00bebc20
c6f1fd10: c6f1fca4 c6f1fcec ffff8ad8 c6ed2400 00000000 c6f1fea8 c6ed2400 00000200
c6f1fd30: 00000000 c01ade30 c6f1fcf0 c6f1fcf0 00000000 c6f1fd44 c6f1fd44 00000000
c6f1fd50: c6ed0510 ffff8ad8 c6ed2400 c01adf38 c6ed2400 c01adf30 c6f1fda0 00000100
c6f1fd70: c6f1fea8 c6ed2400 c6ed2410 c6f1fda0 c018fafc c0b1a960 c0011db4 c0011ea0
c6f1fd90: c0001dac c6f1e000 c6f5ce00 c6f5c206 c6f1fde8 c6f1fe0c 00000000 00000000
c6f1fdb0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 c6f19620
c6f1fdd0: 00000000 00000004 00000000 00000000 00000000 00000000 c6f1fe0c c6f1fda0
c6f1fdf0: c6f5c200 00000000 00000000 00000000 00000000 00000000 00000000 c6f1fda0
c6f1fe10: c6f1fde8 00000200 00200000 00000000 0001a800 00000000 10148a38 00000000
c6f1fe30: 10148a38 10148a38 c6f757a0 c018cc7c 00000200 10144b50 482b8470 c6f5ce00
c6f1fe50: c6f5c200 c6f1feac 00040000 00000000 c0192f94 c0192e60 10148a38 c6f757a0
c6f1fe70: c6f3cd60 c01553d8 800045a4 c6f5ce00 c6f5c200 00000200 c6f1ff50 c008740c
c6f1fe90: 10144b50 482b8470 00000200 c0087490 c0087620 00000200 00000000 00000200
c6f1feb0: 10148a38 10148a38 c017dd68 c6f3cd60 c027b458 00000000 00000000 00000000
c6f1fed0: c6f1e000 c0087600 c6f17e40 00000200 10148a38 c6f1ff50 00000200 10148a38
c6f1fef0: 1012bd84 10148a38 10148a38 00000001 c0087890 c00877f0 00000001 00000003
c6f1ff10: 00000000 00000000 00000000 c6f17e40 00000000 10148a38 00000001 00000200
c6f1ff30: 10148a38 c0005488 00000000 00000001 00000000 10145428 10145428 00000200
c6f1ff50: 00025800 00000000 00000200 10147440 00000200 10148a38 00000004 bfc9f550
c6f1ff70: 00000000 00000000 00000000 00000001 10148a38 00000200 1014829f 00000000
c6f1ff90: 482b4730 00000006 00000004 00000000 00000000 1000cc50 00000000 10007824
c6f1ffb0: 7fffeffd 10147440 10144b50 482b8470 00000200 10148a38 00000001 00000200
c6f1ffd0: 10148a38 1012bd84 10148a38 10148a38 00000001 00000000 482097f8 000055aa
c6f1fff0: 1000c264 00000000 00000000 00000000


Call Trace:
[<c0003bfc>] microblaze_unwind+0x48/0x68
[<c0003924>] show_stack+0x118/0x158
[<c02775cc>] dump_stack+0x20/0x38
[<c0009e94>] warn_slowpath_common+0x7c/0xbc
[<c0009ee0>] warn_slowpath_null+0xc/0x24
[<c0022bb0>] __queue_work+0xe0/0x258
[<c0022d84>] queue_work_on+0x38/0x60
[<c01b10c0>] spi_bitbang_transfer+0x70/0xa0
[<c01aef18>] __spi_async+0xe4/0x108
[<c01aef90>] spi_async_locked+0x10/0x34
[<c01af268>] __spi_sync+0x70/0xcc
[<c01af2e4>] spi_sync+0x4/0x1c
[<c01af430>] spi_write_then_read+0x134/0x1c4
[<c01ad7c0>] read_sr+0x2c/0x7c
[<c01ade2c>] wait_till_ready+0x1c/0x6c
[<c01adf34>] m25p80_write+0xb8/0x268
[<c018faf8>] part_write+0x24/0x44
[<c018cc78>] mtd_write+0x8c/0xbc
[<c0192f90>] mtdchar_write+0x1d4/0x298
[<c0087408>] vfs_write+0xe8/0x1cc
[<c008788c>] SyS_write+0x50/0xa0
SYSCALL



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Mark Brown]

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

On Tue, Jul 09, 2013 at 04:15:13PM +0200, Michal Simek wrote:

> 4. spi_master_release()
> 5. devres_release() with irq

> If interrupt happends between 4 and 5 than it can be the problem.
> Do I understand you correctly?

Yes.

> If this is problematic case we can disable local and global interrupts
> and add it between 2 and 3 or 3-4.
> 	/* Disable all the interrupts just in case */
> 	xspi->write_fn(0, regs_base + XIPIF_V123B_IIER_OFFSET);
> 	/* Disable the global IPIF interrupt */
> 	xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET);

> What do you think about this solution?

That's fine, though if just manually freeing the IRQ works that's also
OK...

> I have also tried to run one thing with and without this patch
> and the results are below.

> When I add this irq disable function between 1 and 2 then
> module removing stucks.

You'll need to wait until the device is quiesced at least if it's
relying on the interrupt to complete operations.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Michal Simek]

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

On 07/09/2013 04:47 PM, Mark Brown wrote:
> On Tue, Jul 09, 2013 at 04:15:13PM +0200, Michal Simek wrote:
> 
>> 4. spi_master_release()
>> 5. devres_release() with irq
> 
>> If interrupt happends between 4 and 5 than it can be the problem.
>> Do I understand you correctly?
> 
> Yes.

great.

> 
>> If this is problematic case we can disable local and global interrupts
>> and add it between 2 and 3 or 3-4.
>> 	/* Disable all the interrupts just in case */
>> 	xspi->write_fn(0, regs_base + XIPIF_V123B_IIER_OFFSET);
>> 	/* Disable the global IPIF interrupt */
>> 	xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET);
> 
>> What do you think about this solution?
> 
> That's fine, though if just manually freeing the IRQ works that's also
> OK...

yeah.


>> I have also tried to run one thing with and without this patch
>> and the results are below.
> 
>> When I add this irq disable function between 1 and 2 then
>> module removing stucks.
> 
> You'll need to wait until the device is quiesced at least if it's
> relying on the interrupt to complete operations.
> 

ok. Will send v2 of this patch.

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 264 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Michal Simek]

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

On 07/08/2013 06:26 PM, Mark Brown wrote:
> On Mon, Jul 08, 2013 at 05:48:14PM +0200, Michal Simek wrote:
>> On 07/08/2013 04:49 PM, Mark Brown wrote:
> 
>>> Is it definitely safe to leave the IRQ hanging around after the master
>>> has been freed - there's no possibility of a late error interrupt or
>>> something?
> 
>> I think it is more generic question if this race condition is fine
>> for all drivers which are using devres groups.
> 
> Well, it's mainly an issue for IRQs - the other resources don't initiate
> events by themselves which is what causes the issue.  It just needs a
> bit of extra care so I wanted to check that this has been thought of.
> 
>> I have just looked at it and devres_release_all() is called where
>> driver is unload and irq are disabled there.
> 
> The problem is the gap between the resources used to handle the IRQ
> being freed and the IRQ itself being freed - if the hardware can be
> guaranteed to be idle then that's fine but we need to be sure that is
> OK.  Otherwise the interrupt handler may get run and be looking at a
> resource which was freed which would be unfortunate.
> 
>> btw: What's the proper way for spi driver unregistration?
> 
>> spi_unregistered_master() (which also free private structure)
>> and
>> spi_master_put()?
> 
> Yes.

Just a follow up on this.
Which function free private structures registered by spi_alloc_master function?
Is it in spi_master_put()?

The reason why I am asking is where clk_xx functions should be added.
I see them between these two functions in sifr for example.

And also I see in drivers in error probe path that drivers are calling kfree(master)
but they are not doing in remove part (like spi-davinci.c).

I just want to clear this in our zynq drivers before we send them out.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v1 3/4] spi/xilinx: Simplify irq allocation

[Mark Brown]

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

On Fri, Jul 12, 2013 at 04:00:24PM +0200, Michal Simek wrote:

> Just a follow up on this.

Sorry, this one fell through the cracks - apologies for the delay.

> Which function free private structures registered by spi_alloc_master function?
> Is it in spi_master_put()?

Yes, or spi_master_unregister() after it's been registered.

> The reason why I am asking is where clk_xx functions should be added.
> I see them between these two functions in sifr for example.

> And also I see in drivers in error probe path that drivers are calling kfree(master)
> but they are not doing in remove part (like spi-davinci.c).

> I just want to clear this in our zynq drivers before we send them out.

I'd not be surprised if there were errors in the error handling paths,
it's (hopefully!) not the best tested code out there.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v1 4/4] spi/xilinx: Use of_property_read_u32 for reading value from node

[Michal Simek]

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

It simplifies driver probing.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index a6bb5b0..07a7bca 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -372,17 +372,9 @@ static int xilinx_spi_probe(struct platform_device *pdev)
 	if (pdata) {
 		num_cs = pdata->num_chipselect;
 		bits_per_word = pdata->bits_per_word;
-	}
-
-	if (pdev->dev.of_node) {
-		const __be32 *prop;
-		int len;
-
-		/* number of slave select bits is required */
-		prop = of_get_property(pdev->dev.of_node, "xlnx,num-ss-bits",
-				       &len);
-		if (prop && len >= sizeof(*prop))
-			num_cs = __be32_to_cpup(prop);
+	} else {
+		of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
+					  &num_cs);
 	}

 	if (!num_cs) {
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v1 4/4] spi/xilinx: Use of_property_read_u32 for reading value from node

[Mark Brown]

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

On Mon, Jul 08, 2013 at 03:29:17PM +0200, Michal Simek wrote:
> It simplifies driver probing.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1 4/4] spi/xilinx: Use of_property_read_u32 for reading value from node

[Michal Simek]

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

Hi Mark,

On 07/08/2013 04:51 PM, Mark Brown wrote:
> On Mon, Jul 08, 2013 at 03:29:17PM +0200, Michal Simek wrote:
>> It simplifies driver probing.
> 
> Applied, thanks.

have you applied this patch?

I can't see it in your topic/xilinx branch.
https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/log/?h=topic/xilinx

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v1 4/4] spi/xilinx: Use of_property_read_u32 for reading value from node

[Mark Brown]

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

On Tue, Jul 09, 2013 at 07:26:27AM +0200, Michal Simek wrote:
> On 07/08/2013 04:51 PM, Mark Brown wrote:

> > Applied, thanks.

> have you applied this patch?

Yes, network is spotty here so pushes aren't working so well.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1 1/4] spi/xilinx: Remove CONFIG_OF from the driver

[Mark Brown]

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

On Mon, Jul 08, 2013 at 03:29:14PM +0200, Michal Simek wrote:
> dev.of_node is in struct device all the time.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]