[PATCH] IDE: Fix platform device registration in Swarm IDE driver

[PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

The Swarm IDE driver uses a release method which is defined in the driver
itself thus potencially oopsable.  The simpel fix would be to just leak
the device but this patch goes the full length and moves the entire
handling of the platform device in the platform code and retains only
the platform driver code in drivers/ide/mips/swarm.c.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---

This patch is for 2.6.27.  The same issue exists in -stable but the patch
won't apply due to other driver changes.

 arch/mips/sibyte/swarm/Makefile   |    3 
 arch/mips/sibyte/swarm/platform.c |   60 ++++++++++++++++
 drivers/ide/mips/swarm.c          |  140 +++++++++++---------------------------
 3 files changed, 104 insertions(+), 99 deletions(-)

Index: linux-mips/arch/mips/sibyte/swarm/platform.c
===================================================================
--- /dev/null
+++ linux-mips/arch/mips/sibyte/swarm/platform.c
@@ -0,0 +1,60 @@
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#include <asm/sibyte/board.h>
+#include <asm/sibyte/sb1250_genbus.h>
+#include <asm/sibyte/sb1250_regs.h>
+
+#define DEV_NAME "ide-swarm"
+
+static struct resource swarm_ide_resource[] = {
+	{
+		.name	= "Swarm GenBus IDE",
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.name	= "Swarm GenBus IDE",
+		.flags	= IORESOURCE_IRQ,
+		.start	= K_INT_GB_IDE,
+		.end	= K_INT_GB_IDE,
+	},
+};
+
+static int __init swarm_ide_init(void)
+{
+	struct platform_device *pdev;
+	u8 __iomem *base;
+	phys_t offset, size;
+
+	if (!SIBYTE_HAVE_IDE)
+		return -ENODEV;
+
+	base = ioremap(A_IO_EXT_BASE, 0x800);
+	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
+	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
+	iounmap(base);
+
+	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
+	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
+	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
+		pr_info(DEV_NAME ": IDE interface at GenBus disabled\n");
+
+		return -EBUSY;
+	}
+
+	pr_info(DEV_NAME ": IDE interface at GenBus slot %i\n", IDE_CS);
+
+	swarm_ide_resource[0].start = offset;
+	swarm_ide_resource[0].end = offset + size - 1;
+
+	pdev = platform_device_register_simple(DEV_NAME, -1,
+		       swarm_ide_resource, ARRAY_SIZE(swarm_ide_resource));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
+device_initcall(swarm_ide_init);
Index: linux-mips/arch/mips/sibyte/swarm/Makefile
===================================================================
--- linux-mips.orig/arch/mips/sibyte/swarm/Makefile
+++ linux-mips/arch/mips/sibyte/swarm/Makefile
@@ -1,3 +1,4 @@
-obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
+obj-y				:= platform.o setup.o rtc_xicor1241.o \
+				   rtc_m41t81.o
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= swarm-i2c.o
Index: linux-mips/drivers/ide/mips/swarm.c
===================================================================
--- linux-mips.orig/drivers/ide/mips/swarm.c
+++ linux-mips/drivers/ide/mips/swarm.c
@@ -46,21 +46,10 @@
 
 #include <asm/io.h>
 
-#include <asm/sibyte/board.h>
-#include <asm/sibyte/sb1250_genbus.h>
-#include <asm/sibyte/sb1250_regs.h>
-
 #define DRV_NAME "ide-swarm"
 
 static char swarm_ide_string[] = DRV_NAME;
 
-static struct resource swarm_ide_resource = {
-	.name	= "SWARM GenBus IDE",
-	.flags	= IORESOURCE_MEM,
-};
-
-static struct platform_device *swarm_ide_dev;
-
 static const struct ide_port_info swarm_port_info = {
 	.name			= DRV_NAME,
 	.host_flags		= IDE_HFLAG_MMIO | IDE_HFLAG_NO_DMA,
@@ -70,41 +59,18 @@ static const struct ide_port_info swarm_
  * swarm_ide_probe - if the board header indicates the existence of
  * Generic Bus IDE, allocate a HWIF for it.
  */
-static int __devinit swarm_ide_probe(struct device *dev)
+static int __devinit swarm_ide_probe(struct platform_device *pdev)
 {
 	u8 __iomem *base;
 	struct ide_host *host;
 	phys_t offset, size;
+	struct resource *r;
 	int i, rc;
 	hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
 
-	if (!SIBYTE_HAVE_IDE)
-		return -ENODEV;
-
-	base = ioremap(A_IO_EXT_BASE, 0x800);
-	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
-	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
-	iounmap(base);
-
-	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
-	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
-	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
-		printk(KERN_INFO DRV_NAME
-		       ": IDE interface at GenBus disabled\n");
-		return -EBUSY;
-	}
-
-	printk(KERN_INFO DRV_NAME ": IDE interface at GenBus slot %i\n",
-	       IDE_CS);
-
-	swarm_ide_resource.start = offset;
-	swarm_ide_resource.end = offset + size - 1;
-	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
-		printk(KERN_ERR DRV_NAME
-		       ": can't request I/O memory resource\n");
-		return -EBUSY;
-	}
-
+	r = pdev->resource;
+	offset = r->start;
+	size = resource_size(r);
 	base = ioremap(offset, size);
 
 	memset(&hw, 0, sizeof(hw));
@@ -113,85 +79,63 @@ static int __devinit swarm_ide_probe(str
 				(unsigned long)(base + ((0x1f0 + i) << 5));
 	hw.io_ports.ctl_addr =
 				(unsigned long)(base + (0x3f6 << 5));
-	hw.irq = K_INT_GB_IDE;
+	hw.irq = r[1].start;
 	hw.chipset = ide_generic;
 
 	rc = ide_host_add(&swarm_port_info, hws, &host);
-	if (rc)
-		goto err;
+	if (rc) {
+		iounmap(base);
+
+		return rc;
+	}
 
-	dev_set_drvdata(dev, host);
+	platform_set_drvdata(pdev, host);
 
 	return 0;
-err:
-	release_resource(&swarm_ide_resource);
-	iounmap(base);
-	return rc;
 }
 
-static struct device_driver swarm_ide_driver = {
-	.name	= swarm_ide_string,
-	.bus	= &platform_bus_type,
-	.probe	= swarm_ide_probe,
-};
-
-static void swarm_ide_platform_release(struct device *device)
+static int __devexit swarm_ide_remove(struct platform_device *pdev)
 {
-	struct platform_device *pldev;
+	struct ide_host *host = pdev->dev.driver_data;
 
-	/* free device */
-	pldev = to_platform_device(device);
-	kfree(pldev);
+	ide_host_remove(host);
+
+	return 0;
 }
 
-static int __devinit swarm_ide_init_module(void)
+static struct platform_driver swarm_ide_driver = {
+	.driver = {
+		.name	= swarm_ide_string,
+		.owner	= THIS_MODULE,
+	},
+	.probe	= swarm_ide_probe,
+	.remove = __devexit_p(swarm_ide_remove),
+};
+
+static int __init swarm_ide_init(void)
 {
-	struct platform_device *pldev;
 	int err;
 
-	printk(KERN_INFO "SWARM IDE driver\n");
-
-	if (driver_register(&swarm_ide_driver)) {
-		printk(KERN_ERR "Driver registration failed\n");
-		err = -ENODEV;
-		goto out;
-	}
+	pr_info("Swarm IDE driver\n");
 
-        if (!(pldev = kzalloc(sizeof (*pldev), GFP_KERNEL))) {
-		err = -ENOMEM;
-		goto out_unregister_driver;
-	}
+	err = platform_driver_register(&swarm_ide_driver);
+	if (err) {
+		pr_err("Driver registration failed\n");
 
-	pldev->name		= swarm_ide_string;
-	pldev->id		= 0;
-	pldev->dev.release	= swarm_ide_platform_release;
-
-	if (platform_device_register(pldev)) {
-		err = -ENODEV;
-		goto out_free_pldev;
+		return err;
 	}
 
-        if (!pldev->dev.driver) {
-		/*
-		 * The driver was not bound to this device, there was
-                 * no hardware at this address. Unregister it, as the
-		 * release fuction will take care of freeing the
-		 * allocated structure
-		 */
-		platform_device_unregister (pldev);
-	}
-
-	swarm_ide_dev = pldev;
-
 	return 0;
+}
 
-out_free_pldev:
-	kfree(pldev);
-
-out_unregister_driver:
-	driver_unregister(&swarm_ide_driver);
-out:
-	return err;
+static void __exit swarm_ide_exit(void)
+{
+	platform_driver_unregister(&swarm_ide_driver);
 }
 
-module_init(swarm_ide_init_module);
+MODULE_DESCRIPTION("Sibyte Swarm platform IDE driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ide-swarm");
+
+module_init(swarm_ide_init);
+module_exit(swarm_ide_exit);

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello.

Ralf Baechle wrote:

> The Swarm IDE driver uses a release method which is defined in the driver
> itself thus potencially oopsable.  The simpel fix would be to just leak
>   

   "Potentially" and "simple". :-P

> the device but this patch goes the full length and moves the entire
> handling of the platform device in the platform code and retains only
> the platform driver code in drivers/ide/mips/swarm.c.
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> ---
>
> This patch is for 2.6.27.  The same issue exists in -stable but the patch
> won't apply due to other driver changes.
>
>  arch/mips/sibyte/swarm/Makefile   |    3 
>  arch/mips/sibyte/swarm/platform.c |   60 ++++++++++++++++
>  drivers/ide/mips/swarm.c          |  140 +++++++++++---------------------------
>  3 files changed, 104 insertions(+), 99 deletions(-)
>
> Index: linux-mips/arch/mips/sibyte/swarm/platform.c
> ===================================================================
> --- /dev/null
> +++ linux-mips/arch/mips/sibyte/swarm/platform.c
> @@ -0,0 +1,60 @@
>   
[...]
> +static struct resource swarm_ide_resource[] = {
> +	{
> +		.name	= "Swarm GenBus IDE",
> +		.flags	= IORESOURCE_MEM,
> +	}, {
> +		.name	= "Swarm GenBus IDE",
> +		.flags	= IORESOURCE_IRQ,
> +		.start	= K_INT_GB_IDE,
> +		.end	= K_INT_GB_IDE,
> +	},
> +};
> +
> +static int __init swarm_ide_init(void)
> +{
>   
[...]
> +	pdev = platform_device_register_simple(DEV_NAME, -1,
> +		       swarm_ide_resource, ARRAY_SIZE(swarm_ide_resource));
>   

   If you have the resources as static array anyway, why not have the 
device in the static variable too and use platform_device_register()?

> Index: linux-mips/drivers/ide/mips/swarm.c
> ===================================================================
> --- linux-mips.orig/drivers/ide/mips/swarm.c
> +++ linux-mips/drivers/ide/mips/swarm.c
> @@ -46,21 +46,10 @@
>  
>  #include <asm/io.h>
>  
> -#include <asm/sibyte/board.h>
> -#include <asm/sibyte/sb1250_genbus.h>
> -#include <asm/sibyte/sb1250_regs.h>
> -
>  #define DRV_NAME "ide-swarm"
>  
>  static char swarm_ide_string[] = DRV_NAME;
>  
> -static struct resource swarm_ide_resource = {
> -	.name	= "SWARM GenBus IDE",
> -	.flags	= IORESOURCE_MEM,
> -};
> -
> -static struct platform_device *swarm_ide_dev;
>   

   Platform device in the driver itself? Interesting... :-)

> @@ -70,41 +59,18 @@ static const struct ide_port_info swarm_
>   * swarm_ide_probe - if the board header indicates the existence of
>   * Generic Bus IDE, allocate a HWIF for it.
>   */
> -static int __devinit swarm_ide_probe(struct device *dev)
> +static int __devinit swarm_ide_probe(struct platform_device *pdev)
>  {
>  	u8 __iomem *base;
>  	struct ide_host *host;
>  	phys_t offset, size;
> +	struct resource *r;
>  	int i, rc;
>  	hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>  
> -	if (!SIBYTE_HAVE_IDE)
> -		return -ENODEV;
> -
> -	base = ioremap(A_IO_EXT_BASE, 0x800);
> -	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
> -	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
> -	iounmap(base);
> -
> -	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
> -	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
> -	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
> -		printk(KERN_INFO DRV_NAME
> -		       ": IDE interface at GenBus disabled\n");
> -		return -EBUSY;
> -	}
> -
> -	printk(KERN_INFO DRV_NAME ": IDE interface at GenBus slot %i\n",
> -	       IDE_CS);
> -
> -	swarm_ide_resource.start = offset;
> -	swarm_ide_resource.end = offset + size - 1;
> -	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
>   

   Why drop request_resource() completely? Replace it by 
request_mem_region().

MBR, Sergei

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Maciej W. Rozycki]

On Wed, 24 Sep 2008, Sergei Shtylyov wrote:

>    Platform device in the driver itself? Interesting... :-)

 Better than nothing for platforms which did not have platform
