[PATCH] gdth: convert to PCI hotplug API

[PATCH] gdth: convert to PCI hotplug API

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/gdth.c |  143 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 86 insertions(+), 57 deletions(-)

06196f50915da97bb897495863f9f084d785c1e4
diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index c825239..1b53e92 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -595,85 +595,107 @@ static int __init gdth_search_isa(ulong32 bios_adr)
 #endif /* CONFIG_ISA */
 
 #ifdef CONFIG_PCI
-static void gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
-                            ushort vendor, ushort dev);
+static gdth_pci_str gdth_pcistr[MAXHA];
+static int gdth_pci_cnt;
+static bool gdth_pci_registered;
 
-static int __init gdth_search_pci(gdth_pci_str *pcistr)
+static bool __init gdth_search_vortex(ushort device)
 {
-    ushort device, cnt;
-    
-    TRACE(("gdth_search_pci()\n"));
-
-    cnt = 0;
-    for (device = 0; device <= PCI_DEVICE_ID_VORTEX_GDT6555; ++device)
-        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
-    for (device = PCI_DEVICE_ID_VORTEX_GDT6x17RP; 
-         device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP; ++device)
-        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
-                    PCI_DEVICE_ID_VORTEX_GDTNEWRX);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
-                    PCI_DEVICE_ID_VORTEX_GDTNEWRX2);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
-                    PCI_DEVICE_ID_INTEL_SRC);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
-                    PCI_DEVICE_ID_INTEL_SRC_XSCALE);
-    return cnt;
+	if (device <= PCI_DEVICE_ID_VORTEX_GDT6555)
+		return true;
+	if (device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP &&
+	    device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP)
+		return true;
+	if (device == PCI_DEVICE_ID_VORTEX_GDTNEWRX ||
+	    device == PCI_DEVICE_ID_VORTEX_GDTNEWRX2)
+		return true;
+	return false;
 }
 
+static int gdth_pci_init_one(struct pci_dev *pdev,
+			     const struct pci_device_id *ent);
+static void gdth_pci_remove_one(struct pci_dev *pdev);
+static void gdth_remove_one(gdth_ha_str *ha);
+
 /* Vortex only makes RAID controllers.
  * We do not really want to specify all 550 ids here, so wildcard match.
  */
-static struct pci_device_id gdthtable[] __maybe_unused = {
-    {PCI_VENDOR_ID_VORTEX,PCI_ANY_ID,PCI_ANY_ID, PCI_ANY_ID},
-    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC,PCI_ANY_ID,PCI_ANY_ID}, 
-    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC_XSCALE,PCI_ANY_ID,PCI_ANY_ID}, 
-    {0}
+static struct pci_device_id gdthtable[] __devinitdata = {
+	{ PCI_VDEVICE(VORTEX, PCI_ANY_ID) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC_XSCALE) },
+	{ }	/* terminate list */
+};
+MODULE_DEVICE_TABLE(pci, gdthtable);
+
+static struct pci_driver gdth_pci_driver = {
+	.name		= "gdth",
+	.id_table	= gdthtable,
+	.probe		= gdth_pci_init_one,
+	.remove		= gdth_pci_remove_one,
 };
-MODULE_DEVICE_TABLE(pci,gdthtable);
 
-static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
-                                   ushort vendor, ushort device)
+static void gdth_pci_remove_one(struct pci_dev *pdev)
+{
+	gdth_ha_str *ha = pci_get_drvdata(pdev);
+
+	pci_set_drvdata(pdev, NULL);
+
+	list_del(&ha->list);
+	gdth_remove_one(ha);
+
+	pci_disable_device(pdev);
+}
+
+static int __devinit gdth_pci_init_one(struct pci_dev *pdev,
+				       const struct pci_device_id *ent)
 {
-    ulong base0, base1, base2;
-    struct pci_dev *pdev;
+	ushort vendor = pdev->vendor;
+	ushort device = pdev->device;
+	ulong base0, base1, base2;
+	int rc;
     
-    TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
-          *cnt, vendor, device));
+	TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
+	       gdth_pci_cnt, vendor, device));
+
+	if (vendor == PCI_VENDOR_ID_VORTEX && !gdth_search_vortex(device))
+		return -ENODEV;
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
+	if (gdth_pci_cnt >= MAXHA)
+		return -EBUSY;
 
