Re: [PATCH 2/16] gdth: split out eisa probing
Christoph Hellwig wrote: On Wed, Oct 03, 2007 at 01:59:23PM -0400, Jeff Garzik wrote: Come on. The patches were posted for comments, and Rolf commented. Don't give him a hard time for a valid comment. Sorry, but these comments are utterly useless. It's not like we're doing anything related to dma mapping, but just moving some init code around. If we actually did a major change in how dma mapping is handled the comment would be apropinquate and a switchover should happen as part of the patch series. A comment noting a useful improvement is always helpful, even if it cannot be addressed immediately. Please don't push reviewers away. That severely demotivates other reviewers, when they see such treatment. We talked about this at the Kernel Summit. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/16] gdth: split out eisa probing
On Wed, Oct 03, 2007 at 01:59:23PM -0400, Jeff Garzik wrote: > Come on. The patches were posted for comments, and Rolf commented. > Don't give him a hard time for a valid comment. Sorry, but these comments are utterly useless. It's not like we're doing anything related to dma mapping, but just moving some init code around. If we actually did a major change in how dma mapping is handled the comment would be apropinquate and a switchover should happen as part of the patch series. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/16] gdth: split out eisa probing
Christoph Hellwig wrote: On Wed, Oct 03, 2007 at 07:32:24PM +0200, Rolf Eike Beer wrote: It's not about passing NULL there. We need to do that. But we should call it with NULL as argument and not with ha->pdev to make this obvious. Feel free to send your cleanup patches once we've finished doing the important heavy lifting on gdth. Come on. The patches were posted for comments, and Rolf commented. Don't give him a hard time for a valid comment. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/16] gdth: split out eisa probing
On Wed, Oct 03, 2007 at 07:32:24PM +0200, Rolf Eike Beer wrote: > It's not about passing NULL there. We need to do that. But we should call it > with NULL as argument and not with ha->pdev to make this obvious. Feel free to send your cleanup patches once we've finished doing the important heavy lifting on gdth. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/16] gdth: split out eisa probing
Christoph Hellwig wrote: > On Tue, Oct 02, 2007 at 07:20:38PM +0200, Rolf Eike Beer wrote: > > Same thing as in ISA patch: this is not PCI and we should not hide what > > we're doing so dma_alloc_consistent(NULL, ...) is better IMHO. > > passing NULL is the documented way to deal with ISA/EISA devices with > the old pci dma api. Any change to these APIs should be a different > patch, and there's about 100 more important things in gdth right now. It's not about passing NULL there. We need to do that. But we should call it with NULL as argument and not with ha->pdev to make this obvious. Eike signature.asc Description: This is a digitally signed message part.
Re: [PATCH 2/16] gdth: split out eisa probing
On Tue, Oct 02, 2007 at 07:20:38PM +0200, Rolf Eike Beer wrote: > Same thing as in ISA patch: this is not PCI and we should not hide what we're > doing so dma_alloc_consistent(NULL, ...) is better IMHO. passing NULL is the documented way to deal with ISA/EISA devices with the old pci dma api. Any change to these APIs should be a different patch, and there's about 100 more important things in gdth right now. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/16] gdth: split out eisa probing
Boaz Harrosh wrote: > Split eisa probing into it's own helper, and do proper error unwinding. > Protect EISA probind by the proper CONFIG_EISA symbol. > > Signed-off-by: Christoph Hellwig <[EMAIL PROTECTED]> > @@ -5694,6 +5582,148 @@ static int gdth_isa_probe_one(struct > scsi_host_template *shtp, ulong32 isa_bios) } > #endif /* CONFIG_ISA */ > > +#ifdef CONFIG_EISA > +static int gdth_eisa_probe_one(struct scsi_host_template *shtp, > + ushort eisa_slot) > +{ > + struct Scsi_Host *shp; > + gdth_ha_str *ha; > + dma_addr_t scratch_dma_handle = 0; > + int error, hanum, i; > + u8 b; > + > + if (!gdth_search_eisa(eisa_slot)) > + return -ENXIO; > + > + shp = scsi_register(shtp,sizeof(gdth_ext_str)); > + if (!shp) > + return -ENOMEM; > + ha = HADATA(shp); > + > + error = -ENODEV; > + if (!gdth_init_eisa(eisa_slot,ha)) > + goto out_host_put; > + > + /* controller found and initialized */ > + printk("Configuring GDT-EISA HA at Slot %d IRQ %u\n", > + eisa_slot >> 12, ha->irq); > + > + error = request_irq(ha->irq, gdth_interrupt, IRQF_DISABLED, "gdth", ha); > + if (error) { > + printk("GDT-EISA: Unable to allocate IRQ\n"); > + goto out_host_put; > + } > + > + shp->unchecked_isa_dma = 0; > + shp->irq = ha->irq; > + shp->dma_channel = 0xff; > + hanum = gdth_ctr_count; > + gdth_ctr_tab[gdth_ctr_count++] = shp; > + gdth_ctr_vtab[gdth_ctr_vcount++] = shp; > + > + NUMDATA(shp)->hanum = (ushort)hanum; > + NUMDATA(shp)->busnum= 0; > + TRACE2(("EISA detect Bus 0: hanum %d\n", > + NUMDATA(shp)->hanum)); > + > + ha->pccb = CMDDATA(shp); > + ha->ccb_phys = 0L; > + > + error = -ENOMEM; > + > + ha->pdev = NULL; > + ha->pscratch = pci_alloc_consistent(ha->pdev, GDTH_SCRATCH, > + &scratch_dma_handle); > + if (!ha->pscratch) > + goto out_free_irq; > + ha->scratch_phys = scratch_dma_handle; > + > + ha->pmsg = pci_alloc_consistent(ha->pdev, sizeof(gdth_msg_str), > + &scratch_dma_handle); > + if (!ha->pmsg) > + goto out_free_pscratch; > + ha->msg_phys = scratch_dma_handle; > + > +#ifdef INT_COAL > + ha->coal_stat = pci_alloc_consistent(ha->pdev, > + sizeof(gdth_coal_status) * MAXOFFSETS, > + &scratch_dma_handle); > + if (!ha->coal_stat) > + goto out_free_pmsg; > + ha->coal_stat_phys = scratch_dma_handle; > +#endif > + > + ha->ccb_phys = pci_map_single(ha->pdev,ha->pccb, > + sizeof(gdth_cmd_str), PCI_DMA_BIDIRECTIONAL); > + if (!ha->ccb_phys) > + goto out_free_coal_stat; Same thing as in ISA patch: this is not PCI and we should not hide what we're doing so dma_alloc_consistent(NULL, ...) is better IMHO. Eike signature.asc Description: This is a digitally signed message part.
[PATCH 2/16] gdth: split out eisa probing
Split eisa probing into it's own helper, and do proper error unwinding. Protect EISA probind by the proper CONFIG_EISA symbol. Signed-off-by: Christoph Hellwig <[EMAIL PROTECTED]> --- drivers/scsi/gdth.c | 270 --- 1 files changed, 150 insertions(+), 120 deletions(-) diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c index 9f82c01..5240acd 100644 --- a/drivers/scsi/gdth.c +++ b/drivers/scsi/gdth.c @@ -479,6 +479,9 @@ static void gdth_scsi_done(struct scsi_cmnd *scp); #ifdef CONFIG_ISA static int gdth_isa_probe_one(struct scsi_host_template *, ulong32); #endif +#ifdef CONFIG_EISA +static int gdth_eisa_probe_one(struct scsi_host_template *, ushort); +#endif #ifdef DEBUG_GDTH static unchar DebugState = DEBUG_GDTH; @@ -4335,129 +4338,14 @@ static int __init gdth_detect(Scsi_Host_Template *shtp) gdth_isa_probe_one(shtp, isa_bios); } #endif - -for (eisa_slot=0x1000; eisa_slot<=0x8000; eisa_slot+=0x1000) { -dma_addr_t scratch_dma_handle; -scratch_dma_handle = 0; - -if (gdth_ctr_count >= MAXHA) +#ifdef CONFIG_EISA +for (eisa_slot = 0x1000; eisa_slot <= 0x8000; eisa_slot += 0x1000) { +if (gdth_ctr_count >= MAXHA) break; -if (gdth_search_eisa(eisa_slot)) { /* controller found */ -shp = scsi_register(shtp,sizeof(gdth_ext_str)); -if (shp == NULL) -continue; - -ha = HADATA(shp); -if (!gdth_init_eisa(eisa_slot,ha)) { -scsi_unregister(shp); -continue; -} -/* controller found and initialized */ -printk("Configuring GDT-EISA HA at Slot %d IRQ %u\n", - eisa_slot>>12,ha->irq); - -if (request_irq(ha->irq,gdth_interrupt,IRQF_DISABLED,"gdth",ha)) { -printk("GDT-EISA: Unable to allocate IRQ\n"); -scsi_unregister(shp); -continue; -} -shp->unchecked_isa_dma = 0; -shp->irq = ha->irq; -shp->dma_channel = 0xff; -hanum = gdth_ctr_count; -gdth_ctr_tab[gdth_ctr_count++] = shp; -gdth_ctr_vtab[gdth_ctr_vcount++] = shp; - -NUMDATA(shp)->hanum = (ushort)hanum; -NUMDATA(shp)->busnum= 0; -TRACE2(("EISA detect Bus 0: hanum %d\n", -NUMDATA(shp)->hanum)); - -ha->pccb = CMDDATA(shp); -ha->ccb_phys = 0L; - -ha->pdev = NULL; -ha->pscratch = pci_alloc_consistent(ha->pdev, GDTH_SCRATCH, -&scratch_dma_handle); -ha->scratch_phys = scratch_dma_handle; -ha->pmsg = pci_alloc_consistent(ha->pdev, sizeof(gdth_msg_str), -&scratch_dma_handle); -ha->msg_phys = scratch_dma_handle; -#ifdef INT_COAL -ha->coal_stat = (gdth_coal_status *) -pci_alloc_consistent(ha->pdev, sizeof(gdth_coal_status) * - MAXOFFSETS, &scratch_dma_handle); -ha->coal_stat_phys = scratch_dma_handle; -#endif -ha->ccb_phys = -pci_map_single(ha->pdev,ha->pccb, - sizeof(gdth_cmd_str),PCI_DMA_BIDIRECTIONAL); -ha->scratch_busy = FALSE; -ha->req_first = NULL; -ha->tid_cnt = MAX_HDRIVES; -if (max_ids > 0 && max_ids < ha->tid_cnt) -ha->tid_cnt = max_ids; -for (i=0; icmd_tab[i].cmnd = UNUSED_CMND; -ha->scan_mode = rescan ? 0x10 : 0; - -if (ha->pscratch == NULL || ha->pmsg == NULL || -!gdth_search_drives(hanum)) { -printk("GDT-EISA: Error during device scan\n"); ---gdth_ctr_count; ---gdth_ctr_vcount; -#ifdef INT_COAL -if (ha->coal_stat) -pci_free_consistent(ha->pdev, sizeof(gdth_coal_status) * -MAXOFFSETS, ha->coal_stat, -ha->coal_stat_phys); -#endif -if (ha->pscratch) -pci_free_consistent(ha->pdev, GDTH_SCRATCH, -ha->pscratch, ha->scratch_phys); -if (ha->pmsg) -pci_free_consistent(ha->pdev, sizeof(gdth_msg_str), -ha->pmsg, ha->msg_phys); -if (ha->ccb_phys) -pci_unmap_single(ha->pdev,ha->ccb_phys, - si