| Message ID | 1474743545-4731-4-git-send-email-linux@rainbow-software.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev) > +{ > + struct Scsi_Host *sh = NULL; > + > + sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev], > + irq[ndev], card[ndev]); > + if (!sh && base[ndev]) > + printk(KERN_WARNING "Card not found at address 0x%03x\n", > + base[ndev]); > + if (!sh) > + return 0; > > + dev_set_drvdata(pdev, sh); Any reason not to move the dev_set_drvdata into generic_NCR5380_hw_init? That would also allow to properly propagate the error down to the caller, which would be useful for the PNP case. Otherwise this look great to me. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 24 Sep 2016, Ondrej Zary wrote: > Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c > Use pnp_driver and isa_driver to manage cards. > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > --- > drivers/scsi/g_NCR5380.c | 310 +++++++++++++++++++++++++--------------------- > drivers/scsi/g_NCR5380.h | 8 -- > 2 files changed, 169 insertions(+), 149 deletions(-) > > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c > index 5162de6..6cf6b00 100644 > --- a/drivers/scsi/g_NCR5380.c > +++ b/drivers/scsi/g_NCR5380.c > @@ -30,24 +30,25 @@ > #include "NCR5380.h" > #include <linux/init.h> > #include <linux/ioport.h> > -#include <linux/isapnp.h> > +#include <linux/isa.h> > +#include <linux/pnp.h> > #include <linux/interrupt.h> > > -static int ncr_irq; > -static int ncr_dma; > -static int ncr_addr; > -static int ncr_5380; > -static int ncr_53c400; > -static int ncr_53c400a; > -static int dtc_3181e; > -static int hp_c2502; > - > -static struct card { > - NCR5380_map_type NCR5380_map_name; > - int irq; > - int dma; > - int board; /* Use NCR53c400, Ricoh, etc. extensions ? */ > -} card; > +#define MAX_CARDS 8 > + > +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 }; > +module_param_array(irq, int, NULL, 0); > +MODULE_PARM_DESC(irq, "IRQ number(s)"); > + > +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 }; > +module_param_array(base, int, NULL, 0); > +MODULE_PARM_DESC(base, "base address(es)"); > + > +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 }; > +module_param_array(card, int, NULL, 0); > +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)"); > + > +MODULE_LICENSE("GPL"); If you are going to change all of the module parameters and break every user's setup, at least explain what changed and why in the commit log and update the documentation in the kernel tree. If you really want to help those users (most of whom gain nothing from this patch) you could also update the documentation elsewhere: http://www.sane-project.org/man/sane-scsi.5.html http://www.xsane.org/rauch-domain/sane-umax/sane-umax-config-doc.html http://www.meier-geinitz.de/sane/mustek-backend/ I'm not in favour of this patch, but Christoph liked it so I'll leave it at that. > > #ifndef SCSI_G_NCR5380_MEM > /* > @@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[]) > } > #endif > > -/** > - * generic_NCR5380_detect - look for NCR5380 controllers > - * @tpnt: the scsi template > - * > - * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E > - * and DTC436(ISAPnP) controllers. > - * > - * Locks: none > - */ > - > -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > +static struct Scsi_Host *generic_NCR5380_hw_init( > + struct scsi_host_template *tpnt, struct device *pdev, > + int base, int irq, int board) This allocates and returns a Scsi_Host so the name is misleading. But the name isn't the only problem; the real problem has already been pointed out by Christoph: the caller needs the error code to be returned not the Scsi_Host pointer. > { > unsigned int *ports; > u8 *magic = NULL; > @@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > struct Scsi_Host *instance; > struct NCR5380_hostdata *hostdata; > #ifdef SCSI_G_NCR5380_MEM > - unsigned long base; > void __iomem *iomem; > resource_size_t iomem_size; > #endif > > - if (ncr_irq) > - card.irq = ncr_irq; > - if (ncr_dma) > - card.dma = ncr_dma; > - if (ncr_addr) > - card.NCR5380_map_name = (NCR5380_map_type) ncr_addr; > - if (ncr_5380) > - card.board = BOARD_NCR5380; > - else if (ncr_53c400) > - card.board = BOARD_NCR53C400; > - else if (ncr_53c400a) > - card.board = BOARD_NCR53C400A; > - else if (dtc_3181e) > - card.board = BOARD_DTC3181E; > - else if (hp_c2502) > - card.board = BOARD_HP_C2502; > -#ifndef SCSI_G_NCR5380_MEM > - if (isapnp_present()) { > - struct pnp_dev *dev = NULL; > - while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) { > - if (pnp_device_attach(dev) < 0) > - continue; > - if (pnp_activate_dev(dev) < 0) { > - printk(KERN_ERR "dtc436e probe: activate failed\n"); > - pnp_device_detach(dev); > - continue; > - } > - if (!pnp_port_valid(dev, 0)) { > - printk(KERN_ERR "dtc436e probe: no valid port\n"); > - pnp_device_detach(dev); > - continue; > - } > - if (pnp_irq_valid(dev, 0)) > - card.irq = pnp_irq(dev, 0); > - else > - card.irq = NO_IRQ; > - if (pnp_dma_valid(dev, 0)) > - card.dma = pnp_dma(dev, 0); > - else > - card.dma = DMA_NONE; > - card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0); > - card.board = BOARD_DTC3181E; > - break; > - } > - } > -#endif > - > - if (!(card.NCR5380_map_name)) > - return 0; > - > ports = NULL; > flags = 0; > - switch (card.board) { > + switch (board) { > case BOARD_NCR5380: > flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP; > break; > @@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > magic_configure(-1, 0, magic); > > region_size = 16; > - > - if (card.NCR5380_map_name != PORT_AUTO) > + if (base) > for (i = 0; ports[i]; i++) { > - if (!request_region(ports[i], region_size, "ncr53c80")) > - continue; > - if (card.NCR5380_map_name == ports[i]) > + if (base == ports[i]) { /* index found */ > + if (!request_region(ports[i], > + region_size, > + "ncr53c80")) > + return NULL; > break; > - release_region(ports[i], region_size); > - } else > + } > + } > + else > for (i = 0; ports[i]; i++) { > - if (!request_region(ports[i], region_size, "ncr53c80")) > + if (!request_region(ports[i], region_size, > + "ncr53c80")) > continue; > if (inb(ports[i]) == 0xff) > break; > @@ -212,38 +157,36 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > magic_configure(i, 0, magic); /* no IRQ yet */ > outb(0xc0, ports[i] + 9); > if (inb(ports[i] + 9) != 0x80) > - return 0; > - card.NCR5380_map_name = ports[i]; > + goto out_release; > + base = ports[i]; > port_idx = i; > } else > - return 0; > + return NULL; > } > else > { > - /* Not a 53C400A style setup - just grab */ > + /* NCR5380 - no configuration, just grab */ > region_size = 8; > - if (!request_region(card.NCR5380_map_name, > - region_size, "ncr5380")) > - return 0; > + if (!base || !request_region(base, region_size, "ncr5380")) > + return NULL; > } > #else > - base = card.NCR5380_map_name; > iomem_size = NCR53C400_region_size; > if (!request_mem_region(base, iomem_size, "ncr5380")) > - return 0; > + return NULL; > iomem = ioremap(base, iomem_size); > if (!iomem) { > release_mem_region(base, iomem_size); > - return 0; > + return NULL; > } > #endif > - instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata)); > + instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata)); > if (instance == NULL) > goto out_release; > hostdata = shost_priv(instance); > > #ifndef SCSI_G_NCR5380_MEM > - instance->io_port = card.NCR5380_map_name; > + instance->io_port = base; > instance->n_io_port = region_size; > hostdata->io_width = 1; /* 8-bit PDMA by default */ > > @@ -251,7 +194,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > * On NCR53C400 boards, NCR5380 registers are mapped 8 past > * the base address. > */ > - switch (card.board) { > + switch (board) { > case BOARD_NCR53C400: > instance->io_port += 8; > hostdata->c400_ctl_status = 0; > @@ -269,10 +212,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > break; > } > #else > - instance->base = card.NCR5380_map_name; > + instance->base = base; > hostdata->iomem = iomem; > hostdata->iomem_size = iomem_size; > - switch (card.board) { > + switch (board) { > case BOARD_NCR53C400: > hostdata->c400_ctl_status = 0x100; > hostdata->c400_blk_cnt = 0x101; > @@ -289,7 +232,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP)) > goto out_unregister; > > - switch (card.board) { > + switch (board) { > case BOARD_NCR53C400: > case BOARD_DTC3181E: > case BOARD_NCR53C400A: > @@ -299,8 +242,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > > NCR5380_maybe_reset_bus(instance); > > - if (card.irq != IRQ_AUTO) > - instance->irq = card.irq; > + if (irq != IRQ_AUTO) > + instance->irq = irq; > else > instance->irq = NCR5380_probe_irq(instance, 0xffff); > > @@ -311,7 +254,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > if (instance->irq != NO_IRQ) { > #ifndef SCSI_G_NCR5380_MEM > /* set IRQ for HP C2502 */ > - if (card.board == BOARD_HP_C2502) > + if (board == BOARD_HP_C2502) > magic_configure(port_idx, instance->irq, magic); > #endif > if (request_irq(instance->irq, generic_NCR5380_intr, > @@ -326,31 +269,28 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no); > } > > - return 1; Instead of this: > + if (scsi_add_host(instance, pdev)) { > + free_irq(irq, instance); > + goto out_unregister; > + } > + scsi_scan_host(instance); > + return instance; > please use the following (note the missing NCR5380_exit() call): + if (scsi_add_host(instance, pdev)) + goto out_free_irq; + scsi_scan_host(instance); + return(instance); + +out_free_irq: + if (instance->irq != NO_IRQ) + free_irq(instance->irq, instance); + NCR5380_exit(instance); > out_unregister: > - scsi_unregister(instance); > + scsi_host_put(instance); > out_release: > #ifndef SCSI_G_NCR5380_MEM > - release_region(card.NCR5380_map_name, region_size); > + release_region(base, region_size); > #else > iounmap(iomem); > release_mem_region(base, iomem_size); > #endif > - return 0; > + return NULL; > } > > -/** > - * generic_NCR5380_release_resources - free resources > - * @instance: host adapter to clean up > - * > - * Free the generic interface resources from this adapter. > - * > - * Locks: none > - */ > - > -static int generic_NCR5380_release_resources(struct Scsi_Host *instance) > +static void generic_NCR5380_release_resources(struct Scsi_Host *instance) > { > + scsi_remove_host(instance); > if (instance->irq != NO_IRQ) > free_irq(instance->irq, instance); > NCR5380_exit(instance); > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance) > release_mem_region(instance->base, hostdata->iomem_size); > } > #endif > - return 0; > + scsi_host_put(instance); The sequence of operations here should be the same as the error path above. > } > > /** > @@ -554,10 +494,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance, > #include "NCR5380.c" > > static struct scsi_host_template driver_template = { > + .module = THIS_MODULE, > .proc_name = DRV_MODULE_NAME, > .name = "Generic NCR5380/NCR53C400 SCSI", > - .detect = generic_NCR5380_detect, > - .release = generic_NCR5380_release_resources, > .info = generic_NCR5380_info, > .queuecommand = generic_NCR5380_queue_command, > .eh_abort_handler = generic_NCR5380_abort, > @@ -571,26 +510,115 @@ static struct scsi_host_template driver_template = { > .max_sectors = 128, > }; > > -#include "scsi_module.c" > > -module_param(ncr_irq, int, 0); > -module_param(ncr_dma, int, 0); > -module_param(ncr_addr, int, 0); > -module_param(ncr_5380, int, 0); > -module_param(ncr_53c400, int, 0); > -module_param(ncr_53c400a, int, 0); > -module_param(dtc_3181e, int, 0); > -module_param(hp_c2502, int, 0); > -MODULE_LICENSE("GPL"); > +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev) > +{ > + struct Scsi_Host *sh = NULL; > + > + sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev], > + irq[ndev], card[ndev]); > + if (!sh && base[ndev]) > + printk(KERN_WARNING "Card not found at address 0x%03x\n", > + base[ndev]); > + if (!sh) > + return 0; For clarity, please write that as, if (!instance) { if (base[ndev]) printk(...) return 0; } > > -#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE) > -static struct isapnp_device_id id_table[] = { > - { > - ISAPNP_ANY_ID, ISAPNP_ANY_ID, > - ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), > - 0}, > - {0} > + dev_set_drvdata(pdev, sh); > + return 1; > +} > + > +static int generic_NCR5380_isa_remove(struct device *pdev, > + unsigned int ndev) > +{ > + generic_NCR5380_release_resources(dev_get_drvdata(pdev)); > + dev_set_drvdata(pdev, NULL); > + return 0; > +} > + > +static struct isa_driver generic_NCR5380_isa_driver = { > + .match = generic_NCR5380_isa_match, > + .remove = generic_NCR5380_isa_remove, > + .driver = { > + .name = DRV_MODULE_NAME > + }, > +}; > +static int isa_registered; > + > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > +static struct pnp_device_id generic_NCR5380_pnp_ids[] = { > + { .id = "DTC436e", .driver_data = BOARD_DTC3181E }, > + { .id = "" } > }; > +MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids); > + > +static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev, > + const struct pnp_device_id *id) > +{ > + struct Scsi_Host *sh; > + int base, irq; > > -MODULE_DEVICE_TABLE(isapnp, id_table); > + if (pnp_activate_dev(pdev) < 0) > + return -EBUSY; > + > + base = pnp_port_start(pdev, 0); > + irq = pnp_irq(pdev, 0); > + > + sh = generic_NCR5380_hw_init(&driver_template, &pdev->dev, base, irq, > + id->driver_data); > + if (!sh) > + return -ENODEV; > + > + pnp_set_drvdata(pdev, sh); > + return 0; > +} > + > +static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev) > +{ > + generic_NCR5380_release_resources(pnp_get_drvdata(pdev)); > + pnp_set_drvdata(pdev, NULL); > +} > + > +static struct pnp_driver generic_NCR5380_pnp_driver = { > + .name = DRV_MODULE_NAME, > + .id_table = generic_NCR5380_pnp_ids, > + .probe = generic_NCR5380_pnp_probe, > + .remove = generic_NCR5380_pnp_remove, > +}; > +static int pnp_registered; > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */ > + > +static int __init generic_NCR5380_init(void) > +{ > + int ret = 0; > + > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > + ret = pnp_register_driver(&generic_NCR5380_pnp_driver); > + if (!ret) > + pnp_registered = 1; > #endif > + ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); > + if (!ret) > + isa_registered = 1; > + > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > + if (pnp_registered) > + ret = 0; > +#endif > + if (isa_registered) > + ret = 0; > + > + return ret; > +} > + > +static void __exit generic_NCR5380_exit(void) > +{ > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > + if (pnp_registered) > + pnp_unregister_driver(&generic_NCR5380_pnp_driver); > +#endif > + if (isa_registered) > + isa_unregister_driver(&generic_NCR5380_isa_driver); > +} > + I find that hard to follow. This should be equivalent and simpler: static int __init generic_NCR5380_init(void) { isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver); return pnp_ret ? isa_ret : 0; #endif return isa_ret; } static void __exit generic_NCR5380_exit(void) { if (!isa_ret) isa_unregister_driver(&generic_NCR5380_isa_driver); #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) if (!pnp_ret) pnp_unregister_driver(&generic_NCR5380_pnp_driver); #endif }
On Monday 26 September 2016, Finn Thain wrote: [...] > Instead of this: > > + if (scsi_add_host(instance, pdev)) { > > + free_irq(irq, instance); > > + goto out_unregister; > > + } > > + scsi_scan_host(instance); > > + return instance; > > please use the following (note the missing NCR5380_exit() call): > > + if (scsi_add_host(instance, pdev)) > + goto out_free_irq; > + scsi_scan_host(instance); > + return(instance); > + > +out_free_irq: > + if (instance->irq != NO_IRQ) > + free_irq(instance->irq, instance); > + NCR5380_exit(instance); > > > out_unregister: > > - scsi_unregister(instance); > > + scsi_host_put(instance); > > out_release: > > #ifndef SCSI_G_NCR5380_MEM > > - release_region(card.NCR5380_map_name, region_size); > > + release_region(base, region_size); > > #else > > iounmap(iomem); > > release_mem_region(base, iomem_size); > > #endif > > - return 0; > > + return NULL; > > } > > > > -/** > > - * generic_NCR5380_release_resources - free resources > > - * @instance: host adapter to clean up > > - * > > - * Free the generic interface resources from this adapter. > > - * > > - * Locks: none > > - */ > > - > > -static int generic_NCR5380_release_resources(struct Scsi_Host *instance) > > +static void generic_NCR5380_release_resources(struct Scsi_Host > > *instance) { > > + scsi_remove_host(instance); > > if (instance->irq != NO_IRQ) > > free_irq(instance->irq, instance); > > NCR5380_exit(instance); > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct > > Scsi_Host *instance) release_mem_region(instance->base, > > hostdata->iomem_size); > > } > > #endif > > - return 0; > > + scsi_host_put(instance); > > The sequence of operations here should be the same as the error path > above. I put scsi_host_put() call at the end because the release_mem_region code (in the MMIO case) dereferences the hostdata pointer. I don't think it's safe to do after scsi_host_put(). [...] > > +static int pnp_registered; > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */ > > + > > +static int __init generic_NCR5380_init(void) > > +{ > > + int ret = 0; > > + > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > + ret = pnp_register_driver(&generic_NCR5380_pnp_driver); > > + if (!ret) > > + pnp_registered = 1; > > #endif > > + ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); > > + if (!ret) > > + isa_registered = 1; > > + > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > + if (pnp_registered) > > + ret = 0; > > +#endif > > + if (isa_registered) > > + ret = 0; > > + > > + return ret; > > +} > > + > > +static void __exit generic_NCR5380_exit(void) > > +{ > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > + if (pnp_registered) > > + pnp_unregister_driver(&generic_NCR5380_pnp_driver); > > +#endif > > + if (isa_registered) > > + isa_unregister_driver(&generic_NCR5380_isa_driver); > > +} > > + > > I find that hard to follow. This should be equivalent and simpler: > > static int __init generic_NCR5380_init(void) > { > isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver); > return pnp_ret ? isa_ret : 0; > #endif > return isa_ret; > } > > static void __exit generic_NCR5380_exit(void) > { > if (!isa_ret) > isa_unregister_driver(&generic_NCR5380_isa_driver); > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > if (!pnp_ret) > pnp_unregister_driver(&generic_NCR5380_pnp_driver); > #endif > } Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - especially this: > return pnp_ret ? isa_ret : 0; Also looking at the _exit function, meaning of isa_ret and pnp_ret is not obvious there. Maybe we should have a module_isa_pnp_driver() macro.
On Tue, 27 Sep 2016, Ondrej Zary wrote: > On Monday 26 September 2016, Finn Thain wrote: > > [...] > > > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct > > > Scsi_Host *instance) release_mem_region(instance->base, > > > hostdata->iomem_size); > > > } > > > #endif > > > - return 0; > > > + scsi_host_put(instance); > > > > The sequence of operations here should be the same as the error path > > above. > > I put scsi_host_put() call at the end because the release_mem_region > code (in the MMIO case) dereferences the hostdata pointer. I don't think > it's safe to do after scsi_host_put(). It's funny you say that, I already fixed a few of those use-after-free bugs in the other 5380 drivers. To avoid the bug, you'd need to use a temporary variable, like the fixes I did elsewhere. Don't worry about changing it, this part of the driver gets revisited in my existing patch queue anyway. > > [...] > > > > +static int pnp_registered; > > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */ > > > + > > > +static int __init generic_NCR5380_init(void) > > > +{ > > > + int ret = 0; > > > + > > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > > + ret = pnp_register_driver(&generic_NCR5380_pnp_driver); > > > + if (!ret) > > > + pnp_registered = 1; > > > #endif > > > + ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); > > > + if (!ret) > > > + isa_registered = 1; > > > + > > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > > + if (pnp_registered) > > > + ret = 0; > > > +#endif > > > + if (isa_registered) > > > + ret = 0; > > > + > > > + return ret; > > > +} > > > + > > > +static void __exit generic_NCR5380_exit(void) > > > +{ > > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > > + if (pnp_registered) > > > + pnp_unregister_driver(&generic_NCR5380_pnp_driver); > > > +#endif > > > + if (isa_registered) > > > + isa_unregister_driver(&generic_NCR5380_isa_driver); > > > +} > > > + > > > > I find that hard to follow. This should be equivalent and simpler: > > > > static int __init generic_NCR5380_init(void) > > { > > isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); > > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver); > > return pnp_ret ? isa_ret : 0; > > #endif > > return isa_ret; > > } > > > > static void __exit generic_NCR5380_exit(void) > > { > > if (!isa_ret) > > isa_unregister_driver(&generic_NCR5380_isa_driver); > > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) > > if (!pnp_ret) > > pnp_unregister_driver(&generic_NCR5380_pnp_driver); > > #endif > > } > > Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - > especially this: > > return pnp_ret ? isa_ret : 0; The problem with that line is that pnp_ret is always discarded, same as in your version. I think my version makes that clear, which is what matters to me. > > Also looking at the _exit function, meaning of isa_ret and pnp_ret is not > obvious there. > > Maybe we should have a module_isa_pnp_driver() macro. > > FWIW I would hope that the hypothetical definition of this `module_isa_pnp_driver' macro would have the same properties as my version: shorter, uses no temporaries and uses one less #ifdef, and is generally easier for me to read.
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c index 5162de6..6cf6b00 100644 --- a/drivers/scsi/g_NCR5380.c +++ b/drivers/scsi/g_NCR5380.c @@ -30,24 +30,25 @@ #include "NCR5380.h" #include <linux/init.h> #include <linux/ioport.h> -#include <linux/isapnp.h> +#include <linux/isa.h> +#include <linux/pnp.h> #include <linux/interrupt.h> -static int ncr_irq; -static int ncr_dma; -static int ncr_addr; -static int ncr_5380; -static int ncr_53c400; -static int ncr_53c400a; -static int dtc_3181e; -static int hp_c2502; - -static struct card { - NCR5380_map_type NCR5380_map_name; - int irq; - int dma; - int board; /* Use NCR53c400, Ricoh, etc. extensions ? */ -} card; +#define MAX_CARDS 8 + +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 }; +module_param_array(irq, int, NULL, 0); +MODULE_PARM_DESC(irq, "IRQ number(s)"); + +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 }; +module_param_array(base, int, NULL, 0); +MODULE_PARM_DESC(base, "base address(es)"); + +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 }; +module_param_array(card, int, NULL, 0); +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)"); + +MODULE_LICENSE("GPL"); #ifndef SCSI_G_NCR5380_MEM /* @@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[]) } #endif -/** - * generic_NCR5380_detect - look for NCR5380 controllers - * @tpnt: the scsi template - * - * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E - * and DTC436(ISAPnP) controllers. - * - * Locks: none - */ - -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) +static struct Scsi_Host *generic_NCR5380_hw_init( + struct scsi_host_template *tpnt, struct device *pdev, + int base, int irq, int board) { unsigned int *ports; u8 *magic = NULL; @@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) struct Scsi_Host *instance; struct NCR5380_hostdata *hostdata; #ifdef SCSI_G_NCR5380_MEM - unsigned long base; void __iomem *iomem; resource_size_t iomem_size; #endif - if (ncr_irq) - card.irq = ncr_irq; - if (ncr_dma) - card.dma = ncr_dma; - if (ncr_addr) - card.NCR5380_map_name = (NCR5380_map_type) ncr_addr; - if (ncr_5380) - card.board = BOARD_NCR5380; - else if (ncr_53c400) - card.board = BOARD_NCR53C400; - else if (ncr_53c400a) - card.board = BOARD_NCR53C400A; - else if (dtc_3181e) - card.board = BOARD_DTC3181E; - else if (hp_c2502) - card.board = BOARD_HP_C2502; -#ifndef SCSI_G_NCR5380_MEM - if (isapnp_present()) { - struct pnp_dev *dev = NULL; - while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) { - if (pnp_device_attach(dev) < 0) - continue; - if (pnp_activate_dev(dev) < 0) { - printk(KERN_ERR "dtc436e probe: activate failed\n"); - pnp_device_detach(dev); - continue; - } - if (!pnp_port_valid(dev, 0)) { - printk(KERN_ERR "dtc436e probe: no valid port\n"); - pnp_device_detach(dev); - continue; - } - if (pnp_irq_valid(dev, 0)) - card.irq = pnp_irq(dev, 0); - else - card.irq = NO_IRQ; - if (pnp_dma_valid(dev, 0)) - card.dma = pnp_dma(dev, 0); - else - card.dma = DMA_NONE; - card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0); - card.board = BOARD_DTC3181E; - break; - } - } -#endif - - if (!(card.NCR5380_map_name)) - return 0; - ports = NULL; flags = 0; - switch (card.board) { + switch (board) { case BOARD_NCR5380: flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP; break; @@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) magic_configure(-1, 0, magic); region_size = 16; - - if (card.NCR5380_map_name != PORT_AUTO) + if (base) for (i = 0; ports[i]; i++) { - if (!request_region(ports[i], region_size, "ncr53c80")) - continue; - if (card.NCR5380_map_name == ports[i]) + if (base == ports[i]) { /* index found */ + if (!request_region(ports[i], + region_size, + "ncr53c80")) + return NULL; break; - release_region(ports[i], region_size); - } else + } + } + else for (i = 0; ports[i]; i++) { - if (!request_region(ports[i], region_size, "ncr53c80")) + if (!request_region(ports[i], region_size, + "ncr53c80")) continue; if (inb(ports[i]) == 0xff) break; @@ -212,38 +157,36 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) magic_configure(i, 0, magic); /* no IRQ yet */ outb(0xc0, ports[i] + 9); if (inb(ports[i] + 9) != 0x80) - return 0; - card.NCR5380_map_name = ports[i]; + goto out_release; + base = ports[i]; port_idx = i; } else - return 0; + return NULL; } else { - /* Not a 53C400A style setup - just grab */ + /* NCR5380 - no configuration, just grab */ region_size = 8; - if (!request_region(card.NCR5380_map_name, - region_size, "ncr5380")) - return 0; + if (!base || !request_region(base, region_size, "ncr5380")) + return NULL; } #else - base = card.NCR5380_map_name; iomem_size = NCR53C400_region_size; if (!request_mem_region(base, iomem_size, "ncr5380")) - return 0; + return NULL; iomem = ioremap(base, iomem_size); if (!iomem) { release_mem_region(base, iomem_size); - return 0; + return NULL; } #endif - instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata)); + instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata)); if (instance == NULL) goto out_release; hostdata = shost_priv(instance); #ifndef SCSI_G_NCR5380_MEM - instance->io_port = card.NCR5380_map_name; + instance->io_port = base; instance->n_io_port = region_size; hostdata->io_width = 1; /* 8-bit PDMA by default */ @@ -251,7 +194,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) * On NCR53C400 boards, NCR5380 registers are mapped 8 past * the base address. */ - switch (card.board) { + switch (board) { case BOARD_NCR53C400: instance->io_port += 8; hostdata->c400_ctl_status = 0; @@ -269,10 +212,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) break; } #else - instance->base = card.NCR5380_map_name; + instance->base = base; hostdata->iomem = iomem; hostdata->iomem_size = iomem_size; - switch (card.board) { + switch (board) { case BOARD_NCR53C400: hostdata->c400_ctl_status = 0x100; hostdata->c400_blk_cnt = 0x101; @@ -289,7 +232,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP)) goto out_unregister; - switch (card.board) { + switch (board) { case BOARD_NCR53C400: case BOARD_DTC3181E: case BOARD_NCR53C400A: @@ -299,8 +242,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) NCR5380_maybe_reset_bus(instance); - if (card.irq != IRQ_AUTO) - instance->irq = card.irq; + if (irq != IRQ_AUTO) + instance->irq = irq; else instance->irq = NCR5380_probe_irq(instance, 0xffff); @@ -311,7 +254,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) if (instance->irq != NO_IRQ) { #ifndef SCSI_G_NCR5380_MEM /* set IRQ for HP C2502 */ - if (card.board == BOARD_HP_C2502) + if (board == BOARD_HP_C2502) magic_configure(port_idx, instance->irq, magic); #endif if (request_irq(instance->irq, generic_NCR5380_intr, @@ -326,31 +269,28 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no); } - return 1; + if (scsi_add_host(instance, pdev)) { + free_irq(irq, instance); + goto out_unregister; + } + scsi_scan_host(instance); + return instance; out_unregister: - scsi_unregister(instance); + scsi_host_put(instance); out_release: #ifndef SCSI_G_NCR5380_MEM - release_region(card.NCR5380_map_name, region_size); + release_region(base, region_size); #else iounmap(iomem); release_mem_region(base, iomem_size); #endif - return 0; + return NULL; } -/** - * generic_NCR5380_release_resources - free resources - * @instance: host adapter to clean up - * - * Free the generic interface resources from this adapter. - * - * Locks: none - */ - -static int generic_NCR5380_release_resources(struct Scsi_Host *instance) +static void generic_NCR5380_release_resources(struct Scsi_Host *instance) { + scsi_remove_host(instance); if (instance->irq != NO_IRQ) free_irq(instance->irq, instance); NCR5380_exit(instance); @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance) release_mem_region(instance->base, hostdata->iomem_size); } #endif - return 0; + scsi_host_put(instance); } /** @@ -554,10 +494,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance, #include "NCR5380.c" static struct scsi_host_template driver_template = { + .module = THIS_MODULE, .proc_name = DRV_MODULE_NAME, .name = "Generic NCR5380/NCR53C400 SCSI", - .detect = generic_NCR5380_detect, - .release = generic_NCR5380_release_resources, .info = generic_NCR5380_info, .queuecommand = generic_NCR5380_queue_command, .eh_abort_handler = generic_NCR5380_abort, @@ -571,26 +510,115 @@ static struct scsi_host_template driver_template = { .max_sectors = 128, }; -#include "scsi_module.c" -module_param(ncr_irq, int, 0); -module_param(ncr_dma, int, 0); -module_param(ncr_addr, int, 0); -module_param(ncr_5380, int, 0); -module_param(ncr_53c400, int, 0); -module_param(ncr_53c400a, int, 0); -module_param(dtc_3181e, int, 0); -module_param(hp_c2502, int, 0); -MODULE_LICENSE("GPL"); +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev) +{ + struct Scsi_Host *sh = NULL; + + sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev], + irq[ndev], card[ndev]); + if (!sh && base[ndev]) + printk(KERN_WARNING "Card not found at address 0x%03x\n", + base[ndev]); + if (!sh) + return 0; -#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE) -static struct isapnp_device_id id_table[] = { - { - ISAPNP_ANY_ID, ISAPNP_ANY_ID, - ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), - 0}, - {0} + dev_set_drvdata(pdev, sh); + return 1; +} + +static int generic_NCR5380_isa_remove(struct device *pdev, + unsigned int ndev) +{ + generic_NCR5380_release_resources(dev_get_drvdata(pdev)); + dev_set_drvdata(pdev, NULL); + return 0; +} + +static struct isa_driver generic_NCR5380_isa_driver = { + .match = generic_NCR5380_isa_match, + .remove = generic_NCR5380_isa_remove, + .driver = { + .name = DRV_MODULE_NAME + }, +}; +static int isa_registered; + +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) +static struct pnp_device_id generic_NCR5380_pnp_ids[] = { + { .id = "DTC436e", .driver_data = BOARD_DTC3181E }, + { .id = "" } }; +MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids); + +static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev, + const struct pnp_device_id *id) +{ + struct Scsi_Host *sh; + int base, irq; -MODULE_DEVICE_TABLE(isapnp, id_table); + if (pnp_activate_dev(pdev) < 0) + return -EBUSY; + + base = pnp_port_start(pdev, 0); + irq = pnp_irq(pdev, 0); + + sh = generic_NCR5380_hw_init(&driver_template, &pdev->dev, base, irq, + id->driver_data); + if (!sh) + return -ENODEV; + + pnp_set_drvdata(pdev, sh); + return 0; +} + +static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev) +{ + generic_NCR5380_release_resources(pnp_get_drvdata(pdev)); + pnp_set_drvdata(pdev, NULL); +} + +static struct pnp_driver generic_NCR5380_pnp_driver = { + .name = DRV_MODULE_NAME, + .id_table = generic_NCR5380_pnp_ids, + .probe = generic_NCR5380_pnp_probe, + .remove = generic_NCR5380_pnp_remove, +}; +static int pnp_registered; +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */ + +static int __init generic_NCR5380_init(void) +{ + int ret = 0; + +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) + ret = pnp_register_driver(&generic_NCR5380_pnp_driver); + if (!ret) + pnp_registered = 1; #endif + ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS); + if (!ret) + isa_registered = 1; + +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) + if (pnp_registered) + ret = 0; +#endif + if (isa_registered) + ret = 0; + + return ret; +} + +static void __exit generic_NCR5380_exit(void) +{ +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) + if (pnp_registered) + pnp_unregister_driver(&generic_NCR5380_pnp_driver); +#endif + if (isa_registered) + isa_unregister_driver(&generic_NCR5380_isa_driver); +} + +module_init(generic_NCR5380_init); +module_exit(generic_NCR5380_exit); diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h index 5951774..b175b92 100644 --- a/drivers/scsi/g_NCR5380.h +++ b/drivers/scsi/g_NCR5380.h @@ -14,15 +14,9 @@ #ifndef GENERIC_NCR5380_H #define GENERIC_NCR5380_H -#define __STRVAL(x) #x -#define STRVAL(x) __STRVAL(x) - #ifndef SCSI_G_NCR5380_MEM #define DRV_MODULE_NAME "g_NCR5380" -#define NCR5380_map_type int -#define NCR5380_map_name port - #define NCR5380_read(reg) \ inb(instance->io_port + (reg)) #define NCR5380_write(reg, value) \ @@ -38,8 +32,6 @@ /* therefore SCSI_G_NCR5380_MEM */ #define DRV_MODULE_NAME "g_NCR5380_mmio" -#define NCR5380_map_type unsigned long -#define NCR5380_map_name base #define NCR53C400_mem_base 0x3880 #define NCR53C400_host_buffer 0x3900 #define NCR53C400_region_size 0x3a00
Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c Use pnp_driver and isa_driver to manage cards. Signed-off-by: Ondrej Zary <linux@rainbow-software.org> --- drivers/scsi/g_NCR5380.c | 310 +++++++++++++++++++++++++--------------------- drivers/scsi/g_NCR5380.h | 8 -- 2 files changed, 169 insertions(+), 149 deletions(-)