-    pdev = NULL;
-    while ((pdev = pci_find_device(vendor, device, pdev)) 
-           != NULL) {
-        if (pci_enable_device(pdev))
-            continue;
-        if (*cnt >= MAXHA)
-            return;
         /* GDT PCI controller found, resources are already in pdev */
-        pcistr[*cnt].pdev = pdev;
-        pcistr[*cnt].irq = pdev->irq;
+	gdth_pcistr[gdth_pci_cnt].pdev = pdev;
+	gdth_pcistr[gdth_pci_cnt].irq = pdev->irq;
         base0 = pci_resource_flags(pdev, 0);
         base1 = pci_resource_flags(pdev, 1);
         base2 = pci_resource_flags(pdev, 2);
         if (device <= PCI_DEVICE_ID_VORTEX_GDT6000B ||   /* GDT6000/B */
             device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP) {  /* MPR */
             if (!(base0 & IORESOURCE_MEM)) 
-                continue;
-            pcistr[*cnt].dpmem = pci_resource_start(pdev, 0);
+		return -ENODEV;
+	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 0);
         } else {                                  /* GDT6110, GDT6120, .. */
             if (!(base0 & IORESOURCE_MEM) ||
                 !(base2 & IORESOURCE_MEM) ||
                 !(base1 & IORESOURCE_IO)) 
-                continue;
-            pcistr[*cnt].dpmem = pci_resource_start(pdev, 2);
-            pcistr[*cnt].io_mm = pci_resource_start(pdev, 0);
-            pcistr[*cnt].io    = pci_resource_start(pdev, 1);
+		return -ENODEV;
+	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 2);
+	    gdth_pcistr[gdth_pci_cnt].io_mm = pci_resource_start(pdev, 0);
+	    gdth_pcistr[gdth_pci_cnt].io    = pci_resource_start(pdev, 1);
         }
         TRACE2(("Controller found at %d/%d, irq %d, dpmem 0x%lx\n",
-                pcistr[*cnt].pdev->bus->number,
-		PCI_SLOT(pcistr[*cnt].pdev->devfn),
-                pcistr[*cnt].irq, pcistr[*cnt].dpmem));
-        (*cnt)++;
-    }       
+		gdth_pcistr[gdth_pci_cnt].pdev->bus->number,
+		PCI_SLOT(gdth_pcistr[gdth_pci_cnt].pdev->devfn),
+		gdth_pcistr[gdth_pci_cnt].irq,
+		gdth_pcistr[gdth_pci_cnt].dpmem));
+	gdth_pci_cnt++;
+
+	return 0;
 }   
 
 static void __init gdth_sort_pci(gdth_pci_str *pcistr, int cnt)
@@ -5100,6 +5122,7 @@ static int __init gdth_pci_probe_one(gdth_pci_str *pcistr, int ctr)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+	pci_set_drvdata(ha->pdev, ha);
 	return 0;
 
  out_free_coal_stat:
@@ -5198,15 +5221,16 @@ static int __init gdth_init(void)
 
 #ifdef CONFIG_PCI
 	/* scanning for PCI controllers */