initialisation at all.  Some driver subsystems do not support
non-driver-model devices at all or anymore.

  Maciej

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 September 2008, Sergei Shtylyov wrote:

[...]

> > @@ -70,41 +59,18 @@ static const struct ide_port_info swarm_
> >   * swarm_ide_probe - if the board header indicates the existence of
> >   * Generic Bus IDE, allocate a HWIF for it.
> >   */
> > -static int __devinit swarm_ide_probe(struct device *dev)
> > +static int __devinit swarm_ide_probe(struct platform_device *pdev)
> >  {
> >  	u8 __iomem *base;
> >  	struct ide_host *host;
> >  	phys_t offset, size;
> > +	struct resource *r;
> >  	int i, rc;
> >  	hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
> >  
> > -	if (!SIBYTE_HAVE_IDE)
> > -		return -ENODEV;
> > -
> > -	base = ioremap(A_IO_EXT_BASE, 0x800);
> > -	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
> > -	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
> > -	iounmap(base);
> > -
> > -	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
> > -	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
> > -	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
> > -		printk(KERN_INFO DRV_NAME
> > -		       ": IDE interface at GenBus disabled\n");
> > -		return -EBUSY;
> > -	}
> > -
> > -	printk(KERN_INFO DRV_NAME ": IDE interface at GenBus slot %i\n",
> > -	       IDE_CS);
> > -
> > -	swarm_ide_resource.start = offset;
> > -	swarm_ide_resource.end = offset + size - 1;
> > -	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
> >   
> 
>    Why drop request_resource() completely? Replace it by 
> request_mem_region().

