LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jiri Slaby <jirislaby@gmail.com>
To: Jeff Garzik <jeff@garzik.org>
Cc: linux-scsi@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] gdth: convert to PCI hotplug API
Date: Wed, 13 Feb 2008 10:21:38 +0100	[thread overview]
Message-ID: <47B2B6A2.9020206@gmail.com> (raw)
In-Reply-To: <20080212234928.GA13892@havoc.gtf.org>

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);
>  

  reply	other threads:[~2008-02-13  9:22 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-12 23:49 Jeff Garzik
2008-02-13  9:21 ` Jiri Slaby [this message]
2008-02-14 21:59   ` Jeff Garzik
2008-02-13 11:07 ` Boaz Harrosh
2008-02-14 21:59   ` Jeff Garzik
2008-02-15 13:50   ` Jan Engelhardt
2008-02-14 21:33 ` James Bottomley
2008-02-15 15:44   ` Jeff Garzik
2008-02-15 15:54     ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47B2B6A2.9020206@gmail.com \
    --to=jirislaby@gmail.com \
    --cc=jeff@garzik.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --subject='Re: [PATCH] gdth: convert to PCI hotplug API' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).