-	{
-		gdth_pci_str pcistr[MAXHA];
+	if (pci_register_driver(&gdth_pci_driver) == 0) {
 		int cnt,ctr;
+		gdth_pci_str *pcistr = gdth_pcistr;
 
-		cnt = gdth_search_pci(pcistr);
+		cnt = gdth_pci_cnt;
 		printk("GDT-HA: Found %d PCI Storage RAID Controllers\n", cnt);
 		gdth_sort_pci(pcistr,cnt);
 		for (ctr = 0; ctr < cnt; ++ctr)
 			gdth_pci_probe_one(pcistr, ctr);
+		gdth_pci_registered = true;
 	}
 #endif /* CONFIG_PCI */
 
@@ -5234,6 +5258,11 @@ static void __exit gdth_exit(void)
 {
 	gdth_ha_str *ha;
 
+#ifdef CONFIG_PCI
+	if (gdth_pci_registered)
+		pci_unregister_driver(&gdth_pci_driver);
+#endif
+
 	list_for_each_entry(ha, &gdth_instances, list)
 		gdth_remove_one(ha);
 

Re: [PATCH] gdth: convert to PCI hotplug API

[Jiri Slaby]

On 02/13/2008 12:49 AM, Jeff Garzik wrote:
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/gdth.c |  143 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 86 insertions(+), 57 deletions(-)
> 
> 06196f50915da97bb897495863f9f084d785c1e4
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index c825239..1b53e92 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -595,85 +595,107 @@ static int __init gdth_search_isa(ulong32 bios_adr)
>  #endif /* CONFIG_ISA */
>  
>  #ifdef CONFIG_PCI
> -static void gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
> -                            ushort vendor, ushort dev);
> +static gdth_pci_str gdth_pcistr[MAXHA];
> +static int gdth_pci_cnt;
> +static bool gdth_pci_registered;
>  
> -static int __init gdth_search_pci(gdth_pci_str *pcistr)
> +static bool __init gdth_search_vortex(ushort device)
>  {
> -    ushort device, cnt;
> -    
> -    TRACE(("gdth_search_pci()\n"));
> -
> -    cnt = 0;
> -    for (device = 0; device <= PCI_DEVICE_ID_VORTEX_GDT6555; ++device)
> -        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
> -    for (device = PCI_DEVICE_ID_VORTEX_GDT6x17RP; 
> -         device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP; ++device)
> -        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
> -    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
> -                    PCI_DEVICE_ID_VORTEX_GDTNEWRX);
> -    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
> -                    PCI_DEVICE_ID_VORTEX_GDTNEWRX2);
> -    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
> -                    PCI_DEVICE_ID_INTEL_SRC);
> -    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
> -                    PCI_DEVICE_ID_INTEL_SRC_XSCALE);
> -    return cnt;
> +	if (device <= PCI_DEVICE_ID_VORTEX_GDT6555)
> +		return true;
> +	if (device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP &&
> +	    device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP)
> +		return true;
> +	if (device == PCI_DEVICE_ID_VORTEX_GDTNEWRX ||
> +	    device == PCI_DEVICE_ID_VORTEX_GDTNEWRX2)
> +		return true;
> +	return false;
>  }
>  
> +static int gdth_pci_init_one(struct pci_dev *pdev,
> +			     const struct pci_device_id *ent);
> +static void gdth_pci_remove_one(struct pci_dev *pdev);
> +static void gdth_remove_one(gdth_ha_str *ha);
> +
>  /* Vortex only makes RAID controllers.
>   * We do not really want to specify all 550 ids here, so wildcard match.
>   */
> -static struct pci_device_id gdthtable[] __maybe_unused = {
> -    {PCI_VENDOR_ID_VORTEX,PCI_ANY_ID,PCI_ANY_ID, PCI_ANY_ID},
> -    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC,PCI_ANY_ID,PCI_ANY_ID}, 
> -    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC_XSCALE,PCI_ANY_ID,PCI_ANY_ID}, 
> -    {0}
> +static struct pci_device_id gdthtable[] __devinitdata = {
> +	{ PCI_VDEVICE(VORTEX, PCI_ANY_ID) },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC) },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC_XSCALE) },
> +	{ }	/* terminate list */
> +};
> +MODULE_DEVICE_TABLE(pci, gdthtable);
> +
> +static struct pci_driver gdth_pci_driver = {
> +	.name		= "gdth",
> +	.id_table	= gdthtable,
> +	.probe		= gdth_pci_init_one,
> +	.remove		= gdth_pci_remove_one,

__devexit_p()

>  };
> -MODULE_DEVICE_TABLE(pci,gdthtable);
>  
> -static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
> -                                   ushort vendor, ushort device)
> +static void gdth_pci_remove_one(struct pci_dev *pdev)
> +{
> +	gdth_ha_str *ha = pci_get_drvdata(pdev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +
> +	list_del(&ha->list);

If you remove the card before the sort and gdth_pci_probe_one function, this 
will oops. Also the adds/removes to the list are racy and may break the 
gdth_instances list.

> +	gdth_remove_one(ha);

And so this...

> +	pci_disable_device(pdev);
> +}
> +
> +static int __devinit gdth_pci_init_one(struct pci_dev *pdev,
> +				       const struct pci_device_id *ent)
>  {
> -    ulong base0, base1, base2;
> -    struct pci_dev *pdev;
> +	ushort vendor = pdev->vendor;
> +	ushort device = pdev->device;
> +	ulong base0, base1, base2;
> +	int rc;
>      
> -    TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
> -          *cnt, vendor, device));
> +	TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
> +	       gdth_pci_cnt, vendor, device));
> +
> +	if (vendor == PCI_VENDOR_ID_VORTEX && !gdth_search_vortex(device))
> +		return -ENODEV;
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +
> +	if (gdth_pci_cnt >= MAXHA)
> +		return -EBUSY;
> -    pdev = NULL;
> -    while ((pdev = pci_find_device(vendor, device, pdev)) 
> -           != NULL) {
> -        if (pci_enable_device(pdev))
> -            continue;
> -        if (*cnt >= MAXHA)
> -            return;
>          /* GDT PCI controller found, resources are already in pdev */
> -        pcistr[*cnt].pdev = pdev;
> -        pcistr[*cnt].irq = pdev->irq;
> +	gdth_pcistr[gdth_pci_cnt].pdev = pdev;
> +	gdth_pcistr[gdth_pci_cnt].irq = pdev->irq;
>          base0 = pci_resource_flags(pdev, 0);
>          base1 = pci_resource_flags(pdev, 1);
>          base2 = pci_resource_flags(pdev, 2);
>          if (device <= PCI_DEVICE_ID_VORTEX_GDT6000B ||   /* GDT6000/B */
>              device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP) {  /* MPR */
>              if (!(base0 & IORESOURCE_MEM)) 
> -                continue;
> -            pcistr[*cnt].dpmem = pci_resource_start(pdev, 0);
> +		return -ENODEV;
> +	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 0);
>          } else {                                  /* GDT6110, GDT6120, .. */
>              if (!(base0 & IORESOURCE_MEM) ||
>                  !(base2 & IORESOURCE_MEM) ||
>                  !(base1 & IORESOURCE_IO)) 
> -                continue;
> -            pcistr[*cnt].dpmem = pci_resource_start(pdev, 2);
> -            pcistr[*cnt].io_mm = pci_resource_start(pdev, 0);
> -            pcistr[*cnt].io    = pci_resource_start(pdev, 1);
> +		return -ENODEV;
> +	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 2);
> +	    gdth_pcistr[gdth_pci_cnt].io_mm = pci_resource_start(pdev, 0);
> +	    gdth_pcistr[gdth_pci_cnt].io    = pci_resource_start(pdev, 1);
>          }
>          TRACE2(("Controller found at %d/%d, irq %d, dpmem 0x%lx\n",
> -                pcistr[*cnt].pdev->bus->number,
> -		PCI_SLOT(pcistr[*cnt].pdev->devfn),
> -                pcistr[*cnt].irq, pcistr[*cnt].dpmem));
> -        (*cnt)++;
> -    }       
> +		gdth_pcistr[gdth_pci_cnt].pdev->bus->number,
> +		PCI_SLOT(gdth_pcistr[gdth_pci_cnt].pdev->devfn),
> +		gdth_pcistr[gdth_pci_cnt].irq,
> +		gdth_pcistr[gdth_pci_cnt].dpmem));
> +	gdth_pci_cnt++;
> +
> +	return 0;
>  }   
>  
>  static void __init gdth_sort_pci(gdth_pci_str *pcistr, int cnt)
> @@ -5100,6 +5122,7 @@ static int __init gdth_pci_probe_one(gdth_pci_str *pcistr, int ctr)
>  	if (error)
>  		goto out_free_coal_stat;
>  	list_add_tail(&ha->list, &gdth_instances);
> +	pci_set_drvdata(ha->pdev, ha);
>  	return 0;
>  
>   out_free_coal_stat:
> @@ -5198,15 +5221,16 @@ static int __init gdth_init(void)
>  
>  #ifdef CONFIG_PCI
>  	/* scanning for PCI controllers */
> -	{
> -		gdth_pci_str pcistr[MAXHA];
> +	if (pci_register_driver(&gdth_pci_driver) == 0) {
>  		int cnt,ctr;
> +		gdth_pci_str *pcistr = gdth_pcistr;
>  
> -		cnt = gdth_search_pci(pcistr);
> +		cnt = gdth_pci_cnt;
>  		printk("GDT-HA: Found %d PCI Storage RAID Controllers\n", cnt);
>  		gdth_sort_pci(pcistr,cnt);
>  		for (ctr = 0; ctr < cnt; ++ctr)
>  			gdth_pci_probe_one(pcistr, ctr);

This is not true hotplug... this should go into the probe function, right? But 
it can't due to the sort? Then what this change brings to the driver? Isn't 
conversion to pci_get_device better in this case (As who will gaurantee that the 
register returns after calling probes for all devices in the future)?

> +		gdth_pci_registered = true;
>  	}
>  #endif /* CONFIG_PCI */
>  
> @@ -5234,6 +5258,11 @@ static void __exit gdth_exit(void)
>  {
>  	gdth_ha_str *ha;
>  
> +#ifdef CONFIG_PCI
> +	if (gdth_pci_registered)
> +		pci_unregister_driver(&gdth_pci_driver);
> +#endif
> +
>  	list_for_each_entry(ha, &gdth_instances, list)
>  		gdth_remove_one(ha);
>  

Re: [PATCH] gdth: convert to PCI hotplug API

[Boaz Harrosh]

On Wed, Feb 13 2008 at 1:49 +0200, Jeff Garzik <jeff@garzik.org> wrote:
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/gdth.c |  143 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 86 insertions(+), 57 deletions(-)
> 
<snip>
below is the same exact patch rebased on my last two bugfixes.
(Already in scsi-rc-fixes)

do you intend this to be pushed into 2.6.25-rcx or this is already
for 2.6.26? Should we put this in -mm tree for testing?

Boaz

THIS IS NOT YET TESTED

---
From: Jeff Garzik <jgarzik@redhat.com>
Date: Wed, 13 Feb 2008 13:01:16 +0200
Subject: [PATCH] gdth: convert to PCI hotplug API

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas>
---
 drivers/scsi/gdth.c |  143 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 86 insertions(+), 57 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 103280e..107d2c7 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -596,85 +596,107 @@ static int __init gdth_search_isa(ulong32 bios_adr)
 #endif /* CONFIG_ISA */
 
 #ifdef CONFIG_PCI
-static void gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
-                            ushort vendor, ushort dev);
+static gdth_pci_str gdth_pcistr[MAXHA];
+static int gdth_pci_cnt;
+static bool gdth_pci_registered;
 
-static int __init gdth_search_pci(gdth_pci_str *pcistr)
+static bool __init gdth_search_vortex(ushort device)
 {
-    ushort device, cnt;
-    
-    TRACE(("gdth_search_pci()\n"));
-
-    cnt = 0;
-    for (device = 0; device <= PCI_DEVICE_ID_VORTEX_GDT6555; ++device)
-        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
-    for (device = PCI_DEVICE_ID_VORTEX_GDT6x17RP; 
-         device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP; ++device)
-        gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, device);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
-                    PCI_DEVICE_ID_VORTEX_GDTNEWRX);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_VORTEX, 
-                    PCI_DEVICE_ID_VORTEX_GDTNEWRX2);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
-                    PCI_DEVICE_ID_INTEL_SRC);
-    gdth_search_dev(pcistr, &cnt, PCI_VENDOR_ID_INTEL,
-                    PCI_DEVICE_ID_INTEL_SRC_XSCALE);
-    return cnt;
+	if (device <= PCI_DEVICE_ID_VORTEX_GDT6555)
+		return true;
+	if (device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP &&
+	    device <= PCI_DEVICE_ID_VORTEX_GDTMAXRP)
+		return true;
+	if (device == PCI_DEVICE_ID_VORTEX_GDTNEWRX ||
+	    device == PCI_DEVICE_ID_VORTEX_GDTNEWRX2)
+		return true;
+	return false;
 }
 