Yes, this needs fixing (otherwise everything looks good).

Ralf: I guess that your next step will be dropping swarm-specific platform ide
driver in favor of generic one (please see drivers/ide/legacy/ide_platform.c)
as they are _very_ similar now? :)

Thanks,
Bart

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

On Wed, Sep 24, 2008 at 02:52:07PM +0100, Maciej W. Rozycki wrote:

> >    Platform device in the driver itself? Interesting... :-)
> 
>  Better than nothing for platforms which did not have platform
> initialisation at all.  Some driver subsystems do not support
> non-driver-model devices at all or anymore.

Historically registration of the device file in the driver itself wasn't
uncommon before it turned out to be not such a great idea.  I hope this
was the last of the affected MIPS-specific drivers.

  Ralf

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>> @@ -70,41 +59,18 @@ static const struct ide_port_info swarm_
>>>   * swarm_ide_probe - if the board header indicates the existence of
>>>   * Generic Bus IDE, allocate a HWIF for it.
>>>   */
>>> -static int __devinit swarm_ide_probe(struct device *dev)
>>> +static int __devinit swarm_ide_probe(struct platform_device *pdev)
>>>  {
>>>  	u8 __iomem *base;
>>>  	struct ide_host *host;
>>>  	phys_t offset, size;
>>> +	struct resource *r;
>>>  	int i, rc;
>>>  	hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>>>  
>>> -	if (!SIBYTE_HAVE_IDE)
>>> -		return -ENODEV;
>>> -
>>> -	base = ioremap(A_IO_EXT_BASE, 0x800);
>>> -	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
>>> -	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
>>> -	iounmap(base);
>>> -
>>> -	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
>>> -	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
>>> -	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
>>> -		printk(KERN_INFO DRV_NAME
>>> -		       ": IDE interface at GenBus disabled\n");
>>> -		return -EBUSY;
>>> -	}
>>> -
>>> -	printk(KERN_INFO DRV_NAME ": IDE interface at GenBus slot %i\n",
>>> -	       IDE_CS);
>>> -
>>> -	swarm_ide_resource.start = offset;
>>> -	swarm_ide_resource.end = offset + size - 1;
>>> -	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
>>>   
>>>       
>>    Why drop request_resource() completely? Replace it by 
>> request_mem_region().
>>     
>
> Yes, this needs fixing (otherwise everything looks good).
>   

   BTW, calling request_resource() seems pointless, as 