+static int gdth_pci_init_one(struct pci_dev *pdev,
+			     const struct pci_device_id *ent);
+static void gdth_pci_remove_one(struct pci_dev *pdev);
+static void gdth_remove_one(gdth_ha_str *ha);
+
 /* Vortex only makes RAID controllers.
  * We do not really want to specify all 550 ids here, so wildcard match.
  */
-static struct pci_device_id gdthtable[] __maybe_unused = {
-    {PCI_VENDOR_ID_VORTEX,PCI_ANY_ID,PCI_ANY_ID, PCI_ANY_ID},
-    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC,PCI_ANY_ID,PCI_ANY_ID}, 
-    {PCI_VENDOR_ID_INTEL,PCI_DEVICE_ID_INTEL_SRC_XSCALE,PCI_ANY_ID,PCI_ANY_ID}, 
-    {0}
+static struct pci_device_id gdthtable[] __devinitdata = {
+	{ PCI_VDEVICE(VORTEX, PCI_ANY_ID) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC_XSCALE) },
+	{ }	/* terminate list */
+};
+MODULE_DEVICE_TABLE(pci, gdthtable);
+
+static struct pci_driver gdth_pci_driver = {
+	.name		= "gdth",
+	.id_table	= gdthtable,
+	.probe		= gdth_pci_init_one,
+	.remove		= gdth_pci_remove_one,
 };
-MODULE_DEVICE_TABLE(pci,gdthtable);
 
-static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
-                                   ushort vendor, ushort device)
+static void gdth_pci_remove_one(struct pci_dev *pdev)
+{
+	gdth_ha_str *ha = pci_get_drvdata(pdev);
+
+	pci_set_drvdata(pdev, NULL);
+
+	list_del(&ha->list);
+	gdth_remove_one(ha);
+
+	pci_disable_device(pdev);
+}
+
+static int __devinit gdth_pci_init_one(struct pci_dev *pdev,
+				       const struct pci_device_id *ent)
 {
-    ulong base0, base1, base2;
-    struct pci_dev *pdev;
+	ushort vendor = pdev->vendor;
+	ushort device = pdev->device;
+	ulong base0, base1, base2;
+	int rc;
     
-    TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
-          *cnt, vendor, device));
+	TRACE(("gdth_search_dev() cnt %d vendor %x device %x\n",
+	       gdth_pci_cnt, vendor, device));
+
+	if (vendor == PCI_VENDOR_ID_VORTEX && !gdth_search_vortex(device))
+		return -ENODEV;
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
+	if (gdth_pci_cnt >= MAXHA)
+		return -EBUSY;
 
-    pdev = NULL;
-    while ((pdev = pci_find_device(vendor, device, pdev)) 
-           != NULL) {
-        if (pci_enable_device(pdev))
-            continue;
-        if (*cnt >= MAXHA)
-            return;
         /* GDT PCI controller found, resources are already in pdev */
-        pcistr[*cnt].pdev = pdev;
-        pcistr[*cnt].irq = pdev->irq;
+	gdth_pcistr[gdth_pci_cnt].pdev = pdev;
+	gdth_pcistr[gdth_pci_cnt].irq = pdev->irq;
         base0 = pci_resource_flags(pdev, 0);
         base1 = pci_resource_flags(pdev, 1);
         base2 = pci_resource_flags(pdev, 2);
         if (device <= PCI_DEVICE_ID_VORTEX_GDT6000B ||   /* GDT6000/B */
             device >= PCI_DEVICE_ID_VORTEX_GDT6x17RP) {  /* MPR */
             if (!(base0 & IORESOURCE_MEM)) 
-                continue;
-            pcistr[*cnt].dpmem = pci_resource_start(pdev, 0);
+		return -ENODEV;
+	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 0);
         } else {                                  /* GDT6110, GDT6120, .. */
             if (!(base0 & IORESOURCE_MEM) ||
                 !(base2 & IORESOURCE_MEM) ||
                 !(base1 & IORESOURCE_IO)) 
-                continue;
-            pcistr[*cnt].dpmem = pci_resource_start(pdev, 2);
-            pcistr[*cnt].io_mm = pci_resource_start(pdev, 0);
-            pcistr[*cnt].io    = pci_resource_start(pdev, 1);
+		return -ENODEV;
+	    gdth_pcistr[gdth_pci_cnt].dpmem = pci_resource_start(pdev, 2);
+	    gdth_pcistr[gdth_pci_cnt].io_mm = pci_resource_start(pdev, 0);
+	    gdth_pcistr[gdth_pci_cnt].io    = pci_resource_start(pdev, 1);
         }
         TRACE2(("Controller found at %d/%d, irq %d, dpmem 0x%lx\n",
-                pcistr[*cnt].pdev->bus->number,
-		PCI_SLOT(pcistr[*cnt].pdev->devfn),
-                pcistr[*cnt].irq, pcistr[*cnt].dpmem));
-        (*cnt)++;
-    }       
+		gdth_pcistr[gdth_pci_cnt].pdev->bus->number,
+		PCI_SLOT(gdth_pcistr[gdth_pci_cnt].pdev->devfn),
+		gdth_pcistr[gdth_pci_cnt].irq,
+		gdth_pcistr[gdth_pci_cnt].dpmem));
+	gdth_pci_cnt++;
+
+	return 0;
 }   
 
 static void __init gdth_sort_pci(gdth_pci_str *pcistr, int cnt)