platform_device_register() call will have called __request_resource() 
already...

WBR, Sergei

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

On Sat, Sep 27, 2008 at 06:59:55PM +0200, Bartlomiej Zolnierkiewicz wrote:

> > > -	swarm_ide_resource.start = offset;
> > > -	swarm_ide_resource.end = offset + size - 1;
> > > -	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
> > >   
> > 
> >    Why drop request_resource() completely? Replace it by 
> > request_mem_region().
> 
> Yes, this needs fixing (otherwise everything looks good).

No, platform_device_add which is called by platform_device_register*
will take care of adding the resources - but only if if's told about them
which the old driver didn't.

Also, in case of a resource conflict a device should not be added at all but
exactly that is what the old code did.  A resource conflict would have been
caught by the platform_driver probing code well too late.

> Ralf: I guess that your next step will be dropping swarm-specific platform ide
> driver in favor of generic one (please see drivers/ide/legacy/ide_platform.c)
> as they are _very_ similar now? :)

Good point - I was already wondering if something like that does exist.
What's left over of the swarm driver way too much looks like it can be
squeezed into some sort of template.

  Ralf

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

On Wed, Sep 24, 2008 at 03:08:13PM +0400, Sergei Shtylyov wrote:

>> +{
>>   
> [...]
>> +	pdev = platform_device_register_simple(DEV_NAME, -1,
>> +		       swarm_ide_resource, ARRAY_SIZE(swarm_ide_resource));
>>   
>
>   If you have the resources as static array anyway, why not have the  
> device in the static variable too and use platform_device_register()?

It saves a few lines of code.

>> -static struct platform_device *swarm_ide_dev;
>>   
>
>   Platform device in the driver itself? Interesting... :-)