@@ -5110,6 +5132,7 @@ static int __init gdth_pci_probe_one(gdth_pci_str *pcistr, int ctr)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+	pci_set_drvdata(ha->pdev, ha);
 
 	scsi_scan_host(shp);
 
@@ -5209,15 +5232,16 @@ static int __init gdth_init(void)
 
 #ifdef CONFIG_PCI
 	/* scanning for PCI controllers */
-	{
-		gdth_pci_str pcistr[MAXHA];
+	if (pci_register_driver(&gdth_pci_driver) == 0) {
 		int cnt,ctr;
+		gdth_pci_str *pcistr = gdth_pcistr;
 
-		cnt = gdth_search_pci(pcistr);
+		cnt = gdth_pci_cnt;
 		printk("GDT-HA: Found %d PCI Storage RAID Controllers\n", cnt);
 		gdth_sort_pci(pcistr,cnt);
 		for (ctr = 0; ctr < cnt; ++ctr)
 			gdth_pci_probe_one(pcistr, ctr);
+		gdth_pci_registered = true;
 	}
 #endif /* CONFIG_PCI */
 
@@ -5252,6 +5276,11 @@ static void __exit gdth_exit(void)
 	del_timer_sync(&gdth_timer);
 #endif
 
+#ifdef CONFIG_PCI
+	if (gdth_pci_registered)
+		pci_unregister_driver(&gdth_pci_driver);
+#endif
+
 	list_for_each_entry(ha, &gdth_instances, list)
 		gdth_remove_one(ha);
 }
-- 
1.5.3.3

Re: [PATCH] gdth: convert to PCI hotplug API

[James Bottomley]

On Tue, 2008-02-12 at 18:49 -0500, Jeff Garzik wrote:
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

Nice work, thanks.  This is PCI only I take it.  ISA and EISA look like
they'll be a tad more troublesome, so PCI only is fine.

> ---
>  drivers/scsi/gdth.c |  143 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 86 insertions(+), 57 deletions(-)
> 
> 06196f50915da97bb897495863f9f084d785c1e4
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index c825239..1b53e92 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -595,85 +595,107 @@ static int __init gdth_search_isa(ulong32 bios_adr)
>  #endif /* CONFIG_ISA */
>  
>  #ifdef CONFIG_PCI
> -static void gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
> -                            ushort vendor, ushort dev);
> +static gdth_pci_str gdth_pcistr[MAXHA];
> +static int gdth_pci_cnt;
> +static bool gdth_pci_registered;

Could we get rid of these static arrays and MAXHA entirely?  It should
be possible just to bung the parameters in pci_str into gdth_ha_str and
dump the arrays.

Thanks,

James

Re: [PATCH] gdth: convert to PCI hotplug API

[Jeff Garzik]

Boaz Harrosh wrote:
> do you intend this to be pushed into 2.6.25-rcx or this is already
> for 2.6.26? Should we put this in -mm tree for testing?