It works and isn't a too bad idea for certain drivers where adding one half
of the code to a platform file, another to the driver file is just too
much fuzz.  It's just that this wasn't done right in case of the Swarm so
I'm gluing that now.

  Ralf

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello.

Ralf Baechle wrote:

>>>+{

>>[...]

>>>+	pdev = platform_device_register_simple(DEV_NAME, -1,
>>>+		       swarm_ide_resource, ARRAY_SIZE(swarm_ide_resource));

>>  If you have the resources as static array anyway, why not have the  
>>device in the static variable too and use platform_device_register()?

> It saves a few lines of code.

    And wastes few words of static data since 
platform_device_register_simple() will kmalloc() the resources and do a copy 
from these resources after which they are not needed -- so, it's worth making 
swarm_ide_resource[] __initdata at least.
    If you were using platform_device_register() with static platform device, 
no memory allocation would have happened, and no data would have been wasted.

WBR, Sergei

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello.

Ralf Baechle wrote:

>>>>-	swarm_ide_resource.start = offset;
>>>>-	swarm_ide_resource.end = offset + size - 1;
>>>>-	if (request_resource(&iomem_resource, &swarm_ide_resource)) {

>>>   Why drop request_resource() completely? Replace it by 
>>>request_mem_region().

>>Yes, this needs fixing (otherwise everything looks good).

> No, platform_device_add which is called by platform_device_register*
> will take care of adding the resources - but only if if's told about them
> which the old driver didn't.

    Ah, I've missed that the platform device was registered without resources 
(ugh) -- request_resource() call wasn't pointless then.  Note however that 
request_mem_region() does somewhat different thing: it pins the memory 
resource for the driver, setting IORESOURCE_BUSY flag on the resource (and it 
also walks the resource tree in depth, using __request_resource() on each 
level.  That's the thing that drivers do routinely on intialization.

>   Ralf

MBR, Sergei

Re: [PATCH] IDE: Fix platform device registration in Swarm IDE driver

[Geert Uytterhoeven]

On Sun, 28 Sep 2008, Sergei Shtylyov wrote:
> Ralf Baechle wrote:
> 
> > > > +{
> 
> > > [...]
> 
> > > > +	pdev = platform_device_register_simple(DEV_NAME, -1,
> > > > +		       swarm_ide_resource, ARRAY_SIZE(swarm_ide_resource));
> 
> > >  If you have the resources as static array anyway, why not have the
> > > device in the static variable too and use platform_device_register()?
> 
> > It saves a few lines of code.
> 
>    And wastes few words of static data since platform_device_register_simple()
> will kmalloc() the resources and do a copy from these resources after which
> they are not needed -- so, it's worth making swarm_ide_resource[] __initdata
> at least.
>    If you were using platform_device_register() with static platform device,
> no memory allocation would have happened, and no data would have been wasted.

Indeed, there are different static/dynamic memory usage patterns for the
various ways to register platform devices. Unfortunately (AFAIK) it's not
properly documented which to use when.

E.g. if some devices may be present (as indicated by e.g. firmware),
which one is the most optimal to use?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

The Swarm IDE driver uses a release method which is defined in the driver
itself thus potentially oopsable.  The simple fix would be to just leak
the device but this patch goes the full length and moves the entire
handling of the platform device in the platform code and retains only
the platform driver code in drivers/ide/mips/swarm.c.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
This patch is for 2.6.27.  The same issue exists in -stable but the patch
won't apply due to other driver changes.

Changes: v2 rewritten to use pata_platform, as suggested.

 arch/mips/sibyte/swarm/Makefile   |    3 
 arch/mips/sibyte/swarm/platform.c |   81 +++++++++++++++
 drivers/ide/mips/Makefile         |    1 
 drivers/ide/mips/swarm.c          |  197 --------------------------------------
 4 files changed, 83 insertions(+), 199 deletions(-)

Index: linux-mips/arch/mips/sibyte/swarm/platform.c
===================================================================
--- /dev/null
+++ linux-mips/arch/mips/sibyte/swarm/platform.c
@@ -0,0 +1,81 @@
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/ata_platform.h>
+
+#include <asm/sibyte/board.h>
+#include <asm/sibyte/sb1250_genbus.h>
+#include <asm/sibyte/sb1250_regs.h>
+
+#define DRV_NAME	"pata-swarm"
+
+#define SWARM_IDE_SHIFT	5
+#define SWARM_IDE_BASE	0x1f0
+#define SWARM_IDE_CTRL	0x3f6
+
+static struct resource swarm_pata_resource[] = {
+	{
+		.name	= "Swarm GenBus IDE",
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.name	= "Swarm GenBus IDE",
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.name	= "Swarm GenBus IDE",
+		.flags	= IORESOURCE_IRQ,
+		.start	= K_INT_GB_IDE,
+		.end	= K_INT_GB_IDE,
+	},
+};
+
+static struct pata_platform_info pata_platform_data = {
+	.ioport_shift	= SWARM_IDE_SHIFT,
+};
+
+static struct platform_device swarm_pata_device = {
+	.name		= "pata_platform",
+	.id		= -1,
+	.resource	= swarm_pata_resource,
+	.num_resources	= ARRAY_SIZE(swarm_pata_resource),
+	.dev  = {
+		.platform_data		= &pata_platform_data,
+		.coherent_dma_mask	= ~0,	/* grumble */
+	},
+};
+
+static int __init swarm_pata_init(void)
+{
+	u8 __iomem *base;
+	phys_t offset, size;
+	struct resource *r;
+
+	if (!SIBYTE_HAVE_IDE)
+		return -ENODEV;
+
+	base = ioremap(A_IO_EXT_BASE, 0x800);
+	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
+	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
+	iounmap(base);
+
+	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
+	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
+	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
+		pr_info(DRV_NAME ": PATA interface at GenBus disabled\n");
+
+		return -EBUSY;
+	}
+
+	pr_info(DRV_NAME ": PATA interface at GenBus slot %i\n", IDE_CS);
+
+	r = swarm_pata_resource;
+	r[0].start = offset + (SWARM_IDE_BASE << SWARM_IDE_SHIFT);
+	r[0].end   = offset + ((SWARM_IDE_BASE + 8) << SWARM_IDE_SHIFT) - 1;
+	r[1].start = offset + (SWARM_IDE_CTRL << SWARM_IDE_SHIFT);
+	r[1].end   = offset + ((SWARM_IDE_CTRL + 1) << SWARM_IDE_SHIFT) - 1;
+
+	return platform_device_register(&swarm_pata_device);
+}
+
+device_initcall(swarm_pata_init);
Index: linux-mips/arch/mips/sibyte/swarm/Makefile
===================================================================
--- linux-mips.orig/arch/mips/sibyte/swarm/Makefile
+++ linux-mips/arch/mips/sibyte/swarm/Makefile
@@ -1,3 +1,4 @@
-obj-y				:= setup.o rtc_xicor1241.o rtc_m41t81.o
+obj-y				:= platform.o setup.o rtc_xicor1241.o \
+				   rtc_m41t81.o
 
 obj-$(CONFIG_I2C_BOARDINFO)	+= swarm-i2c.o
Index: linux-mips/drivers/ide/mips/swarm.c
===================================================================
--- linux-mips.orig/drivers/ide/mips/swarm.c
+++ /dev/null
@@ -1,197 +0,0 @@
-/*
- * Copyright (C) 2001, 2002, 2003 Broadcom Corporation
- * Copyright (C) 2004 MontaVista Software Inc.
- *	Author:	Manish Lachwani, mlachwani@mvista.com
- * Copyright (C) 2004  MIPS Technologies, Inc.  All rights reserved.
- *	Author: Maciej W. Rozycki <macro@mips.com>
- * Copyright (c) 2006, 2008  Maciej W. Rozycki
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
- */
-
-/*
- *  Derived loosely from ide-pmac.c, so:
- *  Copyright (C) 1998 Paul Mackerras.
- *  Copyright (C) 1995-1998 Mark Lord
- */
-
-/*
- * Boards with SiByte processors so far have supported IDE devices via
- * the Generic Bus, PCI bus, and built-in PCMCIA interface.  In all
- * cases, byte-swapping must be avoided for these devices (whereas
- * other PCI devices, for example, will require swapping).  Any
- * SiByte-targetted kernel including IDE support will include this
- * file.  Probing of a Generic Bus for an IDE device is controlled by
- * the definition of "SIBYTE_HAVE_IDE", which is provided by
- * <asm/sibyte/board.h> for Broadcom boards.
- */
-
-#include <linux/ide.h>
-#include <linux/ioport.h>
-#include <linux/kernel.h>
-#include <linux/types.h>
-#include <linux/platform_device.h>
-
-#include <asm/io.h>
-
-#include <asm/sibyte/board.h>
-#include <asm/sibyte/sb1250_genbus.h>
-#include <asm/sibyte/sb1250_regs.h>
-
-#define DRV_NAME "ide-swarm"
-
-static char swarm_ide_string[] = DRV_NAME;
-
-static struct resource swarm_ide_resource = {
-	.name	= "SWARM GenBus IDE",
-	.flags	= IORESOURCE_MEM,
-};
-
-static struct platform_device *swarm_ide_dev;
-
-static const struct ide_port_info swarm_port_info = {
-	.name			= DRV_NAME,
-	.host_flags		= IDE_HFLAG_MMIO | IDE_HFLAG_NO_DMA,
-};
-
-/*
- * swarm_ide_probe - if the board header indicates the existence of
- * Generic Bus IDE, allocate a HWIF for it.
- */
-static int __devinit swarm_ide_probe(struct device *dev)
-{
-	u8 __iomem *base;
-	struct ide_host *host;
-	phys_t offset, size;
-	int i, rc;
-	hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
-
-	if (!SIBYTE_HAVE_IDE)
-		return -ENODEV;
-
-	base = ioremap(A_IO_EXT_BASE, 0x800);
-	offset = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_START_ADDR, IDE_CS));
-	size = __raw_readq(base + R_IO_EXT_REG(R_IO_EXT_MULT_SIZE, IDE_CS));
-	iounmap(base);
-
-	offset = G_IO_START_ADDR(offset) << S_IO_ADDRBASE;
-	size = (G_IO_MULT_SIZE(size) + 1) << S_IO_REGSIZE;
-	if (offset < A_PHYS_GENBUS || offset >= A_PHYS_GENBUS_END) {
-		printk(KERN_INFO DRV_NAME
-		       ": IDE interface at GenBus disabled\n");
-		return -EBUSY;
-	}
-
-	printk(KERN_INFO DRV_NAME ": IDE interface at GenBus slot %i\n",
-	       IDE_CS);
-
-	swarm_ide_resource.start = offset;
-	swarm_ide_resource.end = offset + size - 1;
-	if (request_resource(&iomem_resource, &swarm_ide_resource)) {
-		printk(KERN_ERR DRV_NAME
-		       ": can't request I/O memory resource\n");
-		return -EBUSY;
-	}
-
-	base = ioremap(offset, size);
-
-	memset(&hw, 0, sizeof(hw));
-	for (i = 0; i <= 7; i++)
-		hw.io_ports_array[i] =
-				(unsigned long)(base + ((0x1f0 + i) << 5));
-	hw.io_ports.ctl_addr =
-				(unsigned long)(base + (0x3f6 << 5));
-	hw.irq = K_INT_GB_IDE;
-	hw.chipset = ide_generic;
-
-	rc = ide_host_add(&swarm_port_info, hws, &host);
-	if (rc)
-		goto err;
-
-	dev_set_drvdata(dev, host);
-
-	return 0;
-err:
-	release_resource(&swarm_ide_resource);
-	iounmap(base);
-	return rc;
-}
-
-static struct device_driver swarm_ide_driver = {
-	.name	= swarm_ide_string,
-	.bus	= &platform_bus_type,
-	.probe	= swarm_ide_probe,
-};
-
-static void swarm_ide_platform_release(struct device *device)
-{
-	struct platform_device *pldev;
-
-	/* free device */
-	pldev = to_platform_device(device);
-	kfree(pldev);
-}
-
-static int __devinit swarm_ide_init_module(void)
-{
-	struct platform_device *pldev;
-	int err;
-
-	printk(KERN_INFO "SWARM IDE driver\n");
-
-	if (driver_register(&swarm_ide_driver)) {
-		printk(KERN_ERR "Driver registration failed\n");
-		err = -ENODEV;
-		goto out;
-	}
-
-        if (!(pldev = kzalloc(sizeof (*pldev), GFP_KERNEL))) {
-		err = -ENOMEM;
-		goto out_unregister_driver;
-	}
-
-	pldev->name		= swarm_ide_string;
-	pldev->id		= 0;
-	pldev->dev.release	= swarm_ide_platform_release;
-
-	if (platform_device_register(pldev)) {
-		err = -ENODEV;
-		goto out_free_pldev;
-	}
-
-        if (!pldev->dev.driver) {
-		/*
-		 * The driver was not bound to this device, there was
-                 * no hardware at this address. Unregister it, as the
-		 * release fuction will take care of freeing the
-		 * allocated structure
-		 */
-		platform_device_unregister (pldev);
-	}
-
-	swarm_ide_dev = pldev;
-
-	return 0;
-
-out_free_pldev:
-	kfree(pldev);
-
-out_unregister_driver:
-	driver_unregister(&swarm_ide_driver);
-out:
-	return err;
-}
-
-module_init(swarm_ide_init_module);
Index: linux-mips/drivers/ide/mips/Makefile
===================================================================
--- linux-mips.orig/drivers/ide/mips/Makefile
+++ linux-mips/drivers/ide/mips/Makefile
@@ -1,4 +1,3 @@
-obj-$(CONFIG_BLK_DEV_IDE_SWARM)		+= swarm.o
 obj-$(CONFIG_BLK_DEV_IDE_AU1XXX)	+= au1xxx-ide.o
 
 EXTRA_CFLAGS    := -Idrivers/ide

Re: [PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello.

Ralf Baechle wrote:

> The Swarm IDE driver uses a release method which is defined in the driver
> itself thus potentially oopsable.  The simple fix would be to just leak
> the device but this patch goes the full length and moves the entire
> handling of the platform device in the platform code and retains only
> the platform driver code in drivers/ide/mips/swarm.c.
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>   

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> Index: linux-mips/arch/mips/sibyte/swarm/platform.c
> ===================================================================
> --- /dev/null
> +++ linux-mips/arch/mips/sibyte/swarm/platform.c
> @@ -0,0 +1,81 @@
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/ata_platform.h>
> +
> +#include <asm/sibyte/board.h>
> +#include <asm/sibyte/sb1250_genbus.h>
> +#include <asm/sibyte/sb1250_regs.h>
> +
> +#define DRV_NAME	"pata-swarm"
>   

   Hm, what driver? This is just a platform code. :-)

> +
> +#define SWARM_IDE_SHIFT	5
> +#define SWARM_IDE_BASE	0x1f0
> +#define SWARM_IDE_CTRL	0x3f6
> +
> +static struct resource swarm_pata_resource[] = {
> +	{
> +		.name	= "Swarm GenBus IDE",
> +		.flags	= IORESOURCE_MEM,
> +	}, {
> +		.name	= "Swarm GenBus IDE",
> +		.flags	= IORESOURCE_MEM,
> +	}, {
> +		.name	= "Swarm GenBus IDE",
> +		.flags	= IORESOURCE_IRQ,
> +		.start	= K_INT_GB_IDE,
> +		.end	= K_INT_GB_IDE,
>   

   Giving the resources similar names is pointless. You can just leave 
the 'name' field uninitialized.

MBR, Sergei

Re: [PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

On Mon, Sep 29, 2008 at 01:51:30AM +0400, Sergei Shtylyov wrote:

>> +	{
>> +		.name	= "Swarm GenBus IDE",
>> +		.flags	= IORESOURCE_MEM,
>> +	}, {
>> +		.name	= "Swarm GenBus IDE",
>> +		.flags	= IORESOURCE_MEM,
>> +	}, {
>> +		.name	= "Swarm GenBus IDE",
>> +		.flags	= IORESOURCE_IRQ,
>> +		.start	= K_INT_GB_IDE,
>> +		.end	= K_INT_GB_IDE,
>>   
>
>   Giving the resources similar names is pointless. You can just leave  
> the 'name' field uninitialized.

I could - but I prefer things to have reasonable names in /proc/iomem or
wherever.  Unfortunately the IRQ resources name isn't yet being used to
for the interrupt but there's always something left to do :)

  Ralf

Re: [PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Sergei Shtylyov]

Hello, I wrote:

>> The Swarm IDE driver uses a release method which is defined in the 
>> driver
>> itself thus potentially oopsable.  The simple fix would be to just leak
>> the device but this patch goes the full length and moves the entire
>> handling of the platform device in the platform code and retains only
>> the platform driver code in drivers/ide/mips/swarm.c.
>>
>> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>>   
>
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

   Perhaps worth splitting into arch/mips/ and the follow-up 
drivers/ide/ patches though...

MBR, Sergei

Re: [PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Bartlomiej Zolnierkiewicz]

On Sunday 28 September 2008, Sergei Shtylyov wrote:
> Hello.
> 
> Ralf Baechle wrote:
> 
> > The Swarm IDE driver uses a release method which is defined in the driver
> > itself thus potentially oopsable.  The simple fix would be to just leak
> > the device but this patch goes the full length and moves the entire
> > handling of the platform device in the platform code and retains only
> > the platform driver code in drivers/ide/mips/swarm.c.
> >
> > Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

wow, v2 happened really quickly :)

> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied

[ I removed no longer needed BLK_DEV_IDE_SWARM from ide/Kconfig while at it ]

Re: [PATCH v2] IDE: Fix platform device registration in Swarm IDE driver

[Ralf Baechle]

On Fri, Oct 03, 2008 at 07:00:54PM +0200, Bartlomiej Zolnierkiewicz wrote:

> > > The Swarm IDE driver uses a release method which is defined in the driver
> > > itself thus potentially oopsable.  The simple fix would be to just leak
> > > the device but this patch goes the full length and moves the entire
> > > handling of the platform device in the platform code and retains only
> > > the platform driver code in drivers/ide/mips/swarm.c.
> > >
> > > Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> wow, v2 happened really quickly :)

It not just tidying up, it fixing a potencial crash so had to happen
quickly to (hopefully) make the release ...

> [ I removed no longer needed BLK_DEV_IDE_SWARM from ide/Kconfig while at it ]

Thanks!

  Ralf