Not intended for 2.6.25.  I just wanted to get this "in process" 
somewhere, and keep this issue moving.  I would definitely prefer to 
have this tested before it goes to Linus.

Its a long term goal to kill pci_find_device(), and this conversion was 
a side effect of that.

	Jeff

Re: [PATCH] gdth: convert to PCI hotplug API

[Jeff Garzik]

Comments noted for my next round of revisions (its a low priority, so 
definitely not this week).  Good spotting, thanks!

	Jeff

Re: [PATCH] gdth: convert to PCI hotplug API

[Jan Engelhardt]

On Feb 13 2008 13:07, Boaz Harrosh wrote:
>+static struct pci_device_id gdthtable[] __devinitdata = {
>+	{ PCI_VDEVICE(VORTEX, PCI_ANY_ID) },
>+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC) },
>+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SRC_XSCALE) },
>+	{ }	/* terminate list */
>+};

As usual, +const and +__devinitconst :-)

Re: [PATCH] gdth: convert to PCI hotplug API

[Jeff Garzik]

James Bottomley wrote:
>> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
>> index c825239..1b53e92 100644
>> --- a/drivers/scsi/gdth.c
>> +++ b/drivers/scsi/gdth.c
>> @@ -595,85 +595,107 @@ static int __init gdth_search_isa(ulong32 bios_adr)
>>  #endif /* CONFIG_ISA */
>>  
>>  #ifdef CONFIG_PCI
>> -static void gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
>> -                            ushort vendor, ushort dev);
>> +static gdth_pci_str gdth_pcistr[MAXHA];
>> +static int gdth_pci_cnt;
>> +static bool gdth_pci_registered;
> 
> Could we get rid of these static arrays and MAXHA entirely?  It should
> be possible just to bung the parameters in pci_str into gdth_ha_str and
> dump the arrays.


I kept those array for one reason:  you need it to preserve the existing 
in-driver PCI device sort.

If we can eliminate the sorting, then the array can easily disappear.

I /think/ the sort can be eliminated now because we have pci=reverse, 
but I have not verified that guess.

	Jeff

Re: [PATCH] gdth: convert to PCI hotplug API

[Matthew Wilcox]

On Fri, Feb 15, 2008 at 10:44:52AM -0500, Jeff Garzik wrote:
> I kept those array for one reason:  you need it to preserve the existing 
> in-driver PCI device sort.

Just get rid of it.  I got rid of it for sym2 during 2.5 and very few
people have complained.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."